diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index d4585207a7b8..4ccaabfa8e01 100644 --- 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 index 5f6a4dffd4c6..709e8858f520 100644 --- 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 index 9aefcf800678..7980241a4a78 100644 --- 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 index 982346c519e5..f755165a951e 100644 --- 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,7 +1800,8 @@ 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. @@ -1807,26 +1809,75 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) { 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()}, }, {