LegacyServiceAccountTokenCleanUp alpha #115554
Conversation
|
Hi @yt2985. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Open the PR first. I am still working on the integration test and may be the e2e test. |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
ed33782
to
ad9bebc
Compare
|
/remove-sig api-machinery |
|
/test pull-kubernetes-conformance-kind-ga-only-parallel |
There was a problem hiding this comment.
Implementation looks pretty close, I haven't looked at test coverage yet
Thinking about someone rolling this out, wondering if we'll be happy with all long-unused token secrets getting deleted at the first kube-controller-manager startup. Things like dry-run behavior or rate-limiting come to mind as things we might want to make sure the results are what we expect.
cc @deads2k for any thoughts along those lines on this?
cmd/kube-controller-manager/app/options/legacyserviceaccounttokencleaner.go
Outdated
Show resolved
Hide resolved
| if podMountedSecrets[secretNamespace] != nil { | ||
| return podMountedSecrets[secretNamespace], nil | ||
| } |
There was a problem hiding this comment.
look for existence rather than non-nil:
if secrets, ok := podMountedSecrets[secretNamespace]; ok {
return secrets
}| if podMountedSecrets[secretNamespace] != nil { | ||
| return podMountedSecrets[secretNamespace], nil | ||
| } | ||
| podMountedSecrets[secretNamespace] = sets.NewString() |
There was a problem hiding this comment.
don't store here, otherwise we cache an empty set in an error case, then have the possibility of returning the cached-and-wrong empty set the next time getMountedSecretNames is invoked on the namespace
| for _, pod := range podList { | ||
| podutil.VisitPodSecretNames(pod, func(secretName string) bool { | ||
| podMountedSecrets[secretNamespace].Insert(secretName) | ||
| return true | ||
| }) | ||
| } | ||
| return podMountedSecrets[secretNamespace], nil |
There was a problem hiding this comment.
use a nil set until we know we need to store secret names:
var secrets sets.String
for _, pod := range podList {
podutil.VisitPodSecretNames(pod, func(secretName string) bool {
if secrets == nil {
secrets = sets.NewString()
}
secrets.Insert(secretName)
return true
})
}then after we've constructed the set without errors, cache it:
podMountedSecrets[secretNamespace] = secrets
return secrets, nil| if err != nil { | ||
| return time.Time{}, fmt.Errorf("error parsing trackedSince time: %v", err) | ||
| } | ||
| return trackedSinceTime.Add(24 * time.Hour), nil |
There was a problem hiding this comment.
I think we want to do trackedSinceTime.AddDate(0,0,1) rather than assume 24 hours from 00:00 on the tracked-since date covers all possible times in that date (daylight saving time can put 25 hours in a nominal date)
There was a problem hiding this comment.
this also deserves a comment on this line and on the godoc for the function
There was a problem hiding this comment.
and latestPossibleTrackedSinceTime might be a better function name
There was a problem hiding this comment.
this also deserves a comment on this line and on the godoc for the function
Hi Jordan, may I know what is the godoc here?
pkg/controller/serviceaccount/legacy_serviceaccount_token_cleaner.go
Outdated
Show resolved
Hide resolved
pkg/controller/serviceaccount/legacy_serviceaccount_token_cleaner.go
Outdated
Show resolved
Hide resolved
|
/test pull-kubernetes-e2e-kind |
I've thought a bit about this and I can't think of a "gentle" way to remove a long-unused token gradually via dry-run or rate-limiting because no one will notice until they're broken and we're talking about something that runs infrequently to even hit this. The rate-limit would have to be order of years for someone to notice in time to matter. I did have one different thought, but it would push out the deletion timeframe. What if tokens that hadn't been used in a year were made invalid for use against the kube-apiserver, but remained in the API. That would force client-side failures, with a cluster-admin able to "reset" a secret by removing the last-used annotation. The pattern this would produce is
A year is a long time to not use a token. I don't have strong opinions about whether the extra step is worth the effort, but it does provide a way to break people so they know they must change, but also provide immediate relief that doesn't require distributing new tokens. That ability to avoid distribution is the only meaningful difference between this approach and telling someone to create a new SA token secret. |
this two-phase deletion approach equals to the existing approach:
does this act as the same as the existing approach where we can set the wait time to be two years? do we think one year is long enough to consider a auto legacy token being unused? i think it is. |
Not quite the same, it makes the revocation reversible... marking a token as invalid without deleting it means that action can be undone without needing to distribute a new credential to the impacted user.
I also don't have strong feelings on this. There's two aspects I'm trying to decide if we should provide:
|
|
that said, I don't think we need those for the alpha... let's add an item to the KEP to resolve this question before graduating to beta |
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
Outdated
Show resolved
Hide resolved
test/integration/serviceaccount/legacy_service_account_token_clean_up_test.go
Outdated
Show resolved
Hide resolved
test/integration/serviceaccount/legacy_service_account_token_clean_up_test.go
Outdated
Show resolved
Hide resolved
test/integration/serviceaccount/legacy_service_account_token_clean_up_test.go
Outdated
Show resolved
Hide resolved
test/integration/serviceaccount/legacy_service_account_token_clean_up_test.go
Show resolved
Hide resolved
|
/lgtm |
|
LGTM label has been added. Git tree hash: 2274f3d84ec93368cd6d5b3db6cf5581ce01289a
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, yt2985 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Start to clean up auto-generated service account token.
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
-[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/2799-reduction-of-secret-based-service-account-token