diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go index 839fc14ae0ba..73fcea1babc8 100644 --- a/internal/topology/check/compatibility.go +++ b/internal/topology/check/compatibility.go @@ -296,8 +296,9 @@ func MachinePoolClassesAreUnique(clusterClass *clusterv1.ClusterClass) field.Err return allErrs } -// MachineDeploymentTopologiesAreValidAndDefinedInClusterClass checks that each MachineDeploymentTopology name is not empty -// and unique, and each class in use is defined in ClusterClass.spec.Workers.MachineDeployments. +// MachineDeploymentTopologiesAreValidAndDefinedInClusterClass checks that each MachineDeploymentTopology name is not empty, +// is a valid Kubernetes resource name, is a valid label value, and is unique. It also checks that each class in use is defined +// in ClusterClass.spec.Workers.MachineDeployments. func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList if desired.Spec.Topology.Workers == nil { @@ -310,6 +311,26 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste machineDeploymentClasses := mdClassNamesFromWorkerClass(clusterClass.Spec.Workers) names := sets.Set[string]{} for i, md := range desired.Spec.Topology.Workers.MachineDeployments { + // The Name must be a valid Kubernetes resource name, because it is used to generate the MachineDeployment name. + if errs := validation.IsDNS1123Subdomain(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 resource name: %s", err), + ), + ) + } + } + + // The Name must also be a valid label value, because it is used in some label values. + // + // NOTE This check always returns true in practice, because OpenAPI validation in the + // Cluster CRD ensures that md.Name is <= 63 characters, and the IsDNS1123Subdomain check + // accepts a smaller set of characters than IsValidLabelValue. We keep this check to be able + // to unit test this function. if errs := validation.IsValidLabelValue(md.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( @@ -317,7 +338,7 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste field.Invalid( field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("name"), md.Name, - fmt.Sprintf("must be a valid label value %s", err), + fmt.Sprintf("must be a valid label value: %s", err), ), ) } @@ -360,8 +381,9 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste return allErrs } -// MachinePoolTopologiesAreValidAndDefinedInClusterClass checks that each MachinePoolTopology name is not empty -// and unique, and each class in use is defined in ClusterClass.spec.Workers.MachinePools. +// MachinePoolTopologiesAreValidAndDefinedInClusterClass checks that each MachinePoolTopology name is not empty, +// is a valid Kubernetes resource name, is a valid label value, and is unique. It also checks that each class in use is defined +// in ClusterClass.spec.Workers.MachinePools. func MachinePoolTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList if desired.Spec.Topology.Workers == nil { @@ -374,6 +396,26 @@ func MachinePoolTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cl machinePoolClasses := mpClassNamesFromWorkerClass(clusterClass.Spec.Workers) names := sets.Set[string]{} for i, mp := range desired.Spec.Topology.Workers.MachinePools { + // The Name must be a valid Kubernetes resource name, because it is used to generate the MachineDeployment name. + if errs := validation.IsDNS1123Subdomain(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 resource name: %s", err), + ), + ) + } + } + + // The Name must also be a valid label value, because it is used in some label values. + // + // NOTE This check always returns true in practice, because OpenAPI validation in the + // Cluster CRD ensures that md.Name is <= 63 characters, and the IsDNS1123Subdomain check + // accepts a smaller set of characters than IsValidLabelValue. We keep this check to be able + // to unit test this function. if errs := validation.IsValidLabelValue(mp.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( @@ -381,7 +423,7 @@ func MachinePoolTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cl field.Invalid( field.NewPath("spec", "topology", "workers", "machinePools").Index(i).Child("name"), mp.Name, - fmt.Sprintf("must be a valid label value %s", err), + fmt.Sprintf("must be a valid label value: %s", err), ), ) } diff --git a/internal/topology/check/compatibility_test.go b/internal/topology/check/compatibility_test.go index 9b38f5613069..40112344b149 100644 --- a/internal/topology/check/compatibility_test.go +++ b/internal/topology/check/compatibility_test.go @@ -1402,6 +1402,66 @@ func TestMachineDeploymentTopologiesAreUniqueAndDefinedInClusterClass(t *testing Build(), wantErr: true, }, + { + name: "fail if MachineDeploymentTopology name is not a valid Kubernetes resource name", + 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("under_score"). + WithClass("aa"). + Build()). + Build()). + Build(), + wantErr: true, + }, + { + name: "fail if MachineDeploymentTopology name is not a valid Kubernetes label value", + 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("forward/slash"). + WithClass("aa"). + Build()). + Build()). + Build(), + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1609,6 +1669,66 @@ func TestMachinePoolTopologiesAreUniqueAndDefinedInClusterClass(t *testing.T) { Build(), wantErr: true, }, + { + name: "fail if MachinePoolTopology name is not a valid Kubernetes resource name", + 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("under_score"). + WithClass("aa"). + Build()). + Build()). + Build(), + wantErr: true, + }, + { + name: "fail if MachinePoolTopology name is not a valid Kubernetes label value", + 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("forward/slash"). + WithClass("aa"). + Build()). + Build()). + Build(), + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index f698f217d9eb..3f5d191d91cb 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -1849,6 +1849,90 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), }, + { + name: "should return error when MachineDeploymentTopology name is not a valid Kubernetes resource name", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("under_score"). + WithClass("aa"). + Build()). + Build()). + Build(), + }, + { + name: "should return error when MachinePoolTopology name is not a valid Kubernetes resource name", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachinePool( + builder.MachinePoolTopology("under_score"). + WithClass("aa"). + Build()). + Build()). + Build(), + }, + { + name: "should return error when MachineDeploymentTopology name is not a valid Kubernetes label value", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("forward/slash"). + WithClass("aa"). + Build()). + Build()). + Build(), + }, + { + name: "should return error when MachinePoolTopology name is not a valid Kubernetes label value", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachinePool( + builder.MachinePoolTopology("forward/slash"). + WithClass("aa"). + Build()). + Build()). + Build(), + }, + { + name: "should return error when MachineDeploymentTopology name exceeds 63 characters (the maximum length of a Kubernetes label value)", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachineDeployment( + builder.MachineDeploymentTopology("thisnameisreallymuchlongerthanthemaximumlengthofsixtythreecharacters"). + WithClass("aa"). + Build()). + Build()). + Build(), + }, + { + name: "should return error when MachinePoolTopology name exceeds 63 characters (the maximum length of a Kubernetes label value)", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithMachinePool( + builder.MachinePoolTopology("thisnameisreallymuchlongerthanthemaximumlengthofsixtythreecharacters"). + WithClass("aa"). + Build()). + Build()). + Build(), + }, { name: "should update", expectErr: false,