From e2a6eb0323fd66aa7a7071d0de44829f86983889 Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Tue, 14 Oct 2025 16:16:51 -0400 Subject: [PATCH 1/9] check for firewall ref --- internal/controller/linodemachine_controller.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index 87393719c..8a854d229 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -826,6 +826,14 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge var desiredFWIDs []int if machineScope.LinodeMachine.Spec.FirewallID != 0 { desiredFWIDs = []int{machineScope.LinodeMachine.Spec.FirewallID} + } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { + fwID, err := getFirewallID(ctx, machineScope, logger) + if err != nil { + logger.Error(err, "Failed to get firewallID from firewallRef") + desiredFWIDs = []int{} + } else { + desiredFWIDs = []int{fwID} + } } else { desiredFWIDs = []int{} } From 3c3b63307cd163ff7c859a0f5597c3f5e0fdb97f Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Wed, 15 Oct 2025 10:08:44 -0400 Subject: [PATCH 2/9] add unit test --- .../controller/linodemachine_controller.go | 11 ++-- .../linodemachine_controller_test.go | 52 +++++++++++++++++++ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index abf94d253..8499f44f4 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -823,19 +823,16 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge attachedFWIDs = append(attachedFWIDs, fw.ID) } - var desiredFWIDs []int + desiredFWIDs := []int{} if machineScope.LinodeMachine.Spec.FirewallID != 0 { desiredFWIDs = []int{machineScope.LinodeMachine.Spec.FirewallID} } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { fwID, err := getFirewallID(ctx, machineScope, logger) - if err != nil { - logger.Error(err, "Failed to get firewallID from firewallRef") - desiredFWIDs = []int{} - } else { + if err == nil { desiredFWIDs = []int{fwID} + } else { + logger.Error(err, "Failed to get firewallID from firewallRef") } - } else { - desiredFWIDs = []int{} } // update the firewallID if needed. diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index 010d89813..24e529fa9 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -1968,6 +1968,58 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Expect(err).NotTo(HaveOccurred()) }), ), + Path( + Call("machine firewall update applied when FirewallID is zero but FirewallRef is set", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( + &linodego.Instance{ + ID: 11111, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Tags: []string{"test-cluster-2"}, + Status: linodego.InstanceRunning, + Updated: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().UpdateInstance(ctx, 11111, gomock.Any()).Return( + &linodego.Instance{ + ID: 11111, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Tags: []string{"test-cluster-2", "test-tag"}, + Status: linodego.InstanceRunning, + Updated: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().ListInstanceFirewalls(ctx, 11111, nil).Return( + []linodego.Firewall{}, nil) + mck.LinodeClient.EXPECT().UpdateInstanceFirewalls(ctx, 11111, linodego.InstanceFirewallUpdateOptions{ + FirewallIDs: []int{20}, // Update to firewall ID 20 from firewall ref + }).Return(nil, nil) + }), + Result("machine firewall updates to FirewallID from FirewallRef", func(ctx context.Context, mck Mock) { + linodeFirewall := &infrav1alpha2.LinodeFirewall{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-firewall-ref", + Namespace: namespace, + }, + Spec: infrav1alpha2.LinodeFirewallSpec{ + FirewallID: ptr.To(20), + Enabled: true, + }, + } + Expect(k8sClient.Create(ctx, linodeFirewall)).To(Succeed()) + linodeFirewall.Status.Ready = true + Expect(k8sClient.Status().Update(ctx, linodeFirewall)).To(Succeed()) + + linodeMachine.Spec.FirewallID = 0 // No firewall ID explicitly set + linodeMachine.Spec.FirewallRef = &corev1.ObjectReference{ + Name: "test-firewall-ref", + Namespace: namespace, + } + + _, err = reconciler.reconcile(ctx, logr.Logger{}, mScope) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient.Delete(ctx, linodeFirewall)).To(Succeed()) + }), + ), OneOf( Path( Call("machine firewall list fails", func(ctx context.Context, mck Mock) { From c94f80e67f054cb0233eebfb798333a9bf533a1b Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Wed, 15 Oct 2025 10:21:49 -0400 Subject: [PATCH 3/9] ? --- internal/controller/linodemachine_controller.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index 8499f44f4..f2c51e450 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -827,11 +827,8 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge if machineScope.LinodeMachine.Spec.FirewallID != 0 { desiredFWIDs = []int{machineScope.LinodeMachine.Spec.FirewallID} } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { - fwID, err := getFirewallID(ctx, machineScope, logger) - if err == nil { + if fwID, err := getFirewallID(ctx, machineScope, logger); err == nil { desiredFWIDs = []int{fwID} - } else { - logger.Error(err, "Failed to get firewallID from firewallRef") } } From 0c9e2c386504056c24a910fa374359b40cc77da8 Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Wed, 15 Oct 2025 11:58:49 -0400 Subject: [PATCH 4/9] requeue on err, test updates --- .../controller/linodemachine_controller.go | 2 ++ .../linodemachine_controller_test.go | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index f2c51e450..a125a7057 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -829,6 +829,8 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { if fwID, err := getFirewallID(ctx, machineScope, logger); err == nil { desiredFWIDs = []int{fwID} + } else { + return ctrl.Result{}, err } } diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index 24e529fa9..4d47e26fa 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -2020,6 +2020,41 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Expect(k8sClient.Delete(ctx, linodeFirewall)).To(Succeed()) }), ), + Path( + Call("machine firewall fails to get FirewallID from FirewallRef", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( + &linodego.Instance{ + ID: 11111, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Tags: []string{"test-cluster-2"}, + Status: linodego.InstanceRunning, + Updated: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().UpdateInstance(ctx, 11111, gomock.Any()).Return( + &linodego.Instance{ + ID: 11111, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Tags: []string{"test-cluster-2", "test-tag"}, + Status: linodego.InstanceRunning, + Updated: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().ListInstanceFirewalls(ctx, 11111, nil).Return( + []linodego.Firewall{}, nil) + + }), + Result("machine firewall update error requeues", func(ctx context.Context, mck Mock) { + linodeMachine.Spec.FirewallID = 0 // No firewall ID explicitly set + linodeMachine.Spec.FirewallRef = &corev1.ObjectReference{ + Name: "test-firewall-ref", + Namespace: namespace, + } // this firewall does not exist + _, err := reconciler.reconcile(ctx, mck.Logger(), mScope) + Expect(err).To(HaveOccurred()) + Expect(mck.Logs()).To(ContainSubstring("Failed to fetch LinodeFirewall")) + }), + ), OneOf( Path( Call("machine firewall list fails", func(ctx context.Context, mck Mock) { From 78790b9b0716be4ae84b7c3f371f4cf39a62b0fe Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Wed, 15 Oct 2025 12:01:07 -0400 Subject: [PATCH 5/9] spacing --- internal/controller/linodemachine_controller_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index 4d47e26fa..711a6092c 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -2014,7 +2014,6 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Name: "test-firewall-ref", Namespace: namespace, } - _, err = reconciler.reconcile(ctx, logr.Logger{}, mScope) Expect(err).NotTo(HaveOccurred()) Expect(k8sClient.Delete(ctx, linodeFirewall)).To(Succeed()) @@ -2042,7 +2041,6 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), }, nil) mck.LinodeClient.EXPECT().ListInstanceFirewalls(ctx, 11111, nil).Return( []linodego.Firewall{}, nil) - }), Result("machine firewall update error requeues", func(ctx context.Context, mck Mock) { linodeMachine.Spec.FirewallID = 0 // No firewall ID explicitly set From aa5b272268ddd6bac280a99425c1ce5ea6215077 Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Wed, 15 Oct 2025 12:14:19 -0400 Subject: [PATCH 6/9] requeue delay --- internal/controller/linodemachine_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index a125a7057..8faf1d61c 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -830,7 +830,7 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge if fwID, err := getFirewallID(ctx, machineScope, logger); err == nil { desiredFWIDs = []int{fwID} } else { - return ctrl.Result{}, err + return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerRetryDelay}, err } } From 23877f4b0656928aa65cdc1900321faf578ef169 Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Wed, 15 Oct 2025 12:16:16 -0400 Subject: [PATCH 7/9] cleaner --- internal/controller/linodemachine_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index 8faf1d61c..e4cf7e01b 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -827,11 +827,11 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge if machineScope.LinodeMachine.Spec.FirewallID != 0 { desiredFWIDs = []int{machineScope.LinodeMachine.Spec.FirewallID} } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { - if fwID, err := getFirewallID(ctx, machineScope, logger); err == nil { - desiredFWIDs = []int{fwID} - } else { + fwID, err := getFirewallID(ctx, machineScope, logger) + if err != nil { return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerRetryDelay}, err } + desiredFWIDs = []int{fwID} } // update the firewallID if needed. From eb3fd3f8f7106fae3b64bc9a6385ee390504d461 Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Thu, 16 Oct 2025 09:02:02 -0400 Subject: [PATCH 8/9] return nil not err --- internal/controller/linodemachine_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index e4cf7e01b..30a9fc72a 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -829,7 +829,7 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { fwID, err := getFirewallID(ctx, machineScope, logger) if err != nil { - return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerRetryDelay}, err + return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerRetryDelay}, nil } desiredFWIDs = []int{fwID} } From 6c1606b66b5f8c72271fe635ff28d706d5af1362 Mon Sep 17 00:00:00 2001 From: Sam Cheng Date: Thu, 16 Oct 2025 12:01:13 -0400 Subject: [PATCH 9/9] lint, tests --- internal/controller/linodemachine_controller.go | 1 + internal/controller/linodemachine_controller_test.go | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index 30a9fc72a..8da8b9ab9 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -829,6 +829,7 @@ func (r *LinodeMachineReconciler) reconcileFirewallID(ctx context.Context, logge } else if machineScope.LinodeMachine.Spec.FirewallRef != nil { fwID, err := getFirewallID(ctx, machineScope, logger) if err != nil { + logger.Error(err, "Failed to get firewall ID from firewall ref") return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerRetryDelay}, nil } desiredFWIDs = []int{fwID} diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index 711a6092c..72cb9ba01 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -2048,8 +2048,9 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Name: "test-firewall-ref", Namespace: namespace, } // this firewall does not exist - _, err := reconciler.reconcile(ctx, mck.Logger(), mScope) - Expect(err).To(HaveOccurred()) + res, err := reconciler.reconcile(ctx, mck.Logger(), mScope) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(rutil.DefaultMachineControllerRetryDelay)) Expect(mck.Logs()).To(ContainSubstring("Failed to fetch LinodeFirewall")) }), ),