Reverse firewall precedence to prioritize direct IDs over references (#717)

Direct firewall IDs now take precedence over references for both
LinodeMachine and LinodeCluster resources, bringing firewall configuration
logic in line with how we handle VPC resources. This change provides more
intuitive behavior and prevents reference-resolved IDs from being stored
back in the spec. Documentation updated with migration notes for existing
clusters.

Author: komer3

cloud/services/loadbalancers.go

@@ -141,27 +141,24 @@ func EnsureNodeBalancer(ctx context.Context, clusterScope *scope.ClusterScope, l
 		}
 	}
 
-	// get firewall ID from firewallRef if it exists
-	if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil {
-		firewallID, err := getFirewallID(ctx, clusterScope, logger)
-		if err != nil {
-			logger.Error(err, "Failed to fetch LinodeFirewall ID")
-			return nil, err
-		}
-		createConfig.FirewallID = firewallID
-		clusterScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID = &firewallID
-	}
-
-	// Use a firewall created outside of the CAPL ecosystem
-	// get & validate firewall ID from .Spec.Network.FirewallID if it exists
+	// First check if a direct NodeBalancerFirewallID is specified (prioritize direct ID)
 	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
+	} else if clusterScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil {
+		// Only use NodeBalancerFirewallRef if no direct ID is provided
+		firewallID, err := getFirewallID(ctx, clusterScope, logger)
+		if err != nil {
+			logger.Error(err, "Failed to fetch LinodeFirewall ID from reference")
+			return nil, err
+		}
+		createConfig.FirewallID = firewallID
 	}
 
 	return clusterScope.LinodeClient.CreateNodeBalancer(ctx, createConfig)

cloud/services/loadbalancers_test.go

@@ -162,11 +162,6 @@ func TestEnsureNodeBalancer(t *testing.T) {
 					return nil
 				})
 
-				// Mock GetFirewall call
-				mockClient.EXPECT().GetFirewall(gomock.Any(), 5678).Return(&linodego.Firewall{
-					ID: 5678,
-				}, nil)
-
 				// Mock CreateNodeBalancer call
 				mockClient.EXPECT().CreateNodeBalancer(
 					gomock.Any(),

docs/src/topics/firewalling.md

@@ -195,16 +195,27 @@ When configuring firewalls, you can specify either a direct `firewallID` or a `f
 
 For `LinodeMachine` resources, when both `firewallID` and `firewallRef` are specified:
 
-- `firewallRef` takes precedence over `firewallID`
-- The ID from the referenced `LinodeFirewall` will be used instead of the directly specified `firewallID`
+- `firewallID` takes precedence over `firewallRef`
+- The directly specified `firewallID` will be used instead of the referenced `LinodeFirewall`
 
 #### LinodeCluster NodeBalancer Firewall Precedence
 
 For `LinodeCluster` resources, when both `NodeBalancerFirewallID` and `NodeBalancerFirewallRef` are specified:
 
-- `NodeBalancerFirewallRef` takes precedence over `NodeBalancerFirewallID`
-- The ID from the referenced `LinodeFirewall` will be used instead of the directly specified `NodeBalancerFirewallID`
+- `NodeBalancerFirewallID` takes precedence over `NodeBalancerFirewallRef`
+- The directly specified `NodeBalancerFirewallID` will be used instead of the referenced `LinodeFirewall`
 
 ```admonish warning
-While you can specify both direct IDs and references, it's recommended to use only one approach for clarity and to avoid confusion.
+While describing the precedence rules above, please note that specifying both direct IDs and references in the same resource is not recommended and will be rejected by the webhook validator. You should use either a direct ID or a reference, but not both.
+```
+
+```admonish note title="Migration Note for Existing Clusters"
+In previous versions, the behavior was reversed - references took precedence over direct IDs, and the resolved ID from a reference was stored back in the direct ID field. 
+
+If you have existing clusters that were created with references, you may need to:
+1. Clear the direct ID field (`firewallID` or `NodeBalancerFirewallID`)
+2. Keep only the reference field (`firewallRef` or `NodeBalancerFirewallRef`)
+3. Allow the cluster to reconcile with the new behavior
+
+This ensures that changes to your references will be properly respected.
 ```

internal/controller/linodecluster_controller.go

@@ -248,6 +248,7 @@ func (r *LinodeClusterReconciler) reconcilePreflightLinodeFirewallCheck(ctx cont
 	// 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)

internal/controller/linodecluster_controller_test.go

@@ -190,10 +190,6 @@ var _ = Describe("cluster-lifecycle", Ordered, Label("cluster", "cluster-lifecyc
 					if cScope.LinodeCluster.Spec.NodeBalancerFirewallRef != nil {
 						linodeFirewall.Spec.FirewallID = util.Pointer(123)
 						k8sClient.Update(ctx, &linodeFirewall)
-
-						// Mock GetFirewall call
-						mck.LinodeClient.EXPECT().GetFirewall(gomock.Any(), gomock.Any()).
-							Return(&linodego.Firewall{ID: 123}, nil)
 					}
 
 					// If using direct firewall ID
@@ -224,10 +220,6 @@ var _ = Describe("cluster-lifecycle", Ordered, Label("cluster", "cluster-lifecyc
 				Call("cluster is not created because nb was nil", func(ctx context.Context, mck Mock) {
 					cScope.LinodeClient = mck.LinodeClient
 
-					// Mock GetFirewall call
-					mck.LinodeClient.EXPECT().GetFirewall(gomock.Any(), gomock.Any()).
-						Return(&linodego.Firewall{ID: 123}, nil)
-
 					// Mock CreateNodeBalancer to return nil
 					mck.LinodeClient.EXPECT().CreateNodeBalancer(gomock.Any(), gomock.Any()).
 						Return(nil, nil)
@@ -246,10 +238,6 @@ var _ = Describe("cluster-lifecycle", Ordered, Label("cluster", "cluster-lifecyc
 				Call("cluster is not created because nb config was nil", func(ctx context.Context, mck Mock) {
 					cScope.LinodeClient = mck.LinodeClient
 
-					// Mock GetFirewall call
-					mck.LinodeClient.EXPECT().GetFirewall(gomock.Any(), gomock.Any()).
-						Return(&linodego.Firewall{ID: 123}, nil)
-
 					// Mock CreateNodeBalancerConfig to return nil
 					mck.LinodeClient.EXPECT().CreateNodeBalancerConfig(gomock.Any(), gomock.Any(), gomock.Any()).
 						Return(nil, errors.New("nodeBalancer config created was nil"))

internal/controller/linodemachine_controller.go

@@ -442,6 +442,28 @@ func (r *LinodeMachineReconciler) reconcilePreflightVPC(ctx context.Context, log
 }
 
 func (r *LinodeMachineReconciler) reconcilePreflightLinodeFirewallCheck(ctx context.Context, logger logr.Logger, machineScope *scope.MachineScope) (ctrl.Result, error) {
+	// First check if a direct FirewallID is specified
+	if machineScope.LinodeMachine.Spec.FirewallID != 0 {
+		logger.Info("Verifying direct FirewallID", "firewallID", machineScope.LinodeMachine.Spec.FirewallID)
+		_, err := machineScope.LinodeClient.GetFirewall(ctx, machineScope.LinodeMachine.Spec.FirewallID)
+		if err != nil {
+			logger.Error(err, "Failed to get firewall with provided ID", "firewallID", machineScope.LinodeMachine.Spec.FirewallID)
+			conditions.Set(machineScope.LinodeMachine, metav1.Condition{
+				Type:    ConditionPreflightLinodeFirewallReady,
+				Status:  metav1.ConditionFalse,
+				Reason:  util.CreateError,
+				Message: err.Error(),
+			})
+			return ctrl.Result{RequeueAfter: reconciler.DefaultClusterControllerReconcileDelay}, nil
+		}
+		conditions.Set(machineScope.LinodeMachine, metav1.Condition{
+			Type:   ConditionPreflightLinodeFirewallReady,
+			Status: metav1.ConditionTrue,
+			Reason: "LinodeFirewallReady",
+		})
+		return ctrl.Result{}, nil
+	}
+
 	// If NodeBalancerFirewallID is directly specified, check if it exists
 	if machineScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID != nil {
 		firewallID := *machineScope.LinodeCluster.Spec.Network.NodeBalancerFirewallID

internal/controller/linodemachine_controller_helpers.go

@@ -141,7 +141,7 @@ func newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, gzip
 	}
 
 	// Configure firewall if needed
-	if machineScope.LinodeMachine.Spec.FirewallRef != nil {
+	if machineScope.LinodeMachine.Spec.FirewallRef != nil || machineScope.LinodeMachine.Spec.FirewallID != 0 {
 		if err := configureFirewall(ctx, machineScope, createConfig, logger); err != nil {
 			return nil, err
 		}
@@ -995,13 +995,21 @@ func configurePlacementGroup(ctx context.Context, machineScope *scope.MachineSco
 
 // configureFirewall adds firewall configuration
 func configureFirewall(ctx context.Context, machineScope *scope.MachineScope, createConfig *linodego.InstanceCreateOptions, logger logr.Logger) error {
+	// First check if a direct FirewallID is specified
+	if machineScope.LinodeMachine.Spec.FirewallID != 0 {
+		// Direct FirewallID is provided, use it
+		logger.Info("Using direct FirewallID", "firewallID", machineScope.LinodeMachine.Spec.FirewallID)
+		createConfig.FirewallID = machineScope.LinodeMachine.Spec.FirewallID
+		return nil
+	}
+
+	// If no direct FirewallID, use FirewallRef
 	fwID, err := getFirewallID(ctx, machineScope, logger)
 	if err != nil {
-		logger.Error(err, "Failed to get Firewall config")
+		logger.Error(err, "Failed to get Firewall config from reference")
 		return err
 	}
 
 	createConfig.FirewallID = fwID
-
 	return nil
 }

internal/webhook/v1alpha2/linodecluster_webhook.go

@@ -141,6 +141,14 @@ func (r *linodeClusterValidator) validateLinodeClusterSpec(ctx context.Context,
 		})
 	}
 
+	if spec.Network.NodeBalancerFirewallID != nil && spec.NodeBalancerFirewallRef != nil {
+		errs = append(errs, &field.Error{
+			Field:  "spec.network.nodeBalancerFirewallID/spec.nodeBalancerFirewallRef",
+			Type:   field.ErrorTypeInvalid,
+			Detail: "Cannot specify both NodeBalancerFirewallID and NodeBalancerFirewallRef",
+		})
+	}
+
 	if len(errs) == 0 {
 		return nil
 	}

internal/webhook/v1alpha2/linodecluster_webhook_test.go

@@ -382,3 +382,91 @@ func TestValidateVPCIDAndVPCRef(t *testing.T) {
 		),
 	)
 }
+
+func TestValidateNodeBalancerFirewallIDAndNodeBalancerFirewallRef(t *testing.T) {
+	t.Parallel()
+
+	var (
+		invalidCluster = &infrav1alpha2.LinodeCluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "example",
+				Namespace: "example",
+			},
+			Spec: infrav1alpha2.LinodeClusterSpec{
+				Region: "us-ord",
+				Network: infrav1alpha2.NetworkSpec{
+					NodeBalancerFirewallID: ptr.To(5678),
+				},
+				NodeBalancerFirewallRef: &corev1.ObjectReference{
+					Namespace: "example",
+					Name:      "example",
+					Kind:      "LinodeFirewall",
+				},
+			},
+		}
+		validClusterWithFirewallID = &infrav1alpha2.LinodeCluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "example",
+				Namespace: "example",
+			},
+			Spec: infrav1alpha2.LinodeClusterSpec{
+				Region: "us-ord",
+				Network: infrav1alpha2.NetworkSpec{
+					NodeBalancerFirewallID: ptr.To(5678),
+				},
+			},
+		}
+		validClusterWithFirewallRef = &infrav1alpha2.LinodeCluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "example",
+				Namespace: "example",
+			},
+			Spec: infrav1alpha2.LinodeClusterSpec{
+				Region: "us-ord",
+				NodeBalancerFirewallRef: &corev1.ObjectReference{
+					Namespace: "example",
+					Name:      "example",
+					Kind:      "LinodeFirewall",
+				},
+			},
+		}
+		validator = &linodeClusterValidator{}
+	)
+
+	NewSuite(t, mock.MockLinodeClient{}).Run(
+		OneOf(
+			Path(
+				Call("valid with NodeBalancerFirewallID", func(ctx context.Context, mck Mock) {
+					mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
+				}),
+				Result("success", func(ctx context.Context, mck Mock) {
+					errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, validClusterWithFirewallID.Spec, SkipAPIValidation)
+					require.Empty(t, errs)
+				}),
+			),
+		),
+		OneOf(
+			Path(
+				Call("valid with NodeBalancerFirewallRef", func(ctx context.Context, mck Mock) {
+					mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
+				}),
+				Result("success", func(ctx context.Context, mck Mock) {
+					errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, validClusterWithFirewallRef.Spec, SkipAPIValidation)
+					require.Empty(t, errs)
+				}),
+			),
+		),
+		OneOf(
+			Path(
+				Call("both NodeBalancerFirewallID and NodeBalancerFirewallRef set", func(ctx context.Context, mck Mock) {
+					mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
+				}),
+				Result("error", func(ctx context.Context, mck Mock) {
+					errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, invalidCluster.Spec, SkipAPIValidation)
+					require.NotEmpty(t, errs)
+					require.Contains(t, errs[0].Error(), "Cannot specify both NodeBalancerFirewallID and NodeBalancerFirewallRef")
+				}),
+			),
+		),
+	)
+}

internal/webhook/v1alpha2/linodemachine_webhook.go

@@ -150,6 +150,14 @@ func (r *linodeMachineValidator) validateLinodeMachineSpec(ctx context.Context,
 		})
 	}
 
+	if spec.FirewallID != 0 && spec.FirewallRef != nil {
+		errs = append(errs, &field.Error{
+			Field:  "spec.firewallID/spec.firewallRef",
+			Type:   field.ErrorTypeInvalid,
+			Detail: "Cannot specify both FirewallID and FirewallRef",
+		})
+	}
+
 	if len(errs) == 0 {
 		return nil
 	}