From 242cebdb07174e8bf1f43245dfc5f32bb5d507b8 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Mon, 7 Apr 2025 13:18:59 -0700 Subject: [PATCH 1/8] Add tests that fail when a worker topology name causes invalid Kubernetes resource name The worker topology name is used to generate the name of a Kubernetes resource (MachineDelpoyment or MachinePool), and must therefore be a valid Kubernetes resource name. --- internal/webhooks/cluster_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 6731dd5c1f02..fe74956801d6 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -1872,6 +1872,34 @@ 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 update", expectErr: false, From 0079c983254db431acdd2f59842beeabc279d35d Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Mon, 7 Apr 2025 12:28:43 -0700 Subject: [PATCH 2/8] Add validation that ensures worker topology names are valid Kubernetes resource names --- internal/topology/check/compatibility.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go index 4a093e58d249..71768e5f75a0 100644 --- a/internal/topology/check/compatibility.go +++ b/internal/topology/check/compatibility.go @@ -276,8 +276,8 @@ 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 unique, and 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 { @@ -290,7 +290,8 @@ 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 { + // The Name must be a valid Kubernetes resource name, because it is used to generate the MachineDeployment name. + if errs := validation.IsDNS1123Label(md.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( allErrs, @@ -340,8 +341,8 @@ 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 unique, and 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 { @@ -354,7 +355,8 @@ 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 { + // The Name must be a valid Kubernetes resource name, because it is used to generate the MachinePool name. + if errs := validation.IsDNS1123Label(mp.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( allErrs, From efe96878760873992d7c47a145e4d83924e04daa Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Wed, 9 Apr 2025 09:22:50 -0700 Subject: [PATCH 3/8] fixup! Add validation that ensures worker topology names are valid Kubernetes resource names Use IsDNS1123Subdomain --- internal/topology/check/compatibility.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go index 71768e5f75a0..296aad596911 100644 --- a/internal/topology/check/compatibility.go +++ b/internal/topology/check/compatibility.go @@ -291,7 +291,7 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste 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.IsDNS1123Label(md.Name); len(errs) != 0 { + if errs := validation.IsDNS1123Subdomain(md.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( allErrs, @@ -356,7 +356,7 @@ func MachinePoolTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cl 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 MachinePool name. - if errs := validation.IsDNS1123Label(mp.Name); len(errs) != 0 { + if errs := validation.IsDNS1123Subdomain(mp.Name); len(errs) != 0 { for _, err := range errs { allErrs = append( allErrs, From fbc09ec4c48e531689ba6467e2a255927569199e Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Wed, 9 Apr 2025 11:07:04 -0700 Subject: [PATCH 4/8] fixup! Add validation that ensures worker topology names are valid Kubernetes resource names Check that Name is both a valid Kubernetes resource name, and a valid label value --- internal/topology/check/compatibility.go | 40 +++++++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go index 296aad596911..fcc68f0d4f15 100644 --- a/internal/topology/check/compatibility.go +++ b/internal/topology/check/compatibility.go @@ -277,7 +277,8 @@ func MachinePoolClassesAreUnique(clusterClass *clusterv1.ClusterClass) field.Err } // MachineDeploymentTopologiesAreValidAndDefinedInClusterClass checks that each MachineDeploymentTopology name is not empty, -// is a valid Kubernetes resource name, is unique, and each class in use is defined in ClusterClass.spec.Workers.MachineDeployments. +// 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 { @@ -298,7 +299,21 @@ 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 resource name: %s", err), + ), + ) + } + } + + // The Name must also be a valid label value, because it is used in some label values. + 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), ), ) } @@ -342,7 +357,8 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste } // MachinePoolTopologiesAreValidAndDefinedInClusterClass checks that each MachinePoolTopology name is not empty, -// is a valid Kubernetes resource name, is unique, and each class in use is defined in ClusterClass.spec.Workers.MachinePools. +// 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 { @@ -355,7 +371,7 @@ 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 MachinePool name. + // 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( @@ -363,7 +379,21 @@ 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 resource name: %s", err), + ), + ) + } + } + + // The Name must also be a valid label value, because it is used in some label values. + 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), ), ) } From 2a930cc5bfe3607ce759a76a52483a157f417157 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Wed, 9 Apr 2025 11:07:43 -0700 Subject: [PATCH 5/8] fixup! Add tests that fail when a worker topology name causes invalid Kubernetes resource name Add tests for maximum length and invalid characters in a label value --- internal/webhooks/cluster_test.go | 56 +++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index fe74956801d6..24324857e692 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -1900,6 +1900,62 @@ func TestClusterTopologyValidation(t *testing.T) { 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, From 70beedaf78e4222ec109708d845fe019b5621810 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Thu, 10 Apr 2025 09:52:17 -0700 Subject: [PATCH 6/8] fixup! Add tests that fail when a worker topology name causes invalid Kubernetes resource name Test for max length should fail due to max length, not due to uppercase characters --- internal/webhooks/cluster_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 24324857e692..e1a8590335b4 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -1936,7 +1936,7 @@ func TestClusterTopologyValidation(t *testing.T) { WithClass("foo"). WithVersion("v1.19.1"). WithMachineDeployment( - builder.MachineDeploymentTopology("thisNameIsReallyMuchLongerThanTheMaximumLengthOfSixtyThreeCharacters"). + builder.MachineDeploymentTopology("thisnameisreallymuchlongerthanthemaximumlengthofsixtythreecharacters"). WithClass("aa"). Build()). Build()). @@ -1950,7 +1950,7 @@ func TestClusterTopologyValidation(t *testing.T) { WithClass("foo"). WithVersion("v1.19.1"). WithMachinePool( - builder.MachinePoolTopology("thisNameIsReallyMuchLongerThanTheMaximumLengthOfSixtyThreeCharacters"). + builder.MachinePoolTopology("thisnameisreallymuchlongerthanthemaximumlengthofsixtythreecharacters"). WithClass("aa"). Build()). Build()). From c2064570c935c6d122f6f5cb93bf0dae603e0e3d Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Fri, 11 Apr 2025 08:02:25 -0700 Subject: [PATCH 7/8] fixup! Add validation that ensures worker topology names are valid Kubernetes resource names Explain why we use IsValidLabelValue check --- internal/topology/check/compatibility.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/topology/check/compatibility.go b/internal/topology/check/compatibility.go index fcc68f0d4f15..9eaa18031f6f 100644 --- a/internal/topology/check/compatibility.go +++ b/internal/topology/check/compatibility.go @@ -306,6 +306,11 @@ func MachineDeploymentTopologiesAreValidAndDefinedInClusterClass(desired *cluste } // 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( @@ -386,6 +391,11 @@ func MachinePoolTopologiesAreValidAndDefinedInClusterClass(desired *clusterv1.Cl } // 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( From 2ad7c266ab437301f6230ae53ea879bcb8729bda Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Fri, 11 Apr 2025 08:09:38 -0700 Subject: [PATCH 8/8] fixup! Add tests that fail when a worker topology name causes invalid Kubernetes resource name Add tests to check package --- internal/topology/check/compatibility_test.go | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/internal/topology/check/compatibility_test.go b/internal/topology/check/compatibility_test.go index bc4e6de32086..5b4b4cbd80a7 100644 --- a/internal/topology/check/compatibility_test.go +++ b/internal/topology/check/compatibility_test.go @@ -1333,6 +1333,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) { @@ -1540,6 +1600,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) {