Change default --cert-dir for kubelet to a non-transient location #53317

liggitt · 2017-10-02T03:00:11Z

The default kubelet --cert-dir location is /var/run/kubernetes, which is automatically erased on reboot on many platforms. As of 1.8.0, kubelet TLS bootstrapping and cert rotation now persist files in --cert-dir, this should default to a non-transient location. Default it to the pki subfolder of the default --root-dir Fixes #53288

Additionally, since kubeadm expects a running (albeit crashlooping) kubelet prior to running kubeadm init or kubeadm join, and was using the default --root-dir of /var/lib/kubelet, it should not expect that folder to be empty as a pre-init check. Fixes #53356

kubelet `--cert-dir` now defaults to `/var/lib/kubelet/pki`, in order to ensure bootstrapped and rotated certificates persist beyond a reboot. resolves an issue in kubeadm with false-positive `/var/lib/kubelet is not empty` message during pre-flight checks

jcbsmpsn · 2017-10-02T16:07:04Z

/lgtm

liggitt · 2017-10-02T17:20:30Z

/assign @derekwaynecarr @dchen1107
for approval of kubelet --cert-dir flag default value change

liggitt · 2017-10-03T05:46:27Z

/test pull-kubernetes-e2e-kubeadm-gce

liggitt · 2017-10-03T13:30:32Z

@kubernetes/sig-cluster-lifecycle-bugs @luxas PTAL at the kubeadm pre-init check change

derekwaynecarr · 2017-10-03T15:53:01Z

/approve

liggitt · 2017-10-03T15:53:55Z

/assign @mikedanese @luxas
for approval of kubeadm pre-init check change

kad · 2017-10-03T16:44:25Z

cmd/kubeadm/app/preflight/checks.go

@@ -650,7 +650,6 @@ func RunInitMasterChecks(cfg *kubeadmapi.MasterConfiguration) error {
 		PortOpenCheck{port: 10252},
 		HTTPProxyCheck{Proto: "https", Host: cfg.API.AdvertiseAddress, Port: int(cfg.API.BindPort)},
 		DirAvailableCheck{Path: filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.ManifestsSubDirName)},
-		DirAvailableCheck{Path: "/var/lib/kubelet"},


I think it will be a lot better if DirAvailableCheck will be enhanced to do filepath.Walk() and see if there are files present. It should fail check only if directory contains files, but should tolerate if there are empty sub-dirs.

It should fail check only if directory contains files, but should tolerate if there are empty sub-dirs.

not really... the kubelet is running in the background, so kubeadm should not expect its runtime dir to be empty.

Preflight checks are executed at the time when kubelet has no valid configuration, so should not be really running. And even if it running, it should be nothing in /var/lib/kubelet/pods, which might mean leftovers from previous configurations if kubeadm reset was not properly executed.

As alternative, we can add check that /var/lib/kubelet/pods and /var/lib/kubelet/pki are empty.

@luxas asked that rather than remove these, I change these to check the /var/lib/kubelet/pods subfolder specifically

@luxas asked that rather than remove these, I change these to check the /var/lib/kubelet/pods subfolder specifically

actually, I don't think that's a good idea... the kubelet owns the runtime dir. kubeadm can check the paths kubeadm writes to (pod manifests, etc), but shouldn't make assumptions about what the kubelet writes to its runtime dir. I think this is good as-is.

What would happen on nodes, where there are no static manifests, but kublet was not properly reset and still running some pods and having certs from previous node setup ?

As alternative, we can add check that /var/lib/kubelet/pods and /var/lib/kubelet/pki are empty.

/var/lib/kubelet/pki will not be empty if the kubelet is crashlooping in the background... it generates serving certificates there

I'm ok for now to remove that directory check, as files are generated by crashlooping kubelet. In 1.9 I think we need to have separate issue that would be handling kubelet start/stop in better way in init/join commands. Right now it seems that kubeadm has even more issues, like kubelet will not be started if user did kubeadm reset then kubeadm join --skip-preflight-checks.

timothysc

/lgtm but @luxas for approve.

mikedanese · 2017-10-03T16:55:07Z

/approve

mikedanese · 2017-10-03T16:57:48Z

/hold for lucas

liggitt · 2017-10-03T18:12:58Z

discussed with @luxas on slack about his suggestion to narrow the preflight check to /var/lib/kubelet/pods.

I think that introspecting the kubelet's runtime dir in a preflight is a fragile check for kubeadm to make and will lead to similar broken assumptions in the future. Would like feedback on that from representatives from sig-node (@derekwaynecarr) and sig-cluster-lifecycle (@mikedanese / @timothysc).

If the consensus is to remove the check, @luxas said he is fine with that.

If the consensus is to continue introspecting the runtime dir and just narrow the check, I can make that change as well.

We await your feedback.

mikedanese · 2017-10-03T20:35:58Z

I agree that broad preflight checks like this are bound to break. Kubelet fs layout is not a stable API and there are many reasons this directory wouldn't be empty. If we want to prevent kubeadm from installing over an already init'd node we should write a file that is deleted during kubeadm reset. If we want to check that the kubelet is installed, we should make sure that a kubelet process is running.

jbeda · 2017-10-03T21:10:17Z

It is hard to check if the kubelet is running right now as it crash loops.

For 1.9 to really address this we'd have to have more formal communications between the kubelet and kubeadm. That comes with Kubelet Dynamic Config. We should also have a way to reliably "clean" the kubelet config to reset it on reinstall or when things are busted. If we can't just delete a directory then there should be some other mechanism to do that.

It feels like we need a design doc on the ideal work flow for kubelet/kubeadm for 1.9 that is shared across SIG-cluster-lifecycle and SIG-node to help avoid these types of issues in the future.

liggitt · 2017-10-04T01:20:01Z

Talked with @derekwaynecarr as well and he agreed it didn't make sense for kubeadm to introspect the runtime dir. Sounds like there is consensus to remove this preflight check.

@mikedanese:
If we want to prevent kubeadm from installing over an already init'd node we should write a file that is deleted during kubeadm reset

That's already done... kubeadm init and kubeadm join preflights both fail if the pod manifest dir contains existing manifests. That's populated on init/join, and cleaned up on reset.

@jbeda:
We should also have a way to reliably "clean" the kubelet config to reset it on reinstall or when things are busted. If we can't just delete a directory then there should be some other mechanism to do that.

kubeadm reset shuts down the kubelet service and removes the /var/lib/kubelet dir, which works fine. Removing the --root-dir given to the kubelet as part of cleanup/reset seems fine. Assumptions about the internal structure of it was the issue.

@jbeda:
For 1.9 to really address this we'd have to have more formal communications between the kubelet and kubeadm. That comes with Kubelet Dynamic Config.
...
It feels like we need a design doc on the ideal work flow for kubelet/kubeadm for 1.9 that is shared across SIG-cluster-lifecycle and SIG-node

The "API" boundary here should be the config the kubelet exposes (via flags, as today, or via config objects/files, as in dynamic config)

mikedanese · 2017-10-04T16:35:35Z

/unhold

mikedanese · 2017-10-04T16:35:42Z

/lgtm

k8s-github-robot · 2017-10-04T16:36:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, jcbsmpsn, liggitt, mikedanese, timothysc

Associated issue: 53288

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

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

~~cmd/kubeadm/OWNERS~~ [mikedanese]
~~cmd/kubelet/OWNERS~~ [derekwaynecarr,mikedanese]

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 · 2017-10-04T17:32:44Z

Automatic merge from submit-queue (batch tested with PRs 53317, 52186). If you want to cherry-pick this change to another branch, please follow the instructions here.

…7-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #53317 Cherry pick of #53317 on release-1.8. #53317: Change default --cert-dir for kubelet to a non-transient

k8s-cherrypick-bot · 2017-10-06T00:20:17Z

Commit found in the "release-1.8" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…kubernetes#53317

k8s-github-robot assigned vishh and mtaufen Oct 2, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed 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 Oct 2, 2017

liggitt assigned luxas and jcbsmpsn and unassigned vishh Oct 2, 2017

liggitt added this to the v1.8 milestone Oct 2, 2017

liggitt mentioned this pull request Oct 2, 2017

TLS-bootstrapped kubelet loses client certs after reboot, node stays on NotReady status #53288

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2017

k8s-ci-robot assigned dchen1107 and derekwaynecarr Oct 2, 2017

liggitt force-pushed the fix-kubelet-cert-dir branch from 1a953eb to 540516b Compare October 3, 2017 06:26

Change default --cert-dir for kubelet to a non-transient location

8c25265

liggitt force-pushed the fix-kubelet-cert-dir branch from 540516b to 8c25265 Compare October 3, 2017 06:26

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017

liggitt mentioned this pull request Oct 3, 2017

kubeadm 1.8.0 init fails with "/var/lib/kubelet is not empty" #53356

Closed

liggitt added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Oct 3, 2017

k8s-ci-robot assigned mikedanese Oct 3, 2017

liggitt unassigned mtaufen Oct 3, 2017

kad reviewed Oct 3, 2017

View reviewed changes

timothysc reviewed Oct 3, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2017

kad mentioned this pull request Oct 4, 2017

kubeadm preflight-checks on /var/lib/kubelet/ kubernetes/kubeadm#482

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017

k8s-github-robot merged commit 0690b79 into kubernetes:master Oct 4, 2017

liggitt mentioned this pull request Oct 4, 2017

Automated cherry pick of #53317 #53448

Merged

liggitt deleted the fix-kubelet-cert-dir branch October 4, 2017 17:48

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 6, 2017

jamiehannaford mentioned this pull request Oct 10, 2017

fresh kubeadm 1.5 install fails due to existing /var/lib/kubelet kubernetes/kubeadm#262

Closed

BugRoger added a commit to sapcc/kubernikus that referenced this pull request Oct 13, 2017

makes kubelet bootstrap reboot safe. fixes #41. see also: kubernetes/…

e17c9c1

…kubernetes#53317

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default --cert-dir for kubelet to a non-transient location #53317

Change default --cert-dir for kubelet to a non-transient location #53317

liggitt commented Oct 2, 2017 •

edited by jpbetz

Loading

jcbsmpsn commented Oct 2, 2017

liggitt commented Oct 2, 2017

liggitt commented Oct 3, 2017

liggitt commented Oct 3, 2017

derekwaynecarr commented Oct 3, 2017

liggitt commented Oct 3, 2017

kad Oct 3, 2017

liggitt Oct 3, 2017

kad Oct 3, 2017

liggitt Oct 3, 2017 •

edited

Loading

liggitt Oct 3, 2017 •

edited

Loading

kad Oct 3, 2017

liggitt Oct 3, 2017

kad Oct 3, 2017

timothysc left a comment

mikedanese commented Oct 3, 2017

mikedanese commented Oct 3, 2017

liggitt commented Oct 3, 2017

mikedanese commented Oct 3, 2017 •

edited

Loading

jbeda commented Oct 3, 2017

liggitt commented Oct 4, 2017

mikedanese commented Oct 4, 2017

mikedanese commented Oct 4, 2017

k8s-github-robot commented Oct 4, 2017

k8s-github-robot commented Oct 4, 2017

k8s-cherrypick-bot commented Oct 6, 2017

Change default --cert-dir for kubelet to a non-transient location #53317

Change default --cert-dir for kubelet to a non-transient location #53317

Conversation

liggitt commented Oct 2, 2017 • edited by jpbetz Loading

jcbsmpsn commented Oct 2, 2017

liggitt commented Oct 2, 2017

liggitt commented Oct 3, 2017

liggitt commented Oct 3, 2017

derekwaynecarr commented Oct 3, 2017

liggitt commented Oct 3, 2017

kad Oct 3, 2017

Choose a reason for hiding this comment

liggitt Oct 3, 2017

Choose a reason for hiding this comment

kad Oct 3, 2017

Choose a reason for hiding this comment

liggitt Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

kad Oct 3, 2017

Choose a reason for hiding this comment

liggitt Oct 3, 2017

Choose a reason for hiding this comment

kad Oct 3, 2017

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

mikedanese commented Oct 3, 2017

mikedanese commented Oct 3, 2017

liggitt commented Oct 3, 2017

mikedanese commented Oct 3, 2017 • edited Loading

jbeda commented Oct 3, 2017

liggitt commented Oct 4, 2017

mikedanese commented Oct 4, 2017

mikedanese commented Oct 4, 2017

k8s-github-robot commented Oct 4, 2017

k8s-github-robot commented Oct 4, 2017

k8s-cherrypick-bot commented Oct 6, 2017

liggitt commented Oct 2, 2017 •

edited by jpbetz

Loading

liggitt Oct 3, 2017 •

edited

Loading

liggitt Oct 3, 2017 •

edited

Loading

mikedanese commented Oct 3, 2017 •

edited

Loading