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
KEP-2799: Reduction of Secret-based Service Account Tokens #2800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi
My feedback overall is about the difference between how this KEP names things vs. the conventions I've inferred from other KEPs / deprecations / annotation names etc.
I've explained my thinking inline. Because some of these names are set in stone right from the alpha phase, I think it's important to consider the naming and see if we want to revise, before this goes into any release.
It's especially important because users may need to change their workloads because of this feature removal.
/assign |
4c6f806
to
4ffdba3
Compare
fbe8b39
to
280a43b
Compare
Hi @ ! 1.24 Enhancements team here. Just checking in as we approach enhancements freeze on 18:00pm PT on Thursday Feb 3rd. I'll mark this as
The status of this enhancement is track as |
done |
keps/sig-auth/2799-reduction-of-secret-based-service-account-token/README.md
Show resolved
Hide resolved
|
||
| Alpha | Beta | GA | | ||
| ----- | ---- | ---- | | ||
| - | 1.24 | 1.25 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe why this starts as beta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because in 1.24, all pods should be admitted in 1.22+ and they should be using bound tokens. this feature gate mostly guard deletions of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because in 1.24, all pods should be admitted in 1.22+ and they should be using bound tokens. this feature gate mostly guard deletions of code.
I meant in the KEP itself. It's unusual to start featuregates in beta. Not unheard of, but worthy of explaining in the doc why it's more reasonable to start this one at beta than at alpha. Why is it more reasonable to start this as opt-out than as opt-in?
this feature gate mostly guard deletions of code.
Feature gates guard changes in behavior, whether deletion or addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
keps/sig-auth/2799-reduction-of-secret-based-service-account-token/README.md
Show resolved
Hide resolved
|
||
###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)? | ||
|
||
yes for all feature gates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do all eight combinations of these featuregates make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each feature gate is independent from each other. any combination would continue to work.
keps/sig-auth/2799-reduction-of-secret-based-service-account-token/README.md
Show resolved
Hide resolved
Token Controller starts to remove unused auto-generated secrets (secrets | ||
bi-directionally referenced by the service account) and not mounted by pods. | ||
|
||
When this feature is Beta and enabled by default, delete secrets iff it is over |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the delete contingent on the "tracked-since" configmap? You have to be sure that the last-used seen on service-account secrets is still valid somehow, otherwise disabling the LegacyServiceAccountTokenTracking featuregate and enabling LegacyServiceAccountTokenCleanUp becomes a combination that doesn't make sense to allow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, tracked-since
is required in determining when the secret is lastly used. in other words, if LegacyServiceAccountTokenTracking is disabled, LegacyServiceAccountTokenCleanUp would not clean up tokens because it cannot determine last-used
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt, zshihang 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 |
/assign @liggitt
/assign @mikedanese
/sig auth
@kubernetes/sig-auth-pr-reviews