add secret back to the workqueue with delay time, avoid expired bootstrap tokens not being deleted #77713

zjj2wry · 2019-05-10T03:39:09Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Fixes #77505
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

zjj2wry · 2019-05-10T03:40:17Z

/sig auth
/cc @liggitt

liggitt · 2019-05-10T03:48:18Z

pkg/controller/bootstrap/tokencleaner.go

@@ -52,6 +54,7 @@ type TokenCleanerOptions struct {
 func DefaultTokenCleanerOptions() TokenCleanerOptions {
 	return TokenCleanerOptions{
 		TokenSecretNamespace: api.NamespaceSystem,
+		SecretResync:         defaultSecretResyncInterval,


this is an extremely short resync period. I probably wouldn't set this below an hour

I looked at the kubeadm command and did not provide a valid default value. The token controller is only used for gc. It is reasonable to set a longer time. I have modified it to an hour

what is the offset from the expected GC time?
are you seeing a resync that never happens?

I understand that the token controller will only enqueue the object when adding or modifying the secret. Add a resync in order to re-enqueue all the secrets.

what is the offset from the expected GC time?

Currently set to one hour, maybe longer？

mikedanese · 2019-06-14T01:50:16Z

The bug sites 5 minutes as the desired wait. This doesn't look like it fixes the issue with a one hour resync. Why don't you just add the secret back to the workqueue with a delay == expiration - now?

liggitt · 2019-07-08T22:03:51Z

The bug sites 5 minutes as the desired wait. This doesn't look like it fixes the issue with a one hour resync. Why don't you just add the secret back to the workqueue with a delay == expiration - now?

Given the small volume of expected secrets of this type, that seems like a reasonable approach

zjj2wry · 2019-07-10T10:01:17Z

@mikedanese @liggitt
thank you for reviewing, so token will be deleted in real time 👍 , I will modify this pr

k8s-ci-robot · 2019-07-11T06:59:09Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

zjj2wry · 2019-07-11T07:07:42Z

/priority backlog
@mikedanese @liggitt updated, ptal

liggitt · 2019-07-11T14:10:43Z

pkg/controller/bootstrap/tokencleaner.go

@@ -38,6 +38,8 @@ import (
 	"k8s.io/kubernetes/pkg/util/metrics"
 )

+const defaultSecretResyncInterval = 1 * time.Hour


we can drop the resync now

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

zjj2wry · 2019-07-12T12:29:09Z

/test pull-kubernetes-e2e-gce

liggitt · 2019-07-12T12:53:11Z

pkg/controller/bootstrap/tokencleaner_test.go

+	}
+
+	// token expires after 3 seconds
+	time.AfterFunc(3*time.Second, verifyFunc)


this doesn't test that the secret got auto-added back into the queue. we need to verify the length of the queue is 0, then after the secret expiration, the secret is auto-added back into the queue

you are right, I modified the unit test and found the problem with the previous code. GetExpiration I should be trying to return how long it will expire, the previous code returned a negative number. I should write a good test : )

liggitt · 2019-07-15T16:44:58Z

pkg/controller/bootstrap/tokencleaner_test.go

+	}
+
+	secret := newTokenSecret("tokenID", "tokenSecret")
+	addSecretExpiration(secret, timeString(3*time.Second))


making this identical to the sleep below seems likely to introduce flakes. suggest an expiration of 2 seconds here, and a wait.PollImmediate check below checking every 100ms (with a timeout of wait.ForeverTestTimeout) until the queue length is 1

make sense, updated. thanks ：）：)

liggitt · 2019-07-26T14:12:18Z

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

+	return expired
+}
+
+// GetExpiration get expiration time from now


need to doc the following things:

the meaning of the boolean (isExpired)

the meaning of a zero-value duration when isExpired=false (no expiration)

liggitt · 2019-07-26T14:13:38Z

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

+			expiration, secret.Namespace, secret.Name, err)
+		return 0, true
+	}
+	expAt := currentTime.Sub(expTime)


would be easier to read as

timeRemaining := expTime.Sub(currentTime) if timeRemaining <= 0 { ... } return timeRemaining, false

zjj2wry · 2019-07-30T06:52:44Z

@liggitt updated，PTAL

liggitt

a few last clarifications, then squash to a single commit. thanks!

liggitt · 2019-07-30T14:14:30Z

pkg/controller/bootstrap/tokencleaner.go

@@ -188,7 +188,8 @@ func (tc *TokenCleaner) syncFunc(key string) error {

 func (tc *TokenCleaner) evalSecret(o interface{}) {
 	secret := o.(*v1.Secret)
-	if bootstrapsecretutil.HasExpired(secret, time.Now()) {
+	expiration, ok := bootstrapsecretutil.GetExpiration(secret, time.Now())


rename to ttl, alreadyExpired

liggitt · 2019-07-30T14:26:39Z

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

+}
+
+// GetExpiration checks if the secret expires
+// isExpired indicates whether it expires, and timeRemaining indicates how long it will expire


isExpired indicates if the secret is already expired.
timeRemaining indicates how long until it does expire.
if the secret has no expiration timestamp, returns 0, false.
if there is an error parsing the secret's expiration timestamp, returns 0, true.

liggitt · 2019-07-30T14:27:04Z

pkg/controller/bootstrap/tokencleaner.go

@@ -201,4 +202,7 @@ func (tc *TokenCleaner) evalSecret(o interface{}) {
 			klog.V(3).Infof("Error deleting Secret: %v", err)
 		}
 	}


} else if ttl > 0 {

…trap tokens not being deleted

zjj2wry · 2019-08-01T06:11:58Z

/test pull-kubernetes-e2e-gce-100-performance

zjj2wry · 2019-08-01T14:07:10Z

@liggitt already squash commits, very thank you for code review, ptal

liggitt · 2019-08-01T14:22:33Z

/lgtm
/approve

k8s-ci-robot · 2019-08-01T14:23:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/controller/OWNERS~~ [liggitt]
~~staging/OWNERS~~ [liggitt]

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

KashifSaadat · 2020-01-13T15:15:41Z

@zjj2wry thanks for your work on fixing this! I've hit the same problem with tokens being issued by a Node Authorizer, causing a build-up of 4000+ old bootstrap token secrets that should have been cleared down. Eventually resulted in a timeout to the API fetching secrets and no new nodes getting bootstrap tokens issued.

Do you think this would be eligible for a cherry-pick to the 1.15, 1.16 branches? Doesn't look like it has made it into any release yet.

liggitt · 2020-01-13T15:17:16Z

Do you think this would be eligible for a cherry-pick to the 1.15, 1.16 branches? Doesn't look like it has made it into any release yet.

This is already in all v1.16.x and v1.17.x releases

KashifSaadat · 2020-01-13T15:26:35Z

Ah apologies my mistake, hadn't spotted it in the changelog but I can see it's made it through 👍

k8s-ci-robot requested review from cheftako and deads2k May 10, 2019 03:39

k8s-ci-robot requested a review from liggitt May 10, 2019 03:40

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label May 10, 2019

liggitt reviewed May 10, 2019

View reviewed changes

zjj2wry force-pushed the secret_resync_duration branch from 9a39c6c to f3de10a Compare May 10, 2019 06:31

tallclair assigned mikedanese Jun 12, 2019

zjj2wry force-pushed the secret_resync_duration branch from f3de10a to d8c6ff6 Compare July 11, 2019 06:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 11, 2019

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 11, 2019

zjj2wry force-pushed the secret_resync_duration branch from d8c6ff6 to 73e2187 Compare July 11, 2019 06:59

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jul 11, 2019

zjj2wry force-pushed the secret_resync_duration branch from 73e2187 to 73a7133 Compare July 11, 2019 07:02

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 11, 2019

liggitt reviewed Jul 11, 2019

View reviewed changes

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go Show resolved Hide resolved

zjj2wry force-pushed the secret_resync_duration branch 3 times, most recently from 2b5fd41 to 0c4bb40 Compare July 11, 2019 18:15

liggitt reviewed Jul 12, 2019

View reviewed changes

zjj2wry changed the title ~~Add a resync to avoid expired secrets not being deleted~~ add secret back to the workqueue with delay time, avoid expired bootstrap tokens not being deleted Jul 13, 2019

liggitt reviewed Jul 15, 2019

View reviewed changes

liggitt reviewed Jul 26, 2019

View reviewed changes

liggitt reviewed Jul 30, 2019

View reviewed changes

add secret back to the workqueue with delay time, avoid expired boots…

5c1815a

…trap tokens not being deleted

zjj2wry force-pushed the secret_resync_duration branch from 720647b to 5c1815a Compare August 1, 2019 02:44

k8s-ci-robot assigned liggitt Aug 1, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2019

k8s-ci-robot merged commit b658028 into kubernetes:master Aug 1, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add secret back to the workqueue with delay time, avoid expired bootstrap tokens not being deleted #77713

add secret back to the workqueue with delay time, avoid expired bootstrap tokens not being deleted #77713

zjj2wry commented May 10, 2019 •

edited

zjj2wry commented May 10, 2019

liggitt May 10, 2019

zjj2wry May 10, 2019

neolit123 May 10, 2019

zjj2wry May 13, 2019 •

edited

mikedanese commented Jun 14, 2019 •

edited

liggitt commented Jul 8, 2019

zjj2wry commented Jul 10, 2019

k8s-ci-robot commented Jul 11, 2019

zjj2wry commented Jul 11, 2019

liggitt Jul 11, 2019

zjj2wry commented Jul 12, 2019

liggitt Jul 12, 2019

zjj2wry Jul 13, 2019 •

edited

liggitt Jul 15, 2019 •

edited

zjj2wry Jul 23, 2019

liggitt Jul 26, 2019

liggitt Jul 26, 2019

zjj2wry commented Jul 30, 2019

liggitt left a comment

liggitt Jul 30, 2019

liggitt Jul 30, 2019

liggitt Jul 30, 2019

zjj2wry commented Aug 1, 2019

zjj2wry commented Aug 1, 2019

liggitt commented Aug 1, 2019

k8s-ci-robot commented Aug 1, 2019

KashifSaadat commented Jan 13, 2020

liggitt commented Jan 13, 2020

KashifSaadat commented Jan 13, 2020

add secret back to the workqueue with delay time, avoid expired bootstrap tokens not being deleted #77713

add secret back to the workqueue with delay time, avoid expired bootstrap tokens not being deleted #77713

Conversation

zjj2wry commented May 10, 2019 • edited

zjj2wry commented May 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry May 13, 2019 • edited

Choose a reason for hiding this comment

mikedanese commented Jun 14, 2019 • edited

liggitt commented Jul 8, 2019

zjj2wry commented Jul 10, 2019

k8s-ci-robot commented Jul 11, 2019

zjj2wry commented Jul 11, 2019

Choose a reason for hiding this comment

zjj2wry commented Jul 12, 2019

Choose a reason for hiding this comment

zjj2wry Jul 13, 2019 • edited

Choose a reason for hiding this comment

liggitt Jul 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry commented Jul 30, 2019

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry commented Aug 1, 2019

zjj2wry commented Aug 1, 2019

liggitt commented Aug 1, 2019

k8s-ci-robot commented Aug 1, 2019

KashifSaadat commented Jan 13, 2020

liggitt commented Jan 13, 2020

KashifSaadat commented Jan 13, 2020

zjj2wry commented May 10, 2019 •

edited

zjj2wry May 13, 2019 •

edited

mikedanese commented Jun 14, 2019 •

edited

zjj2wry Jul 13, 2019 •

edited

liggitt Jul 15, 2019 •

edited