internal/webhooks: make MDT.replicas and autoscaler mut. exclusive

Add new validation functions in cluster.go
and clusterclass.go:
- ValidateAutoscalerAnnotationsForCluster
- ValidateAutoscalerAnnotationsForClusterClass

..ForCluster is called from validateTopology().
It makes sure that a given Topology does not contain
MachineDeploymentToplogy objects under Workers, that have
both a "replicas" field set to non-nil and an autoscaler
min/max annotation present.

Also optionally it checks if there are annotations that are set
on the ClusterClass's MachineDeploymentClass for this
MachineDeploymentTopology.

..ForClusterClass accepts a list of Clusters that use
a certain ClusterClass. Inline, it uses the ..ForCluster
function and the same validation as above is performed.
It is called from validate() for a CC on update.

Additionally:
- Add a new builder utility function
  MachineDeploymentTopologyBuilder#WithAnnotations().
- Add 100% test coverage for the new validation functions.

internal/test/builder/builders.go

@@ -187,11 +187,12 @@ func (c *ClusterTopologyBuilder) Build() *clusterv1.Topology {
 
 // MachineDeploymentTopologyBuilder holds the values needed to create a testable MachineDeploymentTopology.
 type MachineDeploymentTopologyBuilder struct {
-	class     string
-	name      string
-	replicas  *int32
-	mhc       *clusterv1.MachineHealthCheckTopology
-	variables []clusterv1.ClusterVariable
+	annotations map[string]string
+	class       string
+	name        string
+	replicas    *int32
+	mhc         *clusterv1.MachineHealthCheckTopology
+	variables   []clusterv1.ClusterVariable
 }
 
 // MachineDeploymentTopology returns a builder used to create a testable MachineDeploymentTopology.
@@ -201,6 +202,12 @@ func MachineDeploymentTopology(name string) *MachineDeploymentTopologyBuilder {
 	}
 }
 
+// WithAnnotations adds annotations map used as the MachineDeploymentTopology annotations.
+func (m *MachineDeploymentTopologyBuilder) WithAnnotations(annotations map[string]string) *MachineDeploymentTopologyBuilder {
+	m.annotations = annotations
+	return m
+}
+
 // WithClass adds a class string used as the MachineDeploymentTopology class.
 func (m *MachineDeploymentTopologyBuilder) WithClass(class string) *MachineDeploymentTopologyBuilder {
 	m.class = class
@@ -228,6 +235,9 @@ func (m *MachineDeploymentTopologyBuilder) WithMachineHealthCheck(mhc *clusterv1
 // Build returns a testable MachineDeploymentTopology with any values passed to the builder.
 func (m *MachineDeploymentTopologyBuilder) Build() clusterv1.MachineDeploymentTopology {
 	md := clusterv1.MachineDeploymentTopology{
+		Metadata: clusterv1.ObjectMeta{
+			Annotations: m.annotations,
+		},
 		Class:              m.class,
 		Name:               m.name,
 		Replicas:           m.replicas,

internal/webhooks/cluster.go

@@ -306,6 +306,10 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 	if clusterClassPollErr == nil {
 		allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass)...)
 	}
+
+	// Validate the Cluster and associated ClusterClass' autoscaler annotations.
+	allErrs = append(allErrs, validateAutoscalerAnnotationsForCluster(newCluster, clusterClass)...)
+
 	if oldCluster != nil { // On update
 		// The ClusterClass must exist to proceed with update validation. Return an error if the ClusterClass was
 		// not found.
@@ -388,6 +392,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 			allErrs = append(allErrs, check.ClusterClassesAreCompatible(oldClusterClass, clusterClass)...)
 		}
 	}
+
 	return allWarnings, allErrs
 }
 
@@ -882,3 +887,61 @@ func validateTopologyMetadata(topology *clusterv1.Topology, fldPath *field.Path)
 	}
 	return allErrs
 }
+
+// validateAutoscalerAnnotationsForCluster iterates the MachineDeploymentsTopology objects under Workers and ensures the replicas
+// field and min/max annotations for autoscaler are not set at the same time. Optionally it also checks if a given ClusterClass has
+// the annotations that may apply to this Cluster.
+func validateAutoscalerAnnotationsForCluster(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList {
+	var allErrs field.ErrorList
+
+	if cluster.Spec.Topology == nil || cluster.Spec.Topology.Workers == nil {
+		return allErrs
+	}
+
+	fldPath := field.NewPath("spec", "topology")
+	for i, mdt := range cluster.Spec.Topology.Workers.MachineDeployments {
+		if mdt.Replicas == nil {
+			continue
+		}
+		for k := range mdt.Metadata.Annotations {
+			if k == clusterv1.AutoscalerMinSizeAnnotation || k == clusterv1.AutoscalerMaxSizeAnnotation {
+				allErrs = append(
+					allErrs,
+					field.Invalid(
+						fldPath.Child("workers", "machineDeployments").Index(i).Child("replicas"),
+						cluster.Spec.Topology.Workers.MachineDeployments[i].Replicas,
+						fmt.Sprintf("cannot be set for cluster %q in namespace %q if the same MachineDeploymentTopology has autoscaler annotations",
+							cluster.Name, cluster.Namespace),
+					),
+				)
+				break
+			}
+		}
+
+		// Find a matching MachineDeploymentClass for this MachineDeploymentTopology and make sure it does not have
+		// the autoscaler annotations in its Template. Skip this step entirely if clusterClass is nil.
+		if clusterClass == nil {
+			continue
+		}
+		for _, mdc := range clusterClass.Spec.Workers.MachineDeployments {
+			if mdc.Class != mdt.Class {
+				continue
+			}
+			for k := range mdc.Template.Metadata.Annotations {
+				if k == clusterv1.AutoscalerMinSizeAnnotation || k == clusterv1.AutoscalerMaxSizeAnnotation {
+					allErrs = append(
+						allErrs,
+						field.Invalid(
+							fldPath.Child("workers", "machineDeployments").Index(i).Child("replicas"),
+							cluster.Spec.Topology.Workers.MachineDeployments[i].Replicas,
+							fmt.Sprintf("cannot be set for cluster %q in namespace %q if the source class %q of this MachineDeploymentTopology has autoscaler annotations",
+								cluster.Name, cluster.Namespace, mdt.Class),
+						),
+					)
+					break
+				}
+			}
+		}
+	}
+	return allErrs
+}

internal/webhooks/cluster_test.go

@@ -3078,6 +3078,147 @@ func Test_validateTopologyMachinePoolVersions(t *testing.T) {
 	}
 }
 
+func TestValidateAutoscalerAnnotationsForCluster(t *testing.T) {
+	tests := []struct {
+		name         string
+		expectErr    bool
+		cluster      *clusterv1.Cluster
+		clusterClass *clusterv1.ClusterClass
+	}{
+		{
+			name:      "no workers in topology",
+			expectErr: false,
+			cluster:   &clusterv1.Cluster{},
+		},
+		{
+			name:      "replicas is not set",
+			expectErr: false,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+						Build()).
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set but there are no autoscaler annotations",
+			expectErr: false,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+						WithReplicas(2).
+						Build(),
+					).
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set on an MD that has only one annotation",
+			expectErr: true,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers2").
+						WithReplicas(2).
+						WithAnnotations(map[string]string{
+							clusterv1.AutoscalerMinSizeAnnotation: "2",
+						}).
+						Build(),
+					).
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set on an MD that has two annotations",
+			expectErr: true,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+						WithReplicas(2).
+						Build(),
+					).
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers2").
+						WithReplicas(2).
+						WithAnnotations(map[string]string{
+							clusterv1.AutoscalerMinSizeAnnotation: "2",
+							clusterv1.AutoscalerMaxSizeAnnotation: "20",
+						}).
+						Build(),
+					).
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set, there are no autoscaler annotations on the Cluster MDT, but there is no matching ClusterClass MDC",
+			expectErr: false,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+						WithClass("mdc1").
+						WithReplicas(2).
+						Build(),
+					).
+					Build()).
+				Build(),
+			clusterClass: builder.ClusterClass("ns", "name").
+				WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("mdc2").Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set, there are no autoscaler annotations on the Cluster MDT, but there is only one on the ClusterClass MDC",
+			expectErr: true,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+						WithClass("mdc1").
+						WithReplicas(2).
+						Build(),
+					).
+					Build()).
+				Build(),
+			clusterClass: builder.ClusterClass("ns", "name").
+				WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("mdc1").
+					WithAnnotations(map[string]string{
+						clusterv1.AutoscalerMinSizeAnnotation: "2",
+					}).
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set, there are no autoscaler annotations on the Cluster MDT, but there are two on the ClusterClass MDC",
+			expectErr: true,
+			cluster: builder.Cluster("ns", "name").WithTopology(
+				builder.ClusterTopology().
+					WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+						WithClass("mdc1").
+						WithReplicas(2).
+						Build(),
+					).
+					Build()).
+				Build(),
+			clusterClass: builder.ClusterClass("ns", "name").
+				WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("mdc1").
+					WithAnnotations(map[string]string{
+						clusterv1.AutoscalerMinSizeAnnotation: "2",
+						clusterv1.AutoscalerMaxSizeAnnotation: "20",
+					}).
+					Build()).
+				Build(),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			err := validateAutoscalerAnnotationsForCluster(tt.cluster, tt.clusterClass)
+			if tt.expectErr {
+				g.Expect(err).ToNot(BeEmpty())
+			} else {
+				g.Expect(err).To(BeEmpty())
+			}
+		})
+	}
+}
+
 func refToUnstructured(ref *corev1.ObjectReference) *unstructured.Unstructured {
 	gvk := ref.GetObjectKind().GroupVersionKind()
 	output := &unstructured.Unstructured{}

internal/webhooks/clusterclass.go

@@ -197,6 +197,9 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC
 		// Ensure no MachineHealthCheck currently in use has been removed from the ClusterClass.
 		allErrs = append(allErrs,
 			validateUpdatesToMachineHealthCheckClasses(clusters, oldClusterClass, newClusterClass)...)
+
+		allErrs = append(allErrs,
+			validateAutoscalerAnnotationsForClusterClass(clusters, newClusterClass)...)
 	}
 
 	if len(allErrs) > 0 {
@@ -506,3 +509,14 @@ func validateClusterClassMetadata(clusterClass *clusterv1.ClusterClass) field.Er
 	}
 	return allErrs
 }
+
+// validateAutoscalerAnnotationsForClusterClass iterates over a list of Clusters that use a ClusterClass and returns
+// errors if the ClusterClass contains autoscaler annotations while a Cluster has worker replicas.
+func validateAutoscalerAnnotationsForClusterClass(clusters []clusterv1.Cluster, newClusterClass *clusterv1.ClusterClass) field.ErrorList {
+	var allErrs field.ErrorList
+	for _, c := range clusters {
+		c := c
+		allErrs = append(allErrs, validateAutoscalerAnnotationsForCluster(&c, newClusterClass)...)
+	}
+	return allErrs
+}

internal/webhooks/clusterclass_test.go

@@ -2429,6 +2429,88 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
 	}
 }
 
+func TestValidateAutoscalerAnnotationsForClusterClass(t *testing.T) {
+	tests := []struct {
+		name         string
+		expectErr    bool
+		clusters     []clusterv1.Cluster
+		clusterClass *clusterv1.ClusterClass
+	}{
+		{
+			name:      "replicas is set in one cluster, there is an autoscaler annotation on the matching ClusterClass MDC",
+			expectErr: true,
+			clusters: []clusterv1.Cluster{
+				*builder.Cluster("ns", "cname1").Build(),
+				*builder.Cluster("ns", "cname2").WithTopology(
+					builder.ClusterTopology().
+						WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+							WithClass("mdc1").
+							WithReplicas(2).
+							Build(),
+						).
+						Build()).
+					Build(),
+			},
+			clusterClass: builder.ClusterClass("ns", "ccname1").
+				WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("mdc1").
+					WithAnnotations(map[string]string{
+						clusterv1.AutoscalerMaxSizeAnnotation: "20",
+					}).
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set in one cluster, there are no autoscaler annotation on the matching ClusterClass MDC",
+			expectErr: false,
+			clusters: []clusterv1.Cluster{
+				*builder.Cluster("ns", "cname1").Build(),
+				*builder.Cluster("ns", "cname2").WithTopology(
+					builder.ClusterTopology().
+						WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+							WithClass("mdc1").
+							WithReplicas(2).
+							Build(),
+						).
+						Build()).
+					Build(),
+			},
+			clusterClass: builder.ClusterClass("ns", "ccname1").
+				WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("mdc1").
+					Build()).
+				Build(),
+		},
+		{
+			name:      "replicas is set in one cluster, but the ClusterClass has no annotations",
+			expectErr: false,
+			clusters: []clusterv1.Cluster{
+				*builder.Cluster("ns", "cname1").Build(),
+				*builder.Cluster("ns", "cname2").WithTopology(
+					builder.ClusterTopology().
+						WithMachineDeployment(builder.MachineDeploymentTopology("workers1").
+							WithClass("mdc1").
+							WithReplicas(2).
+							Build(),
+						).
+						Build()).
+					Build(),
+			},
+			clusterClass: builder.ClusterClass("ns", "ccname1").Build(),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			err := validateAutoscalerAnnotationsForClusterClass(tt.clusters, tt.clusterClass)
+			if tt.expectErr {
+				g.Expect(err).ToNot(BeEmpty())
+			} else {
+				g.Expect(err).To(BeEmpty())
+			}
+		})
+	}
+}
+
 func invalidLabels() map[string]string {
 	return map[string]string{
 		"foo":          "$invalid-key",