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 c40c8a292..4be75f781 100644 --- a/internal/controller/linodecluster_controller.go +++ b/internal/controller/linodecluster_controller.go @@ -108,7 +108,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) @@ -233,10 +232,14 @@ func (r *LinodeClusterReconciler) performPreflightChecks(ctx context.Context, lo } } - 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 @@ 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 { - 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 == "" { diff --git a/internal/controller/linodecluster_controller_test.go b/internal/controller/linodecluster_controller_test.go index 7fcd50a70..875a1e241 100644 --- a/internal/controller/linodecluster_controller_test.go +++ b/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 diff --git a/internal/controller/linodefirewall_controller_test.go b/internal/controller/linodefirewall_controller_test.go index c9fb59f1f..b4e3de5be 100644 --- a/internal/controller/linodefirewall_controller_test.go +++ b/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,