ClusterClass webhook allow compatible reference changes

[fabriziopandini]

Current ClusterClass webhook implementation ensures that references to template Objects are correct and fully provided. and then enforce immutability.

We should allow for reference changes, thus supporting the template rotation operation pattern,

However, in this case, it is required to implement checks ensuring compliance with the compatibility rules defined in the proposal (the last bullet in the list)

Environment:

• Cluster-api version: master

/kind bug

[fabriziopandini]

/area topology
/milestone v0.4

[fabriziopandini]

@yastij to add about the use case of renaming an apigroup, which is not included in the current compatibility definition

[sbueringer]

Minor addition: Afaik we currently make sure they are immutable:

cluster-api/api/v1alpha4/clusterclass_webhook.go

Lines 132 to 200 in f950b4c

	func (in ClusterClass) validateCompatibleSpecChanges(old *ClusterClass) field.ErrorList {
	var allErrs field.ErrorList
	// in case of create, no changes to verify
	// return early.
	if old == nil {
	return nil
	}
	// Ensure that the old MachineDeployments still exist.
	allErrs = append(allErrs, in.validateMachineDeploymentsChanges(old)...)
	if !reflect.DeepEqual(in.Spec.Infrastructure, old.Spec.Infrastructure) {
	allErrs = append(allErrs,
	field.Invalid(
	field.NewPath("spec", "infrastructure"),
	in.Spec.Infrastructure,
	"cannot be changed.",
	),
	)
	}
	if !reflect.DeepEqual(in.Spec.ControlPlane, old.Spec.ControlPlane) {
	allErrs = append(allErrs,
	field.Invalid(
	field.NewPath("spec", "controlPlane"),
	in.Spec.Infrastructure,
	"cannot be changed.",
	),
	)
	}
	return allErrs
	}
	func (in ClusterClass) validateMachineDeploymentsChanges(old *ClusterClass) field.ErrorList {
	var allErrs field.ErrorList
	// Ensure no MachineDeployment class was removed.
	classes := in.Spec.Workers.classNames()
	for _, oldClass := range old.Spec.Workers.MachineDeployments {
	if !classes.Has(oldClass.Class) {
	allErrs = append(allErrs,
	field.Invalid(
	field.NewPath("spec", "workers", "machineDeployments"),
	in.Spec.Workers.MachineDeployments,
	fmt.Sprintf("The %q MachineDeployment class can't be removed.", oldClass.Class),
	),
	)
	}
	}
	// Ensure no previous MachineDeployment class was modified.
	for _, class := range in.Spec.Workers.MachineDeployments {
	for _, oldClass := range old.Spec.Workers.MachineDeployments {
	if class.Class == oldClass.Class && !reflect.DeepEqual(class, oldClass) {
	allErrs = append(allErrs,
	field.Invalid(
	field.NewPath("spec", "workers", "machineDeployments"),
	class,
	"cannot be changed.",
	),
	)
	}
	}
	}
	return allErrs
	}

[fabriziopandini]

@sbueringer you are right, and sorry for did not provide full context here.

The background idea behind this issue it that at this stage, IMO we can reasonably start to allow an initial set of ClusterClass mutations by enabling metadata changes (see ClusterClass webhook should allow for metadata mutation #5192) and template rotation according to the idea of compatible change (this issue).

A similar discussion is happening on #5195 (comment), however with a slight different perspective (how to react timely to changes).

I have update the issue title and description accordingly.

[fabriziopandini]

/assign
/lifecycle active