fix token controller keyFunc bug #68119

WanLinghao · 2018-08-31T08:20:48Z

Currently, token manager use keyFunc like: fmt.Sprintf("%q/%q/%#v", name, namespace, tr.Spec).
Since tr.Spec contains point fields, new token request would not reuse the cache at all.
This patch fix this, also adds unit test.

NONE

WanLinghao · 2018-08-31T08:21:38Z

/assign mikedanese

WanLinghao · 2018-09-01T05:10:52Z

/retest

WanLinghao · 2018-09-04T01:42:59Z

@awly PTAL thanks

awly · 2018-09-04T17:29:15Z

pkg/kubelet/token/token_manager.go

+		return "", fmt.Errorf("bound object was nil for tr: %#v", tr)
+	}
+
+	specKey := fmt.Sprintf("%#v:%d:%#v", tr.Spec.Audiences, *tr.Spec.ExpirationSeconds, *tr.Spec.BoundObjectRef)


Call fmt.Sprintf only once, with all of the vars.
Also, any reason to use : here as separator vs / below?

awly · 2018-09-04T17:30:54Z

pkg/kubelet/token/token_manager.go

-func keyFunc(name, namespace string, tr *authenticationv1.TokenRequest) string {
-	return fmt.Sprintf("%q/%q/%#v", name, namespace, tr.Spec)
+func keyFunc(name, namespace string, tr *authenticationv1.TokenRequest) (string, error) {
+	if tr.Spec.ExpirationSeconds == nil {


I don't think this should return error just because dereferencing the pointers would panic. Declare vars with empty values for these fields and set them if fields aren't nil.

awly · 2018-09-04T17:32:11Z

pkg/kubelet/token/token_manager_test.go

+			_, hit := mgr.get(getKey(c.target))
+
+			if hit != c.shouldHit {
+				t.Errorf("%s: got unexpected hit result", c.name)


Add hit/shouldHit to the message

awly · 2018-09-04T17:33:38Z

pkg/kubelet/token/token_manager_test.go

+							ExpirationSeconds: getInt64Point(2000),
+							BoundObjectRef: &authenticationv1.BoundObjectReference{
+								Kind:       "pod",
+								APIVersion: "",


Remove this line on all TokenRequests

awly · 2018-09-04T17:34:33Z

pkg/kubelet/token/token_manager_test.go

@@ -221,3 +221,197 @@ func TestCleanup(t *testing.T) {
 		})
 	}
 }
+
+type tokenRequestUnit struct {


Move this into TestKeyFunc

WanLinghao · 2018-09-06T01:22:17Z

@awly comments addressed, PTAL

mikedanese · 2018-09-06T14:22:39Z

pkg/kubelet/token/token_manager.go

@@ -147,5 +148,16 @@ func (m *Manager) requiresRefresh(tr *authenticationv1.TokenRequest) bool {

 // keys should be nonconfidential and safe to log
 func keyFunc(name, namespace string, tr *authenticationv1.TokenRequest) string {
-	return fmt.Sprintf("%q/%q/%#v", name, namespace, tr.Spec)
+	if tr.Spec.ExpirationSeconds == nil {


I don't like that this modifies the the provided token request. I pushed a change that fixes that.

…name, namespace, tr.Spec)`. Since tr.Spec contains point fields, new token request would not reuse the cache at all. This patch fix this, also adds unit test. Signed-off-by: Mike Danese <mikedanese@google.com>

mikedanese · 2018-09-06T16:05:15Z

/lgtm

awly · 2018-09-06T16:26:10Z

/lgtm

k8s-ci-robot · 2018-09-06T16:26:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, mikedanese, WanLinghao

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:

~~pkg/kubelet/token/OWNERS~~ [mikedanese]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mikedanese · 2018-09-06T19:00:05Z

@WanLinghao I pushed a slight change since I want to get this into 1.12. You are preserved as the author. Thanks for finding this!

k8s-github-robot · 2018-09-06T21:50:27Z

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

k8s-github-robot · 2018-09-06T23:20:35Z

Automatic merge from submit-queue (batch tested with PRs 68119, 68191). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

k8s-ci-robot · 2018-09-13T12:14:10Z

@WanLinghao: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-integration	`794e665`	link	`/test pull-kubernetes-integration`
pull-kubernetes-e2e-kops-aws	`794e665`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-local-e2e-containerized	`794e665`	link	`/test pull-kubernetes-local-e2e-containerized`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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 requested review from mikedanese and tallclair August 31, 2018 08:21

k8s-ci-robot assigned mikedanese Aug 31, 2018

WanLinghao force-pushed the token_controller_cachekey_fix branch from 239bf62 to 6f1dfa6 Compare August 31, 2018 10:08

mikedanese added the kind/bug Categorizes issue or PR as related to a bug. label Sep 1, 2018

mikedanese added this to the v1.12 milestone Sep 1, 2018

mikedanese assigned awly Sep 1, 2018

awly reviewed Sep 4, 2018

View reviewed changes

tallclair removed their request for review September 5, 2018 00:21

WanLinghao force-pushed the token_controller_cachekey_fix branch from 6f1dfa6 to a329709 Compare September 5, 2018 02:11

mikedanese reviewed Sep 6, 2018

View reviewed changes

Currently, token manager use keyFunc like: `fmt.Sprintf("%q/%q/%#v", …

794e665

…name, namespace, tr.Spec)`. Since tr.Spec contains point fields, new token request would not reuse the cache at all. This patch fix this, also adds unit test. Signed-off-by: Mike Danese <mikedanese@google.com>

mikedanese force-pushed the token_controller_cachekey_fix branch from a329709 to 794e665 Compare September 6, 2018 16:04

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 6, 2018

mikedanese added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 6, 2018

k8s-github-robot merged commit 4bb3712 into kubernetes:master Sep 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix token controller keyFunc bug #68119

fix token controller keyFunc bug #68119

WanLinghao commented Aug 31, 2018

WanLinghao commented Aug 31, 2018

WanLinghao commented Sep 1, 2018

WanLinghao commented Sep 4, 2018

awly Sep 4, 2018

awly Sep 4, 2018

awly Sep 4, 2018

awly Sep 4, 2018

awly Sep 4, 2018

WanLinghao commented Sep 6, 2018

mikedanese Sep 6, 2018 •

edited

Loading

mikedanese commented Sep 6, 2018

awly commented Sep 6, 2018

k8s-ci-robot commented Sep 6, 2018

mikedanese commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

k8s-ci-robot commented Sep 13, 2018

fix token controller keyFunc bug #68119

fix token controller keyFunc bug #68119

Conversation

WanLinghao commented Aug 31, 2018

WanLinghao commented Aug 31, 2018

WanLinghao commented Sep 1, 2018

WanLinghao commented Sep 4, 2018

awly Sep 4, 2018

Choose a reason for hiding this comment

awly Sep 4, 2018

Choose a reason for hiding this comment

awly Sep 4, 2018

Choose a reason for hiding this comment

awly Sep 4, 2018

Choose a reason for hiding this comment

awly Sep 4, 2018

Choose a reason for hiding this comment

WanLinghao commented Sep 6, 2018

mikedanese Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

mikedanese commented Sep 6, 2018

awly commented Sep 6, 2018

k8s-ci-robot commented Sep 6, 2018

mikedanese commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

k8s-ci-robot commented Sep 13, 2018

mikedanese Sep 6, 2018 •

edited

Loading