Skip to content
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.

Sign up for GitHub

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

Jump to bottom

read openstack auth config from client config #60200

Merged
merged 1 commit into from May 10, 2018
Merged

read openstack auth config from client config #60200

merged 1 commit into from May 10, 2018

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Feb 22, 2018 •
edited

What this PR does / why we need it:

// TODO: read/persist client configuration(OS_XXX env vars) in config

/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:

try to read openstack auth config from client config and fall back to read from the environment variables if not available

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. area/provider/openstack Issues or PRs related to openstack provider size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 22, 2018
@dixudx
Copy link
Member Author

dixudx commented Feb 26, 2018

/assign @liggitt

PTAL. Thanks.

liggitt
liggitt reviewed Feb 27, 2018
View reviewed changes
staging/src/k8s.io/client-go/plugin/pkg/client/auth/openstack/openstack.go Outdated
var authOpt gophercloud.AuthOptions
configByte, err := json.Marshal(config)
if err == nil {
json.Unmarshal(configByte, &authOpt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no error handling here?

Copy link
Member

@liggitt liggitt Feb 27, 2018 •
edited

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)

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

liggitt
liggitt reviewed Feb 27, 2018
View reviewed changes
staging/src/k8s.io/client-go/plugin/pkg/client/auth/openstack/openstack.go Outdated
json.Unmarshal(configByte, &authOpt)
}
// not empty
if !reflect.DeepEqual(authOpt, gophercloud.AuthOptions{}) {
Copy link
Member

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{}

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2018
@dixudx
Copy link
Member Author

dixudx commented Feb 27, 2018

@liggitt Updated. PTAL. Thanks.

@dixudx
Copy link
Member Author

dixudx commented Mar 1, 2018

an example in doc (link to PR against 1.10 doc branch)

@dims has already got an in-progress PR kubernetes/website#7304 on this.

@dims
Copy link
Member

dims commented Mar 1, 2018

@dixudx please do take that over if you can :)

@dims dims mentioned this pull request Mar 14, 2018
Closed
@dims
Copy link
Member

dims commented Apr 5, 2018

/retest

@dixudx dixudx force-pushed the clientgo_openstack_config branch from f512bda to dee2430 Compare April 9, 2018 07:39
liggitt
liggitt reviewed Apr 18, 2018
View reviewed changes
staging/src/k8s.io/client-go/plugin/pkg/client/auth/openstack/openstack.go Outdated
@@ -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)
Copy link
Member

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?

liggitt
liggitt reviewed Apr 18, 2018
View reviewed changes
staging/src/k8s.io/client-go/plugin/pkg/client/auth/openstack/openstack.go Outdated
// not empty
if (authOpt != gophercloud.AuthOptions{}) {
// TODO: remove this when gophercloud supports marshal this field
endpoint, ok := config["identityEndpoint"]
Copy link
Member

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.

Copy link
Member Author

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.

@dixudx
Copy link
Member Author

dixudx commented Apr 19, 2018

@liggitt Updated. PTAL. Thanks.

@dims
Copy link
Member

dims commented Apr 24, 2018

@liggitt ok with the changes based on your comments?

@dixudx
Copy link
Member Author

dixudx commented May 9, 2018

ping @liggitt for another look. Thanks.

@dims
Copy link
Member

dims commented May 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2018
@dixudx
Copy link
Member Author

dixudx commented May 9, 2018

/test pull-kubernetes-e2e-gce

@dims
Copy link
Member

dims commented May 10, 2018

/assign @sttts

@sttts @liggitt can you please take a look? this seems ready.

@liggitt
Copy link
Member

liggitt commented May 10, 2018

/lgtm

needs a better release note and PR with updated documentation on the accepted parameters

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 9dcbdc3 into kubernetes:master May 10, 2018
@dixudx dixudx deleted the clientgo_openstack_config branch May 11, 2018 00:43
@dlipovetsky dlipovetsky mentioned this pull request May 14, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@dims dims

@sttts sttts

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dixudx @dims @liggitt @k8s-ci-robot @k8s-github-robot @sttts