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

Jump to bottom

KEP-2799: Reduction of Secret-based Service Account Tokens #2800

Merged
merged 1 commit into from Jan 27, 2022
Merged

KEP-2799: Reduction of Secret-based Service Account Tokens #2800

merged 1 commit into from Jan 27, 2022

Conversation

zshihang
Copy link
Contributor

/assign @liggitt
/assign @mikedanese
/sig auth
@kubernetes/sig-auth-pr-reviews

@k8s-ci-robot k8s-ci-robot added 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. labels Jun 25, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 25, 2021
sftim
sftim reviewed Jul 1, 2021
View reviewed changes
Copy link
Contributor

@sftim sftim left a 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.

keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/kep.yaml Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/kep.yaml Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
@enj enj added this to Needs Triage in SIG Auth Old Jul 6, 2021
@liggitt liggitt moved this from Needs Triage to KEP Backlog in SIG Auth Old Jul 12, 2021
@enj
Copy link
Member

enj commented Sep 7, 2021

/assign

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 9, 2021
liggitt
liggitt reviewed Sep 10, 2021
View reviewed changes
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2799-token-controller-deprecation/README.md Outdated Show resolved Hide resolved
@zshihang zshihang changed the title KEP-2799: Token Controller Deprecation KEP-2799: Reduction of Secret-based Service Account Tokens Sep 10, 2021
@liggitt liggitt mentioned this pull request Sep 12, 2021
Merged
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2022
@liggitt liggitt mentioned this pull request Jan 19, 2022
Open
39 tasks
@gracenng
Copy link
Member

gracenng commented Jan 20, 2022 •
edited

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 beta while awaiting your confirmation
Here’s where this enhancement currently stands:

  • Updated KEP file using the latest template has been merged into the k/enhancements repo.
  • KEP status is marked as implementable for this release with latest-milestone: 1.24
  • KEP has a test plan section filled out.
  • KEP has up to date graduation criteria.
  • KEP has a production readiness review that has been completed and merged into k/enhancements.

The status of this enhancement is track as at risk
Thanks!

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2022
@zshihang
Copy link
Contributor Author

Upgrade / Downgrade Strategy 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 beta while awaiting your confirmation Here’s where this enhancement currently stands:

  • Updated KEP file using the latest template has been merged into the k/enhancements repo.
  • KEP status is marked as implementable for this release with latest-milestone: 1.24
  • KEP has a test plan section filled out.
  • KEP has up to date graduation criteria.
  • KEP has a production readiness review that has been completed and merged into k/enhancements.

The status of this enhancement is track as at risk Thanks!

done

deads2k
deads2k reviewed Jan 26, 2022
View reviewed changes
keps/sig-auth/2799-reduction-of-secret-based-service-account-token/README.md

| Alpha | Beta | GA |
| ----- | ---- | ---- |
| - | 1.24 | 1.25 |
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@deads2k deads2k Jan 26, 2022 •
edited

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

deads2k
deads2k reviewed Jan 26, 2022
View reviewed changes
keps/sig-auth/2799-reduction-of-secret-based-service-account-token/README.md

###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?

yes for all feature gates.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

deads2k
deads2k reviewed Jan 26, 2022
View reviewed changes
keps/sig-auth/2799-reduction-of-secret-based-service-account-token/README.md
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
Copy link
Contributor

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.

Copy link
Contributor Author

@zshihang zshihang Jan 26, 2022 •
edited

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

@deads2k
Copy link
Contributor

deads2k commented Jan 27, 2022

/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 Jan 27, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2022
@k8s-ci-robot k8s-ci-robot merged commit 1a356e3 into kubernetes:master Jan 27, 2022
SIG Auth Old automation moved this from Pending other SIGs to Closed / Done Jan 27, 2022
@zshihang zshihang mentioned this pull request Feb 23, 2022
Merged
@zshihang zshihang mentioned this pull request Mar 21, 2022
Merged
@reylejano reylejano mentioned this pull request Mar 24, 2022
Merged
@zshihang zshihang mentioned this pull request Mar 30, 2022
Closed
@sftim sftim mentioned this pull request Mar 31, 2022
Closed
@manusa manusa mentioned this pull request May 10, 2022
Merged
@yuanchen8911 yuanchen8911 mentioned this pull request Jun 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@liggitt liggitt liggitt left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@kfox1111 kfox1111 kfox1111 left review comments

@mikedanese mikedanese mikedanese left review comments

@deads2k deads2k deads2k left review comments

@sftim sftim sftim left review comments

@enj enj Awaiting requested review from enj

Assignees

@liggitt liggitt

@mikedanese mikedanese

@enj enj

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
SIG Auth
Archived in project
SIG Auth Old
Closed / Done
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet