🌱 Weaken ClusterClass webhook variable validation on update

[killianmuldoon]

Remove all validation of variables in the ClusterClass during in-place updates. This validation is no longer representative of the actual state of the cluster with external variables.

Instead this validation is done during Cluster reconciliation. This is a downgrade in UX as invalid ClusterClass updates are no longer rejected, before being committed to the API server but the same validation is still done.

Part of: #7985

[fabriziopandini]

This is a downgrade in UX as invalid ClusterClass updates are no longer rejected, before being committed to the API server but the same validation is still done.

Overall I'm ok with these changes because it comes with other advantages, like making ClusterClass more gitops friendly and also fully unblocking the potential of external patches.

I also think that over time we can seek some better trade-off around this UX, e.g doing the validation when only inline variables changes, but this can be problematic because there is no more guarantee that variable values are valid at any given time, so it requires a little bit more thinking.

[fabriziopandini]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: ccba4a78cfa3c2cd6fbf5eb20edc465018af50de

[sbueringer]

I also think that over time we can seek some better trade-off around this UX, e.g doing the validation when only inline variables changes, but this can be problematic because there is no more guarantee that variable values are valid at any given time, so it requires a little bit more thinking.

Agree, it requires more thinking.

My current take is that it's virtually impossible to validate variable values on clusters with only the information from inline variables as soon as the ClusterClass also contains external variable definitions.

The problem is that the external variables also have impact on the internal variables. E.g. if there's a variable which is defined inline and external the external variable can make it necessary to define the variable value with a definitionFrom.

We could have some basic validation which is roughly if variable values are defined with the inline DefinitionFrom or globally we can at least validate them against the inline schemas if there are corresponding schemas otherwise skip the validation.
I just fear that the validation will be so basic that it's had to understand for users what we are actually validating and how trustworthy the results are.

Maybe it's good enough to just not do any variable validation on ClusterClass updates as:

• we recommend to keep ClusterClasses immutable
• we recommend to prefer external patches over inline patches

Assuming validation based on external variable definitions is not an option as we would have to run the ClusterClass reconciler as part of the webhook which sounds brittle.

I think it also fits well into the new decoupled way of doing things. Basically if the new ClusterClass would break existing Clusters, the reconciliation of those Clusters will fail anyway as we're validating variables now in the Cluster topology reconciler.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 [sbueringer]

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