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

gcp client auth plugin: persist default cache on unauthorized #66314

Merged
merged 1 commit into from
Sep 14, 2018
Merged

gcp client auth plugin: persist default cache on unauthorized #66314

merged 1 commit into from
Sep 14, 2018

Conversation

j3ffml
Copy link
Contributor

@j3ffml j3ffml commented Jul 17, 2018 •
edited

What this PR does / why we need it:

This PR fixes an edge case error introduced by #46694. It changes the gcp auth client plugin's roundtripper to persist the default cache instead of an empty map. An empty cache is not correct for commandTokenSource, which uses the cache to store calling details for the external command it execs to generate refresh tokens. Persisting a completely empty cache breaks that behavior.

This bug caused an issue where running kubectl command against a GKE cluster with an expired refresh token would break token refresh permanently until gcloud container clusters get-credentials was re-run.

Special notes for your reviewer:

Release note:

Fix bug that caused `kubectl` commands to sometimes fail to refresh access token when running against GKE clusters.

@j3ffml j3ffml requested review from mengqiy and cjcullen July 17, 2018 23:24
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2018
mengqiy
mengqiy reviewed Jul 18, 2018
View reviewed changes
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

LGTM
But I'd like to defer the final call to @cjcullen

cjcullen
cjcullen reviewed Jul 19, 2018
View reviewed changes
staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go Outdated
@@ -247,6 +253,18 @@ func (t *cachedTokenSource) update(tok *oauth2.Token) map[string]string {
return ret
}

func (t *cachedTokenSource) baseCache() map[string]string {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment like:

// baseCache is the base configuration value for this TokenSource, without any cached ephemeral tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@cjcullen
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2018
@cjcullen cjcullen added this to the v1.12 milestone Sep 10, 2018
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 10, 2018
@j3ffml
gcp client auth plugin: persist default cache on unauthorized
73e5e43 
The default cache for a cachedTokenSource is not always empty. In the
case of commandTokenSource, it contains calling details for the
external command that is used to generate refresh tokens. Persisting
a completely empty cache will thus break ability for the plugin to
obtain refresh tokens. This changes the roundtripper to persist
the default cache instead of assuming an empty map.
@j3ffml
Copy link
Contributor Author

j3ffml commented Sep 10, 2018

/lgtm

rebase only

@k8s-ci-robot
Copy link
Contributor

@jlowdermilk: you cannot LGTM your own PR.

In response to this:

/lgtm

rebase only

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.

@cjcullen
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, jlowdermilk

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

@j3ffml
Copy link
Contributor Author

j3ffml commented Sep 10, 2018

/test pull-kubernetes-kubemark-e2e-gce-big

@cjcullen cjcullen added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 13, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit c04fe8c into kubernetes:master Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mengqiy mengqiy mengqiy left review comments

@cjcullen cjcullen cjcullen left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

Assignees

@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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@j3ffml @cjcullen @k8s-ci-robot @fejta-bot @mengqiy