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

Clear auth config when gcp app default credentials fail #46694

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Clear auth config when gcp app default credentials fail #46694

merged 1 commit into from
Jun 21, 2017
+79 −2

Conversation

matt-tyler
Copy link
Contributor

What this PR does / why we need it:

Specific use case is when utilizing multiple gcp accounts, the user may provide credentials for the wrong account.

This change ensures the incorrect credentials are not cached in auth config, and logs an appropriate message.

Which issue this PR fixes : fixes #38075

Special notes for your reviewer:

Release note:

Tokens retrieved from Google Cloud with application default credentials will not be cached if the client fails authorization

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @matt-tyler. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 31, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 31, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2017
j3ffml
j3ffml reviewed Jun 15, 2017
View reviewed changes
staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go Outdated
handler := func(res *http.Response) {
switch res.StatusCode {
case 401:
if g.useDefaultTokenSource {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it also makes sense to purge the cache if we are using a cmd token source, since those are cached in the exact same way.

staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go Outdated
switch res.StatusCode {
case 401:
if g.useDefaultTokenSource {
glog.V(4).Infof("The application default credentials that were supplied are invalid for the target cluster")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest removing the special casing for default token source and change this log to "The credentials that were supplied..."

@j3ffml
Copy link
Contributor

j3ffml commented Jun 15, 2017

@k8s-bot ok-to-test

@j3ffml
Copy link
Contributor

j3ffml commented Jun 15, 2017

Also @matt-tyler, thanks for the pr and sorry for the long delay reviewing.

@matt-tyler
Copy link
Contributor Author

@jlowdermilk No worries, I'll find some time and apply the changes necessary to purge the cache for the cmd token source as well.

@matt-tyler
Clear auth config when gcp credentials fail
b920167 
Specific use case is when utilizing multiple
gcp accounts, the user may provide credentials
for the wrong account.

This change ensures the incorrect credentials
are not cached in auth config, and logs an
appropriate message.
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 18, 2017
@matt-tyler
Copy link
Contributor Author

@jlowdermilk I've rebased and updated the pull request based on your comments. Let me know if there is any more you would like me to do.

@j3ffml j3ffml removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 18, 2017
j3ffml
j3ffml approved these changes Jun 18, 2017
View reviewed changes
Copy link
Contributor

@j3ffml j3ffml left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks!

@j3ffml j3ffml modified the milestones: v1.8, v1.7 Jun 18, 2017
@spiffxp
Copy link
Member

spiffxp commented Jun 19, 2017

/retest

@marun marun added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jun 19, 2017
@j3ffml j3ffml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2017
@marun
Copy link
Contributor

marun commented Jun 20, 2017

/retest

@marun
Copy link
Contributor

marun commented Jun 21, 2017

@jlowdermilk Is there a reason you're not approving this PR?

@j3ffml
Copy link
Contributor

j3ffml commented Jun 21, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlowdermilk, matt-tyler

Associated issue: 38075

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 34515, 47236, 46694, 47819, 47792)

@k8s-github-robot k8s-github-robot merged commit d3ee9df into kubernetes:master Jun 21, 2017
@j3ffml j3ffml mentioned this pull request Jul 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j3ffml j3ffml j3ffml approved these changes

Assignees

@j3ffml j3ffml

@cjcullen cjcullen

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

kubectl: Bad creds get cached in ~/.kube/config, causing user confusion until expiry (or manual file edit)
7 participants
@matt-tyler @k8s-ci-robot @j3ffml @spiffxp @marun @k8s-github-robot @cjcullen