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 rotating kubelet client certificate to beta. #51045

Merged
merged 1 commit into from
Aug 30, 2017
Merged

Move rotating kubelet client certificate to beta. #51045

merged 1 commit into from
Aug 30, 2017

Conversation

jcbsmpsn
Copy link
Contributor 

Release the kubelet client certificate rotation as beta.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 21, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 21, 2017
ericchiang
ericchiang reviewed Aug 21, 2017
View reviewed changes
pkg/features/kube_features.go Outdated
@@ -152,7 +152,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
Accelerators: {Default: false, PreRelease: utilfeature.Alpha},
TaintBasedEvictions: {Default: false, PreRelease: utilfeature.Alpha},
RotateKubeletServerCertificate: {Default: false, PreRelease: utilfeature.Alpha},
RotateKubeletClientCertificate: {Default: false, PreRelease: utilfeature.Alpha},
RotateKubeletClientCertificate: {Default: false, PreRelease: utilfeature.Beta},
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this default to true?

Copy link
Contributor Author

@jcbsmpsn jcbsmpsn Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So, I'd like to set the default value for the flag to true, which would turn this feature on by default when it goes GA. Leaving this value false which the feature is beta would mean people won't get the beta feature enabled by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the timeline for GA? I'd imagine most setups will still use static TLS for the foreseeable future and it seems odd to turn it on by default if a cluster isn't using it.

Copy link
Member

Choose a reason for hiding this comment

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

what is the behavior if the controller manager doesn't have a signing CA set up? does this degrade gracefully if unable to submit the CSR, or if the submitted CSR is never approved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mis-understood the behavior of the signing CA. I'm going to turn the flag to false by default, and put the beta feature to true.

ericchiang
ericchiang reviewed Aug 21, 2017
View reviewed changes
cmd/kubelet/app/options/options.go Outdated
@@ -203,6 +205,8 @@ func (f *KubeletFlags) AddFlags(fs *pflag.FlagSet) {
"If the file specified by --kubeconfig does not exist, the bootstrap kubeconfig is used to request a client certificate from the API server. "+
"On success, a kubeconfig file referencing the generated client certificate and key is written to the path specified by --kubeconfig. "+
"The client certificate and key file will be stored in the directory pointed by --cert-dir.")
fs.BoolVar(&f.RotateCertificates, "rotate-certificates", f.RotateCertificates, "<Warning: Beta feature> Auto rotate the kubelet client certificates by requesting new certificates from the kube-apiserver when the certificate expiration approaches.")
fs.MarkHidden("rotate-certificates") // Mark this flag as hidden because it is still in Beta.
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this pattern been used elsewhere? I don't remember us hiding other flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know of it existing anywhere else. You think just the warning text and not hiding it is the best way to go?

@ericchiang
Copy link
Contributor

cc @kubernetes/sig-auth-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Aug 21, 2017
@jcbsmpsn jcbsmpsn force-pushed the rotate-kubelet-client-certificate-beta branch 4 times, most recently from 964ab0a to e74416b Compare August 22, 2017 20:34
@ericchiang
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2017
@ericchiang
Copy link
Contributor

/assign @dchen1107

For approval.

@mikedanese
Copy link
Member

/approve no-issue

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@jcbsmpsn jcbsmpsn force-pushed the rotate-kubelet-client-certificate-beta branch from e74416b to b11bdc0 Compare August 28, 2017 17:00
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2017
@jcbsmpsn
Copy link
Contributor Author

jcbsmpsn commented Aug 28, 2017 •
edited
Loading

/assign @smarterclayton Do you have any time to review this for approval?

@smarterclayton
Copy link
Contributor

/approve

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 29, 2017
@jcbsmpsn jcbsmpsn force-pushed the rotate-kubelet-client-certificate-beta branch from b11bdc0 to a0d81d1 Compare August 29, 2017 16:26
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017
@ericchiang
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, jcbsmpsn, mikedanese, smarterclayton

Associated issue requirement bypassed by: mikedanese

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49961, 50005, 50738, 51045, 49927)

@k8s-github-robot k8s-github-robot merged commit bb8fed8 into kubernetes:master Aug 30, 2017
@ericchiang ericchiang mentioned this pull request Sep 8, 2017
Closed
This was referenced Oct 5, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericchiang ericchiang ericchiang left review comments

@liggitt liggitt liggitt left review comments

Assignees

@lavalamp lavalamp

@smarterclayton smarterclayton

@ericchiang ericchiang

@dchen1107 dchen1107

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jcbsmpsn @ericchiang @mikedanese @k8s-github-robot @smarterclayton @liggitt @lavalamp @dchen1107 @k8s-ci-robot