rackspace / gophercloud

A Go SDK for OpenStack. IN FEATURE FREEZE. See Issue #592
http://gophercloud.io
Other
456 stars 181 forks source link

Add support for Keystone Trust (v3) #580

Closed codevulture closed 8 years ago

codevulture commented 8 years ago

Adding Keystone Trust

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.03%) to 80.517% when pulling 8d6c44b84e5f1d7def88be78a66a14f2cead2bee on codevulture:keystone_api into adc206589ed49d18cecc9890ab93534704b04702 on rackspace:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.0005%) to 80.543% when pulling beebc842c43c883fb7dc7e59a0a8c5357be50c77 on codevulture:keystone_api into adc206589ed49d18cecc9890ab93534704b04702 on rackspace:master.

jrperritt commented 8 years ago

This looks like an Identity Extension and, as such, should go under openstack/identity/v3/extensions (similar to v2).

codevulture commented 8 years ago

Hi @jrperritt : It is an Identity v3 feature which is a part of that version which was not present in v2, so i think i could not be treated as extenstion but a v3 api feature itself ? Please correct me if i am wrong.

jrperritt commented 8 years ago

It is an Identity v3 feature which is a part of that version which was not present in v2

AFAICT, it is a feature that is not standard on all OpenStack clouds, which is why it should go in openstack/identity/v3/extensions (the extensions directory for v3 does not yet exist)

See here: http://developer.openstack.org/api-ref-identity-v3-ext.html#identity_v3_OS-TRUST-ext

codevulture commented 8 years ago

@jrperritt : Ohh i think there is some misunderstanding, the patch includes how to USE Trust Id when it is entered through Auth Options not to generate Trust Id. User needs to supply Trust id which is generated through above API, when entering credentials so that it could be used as a token.

codevulture commented 8 years ago

@jrperritt : Please review.

jrperritt commented 8 years ago

Ohh i think there is some misunderstanding

There seems to be; as I said, this is an extension. You've implemented this directly in the gophercloud, openstack, and tokens (v3) packages. It will not get merged like this.

the patch includes how to USE Trust Id when it is entered through Auth Options

It is an extension; it doesn't belong in gophercloud.AuthOptions.

codevulture commented 8 years ago

I have written usage tests in: http://paste.openstack.org/show/521847/ ; Please check.

yuanying commented 8 years ago

Hi, @jrperritt. One question. If we add extensions.trust.AuthOptions, which is better to add Authenticate method like AuthenticateV3Trust to extensions directory or simply to modify AuthenticateV3 method to support trust?

jrperritt commented 8 years ago

Issue

Solution

ToAuthOptionsV3Map() (map[string]interface{}, error)
func (ao AuthOptions) ToAuthOptionsV3Map() (map[string]interface{}, error) {
  // current tokensv3.Create logic should go here
}
type AuthOptionsExt struct {
  gophercloud.AuthOptionsV3er
  TrustID string
}

func (ao AuthOptionsExt) ToAuthOptionsV3Map() (map[string]interface{}, error) {
  m, err := ao.AuthOptionsV3er.ToAuthOptionsV3Map()
  //error handling

  // add logic for adding TrustID to m

  return m, nil
}
rochaporto commented 8 years ago

@codevulture will you continue this one? we're looking forward to get this in, let me know if we can help.

jrperritt commented 8 years ago

For anyone interested, I'll be adding support for this to gophercloud/gophercloud. I'll still merge this one here if anyone gets around to doing it.

rochaporto commented 8 years ago

@jrperritt sounds great, looking fwd!

codevulture commented 8 years ago

@jrperritt , @rochaporto : Thanks, any help would be appreciated. I have made this patch as per the requirements according to the comments. Hope this patch could be used. @jrperritt : can you please have a quick look at it so it could be merged. @rochaporto : Please feel free to commit in this patch.Thanks.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.03%) to 80.63% when pulling 41add948d87aaa947d598cc0d703536ea20f3988 on codevulture:keystone_api into 1f02c2bb21f4995dc5afb78314bbb2627fbb32d6 on rackspace:master.

codevulture commented 8 years ago

@jrperritt : I saw https://github.com/gophercloud/gophercloud/commit/0bc5578dc193f747667cdeee035460f4a168e015 , Is this commit not of use now ? I want these to be merged as soon as possible because it has been ~2 months now.

jrperritt commented 8 years ago

I want these to be merged as soon as possible because it has been ~2 months now.

It would be a mistake to think that the amount of time that has elapsed has any bearing on the suitability of this PR to get merged.

What does have bearing is style and correctness, and in those aspects this PR does look satisfactory now.

codevulture commented 8 years ago

Hi @jrperritt Can i get your contact details Irc / email-id? I had some questions that i wanted to ask..Thanks.

jrperritt commented 8 years ago

@codevulture Sure. Just to be clear:

codevulture commented 8 years ago

@jrperritt : Thanks for the information.

Gophercloud questions should be asked on Github so that others may benefit. Okay, Through comments on patches i guess, Is there a public channel for gophercloud repo on irc/slack?

I have gone through https://github.com/rackspace/gophercloud/issues/592 and wanted to ask that should the new repo(gophercloud/gophercloud) be imported now instead of(rackspace/gophercloud) in projects like: https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack.go Or should we wait for a specific release? Thanks in advance.

jrperritt commented 8 years ago

Is there a public channel for gophercloud repo on irc/slack?

No

should the new repo(gophercloud/gophercloud) be imported ... Or should we wait for a specific release? Thanks in advance.

You may use this repo or the new repo at any time, separately or in conjunction. Currently, the new repo has no plans for a release schedule. As it says in the README: if you want to use it, vendor it and write integration tests for the parts that you use (Note: kubernetes already does this: https://github.com/kubernetes/kubernetes/tree/master/vendor/github.com/rackspace/gophercloud).

codevulture commented 8 years ago

@jrperritt : Yes Kubernetes uses rackspace/gophercloud repo for now, So the features and modifications you added in new gophercloud/gophercloud related to trust, should i add those in this repo also as those are related to trust and might be also be needed here, What is your opinion?

jrperritt commented 8 years ago

should i add those in this repo also as those are related to trust and might be also be needed here

The PR you made for authentication via Trust ID was merged in this repo, because it was submitted prior to declaring the feature-freeze. This repo is now closed to new features.

codevulture commented 8 years ago

@jrperritt : Ohkay, that's why i had a doubt that would the repos in kubernetes or other projects that uses this repo have a plan to use gophercloud/gophercloud in future. But that said i guess only patches that resolve bugs are entertained now or we have to use new repo and make changes in kubernetes or other projects to update it's vendor deps as applicable.