Make MHC ClusterClass authoritative on paths

Signed-off-by: killianmuldoon <[email protected]>
kubernetes-sigs · Jan 18, 2022 · 43c82f9 · 43c82f9
1 parent 32c2c16
commit 43c82f9
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 8 deletions.
diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go
@@ -291,7 +291,9 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d
 	ctx, log = log.WithObject(current).Into(ctx)
 
 	// Check differences between current and desired MachineHealthChecks, and patch if required.
-	patchHelper, err := mergepatch.NewHelper(current, desired, r.Client)
+	// NOTE: we want to be authoritative on the entire spec because the users are
+	// expected to change MHC fields from the ClusterClass only.
+	patchHelper, err := mergepatch.NewHelper(current, desired, r.Client, mergepatch.AuthoritativePaths{contract.Path{"spec"}})
 	if err != nil {
 		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: current})
 	}

diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go
@@ -1774,6 +1774,7 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) {
 		Build()
 
 	maxUnhealthy := intstr.Parse("45%")
+	// TODO: (killianmuldoon) This builder should be copied and not just passed around.
 	mhcBuilder := builder.MachineHealthCheck(metav1.NamespaceDefault, "md-1").
 		WithSelector(*selectorForMachineDeploymentMHC(md)).
 		WithUnhealthyConditions([]clusterv1.UnhealthyCondition{
@@ -1799,34 +1800,84 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) {
 			name:    "Create a MachineHealthCheck if the MachineDeployment is being created",
 			current: nil,
 			desired: []*scope.MachineDeploymentState{
-				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate, mhcBuilder.Build()),
+				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate,
+					mhcBuilder.Build()),
 			},
 			want: []*clusterv1.MachineHealthCheck{
 				mhcBuilder.
 					WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}).
 					Build()},
 		},
 		{
-			name:    "Create a new MachineHealthCheck if the MachineDeployment is modified to include one",
-			current: []*scope.MachineDeploymentState{newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate, nil)},
+			name: "Create a new MachineHealthCheck if the MachineDeployment is modified to include one",
+			current: []*scope.MachineDeploymentState{
+				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate,
+					nil)},
 			// MHC is added in the desired state of the MachineDeployment
 			desired: []*scope.MachineDeploymentState{
-				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate, mhcBuilder.Build()),
+				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate,
+					mhcBuilder.Build()),
 			},
 			want: []*clusterv1.MachineHealthCheck{
 				mhcBuilder.
 					WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}).
 					Build()}},
 		{
-			name: "Update MachineHealthCheck spec if the spec changes",
+			name: "Update MachineHealthCheck spec adding a field if the spec adds a field",
 			current: []*scope.MachineDeploymentState{
 				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate, mhcBuilder.Build()),
 			},
-			desired: []*scope.MachineDeploymentState{newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate, mhcBuilder.WithMaxUnhealthy(&maxUnhealthy).Build())},
+			desired: []*scope.MachineDeploymentState{
+				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate,
+					mhcBuilder.WithMaxUnhealthy(&maxUnhealthy).Build())},
 			want: []*clusterv1.MachineHealthCheck{
-				mhcBuilder.
+				builder.MachineHealthCheck(metav1.NamespaceDefault, "md-1").
+					WithSelector(*selectorForMachineDeploymentMHC(md)).
+					WithUnhealthyConditions([]clusterv1.UnhealthyCondition{
+						{
+							Type:    corev1.NodeReady,
+							Status:  corev1.ConditionUnknown,
+							Timeout: metav1.Duration{Duration: 5 * time.Minute},
+						},
+					}).
 					WithMaxUnhealthy(&maxUnhealthy).
+					WithClusterName("cluster1").
+					WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}).
+					Build()},
+		},
+		{
+			name: "Update MachineHealthCheck spec removing a field if the spec removes a field",
+			current: []*scope.MachineDeploymentState{
+				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate,
+					mhcBuilder.WithMaxUnhealthy(&maxUnhealthy).Build()),
+			},
+			desired: []*scope.MachineDeploymentState{
+				newFakeMachineDeploymentTopologyState("md-1", infrastructureMachineTemplate, bootstrapTemplate,
+					builder.MachineHealthCheck(metav1.NamespaceDefault, "md-1").
+						WithSelector(*selectorForMachineDeploymentMHC(md)).
+						WithUnhealthyConditions([]clusterv1.UnhealthyCondition{
+							{
+								Type:    corev1.NodeReady,
+								Status:  corev1.ConditionUnknown,
+								Timeout: metav1.Duration{Duration: 5 * time.Minute},
+							},
+						}).
+						WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}).
+						WithClusterName("cluster1").
+						Build()),
+			},
+			want: []*clusterv1.MachineHealthCheck{
+				builder.MachineHealthCheck(metav1.NamespaceDefault, "md-1").
+					WithSelector(*selectorForMachineDeploymentMHC(md)).
+					WithUnhealthyConditions([]clusterv1.UnhealthyCondition{
+						{
+							Type:    corev1.NodeReady,
+							Status:  corev1.ConditionUnknown,
+							Timeout: metav1.Duration{Duration: 5 * time.Minute},
+						},
+					}).
 					WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}).
+					WithClusterName("cluster1").
 					Build()},
 		},
 		{