Cluster-api: KCP/CABPK should reject known invalid Kubernetes versions

[ncdc]

What steps did you take and what happened:

1. Try to create a cluster with KCP.spec.kubernetesVersion set to something like v1.19.1_mycompany.2
2. kubeadm bootstrapping fails with this in the output

couldn't parse Kubernetes version "v1.19.1_mycompany.2": could not parse pre-release/metadata (_mycompany.2) in version "v1.19.1_mycompany.2"

What did you expect to happen:

1. kubeadm bootstrapping succeeds

Anything else you would like to add:
For this example, a valid/appropriate version would be v1.19.1+mycompany.2.

kubeadm only accepts valid semver values. We should consider adding webhook validations for both KCP and KubeadmConfig.

Environment:

• Cluster-api version: v0.3.x

/kind bug
/area control-plane
/area bootstrap

[ncdc]

cc @fabriziopandini

[vincepri]

Seems we're using this regex https://github.com/kubernetes-sigs/cluster-api/blob/b0236f2b3c1d0ee9e6a8dffd1baaa22fd8aee17d/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go#L51 to validate the KCP.Spec.Version field, and it seems it does allow for underscores 🤔

[ncdc]

Compare with https://github.com/kubernetes/kubernetes/blob/ccfdc09f3559f0cb220dd4d921951f1badf15076/cmd/kubeadm/app/util/config/common.go#L100-L102

[vincepri]

cc @CecileRobertMichon as well, given that the regex came in https://github.com/kubernetes-sigs/cluster-api/pull/3147

[vincepri]

@ncdc Not sure if you wanna tackle this in v0.3.x or later, if we do remove the underscore from the regex, it could technically break some controllers out there from patching, that said their clusters would already pretty much be broken at that point

[ncdc]

/milestone v0.4.0

[CecileRobertMichon]

@vincepri regex actually came from https://github.com/kubernetes-sigs/cluster-api/commit/32e6e61a254c170095c85816fb0ea0f6d575bb89#diff-99f38c7c07bfcc03a528180716bafab5R58, I just reused the same one in webhook validation (couldn't use the one in util because it created a dependency cycle)

[vincepri]

Thanks, I wish we added a comment on where that regex came from :/

[vincepri]

We can probably use this issue to align all our validations on Kubernetes versions, and use the exact same code / parsing everywhere

[vincepri]

@fabriziopandini What do you think the best way to proceed here? Are you able to work on this?

[fabriziopandini]

Let me take a look tomorrow and try to get a picture of all the points where we are doing version validation before making an action plan

[fabriziopandini]

@vincepri I have found following field in the API holding a Kubernetes version and the corresponding web hook validation:

1. Machine.Spec.Version: validated using wrong regex :-(
2. MachineDeployments.Spec.Template.Spec.Version: not validated :-(
3. KubeadmConfig.Spec.ClusterConfiguration.KubernetesVersion: not validated :-(
4. KubeadmConfigTemplate.Spec.Template.Spec.ClusterConfiguration.KubernetesVersion: not validated :-(
5. KubeadmControlPlane.Spec.Version: validated using wrong regex :-(
6. MachinePools.Spec.Template.Spec.Version: not validated :-(

So my plan is to craft a PR that ensures

• all the defaulting web hook adding v in front of version, if missing.
• all the validating web hook are checking version using version.ParseSemantic

[fabriziopandini]

/assign
/lifecycle active

[vincepri]

all the defaulting web hook add adding v in front of version, if missing.

This might be a breaking change and cause a rollout in some cases

[fabriziopandini]

/remove-lifecycle active
TBD an approach that does not trigger rollouts

[fabriziopandini]

/unassing @fabriziopandini