Merge pull request #12561 from sbueringer/pr-improve-topo-name-validation

k8s-ci-robot · web-flow · commit d4663b2eea3a · 2025-07-31T13:17:15.000-07:00
🌱 Improve validation of worker topology names in Cluster resource
diff --git a/api/core/v1beta2/cluster_types.go b/api/core/v1beta2/cluster_types.go
@@ -836,6 +836,7 @@ type MachineDeploymentTopology struct {
 	// +required
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=63
+	// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`
 	Name string `json:"name,omitempty"`
 
 	// failureDomain is the failure domain the machines will be created in.
@@ -1146,6 +1147,7 @@ type MachinePoolTopology struct {
 	// +required
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=63
+	// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`
 	Name string `json:"name,omitempty"`
 
 	// failureDomains is the list of failure domains the machine pool will be created in.
diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml
diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go
@@ -23,7 +23,6 @@ import (
 
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -287,19 +286,6 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste
 	machineDeploymentClasses := mdClassNamesFromWorkerClass(clusterClass.Spec.Workers)
 	names := sets.Set[string]{}
 	for i, md := range desired.Spec.Topology.Workers.MachineDeployments {
-		if errs := validation.IsValidLabelValue(md.Name); len(errs) != 0 {
-			for _, err := range errs {
-				allErrs = append(
-					allErrs,
-					field.Invalid(
-						field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("name"),
-						md.Name,
-						fmt.Sprintf("must be a valid label value %s", err),
-					),
-				)
-			}
-		}
-
 		if !machineDeploymentClasses.Has(md.Class) {
 			allErrs = append(allErrs,
 				field.Invalid(
@@ -348,19 +334,6 @@ func MachinePoolTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cl
 	machinePoolClasses := mpClassNamesFromWorkerClass(clusterClass.Spec.Workers)
 	names := sets.Set[string]{}
 	for i, mp := range desired.Spec.Topology.Workers.MachinePools {
-		if errs := validation.IsValidLabelValue(mp.Name); len(errs) != 0 {
-			for _, err := range errs {
-				allErrs = append(
-					allErrs,
-					field.Invalid(
-						field.NewPath("spec", "topology", "workers", "machinePools").Index(i).Child("name"),
-						mp.Name,
-						fmt.Sprintf("must be a valid label value %s", err),
-					),
-				)
-			}
-		}
-
 		if !machinePoolClasses.Has(mp.Class) {
 			allErrs = append(allErrs,
 				field.Invalid(
diff --git a/internal/topology/check/compatibility_test.go b/internal/topology/check/compatibility_test.go
@@ -1303,36 +1303,6 @@ func TestMachineDeploymentTopologiesAreUniqueAndDefinedInClusterClass(t *testing
 				Build(),
 			wantErr: false,
 		},
-		{
-			name: "fail if MachineDeploymentTopology name is longer than 63 characters",
-			clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(
-					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
-				WithControlPlaneTemplate(
-					builder.ControlPlane(metav1.NamespaceDefault, "cp1").Build()).
-				WithControlPlaneInfrastructureMachineTemplate(
-					builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpinfra1").Build()).
-				WithWorkerMachineDeploymentClasses(
-					*builder.MachineDeploymentClass("aa").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
-						Build()).
-				Build(),
-			cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
-				WithTopology(
-					builder.ClusterTopology().
-						WithClass("class1").
-						WithVersion("v1.22.2").
-						WithMachineDeployment(
-							builder.MachineDeploymentTopology("machine-deployment-topology-name-that-has-longerthan63characterlooooooooooooooooooooooongname").
-								WithClass("aa").
-								Build()).
-						Build()).
-				Build(),
-			wantErr: true,
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1510,36 +1480,6 @@ func TestMachinePoolTopologiesAreUniqueAndDefinedInClusterClass(t *testing.T) {
 				Build(),
 			wantErr: false,
 		},
-		{
-			name: "fail if MachinePoolTopology name is longer than 63 characters",
-			clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
-				WithInfrastructureClusterTemplate(
-					builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
-				WithControlPlaneTemplate(
-					builder.ControlPlane(metav1.NamespaceDefault, "cp1").Build()).
-				WithControlPlaneInfrastructureMachineTemplate(
-					builder.InfrastructureMachinePoolTemplate(metav1.NamespaceDefault, "cpinfra1").Build()).
-				WithWorkerMachinePoolClasses(
-					*builder.MachinePoolClass("aa").
-						WithInfrastructureTemplate(
-							builder.InfrastructureMachinePoolTemplate(metav1.NamespaceDefault, "infra1").Build()).
-						WithBootstrapTemplate(
-							builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
-						Build()).
-				Build(),
-			cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
-				WithTopology(
-					builder.ClusterTopology().
-						WithClass("class1").
-						WithVersion("v1.22.2").
-						WithMachinePool(
-							builder.MachinePoolTopology("machine-pool-topology-name-that-has-longerthan63characterlooooooooooooooooooooooongname").
-								WithClass("aa").
-								Build()).
-						Build()).
-				Build(),
-			wantErr: true,
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/internal/webhooks/test/cluster_test.go b/internal/webhooks/test/cluster_test.go
@@ -0,0 +1,104 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package test
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
+	"sigs.k8s.io/cluster-api/util/test/builder"
+)
+
+func Test_ValidateCluster(t *testing.T) {
+	tests := []struct {
+		name           string
+		mdTopologyName string
+		mpTopologyName string
+		cluster        *clusterv1.Cluster
+		wantErrs       []string
+	}{
+		{
+			name:           "fail if names are not valid Kubernetes resource names",
+			mdTopologyName: "under_score",
+			mpTopologyName: "under_score",
+			wantErrs: []string{
+				"spec.topology.workers.machineDeployments[0].name: Invalid value: \"under_score\": spec.topology.workers.machineDeployments[0].name in body should match",
+				"spec.topology.workers.machinePools[0].name: Invalid value: \"under_score\": spec.topology.workers.machinePools[0].name in body should match",
+			},
+		},
+		{
+			name:           "fail if names are not valid Kubernetes label values",
+			mdTopologyName: "forward/slash",
+			mpTopologyName: "forward/slash",
+			wantErrs: []string{
+				"spec.topology.workers.machineDeployments[0].name: Invalid value: \"forward/slash\": spec.topology.workers.machineDeployments[0].name in body should match",
+				"spec.topology.workers.machinePools[0].name: Invalid value: \"forward/slash\": spec.topology.workers.machinePools[0].name in body should match",
+			},
+		},
+		{
+			name:           "fail if names are longer than 63 characters (the maximum length of a Kubernetes label value)",
+			mdTopologyName: "machine-deployment-topology-name-that-has-longerthan63characterlooooooooooooooooooooooongname",
+			mpTopologyName: "machine-pool-topology-name-that-has-longerthan63characterlooooooooooooooooooooooongname",
+			wantErrs: []string{
+				"spec.topology.workers.machineDeployments[0].name: Too long: may not be more than 63 bytes",
+				"spec.topology.workers.machinePools[0].name: Too long: may not be more than 63 bytes",
+			},
+		},
+		{
+			name:           "succeed with valid name",
+			mdTopologyName: "valid-machine-deployment-topology-name",
+			mpTopologyName: "valid-machine-pool-topology-name",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			cluster := builder.Cluster(metav1.NamespaceDefault, "cluster1").
+				WithTopology(
+					builder.ClusterTopology().
+						WithClass("class1").
+						WithVersion("v1.22.2").
+						WithMachineDeployment(
+							builder.MachineDeploymentTopology(tt.mdTopologyName).
+								WithClass("aa").
+								Build()).
+						WithMachinePool(
+							builder.MachinePoolTopology(tt.mpTopologyName).
+								WithClass("aa").
+								Build()).
+						Build()).
+				Build()
+
+			err := env.CreateAndWait(ctx, cluster)
+
+			if len(tt.wantErrs) > 0 {
+				g.Expect(err).To(HaveOccurred())
+				for _, wantErr := range tt.wantErrs {
+					g.Expect(err.Error()).To(ContainSubstring(wantErr))
+				}
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(env.CleanupAndWait(ctx, cluster)).To(Succeed())
+			}
+		})
+	}
+}
