Make bootstrap client cert loading part of rotation #69890

smarterclayton · 2018-10-16T19:41:20Z

Ensure that bootstrap+client-cert rotation in the Kubelet can:

happen in the background so that static pods are launched immediately
collapse down to a single CSR request codepath
reorganize the code to allow future improvements to fetching bootstrap creds

Reorganize how the kubelet client config is determined. If rotation is
off, simplify the code path. If rotation is on, load the config
from disk, and then pass that into the cert manager. The cert manager
creates a client each time it tries to request a new cert.

Preserves existing behavior where:

bootstrap kubeconfig is used if the current kubeconfig is invalid/expired
we create the kubeconfig file based on the bootstrap kubeconfig, pointing to
the location that new client certs will be placed
the newest client cert is used once it has been loaded

reverted in #71173, original release note was:

When a kubelet is using --bootstrap-kubeconfig and certificate rotation, it no longer waits for bootstrap to succeed before launching static pods.

NONE

Fixes #68686

smarterclayton · 2018-10-16T19:42:29Z

/assign @awly

@mikedanese @liggitt as discussed, @aaronlevy will allow bootkube and other self-hosting mechanisms to rely on bootstrapped master nodes and use static pods.

smarterclayton · 2018-10-16T19:58:33Z

I reverted #66056 in this - perhaps we could instead do a more aggressive dial timeout for bootstrap credentials (now that we always do both in the background)?

smarterclayton · 2018-10-16T20:20:44Z

Testing this here and in openshift/origin#21274 so I have a better baseline across different environments

smarterclayton · 2018-10-17T20:07:26Z

There are four core scenarios to test:

rotation on, bootstrap on
1. no client certs, --kubeconfig file doesn't exist -> kubeconfig is created pointed at destination of client certs, cert is refreshed
2. client certs exist, --kubeconfig file doesn't exist -> kubeconfig is created pointing at client certs, cert is not refreshed
3. client certs exist, --kubeconfig exists, certs are valid -> cert is not refreshed until next interval
4. client certs exist, --kubeconfig exists, certs are expired -> static pod starts and cert is refreshed
rotation off, bootstrap on
1. bootstrap cert is expired -> rebootstrap
2. bootstrap file doesn't exist -> exit with error
3. regular kubeconfig exists but is invalid -> exit with error
rotation off, bootstrap off
1. kubeconfig doesn't exist -> exit with error
2. kubeconfig exists but is invalid/expired -> static pods start, errors are logged
rotation on, bootstrap off

awly

Re #66056: it should be fine to remove if we add aggressive timeout and fast retries for initial bootstrap.

For context: it was needed on GCE because network programming can lag behind and bootstrapping would get trapped in a very long timeout, even after master IP becomes reachable from Node. cc @mikedanese

awly · 2018-10-17T22:31:36Z

cmd/kubelet/app/server.go

+			// XXX: When an external bootstrap source is available, it should be possible to always use that source
+			//      to retrieve new credentials.
+			config := certConfig
+			if current != nil {


IIUC, if certificate manager has an expired cert and fails to rotate it, Current() will return that expired cert.
Can we check that current is still valid here and fall back to certConfig if it's not?

Current() returns nil if the cert is expired.

Or at least it should, that was the intended behavior a while back. The transport rotation loop checks for the transition from valid -> invalid and uses that as the "die if this persists"

The cached cert is only updated on successful rotation: http://go/gh/kubernetes/kubernetes/blob/3c493bc95a825f0f6eb38ef78a19811f204053ec/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go#L396
And Current() just returns the cached cert without checking expiry http://go/gh/kubernetes/kubernetes/blob/3c493bc95a825f0f6eb38ef78a19811f204053ec/staging/src/k8s.io/client-go/util/certificate/certificate_manager.go#L204-L208

Ah, I added the filtering to getCurrentCertificateOrBootstrap. There's very little point in the cert manager handing me back an expired cert - can you think of a case where that makes sense? If not, we may want to change the signature and verify callers get the right thing (callers should have to deal with nil in general). Alternatively, we make every caller filter for a valid cert.

I think manager.Current() should always check expiration and return nil when cert is expired.

Specifically, scenario I'm concerned about is:

kubelet has valid bootstrap creds

kubelet bootstraps new client cert with short expiration

manager.Current() starts returning this cert

later, rotation using bootstrapped client cert fails

even later, client cert expires

rotation flow still gets it from manager.Current(), but fails trying to rotate because it's expired

I'd expect rotation to switch back to using bootstrap cert again when active client cert expires.

ok, I agree. I'll make that change and add tests

awly · 2018-10-17T22:33:00Z

pkg/kubelet/certificate/bootstrap/bootstrap.go

+		return clientConfig, clientConfig, nil
+	}
+
+	store, err := certificate.NewFileStore("kubelet-client", certDir, certDir, "", "")


Is this only used to generate pemPath?
moveit down to where it's used.

it's high in the function to fail earlier on local config

awly · 2018-10-17T22:33:12Z

pkg/kubelet/certificate/bootstrap/bootstrap.go

+// kubeconfigPath on disk is populated based on bootstrapPath but pointing to the location of the client cert
+// in certDir. This preserves the historical behavior of bootstrapping where on subsequent restarts the
+// most recent client cert is used to request new client certs instead of the initial token.
+func LoadClientConfig(kubeconfigPath string, bootstrapPath string, certDir string) (certConfig, userConfig *restclient.Config, err error) {


bootstrapPath, certDir string

awly · 2018-10-17T22:34:59Z

cmd/kubelet/app/server.go

+		return &transportConfig, closeAllConns, nil
+	}
+
+	if len(s.BootstrapKubeconfig) > 0 {


Should this get called before starting the cert managed above?

this is in the unmanaged path. In the managed path bootstrap is managed by the manager.

fedebongio · 2018-10-18T20:44:43Z

/assign @caesarxuchao

smarterclayton · 2018-10-23T03:31:18Z

/assign @awly

smarterclayton · 2018-11-07T00:20:37Z

still looking at making the waitForServer path able to have a more aggressive timeout and dial.

smarterclayton · 2018-11-15T22:24:39Z

Since the timeouts seemed prone to follow up, split all that out. This is just pulling bootstrap into rotation, and having m.Current() return nil when the cert is expired.

smarterclayton · 2018-11-16T19:46:45Z

/retest

smarterclayton · 2018-11-16T20:41:55Z

@awly this is back to effectively the original PR you reviewed. Was your previous review sufficient or were you looking for more?

mikedanese

I'm +1 on getting this in to CI and seeing how it goes.

/lgtm
/approve

cmd/kubelet/app/server.go

k8s-ci-robot · 2018-11-17T02:26:20Z

[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.

The pull request process is described here

Needs approval from an approver in each of these files:

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2018-11-17T02:33:09Z

/hold
for the csiClient question

Ensure that bootstrap+clientcert-rotation in the Kubelet can: 1. happen in the background so that static pods aren't blocked by bootstrap 2. collapse down to a single call path for requesting a CSR 3. reorganize the code to allow future flexibility in retrieving bootstrap creds Fetching the first certificate and later certificates when the kubelet is using client rotation and bootstrapping should share the same code path. We also want to start the Kubelet static pod loop before bootstrapping completes. Finally, we want to take an incremental step towards improving how the bootstrap credentials are loaded from disk (potentially allowing for a CLI call to get credentials, or a remote plugin that better integrates with cloud providers or KSMs). Reorganize how the kubelet client config is determined. If rotation is off, simplify the code path. If rotation is on, load the config from disk, and then pass that into the cert manager. The cert manager creates a client each time it tries to request a new cert. Preserve existing behavior where: 1. bootstrap kubeconfig is used if the current kubeconfig is invalid/expired 2. we create the kubeconfig file based on the bootstrap kubeconfig, pointing to the location that new client certs will be placed 3. the newest client cert is used once it has been loaded

Expose both a Stop() method (for cleanup) and a method to force cert rotation, but only expose Stop() on the interface. Verify that we choose the correct client.

k8s-ci-robot · 2018-11-17T02:51:22Z

New changes are detected. LGTM label has been removed.

smarterclayton · 2018-11-17T02:51:33Z

Comments addressed

liggitt · 2018-11-17T03:01:23Z

/hold cancel

smarterclayton · 2018-11-17T03:49:27Z

/retest

k8s-ci-robot · 2018-11-17T07:42:52Z

@smarterclayton: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-integration	`de293b2`	link	`/test pull-kubernetes-integration`
pull-kubernetes-kubemark-e2e-gce-big	`de293b2`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`

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.

smarterclayton · 2018-11-18T00:57:50Z

This was reverted in #71173 due to breaking kubeadm's requirement that --kubeconfig be provided on masters as a high powered credential and cause a rotation of the cert. Restored in #71174 with coverage for kubeadm's flow.

k8s-ci-robot requested review from liggitt and mikedanese October 16, 2018 19:41

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2018

k8s-ci-robot assigned awly Oct 16, 2018

smarterclayton force-pushed the bootstrap_retry branch 2 times, most recently from 2e6a32b to 33ad8ce Compare October 16, 2018 19:56

smarterclayton force-pushed the bootstrap_retry branch 2 times, most recently from 47a2f3d to 3c493bc Compare October 17, 2018 19:52

awly reviewed Oct 17, 2018

View reviewed changes

k8s-ci-robot assigned caesarxuchao Oct 18, 2018

smarterclayton force-pushed the bootstrap_retry branch from 3c493bc to 4e141c8 Compare November 7, 2018 00:17

smarterclayton added the kind/bug Categorizes issue or PR as related to a bug. label Nov 10, 2018

smarterclayton force-pushed the bootstrap_retry branch 2 times, most recently from f6e246b to f008184 Compare November 16, 2018 19:07

k8s-ci-robot assigned mikedanese Nov 17, 2018

mikedanese reviewed Nov 17, 2018

View reviewed changes

cmd/kubelet/app/server.go Show resolved Hide resolved

cmd/kubelet/app/server.go Outdated Show resolved Hide resolved

cmd/kubelet/app/server.go Show resolved Hide resolved

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

mikedanese added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 17, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2018

smarterclayton added 2 commits November 16, 2018 21:50

Ensure the bootstrap rotation code is tested by forcing rotation

de293b2

Expose both a Stop() method (for cleanup) and a method to force cert rotation, but only expose Stop() on the interface. Verify that we choose the correct client.

smarterclayton force-pushed the bootstrap_retry branch from f008184 to de293b2 Compare November 17, 2018 02:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2018

mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2018

k8s-ci-robot merged commit 3be3510 into kubernetes:master Nov 17, 2018

This was referenced Nov 17, 2018

Revert "Make bootstrap client cert loading part of rotation" #71173

Merged

Restore bootstrap in the background with fix to preserve kubeadm behavior #71174

Merged

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 19, 2018

liggitt mentioned this pull request Oct 17, 2022

[WIP]When kubelet enable rotate-certificates and bootstrap, request CSR in foreground #112240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make bootstrap client cert loading part of rotation #69890

Make bootstrap client cert loading part of rotation #69890

smarterclayton commented Oct 16, 2018 •

edited by liggitt

smarterclayton commented Oct 16, 2018

smarterclayton commented Oct 16, 2018

smarterclayton commented Oct 16, 2018

smarterclayton commented Oct 17, 2018 •

edited

awly left a comment

awly Oct 17, 2018

smarterclayton Oct 23, 2018

smarterclayton Oct 23, 2018

awly Oct 23, 2018

smarterclayton Oct 25, 2018

awly Oct 30, 2018

smarterclayton Nov 1, 2018

awly Oct 17, 2018

smarterclayton Oct 23, 2018

awly Oct 17, 2018

awly Oct 17, 2018

smarterclayton Oct 23, 2018

fedebongio commented Oct 18, 2018

smarterclayton commented Oct 23, 2018

smarterclayton commented Nov 7, 2018

smarterclayton commented Nov 15, 2018

smarterclayton commented Nov 16, 2018

smarterclayton commented Nov 16, 2018

mikedanese left a comment

k8s-ci-robot commented Nov 17, 2018

liggitt commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

smarterclayton commented Nov 17, 2018

liggitt commented Nov 17, 2018

smarterclayton commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

smarterclayton commented Nov 18, 2018

Make bootstrap client cert loading part of rotation #69890

Make bootstrap client cert loading part of rotation #69890

Conversation

smarterclayton commented Oct 16, 2018 • edited by liggitt

smarterclayton commented Oct 16, 2018

smarterclayton commented Oct 16, 2018

smarterclayton commented Oct 16, 2018

smarterclayton commented Oct 17, 2018 • edited

awly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedebongio commented Oct 18, 2018

smarterclayton commented Oct 23, 2018

smarterclayton commented Nov 7, 2018

smarterclayton commented Nov 15, 2018

smarterclayton commented Nov 16, 2018

smarterclayton commented Nov 16, 2018

mikedanese left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 17, 2018

liggitt commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

smarterclayton commented Nov 17, 2018

liggitt commented Nov 17, 2018

smarterclayton commented Nov 17, 2018

k8s-ci-robot commented Nov 17, 2018

smarterclayton commented Nov 18, 2018

smarterclayton commented Oct 16, 2018 •

edited by liggitt

smarterclayton commented Oct 17, 2018 •

edited