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.

Sign up for GitHub

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

move BoundServiceAccountTokenVolume to beta #95667

Merged
merged 1 commit into from Feb 18, 2021
Merged

move BoundServiceAccountTokenVolume to beta #95667

merged 1 commit into from Feb 18, 2021

Conversation

zshihang
Copy link
Contributor

@zshihang zshihang commented Oct 17, 2020 •
edited by liggitt

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

graduate feature BoundServiceAccountTokenVolume to beta as the criteria is met.

https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/1205-bound-service-account-tokens#alpha-beta
upgrade test is passing: https://k8s-testgrid.appspot.com/sig-auth-gce#upgrade-tests

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The BoundServiceAccountTokenVolume feature has been promoted to beta, and enabled by default.
* This changes the tokens provided to containers at `/var/run/secrets/kubernetes.io/serviceaccount/token` to be time-limited, auto-refreshed, and invalidated when the containing pod is deleted.
* Clients should reload the token from disk periodically (once per minute is recommended) to ensure they continue to use a valid token. `k8s.io/client-go` version v11.0.0+ and v0.15.0+ reload tokens automatically.
* By default, injected tokens are given an extended lifetime so they remain valid even after a new refreshed token is provided. The metric `serviceaccount_stale_tokens_total` can be used to monitor for workloads that are depending on the extended lifetime and are continuing to use tokens even after a refreshed token is provided to the container. If that metric indicates no existing workloads are depending on extended lifetimes, injected token lifetime can be shortened to 1 hour by starting `kube-apiserver` with `--service-account-extend-token-expiration=false`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/1205-bound-service-account-tokens

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 17, 2020
@zshihang
Copy link
Contributor Author

/cc @liggitt
/cc @mikedanese

@zshihang
Copy link
Contributor Author

/sig auth
/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 17, 2020
@fejta-bot
Copy link

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.

@liggitt
Copy link
Member

liggitt commented Oct 18, 2020

the upgrade CI test results look good, thanks for seeing that through!

  • bazel-test needs default RBAC roles regenerated for the now-enabled system:controller:root-ca-cert-publisher

  • unit, integration, verify tests and kubeadm are not yet passing the required flags for service account token signing, so they are failing with the following error:

service-account-issuer is a required flag when BoundServiceAccountTokenVolume is enabled, --service-account-signing-key-file and --service-account-issuer are required flags

@liggitt
Copy link
Member

liggitt commented Oct 18, 2020

The release note needs to more explicitly indicate what new API server arguments are required (and that the BoundServiceAccountTokenVolume feature can be disabled if desired while in beta in lieu of providing the new arguments)

@zshihang
Copy link
Contributor Author

zshihang commented Oct 19, 2020 •
edited

  • unit, integration, verify tests and kubeadm are not yet passing the required flags for service account token signing, so they are failing with the following error:

service-account-issuer is a required flag when BoundServiceAccountTokenVolume is enabled, --service-account-signing-key-file and --service-account-issuer are required flags

that's what i was confused: those flags should be validated if TokenRequest is enabled but looks like code only requires those flags when BoundServiceAccountTokenVolume is enabled. should we fix the validating logic? ideally, this change should not cause any tests failures because those flags are required by TokenRequest and TokenRequest is enabled by default.

the BoundServiceAccountTokenVolume feature can be disabled if desired while in beta in lieu of providing the new arguments

the apiserver will fail to start if those arguments are missing. how this feature can be disabled by not providing the new arguments? or use AllBeta=true,BoundServiceAccountTokenVolume=false to disable?

@k8s-ci-robot 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. labels Oct 21, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2020
liggitt
liggitt reviewed Nov 4, 2020
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

/milestone v1.21

pkg/kubeapiserver/options/authentication.go Outdated
@@ -165,7 +165,7 @@ func (o *BuiltInAuthenticationOptions) WithRequestHeader() *BuiltInAuthenticatio

// WithServiceAccounts set default value for service account authentication
func (o *BuiltInAuthenticationOptions) WithServiceAccounts() *BuiltInAuthenticationOptions {
o.ServiceAccounts = &ServiceAccountAuthenticationOptions{Lookup: true}
o.ServiceAccounts = &ServiceAccountAuthenticationOptions{Lookup: true, ExtendExpiration: true}
Copy link
Member

Choose a reason for hiding this comment

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

move this to a separate PR for merging in 1.20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cmd/kubeadm/app/phases/controlplane/manifests.go Outdated
@@ -202,6 +202,14 @@ func getAPIServerCommand(cfg *kubeadmapi.ClusterConfiguration, localAPIEndpoint
if cfg.APIServer.ExtraArgs == nil {
cfg.APIServer.ExtraArgs = map[string]string{}
}

// TODO: remove in 1.21. used for smooth transition in 1.19->1.20 upgrades around the BoundServiceAccountTokenVolume feature:
Copy link
Member

Choose a reason for hiding this comment

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

BoundServiceAccountTokenVolume to beta will be held for 1.21, so this change can be dropped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Nov 4, 2020
This was referenced Nov 4, 2020
Closed
Merged
@RA489
Copy link

RA489 commented Nov 5, 2020

/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-bazel-test
/test pull-kubernetes-conformance-kind-ipv6-parallel

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2020
@liggitt
Copy link
Member

liggitt commented Nov 5, 2020

lgtm, will hold for 1.21

@neolit123
Copy link
Member

/remove-area kubeadm

@neolit123 neolit123 mentioned this pull request Dec 3, 2020
Closed
@liggitt
Copy link
Member

liggitt commented Feb 18, 2021

/test all

@liggitt
Copy link
Member

liggitt commented Feb 18, 2021

/hold cancel
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 18, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 Feb 18, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 18, 2021 •
edited

@zshihang: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e 0d35a85e5d1314ef7b2f4b6b1758f44432ccf392 link /test pull-kubernetes-local-e2e

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.

@zshihang
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 7aca497 into kubernetes:master Feb 18, 2021
@liggitt liggitt moved this from In progress to 1.21 - Done in SIG-Auth: Projected Service Account Tokens Feb 23, 2021
This was referenced Mar 6, 2021
Merged
Closed
@zshihang zshihang mentioned this pull request Mar 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@liggitt liggitt liggitt left review comments

@mikedanese mikedanese Awaiting requested review from mikedanese

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG-Auth: Projected Service Account T...
1.21 - Done
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@zshihang @fejta-bot @liggitt @dims @neolit123 @RA489 @k8s-ci-robot @mikedanese