diff --git a/pkg/firewalls/controller.go b/pkg/firewalls/controller.go index 589c6ac149..81bc18b321 100644 --- a/pkg/firewalls/controller.go +++ b/pkg/firewalls/controller.go @@ -106,7 +106,7 @@ func NewFirewallController( sourceRanges := lbSourceRanges(logger, flags.F.OverrideHealthCheckSourceCIDRs) logger.Info("Using source ranges", "ranges", sourceRanges) if enableCR { - firewallCRPool := NewFirewallCRPool(ctx.FirewallClient, ctx.Cloud, ctx.ClusterNamer, sourceRanges, portRanges, disableFWEnforcement, logger) + firewallCRPool := NewFirewallCRPool(ctx.FirewallClient, ctx.Cloud, ctx.ClusterNamer, sourceRanges, portRanges, !disableFWEnforcement, logger) compositeFirewallPool.pools = append(compositeFirewallPool.pools, firewallCRPool) } if !disableFWEnforcement { diff --git a/pkg/firewalls/firewalls_l7_cr.go b/pkg/firewalls/firewalls_l7_cr.go index 85d0ef3352..2c81e212f9 100644 --- a/pkg/firewalls/firewalls_l7_cr.go +++ b/pkg/firewalls/firewalls_l7_cr.go @@ -41,7 +41,10 @@ type FirewallCR struct { // all the port ranges to open with each call to Sync() nodePortRanges []string firewallClient firewallclient.Interface - dryRun bool + // If set to true, CRs are marked as "disabled" and errors are not + // propagated. Firewalls should be still created/updated by L7LB, not the + // Platform Firewall + dryRun bool logger klog.Logger } @@ -85,13 +88,13 @@ func (fr *FirewallCR) Sync(nodeNames, additionalPorts, additionalRanges []string if err != nil { return err } - return ensureFirewallCR(fr.firewallClient, expectedFirewallCR, fr.logger) + return ensureFirewallCR(fr.firewallClient, expectedFirewallCR, fr.logger, fr.dryRun) } // ensureFirewallCR creates/updates the firewall CR // On CR update, it will read the conditions to see if there are errors updated by PFW controller. // If the Spec was updated by others, it will reconcile the Spec. -func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewallv1.GCPFirewall, logger klog.Logger) error { +func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewallv1.GCPFirewall, logger klog.Logger, dryRun bool) error { fw := client.NetworkingV1().GCPFirewalls() currentFWCR, err := fw.Get(context.Background(), expectedFWCR.Name, metav1.GetOptions{}) logger.V(3).Info("ensureFirewallCR Get CR", "currentFirewallCR", fmt.Sprintf("%+v", currentFWCR), "err", err) @@ -103,6 +106,10 @@ func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewall } return err } + if currentFWCR.DeletionTimestamp != nil { + logger.V(3).Info("ensureFirewallCR: The CR contains DeletionTimestamp. Skipping Ensure", "currentFirewallDeletionTimestamp", fmt.Sprintf("%+v", currentFWCR.DeletionTimestamp)) + return nil + } if !reflect.DeepEqual(currentFWCR.Spec, expectedFWCR.Spec) { // Update the current firewall CR logger.V(3).Info("ensureFirewallCR Update CR", "currentFirewallCR", fmt.Sprintf("%+v", currentFWCR.Spec), "expectedFirewallCR", fmt.Sprintf("%+v", expectedFWCR.Spec)) @@ -116,7 +123,11 @@ func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewall con.Reason == string(gcpfirewallv1.FirewallRuleReasonSyncError) { // Use recorder to emit the cmd in Sync() logger.V(3).Info("ensureFirewallCR: Could not enforce Firewall CR", "currentFirewallCRName", currentFWCR.Name, "reason", con.Reason) - return fmt.Errorf(con.Reason) + if dryRun { + return nil + } else { + return fmt.Errorf(con.Reason) + } } } return nil @@ -130,14 +141,14 @@ func deleteFirewallCR(client firewallclient.Interface, name string, logger klog. } // NewFirewallCR constructs the firewall CR from name, ports and ranges -func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, enforced bool) (*gcpfirewallv1.GCPFirewall, error) { +func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, dryRun bool) (*gcpfirewallv1.GCPFirewall, error) { firewallCR := &gcpfirewallv1.GCPFirewall{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, Spec: gcpfirewallv1.GCPFirewallSpec{ Action: gcpfirewallv1.ActionAllow, - Disabled: !enforced, + Disabled: dryRun, }, } var protocolPorts []gcpfirewallv1.ProtocolPort diff --git a/pkg/firewalls/firewalls_test.go b/pkg/firewalls/firewalls_test.go index 89306a1100..df90b78eef 100644 --- a/pkg/firewalls/firewalls_test.go +++ b/pkg/firewalls/firewalls_test.go @@ -23,6 +23,7 @@ import ( "strings" "testing" + gcpfirewallv1 "github.com/GoogleCloudPlatform/gke-networking-api/apis/gcpfirewall/v1" "k8s.io/klog/v2" firewallclient "github.com/GoogleCloudPlatform/gke-networking-api/client/gcpfirewall/clientset/versioned/fake" @@ -58,7 +59,7 @@ func TestFirewallPoolSync(t *testing.T) { if err := fcrp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) } @@ -76,7 +77,7 @@ func TestFirewallPoolSyncNodes(t *testing.T) { if err := fcrp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) // Add nodes nodes = append(nodes, "node-d", "node-e") @@ -84,7 +85,7 @@ func TestFirewallPoolSyncNodes(t *testing.T) { t.Errorf("unexpected err when syncing firewall, err: %v", err) } verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t) - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) // Remove nodes nodes = []string{"node-a", "node-c"} @@ -92,7 +93,7 @@ func TestFirewallPoolSyncNodes(t *testing.T) { t.Errorf("unexpected err when syncing firewall, err: %v", err) } verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t) - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) } func TestFirewallPoolSyncSrcRanges(t *testing.T) { @@ -112,7 +113,7 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) // Manually modify source ranges to bad values. f, _ := fwp.GetFirewall(ruleName) @@ -125,7 +126,7 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) { t.Errorf("unexpected err when syncing firewall, err: %v", err) } verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t) - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) } func TestFirewallPoolSyncPorts(t *testing.T) { @@ -146,7 +147,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) { if err := fcrp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, true, t) // Verify a preset ports' list fp = NewFirewallPool(fwp, defaultNamer, srcRanges, portRanges(), klog.TODO()) @@ -160,7 +161,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) { if err := fcrp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) // Manually modify port list to bad values. f, _ := fwp.GetFirewall(ruleName) @@ -178,7 +179,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) { if err := fcrp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) // Verify additional ports are included negTargetports := []string{"80", "443", "8080"} @@ -190,7 +191,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) { if err := fcrp.Sync(nodes, negTargetports, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, append(portRanges(), negTargetports...), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, append(portRanges(), negTargetports...), true, true, t) if err := fp.Sync(nodes, negTargetports, nil, false); err != nil { t.Errorf("unexpected err when syncing firewall, err: %v", err) @@ -200,7 +201,50 @@ func TestFirewallPoolSyncPorts(t *testing.T) { if err := fcrp.Sync(nodes, negTargetports, nil, false); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, negTargetports, true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, negTargetports, true, true, t) +} + +func TestFirewallCRSyncDryRun(t *testing.T) { + t.Parallel() + testCases := []struct { + desc string + dryRun bool + wantError bool + }{ + { + desc: "dry run", + dryRun: true, + wantError: false, + }, + { + desc: "full mode", + dryRun: false, + wantError: true, + }, + } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + fwp := NewFakeFirewallsProvider(false, false) + nodes := []string{"node-a", "node-b", "node-c"} + + fwClient := firewallclient.NewSimpleClientset() + fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), tc.dryRun, klog.TODO()) + // Create CR + if err := fcrp.Sync(nodes, nil, nil, true); err != nil { + t.Fatal(err) + } + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, tc.dryRun, t) + + markCRWithReconciliationError(fwClient, t) + + err := fcrp.Sync(nodes, nil, nil, true) + if (err != nil) != tc.wantError { + t.Errorf("Sync() = %v, want error %v", err, tc.wantError) + } + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, tc.dryRun, t) + }) + } + } func TestFirewallPoolSyncGetServerError(t *testing.T) { @@ -266,7 +310,7 @@ func TestFirewallPoolSyncRanges(t *testing.T) { if err := fcrp.Sync(nodes, nil, tc.additionalRanges, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, resultRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, resultRanges, portRanges(), true, true, t) }) } } @@ -285,7 +329,7 @@ func TestFirewallPoolGC(t *testing.T) { if err := fcrp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } - verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t) + verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t) if err := fp.GC(); err != nil { t.Fatal(err) @@ -394,7 +438,7 @@ func validateXPNError(err error, op string, t *testing.T) { } } -func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, sourceRanges, expectedPorts []string, crEnabled bool, t *testing.T) { +func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, sourceRanges, expectedPorts []string, crEnabled bool, dryRun bool, t *testing.T) { if !crEnabled { fw := firewallclient.NetworkingV1().GCPFirewalls() actualFW, _ := fw.Get(context.TODO(), ruleName, metav1.GetOptions{}) @@ -413,6 +457,11 @@ func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, if actualFW.Spec.Action != "ALLOW" { t.Errorf("Action isn't ALLOW") } + + if actualFW.Spec.Disabled != dryRun { + t.Errorf("Spec.Disabled should equal {%v}", dryRun) + } + ports := sets.NewString(expectedPorts...) srcranges := sets.NewString(sourceRanges...) @@ -472,3 +521,15 @@ func verifyFirewallRule(fwp *fakeFirewallsProvider, ruleName string, expectedNod t.Errorf("source CIDRs doesn't equal expected CIDRs. Actual: %v, Expected: %v", f.SourceRanges, sourceRanges) } } + +func markCRWithReconciliationError(firewallclient *firewallclient.Clientset, t *testing.T) { + fw, _ := firewallclient.NetworkingV1().GCPFirewalls().Get(context.TODO(), ruleName, metav1.GetOptions{}) + if fw == nil { + t.Errorf("firewallCR not found") + } + fw.Status.Conditions = []metav1.Condition{{Reason: string(gcpfirewallv1.FirewallRuleReasonSyncError)}} + _, err := firewallclient.NetworkingV1().GCPFirewalls().Update(context.TODO(), fw, metav1.UpdateOptions{}) + if err != nil { + t.Errorf("could not update firewall CR, err %v", err) + } +}