Prevent breaking an existing owner reference for MachineHealthChecks …

…of ControlPlanes in topology

internal/controllers/topology/cluster/desired_state.go

@@ -74,8 +74,7 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (*
 			desiredState.ControlPlane.Object,
 			selectorForControlPlaneMHC(),
 			s.Current.Cluster.Name,
-			s.Blueprint.ControlPlane.MachineHealthCheck,
-			s.Current.ControlPlane.MachineHealthCheck)
+			s.Blueprint.ControlPlane.MachineHealthCheck)
 	}
 
 	// Compute the desired state for the Cluster object adding a reference to the
@@ -548,17 +547,12 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
 
 	// If the ClusterClass defines a MachineHealthCheck for the MachineDeployment add it to the desired state.
 	if machineDeploymentBlueprint.MachineHealthCheck != nil {
-		var currentMachineHealthCheck *clusterv1.MachineHealthCheck
-		if currentMachineDeployment != nil {
-			currentMachineHealthCheck = currentMachineDeployment.MachineHealthCheck
-		}
 		// Note: The MHC is going to use a selector that provides a minimal set of labels which are common to all MachineSets belonging to the MachineDeployment.
 		desiredMachineDeployment.MachineHealthCheck = computeMachineHealthCheck(
 			desiredMachineDeploymentObj,
 			selectorForMachineDeploymentMHC(desiredMachineDeploymentObj),
 			s.Current.Cluster.Name,
-			machineDeploymentBlueprint.MachineHealthCheck,
-			currentMachineHealthCheck)
+			machineDeploymentBlueprint.MachineHealthCheck)
 	}
 	return desiredMachineDeployment, nil
 }
@@ -797,7 +791,7 @@ func ownerReferenceTo(obj client.Object) *metav1.OwnerReference {
 	}
 }
 
-func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1.LabelSelector, clusterName string, check *clusterv1.MachineHealthCheckClass, current *clusterv1.MachineHealthCheck) *clusterv1.MachineHealthCheck {
+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.
 	mhc := &clusterv1.MachineHealthCheck{
 		TypeMeta: metav1.TypeMeta{
@@ -823,14 +817,6 @@ func computeMachineHealthCheck(healthCheckTarget client.Object, selector *metav1
 	// state of the object won't be different from the current state due to webhook Defaulting.
 	mhc.Default()
 
-	// Carry over ownerReference to the target object.
-	// NOTE: this prevents to the ownerRef to be deleted by server side apply.
-	if current != nil {
-		if ref := getOwnerReferenceFrom(current, healthCheckTarget); ref != nil {
-			mhc.SetOwnerReferences([]metav1.OwnerReference{*ref})
-		}
-	}
-
 	return mhc
 }
 

internal/controllers/topology/cluster/desired_state_test.go

@@ -1634,20 +1634,6 @@ func Test_computeMachineHealthCheck(t *testing.T) {
 	}}
 	healthCheckTarget := builder.MachineDeployment("ns1", "md1").Build()
 	clusterName := "cluster1"
-	current := &clusterv1.MachineHealthCheck{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind,
-			APIVersion: clusterv1.GroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "md1",
-			Namespace: "ns1",
-			// The only thing we care about in current is the owner reference to the target object
-			OwnerReferences: []metav1.OwnerReference{
-				*ownerReferenceTo(healthCheckTarget),
-			},
-		},
-	}
 	want := &clusterv1.MachineHealthCheck{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind,
@@ -1658,9 +1644,6 @@ func Test_computeMachineHealthCheck(t *testing.T) {
 			Namespace: "ns1",
 			// Label is added by defaulting values using MachineHealthCheck.Default()
 			Labels: map[string]string{"cluster.x-k8s.io/cluster-name": "cluster1"},
-			OwnerReferences: []metav1.OwnerReference{
-				*ownerReferenceTo(healthCheckTarget),
-			},
 		},
 		Spec: clusterv1.MachineHealthCheckSpec{
 			ClusterName: "cluster1",
@@ -1689,7 +1672,7 @@ func Test_computeMachineHealthCheck(t *testing.T) {
 	t.Run("set all fields correctly", func(t *testing.T) {
 		g := NewWithT(t)
 
-		got := computeMachineHealthCheck(healthCheckTarget, selector, clusterName, mhcSpec, current)
+		got := computeMachineHealthCheck(healthCheckTarget, selector, clusterName, mhcSpec)
 
 		g.Expect(got).To(Equal(want), cmp.Diff(got, want))
 	})

internal/controllers/topology/cluster/reconcile_state.go

@@ -31,7 +31,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/cluster-api/internal/contract"
 	"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope"
 	"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge"
@@ -249,14 +248,6 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)
 
 	// If the ControlPlane has defined a current or desired MachineHealthCheck attempt to reconcile it.
 	if s.Desired.ControlPlane.MachineHealthCheck != nil || s.Current.ControlPlane.MachineHealthCheck != nil {
-		// Set the ControlPlane Object and the Cluster as owners for the MachineHealthCheck to ensure object garbage collection
-		// in case something happens before the MHC sets ownership to the Cluster.
-		if s.Desired.ControlPlane.MachineHealthCheck != nil {
-			s.Desired.ControlPlane.MachineHealthCheck.SetOwnerReferences([]metav1.OwnerReference{
-				*ownerReferenceTo(s.Desired.ControlPlane.Object),
-			})
-		}
-
 		// Reconcile the current and desired state of the MachineHealthCheck.
 		if err := r.reconcileMachineHealthCheck(ctx, s.Current.ControlPlane.MachineHealthCheck, s.Desired.ControlPlane.MachineHealthCheck); err != nil {
 			return err
@@ -272,18 +263,6 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d
 
 	// If a current MachineHealthCheck doesn't exist but there is a desired MachineHealthCheck attempt to create.
 	if current == nil && desired != nil {
-		// First ensure the ownerReferences are valid.
-		refs := []metav1.OwnerReference{}
-		for _, ref := range desired.OwnerReferences {
-			currentRef := ref
-			ownerRef, err := r.resolveOwnerReferenceIfIncomplete(ctx, desired.Namespace, &currentRef)
-			if err != nil {
-				return errors.Wrapf(err, "failed to resolve owner reference %v for %s", ref, tlog.KObj{Obj: desired})
-			}
-			refs = append(refs, *ownerRef)
-		}
-		desired.OwnerReferences = refs
-
 		log.Infof("Creating %s", tlog.KObj{Obj: desired})
 		helper, err := r.patchHelperFactory(nil, desired)
 		if err != nil {
@@ -331,34 +310,6 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d
 	return nil
 }
 
-// resolveOwnerReferenceIfIncomplete checks if an OwnerReferences is valid. If not it attempts to get the referenced object
-// to create a valid ownerReference.
-func (r *Reconciler) resolveOwnerReferenceIfIncomplete(ctx context.Context, namespace string, reference *metav1.OwnerReference) (*metav1.OwnerReference, error) {
-	var owner *unstructured.Unstructured
-	var err error
-
-	// First check that the ownerReference has at least Name Kind and APIVersion defined.
-	if reference.Name == "" || reference.APIVersion == "" || reference.Kind == "" {
-		return nil, errors.Errorf("ownerReference not valid: %v", reference)
-	}
-
-	// If the UID field is not empty this is a fully valid reference and can be returned and used.
-	if reference.UID != "" {
-		return reference, nil
-	}
-
-	// Get the object set an ownerReference for the MachineHealthCheck.
-	owner, err = external.Get(ctx, r.Client, &corev1.ObjectReference{
-		Kind:       reference.Kind,
-		Name:       reference.Name,
-		APIVersion: reference.APIVersion},
-		namespace)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to retrieve %s %q in namespace %q", reference.Kind, reference.Name, namespace)
-	}
-	return ownerReferenceTo(owner), nil
-}
-
 // reconcileCluster reconciles the desired state of the Cluster object.
 // NOTE: this assumes reconcileInfrastructureCluster and reconcileControlPlane being already completed;
 // most specifically, after a Cluster is created it is assumed that the reference to the InfrastructureCluster /
@@ -448,13 +399,8 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, cluster *clust
 	}
 	r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: md.Object})
 
-	// If the MachineDeployment has defined a MachineHealthCheck set the OwnerReference and reconcile it.
+	// If the MachineDeployment has defined a MachineHealthCheck reconcile it.
 	if md.MachineHealthCheck != nil {
-		// Set the MachineDeployment Object as owner to ensure object garbage collection in case something
-		// happens before the MHC sets ownership to the Cluster.
-		md.MachineHealthCheck.SetOwnerReferences([]metav1.OwnerReference{
-			*ownerReferenceTo(md.Object),
-		})
 		if err := r.reconcileMachineHealthCheck(ctx, nil, md.MachineHealthCheck); err != nil {
 			return err
 		}

internal/controllers/topology/cluster/reconcile_state_test.go

@@ -870,7 +870,6 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) {
 				InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy(),
 				MachineHealthCheck:            mhcBuilder.Build()},
 			want: mhcBuilder.DeepCopy().
-				WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(controlPlane1)}).
 				WithDefaulter(true).
 				Build(),
 		},
@@ -960,7 +959,6 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) {
 					g.Expect(env.PatchAndWait(ctx, tt.current.InfrastructureMachineTemplate, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed())
 				}
 				if tt.current.MachineHealthCheck != nil {
-					tt.current.MachineHealthCheck.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(tt.current.Object)})
 					g.Expect(env.PatchAndWait(ctx, tt.current.MachineHealthCheck, client.ForceOwnership, client.FieldOwner(structuredmerge.TopologyManagerName))).To(Succeed())
 				}
 			}
@@ -997,11 +995,6 @@ func TestReconcileControlPlaneMachineHealthCheck(t *testing.T) {
 			}
 
 			g.Expect(err).ToNot(HaveOccurred())
-
-			if tt.want != nil {
-				tt.want.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(gotCP)})
-			}
-
 			g.Expect(gotMHC).To(EqualObject(tt.want, IgnoreAutogeneratedMetadata, IgnorePaths{{"kind"}, {"apiVersion"}}))
 		})
 	}
@@ -1834,7 +1827,6 @@ func TestReconcileMachineDeploymentMachineHealthCheck(t *testing.T) {
 				Timeout: metav1.Duration{Duration: 5 * time.Minute},
 			},
 		}).
-		WithOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(md)}).
 		WithClusterName("cluster1")
 
 	infrastructureMachineTemplate := builder.TestInfrastructureMachineTemplate(metav1.NamespaceDefault, "infrastructure-machine-1").Build()
@@ -2052,22 +2044,6 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) {
 			desired: mhcBuilder.DeepCopy().Build(),
 			want:    mhcBuilder.DeepCopy().WithDefaulter(true).Build(),
 		},
-		{
-			name:    "Successfully create a valid Ownerreference on the MachineHealthCheck",
-			current: nil,
-			// update the unhealthy conditions in the MachineHealthCheck
-			desired: mhcBuilder.DeepCopy().
-				// Desired object has an incomplete owner reference which has no UID.
-				WithOwnerReferences([]metav1.OwnerReference{{Name: cp.GetName(), Kind: cp.GetKind(), APIVersion: cp.GetAPIVersion()}}).
-				Build(),
-			// Want a reconciled object with a full ownerReference including UID
-			want: mhcBuilder.DeepCopy().
-				WithOwnerReferences([]metav1.OwnerReference{{Name: cp.GetName(), Kind: cp.GetKind(), APIVersion: cp.GetAPIVersion(), UID: cp.GetUID()}}).
-				WithDefaulter(true).
-				Build(),
-			wantErr: false,
-		},
-
 		{
 			name:    "Update a MachineHealthCheck with changes",
 			current: mhcBuilder.DeepCopy().Build(),

test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml

@@ -13,6 +13,12 @@ spec:
         kind: DockerMachineTemplate
         apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
         name: quick-start-control-plane
+    machineHealthCheck:
+      maxUnhealthy: 100%
+      unhealthyConditions:
+      - type: e2e.remediation.condition
+        status: "False"
+        timeout: 20s
   infrastructure:
     ref:
       apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
@@ -32,6 +38,12 @@ spec:
             apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
             kind: DockerMachineTemplate
             name: quick-start-default-worker-machinetemplate
+      machineHealthCheck:
+        maxUnhealthy: 100%
+        unhealthyConditions:
+        - type: e2e.remediation.condition
+          status: "False"
+          timeout: 20s
   variables:
   - name: lbImageRepository
     required: true