Merge pull request #5731 from sbueringer/pr-authoritative-patch-md-se…

…lector 🐛 ClusterClass: patch MachineDeployment selector authoritatively
kubernetes-sigs · Nov 29, 2021 · bcc2509 · bcc2509
2 parents c50e5ba + fca434e
commit bcc2509
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 7 deletions.
diff --git a/api/v1beta1/machinedeployment_webhook.go b/api/v1beta1/machinedeployment_webhook.go
@@ -85,8 +85,8 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error {
 		allErrs = append(
 			allErrs,
 			field.Invalid(
-				field.NewPath("spec", "template", "labels"),
-				m.Spec.Template.Labels,
+				field.NewPath("spec", "template", "metadata", "labels"),
+				m.Spec.Template.ObjectMeta.Labels,
 				fmt.Sprintf("must match spec.selector %q", selector.String()),
 			),
 		)

diff --git a/api/v1beta1/machineset_webhook.go b/api/v1beta1/machineset_webhook.go
@@ -104,8 +104,8 @@ func (m *MachineSet) validate(old *MachineSet) error {
 		allErrs = append(
 			allErrs,
 			field.Invalid(
-				field.NewPath("spec", "template", "labels"),
-				m.Spec.Template.Labels,
+				field.NewPath("spec", "template", "metadata", "labels"),
+				m.Spec.Template.ObjectMeta.Labels,
 				fmt.Sprintf("must match spec.selector %q", selector.String()),
 			),
 		)

diff --git a/controllers/topology/reconcile_state.go b/controllers/topology/reconcile_state.go
@@ -327,6 +327,10 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster
 		// Note: nested metadata have only labels and annotations, so it is possible to override the entire
 		// parent struct.
 		{"spec", "template", "metadata"},
+		// Note: we want to be authoritative for the selector too, because if the selector and metadata.labels
+		// change, the metadata.labels might not match the selector anymore, if we don't delete outdated labels
+		// from the selector.
+		{"spec", "selector"},
 	})
 	if err != nil {
 		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: currentMD.Object})

diff --git a/controllers/topology/reconcile_state_test.go b/controllers/topology/reconcile_state_test.go
@@ -796,8 +796,11 @@ func TestReconcileMachineDeployments(t *testing.T) {
 	infrastructureMachineTemplate9m := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-9m").Build()
 	bootstrapTemplate9m := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap-config-9m").Build()
 	md9 := newFakeMachineDeploymentTopologyState("md-9m", infrastructureMachineTemplate9m, bootstrapTemplate9m)
-	md9WithInstanceSpecificTemplateMetadata := newFakeMachineDeploymentTopologyState("md-9m", infrastructureMachineTemplate9m, bootstrapTemplate9m)
-	md9WithInstanceSpecificTemplateMetadata.Object.Spec.Template.ObjectMeta.Labels = map[string]string{"foo": "bar"}
+	md9.Object.Spec.Template.ObjectMeta.Labels = map[string]string{clusterv1.ClusterLabelName: "cluster-name"}
+	md9.Object.Spec.Selector.MatchLabels = map[string]string{clusterv1.ClusterLabelName: "cluster-name"}
+	md9WithInstanceSpecificTemplateMetadataAndSelector := newFakeMachineDeploymentTopologyState("md-9m", infrastructureMachineTemplate9m, bootstrapTemplate9m)
+	md9WithInstanceSpecificTemplateMetadataAndSelector.Object.Spec.Template.ObjectMeta.Labels = map[string]string{"foo": "bar"}
+	md9WithInstanceSpecificTemplateMetadataAndSelector.Object.Spec.Selector.MatchLabels = map[string]string{"foo": "bar"}
 
 	tests := []struct {
 		name                                      string
@@ -892,7 +895,7 @@ func TestReconcileMachineDeployments(t *testing.T) {
 		},
 		{
 			name:    "Enforce template metadata",
-			current: []*scope.MachineDeploymentState{md9WithInstanceSpecificTemplateMetadata},
+			current: []*scope.MachineDeploymentState{md9WithInstanceSpecificTemplateMetadataAndSelector},
 			desired: []*scope.MachineDeploymentState{md9},
 			want:    []*scope.MachineDeploymentState{md9},
 			wantErr: false,