From 5438128a9108bbc3933c75f6b893a21e77309914 Mon Sep 17 00:00:00 2001 From: Muhammad Adil Ghaffar Date: Mon, 9 Sep 2024 16:27:14 +0300 Subject: [PATCH] Add NamingStrategy to MachineDeployment Signed-off-by: Muhammad Adil Ghaffar --- api/v1beta1/machinedeployment_types.go | 22 ++ api/v1beta1/zz_generated.deepcopy.go | 20 ++ api/v1beta1/zz_generated.openapi.go | 29 +- .../cluster.x-k8s.io_machinedeployments.yaml | 19 ++ .../core/v1alpha3/zz_generated.conversion.go | 1 + .../core/v1alpha4/zz_generated.conversion.go | 1 + .../machineset/machineset_controller.go | 41 ++- .../machineset/machineset_controller_test.go | 279 ++++++++++++++---- internal/topology/names/names.go | 10 + internal/webhooks/machinedeployment.go | 41 +++ 10 files changed, 394 insertions(+), 69 deletions(-) diff --git a/api/v1beta1/machinedeployment_types.go b/api/v1beta1/machinedeployment_types.go index fe4d4a198edd..286614635a7a 100644 --- a/api/v1beta1/machinedeployment_types.go +++ b/api/v1beta1/machinedeployment_types.go @@ -127,6 +127,11 @@ type MachineDeploymentSpec struct { // +optional Strategy *MachineDeploymentStrategy `json:"strategy,omitempty"` + // MachineNamingStrategy allows changing the naming pattern used when creating Machines. + // Note: InfraMachines will use the same name as the corresponding Machines. + // +optional + MachineNamingStrategy *MachineNamingStrategy `json:"machineNamingStrategy,omitempty"` + // MinReadySeconds is the minimum number of seconds for which a Node for a newly created machine should be ready before considering the replica available. // Defaults to 0 (machine will be considered available as soon as the Node is ready) // +optional @@ -250,6 +255,23 @@ type RemediationStrategy struct { // ANCHOR_END: RemediationStrategy +// MachineNamingStrategy allows changing the naming pattern used when creating Machines. +// Note: InfraMachines will use the same name as the corresponding Machines. +type MachineNamingStrategy struct { + // Template defines the template to use for generating the names of the Machine objects. + // If not defined, it will fallback to `{{ .machineDeployment.name }}-{{ .random }}`. + // If the generated name string exceeds 63 characters, it will be trimmed to 58 characters and will + // get concatenated with a random suffix of length 5. + // Length of the template string must not exceed 256 characters. + // The template allows the following variables `.cluster.name`, `.machineDeployment.name` and `.random`. + // The variable `.cluster.name` retrieves the name of the cluster object that owns the Machines being created. + // The variable `.machineDeployment.name` retrieves the name of the MachineDeployment object that owns the Machines being created. + // The variable `.random` is substituted with random alphanumeric string, without vowels, of length 5. + // +optional + // +kubebuilder:validation:MaxLength=256 + Template string `json:"template,omitempty"` +} + // ANCHOR: MachineDeploymentStatus // MachineDeploymentStatus defines the observed state of MachineDeployment. diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index b08c77422b8d..1c931e2b9d7e 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -1318,6 +1318,11 @@ func (in *MachineDeploymentSpec) DeepCopyInto(out *MachineDeploymentSpec) { *out = new(MachineDeploymentStrategy) (*in).DeepCopyInto(*out) } + if in.MachineNamingStrategy != nil { + in, out := &in.MachineNamingStrategy, &out.MachineNamingStrategy + *out = new(MachineNamingStrategy) + **out = **in + } if in.MinReadySeconds != nil { in, out := &in.MinReadySeconds, &out.MinReadySeconds *out = new(int32) @@ -1764,6 +1769,21 @@ func (in *MachineList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *MachineNamingStrategy) DeepCopyInto(out *MachineNamingStrategy) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MachineNamingStrategy. +func (in *MachineNamingStrategy) DeepCopy() *MachineNamingStrategy { + if in == nil { + return nil + } + out := new(MachineNamingStrategy) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MachinePoolClass) DeepCopyInto(out *MachinePoolClass) { *out = *in diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index a7345be50029..6a0c5bbb55c3 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -82,6 +82,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "sigs.k8s.io/cluster-api/api/v1beta1.MachineHealthCheckTopology": schema_sigsk8sio_cluster_api_api_v1beta1_MachineHealthCheckTopology(ref), "sigs.k8s.io/cluster-api/api/v1beta1.MachineHealthCheckV1Beta2Status": schema_sigsk8sio_cluster_api_api_v1beta1_MachineHealthCheckV1Beta2Status(ref), "sigs.k8s.io/cluster-api/api/v1beta1.MachineList": schema_sigsk8sio_cluster_api_api_v1beta1_MachineList(ref), + "sigs.k8s.io/cluster-api/api/v1beta1.MachineNamingStrategy": schema_sigsk8sio_cluster_api_api_v1beta1_MachineNamingStrategy(ref), "sigs.k8s.io/cluster-api/api/v1beta1.MachinePoolClass": schema_sigsk8sio_cluster_api_api_v1beta1_MachinePoolClass(ref), "sigs.k8s.io/cluster-api/api/v1beta1.MachinePoolClassNamingStrategy": schema_sigsk8sio_cluster_api_api_v1beta1_MachinePoolClassNamingStrategy(ref), "sigs.k8s.io/cluster-api/api/v1beta1.MachinePoolClassTemplate": schema_sigsk8sio_cluster_api_api_v1beta1_MachinePoolClassTemplate(ref), @@ -2213,6 +2214,12 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_MachineDeploymentSpec(ref common.R Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.MachineDeploymentStrategy"), }, }, + "machineNamingStrategy": { + SchemaProps: spec.SchemaProps{ + Description: "MachineNamingStrategy allows changing the naming pattern used when creating Machines. Note: InfraMachines will use the same name as the corresponding Machines.", + Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.MachineNamingStrategy"), + }, + }, "minReadySeconds": { SchemaProps: spec.SchemaProps{ Description: "MinReadySeconds is the minimum number of seconds for which a Node for a newly created machine should be ready before considering the replica available. Defaults to 0 (machine will be considered available as soon as the Node is ready)", @@ -2246,7 +2253,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_MachineDeploymentSpec(ref common.R }, }, Dependencies: []string{ - "k8s.io/apimachinery/pkg/apis/meta/v1.LabelSelector", "k8s.io/apimachinery/pkg/apis/meta/v1.Time", "sigs.k8s.io/cluster-api/api/v1beta1.MachineDeploymentStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.MachineTemplateSpec"}, + "k8s.io/apimachinery/pkg/apis/meta/v1.LabelSelector", "k8s.io/apimachinery/pkg/apis/meta/v1.Time", "sigs.k8s.io/cluster-api/api/v1beta1.MachineDeploymentStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.MachineNamingStrategy", "sigs.k8s.io/cluster-api/api/v1beta1.MachineTemplateSpec"}, } } @@ -3021,6 +3028,26 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_MachineList(ref common.ReferenceCa } } +func schema_sigsk8sio_cluster_api_api_v1beta1_MachineNamingStrategy(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "MachineNamingStrategy allows changing the naming pattern used when creating Machines. Note: InfraMachines will use the same name as the corresponding Machines.", + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "template": { + SchemaProps: spec.SchemaProps{ + Description: "Template defines the template to use for generating the names of the Machine objects. If not defined, it will fallback to `{{ .machineDeployment.name }}-{{ .random }}`. If the generated name string exceeds 63 characters, it will be trimmed to 58 characters and will get concatenated with a random suffix of length 5. Length of the template string must not exceed 256 characters. The template allows the following variables `.cluster.name`, `.machineDeployment.name` and `.random`. The variable `.cluster.name` retrieves the name of the cluster object that owns the Machines being created. The variable `.machineDeployment.name` retrieves the name of the MachineDeployment object that owns the Machines being created. The variable `.random` is substituted with random alphanumeric string, without vowels, of length 5.", + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, + } +} + func schema_sigsk8sio_cluster_api_api_v1beta1_MachinePoolClass(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ diff --git a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml index f71420031e22..c91a497dcabc 100644 --- a/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml +++ b/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml @@ -1120,6 +1120,25 @@ spec: to. minLength: 1 type: string + machineNamingStrategy: + description: |- + MachineNamingStrategy allows changing the naming pattern used when creating Machines. + Note: InfraMachines will use the same name as the corresponding Machines. + properties: + template: + description: |- + Template defines the template to use for generating the names of the Machine objects. + If not defined, it will fallback to `{{ .machineDeployment.name }}-{{ .random }}`. + If the generated name string exceeds 63 characters, it will be trimmed to 58 characters and will + get concatenated with a random suffix of length 5. + Length of the template string must not exceed 256 characters. + The template allows the following variables `.cluster.name`, `.machineDeployment.name` and `.random`. + The variable `.cluster.name` retrieves the name of the cluster object that owns the Machines being created. + The variable `.machineDeployment.name` retrieves the name of the MachineDeployment object that owns the Machines being created. + The variable `.random` is substituted with random alphanumeric string, without vowels, of length 5. + maxLength: 256 + type: string + type: object minReadySeconds: description: |- MinReadySeconds is the minimum number of seconds for which a Node for a newly created machine should be ready before considering the replica available. diff --git a/internal/apis/core/v1alpha3/zz_generated.conversion.go b/internal/apis/core/v1alpha3/zz_generated.conversion.go index 23d631ab59e9..64e01cfa08ca 100644 --- a/internal/apis/core/v1alpha3/zz_generated.conversion.go +++ b/internal/apis/core/v1alpha3/zz_generated.conversion.go @@ -783,6 +783,7 @@ func autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha3_MachineDeploymentSpec } else { out.Strategy = nil } + // WARNING: in.MachineNamingStrategy requires manual conversion: does not exist in peer-type out.MinReadySeconds = (*int32)(unsafe.Pointer(in.MinReadySeconds)) out.RevisionHistoryLimit = (*int32)(unsafe.Pointer(in.RevisionHistoryLimit)) out.Paused = in.Paused diff --git a/internal/apis/core/v1alpha4/zz_generated.conversion.go b/internal/apis/core/v1alpha4/zz_generated.conversion.go index 14df0dfc9112..a8db7a668788 100644 --- a/internal/apis/core/v1alpha4/zz_generated.conversion.go +++ b/internal/apis/core/v1alpha4/zz_generated.conversion.go @@ -1168,6 +1168,7 @@ func autoConvert_v1beta1_MachineDeploymentSpec_To_v1alpha4_MachineDeploymentSpec } else { out.Strategy = nil } + // WARNING: in.MachineNamingStrategy requires manual conversion: does not exist in peer-type out.MinReadySeconds = (*int32)(unsafe.Pointer(in.MinReadySeconds)) out.RevisionHistoryLimit = (*int32)(unsafe.Pointer(in.RevisionHistoryLimit)) out.Paused = in.Paused diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 66e6fcd25ca3..0878f74929dc 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -50,6 +50,7 @@ import ( "sigs.k8s.io/cluster-api/controllers/noderefutil" "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/controllers/machine" + topologynames "sigs.k8s.io/cluster-api/internal/topology/names" "sigs.k8s.io/cluster-api/internal/util/ssa" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/collections" @@ -518,8 +519,11 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e } // Update Machine to propagate in-place mutable fields from the MachineSet. - updatedMachine := r.computeDesiredMachine(machineSet, m) - err := ssa.Patch(ctx, r.Client, machineSetManagerName, updatedMachine, ssa.WithCachingProxy{Cache: r.ssaCache, Original: m}) + updatedMachine, err := r.computeDesiredMachine(ctx, machineSet, m) + if err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to update Machine: failed to compute desired Machine") + } + err = ssa.Patch(ctx, r.Client, machineSetManagerName, updatedMachine, ssa.WithCachingProxy{Cache: r.ssaCache, Original: m}) if err != nil { log.Error(err, "Failed to update Machine", "Machine", klog.KObj(updatedMachine)) return ctrl.Result{}, errors.Wrapf(err, "failed to update Machine %q", klog.KObj(updatedMachine)) @@ -612,7 +616,10 @@ func (r *Reconciler) syncReplicas(ctx context.Context, s *scope) (ctrl.Result, e for i := range diff { // Create a new logger so the global logger is not modified. log := log - machine := r.computeDesiredMachine(ms, nil) + machine, computeMachineErr := r.computeDesiredMachine(ctx, ms, nil) + if computeMachineErr != nil { + return ctrl.Result{}, errors.Wrap(computeMachineErr, "failed to create Machine: failed to compute desired Machine") + } // Clone and set the infrastructure and bootstrap references. var ( infraRef, bootstrapRef *corev1.ObjectReference @@ -747,14 +754,36 @@ func (r *Reconciler) syncReplicas(ctx context.Context, s *scope) (ctrl.Result, e // There are small differences in how we calculate the Machine depending on if it // is a create or update. Example: for a new Machine we have to calculate a new name, // while for an existing Machine we have to use the name of the existing Machine. -func (r *Reconciler) computeDesiredMachine(machineSet *clusterv1.MachineSet, existingMachine *clusterv1.Machine) *clusterv1.Machine { +func (r *Reconciler) computeDesiredMachine(ctx context.Context, machineSet *clusterv1.MachineSet, existingMachine *clusterv1.Machine) (*clusterv1.Machine, error) { + machineName := names.SimpleNameGenerator.GenerateName(fmt.Sprintf("%s-", machineSet.Name)) + // If the MachineSet is part of a MachineDeployment, check MachineNamingStrategy + // and name Machine accordingly. + if isDeploymentChild(machineSet) { + owner, err := r.getOwnerMachineDeployment(ctx, machineSet) + if err != nil { + return nil, err + } + nameTemplate := "{{ .machineDeployment.name }}-{{ .random }}" + if owner.Spec.MachineNamingStrategy != nil && owner.Spec.MachineNamingStrategy.Template != "" { + nameTemplate = owner.Spec.MachineNamingStrategy.Template + if !strings.Contains(nameTemplate, "{{ .random }}") { + return nil, errors.New("cannot generate MachineDeployment machine name: {{ .random }} is missing in machineNamingStrategy.template") + } + } + generatedMachineName, err := topologynames.MachineDeploymentMachineNameGenerator(nameTemplate, machineSet.Spec.ClusterName, owner.Name).GenerateName() + if err != nil { + return nil, errors.Wrap(err, "failed to generate name for MachineDeployment machine") + } + machineName = generatedMachineName + } + desiredMachine := &clusterv1.Machine{ TypeMeta: metav1.TypeMeta{ APIVersion: clusterv1.GroupVersion.String(), Kind: "Machine", }, ObjectMeta: metav1.ObjectMeta{ - Name: names.SimpleNameGenerator.GenerateName(fmt.Sprintf("%s-", machineSet.Name)), + Name: machineName, Namespace: machineSet.Namespace, // Note: By setting the ownerRef on creation we signal to the Machine controller that this is not a stand-alone Machine. OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(machineSet, machineSetKind)}, @@ -812,7 +841,7 @@ func (r *Reconciler) computeDesiredMachine(machineSet *clusterv1.MachineSet, exi desiredMachine.Spec.NodeDeletionTimeout = machineSet.Spec.Template.Spec.NodeDeletionTimeout desiredMachine.Spec.NodeVolumeDetachTimeout = machineSet.Spec.Template.Spec.NodeVolumeDetachTimeout - return desiredMachine + return desiredMachine, nil } // updateExternalObject updates the external object passed in with the diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 246844e32529..07cdd4d8d9e9 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -22,6 +22,7 @@ import ( "time" . "github.com/onsi/gomega" + gomegatypes "github.com/onsi/gomega/types" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1096,8 +1097,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) { Name: "ms-1", Namespace: namespace.Name, Labels: map[string]string{ - "label-1": "true", - clusterv1.MachineDeploymentNameLabel: "md-1", + "label-1": "true", }, }, Spec: clusterv1.MachineSetSpec{ @@ -1347,11 +1347,10 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) { // Drop "dropped-label" } expectedLabels := map[string]string{ - "preserved-label": "preserved-value", - "modified-label": "modified-value-2", - clusterv1.MachineSetNameLabel: ms.Name, - clusterv1.MachineDeploymentNameLabel: "md-1", - clusterv1.ClusterNameLabel: testClusterName, // This label is added by the Machine controller. + "preserved-label": "preserved-value", + "modified-label": "modified-value-2", + clusterv1.MachineSetNameLabel: ms.Name, + clusterv1.ClusterNameLabel: testClusterName, // This label is added by the Machine controller. } ms.Spec.Template.Annotations = map[string]string{ "preserved-annotation": "preserved-value", // Keep the annotation and value as is @@ -2063,10 +2062,27 @@ func TestMachineSetReconciler_syncReplicas(t *testing.T) { }) } +type ComputeDesiredMachineTestCase struct { + name string + md *clusterv1.MachineDeployment + existingMachine *clusterv1.Machine + wantMachine *clusterv1.Machine + wantName []gomegatypes.GomegaMatcher +} + func TestComputeDesiredMachine(t *testing.T) { duration5s := &metav1.Duration{Duration: 5 * time.Second} duration10s := &metav1.Duration{Duration: 10 * time.Second} + namingTemplateKey := "-md" + mdName := "testmd" + testCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: metav1.NamespaceDefault, + }, + } + infraRef := corev1.ObjectReference{ Kind: "GenericInfrastructureMachineTemplate", Name: "infra-template-1", @@ -2078,53 +2094,36 @@ func TestComputeDesiredMachine(t *testing.T) { APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", } - ms := &clusterv1.MachineSet{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", - Name: "ms1", - Labels: map[string]string{ - clusterv1.MachineDeploymentNameLabel: "md1", - }, + machineTemplateSpec := clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ + Labels: map[string]string{"machine-label1": "machine-value1"}, + Annotations: map[string]string{"machine-annotation1": "machine-value1"}, }, - Spec: clusterv1.MachineSetSpec{ - ClusterName: "test-cluster", - Replicas: ptr.To[int32](3), - MinReadySeconds: 10, - Selector: metav1.LabelSelector{ - MatchLabels: map[string]string{"k1": "v1"}, - }, - Template: clusterv1.MachineTemplateSpec{ - ObjectMeta: clusterv1.ObjectMeta{ - Labels: map[string]string{"machine-label1": "machine-value1"}, - Annotations: map[string]string{"machine-annotation1": "machine-value1"}, - }, - Spec: clusterv1.MachineSpec{ - Version: ptr.To("v1.25.3"), - InfrastructureRef: infraRef, - Bootstrap: clusterv1.Bootstrap{ - ConfigRef: &bootstrapRef, - }, - NodeDrainTimeout: duration10s, - NodeVolumeDetachTimeout: duration10s, - NodeDeletionTimeout: duration10s, - }, + Spec: clusterv1.MachineSpec{ + ClusterName: testClusterName, + Version: ptr.To("v1.25.3"), + InfrastructureRef: infraRef, + Bootstrap: clusterv1.Bootstrap{ + ConfigRef: &bootstrapRef, }, + NodeDrainTimeout: duration10s, + NodeVolumeDetachTimeout: duration10s, + NodeDeletionTimeout: duration10s, }, } skeletonMachine := &clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "default", + Namespace: metav1.NamespaceDefault, Labels: map[string]string{ - "machine-label1": "machine-value1", - clusterv1.MachineSetNameLabel: "ms1", - clusterv1.MachineDeploymentNameLabel: "md1", + "machine-label1": "machine-value1", + clusterv1.MachineSetNameLabel: "ms1", }, Annotations: map[string]string{"machine-annotation1": "machine-value1"}, Finalizers: []string{clusterv1.MachineFinalizer}, }, Spec: clusterv1.MachineSpec{ - ClusterName: "test-cluster", + ClusterName: testClusterName, Version: ptr.To("v1.25.3"), NodeDrainTimeout: duration10s, NodeVolumeDetachTimeout: duration10s, @@ -2165,56 +2164,212 @@ func TestComputeDesiredMachine(t *testing.T) { expectedUpdatedMachine.Spec.InfrastructureRef = *existingMachine.Spec.InfrastructureRef.DeepCopy() expectedUpdatedMachine.Spec.Bootstrap.ConfigRef = existingMachine.Spec.Bootstrap.ConfigRef.DeepCopy() - tests := []struct { - name string - existingMachine *clusterv1.Machine - want *clusterv1.Machine - }{ + tests := []ComputeDesiredMachineTestCase{ { - name: "creating a new Machine", + name: "should return the correct Machine object when creating a new Machine", + md: &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: mdName, + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineDeploymentSpec{ + ClusterName: testClusterName, + MachineNamingStrategy: &clusterv1.MachineNamingStrategy{ + Template: "{{ .machineDeployment.name }}" + namingTemplateKey + "-{{ .random }}", + }, + Template: machineTemplateSpec, + }, + }, existingMachine: nil, - want: expectedNewMachine, + wantMachine: expectedNewMachine, + wantName: []gomegatypes.GomegaMatcher{ + HavePrefix(mdName + namingTemplateKey), + Not(HaveSuffix("00000")), + }, + }, + { + name: "should return error when creating a new Machine when '.random' is not added in template", + md: &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: mdName, + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineDeploymentSpec{ + ClusterName: testClusterName, + MachineNamingStrategy: &clusterv1.MachineNamingStrategy{ + Template: "{{ .machineDeployment.name }}" + namingTemplateKey, + }, + Template: machineTemplateSpec, + }, + }, + existingMachine: nil, + wantMachine: nil, + }, + { + name: "should not return error when creating a new Machine when the generated name exceeds 63", + md: &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: mdName, + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineDeploymentSpec{ + ClusterName: testClusterName, + MachineNamingStrategy: &clusterv1.MachineNamingStrategy{ + Template: "{{ .random }}" + fmt.Sprintf("%059d", 0), + }, + Template: machineTemplateSpec, + }, + }, + existingMachine: nil, + wantMachine: expectedNewMachine, + wantName: []gomegatypes.GomegaMatcher{ + ContainSubstring(fmt.Sprintf("%053d", 0)), + Not(HaveSuffix("00000")), + }, + }, + { + name: "should return error when creating a new Machine", + md: &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: mdName, + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineDeploymentSpec{ + ClusterName: testClusterName, + MachineNamingStrategy: &clusterv1.MachineNamingStrategy{ + Template: "some-hardcoded-name-{{ .doesnotexistindata }}-{{ .random }}", // invalid template + }, + Template: machineTemplateSpec, + }, + }, + existingMachine: nil, + wantMachine: nil, + }, + { + name: "should return the correct Machine object when creating a new Machine with default templated name", + md: &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: mdName, + Namespace: metav1.NamespaceDefault, + }, + Spec: clusterv1.MachineDeploymentSpec{ + ClusterName: testClusterName, + Template: machineTemplateSpec, + }, + }, + existingMachine: nil, + wantMachine: expectedNewMachine, + wantName: []gomegatypes.GomegaMatcher{ + HavePrefix(mdName), + Not(HaveSuffix("00000")), + }, }, { name: "updating an existing Machine", existingMachine: existingMachine, - want: expectedUpdatedMachine, + wantMachine: expectedUpdatedMachine, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got := (&Reconciler{}).computeDesiredMachine(ms, tt.existingMachine) - assertMachine(g, got, tt.want) + var got *clusterv1.Machine + var err error + if tt.existingMachine == nil { + ms := &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: metav1.NamespaceDefault, + Name: "ms1", + Labels: map[string]string{ + clusterv1.MachineDeploymentNameLabel: mdName, + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: tt.md.Name, + UID: tt.md.UID, + }, + }, + }, + Spec: clusterv1.MachineSetSpec{ + ClusterName: testClusterName, + Replicas: ptr.To[int32](3), + MinReadySeconds: 10, + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"k1": "v1"}, + }, + Template: machineTemplateSpec, + }, + } + c := fake.NewClientBuilder().WithObjects( + testCluster, + ms, + tt.md, + ).WithStatusSubresource(&clusterv1.MachineSet{}).Build() + msr := &Reconciler{ + Client: c, + recorder: record.NewFakeRecorder(32), + } + got, err = msr.computeDesiredMachine(ctx, ms, tt.existingMachine) + } else { + ms := &clusterv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: metav1.NamespaceDefault, + Name: "ms1", + }, + Spec: clusterv1.MachineSetSpec{ + ClusterName: testClusterName, + Replicas: ptr.To[int32](3), + MinReadySeconds: 10, + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"k1": "v1"}, + }, + Template: machineTemplateSpec, + }, + } + got, err = (&Reconciler{}).computeDesiredMachine(ctx, ms, tt.existingMachine) + } + + if tt.wantMachine == nil { + g.Expect(err).To(HaveOccurred()) + return + } + assertMachine(g, got, tt) }) } } -func assertMachine(g *WithT, actualMachine *clusterv1.Machine, expectedMachine *clusterv1.Machine) { +func assertMachine(g *WithT, actualMachine *clusterv1.Machine, testData ComputeDesiredMachineTestCase) { // Check Name - if expectedMachine.Name != "" { - g.Expect(actualMachine.Name).Should(Equal(expectedMachine.Name)) + if testData.existingMachine == nil { + for _, matcher := range testData.wantName { + g.Expect(actualMachine.Name).To(matcher) + } + } + if testData.wantMachine.Name != "" { + g.Expect(actualMachine.Name).Should(Equal(testData.wantMachine.Name)) } // Check UID - if expectedMachine.UID != "" { - g.Expect(actualMachine.UID).Should(Equal(expectedMachine.UID)) + if testData.wantMachine.UID != "" { + g.Expect(actualMachine.UID).Should(Equal(testData.wantMachine.UID)) } // Check Namespace - g.Expect(actualMachine.Namespace).Should(Equal(expectedMachine.Namespace)) + g.Expect(actualMachine.Namespace).Should(Equal(testData.wantMachine.Namespace)) // Check Labels - for k, v := range expectedMachine.Labels { + for k, v := range testData.wantMachine.Labels { g.Expect(actualMachine.Labels).Should(HaveKeyWithValue(k, v)) } // Check Annotations - for k, v := range expectedMachine.Annotations { + for k, v := range testData.wantMachine.Annotations { g.Expect(actualMachine.Annotations).Should(HaveKeyWithValue(k, v)) } // Check Spec - g.Expect(actualMachine.Spec).Should(BeComparableTo(expectedMachine.Spec)) + g.Expect(actualMachine.Spec).Should(BeComparableTo(testData.wantMachine.Spec)) // Check Finalizer - if expectedMachine.Finalizers != nil { - g.Expect(actualMachine.Finalizers).Should(Equal(expectedMachine.Finalizers)) + if testData.wantMachine.Finalizers != nil { + g.Expect(actualMachine.Finalizers).Should(Equal(testData.wantMachine.Finalizers)) } } diff --git a/internal/topology/names/names.go b/internal/topology/names/names.go index 0796d94debe4..52ebd573199c 100644 --- a/internal/topology/names/names.go +++ b/internal/topology/names/names.go @@ -96,6 +96,16 @@ func KCPMachineNameGenerator(templateString, clusterName, kubeadmControlPlaneNam }) } +// MachineDeploymentMachineNameGenerator returns a generator for creating a kcp machine name. +func MachineDeploymentMachineNameGenerator(templateString, clusterName, machineDeploymentName string) NameGenerator { + return newTemplateGenerator(templateString, clusterName, + map[string]interface{}{ + "machineDeployment": map[string]interface{}{ + "name": machineDeploymentName, + }, + }) +} + // 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/machinedeployment.go b/internal/webhooks/machinedeployment.go index 80dae949086b..5ea966a18b11 100644 --- a/internal/webhooks/machinedeployment.go +++ b/internal/webhooks/machinedeployment.go @@ -38,6 +38,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/feature" + names "sigs.k8s.io/cluster-api/internal/topology/names" "sigs.k8s.io/cluster-api/util/version" ) @@ -291,6 +292,10 @@ func (webhook *MachineDeployment) validate(oldMD, newMD *clusterv1.MachineDeploy } } + if newMD.Spec.MachineNamingStrategy != nil { + allErrs = append(allErrs, validateNamingStrategy(newMD.Spec.MachineNamingStrategy, specPath.Child("machineNamingStrategy"))...) + } + // Validate the metadata of the template. allErrs = append(allErrs, newMD.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) @@ -301,6 +306,42 @@ func (webhook *MachineDeployment) validate(oldMD, newMD *clusterv1.MachineDeploy return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("MachineDeployment").GroupKind(), newMD.Name, allErrs) } +func validateNamingStrategy(machineNamingStrategy *clusterv1.MachineNamingStrategy, pathPrefix *field.Path) field.ErrorList { + var allErrs field.ErrorList + + if machineNamingStrategy.Template != "" { + if !strings.Contains(machineNamingStrategy.Template, "{{ .random }}") { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("template"), + machineNamingStrategy.Template, + "invalid template, {{ .random }} is missing", + )) + return allErrs + } + name, err := names.MachineDeploymentMachineNameGenerator(machineNamingStrategy.Template, "cluster", "machinedeployment").GenerateName() + if err != nil { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("template"), + machineNamingStrategy.Template, + fmt.Sprintf("invalid template: %v", err), + )) + } else { + for _, err := range validation.IsDNS1123Subdomain(name) { + allErrs = append(allErrs, + field.Invalid( + pathPrefix.Child("template"), + machineNamingStrategy.Template, + fmt.Sprintf("invalid template, generated names would not be valid Kubernetes object names: %v", err), + )) + } + } + } + + return allErrs +} + // calculateMachineDeploymentReplicas calculates the default value of the replicas field. // The value will be calculated based on the following logic: // * if replicas is already set on newMD, keep the current value