Merge pull request #5949 from killianmuldoon/fix/mhc-authoritative-paths

🐛 Make MHC ClusterClass authoritative on paths
kubernetes-sigs · Jan 19, 2022 · cf3852f · cf3852f
2 parents 07e31b7 + e8567a4
commit cf3852f
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 10 deletions.
diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go
@@ -738,7 +738,7 @@ func ownerReferenceTo(obj client.Object) *metav1.OwnerReference {
 
 func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass) *clusterv1.MachineHealthCheck {
 	// Create a MachineHealthCheck with the spec given in the ClusterClass.
-	return &clusterv1.MachineHealthCheck{
+	mhc := &clusterv1.MachineHealthCheck{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind,
 			APIVersion: clusterv1.GroupVersion.String(),
@@ -757,6 +757,10 @@ func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1
 			RemediationTemplate: check.RemediationTemplate,
 		},
 	}
+	// Default all fields in the MachineHealthCheck using the same function called in the webhook. This ensures the desired
+	// state of the object won't be different from the current state due to webhook Defaulting.
+	mhc.Default()
+	return mhc
 }
 
 func selectorForControlPlaneMHC() *metav1.LabelSelector {

diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go
@@ -26,6 +26,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
@@ -1430,6 +1431,7 @@ func TestMergeMap(t *testing.T) {
 }
 
 func Test_computeMachineHealthCheck(t *testing.T) {
+	maxUnhealthyValue := intstr.FromString("100%")
 	mhcSpec := &clusterv1.MachineHealthCheckClass{
 		UnhealthyConditions: []clusterv1.UnhealthyCondition{
 			{
@@ -1446,7 +1448,6 @@ func Test_computeMachineHealthCheck(t *testing.T) {
 		NodeStartupTimeout: &metav1.Duration{
 			Duration: time.Duration(1)},
 	}
-
 	selector := &metav1.LabelSelector{MatchLabels: map[string]string{
 		"foo": "bar",
 	}}
@@ -1460,12 +1461,16 @@ func Test_computeMachineHealthCheck(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "md1",
 			Namespace: "ns1",
+			// Label is added by defaulting values using MachineHealthCheck.Default()
+			Labels: map[string]string{"cluster.x-k8s.io/cluster-name": "cluster1"},
 		},
 		Spec: clusterv1.MachineHealthCheckSpec{
 			ClusterName: "cluster1",
 			Selector: metav1.LabelSelector{MatchLabels: map[string]string{
 				"foo": "bar",
 			}},
+			// MaxUnhealthy is added by defaulting values using MachineHealthCheck.Default()
+			MaxUnhealthy: &maxUnhealthyValue,
 			UnhealthyConditions: []clusterv1.UnhealthyCondition{
 				{
 					Type:    corev1.NodeReady,

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()},
 		},
 		{