From 51bf143faab2a0b1cdd92e9da16e7bf156ee3277 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Sat, 28 Mar 2026 14:32:52 -0400 Subject: [PATCH 1/3] Move firewall check to preflight check Signed-off-by: Zhiwei Liang --- cloud/services/loadbalancers.go | 7 +----- cloud/services/loadbalancers_test.go | 23 ------------------- .../controller/linodecluster_controller.go | 15 +++++++----- .../linodecluster_controller_test.go | 15 ++++++++++++ 4 files changed, 25 insertions(+), 35 deletions(-) diff --git a/cloud/services/loadbalancers.go b/cloud/services/loadbalancers.go index eb0ee090c..c7be5f592 100644 --- a/cloud/services/loadbalancers.go +++ b/cloud/services/loadbalancers.go @@ -160,12 +160,7 @@ func EnsureNodeBalancer(ctx context.Context, clusterScope *scope.ClusterScope, l if clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID != nil { firewallID := *clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID logger.Info("Using direct NodeBalancerFirewallID", "firewallID", firewallID) - firewall, err := clusterScope.LinodeClient.GetFirewall(ctx, firewallID) - if err != nil { - logger.Error(err, "Failed to fetch Linode Firewall from the Linode API") - return nil, err - } - createConfig.FirewallID = firewall.ID + createConfig.FirewallID = firewallID } else if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil { // Only use NodeBalancerFirewallRef if no direct ID is provided firewallID, err := getFirewallID(ctx, clusterScope, logger) diff --git a/cloud/services/loadbalancers_test.go b/cloud/services/loadbalancers_test.go index ef9d3722d..bfeaf621a 100644 --- a/cloud/services/loadbalancers_test.go +++ b/cloud/services/loadbalancers_test.go @@ -228,9 +228,6 @@ func TestEnsureNodeBalancer(t *testing.T) { }, }, expects: func(mockClient *mock.MockLinodeClient, mockK8sClient *mock.MockK8sClient) { - mockClient.EXPECT().GetFirewall(gomock.Any(), 5678).Return(&linodego.Firewall{ - ID: 5678, - }, nil) mockClient.EXPECT().CreateNodeBalancer(gomock.Any(), linodego.NodeBalancerCreateOptions{ Label: util.Pointer("test-cluster"), Region: "us-east", @@ -269,26 +266,6 @@ func TestEnsureNodeBalancer(t *testing.T) { }, expectedError: fmt.Errorf("Failed to fetch LinodeFirewall"), }, - { - name: "Error - Direct FirewallID not found", - clusterScope: &scope.ClusterScope{ - LinodeCluster: &infrav1alpha2.LinodeCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-cluster", - UID: "test-uid", - }, - Spec: infrav1alpha2.LinodeClusterSpec{ - Network: infrav1alpha2.NetworkSpec{ - NodeBalancerFirewallID: util.Pointer(9999), - }, - }, - }, - }, - expects: func(mockClient *mock.MockLinodeClient, mockK8sClient *mock.MockK8sClient) { - mockClient.EXPECT().GetFirewall(gomock.Any(), 9999).Return(nil, fmt.Errorf("Firewall not found")) - }, - expectedError: fmt.Errorf("Firewall not found"), - }, { name: "Success - Create NodeBalancer in VPC", clusterScope: &scope.ClusterScope{ diff --git a/internal/controller/linodecluster_controller.go b/internal/controller/linodecluster_controller.go index 72baadf1f..bc11e8c25 100644 --- a/internal/controller/linodecluster_controller.go +++ b/internal/controller/linodecluster_controller.go @@ -228,10 +228,14 @@ func (r *LinodeClusterReconciler) performPreflightChecks(ctx context.Context, lo } } - if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil { + // Check firewall configuration - either direct ID or reference if !reconciler.ConditionTrue(clusterScope.LinodeCluster.GetCondition(ConditionPreflightLinodeNBFirewallReady)) { - res, err := r.reconcilePreflightLinodeFirewallCheck(ctx, logger, clusterScope) - if err != nil || !res.IsZero() { + if clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID != nil { + if res, err := r.reconcilePreflightFirewallID(ctx, logger, clusterScope); err != nil || !res.IsZero() { + return res, err + } + } else if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil { + if res, err := r.reconcilePreflightFirewallRef(ctx, logger, clusterScope); err != nil || !res.IsZero() { return res, err } } @@ -240,9 +244,7 @@ func (r *LinodeClusterReconciler) performPreflightChecks(ctx context.Context, lo return ctrl.Result{}, nil } -func (r *LinodeClusterReconciler) reconcilePreflightLinodeFirewallCheck(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) (ctrl.Result, error) { - // If NodeBalancerFirewallID is directly specified, check if it exists - if clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID != nil { +func (r *LinodeClusterReconciler) reconcilePreflightFirewallID(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) (ctrl.Result, error) { firewallID := *clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID logger.Info("Verifying direct NodeBalancerFirewallID", "firewallID", firewallID) _, err := clusterScope.LinodeClient.GetFirewall(ctx, firewallID) @@ -264,6 +266,7 @@ func (r *LinodeClusterReconciler) reconcilePreflightLinodeFirewallCheck(ctx cont return ctrl.Result{}, nil } +func (r *LinodeClusterReconciler) reconcilePreflightFirewallRef(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) (ctrl.Result, error) { name := clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef.Name namespace := clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef.Namespace if namespace == "" { diff --git a/internal/controller/linodecluster_controller_test.go b/internal/controller/linodecluster_controller_test.go index b5998bb70..c5e8207c7 100644 --- a/internal/controller/linodecluster_controller_test.go +++ b/internal/controller/linodecluster_controller_test.go @@ -184,6 +184,21 @@ var _ = Describe("cluster-lifecycle", Ordered, Label("cluster", "cluster-lifecyc Expect(linodeCluster.GetCondition(ConditionPreflightLinodeNBFirewallReady).Status).To(Equal(metav1.ConditionFalse)) }), ), + Path( + Call("direct firewall ID not found", func(ctx context.Context, mck Mock) { + cScope.LinodeClient = mck.LinodeClient + cScope.LinodeCluster.Spec.NodeBalancerFirewallRef = nil + cScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID = util.Pointer(9999) + mck.LinodeClient.EXPECT().GetFirewall(gomock.Any(), 9999).Return(nil, errors.New("firewall not found")) + }), + Result("preflight sets condition false", func(ctx context.Context, mck Mock) { + reconciler.Client = k8sClient + res, err := reconciler.reconcile(ctx, cScope, mck.Logger()) + Expect(err).NotTo(HaveOccurred()) + Expect(res.RequeueAfter).To(Equal(rec.DefaultClusterControllerReconcileDelay)) + Expect(linodeCluster.GetCondition(ConditionPreflightLinodeNBFirewallReady).Status).To(Equal(metav1.ConditionFalse)) + }), + ), Path( Call("cluster is not created because there was an error creating nb", func(ctx context.Context, mck Mock) { cScope.LinodeClient = mck.LinodeClient From e617ef3bb639110c698117cbd9def4e484453040 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Mon, 4 May 2026 02:25:28 -0400 Subject: [PATCH 2/3] format Signed-off-by: Zhiwei Liang --- internal/controller/linodecluster_controller.go | 1 - internal/controller/linodefirewall_controller_test.go | 6 ++++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/controller/linodecluster_controller.go b/internal/controller/linodecluster_controller.go index bc11e8c25..ac6118ffa 100644 --- a/internal/controller/linodecluster_controller.go +++ b/internal/controller/linodecluster_controller.go @@ -107,7 +107,6 @@ func (r *LinodeClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques LinodeMachineList: infrav1alpha2.LinodeMachineList{}, }, ) - if err != nil { logger.Info("Failed to create cluster scope", "error", err.Error()) return ctrl.Result{}, fmt.Errorf("failed to create cluster scope: %w", err) diff --git a/internal/controller/linodefirewall_controller_test.go b/internal/controller/linodefirewall_controller_test.go index f216129bf..3e7e54eca 100644 --- a/internal/controller/linodefirewall_controller_test.go +++ b/internal/controller/linodefirewall_controller_test.go @@ -176,7 +176,8 @@ var _ = Describe("lifecycle", Ordered, Label("firewalls", "lifecycle"), func() { Protocol: "TCP", Addresses: &infrav1alpha2.NetworkAddresses{ IPv4: &[]string{fmt.Sprintf("192.168.%d.%d", idx, 0)}, - }}) + }, + }) } res, err := reconciler.reconcile(ctx, mck.Logger(), &fwScope) Expect(err).ToNot(HaveOccurred()) @@ -273,7 +274,8 @@ var _ = Describe("lifecycle", Ordered, Label("firewalls", "lifecycle"), func() { Protocol: "TCP", Addresses: &infrav1alpha2.NetworkAddresses{ IPv4: &ipv4s, - }}) + }, + }) mck.LinodeClient.EXPECT().GetFirewall(ctx, 1).Return(&linodego.Firewall{ ID: 1, From a1bae139334946c7d01ebf8ef489f6bd540e6874 Mon Sep 17 00:00:00 2001 From: Zhiwei Liang Date: Mon, 4 May 2026 02:48:55 -0400 Subject: [PATCH 3/3] format again Signed-off-by: Zhiwei Liang --- .../controller/linodecluster_controller.go | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/internal/controller/linodecluster_controller.go b/internal/controller/linodecluster_controller.go index d131d375d..4be75f781 100644 --- a/internal/controller/linodecluster_controller.go +++ b/internal/controller/linodecluster_controller.go @@ -233,7 +233,7 @@ func (r *LinodeClusterReconciler) performPreflightChecks(ctx context.Context, lo } // Check firewall configuration - either direct ID or reference - if !reconciler.ConditionTrue(clusterScope.LinodeCluster.GetCondition(ConditionPreflightLinodeNBFirewallReady)) { + if !reconciler.ConditionTrue(clusterScope.LinodeCluster.GetCondition(ConditionPreflightLinodeNBFirewallReady)) { if clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID != nil { if res, err := r.reconcilePreflightFirewallID(ctx, logger, clusterScope); err != nil || !res.IsZero() { return res, err @@ -249,26 +249,26 @@ func (r *LinodeClusterReconciler) performPreflightChecks(ctx context.Context, lo } func (r *LinodeClusterReconciler) reconcilePreflightFirewallID(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) (ctrl.Result, error) { - firewallID := *clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID - logger.Info("Verifying direct NodeBalancerFirewallID", "firewallID", firewallID) - _, err := clusterScope.LinodeClient.GetFirewall(ctx, firewallID) - if err != nil { - logger.Error(err, "Failed to get NodeBalancer firewall with provided ID", "firewallID", firewallID) - clusterScope.LinodeCluster.SetCondition(metav1.Condition{ - Type: ConditionPreflightLinodeNBFirewallReady, - Status: metav1.ConditionFalse, - Reason: util.CreateError, - Message: err.Error(), - }) - return ctrl.Result{RequeueAfter: reconciler.WithJitter(reconciler.DefaultClusterControllerReconcileDelay)}, nil - } + firewallID := *clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID + logger.Info("Verifying direct NodeBalancerFirewallID", "firewallID", firewallID) + _, err := clusterScope.LinodeClient.GetFirewall(ctx, firewallID) + if err != nil { + logger.Error(err, "Failed to get NodeBalancer firewall with provided ID", "firewallID", firewallID) clusterScope.LinodeCluster.SetCondition(metav1.Condition{ - Type: ConditionPreflightLinodeNBFirewallReady, - Status: metav1.ConditionTrue, - Reason: "LinodeFirewallReady", // We have to set the reason to not fail object patching + Type: ConditionPreflightLinodeNBFirewallReady, + Status: metav1.ConditionFalse, + Reason: util.CreateError, + Message: err.Error(), }) - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: reconciler.WithJitter(reconciler.DefaultClusterControllerReconcileDelay)}, nil } + clusterScope.LinodeCluster.SetCondition(metav1.Condition{ + Type: ConditionPreflightLinodeNBFirewallReady, + Status: metav1.ConditionTrue, + Reason: "LinodeFirewallReady", // We have to set the reason to not fail object patching + }) + return ctrl.Result{}, nil +} func (r *LinodeClusterReconciler) reconcilePreflightFirewallRef(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) (ctrl.Result, error) { name := clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef.Name