Allow KCP users to change node registration options

[tvs]

User Story

As a developer, we would like to be able to change the node registration options used by the KCP. When upgrading, we frequently change these settings to add node labels containing information about that particular node (i.e. the distribution version of the virtual machine image).

This is currently blocked by the validating webhook:

admission webhook \"validation.kubeadmcontrolplane.controlplane.cluster.x-k8s.io\" denied the request: KubeadmControlPlane.controlplane.cluster.x-k8s.io \"crap-tkc-control-plane\" is invalid: [spec.kubeadmConfigSpec.initConfiguration.nodeRegistration.kubeletExtraArgs.node-labels: Forbidden: cannot be modified, spec.kubeadmConfigSpec.joinConfiguration.nodeRegistration.kubeletExtraArgs.node-labels: Forbidden: cannot be modified

We've also had cause in the past to change other fields in the option such as tls-cipher-suites.

While this isn't as important for the init configuration, given that it's effectively unused after the initial control plane node has been made available, it's convenient in our reconciliation system to not have to determine whether we're updating an existing KCP and instead just generate one from scratch and apply it. It's also nice to see the configurations line up, as it reduces confusion when looking at a KCP that has had several configurations rolled through it.

Detailed Description

The validating webhook should allow users to change a KCP's .spec.kubeadmConfigSpec.initConfiguration.nodeRegistration and .spec.kubeadmConfigSpec.joinConfiguration.nodeRegistration options.

Anything else you would like to add:

I suspect there are other things in the cluster configuration that we'd like to be able to manipulate as well. In the past, we've had cause to change fields such as .clusterConfiguration.apiServer.extraArgs.audit-log-path and .enable-admission-plugins.

/kind feature

[vincepri]

/milestone v0.3.x
/priority important-soon

[detiber]

I'm wondering if it would make sense to keep spec.kubeadmConfigSpec.initConfiguration.nodeRegistration immutable, but allow changes to spec.kubeadmConfigSpec.joinConfiguration?

My reasoning for suggesting this is that spec.kuberadmConfigSpec.initConfiguration.nodeRegistration would never impact the running cluster, since we do not allow operations for those types of actions in-place, but allowing mutation of the joinConfiguration should be sufficient to change the node registration configuration for all of the Machines in the workload cluster.

The only time changes to spec.kubeadmConfigSpec.initConfiguration.nodeRegistration would have any effect is if the KubeadmControlPlane was scaled to 0 and back up, which is not something that we support today (because of the data loss that would be involved in that action)

[fabriziopandini]

@detiber I was thinking something on the same line, but I thing that the current logic that detects rollouts compares the machineConfiguration with KCP.InitConfiguration or KCP.JoinConfiguration depending on the fact that the machine is the initial control plane or a joining control plane.
So, if I'm not wrong, In order to get machine rolled out properly the user should change both Init & Join configuration...

[detiber]

@fabriziopandini good point, we may need to special case the handling of nodeRegistration for that edge case.

It does make me wonder for v1alpha4+ if it would make sense to break nodeRegistration out of InitConfiguration/JoinConfiguration in the types that we expose (we could also do this with other subconfig as it makes sense as well). That would allow us to more easily control mutability of subsets of config as well as make it easier for us to compare various different concerns (in-cluster managed config vs on-host config for example)

[ncdc]

Also see #2083

[tvs]

@detiber Right. Like I said, it's not as important, since it'll largely be ignored past the initial node coming up.

The reason for suggesting that the init config also become mutable is purely from the perspective of our controllers.

When we generate configuration for the KubeadmControlPlane (and every other CAPI component for that matter), we generate the .spec from scratch on every reconciliation and then run something like controller-runtime's CreateOrUpdate against the object. With parts of the spec being immutable, it means that CreateOrUpdate is no longer usable and our controller needs to become aware of what is and is not allowed to be filled in based on whether it's the first time we're creating the object or if we're just looking to update it. It's not necessarily difficult to tease that out, but it is more cumbersome than the other CAPI objects we're also creating.

The reason for focusing on just nodeRegistration at the moment is because it's the only field that we change during regular operation (as mentioned, the node-labels changing when we change the underlying VM images). I totally expect there are other configurations we'd like to mutate going forward, particularly as we try to maintain backwards compatibility with other versions of our product (in the past we've reconfigured the audit-logging settings between releases, for example).

[fabriziopandini]

It does make me wonder for v1alpha4+ if it would make sense to break nodeRegistration out of InitConfiguration/JoinConfiguration

I'm starting to thing that, having InitConfiguration/JoinConfiguration does not make really sense for the sake of ClusterAPI. This is a kubeadm detail that should be managed internally by CABPK/KCP

[ncdc]

Slightly tangential but semi-related - would it make sense to stop exposing the kubeadm v1betax types in our KubeadmConfig/KubeadmControlPlane specs, and instead use our own types? This would allow us to separate what users fill in from which kubeadm API version we end up using in our bootstrap data.

[detiber]

Slightly tangential but semi-related - would it make sense to stop exposing the kubeadm v1betax types in our KubeadmConfig/KubeadmControlPlane specs, and instead use our own types? This would allow us to separate what users fill in from which kubeadm API version we end up using in our bootstrap data.

100%, however it does introduce the challenge that we still need to generate a valid kubeadm config for the purposes of bootstrapping and modifying the stored in-cluster configuration.

[ncdc]

Exactly - we'd have to know which kubeadm API version to use

[ncdc]

Moving the kubeadm types discussion over to #2769

[vincepri]

/milestone v0.3.7