ClusterClass rebase should check variable compatibility

[killianmuldoon]

PR #8153 removes the variable compatibility checks for Clusters using a ClusterClass when that class is being updated in place. This change is a consequence of external variables.

During ClusterClass rebase - moving a Cluster from one ClusterClass to another - similar variable checks could be done allowing users to safely update from one set of variables to another while changing ClusterClass. Currently validation of those variables is not done during ClusterClass rebase.

This validation should be added to give rebase webhook based validation of variable changes.

/area topology
/kind feature

[sbueringer]

I think we're very close to having that already

Let's assume we have a Cluster update call which changes the ClusterClass.

Looking at this code:

cluster-api/internal/webhooks/cluster.go

Lines 247 to 270 in d9226f2

	clusterClass, clusterClassPollErr := webhook.pollClusterClassForCluster(ctx, newCluster)
	if clusterClassPollErr != nil &&
	// If the error is anything other than "NotFound" or "NotReconciled" return all errors at this point.
	!(apierrors.IsNotFound(clusterClassPollErr) || errors.Is(clusterClassPollErr, errClusterClassNotReconciled)) {
	allErrs = append(
	allErrs, field.InternalError(
	fldPath.Child("class"),
	clusterClassPollErr))
	return allErrs
	}
	if clusterClassPollErr == nil {
	// If there's no error validate the Cluster based on the ClusterClass.
	allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass, fldPath)...)
	}
	if oldCluster != nil { // On update
	// The ClusterClass must exist to proceed with update validation. Return an error if the ClusterClass was
	// not found.
	if apierrors.IsNotFound(clusterClassPollErr) {
	allErrs = append(
	allErrs, field.InternalError(
	fldPath.Child("class"),
	clusterClassPollErr))
	return allErrs
	}

As soon as oldCluster is != nil we don't accept not found errors right now. If we now change the code to

	if oldCluster != nil { // On update
		if clusterClassPollErr != nil {
			allErrs = append(
				allErrs, field.InternalError(
					fldPath.Child("class"),
					clusterClassPollErr))
			return allErrs
		}

We can ensure that either ValidateClusterForClusterClass is run in the update case or we return with an error because the new ClusterClass is either not found or not reconciled.

I think ValidateClusterForClusterClass already does all that we need, it ensures that the variables in new Cluster are valid according to the new ClusterClass. I don't think we have to do any additional validation with the old ClusterClass.

I think this is also in line with our idea that for a rebase old and new ClusterClass have to exist and be successfully reconciled.

[fabriziopandini]

/triage accepted

I think we should have strict validation for the rebase operation and block if CC is not ready or other issues; we should also check if we are doing default, so new variables with defaults are added (this can be also a separate PR).

The only issue that I see is when ClusterClass introduces some breaking changes on variables because in this case rebase won't be possible. But I think that this is a bigger discussion and soon or later we have to start thinking about some sort of support for variable conversion during rebase.

[killianmuldoon]

/help

[k8s-ci-robot]

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

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

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.

[fabriziopandini]

/priority important-longterm