Move NodeBalancer firewall validation from creation time to preflight check

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)

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{

internal/controller/linodecluster_controller.go

@@ -108,7 +108,6 @@
 			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)
@@ -233,10 +232,14 @@
 		}
 	}
 
-	if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil {
-		if !reconciler.ConditionTrue(clusterScope.LinodeCluster.GetCondition(ConditionPreflightLinodeNBFirewallReady)) {
-			res, err := r.reconcilePreflightLinodeFirewallCheck(ctx, logger, clusterScope)
-			if err != nil || !res.IsZero() {
+	// Check firewall configuration - either direct ID or reference
+	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
+			}
+		} else if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil {
+			if res, err := r.reconcilePreflightFirewallRef(ctx, logger, clusterScope); err != nil || !res.IsZero() {
 				return res, err
 			}
 		}
@@ -245,30 +248,29 @@
 	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 {
-		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
-		}
+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.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
 	namespace := clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef.Namespace
 	if namespace == "" {

internal/controller/linodecluster_controller_test.go

@@ -188,6 +188,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

internal/controller/linodefirewall_controller_test.go

@@ -181,7 +181,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())
@@ -280,7 +281,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,