add client based checks to clusterclass webhook

Signed-off-by: killianmuldoon <[email protected]>

internal/builder/builders.go

@@ -31,6 +31,7 @@ import (
 type ClusterBuilder struct {
 	namespace             string
 	name                  string
+	labels                map[string]string
 	topology              *clusterv1.Topology
 	infrastructureCluster *unstructured.Unstructured
 	controlPlane          *unstructured.Unstructured
@@ -44,6 +45,12 @@ func Cluster(namespace, name string) *ClusterBuilder {
 	}
 }
 
+// WithLabels sets the labels for the MachineDeploymentClassBuilder.
+func (c *ClusterBuilder) WithLabels(labels map[string]string) *ClusterBuilder {
+	c.labels = labels
+	return c
+}
+
 // WithInfrastructureCluster adds the passed InfrastructureCluster to the ClusterBuilder.
 func (c *ClusterBuilder) WithInfrastructureCluster(t *unstructured.Unstructured) *ClusterBuilder {
 	c.infrastructureCluster = t
@@ -72,6 +79,7 @@ func (c *ClusterBuilder) Build() *clusterv1.Cluster {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      c.name,
 			Namespace: c.namespace,
+			Labels:    c.labels,
 		},
 		Spec: clusterv1.ClusterSpec{
 			Topology: c.topology,
@@ -314,14 +322,10 @@ func (m *MachineDeploymentClassBuilder) Build() *clusterv1.MachineDeploymentClas
 		},
 	}
 	if m.bootstrapTemplate != nil {
-		obj.Template.Bootstrap = clusterv1.LocalObjectTemplate{
-			Ref: objToRef(m.bootstrapTemplate),
-		}
+		obj.Template.Bootstrap.Ref = objToRef(m.bootstrapTemplate)
 	}
 	if m.infrastructureMachineTemplate != nil {
-		obj.Template.Infrastructure = clusterv1.LocalObjectTemplate{
-			Ref: objToRef(m.infrastructureMachineTemplate),
-		}
+		obj.Template.Infrastructure.Ref = objToRef(m.infrastructureMachineTemplate)
 	}
 	return obj
 }

internal/topology/check/compatibility.go

@@ -226,20 +226,6 @@ func ClusterClassesAreCompatible(current, desired *clusterv1.ClusterClass) field
 func MachineDeploymentClassesAreCompatible(current, desired *clusterv1.ClusterClass) field.ErrorList {
 	var allErrs field.ErrorList
 
-	// Ensure no MachineDeployment class was removed.
-	classes := classNamesFromWorkerClass(desired.Spec.Workers)
-	for i, oldClass := range current.Spec.Workers.MachineDeployments {
-		if !classes.Has(oldClass.Class) {
-			allErrs = append(allErrs,
-				field.Invalid(
-					field.NewPath("spec", "workers", "machineDeployments").Index(i),
-					desired.Spec.Workers.MachineDeployments,
-					fmt.Sprintf("The %q MachineDeployment class can't be removed.", oldClass.Class),
-				),
-			)
-		}
-	}
-
 	// Ensure previous MachineDeployment class was modified in a compatible way.
 	for _, class := range desired.Spec.Workers.MachineDeployments {
 		for i, oldClass := range current.Spec.Workers.MachineDeployments {

internal/topology/check/compatibility_test.go

@@ -518,45 +518,6 @@ func TestClusterClassesAreCompatible(t *testing.T) {
 				Build(),
 			wantErr: false,
 		},
-		{
-			name: "error if machineDeploymentClass is removed from ClusterClass",
-			current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(
-					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
-				WithControlPlaneTemplate(
-					refToUnstructured(ref)).
-				WithControlPlaneInfrastructureMachineTemplate(
-					refToUnstructured(ref)).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
-						Build(),
-					*builder.MachineDeploymentClass("bb").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
-						Build()).
-				Build(),
-			desired: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(
-					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
-				WithControlPlaneTemplate(
-					refToUnstructured(ref)).
-				WithControlPlaneInfrastructureMachineTemplate(
-					refToUnstructured(ref)).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							refToUnstructured(incompatibleRef)).Build()).
-				Build(),
-			wantErr: true,
-		},
 	}
 	for _, tt := range tests {
 		g := NewWithT(t)
@@ -629,45 +590,6 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) {
 				Build(),
 			wantErr: false,
 		},
-		{
-			name: "error if machineDeploymentClass is removed from ClusterClass",
-			current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(
-					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
-				WithControlPlaneTemplate(
-					refToUnstructured(ref)).
-				WithControlPlaneInfrastructureMachineTemplate(
-					refToUnstructured(ref)).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
-						Build(),
-					*builder.MachineDeploymentClass("bb").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
-						Build()).
-				Build(),
-			desired: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(
-					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
-				WithControlPlaneTemplate(
-					refToUnstructured(ref)).
-				WithControlPlaneInfrastructureMachineTemplate(
-					refToUnstructured(ref)).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							refToUnstructured(incompatibleRef)).Build()).
-				Build(),
-			wantErr: true,
-		},
 		{
 			name: "error if machineDeploymentClass has multiple incompatible references",
 			current: builder.ClusterClass(metav1.NamespaceDefault, "class1").

webhooks/cluster_test.go

@@ -696,36 +696,6 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) {
 				Build(),
 			wantErr: false,
 		},
-		{
-			name: "Reject cluster.topology.class change with a deleted MachineDeploymentClass",
-			firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(refToUnstructured(ref)).
-				WithControlPlaneTemplate(refToUnstructured(ref)).
-				WithControlPlaneInfrastructureMachineTemplate(refToUnstructured(ref)).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(refToUnstructured(ref)).
-						WithBootstrapTemplate(refToUnstructured(ref)).
-						Build(),
-					*builder.MachineDeploymentClass("bb").
-						WithInfrastructureTemplate(refToUnstructured(ref)).
-						WithBootstrapTemplate(refToUnstructured(ref)).
-						Build(),
-				).
-				Build(),
-			secondClass: builder.ClusterClass(metav1.NamespaceDefault, "class2").
-				WithInfrastructureClusterTemplate(refToUnstructured(ref)).
-				WithControlPlaneTemplate(refToUnstructured(ref)).
-				WithControlPlaneInfrastructureMachineTemplate(refToUnstructured(ref)).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(refToUnstructured(ref)).
-						WithBootstrapTemplate(refToUnstructured(ref)).
-						Build(),
-				).
-				Build(),
-			wantErr: true,
-		},
 		{
 			name: "Accept cluster.topology.class change with an added MachineDeploymentClass",
 			firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").

webhooks/clusterclass.go

@@ -23,11 +23,13 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/topology/check"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
 
@@ -43,7 +45,9 @@ func (webhook *ClusterClass) SetupWebhookWithManager(mgr ctrl.Manager) error {
 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-clusterclass,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=clusterclasses,versions=v1beta1,name=default.clusterclass.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
 // ClusterClass implements a validation and defaulting webhook for ClusterClass.
-type ClusterClass struct{}
+type ClusterClass struct {
+	Client client.Reader
+}
 
 var _ webhook.CustomDefaulter = &ClusterClass{}
 var _ webhook.CustomValidator = &ClusterClass{}
@@ -99,6 +103,19 @@ func (webhook *ClusterClass) ValidateUpdate(_ context.Context, oldObj, newObj ru
 
 // ValidateDelete implements validation for ClusterClass delete.
 func (webhook *ClusterClass) ValidateDelete(ctx context.Context, obj runtime.Object) error {
+	clusterClass, ok := obj.(*clusterv1.ClusterClass)
+	if !ok {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", obj))
+	}
+	clusters, err := webhook.clustersUsingClusterClass(clusterClass)
+	if err != nil {
+		return err.ToAggregate()
+	}
+	if len(clusters) != 0 {
+		// TODO: Improve error here to include the names of some clusters using the clusterClass.
+		return field.Forbidden(field.NewPath("clusterClass"),
+			fmt.Sprintf("clusterClass %v can not be deleted as it is still in use by some clusters", clusterClass.Name))
+	}
 	return nil
 }
 
@@ -123,8 +140,126 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error {
 	// Ensure spec changes are compatible.
 	allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...)
 
+	// Ensure no MachineDeploymentClass currently in use has been removed from the ClusterClass.
+	allErrs = append(allErrs, webhook.validateLivingMachineDeploymentClassesNotRemoved(old, new)...)
+
 	if len(allErrs) > 0 {
 		return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs)
 	}
 	return nil
 }
+
+func (webhook *ClusterClass) validateLivingMachineDeploymentClassesNotRemoved(old, new *clusterv1.ClusterClass) field.ErrorList {
+	var allErrs field.ErrorList
+	if old == nil {
+		return allErrs
+	}
+	missingClasses := webhook.removedMachineClasses(old, new)
+
+	// If no classes have been removed return early as no further checks are needed.
+	if len(missingClasses) == 0 {
+		return allErrs
+	}
+
+	// TODO: The first part of this method will need to be broken out to properly account for changes when also validating variables.
+	// Retrieve all clusters using the ClusterClass.
+	clusters, errs := webhook.clustersUsingClusterClass(old)
+	if len(errs) > 0 {
+		allErrs = append(allErrs, errs...)
+		return allErrs
+	}
+
+	// Retrieve all the MachineDeployments in the clusterClass namespace using a managed topology.
+	machineDeployments, errs := webhook.topologyManagedMachineDeployments(new.Namespace)
+	if len(errs) > 0 {
+		allErrs = append(allErrs, errs...)
+		return allErrs
+	}
+
+	// Create a set of records of machineDeploymentClass information where the name is a fully qualified name for the
+	// MachineDeployment topology in the form cluster.Name/machineDeploymentTopology.Name and links to the machineDeploymentClass Name.
+	mdcRecords := map[string]string{}
+	for _, c := range clusters {
+		for _, machineDeploymentClass := range c.Spec.Topology.Workers.MachineDeployments {
+			if missingClasses.Has(machineDeploymentClass.Class) {
+				mdcRecords[fmt.Sprintf("%v/%v", c.Name, machineDeploymentClass.Name)] = machineDeploymentClass.Class
+			}
+		}
+	}
+
+	// For each machineDeployment using a managed topology return an error if it is using a class that has been removed in the ClusterClass change.
+	for _, md := range machineDeployments {
+		mdName := fmt.Sprintf("%v/%v", md.Labels[clusterv1.ClusterLabelName], md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName])
+		if _, ok := mdcRecords[mdName]; ok {
+			allErrs = append(allErrs, field.Forbidden(field.NewPath(""), fmt.Sprintf("MachineDeploymentClass %v is in use in MachineDeployment %v in Cluster %v. ClusterClass %v modification not allowed",
+				mdcRecords[mdName], md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName], md.Labels[clusterv1.ClusterLabelName], old.Name),
+			))
+		}
+	}
+
+	return allErrs
+}
+
+func (webhook *ClusterClass) topologyManagedMachineDeployments(namespace string) ([]clusterv1.MachineDeployment, field.ErrorList) {
+	var allErrs field.ErrorList
+
+	// List all the MachineDeployments in the current cluster using a managed topology.
+	machineDeployments := &clusterv1.MachineDeploymentList{}
+	err := webhook.Client.List(context.Background(), machineDeployments,
+		client.MatchingLabels{
+			clusterv1.ClusterTopologyOwnedLabel: "",
+		},
+		// TODO: Should this be namespaced in future?
+		client.InNamespace(namespace),
+	)
+	if err != nil {
+		allErrs = append(allErrs, field.InternalError(field.NewPath(""), err))
+		return nil, allErrs
+	}
+	return machineDeployments.Items, nil
+}
+
+func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String {
+	missingClasses := sets.NewString()
+
+	// Ensure no MachineDeploymentClass in use has been removed.
+	classes := webhook.classNamesFromWorkerClass(new.Spec.Workers)
+	for _, oldClass := range old.Spec.Workers.MachineDeployments {
+		if !classes.Has(oldClass.Class) {
+			missingClasses.Insert(oldClass.Class)
+		}
+	}
+	return missingClasses
+}
+
+// classNames returns the set of MachineDeployment class names.
+func (webhook *ClusterClass) classNamesFromWorkerClass(w clusterv1.WorkersClass) sets.String {
+	classes := sets.NewString()
+	for _, class := range w.MachineDeployments {
+		classes.Insert(class.Class)
+	}
+	return classes
+}
+
+func (webhook *ClusterClass) clustersUsingClusterClass(clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, field.ErrorList) {
+	var allErrs field.ErrorList
+	clusters := &clusterv1.ClusterList{}
+	clustersUsingClusterClass := []clusterv1.Cluster{}
+	err := webhook.Client.List(context.Background(), clusters,
+		client.MatchingLabels{
+			clusterv1.ClusterTopologyOwnedLabel: "",
+		},
+		//TODO: This will be an issue when doing clusterClass across namespaces.
+		client.InNamespace(clusterClass.Namespace),
+	)
+	if err != nil {
+		allErrs = append(allErrs, field.InternalError(field.NewPath(""), err))
+		return nil, allErrs
+	}
+	for _, c := range clusters.Items {
+		if c.Spec.Topology.Class == clusterClass.Name {
+			clustersUsingClusterClass = append(clustersUsingClusterClass, c)
+		}
+	}
+	return clustersUsingClusterClass, nil
+}