🐛 Make MachineSpec Version validation consistent with KCP

[CecileRobertMichon]

What this PR does / why we need it: Makes MachineSpec Version validation consistent with KCP version validation. For both KCP and Machine Spec.Version, the version will be defaulted to have a "v prefix if the user doesn't pass it in with the prefix to avoid this being a breaking change.

⚠️ The v prefix is now required for MachineSpec.Version and will be defaulted if not provided.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3128

[vincepri]

/milestone v0.3.7

[detiber]

/hold
It looks like this would break any existing reconciliation of resources that may already exist with the previous validation.

Would it be better to update KCP to allow a non-'v' prefixed version and add a TODO item for when we are ready to introduce breaking changes with v1alpha4 to add validation with 'v' that way we could handle the conversion automatically through a conversion webhook?

Alternatively, we could probably add the v prefix as part of the defaulting webhook, which should run prior to any defaulting webhooks we have, but that would likely require us to do the validation in a webhook rather than as part of the openapi spec, since I believe that validation is processed prior to defaulting webhooks being run.

[vincepri]

IIRC defaulting webhooks should run before any validation, but I don't remember about OpenAPI validation  — we should add a test for this

[CecileRobertMichon]

openapi validation will run before any webhooks. It is a breaking change, just like #2691 was (CAPZ e2e tests were broken by that change when it was merged actually). Right now we're inconsistent though, so this was aiming to bring that consistency back, with #2691 (comment) in mind.

I personally don't agree with breaking the user to make them learn and I would prefer to change KCP to be tolerant to non-v prefixed versions (I actually don't know if that's even a kubeadm requirement), but since that decision was already taken in #2691 I didn't want to reverse it.

@randomvariable do you remember why that stricter validation had to be added in the first place? Semver validation was already added in #2647 and #2674

[vincepri]

If we move the regex to a Go-regex and use it in the validation webook + defaulting webhook that fixes the Machine version, we should be good. In v1alpha4 we can drop this behavior and standardize on the openapi-regex

[CecileRobertMichon]

@vincepri should I also do this for KCP? Also do you know why the v is required?

[vincepri]

Today, the leading v is required in KCP

[CecileRobertMichon]

Today, the leading v is required in KCP

I know it's required since #2691 but my question was more why does it need to be required? ie. would it have a negative impact to relax that requirement by only checking for a valid semver version?

[vincepri]

Ah got it, apologies I got distracted and understood something else. IIRC the decision was to make it consistent within the whole codebase when dealing with upgrades. We've found for example that most of the container images have leading v everywhere as well, and we ended up defaulting everywhere.

I'm open to revisit that decision, a lot has changed since then.

[CecileRobertMichon]

@detiber @vincepri updated the PR to do regex validation in the webhooks with defaulting in place for both KCP and Machine. Let me know what you think.

[CecileRobertMichon]

Seeing test failures in machinehealthcheck_controller_test, seems related to https://kubernetes.slack.com/archives/C8TSNPY4T/p1591379760446600

[vincepri]

@CecileRobertMichon is this rebased on latest master?

[CecileRobertMichon]

@vincepri yes it is

[detiber]

/lgtm

[vincepri]

/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, vincepri

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:

• OWNERS [vincepri]

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