Ensure infra and bootstrap objects are owned by Machines

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

internal/controllers/machine/machine_controller_phases.go

@@ -116,18 +116,10 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 		return external.ReconcileOutput{}, err
 	}
 
-	// With the migration from v1alpha2 to v1alpha3, Machine controllers should be the owner for the
-	// infra Machines, hence remove any existing machineset controller owner reference
-	if controller := metav1.GetControllerOf(obj); controller != nil && controller.Kind == "MachineSet" {
-		gv, err := schema.ParseGroupVersion(controller.APIVersion)
-		if err != nil {
-			return external.ReconcileOutput{}, err
-		}
-		if gv.Group == clusterv1.GroupVersion.Group {
-			ownerRefs := util.RemoveOwnerRef(obj.GetOwnerReferences(), *controller)
-			obj.SetOwnerReferences(ownerRefs)
-		}
-	}
+	// removeOutdatedOwnerRefs removes MachineSet and ControlPlane owners from the objects referred to by a Machine.
+	// MachineSet and ControlPlane ownerReferences are added as Machine's don't exist at the time when objects
+	// owned are created. At this point the Machine exists and can be set as the controller reference.
+	removeOutdatedOwnerRefs(cluster, m, obj)
 
 	// Set external object ControllerReference to the Machine.
 	if err := controllerutil.SetControllerReference(m, obj, r.Client.Scheme()); err != nil {
@@ -372,3 +364,30 @@ func (r *Reconciler) reconcileCertificateExpiry(ctx context.Context, _ *clusterv
 
 	return ctrl.Result{}, nil
 }
+
+// removeOutdatedOwnerRefs will remove any MachineSet or ControlPlane ownerReferences from Infrastructure Machine and
+// Bootstrap objects referred to by the machine. These ownerReferences are added on  creation as the Machine does not exist
+// when the objects are initially created.
+func removeOutdatedOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, obj *unstructured.Unstructured) {
+	cpGVK := getControlPlaneGVKForMachine(cluster, m)
+	for _, owner := range obj.GetOwnerReferences() {
+		if (owner.APIVersion == clusterv1.GroupVersion.String() && owner.Kind == "MachineSet") ||
+			(cpGVK != nil && owner.APIVersion == cpGVK.GroupVersion().String() && owner.Kind == cpGVK.Kind) {
+			ownerRefs := util.RemoveOwnerRef(obj.GetOwnerReferences(), owner)
+			obj.SetOwnerReferences(ownerRefs)
+		}
+	}
+}
+
+// getControlPlaneGVKForMachine returns the Kind of the control plane in the Cluster associated with the Machine.
+// This function checks that the machine is managed by a control plane, and then retrieves the kind from the Cluster's
+// .spec.controlPlaneRef.
+func getControlPlaneGVKForMachine(cluster *clusterv1.Cluster, machine *clusterv1.Machine) *schema.GroupVersionKind {
+	if _, ok := machine.GetLabels()[clusterv1.MachineControlPlaneLabelName]; ok {
+		if cluster.Spec.ControlPlaneRef != nil {
+			gvk := cluster.Spec.ControlPlaneRef.GroupVersionKind()
+			return &gvk
+		}
+	}
+	return nil
+}

internal/controllers/machineset/machineset_controller_test.go

@@ -228,14 +228,24 @@ func TestMachineSetReconciler(t *testing.T) {
 			g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("annotation-1", "true"), "have annotations of MachineTemplate applied")
 			g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("precedence", "MachineSet"), "the annotations from the MachineSpec template to overwrite the infrastructure template ones")
 			g.Expect(im.GetLabels()).To(HaveKeyWithValue("label-1", "true"), "have labels of MachineTemplate applied")
+		}
+		g.Eventually(func() bool {
+			g.Expect(env.List(ctx, infraMachines, client.InNamespace(namespace.Name))).To(Succeed())
+			// The Machine reconciler should remove the ownerReference to the MachineSet on the InfrastructureMachine.
 			hasMSOwnerRef := false
-			for _, o := range im.GetOwnerReferences() {
-				if o.Kind == machineSetKind.Kind {
-					hasMSOwnerRef = true
+			hasMachineOwnerRef := false
+			for _, im := range infraMachines.Items {
+				for _, o := range im.GetOwnerReferences() {
+					if o.Kind == machineSetKind.Kind {
+						hasMSOwnerRef = true
+					}
+					if o.Kind == "Machine" {
+						hasMachineOwnerRef = true
+					}
 				}
 			}
-			g.Expect(hasMSOwnerRef).To(BeTrue(), "have ownerRef to MachineSet")
-		}
+			return !hasMSOwnerRef && hasMachineOwnerRef
+		}, timeout).Should(BeTrue(), "infraMachine should not have ownerRef to MachineSet")
 
 		t.Log("Creating a BootstrapConfig for each Machine")
 		bootstrapConfigs := &unstructured.UnstructuredList{}
@@ -251,14 +261,24 @@ func TestMachineSetReconciler(t *testing.T) {
 			g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("annotation-1", "true"), "have annotations of MachineTemplate applied")
 			g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("precedence", "MachineSet"), "the annotations from the MachineSpec template to overwrite the bootstrap config template ones")
 			g.Expect(im.GetLabels()).To(HaveKeyWithValue("label-1", "true"), "have labels of MachineTemplate applied")
+		}
+		g.Eventually(func() bool {
+			g.Expect(env.List(ctx, bootstrapConfigs, client.InNamespace(namespace.Name))).To(Succeed())
+			// The Machine reconciler should remove the ownerReference to the MachineSet on the Bootstrap object.
 			hasMSOwnerRef := false
-			for _, o := range im.GetOwnerReferences() {
-				if o.Kind == machineSetKind.Kind {
-					hasMSOwnerRef = true
+			hasMachineOwnerRef := false
+			for _, im := range bootstrapConfigs.Items {
+				for _, o := range im.GetOwnerReferences() {
+					if o.Kind == machineSetKind.Kind {
+						hasMSOwnerRef = true
+					}
+					if o.Kind == "Machine" {
+						hasMachineOwnerRef = true
+					}
 				}
 			}
-			g.Expect(hasMSOwnerRef).To(BeTrue(), "have ownerRef to MachineSet")
-		}
+			return !hasMSOwnerRef && hasMachineOwnerRef
+		}, timeout).Should(BeTrue(), "bootstrap should not have ownerRef to MachineSet")
 
 		// Set the infrastructure reference as ready.
 		for _, m := range machines.Items {