Skip to content
This repository has been archived by the owner on Aug 1, 2023. It is now read-only.

Add support for Keystone Trust (v3) #580

Merged
merged 1 commit into from
Aug 3, 2016
Merged

Add support for Keystone Trust (v3) #580

merged 1 commit into from
Aug 3, 2016

Conversation

codevulture
Copy link
Contributor

Adding Keystone Trust

@coveralls
Copy link

coveralls commented May 20, 2016

Coverage Status

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

codevulture added a commit to codevulture/kubernetes that referenced this pull request May 22, 2016
This change would be merged after below is completed.
Adding Support in Gopherhead Library:[wip] rackspace/gophercloud#580
@coveralls
Copy link

coveralls commented May 23, 2016

Coverage Status

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

@codevulture codevulture changed the title [WIP] Add Keystone Trust Support Add support for Keystone Trust (v3) May 23, 2016
@jrperritt
Copy link
Contributor

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

@codevulture
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

codevulture commented Jun 9, 2016

@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
Copy link
Contributor Author

@jrperritt : Please review.

@jrperritt
Copy link
Contributor

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
Copy link
Contributor Author

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

@yuanying
Copy link

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
Copy link
Contributor

Issue

  • The scope of this PR is only Identity v3.
  • You want to add an option to authenticate via trust
  • Trusts are an extension to OpenStack proper (I think in general, but certainly from the point of view of this repo).
  • Because of the point above, we can't put TrustID in gophercloud.AuthOptions
  • Currently, the Identity v3 authentication functions accept a gophercloud.AuthOptions.
  • Whatever the implementation, it needs to be backwards-compatible

Solution

  • Instead of having the Identity v3 functions accept a gophercloud.AuthOptions, have them accept an interface (like AuthOptionsV3er).
  • The interface above should have a single method defined on it:
ToAuthOptionsV3Map() (map[string]interface{}, error)
  • A new method should be created on gophercloud.AuthOptions, namely:
func (ao AuthOptions) ToAuthOptionsV3Map() (map[string]interface{}, error) {
  // current tokensv3.Create logic should go here
}
  • A new type called AuthOptionsExt should be created in the requests.go file of the trust extensions directory:
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
}
  • tokensV3.Create should accept a AuthOptionsV3er and call ToAuthOptionsV3map to create the request body

@rochaporto
Copy link

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

@jrperritt
Copy link
Contributor

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
Copy link

@jrperritt sounds great, looking fwd!

@codevulture
Copy link
Contributor Author

codevulture commented Jul 26, 2016

@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
Copy link

coveralls commented Jul 27, 2016

Coverage Status

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

@codevulture
Copy link
Contributor Author

@jrperritt : I saw gophercloud/gophercloud@0bc5578 , 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
Copy link
Contributor

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.

@jrperritt jrperritt merged commit 0d85999 into rackspace:master Aug 3, 2016
@codevulture
Copy link
Contributor Author

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

@jrperritt
Copy link
Contributor

@codevulture Sure. Just to be clear:

  • Gophercloud questions should be asked on Github so that others may benefit.
  • OpenStack questions should be directed to OpenStack communication channels (IRC/slack/mailing list).
  • Other questions can reach me via Twitter (message or tweet): @jonperritt

@codevulture
Copy link
Contributor Author

@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 #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
Copy link
Contributor

jrperritt commented Aug 19, 2016

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

@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.

codevulture added a commit to codevulture/kubernetes that referenced this pull request Aug 29, 2016
This change would be merged after below is completed.
Added Support in Gopherhead Library:rackspace/gophercloud#580
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants