Only rotate certificates in the background #58930

smarterclayton · 2018-01-28T19:35:33Z

Change the Kubelet to not block until the first certs have rotated (we didn't act on it anyway) and fall back to the bootstrap cert if the most recent rotated cert is expired on startup.

The certificate manager originally had a "block on startup" rotation behavior to ensure at least one rotation happened on startup. However, since rotation may not succeed within the first time window the code was changed to simply print the error rather than return it. This meant that the blocking rotation has no purpose - it cannot cause the kubelet to fail, and it does block the kubelet from starting static pods before the api server becomes available.

The current block behavior causes a bootstrapped kubelet that is also set to run static pods to wait several minutes before actually launching the static pods, which means self-hosted masters using static pods have a pointless delay on startup.

Since blocking rotation has no benefit and can't actually fail startup, this commit removes the blocking behavior and simplifies the code at the same time. The goroutine for rotation now completely owns the deadline, the shouldRotate() method is removed, and the method that sets rotationDeadline now returns it. We also explicitly guard against a negative sleep interval and omit the message.

Should have no impact on bootstrapping except the removal of a long delay on startup before static pods start.

The other change is that an expired certificate from the cert manager is not considered a valid cert, which triggers an immediate rotation. This causes the cert manager to fall back to the original bootstrap certificate until a new certificate is issued. This allows the bootstrap certificate on masters to be "higher powered" and allow the node to function prior to initial approval, which means someone configuring the masters with a pre-generated client cert can be guaranteed that the kubelet will be able to communicate to report self-hosted static pod status, even if the first client rotation hasn't happened. This makes master self-hosting more predictable for static configuration environments.

When using client or server certificate rotation, the Kubelet will no longer wait until the initial rotation succeeds or fails before starting static pods.  This makes running self-hosted masters with rotation more predictable.

smarterclayton · 2018-01-28T19:40:56Z

@kubernetes/sig-cluster-lifecycle-pr-reviews @mikedanese @liggitt @deads2k

In openshift we are using node bootstrapping and cert rotation (client and server certs). As I was going to enable static pods for master configuration, I realized that the foreground pause for bootstrapping caused a pointless and long delay (because Start() on cert managers won't cause the Kubelet to fail to startup, just get printed to logs).

This removes the foreground rotation but preserves all the error handling behavior, which then means that a master node configured for both bootstrapping AND static pods won't have an arbitrarily long wait before static pods start in some cases (before the client eventually times out and errors). It changes no behavior observable to an outsider except the following:

static pods can now start before client cert bootstrapping finishes (good)
the kubelet app server can start before a server cert bootstrapping finishes (different error to clients, server returns an error instead of returning connection refused)

For 2, since the only difference is error type (ECONNREFUSED vs a TLS specific error) I don't think there's a meaningful impact or a regression in outcome. The underlying intent I think has always been for bootstrapping to be backgroundable (certainly we treat the kubelet server startup as a bunch of background loops rather than as a single process).

This could be a backport candidate for at least 1.9 if we want to make self hosting less annoying to 1.9 users.

The certificate manager originally had a "block on startup" rotation behavior to ensure at least one rotation happened on startup. However, since rotation may not succeed within the first time window the code was changed to simply print the error rather than return it. This meant that the blocking rotation has no purpose - it cannot cause the kubelet to fail, and it *does* block the kubelet from starting static pods before the api server becomes available. The current block behavior causes a bootstrapped kubelet that is also set to run static pods to wait several minutes before actually launching the static pods, which means self-hosted masters using static pods have a pointless delay on startup. Since blocking rotation has no benefit and can't actually fail startup, this commit removes the blocking behavior and simplifies the code at the same time. The goroutine for rotation now completely owns the deadline, the shouldRotate() method is removed, and the method that sets rotationDeadline now returns it. We also explicitly guard against a negative sleep interval and omit the message. Should have no impact on bootstrapping except the removal of a long delay on startup before static pods start. Also add a guard condition where if the current cert in the store is expired, we fall back to the bootstrap cert initially (we use the bootstrap cert to communicate with the server). This is consistent with when we don't have a cert yet.

smarterclayton · 2018-01-30T18:13:52Z

@timothysc this looks like the minimal thing I needed to get static pods working with bootstrapping. I think it's generally relevant for everyone.

mikedanese · 2018-01-31T00:59:40Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

 // current certificate should be rotated, 80%+/-10% of the expiration of the
 // certificate.
-func (m *manager) setRotationDeadline() {
+func (m *manager) nextRotationDeadline() time.Time {
+	// forceRotation is not protected by locks


why? comment could use some justification.

I was probably being overzealous, the only thing protected by certAccessLock is... cert. Before, there was a set of local vars that were called in two different goroutines, now everything is in a single goroutine. I can remove the comment.

mikedanese · 2018-01-31T01:08:32Z

change seems reasonable. it might cause some spurious authorization errors early in kubelet boot but that doesn't seem like an issue.

smarterclayton · 2018-01-31T06:13:42Z

And we will kill ourselves after X min if we can't get a client cert (crash loop) which gives time for static pods to init

timothysc · 2018-01-31T16:15:21Z

seems reasonable to me. @jbeda @mattmoyer - thoughts?

mikedanese · 2018-01-31T22:38:04Z

LGTM
/approve

mikedanese · 2018-02-01T01:02:33Z

/lgtm

k8s-ci-robot · 2018-02-01T01:03:03Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

~~pkg/kubelet/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/client-go/util/certificate/OWNERS~~ [mikedanese,smarterclayton]

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 · 2018-02-01T19:24:02Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-01T20:05:14Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 28, 2018

k8s-ci-robot requested review from dashpole and mikedanese January 28, 2018 19:35

k8s-ci-robot added 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. labels Jan 28, 2018

smarterclayton force-pushed the background_rotate branch from ffab1fd to 44493de Compare January 28, 2018 22:49

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 Jan 28, 2018

mikedanese self-assigned this Jan 30, 2018

mikedanese reviewed Jan 31, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2018

k8s-github-robot merged commit 06472a0 into kubernetes:master Feb 1, 2018

smarterclayton mentioned this pull request Feb 4, 2018

Cap how long the kubelet waits when it has no client cert #59316

Merged

liggitt mentioned this pull request Nov 24, 2020

Improve the documentation for "RotateCertificates" config value #95546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only rotate certificates in the background #58930

Only rotate certificates in the background #58930

smarterclayton commented Jan 28, 2018 •

edited

smarterclayton commented Jan 28, 2018 •

edited

smarterclayton commented Jan 30, 2018

mikedanese Jan 31, 2018 •

edited

smarterclayton Jan 31, 2018

mikedanese commented Jan 31, 2018

smarterclayton commented Jan 31, 2018

timothysc commented Jan 31, 2018

mikedanese commented Jan 31, 2018

mikedanese commented Feb 1, 2018

k8s-ci-robot commented Feb 1, 2018

k8s-github-robot commented Feb 1, 2018

k8s-github-robot commented Feb 1, 2018

Only rotate certificates in the background #58930

Only rotate certificates in the background #58930

Conversation

smarterclayton commented Jan 28, 2018 • edited

smarterclayton commented Jan 28, 2018 • edited

smarterclayton commented Jan 30, 2018

mikedanese Jan 31, 2018 • edited

Choose a reason for hiding this comment

smarterclayton Jan 31, 2018

Choose a reason for hiding this comment

mikedanese commented Jan 31, 2018

smarterclayton commented Jan 31, 2018

timothysc commented Jan 31, 2018

mikedanese commented Jan 31, 2018

mikedanese commented Feb 1, 2018

k8s-ci-robot commented Feb 1, 2018

k8s-github-robot commented Feb 1, 2018

k8s-github-robot commented Feb 1, 2018

smarterclayton commented Jan 28, 2018 •

edited

smarterclayton commented Jan 28, 2018 •

edited

mikedanese Jan 31, 2018 •

edited