Keep MachineStaticIPFinalizer to prevent MachineStaticIPFinalizer fro…

…m being able to release IP after being removed by CAPE (#28)

controllers/elfmachine_controller.go

@@ -223,20 +223,9 @@ func (r *ElfMachineReconciler) reconcileDelete(ctx *context.MachineContext) (rec
 }
 
 func (r *ElfMachineReconciler) reconcileIPAddress(ctx *context.MachineContext) (reconcile.Result, error) {
-	// If the ElfMachine is in an error state, return early.
-	if ctx.ElfMachine.IsFailed() {
-		r.Logger.V(2).Info("Error state detected, skipping reconciliation")
-		return reconcile.Result{}, nil
-	}
-
 	devices := ctx.ElfMachine.Spec.Network.Devices
-	if len(devices) == 0 {
-		ctx.Logger.V(2).Info("No network device found")
-		return ctrl.Result{}, nil
-	}
-
-	if !ipamutil.NeedsAllocateIP(devices) {
-		ctx.Logger.V(6).Info("No need to allocate static IP")
+	if !ipamutil.HasStaticIPDevice(devices) {
+		ctx.Logger.V(6).Info("No static IP network device found")
 		return ctrl.Result{}, nil
 	}
 
@@ -249,11 +238,31 @@ func (r *ElfMachineReconciler) reconcileIPAddress(ctx *context.MachineContext) (
 	// If the ElfMachine doesn't have MachineStaticIPFinalizer, add it and return with requeue.
 	// In next reconcile, the static IP will be allocated.
 	if !ctrlutil.ContainsFinalizer(ctx.ElfMachine, MachineStaticIPFinalizer) {
+		// Add MachineStaticIPFinalizer after setting MachineFinalizer,
+		// otherwise MachineStaticIPFinalizer may be overwritten by CAPE
+		// when setting MachineFinalizer.
+		if !ctrlutil.ContainsFinalizer(ctx.ElfMachine, capev1.MachineFinalizer) {
+			r.Logger.V(2).Info("Waiting for CAPE to set MachineFinalizer on ElfMachine")
+
+			return reconcile.Result{RequeueAfter: config.DefaultRequeue}, nil
+		}
+
 		ctrlutil.AddFinalizer(ctx.ElfMachine, MachineStaticIPFinalizer)
 
 		return ctrl.Result{RequeueAfter: 3 * time.Second}, nil
 	}
 
+	// If the ElfMachine is in an error state, return early.
+	if ctx.ElfMachine.IsFailed() {
+		r.Logger.V(2).Info("Error state detected, skipping reconciliation")
+		return reconcile.Result{}, nil
+	}
+
+	if !ipamutil.NeedsAllocateIP(devices) {
+		ctx.Logger.V(6).Info("No need to allocate static IP")
+		return ctrl.Result{}, nil
+	}
+
 	ipPool, err := r.getIPPool(ctx)
 	if err != nil {
 		return ctrl.Result{}, err

controllers/elfmachine_controller_test.go

@@ -84,6 +84,8 @@ var _ = Describe("ElfMachineReconciler", func() {
 	})
 
 	It("should not reconcile when ElfMachine in an error state", func() {
+		ctrlutil.AddFinalizer(elfMachine, capev1.MachineFinalizer)
+		ctrlutil.AddFinalizer(elfMachine, MachineStaticIPFinalizer)
 		elfMachine.Status.FailureMessage = pointer.String("some error")
 		ctrlContext := newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate)
 		fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine)
@@ -117,7 +119,7 @@ var _ = Describe("ElfMachineReconciler", func() {
 		Expect(logBuffer.String()).To(ContainSubstring("ElfMachine linked to a cluster that is paused"))
 	})
 
-	It("should not reconcile without devices", func() {
+	It("should not reconcile without static IP network devices", func() {
 		ctrlutil.RemoveFinalizer(elfMachine, MachineStaticIPFinalizer)
 		elfMachine.Spec.Network.Devices = []capev1.NetworkDeviceSpec{}
 		ctrlContext := newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate)
@@ -127,12 +129,14 @@ var _ = Describe("ElfMachineReconciler", func() {
 		result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: capiutil.ObjectKey(elfMachine)})
 		Expect(result).To(BeZero())
 		Expect(err).ToNot(HaveOccurred())
-		Expect(logBuffer.String()).To(ContainSubstring("No network device found"))
+		Expect(logBuffer.String()).To(ContainSubstring("No static IP network device found"))
 		Expect(ctrlContext.Client.Get(ctrlContext, capiutil.ObjectKey(elfMachine), elfMachine)).To(Succeed())
 		Expect(ctrlutil.ContainsFinalizer(elfMachine, MachineStaticIPFinalizer)).To(BeFalse())
 	})
 
 	It("should not reconcile when no need to allocate static IP", func() {
+		ctrlutil.AddFinalizer(elfMachine, capev1.MachineFinalizer)
+		ctrlutil.AddFinalizer(elfMachine, MachineStaticIPFinalizer)
 		elfMachine.Spec.Network.Devices = []capev1.NetworkDeviceSpec{
 			{NetworkType: capev1.NetworkTypeIPV4, IPAddrs: []string{fake.IP()}},
 			{NetworkType: capev1.NetworkTypeIPV4DHCP},
@@ -147,7 +151,6 @@ var _ = Describe("ElfMachineReconciler", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(logBuffer.String()).To(ContainSubstring("No need to allocate static IP"))
 		Expect(ctrlContext.Client.Get(ctrlContext, capiutil.ObjectKey(elfMachine), elfMachine)).To(Succeed())
-		Expect(ctrlutil.ContainsFinalizer(elfMachine, MachineStaticIPFinalizer)).To(BeFalse())
 	})
 
 	It("should set MachineStaticIPFinalizer first", func() {
@@ -159,6 +162,18 @@ var _ = Describe("ElfMachineReconciler", func() {
 		Expect(result).NotTo(BeZero())
 		Expect(err).NotTo(HaveOccurred())
 		Expect(ctrlContext.Client.Get(ctrlContext, capiutil.ObjectKey(elfMachine), elfMachine)).To(Succeed())
+		Expect(ctrlutil.ContainsFinalizer(elfMachine, MachineStaticIPFinalizer)).To(BeFalse())
+		Expect(logBuffer.String()).To(ContainSubstring("Waiting for CAPE to set MachineFinalizer on ElfMachine"))
+
+		ctrlutil.AddFinalizer(elfMachine, capev1.MachineFinalizer)
+		ctrlContext = newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate)
+		fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine)
+
+		reconciler = &ElfMachineReconciler{ControllerContext: ctrlContext}
+		result, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: capiutil.ObjectKey(elfMachine)})
+		Expect(result).NotTo(BeZero())
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ctrlContext.Client.Get(ctrlContext, capiutil.ObjectKey(elfMachine), elfMachine)).To(Succeed())
 		Expect(ctrlutil.ContainsFinalizer(elfMachine, MachineStaticIPFinalizer)).To(BeTrue())
 	})