Make sure that during gcpfirewalls cr creation, sync errors are not returned in Shadow run (when l7LB still reconciles the FWs).

Also fix dryRun flag to properly mean "testing" flow (the logic of the flag usage has not been changed)

Author: maciejriedl

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 {

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
 }
@@ -81,17 +84,17 @@ func (fr *FirewallCR) Sync(nodeNames, additionalPorts, additionalRanges []string
 	ranges.Insert(additionalRanges...)
 
 	fr.logger.V(3).Info("Firewall CR is enabled.")
-	expectedFirewallCR, err := NewFirewallCR(name, ports.List(), ranges.UnsortedList(), []string{}, fr.dryRun)
+	expectedFirewallCR, err := NewFirewallCR(name, ports.List(), ranges.UnsortedList(), []string{}, !fr.dryRun)
 	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