From b36fde048ab0897f350392180208356375b2cecb Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Tue, 22 Nov 2022 18:54:30 +0000 Subject: [PATCH] Ensure infra and bootstrap objects are owned by Machines Signed-off-by: killianmuldoon --- .../machine/machine_controller_phases.go | 46 ++++++++++++++----- .../machineset/machineset_controller_test.go | 40 ++++++++++++---- 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/internal/controllers/machine/machine_controller_phases.go b/internal/controllers/machine/machine_controller_phases.go index 4fa3685e3c3b..dbee3e707b0b 100644 --- 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 index 49db0fa23647..9dfb4bdff096 100644 --- 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 {