recommend draining the node before updating kubelet

[tzneal]

The Kubernetes docs state that minor upgrades of kubelet require draining the node:

If you are performing a minor version upgrade for any kubelet, you must first drain the node (or nodes) that you are upgrading.

It’s also been stated, but not in documentation that patch upgrades don’t require draining the node:

in place minor version kubelet upgrades are not supported without draining. I think in place patch version upgrades are.

My contention is that currently in-place patch upgrades of kubelet are unsafe and we should clarify the K8s documentation to explicitly state that you must drain the node in all circumstances when updating kubelet.

Why?

I suspect there are other examples of when in-place kubelet upgrades fail, but the most recent one I've found is a failure to resolve a CVE for running pods. A recent bug in kubelet allowed pods to bypass seccomp enforcement. A CVE (CVE-2023-2431) was published and the announcement was sent to the community. A fix for this CVE was backported into the 1.27.2 patch release of kubelet.

Without this docs change, users may assume that in-place updating of kubelet will resolve this CVE. It will, but only for newly admitted pods. An in-place update will not terminate any running pods allowing them to continue to bypass seccomp enforcement.

To demonstrate the failure, I launched a pod with an empty string localhostProfile on a node with kubelet 1.27.0. I then stopped kubelet and replaced its binary with the 1.27.6 patch version of kubelet before restarting it. The pod is still running on the node with no seccomp enforcement, but any vulnerability scans which look at the kubelet version will indicate that it’s patched and not susceptible to the CVE.

$ kubectl get pod test-pod -o json | jq '{status:  .status.phase, seccomp: .spec.containers[0].securityContext, nodeName: .spec.nodeName}'
{
  "status": "Running",
  "seccomp": {
    "seccompProfile": {
      "localhostProfile": "",
      "type": "Localhost"
    }
  },
  "nodeName": "i-05a72568b9cceb470.us-west-2.compute.internal"
}

$ kubectl get node i-05a72568b9cceb470.us-west-2.compute.internal
NAME                                             STATUS   ROLES    AGE     VERSION
i-05a72568b9cceb470.us-west-2.compute.internal   Ready    <none>   9m50s   v1.27.6

[tzneal]

/cc @micahhausler
/cc @dims

[dims]

/assign @derekwaynecarr @SergeyKanzhelev @mrunalp

[tzneal]

/cc @liggitt

[liggitt]

I don't think we should say this here or require this. There are two aspects:

1. kubelet compatibility
2. effectively fixing bugs

This document is focused on kubelet compatibility. Kubelet should work properly when restarted against state persisted by another patch version of the kubelet.

Fixing some bugs require restarting containers. That seems like a useful thing to note in releases where a bugfix requiring a container restart is released, but I would not impose that requirement on all patch updates.

[tzneal]

My concern is that, for this example, there are theoretically in the wild pods that are running with no seccomp enforcement but with a known "good" kubelet version even though users followed the current documentation which doesn't suggest that a node drain is necessary.

That seems like a useful thing to note in releases where a bugfix requiring a container restart is released, but I would not impose that requirement on all patch updates.

I think a careful evaluation of the question of "Will running containers require a restart?" for every backported fix is possible, but its a much larger burden than recommending draining the node in all cases. For example, it was missed in this case.

Users can still not drain the node if they choose, but its a risk assessment they make at that point.

How do you feel about suggesting draining the node, something like:

- Before performing a minor version `kubelet` upgrade, [drain](/docs/tasks/administer-cluster/safely-drain-node/) pods from that node.
- In-place minor version `kubelet` upgrades are not supported.
- For in-place patch `kubelet` upgrades, draining the node is the safest approach but not strictly necessary.

[liggitt]

I think we should keep this document focused on the kubelet compatibility requirements, and make it unambiguous that kubelet is compatible with runtime state persisted by other patch versions within the same minor.

[sftim]

@tzneal what are you plans for this PR, given the feedback so far?

[tzneal]

@tzneal what are you plans for this PR, given the feedback so far?

I'm going to find a different place in the docs to put this wording. I don't think that the danger of in-place updates is expressed anywhere currently in the documentation which leaves users vulnerable to CVEs they think they've mitigated.

[tzneal]

Added a warning in the upgrade guide that draining the node may be necessary to resolve CVEs or bugs.

[netlify]

✅ Pull request preview available for checking

Built without sensitive environment variables

Name	Link
🔨 Latest commit	20cfb80
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/650b7323e1631800089fb129
😎 Deploy Preview	https://deploy-preview-43053--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[sftim]

/lgtm

You could also propose a change to https://kubernetes.io/docs/concepts/security/security-checklist/ (maybe another PR, I'm not sure what works best).

I'm thinking of adding an item to say [words to the effect] that there are no nodes where:

• some Pod x is running
• the version of the kubelet that started Pod x is more than one minor version less than the cluster's version of Kubernetes

Hard to express though, isn't it? I initially assumed that systemctl stop kubelet would shut down workloads but that's not the case, and this is moderately surprising. So it's worth documenting a bit more IMO.

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: d44750628ce2b4b7ec89104a489485252a3d02a4

[tzneal]

Hard to express though, isn't it? I initially assumed that systemctl stop kubelet would shut down workloads but that's not the case, and this is moderately surprising. So it's worth documenting a bit more IMO.

Agree, regarding documenting more. is there a good way to determine which version of kubelet started the pod?

For the security page, maybe a new section on upgrading? The page I modified was what was linked in the CVE, so I figured it was the best place to start.

[tengqm]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 994a79b40398adcd1216fe62de0a94ca665114a4

[sftim]

is there a good way to determine which version of kubelet started the pod?

I don't think it's generally possible. The kubelet may have restarted since, and the other end of the CRI socket is a black box as far as Kubernetes is concerned.

[sftim]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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:

• content/en/OWNERS [sftim]

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