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.

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

BoundServiceAccountTokenVolume: fix InClusterConfig #77613

Merged
merged 2 commits into from
May 15, 2019

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented May 8, 2019

Missed this in dba85e58deba and rest.Config is dropping BearerTokenFile path before the token source is created. This is causing InClusterConfig to not refresh tokens from disk.

😢 😭

I'm thinking about how to get some test coverage here.

/kind bug
/sig auth

NONE

Fixes #77651

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 8, 2019
@mikedanese mikedanese added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 8, 2019
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 8, 2019
@liggitt
Copy link
Member

liggitt commented May 8, 2019

/lgtm
/approve

/hold
this desperately needs a test

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 8, 2019
@liggitt
Copy link
Member

liggitt commented May 8, 2019

I0508 20:16:54.000] should handle in-cluster config [It]
I0508 20:16:54.000] test/e2e/kubectl/kubectl.go:616
I0508 20:16:54.000]
I0508 20:16:54.000] Expected an error to have occurred. Got:
I0508 20:16:54.000] <nil>: nil

I think we also need these to ensure the overridden command line --token trumps:

diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/authentication.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/authentication.go
index fb6a7fa3ba..dda51b60d2 100644
--- a/staging/src/k8s.io/apiserver/pkg/util/webhook/authentication.go
+++ b/staging/src/k8s.io/apiserver/pkg/util/webhook/authentication.go
@@ -171,6 +171,7 @@ func restConfigFromKubeconfig(configAuthInfo *clientcmdapi.AuthInfo) (*rest.Conf
        // blindly overwrite existing values based on precedence
        if len(configAuthInfo.Token) > 0 {
                config.BearerToken = configAuthInfo.Token
+               config.BearerTokenFile = configAuthInfo.TokenFile
        } else if len(configAuthInfo.TokenFile) > 0 {
                tokenBytes, err := ioutil.ReadFile(configAuthInfo.TokenFile)
                if err != nil {
diff --git a/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go b/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go
index 878e0df79f..c62ee03c77 100644
--- a/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go
+++ b/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go
@@ -228,6 +228,7 @@ func (config *DirectClientConfig) getUserIdentificationPartialConfig(configAuthI
        // blindly overwrite existing values based on precedence
        if len(configAuthInfo.Token) > 0 {
                mergedConfig.BearerToken = configAuthInfo.Token
+               mergedConfig.BearerTokenFile = configAuthInfo.TokenFile
        } else if len(configAuthInfo.TokenFile) > 0 {
                tokenBytes, err := ioutil.ReadFile(configAuthInfo.TokenFile)
                if err != nil {
@@ -491,6 +492,7 @@ func (config *inClusterClientConfig) ClientConfig() (*restclient.Config, error)
                }
                if token := config.overrides.AuthInfo.Token; len(token) > 0 {
                        icc.BearerToken = token
+                       icc.BearerTokenFile = ""
                }
                if certificateAuthorityFile := config.overrides.ClusterInfo.CertificateAuthority; len(certificateAuthorityFile) > 0 {
                        icc.TLSClientConfig.CAFile = certificateAuthorityFile

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/apiserver area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 8, 2019
@mikedanese
Copy link
Member Author

I'm headed down the path of creating a new test image and e2e for in cluster config. Feel free to stop me if you have any bright ideas.

@mikedanese mikedanese force-pushed the fixinclusterconfig branch 2 times, most recently from a4ba57a to d4c676f Compare May 10, 2019 01:03
@@ -410,4 +415,143 @@ var _ = SIGDescribe("ServiceAccounts", func() {
}
}
})

//It("should support InClusterConfig with token rotation [Slow]", func() {
It("should support InClusterConfig with token rotation", func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test works now but its slow (~10 minutes). It's pretty gross because I want it to run on non-alpha clusters. I could drop a bunch of stuff if we are ok with this only running in the alpha test suite.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also it will still be slow even if we only run it in the alpha suite, just way smaller.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we check feature gates in the e2e process (it's not even the same process as the apiserver)... typically we indicate non-default features with feature tags in the test description

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, fixed.

@liggitt
Copy link
Member

liggitt commented May 14, 2019

would it make sense to split the e2e test into its own commit? were you planning to pick the test back to 1.12/1.13/1.14 as well?

@liggitt
Copy link
Member

liggitt commented May 14, 2019

the test is ugly, but I want coverage more than beauty at the moment. with the [Slow] and [Feature] tags, do we know what suite this will run in so we can monitor it?

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese

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

@liggitt
Copy link
Member

liggitt commented May 14, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2019
@mikedanese
Copy link
Member Author

/retest

2 similar comments
@mikedanese
Copy link
Member Author

/retest

@mikedanese
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 5c4b652 into kubernetes:master May 15, 2019
@mikedanese mikedanese deleted the fixinclusterconfig branch May 15, 2019 03:47
k8s-ci-robot added a commit that referenced this pull request May 18, 2019
…7613-upstream-release-1.12

Automated cherry pick of #77613 upstream release 1.12
k8s-ci-robot added a commit that referenced this pull request May 21, 2019
…7613-upstream-release-1.14

Automated cherry pick of #77613 upstream release 1.14
k8s-ci-robot added a commit that referenced this pull request May 21, 2019
…7613-upstream-release-1.13

Automated cherry pick of #77613 upstream release 1.13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BoundServiceAccountTokenVolume: kube-proxy doesn't reload ServiceAccount tokens
4 participants