Move firewall check to preflight check

Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>

Author: ChihweiLHBird

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

@@ -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 == "" {

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