-
Notifications
You must be signed in to change notification settings - Fork 39.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
read openstack auth config from client config #60200
read openstack auth config from client config #60200
Conversation
b2d6b14
to
4dbc516
Compare
/assign @liggitt PTAL. Thanks. |
var authOpt gophercloud.AuthOptions | ||
configByte, err := json.Marshal(config) | ||
if err == nil { | ||
json.Unmarshal(configByte, &authOpt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no error handling here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a test, and an example in doc (link to PR against 1.10 doc branch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like some options are not serialized in the AuthOptions struct. IdentityEndpoint in particular seems to be ignored. How would the Token()
method know what server to contact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would the Token() method know what server to contact?
L62 func (t *tokenGetter) Token()
will use this authOpt
to get an authenticated client, where IdentityEndpoint
will be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like some options are not serialized in the AuthOptions struct. IdentityEndpoint in particular seems to be ignored.
Good catch. AuthOptions
omits this filed. We need to retrieve this value explicitly.
json.Unmarshal(configByte, &authOpt) | ||
} | ||
// not empty | ||
if !reflect.DeepEqual(authOpt, gophercloud.AuthOptions{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be able to just compare straight: authOpt != gophercloud.AuthOptions{}
4dbc516
to
f512bda
Compare
@liggitt Updated. PTAL. Thanks. |
@dims has already got an in-progress PR kubernetes/website#7304 on this. |
@dixudx please do take that over if you can :) |
/retest |
f512bda
to
dee2430
Compare
@@ -145,11 +156,33 @@ func newOpenstackAuthProvider(clusterAddress string, config map[string]string, p | |||
} | |||
} | |||
|
|||
// TODO: read/persist client configuration(OS_XXX env vars) in config | |||
configByte, err := json.Marshal(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not good treat a single config struct as a mix of two API types (we expect a "ttl" key above, and this checks if they happened to specify keys that match the gophercloud.AuthOptions
struct). what would happen if the gophercloud.AuthOptions
struct added a ttl field that was an int?
// not empty | ||
if (authOpt != gophercloud.AuthOptions{}) { | ||
// TODO: remove this when gophercloud supports marshal this field | ||
endpoint, ok := config["identityEndpoint"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is another sign that this probably isn't the right approach.
if you want to populate options from inline config, I'd suggest doing it explicitly, like this:
gophercloud.AuthOptions{
IdentityEndpoint: config["identityEndpoint"],
Username: config["username"],
Password: config["password"],
DomainName: config["name"],
TenantID: config["tenantId"],
TenantName: config["tenantName"],
}
by accepting "identityEndpoint", you are already diverging from a standard auth options json blob, so being explicit doesn't seem bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are already diverging from a standard auth options json blob, so being explicit doesn't seem bad.
Make sense.
dee2430
to
10201f3
Compare
@liggitt Updated. PTAL. Thanks. |
@liggitt ok with the changes based on your comments? |
ping @liggitt for another look. Thanks. |
/lgtm |
/test pull-kubernetes-e2e-gce |
/lgtm needs a better release note and PR with updated documentation on the accepted parameters |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, dixudx, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
/sig openstack
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
/assign @dims
Release note: