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 ClusterBuilder.
+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/envtest/environment.go

@@ -219,7 +219,7 @@ func newEnvironment(uncachedObjs ...client.Object) *Environment {
 	if err := (&webhooks.Cluster{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil {
 		klog.Fatalf("unable to create webhook: %+v", err)
 	}
-	if err := (&webhooks.ClusterClass{}).SetupWebhookWithManager(mgr); err != nil {
+	if err := (&webhooks.ClusterClass{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil {
 		klog.Fatalf("unable to create webhook: %+v", err)
 	}
 	if err := (&clusterv1.Machine{}).SetupWebhookWithManager(mgr); err != nil {

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

@@ -519,7 +519,7 @@ func TestClusterClassesAreCompatible(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "error if machineDeploymentClass is removed from ClusterClass",
+			name: "pass if machineDeploymentClass is removed from ClusterClass",
 			current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
 				WithInfrastructureClusterTemplate(
 					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
@@ -555,7 +555,7 @@ func TestClusterClassesAreCompatible(t *testing.T) {
 						WithBootstrapTemplate(
 							refToUnstructured(incompatibleRef)).Build()).
 				Build(),
-			wantErr: true,
+			wantErr: false,
 		},
 	}
 	for _, tt := range tests {
@@ -630,7 +630,7 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "error if machineDeploymentClass is removed from ClusterClass",
+			name: "pass if machineDeploymentClass is removed from ClusterClass",
 			current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
 				WithInfrastructureClusterTemplate(
 					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
@@ -666,7 +666,7 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) {
 						WithBootstrapTemplate(
 							refToUnstructured(incompatibleRef)).Build()).
 				Build(),
-			wantErr: true,
+			wantErr: false,
 		},
 		{
 			name: "error if machineDeploymentClass has multiple incompatible references",

internal/webhooks/cluster_test.go

@@ -697,7 +697,7 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "Reject cluster.topology.class change with a deleted MachineDeploymentClass",
+			name: "Accept cluster.topology.class change with a deleted MachineDeploymentClass",
 			firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
 				WithInfrastructureClusterTemplate(refToUnstructured(ref)).
 				WithControlPlaneTemplate(refToUnstructured(ref)).
@@ -724,7 +724,7 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) {
 						Build(),
 				).
 				Build(),
-			wantErr: true,
+			wantErr: false,
 		},
 		{
 			name: "Accept cluster.topology.class change with an added MachineDeploymentClass",

internal/webhooks/clusterclass.go

@@ -20,14 +20,17 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/pkg/errors"
 	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 +46,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{}
@@ -76,16 +81,16 @@ func defaultNamespace(ref *corev1.ObjectReference, namespace string) {
 }
 
 // ValidateCreate implements validation for ClusterClass create.
-func (webhook *ClusterClass) ValidateCreate(_ context.Context, obj runtime.Object) error {
+func (webhook *ClusterClass) ValidateCreate(ctx context.Context, obj runtime.Object) error {
 	in, ok := obj.(*clusterv1.ClusterClass)
 	if !ok {
 		return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", obj))
 	}
-	return webhook.validate(nil, in)
+	return webhook.validate(ctx, nil, in)
 }
 
 // ValidateUpdate implements validation for ClusterClass update.
-func (webhook *ClusterClass) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) error {
+func (webhook *ClusterClass) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) error {
 	newClusterClass, ok := newObj.(*clusterv1.ClusterClass)
 	if !ok {
 		return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", newObj))
@@ -94,15 +99,30 @@ func (webhook *ClusterClass) ValidateUpdate(_ context.Context, oldObj, newObj ru
 	if !ok {
 		return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", oldObj))
 	}
-	return webhook.validate(oldClusterClass, newClusterClass)
+	return webhook.validate(ctx, oldClusterClass, newClusterClass)
 }
 
 // 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.getClustersUsingClusterClass(ctx, clusterClass)
+	if err != nil {
+		return apierrors.NewInternalError(errors.Wrapf(err, "could not retrieve Clusters using ClusterClass"))
+	}
+
+	if len(clusters) > 0 {
+		// TODO(killianmuldoon): Improve error here to include the names of some clusters using the clusterClass.
+		return apierrors.NewForbidden(clusterv1.GroupVersion.WithResource("ClusterClass").GroupResource(), clusterClass.Name,
+			fmt.Errorf("cannot be deleted. %d clusters still using the ClusterClass", len(clusters)))
+	}
 	return nil
 }
 
-func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error {
+func (webhook *ClusterClass) validate(ctx context.Context, old, new *clusterv1.ClusterClass) error {
 	// NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the web hook
 	// must prevent creating new objects new case the feature flag is disabled.
 	if !feature.Gates.Enabled(feature.ClusterTopology) {
@@ -111,7 +131,6 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error {
 			"can be set only if the ClusterTopology feature flag is enabled",
 		)
 	}
-
 	var allErrs field.ErrorList
 
 	// Ensure all references are valid.
@@ -120,11 +139,88 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error {
 	// Ensure all MachineDeployment classes are unique.
 	allErrs = append(allErrs, check.MachineDeploymentClassesAreUnique(new)...)
 
-	// Ensure spec changes are compatible.
-	allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...)
-
+	// If this is an update run additional validation.
+	if old != nil {
+		// Ensure spec changes are compatible.
+		allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...)
+
+		// Retrieve all clusters using the ClusterClass.
+		clusters, err := webhook.getClustersUsingClusterClass(ctx, old)
+		if err != nil {
+			allErrs = append(allErrs, field.InternalError(field.NewPath(""),
+				errors.Wrapf(err, "Clusters using ClusterClass %v can not be retrieved", old.Name)))
+			return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs)
+		}
+
+		// Ensure no MachineDeploymentClass currently in use has been removed from the ClusterClass.
+		allErrs = append(allErrs, webhook.validateRemovedMachineDeploymentClassesAreNotUsed(clusters, old, new)...)
+	}
 	if len(allErrs) > 0 {
 		return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs)
 	}
 	return nil
 }
+
+func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(clusters []clusterv1.Cluster, old, new *clusterv1.ClusterClass) field.ErrorList {
+	var allErrs field.ErrorList
+
+	removedClasses := webhook.removedMachineClasses(old, new)
+	// If no classes have been removed return early as no further checks are needed.
+	if len(removedClasses) == 0 {
+		return nil
+	}
+	// Error if any Cluster using the ClusterClass uses a MachineDeploymentClass that has been removed.
+	for _, c := range clusters {
+		for _, machineDeploymentTopology := range c.Spec.Topology.Workers.MachineDeployments {
+			if removedClasses.Has(machineDeploymentTopology.Class) {
+				// TODO(killianmuldoon): Improve error printing here so large scale changes don't flood the error log e.g. deduplication, only example usages given.
+				allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "workers", "machineDeployments"),
+					fmt.Sprintf("MachineDeploymentClass %v is in use in MachineDeploymentTopology %v in Cluster %v. ClusterClass %v modification not allowed",
+						machineDeploymentTopology.Class, machineDeploymentTopology.Name, c.Name, old.Name),
+				))
+			}
+		}
+	}
+	return allErrs
+}
+
+func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String {
+	removedClasses := sets.NewString()
+
+	classes := webhook.classNamesFromWorkerClass(new.Spec.Workers)
+	for _, oldClass := range old.Spec.Workers.MachineDeployments {
+		if !classes.Has(oldClass.Class) {
+			removedClasses.Insert(oldClass.Class)
+		}
+	}
+	return removedClasses
+}
+
+// classNamesFromWorkerClass 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) getClustersUsingClusterClass(ctx context.Context, clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, error) {
+	clusters := &clusterv1.ClusterList{}
+	clustersUsingClusterClass := []clusterv1.Cluster{}
+	err := webhook.Client.List(ctx, clusters,
+		client.MatchingLabels{
+			clusterv1.ClusterTopologyOwnedLabel: "",
+		},
+		client.InNamespace(clusterClass.Namespace),
+	)
+	if err != nil {
+		return nil, err
+	}
+	for _, c := range clusters.Items {
+		if c.Spec.Topology.Class == clusterClass.Name {
+			clustersUsingClusterClass = append(clustersUsingClusterClass, c)
+		}
+	}
+	return clustersUsingClusterClass, nil
+}