Kubelet does not update node-labels from flags if they change

[smarterclayton]

Once a kubelet has created a node object for itself via registration, if the --node-labels flag ever changes the labels aren't updated. I see the labels being set into the objectmeta in the init code, but I don't see the server being updated (external ID hasn't changed).

Recreate:

1. Start kubelet with no --node-labels, wait for it to register
2. Update kubelet to start with --node-labels a=b

Expect:

1. API object for node to have labels a=b

Actual:

1. API object labels don't change until node object is deleted

I don't see any obvious place where this has been broken, but it looks like the patch calculated by nodeutil is only the conditions.

[jhorwit2]

@smarterclayton this is intended functionality. The flag (see below) mentions they're only added when registering.

https://kubernetes.io/docs/reference/generated/kubelet/

--node-labels mapStringString Labels to add when registering the node in the cluster. Labels must be key=value pairs separated by ','.

[jhorwit2]

I think we should probably rename it to --register-with-labels so it matches --register-with-taints especially since it's still an alpha feature and that's more clear.

[smarterclayton]

This is going to be broken with dynamic configuration - as a user, it was unexpected for new labels not to be added. I think it's time to revisit this.

[smarterclayton]

@mtaufen @liggitt re dynamic reconfiguration.

[smarterclayton]

note: It's silly we call this an alpha feature when it's been in the kubelet since 1.1. We should own up to it one way or another.

[liggitt]

kubelet ownership of its own labels is not deterministic and is problematic. Kubelet updating labels on an existing Node API object on start is not the direction we want to go, since it removes the ability to centrally manage those labels via the API

[smarterclayton]

I disagree - the kubelet owns its own labels (those specified at --node-labels time) at a minimum. I agree that node labels should be manageable via an API, especially those set by humans. I do not agree that a fleet of bootstrapped nodes in scaling groups should not be able to specify their own labels and keep them up to date based on config. And given that the node controller can stomp down nodes, the newly bootstrapped nodes in those scale groups will end up having those labels (when the node object is deleted), so having a weird inconsistent state where labels sometimes show up is pointless and confusing.

Since we're not going to remove --node-labels from the kubelet, and we are moving head on towards bootstrapped and centrally configured nodes (dynamic node config), the node should manage at a minimum the set of labels and taints specified by CLI. I don't actually care whether the old labels are removed, since most of those label changes will be managed by rotating scale groups, but the current state is dumb.

[liggitt]

I disagree - the kubelet owns its own labels

That is incompatible with using labels to segment node groups for anything security related. Why wouldn't the person applying config to nodes also label them?

[smarterclayton]

In that case we should just make the node controller do it based on the config - it can observe the active config correctly. But if we really care about security, then the node shouldn’t be able to set taints or labels on create other and only the node controller manages that set. That seems strictly better, although disabling node labels ends up being a painful deprecation security process.

[smarterclayton]

Disabling/preventing the kubelets node-labels and register-taints flags, that is

[liggitt]

if we really care about security, then the node shouldn’t be able to set taints or labels on create other and only the node controller manages that set. That seems strictly better, although disabling node labels ends up being a painful deprecation security process.

That is the goal, trying to work through how to transition there. It probably looks like whitelisting existing labels/taints the kubelets set, but in the meantime I want to avoid expansion of kubelet control over taints/labels

cc @mikedanese @kubernetes/sig-auth-misc

[mikedanese]

This might be a dup of #18394 but is more specific

[liggitt]

this seems like the same issue as #18394
/close