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

cluster-api/controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

Line 51 in b0236f2

var kubeSemver = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)

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 #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 32e6e61#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

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[fabriziopandini]

/lifecycle frozen
I think we should make version management consistent across resources and leverage v1alpha3 --> v1alpha4 conversion to fix existing resources without triggering rollout
@vincepri opinions?

[fabriziopandini]

/priority important-soon
This should be re-evaluated after #4163 merged

[fabriziopandini]

/help

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vincepri]

I think we should make version management consistent across resources and leverage v1alpha3 --> v1alpha4 conversion to fix existing resources without triggering rollout

How would that work for folks using gitops? That would cause their changes to be overwritten, and possibly still causing a rollout if we overwrite their changes.

I'd rather have the validation/defaulting/mutation be in a webhook, and return a warning (I think there is support for these now?) and in the next version we'd start breaking folks. I'd want to avoid any magic changes during upgrades using the CLI.

[vincepri]

/milestone Next

[vincepri]

/assign @killianmuldoon
/milestone v1.1

[fabriziopandini]

/close
in favour of #7011 and #7010

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

/close
in favour of #7011 and #7010

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.