Ensure infra and bootstrap objects are owned by Machines

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
kubernetes-sigs · Nov 28, 2022 · b36fde0 · b36fde0
1 parent 0a17fa2
commit b36fde0
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 21 deletions.
diff --git a/internal/controllers/machine/machine_controller_phases.go b/internal/controllers/machine/machine_controller_phases.go
@@ -116,17 +116,11 @@ 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 control plane owners from the objects referred to by a Machine.
+	// These ownerReferences are added initially because Machines don't exist when those objects are created.
+	// At this point the Machine exists and can be set as the controller reference.
+	if err := removeOutdatedOwnerRefs(cluster, m, obj); err != nil {
+		return external.ReconcileOutput{}, err
 	}
 
 	// Set external object ControllerReference to the Machine.
@@ -372,3 +366,33 @@ func (r *Reconciler) reconcileCertificateExpiry(ctx context.Context, _ *clusterv
 
 	return ctrl.Result{}, nil
 }
+
+// removeOutdatedOwnerRefs will remove any MachineSet or control plane ownerReferences from passed objects.
+func removeOutdatedOwnerRefs(cluster *clusterv1.Cluster, m *clusterv1.Machine, obj *unstructured.Unstructured) error {
+	cpGVK := getControlPlaneGVKForMachine(cluster, m)
+	for _, owner := range obj.GetOwnerReferences() {
+		ownerGV, err := schema.ParseGroupVersion(owner.APIVersion)
+		if err != nil {
+			return errors.Wrapf(err, "Could not remove ownerReference %v from object %s/%s", owner.String(), obj.GetKind(), obj.GetName())
+		}
+		if (ownerGV.Group == clusterv1.GroupVersion.Group && owner.Kind == "MachineSet") ||
+			(cpGVK != nil && ownerGV.Group == cpGVK.GroupVersion().Group && owner.Kind == cpGVK.Kind) {
+			ownerRefs := util.RemoveOwnerRef(obj.GetOwnerReferences(), owner)
+			obj.SetOwnerReferences(ownerRefs)
+		}
+	}
+	return nil
+}
+
+// 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
+}
diff --git a/internal/controllers/machineset/machineset_controller_test.go b/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 {