From bf0b2e551efbaffe344f426578b29386320abb0f Mon Sep 17 00:00:00 2001 From: arshadda Date: Tue, 18 Feb 2025 19:53:39 +0530 Subject: [PATCH 1/5] Add Namingstrategy to infracluster --- api/v1beta1/clusterclass_types.go | 17 +++++ api/v1beta1/zz_generated.deepcopy.go | 25 ++++++++ api/v1beta1/zz_generated.openapi.go | 26 ++++++++ .../cluster.x-k8s.io_clusterclasses.yaml | 15 +++++ exp/topology/desiredstate/desired_state.go | 7 ++- internal/apis/core/v1alpha4/conversion.go | 1 + .../core/v1alpha4/zz_generated.conversion.go | 1 + internal/topology/names/names.go | 10 +++ internal/webhooks/clusterclass.go | 40 ++++++++++++ internal/webhooks/clusterclass_test.go | 62 +++++++++++++++++++ .../clusterclass-quick-start-runtimesdk.yaml | 2 + util/test/builder/builders.go | 10 +++ util/test/builder/zz_generated.deepcopy.go | 5 ++ 13 files changed, 220 insertions(+), 1 deletion(-) diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go index b637fcc26126..bff799bfd7e6 100644 --- a/api/v1beta1/clusterclass_types.go +++ b/api/v1beta1/clusterclass_types.go @@ -100,6 +100,10 @@ type ClusterClassSpec struct { // +optional Infrastructure LocalObjectTemplate `json:"infrastructure,omitempty"` + // infrastructureNamingStrategy allows changing the naming pattern used when creating the infrastructure object. + // +optional + InfrastructureNamingStrategy *InfrastructuresNamingStrategy `json:"infrastructureNamingStrategy,omitempty"` + // controlPlane is a reference to a local struct that holds the details // for provisioning the Control Plane for the Cluster. // +optional @@ -207,6 +211,19 @@ type ControlPlaneClassNamingStrategy struct { Template *string `json:"template,omitempty"` } +// InfrastructuresNamingStrategy defines the naming strategy for infrastructure objects. +type InfrastructuresNamingStrategy struct { + // template defines the template to use for generating the name of the Infrastructure object. + // If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. + // If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will + // get concatenated with a random suffix of length 5. + // The templating mechanism provides the following arguments: + // * `.cluster.name`: The name of the cluster object. + // * `.random`: A random alphanumeric string, without vowels, of length 5. + // +optional + Template *string `json:"template,omitempty"` +} + // WorkersClass is a collection of deployment classes. type WorkersClass struct { // machineDeployments is a list of machine deployment classes that can be used to create diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 7b6656d83d1e..9b5c62f3d3a5 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -211,6 +211,11 @@ func (in *ClusterClassSpec) DeepCopyInto(out *ClusterClassSpec) { copy(*out, *in) } in.Infrastructure.DeepCopyInto(&out.Infrastructure) + if in.InfrastructureNamingStrategy != nil { + in, out := &in.InfrastructureNamingStrategy, &out.InfrastructureNamingStrategy + *out = new(InfrastructuresNamingStrategy) + (*in).DeepCopyInto(*out) + } in.ControlPlane.DeepCopyInto(&out.ControlPlane) in.Workers.DeepCopyInto(&out.Workers) if in.Variables != nil { @@ -877,6 +882,26 @@ func (in FailureDomains) DeepCopy() FailureDomains { return *out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *InfrastructuresNamingStrategy) DeepCopyInto(out *InfrastructuresNamingStrategy) { + *out = *in + if in.Template != nil { + in, out := &in.Template, &out.Template + *out = new(string) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InfrastructuresNamingStrategy. +func (in *InfrastructuresNamingStrategy) DeepCopy() *InfrastructuresNamingStrategy { + if in == nil { + return nil + } + out := new(InfrastructuresNamingStrategy) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *JSONPatch) DeepCopyInto(out *JSONPatch) { *out = *in diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index eceda3181e20..94aa922fb675 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -56,6 +56,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "sigs.k8s.io/cluster-api/api/v1beta1.ControlPlaneVariables": schema_sigsk8sio_cluster_api_api_v1beta1_ControlPlaneVariables(ref), "sigs.k8s.io/cluster-api/api/v1beta1.ExternalPatchDefinition": schema_sigsk8sio_cluster_api_api_v1beta1_ExternalPatchDefinition(ref), "sigs.k8s.io/cluster-api/api/v1beta1.FailureDomainSpec": schema_sigsk8sio_cluster_api_api_v1beta1_FailureDomainSpec(ref), + "sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy": schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructuresNamingStrategy(ref), "sigs.k8s.io/cluster-api/api/v1beta1.JSONPatch": schema_sigsk8sio_cluster_api_api_v1beta1_JSONPatch(ref), "sigs.k8s.io/cluster-api/api/v1beta1.JSONPatchValue": schema_sigsk8sio_cluster_api_api_v1beta1_JSONPatchValue(ref), "sigs.k8s.io/cluster-api/api/v1beta1.JSONSchemaProps": schema_sigsk8sio_cluster_api_api_v1beta1_JSONSchemaProps(ref), @@ -441,6 +442,11 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_ClusterClassSpec(ref common.Refere Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.LocalObjectTemplate"), }, }, + "infrastructureNamingStrategy": { + SchemaProps: spec.SchemaProps{ + Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy"), + }, + }, "controlPlane": { SchemaProps: spec.SchemaProps{ Description: "controlPlane is a reference to a local struct that holds the details for provisioning the Control Plane for the Cluster.", @@ -1530,6 +1536,26 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_FailureDomainSpec(ref common.Refer } } +func schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructuresNamingStrategy(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "InfrastructuresNamingStrategy defines the naming strategy for infrastructure objects.", + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "template": { + SchemaProps: spec.SchemaProps{ + Description: "template defines the template to use for generating the name of the Infrastructure object. If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object. * `.random`: A random alphanumeric string, without vowels, of length 5.", + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, + } +} + func schema_sigsk8sio_cluster_api_api_v1beta1_JSONPatch(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ diff --git a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml index 2dba022d5072..a195cd618b74 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml @@ -859,6 +859,21 @@ spec: required: - ref type: object + infrastructureNamingStrategy: + description: InfrastructuresNamingStrategy defines the naming strategy + for infrastructure objects. + properties: + template: + description: |- + template defines the template to use for generating the name of the Infrastructure object. + If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. + If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will + get concatenated with a random suffix of length 5. + The templating mechanism provides the following arguments: + * `.cluster.name`: The name of the cluster object. + * `.random`: A random alphanumeric string, without vowels, of length 5. + type: string + type: object patches: description: |- patches defines the patches which are applied to customize diff --git a/exp/topology/desiredstate/desired_state.go b/exp/topology/desiredstate/desired_state.go index a96dc5b13ec3..3f1bf6133cde 100644 --- a/exp/topology/desiredstate/desired_state.go +++ b/exp/topology/desiredstate/desired_state.go @@ -193,11 +193,16 @@ func computeInfrastructureCluster(_ context.Context, s *scope.Scope) (*unstructu cluster := s.Current.Cluster currentRef := cluster.Spec.InfrastructureRef + nameTemplate := "{{ .cluster.name }}-{{ .random }}" + if s.Blueprint.ClusterClass.Spec.InfrastructureNamingStrategy != nil && s.Blueprint.ClusterClass.Spec.InfrastructureNamingStrategy.Template != nil { + nameTemplate = *s.Blueprint.ClusterClass.Spec.InfrastructureNamingStrategy.Template + } + infrastructureCluster, err := templateToObject(templateToInput{ template: template, templateClonedFromRef: templateClonedFromRef, cluster: cluster, - nameGenerator: topologynames.SimpleNameGenerator(fmt.Sprintf("%s-", cluster.Name)), + nameGenerator: topologynames.InfraClusterNameGenerator(nameTemplate, cluster.Name, s.Blueprint.InfrastructureClusterTemplate.GetName()), currentObjectRef: currentRef, // Note: It is not possible to add an ownerRef to Cluster at this stage, otherwise the provisioning // of the infrastructure cluster starts no matter of the object being actually referenced by the Cluster itself. diff --git a/internal/apis/core/v1alpha4/conversion.go b/internal/apis/core/v1alpha4/conversion.go index f006f2895a65..6f0a1f49a998 100644 --- a/internal/apis/core/v1alpha4/conversion.go +++ b/internal/apis/core/v1alpha4/conversion.go @@ -133,6 +133,7 @@ func (src *ClusterClass) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.ControlPlane.MachineHealthCheck = restored.Spec.ControlPlane.MachineHealthCheck dst.Spec.ControlPlane.ReadinessGates = restored.Spec.ControlPlane.ReadinessGates dst.Spec.ControlPlane.NamingStrategy = restored.Spec.ControlPlane.NamingStrategy + dst.Spec.InfrastructureNamingStrategy = restored.Spec.InfrastructureNamingStrategy dst.Spec.ControlPlane.NodeDrainTimeout = restored.Spec.ControlPlane.NodeDrainTimeout dst.Spec.ControlPlane.NodeVolumeDetachTimeout = restored.Spec.ControlPlane.NodeVolumeDetachTimeout dst.Spec.ControlPlane.NodeDeletionTimeout = restored.Spec.ControlPlane.NodeDeletionTimeout diff --git a/internal/apis/core/v1alpha4/zz_generated.conversion.go b/internal/apis/core/v1alpha4/zz_generated.conversion.go index ca7076623f9e..c5bbfcfd5d90 100644 --- a/internal/apis/core/v1alpha4/zz_generated.conversion.go +++ b/internal/apis/core/v1alpha4/zz_generated.conversion.go @@ -646,6 +646,7 @@ func autoConvert_v1beta1_ClusterClassSpec_To_v1alpha4_ClusterClassSpec(in *v1bet if err := Convert_v1beta1_LocalObjectTemplate_To_v1alpha4_LocalObjectTemplate(&in.Infrastructure, &out.Infrastructure, s); err != nil { return err } + // WARNING: in.InfrastructureNamingStrategy requires manual conversion: does not exist in peer-type if err := Convert_v1beta1_ControlPlaneClass_To_v1alpha4_ControlPlaneClass(&in.ControlPlane, &out.ControlPlane, s); err != nil { return err } diff --git a/internal/topology/names/names.go b/internal/topology/names/names.go index f6d4259b7e8f..671f9355d8dc 100644 --- a/internal/topology/names/names.go +++ b/internal/topology/names/names.go @@ -106,6 +106,16 @@ func MachineSetMachineNameGenerator(templateString, clusterName, machineSetName }) } +// InfraClusterNameGenerator returns a generator for creating a infrastructure cluster name. +func InfraClusterNameGenerator(templateString, clusterName, infraClusterName string) NameGenerator { + return newTemplateGenerator(templateString, clusterName, + map[string]interface{}{ + "infraCluster": map[string]interface{}{ + "name": infraClusterName, + }, + }) +} + // templateGenerator parses the template string as text/template and executes it using // the passed data to generate a name. type templateGenerator struct { diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 544f2dbd500a..4d427104c271 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -154,6 +154,7 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC ) } var allErrs field.ErrorList + specPath := field.NewPath("spec") // Ensure all references are valid. allErrs = append(allErrs, check.ClusterClassReferencesAreValid(newClusterClass)...) @@ -170,6 +171,10 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC // Ensure NamingStrategies are valid. allErrs = append(allErrs, validateNamingStrategies(newClusterClass)...) + if newClusterClass.Spec.InfrastructureNamingStrategy != nil { + allErrs = append(allErrs, validateInfraClusterNamingStrategy(newClusterClass.Spec.InfrastructureNamingStrategy, specPath.Child("infraCluster"))...) + } + // Validate variables. var oldClusterClassVariables []clusterv1.ClusterClassVariable if oldClusterClass != nil { @@ -220,6 +225,41 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC return nil } +func validateInfraClusterNamingStrategy(infraClusterNamingStrategy *clusterv1.InfrastructuresNamingStrategy, pathPrefix *field.Path) field.ErrorList { + var allErrs field.ErrorList + + if infraClusterNamingStrategy.Template != nil { + if !strings.Contains(*infraClusterNamingStrategy.Template, "{{ .random }}") { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("template"), + infraClusterNamingStrategy.Template, + "invalid template, {{ .random }} is missing", + )) + } + name, err := topologynames.InfraClusterNameGenerator(*infraClusterNamingStrategy.Template, "cluster", "infraCluster").GenerateName() + if err != nil { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("template"), + infraClusterNamingStrategy.Template, + fmt.Sprintf("invalid template: %v", err), + )) + } else { + for _, err := range validation.IsDNS1123Subdomain(name) { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("template"), + infraClusterNamingStrategy.Template, + fmt.Sprintf("invalid template, generated names would not be valid Kubernetes object names: %v", err), + )) + } + } + } + + return allErrs +} + // validateUpdatesToMachineHealthCheckClasses checks if the updates made to MachineHealthChecks are valid. // It makes sure that if a MachineHealthCheck definition is dropped from the ClusterClass then none of the // clusters using the ClusterClass rely on it to create a MachineHealthCheck. diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index aeb4a3249f6b..0f3ea537466c 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1670,6 +1670,68 @@ func TestClusterClassValidation(t *testing.T) { Build(), expectErr: false, }, + { + name: "should not return error for valid namingStrategy.template", + in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("{{ .cluster.name }}-infra-{{ .random }}")}). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithControlPlaneNamingStrategy(&clusterv1.ControlPlaneClassNamingStrategy{Template: ptr.To("{{ .cluster.name }}-cp-{{ .random }}")}). + 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()). + WithNamingStrategy(&clusterv1.MachineDeploymentClassNamingStrategy{Template: ptr.To("{{ .cluster.name }}-md-{{ .machineDeployment.topologyName }}-{{ .random }}")}). + Build()). + WithWorkerMachinePoolClasses( + *builder.MachinePoolClass("bb"). + WithInfrastructureTemplate( + builder.InfrastructureMachinePoolTemplate(metav1.NamespaceDefault, "infra2").Build()). + WithBootstrapTemplate( + builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap2").Build()). + WithNamingStrategy(&clusterv1.MachinePoolClassNamingStrategy{Template: ptr.To("{{ .cluster.name }}-md-{{ .machinePool.topologyName }}-{{ .random }}")}). + Build()). + Build(), + expectErr: false, + }, + { + name: "should return error for invalid InfraCluster InfrastructuresNamingStrategy.template", + in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("template-infra-{{ .invalidkey }}")}). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithControlPlaneInfrastructureMachineTemplate( + builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfra1"). + Build()). + Build(), + expectErr: true, + }, + { + name: "should return error for invalid InfraCluster InfrastructuresNamingStrategy.template when the generated name does not conform to RFC 1123", + in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("template-infra-{{ .invalidkey }}-")}). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithControlPlaneInfrastructureMachineTemplate( + builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfra1"). + Build()). + Build(), + expectErr: true, + }, { name: "should return error for invalid ControlPlane namingStrategy.template", in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). diff --git a/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml b/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml index 538a26ddca8a..f8181538ecaa 100644 --- a/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml +++ b/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start-runtimesdk.yaml @@ -20,6 +20,8 @@ spec: apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 kind: DockerClusterTemplate name: quick-start-cluster + infrastructureNamingStrategy: + template: "{{ .cluster.name }}-infra-{{ .random }}" workers: machineDeployments: - class: default-worker diff --git a/util/test/builder/builders.go b/util/test/builder/builders.go index 91f843040787..ba894a25efd2 100644 --- a/util/test/builder/builders.go +++ b/util/test/builder/builders.go @@ -347,6 +347,7 @@ type ClusterClassBuilder struct { controlPlaneNodeVolumeDetachTimeout *metav1.Duration controlPlaneNodeDeletionTimeout *metav1.Duration controlPlaneNamingStrategy *clusterv1.ControlPlaneClassNamingStrategy + infraClusterNamingStrategy *clusterv1.InfrastructuresNamingStrategy machineDeploymentClasses []clusterv1.MachineDeploymentClass machinePoolClasses []clusterv1.MachinePoolClass variables []clusterv1.ClusterClassVariable @@ -426,6 +427,12 @@ func (c *ClusterClassBuilder) WithControlPlaneNamingStrategy(n *clusterv1.Contro return c } +// WithInfraClusterStrategy sets the NamingStrategy for the infra cluster to the ClusterClassBuilder. +func (c *ClusterClassBuilder) WithInfraClusterStrategy(n *clusterv1.InfrastructuresNamingStrategy) *ClusterClassBuilder { + c.infraClusterNamingStrategy = n + return c +} + // WithVariables adds the Variables to the ClusterClassBuilder. func (c *ClusterClassBuilder) WithVariables(vars ...clusterv1.ClusterClassVariable) *ClusterClassBuilder { c.variables = vars @@ -524,6 +531,9 @@ func (c *ClusterClassBuilder) Build() *clusterv1.ClusterClass { if c.controlPlaneNamingStrategy != nil { obj.Spec.ControlPlane.NamingStrategy = c.controlPlaneNamingStrategy } + if c.infraClusterNamingStrategy != nil { + obj.Spec.InfrastructureNamingStrategy = c.infraClusterNamingStrategy + } obj.Spec.Workers.MachineDeployments = c.machineDeploymentClasses obj.Spec.Workers.MachinePools = c.machinePoolClasses diff --git a/util/test/builder/zz_generated.deepcopy.go b/util/test/builder/zz_generated.deepcopy.go index 1a7ed63726f7..1f156fc4404a 100644 --- a/util/test/builder/zz_generated.deepcopy.go +++ b/util/test/builder/zz_generated.deepcopy.go @@ -163,6 +163,11 @@ func (in *ClusterClassBuilder) DeepCopyInto(out *ClusterClassBuilder) { *out = new(v1beta1.ControlPlaneClassNamingStrategy) (*in).DeepCopyInto(*out) } + if in.infraClusterNamingStrategy != nil { + in, out := &in.infraClusterNamingStrategy, &out.infraClusterNamingStrategy + *out = new(v1beta1.InfrastructuresNamingStrategy) + (*in).DeepCopyInto(*out) + } if in.machineDeploymentClasses != nil { in, out := &in.machineDeploymentClasses, &out.machineDeploymentClasses *out = make([]v1beta1.MachineDeploymentClass, len(*in)) From 8a33973368315971f9c2f43cb189caf68884d5bd Mon Sep 17 00:00:00 2001 From: arshadda Date: Wed, 26 Feb 2025 10:36:29 +0530 Subject: [PATCH 2/5] address review comments --- api/v1beta1/zz_generated.openapi.go | 3 +- .../cluster.x-k8s.io_clusterclasses.yaml | 4 +-- internal/webhooks/clusterclass.go | 2 +- internal/webhooks/clusterclass_test.go | 33 +------------------ 4 files changed, 6 insertions(+), 36 deletions(-) diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 94aa922fb675..4bea8e91de22 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -444,7 +444,8 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_ClusterClassSpec(ref common.Refere }, "infrastructureNamingStrategy": { SchemaProps: spec.SchemaProps{ - Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy"), + Description: "infrastructureNamingStrategy allows changing the naming pattern used when creating the infrastructure object.", + Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy"), }, }, "controlPlane": { diff --git a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml index a195cd618b74..8d6410a38a50 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml @@ -860,8 +860,8 @@ spec: - ref type: object infrastructureNamingStrategy: - description: InfrastructuresNamingStrategy defines the naming strategy - for infrastructure objects. + description: infrastructureNamingStrategy allows changing the naming + pattern used when creating the infrastructure object. properties: template: description: |- diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 4d427104c271..4533370d9a2e 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -172,7 +172,7 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC allErrs = append(allErrs, validateNamingStrategies(newClusterClass)...) if newClusterClass.Spec.InfrastructureNamingStrategy != nil { - allErrs = append(allErrs, validateInfraClusterNamingStrategy(newClusterClass.Spec.InfrastructureNamingStrategy, specPath.Child("infraCluster"))...) + allErrs = append(allErrs, validateInfraClusterNamingStrategy(newClusterClass.Spec.InfrastructureNamingStrategy, specPath.Child("infrastructureNamingStrategy"))...) } // Validate variables. diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index 0f3ea537466c..442b77b67c6c 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1639,37 +1639,6 @@ func TestClusterClassValidation(t *testing.T) { Build(), expectErr: true, }, - { - name: "should not return error for valid namingStrategy.template", - in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). - WithInfrastructureClusterTemplate( - builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithControlPlaneTemplate( - builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). - Build()). - WithControlPlaneNamingStrategy(&clusterv1.ControlPlaneClassNamingStrategy{Template: ptr.To("{{ .cluster.name }}-cp-{{ .random }}")}). - 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()). - WithNamingStrategy(&clusterv1.MachineDeploymentClassNamingStrategy{Template: ptr.To("{{ .cluster.name }}-md-{{ .machineDeployment.topologyName }}-{{ .random }}")}). - Build()). - WithWorkerMachinePoolClasses( - *builder.MachinePoolClass("bb"). - WithInfrastructureTemplate( - builder.InfrastructureMachinePoolTemplate(metav1.NamespaceDefault, "infra2").Build()). - WithBootstrapTemplate( - builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap2").Build()). - WithNamingStrategy(&clusterv1.MachinePoolClassNamingStrategy{Template: ptr.To("{{ .cluster.name }}-md-{{ .machinePool.topologyName }}-{{ .random }}")}). - Build()). - Build(), - expectErr: false, - }, { name: "should not return error for valid namingStrategy.template", in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). @@ -1722,7 +1691,7 @@ func TestClusterClassValidation(t *testing.T) { in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("template-infra-{{ .invalidkey }}-")}). + WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("template-infra-{{ .cluster.name }}-")}). WithControlPlaneTemplate( builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). From 615c03dc824f76e803d0107980ae708760094618 Mon Sep 17 00:00:00 2001 From: arshadda Date: Tue, 4 Mar 2025 09:44:38 +0530 Subject: [PATCH 3/5] address review comment --- api/v1beta1/clusterclass_types.go | 3 +-- api/v1beta1/zz_generated.openapi.go | 4 ++-- config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml | 3 +-- internal/webhooks/clusterclass.go | 8 -------- 4 files changed, 4 insertions(+), 14 deletions(-) diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go index bff799bfd7e6..4e3697440b12 100644 --- a/api/v1beta1/clusterclass_types.go +++ b/api/v1beta1/clusterclass_types.go @@ -214,12 +214,11 @@ type ControlPlaneClassNamingStrategy struct { // InfrastructuresNamingStrategy defines the naming strategy for infrastructure objects. type InfrastructuresNamingStrategy struct { // template defines the template to use for generating the name of the Infrastructure object. - // If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. + // If not defined, it will fallback to `{{ .cluster.name }}`. // If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will // get concatenated with a random suffix of length 5. // The templating mechanism provides the following arguments: // * `.cluster.name`: The name of the cluster object. - // * `.random`: A random alphanumeric string, without vowels, of length 5. // +optional Template *string `json:"template,omitempty"` } diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 4bea8e91de22..4c983ae19428 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -494,7 +494,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_ClusterClassSpec(ref common.Refere }, }, Dependencies: []string{ - "sigs.k8s.io/cluster-api/api/v1beta1.ClusterAvailabilityGate", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassPatch", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassVariable", "sigs.k8s.io/cluster-api/api/v1beta1.ControlPlaneClass", "sigs.k8s.io/cluster-api/api/v1beta1.LocalObjectTemplate", "sigs.k8s.io/cluster-api/api/v1beta1.WorkersClass"}, + "sigs.k8s.io/cluster-api/api/v1beta1.ClusterAvailabilityGate", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassPatch", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassVariable", "sigs.k8s.io/cluster-api/api/v1beta1.ControlPlaneClass", "sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.LocalObjectTemplate", "sigs.k8s.io/cluster-api/api/v1beta1.WorkersClass"}, } } @@ -1546,7 +1546,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructuresNamingStrategy(ref Properties: map[string]spec.Schema{ "template": { SchemaProps: spec.SchemaProps{ - Description: "template defines the template to use for generating the name of the Infrastructure object. If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object. * `.random`: A random alphanumeric string, without vowels, of length 5.", + Description: "template defines the template to use for generating the name of the Infrastructure object. If not defined, it will fallback to `{{ .cluster.name }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object.", Type: []string{"string"}, Format: "", }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml index 8d6410a38a50..50f3b698a306 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml @@ -866,12 +866,11 @@ spec: template: description: |- template defines the template to use for generating the name of the Infrastructure object. - If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. + If not defined, it will fallback to `{{ .cluster.name }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object. - * `.random`: A random alphanumeric string, without vowels, of length 5. type: string type: object patches: diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 4533370d9a2e..3938934f11e2 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -229,14 +229,6 @@ func validateInfraClusterNamingStrategy(infraClusterNamingStrategy *clusterv1.In var allErrs field.ErrorList if infraClusterNamingStrategy.Template != nil { - if !strings.Contains(*infraClusterNamingStrategy.Template, "{{ .random }}") { - allErrs = append(allErrs, - field.Invalid( - pathPrefix.Child("template"), - infraClusterNamingStrategy.Template, - "invalid template, {{ .random }} is missing", - )) - } name, err := topologynames.InfraClusterNameGenerator(*infraClusterNamingStrategy.Template, "cluster", "infraCluster").GenerateName() if err != nil { allErrs = append(allErrs, From d270f2bd2682729326f5e94ecde9557d2b46c9f8 Mon Sep 17 00:00:00 2001 From: arshadda Date: Wed, 5 Mar 2025 19:43:02 +0530 Subject: [PATCH 4/5] address review comments --- api/v1beta1/clusterclass_types.go | 9 ++-- api/v1beta1/zz_generated.deepcopy.go | 10 ++-- api/v1beta1/zz_generated.openapi.go | 12 ++--- .../cluster.x-k8s.io_clusterclasses.yaml | 3 +- exp/topology/desiredstate/desired_state.go | 2 +- internal/topology/names/names.go | 8 +-- internal/webhooks/clusterclass.go | 49 +++++++------------ internal/webhooks/clusterclass_test.go | 6 +-- util/test/builder/builders.go | 4 +- util/test/builder/zz_generated.deepcopy.go | 2 +- 10 files changed, 44 insertions(+), 61 deletions(-) diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go index 4e3697440b12..661d571083a9 100644 --- a/api/v1beta1/clusterclass_types.go +++ b/api/v1beta1/clusterclass_types.go @@ -102,7 +102,7 @@ type ClusterClassSpec struct { // infrastructureNamingStrategy allows changing the naming pattern used when creating the infrastructure object. // +optional - InfrastructureNamingStrategy *InfrastructuresNamingStrategy `json:"infrastructureNamingStrategy,omitempty"` + InfrastructureNamingStrategy *InfrastructureNamingStrategy `json:"infrastructureNamingStrategy,omitempty"` // controlPlane is a reference to a local struct that holds the details // for provisioning the Control Plane for the Cluster. @@ -211,14 +211,15 @@ type ControlPlaneClassNamingStrategy struct { Template *string `json:"template,omitempty"` } -// InfrastructuresNamingStrategy defines the naming strategy for infrastructure objects. -type InfrastructuresNamingStrategy struct { +// InfrastructureNamingStrategy defines the naming strategy for infrastructure objects. +type InfrastructureNamingStrategy struct { // template defines the template to use for generating the name of the Infrastructure object. - // If not defined, it will fallback to `{{ .cluster.name }}`. + // If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. // If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will // get concatenated with a random suffix of length 5. // The templating mechanism provides the following arguments: // * `.cluster.name`: The name of the cluster object. + // * `.random`: A random alphanumeric string, without vowels, of length 5. // +optional Template *string `json:"template,omitempty"` } diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 9b5c62f3d3a5..ec25c1dfb000 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -213,7 +213,7 @@ func (in *ClusterClassSpec) DeepCopyInto(out *ClusterClassSpec) { in.Infrastructure.DeepCopyInto(&out.Infrastructure) if in.InfrastructureNamingStrategy != nil { in, out := &in.InfrastructureNamingStrategy, &out.InfrastructureNamingStrategy - *out = new(InfrastructuresNamingStrategy) + *out = new(InfrastructureNamingStrategy) (*in).DeepCopyInto(*out) } in.ControlPlane.DeepCopyInto(&out.ControlPlane) @@ -883,7 +883,7 @@ func (in FailureDomains) DeepCopy() FailureDomains { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *InfrastructuresNamingStrategy) DeepCopyInto(out *InfrastructuresNamingStrategy) { +func (in *InfrastructureNamingStrategy) DeepCopyInto(out *InfrastructureNamingStrategy) { *out = *in if in.Template != nil { in, out := &in.Template, &out.Template @@ -892,12 +892,12 @@ func (in *InfrastructuresNamingStrategy) DeepCopyInto(out *InfrastructuresNaming } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InfrastructuresNamingStrategy. -func (in *InfrastructuresNamingStrategy) DeepCopy() *InfrastructuresNamingStrategy { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InfrastructureNamingStrategy. +func (in *InfrastructureNamingStrategy) DeepCopy() *InfrastructureNamingStrategy { if in == nil { return nil } - out := new(InfrastructuresNamingStrategy) + out := new(InfrastructureNamingStrategy) in.DeepCopyInto(out) return out } diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 4c983ae19428..9c211d64866e 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -56,7 +56,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "sigs.k8s.io/cluster-api/api/v1beta1.ControlPlaneVariables": schema_sigsk8sio_cluster_api_api_v1beta1_ControlPlaneVariables(ref), "sigs.k8s.io/cluster-api/api/v1beta1.ExternalPatchDefinition": schema_sigsk8sio_cluster_api_api_v1beta1_ExternalPatchDefinition(ref), "sigs.k8s.io/cluster-api/api/v1beta1.FailureDomainSpec": schema_sigsk8sio_cluster_api_api_v1beta1_FailureDomainSpec(ref), - "sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy": schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructuresNamingStrategy(ref), + "sigs.k8s.io/cluster-api/api/v1beta1.InfrastructureNamingStrategy": schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructureNamingStrategy(ref), "sigs.k8s.io/cluster-api/api/v1beta1.JSONPatch": schema_sigsk8sio_cluster_api_api_v1beta1_JSONPatch(ref), "sigs.k8s.io/cluster-api/api/v1beta1.JSONPatchValue": schema_sigsk8sio_cluster_api_api_v1beta1_JSONPatchValue(ref), "sigs.k8s.io/cluster-api/api/v1beta1.JSONSchemaProps": schema_sigsk8sio_cluster_api_api_v1beta1_JSONSchemaProps(ref), @@ -445,7 +445,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_ClusterClassSpec(ref common.Refere "infrastructureNamingStrategy": { SchemaProps: spec.SchemaProps{ Description: "infrastructureNamingStrategy allows changing the naming pattern used when creating the infrastructure object.", - Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy"), + Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.InfrastructureNamingStrategy"), }, }, "controlPlane": { @@ -494,7 +494,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_ClusterClassSpec(ref common.Refere }, }, Dependencies: []string{ - "sigs.k8s.io/cluster-api/api/v1beta1.ClusterAvailabilityGate", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassPatch", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassVariable", "sigs.k8s.io/cluster-api/api/v1beta1.ControlPlaneClass", "sigs.k8s.io/cluster-api/api/v1beta1.InfrastructuresNamingStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.LocalObjectTemplate", "sigs.k8s.io/cluster-api/api/v1beta1.WorkersClass"}, + "sigs.k8s.io/cluster-api/api/v1beta1.ClusterAvailabilityGate", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassPatch", "sigs.k8s.io/cluster-api/api/v1beta1.ClusterClassVariable", "sigs.k8s.io/cluster-api/api/v1beta1.ControlPlaneClass", "sigs.k8s.io/cluster-api/api/v1beta1.InfrastructureNamingStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.LocalObjectTemplate", "sigs.k8s.io/cluster-api/api/v1beta1.WorkersClass"}, } } @@ -1537,16 +1537,16 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_FailureDomainSpec(ref common.Refer } } -func schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructuresNamingStrategy(ref common.ReferenceCallback) common.OpenAPIDefinition { +func schema_sigsk8sio_cluster_api_api_v1beta1_InfrastructureNamingStrategy(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ - Description: "InfrastructuresNamingStrategy defines the naming strategy for infrastructure objects.", + Description: "InfrastructureNamingStrategy defines the naming strategy for infrastructure objects.", Type: []string{"object"}, Properties: map[string]spec.Schema{ "template": { SchemaProps: spec.SchemaProps{ - Description: "template defines the template to use for generating the name of the Infrastructure object. If not defined, it will fallback to `{{ .cluster.name }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object.", + Description: "template defines the template to use for generating the name of the Infrastructure object. If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object. * `.random`: A random alphanumeric string, without vowels, of length 5.", Type: []string{"string"}, Format: "", }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml index 50f3b698a306..8d6410a38a50 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml @@ -866,11 +866,12 @@ spec: template: description: |- template defines the template to use for generating the name of the Infrastructure object. - If not defined, it will fallback to `{{ .cluster.name }}`. + If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`. If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. The templating mechanism provides the following arguments: * `.cluster.name`: The name of the cluster object. + * `.random`: A random alphanumeric string, without vowels, of length 5. type: string type: object patches: diff --git a/exp/topology/desiredstate/desired_state.go b/exp/topology/desiredstate/desired_state.go index 3f1bf6133cde..85fdd63a1ef1 100644 --- a/exp/topology/desiredstate/desired_state.go +++ b/exp/topology/desiredstate/desired_state.go @@ -202,7 +202,7 @@ func computeInfrastructureCluster(_ context.Context, s *scope.Scope) (*unstructu template: template, templateClonedFromRef: templateClonedFromRef, cluster: cluster, - nameGenerator: topologynames.InfraClusterNameGenerator(nameTemplate, cluster.Name, s.Blueprint.InfrastructureClusterTemplate.GetName()), + nameGenerator: topologynames.InfraClusterNameGenerator(nameTemplate, cluster.Name), currentObjectRef: currentRef, // Note: It is not possible to add an ownerRef to Cluster at this stage, otherwise the provisioning // of the infrastructure cluster starts no matter of the object being actually referenced by the Cluster itself. diff --git a/internal/topology/names/names.go b/internal/topology/names/names.go index 671f9355d8dc..c43e376c3467 100644 --- a/internal/topology/names/names.go +++ b/internal/topology/names/names.go @@ -107,13 +107,9 @@ func MachineSetMachineNameGenerator(templateString, clusterName, machineSetName } // InfraClusterNameGenerator returns a generator for creating a infrastructure cluster name. -func InfraClusterNameGenerator(templateString, clusterName, infraClusterName string) NameGenerator { +func InfraClusterNameGenerator(templateString, clusterName string) NameGenerator { return newTemplateGenerator(templateString, clusterName, - map[string]interface{}{ - "infraCluster": map[string]interface{}{ - "name": infraClusterName, - }, - }) + map[string]interface{}{}) } // templateGenerator parses the template string as text/template and executes it using diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 3938934f11e2..cd98ec83ea7e 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -154,7 +154,6 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC ) } var allErrs field.ErrorList - specPath := field.NewPath("spec") // Ensure all references are valid. allErrs = append(allErrs, check.ClusterClassReferencesAreValid(newClusterClass)...) @@ -171,10 +170,6 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC // Ensure NamingStrategies are valid. allErrs = append(allErrs, validateNamingStrategies(newClusterClass)...) - if newClusterClass.Spec.InfrastructureNamingStrategy != nil { - allErrs = append(allErrs, validateInfraClusterNamingStrategy(newClusterClass.Spec.InfrastructureNamingStrategy, specPath.Child("infrastructureNamingStrategy"))...) - } - // Validate variables. var oldClusterClassVariables []clusterv1.ClusterClassVariable if oldClusterClass != nil { @@ -225,33 +220,6 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC return nil } -func validateInfraClusterNamingStrategy(infraClusterNamingStrategy *clusterv1.InfrastructuresNamingStrategy, pathPrefix *field.Path) field.ErrorList { - var allErrs field.ErrorList - - if infraClusterNamingStrategy.Template != nil { - name, err := topologynames.InfraClusterNameGenerator(*infraClusterNamingStrategy.Template, "cluster", "infraCluster").GenerateName() - if err != nil { - allErrs = append(allErrs, - field.Invalid( - pathPrefix.Child("template"), - infraClusterNamingStrategy.Template, - fmt.Sprintf("invalid template: %v", err), - )) - } else { - for _, err := range validation.IsDNS1123Subdomain(name) { - allErrs = append(allErrs, - field.Invalid( - pathPrefix.Child("template"), - infraClusterNamingStrategy.Template, - fmt.Sprintf("invalid template, generated names would not be valid Kubernetes object names: %v", err), - )) - } - } - } - - return allErrs -} - // validateUpdatesToMachineHealthCheckClasses checks if the updates made to MachineHealthChecks are valid. // It makes sure that if a MachineHealthCheck definition is dropped from the ClusterClass then none of the // clusters using the ClusterClass rely on it to create a MachineHealthCheck. @@ -467,6 +435,23 @@ func validateMachineHealthCheckClasses(clusterClass *clusterv1.ClusterClass) fie func validateNamingStrategies(clusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList + if clusterClass.Spec.InfrastructureNamingStrategy != nil && clusterClass.Spec.InfrastructureNamingStrategy.Template != nil { + name, err := topologynames.InfraClusterNameGenerator(*clusterClass.Spec.InfrastructureNamingStrategy.Template, "cluster").GenerateName() + templateFldPath := field.NewPath("spec", "infrastructureNamingStrategy", "template") + if err != nil { + allErrs = append(allErrs, + field.Invalid( + templateFldPath, + *clusterClass.Spec.InfrastructureNamingStrategy.Template, + fmt.Sprintf("invalid InfraCluster name template: %v", err), + )) + } else { + for _, err := range validation.IsDNS1123Subdomain(name) { + allErrs = append(allErrs, field.Invalid(templateFldPath, *clusterClass.Spec.InfrastructureNamingStrategy.Template, err)) + } + } + } + if clusterClass.Spec.ControlPlane.NamingStrategy != nil && clusterClass.Spec.ControlPlane.NamingStrategy.Template != nil { name, err := topologynames.ControlPlaneNameGenerator(*clusterClass.Spec.ControlPlane.NamingStrategy.Template, "cluster").GenerateName() templateFldPath := field.NewPath("spec", "controlPlane", "namingStrategy", "template") diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index 442b77b67c6c..daaf6d5f45df 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1644,7 +1644,7 @@ func TestClusterClassValidation(t *testing.T) { in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("{{ .cluster.name }}-infra-{{ .random }}")}). + WithInfraClusterStrategy(&clusterv1.InfrastructureNamingStrategy{Template: ptr.To("{{ .cluster.name }}-infra-{{ .random }}")}). WithControlPlaneTemplate( builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). @@ -1676,7 +1676,7 @@ func TestClusterClassValidation(t *testing.T) { in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("template-infra-{{ .invalidkey }}")}). + WithInfraClusterStrategy(&clusterv1.InfrastructureNamingStrategy{Template: ptr.To("template-infra-{{ .invalidkey }}")}). WithControlPlaneTemplate( builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). @@ -1691,7 +1691,7 @@ func TestClusterClassValidation(t *testing.T) { in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). - WithInfraClusterStrategy(&clusterv1.InfrastructuresNamingStrategy{Template: ptr.To("template-infra-{{ .cluster.name }}-")}). + WithInfraClusterStrategy(&clusterv1.InfrastructureNamingStrategy{Template: ptr.To("template-infra-{{ .cluster.name }}-")}). WithControlPlaneTemplate( builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). Build()). diff --git a/util/test/builder/builders.go b/util/test/builder/builders.go index ba894a25efd2..5835e56b41d4 100644 --- a/util/test/builder/builders.go +++ b/util/test/builder/builders.go @@ -347,7 +347,7 @@ type ClusterClassBuilder struct { controlPlaneNodeVolumeDetachTimeout *metav1.Duration controlPlaneNodeDeletionTimeout *metav1.Duration controlPlaneNamingStrategy *clusterv1.ControlPlaneClassNamingStrategy - infraClusterNamingStrategy *clusterv1.InfrastructuresNamingStrategy + infraClusterNamingStrategy *clusterv1.InfrastructureNamingStrategy machineDeploymentClasses []clusterv1.MachineDeploymentClass machinePoolClasses []clusterv1.MachinePoolClass variables []clusterv1.ClusterClassVariable @@ -428,7 +428,7 @@ func (c *ClusterClassBuilder) WithControlPlaneNamingStrategy(n *clusterv1.Contro } // WithInfraClusterStrategy sets the NamingStrategy for the infra cluster to the ClusterClassBuilder. -func (c *ClusterClassBuilder) WithInfraClusterStrategy(n *clusterv1.InfrastructuresNamingStrategy) *ClusterClassBuilder { +func (c *ClusterClassBuilder) WithInfraClusterStrategy(n *clusterv1.InfrastructureNamingStrategy) *ClusterClassBuilder { c.infraClusterNamingStrategy = n return c } diff --git a/util/test/builder/zz_generated.deepcopy.go b/util/test/builder/zz_generated.deepcopy.go index 1f156fc4404a..af7d0bdb9b76 100644 --- a/util/test/builder/zz_generated.deepcopy.go +++ b/util/test/builder/zz_generated.deepcopy.go @@ -165,7 +165,7 @@ func (in *ClusterClassBuilder) DeepCopyInto(out *ClusterClassBuilder) { } if in.infraClusterNamingStrategy != nil { in, out := &in.infraClusterNamingStrategy, &out.infraClusterNamingStrategy - *out = new(v1beta1.InfrastructuresNamingStrategy) + *out = new(v1beta1.InfrastructureNamingStrategy) (*in).DeepCopyInto(*out) } if in.machineDeploymentClasses != nil { From 6c45c51c1d06371686de3930cc6c97799896eba5 Mon Sep 17 00:00:00 2001 From: arshadda Date: Wed, 5 Mar 2025 22:14:02 +0530 Subject: [PATCH 5/5] address review comment --- internal/webhooks/clusterclass_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index daaf6d5f45df..6447ed8e002f 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1672,7 +1672,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: false, }, { - name: "should return error for invalid InfraCluster InfrastructuresNamingStrategy.template", + name: "should return error for invalid InfraCluster InfrastructureNamingStrategy.template", in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). @@ -1687,7 +1687,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: true, }, { - name: "should return error for invalid InfraCluster InfrastructuresNamingStrategy.template when the generated name does not conform to RFC 1123", + name: "should return error for invalid InfraCluster InfrastructureNamingStrategy.template when the generated name does not conform to RFC 1123", in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate( builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).