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

cloudbuild.yaml

@@ -4,7 +4,7 @@ options:
   substitution_option: ALLOW_LOOSE
   machineType: E2_HIGHCPU_8
 steps:
-- name: gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20230206-8160eea68e
+- name: gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250513-9264efb079
   id: push-images
   entrypoint: bash
   env:
@@ -20,7 +20,7 @@ steps:
     # Build and push every image except e2e-test 
     make all-push ALL_ARCH="amd64 arm64" \
       CONTAINER_BINARIES="glbc 404-server 404-server-with-metrics echo fuzzer"
-- name: gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20230206-8160eea68e
+- name: gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250513-9264efb079
   id: push-e2e-test-image
   entrypoint: bash
   env:

cmd/e2e-test/logging_test.go

@@ -170,9 +170,11 @@ func TestLogging(t *testing.T) {
 				gclb, err = fuzz.GCLBForVIP(context.Background(), Framework.Cloud, params)
 				if err != nil {
 					t.Logf("Failed to GCP resources for LB with IP = %q: %v", vip, err)
+					err = fmt.Errorf("failed to GCP resources for LB with IP = %q: %v", vip, err)
 					return false, nil
 				}
-				if err := verifyLogging(t, gclb, s.Namespace, svcName, tc.transition); err != nil {
+				if err = verifyLogging(t, gclb, s.Namespace, svcName, tc.transition); err != nil {
+					err = fmt.Errorf("failed to verify logging for LB with IP = %q: %v", vip, err)
 					return false, nil
 				}
 				return true, nil

pkg/address/manager.go

@@ -213,7 +213,7 @@ func (m *Manager) ensureAddressReservation() (string, IPAddressType, error) {
 		return addr.Address, IPAddrManaged, nil
 	}
 
-	m.frLogger.V(2).Info("Address reserve error", "err", reserveErr)
+	m.frLogger.V(2).Info("Unable to reserve address (could be expected). Checking for conflict", "err", reserveErr)
 
 	if utils.IsNetworkTierMismatchGCEError(reserveErr) {
 		receivedNetworkTier := cloud.NetworkTierPremium

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
@@ -169,6 +180,15 @@ func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, enforced b
 		}
 		protocolPorts = append(protocolPorts, protocolPort)
 	}
+
+	// TCP:all is the default if the ports' list is empty.
+	if len(protocolPorts) == 0 {
+		protocolPort := gcpfirewallv1.ProtocolPort{
+			Protocol: gcpfirewallv1.ProtocolTCP,
+		}
+		protocolPorts = append(protocolPorts, protocolPort)
+	}
+
 	firewallCR.Spec.Ports = protocolPorts
 
 	var src_cidrs, dst_cidrs []gcpfirewallv1.CIDR

pkg/firewalls/firewalls_test.go

@@ -131,13 +131,30 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) {
 func TestFirewallPoolSyncPorts(t *testing.T) {
 	fwp := NewFakeFirewallsProvider(false, false)
 	fwClient := firewallclient.NewSimpleClientset()
-	fp := NewFirewallPool(fwp, defaultNamer, srcRanges, portRanges(), klog.TODO())
-	fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), true, klog.TODO())
 	nodes := []string{"node-a", "node-b", "node-c"}
+	emptyPortRanges := make([]string, 0)
+
+	// Verify empty ports' list
+	fp := NewFirewallPool(fwp, defaultNamer, srcRanges, emptyPortRanges, klog.TODO())
+	fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, emptyPortRanges, true, klog.TODO())
 
 	if err := fp.Sync(nodes, nil, nil, true); err != nil {
 		t.Fatal(err)
 	}
+	verifyFirewallRule(fwp, ruleName, nodes, srcRanges, emptyPortRanges, t)
+
+	if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
+		t.Fatal(err)
+	}
+	verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, t)
+
+	// Verify a preset ports' list
+	fp = NewFirewallPool(fwp, defaultNamer, srcRanges, portRanges(), klog.TODO())
+	fcrp = NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), true, klog.TODO())
+
+	if err := fp.Sync(nodes, nil, nil, true); err != nil {
+		t.Errorf("unexpected err when syncing firewall, err: %v", err)
+	}
 	verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t)
 
 	if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
@@ -399,14 +416,20 @@ func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string,
 	ports := sets.NewString(expectedPorts...)
 	srcranges := sets.NewString(sourceRanges...)
 
+	// Empty ports' list would mean that all protocols are permitted
+	// (not only TCP)
+	if len(actualFW.Spec.Ports) == 0 {
+		t.Errorf("Empty list of allowed protocols is not permited")
+	}
+
 	actualPorts := sets.NewString()
 	for _, protocolports := range actualFW.Spec.Ports {
 		if protocolports.Protocol != "TCP" {
 			t.Errorf("Protocol isn't TCP")
 		}
 		if protocolports.EndPort != nil {
 			actualPorts.Insert(fmt.Sprintf("%d-%d", *protocolports.StartPort, *protocolports.EndPort))
-		} else {
+		} else if protocolports.StartPort != nil {
 			actualPorts.Insert(fmt.Sprintf("%d", *protocolports.StartPort))
 		}
 

pkg/forwardingrules/optimized/compare_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"k8s.io/ingress-gce/pkg/composite"
 	"k8s.io/ingress-gce/pkg/forwardingrules"
 	"k8s.io/ingress-gce/pkg/forwardingrules/optimized"
@@ -198,7 +199,11 @@ func TestCompare(t *testing.T) {
 				t.Fatalf("optimized.Compare() error = %v", err)
 			}
 
-			if diff := cmp.Diff(ops, tC.wantCalls); diff != "" {
+			// We execute those in parallel so we don't care about the order at each stage
+			sortFROpt := cmpopts.SortSlices(func(a, b *composite.ForwardingRule) bool { return a.Name < b.Name })
+			sortDeletesOpt := cmpopts.SortSlices(func(a, b string) bool { return a < b })
+
+			if diff := cmp.Diff(ops, tC.wantCalls, sortFROpt, sortDeletesOpt); diff != "" {
 				t.Errorf("want != got, (-want, +got):\n%s", diff)
 			}
 		})

pkg/forwardingrules/optimized/fetch_test.go

@@ -89,7 +89,9 @@ func TestFetch(t *testing.T) {
 			}
 
 			ignoreOpts := cmpopts.IgnoreFields(composite.ForwardingRule{}, "Version", "SelfLink", "Region")
-			if diff := cmp.Diff(tC.want, got, ignoreOpts); diff != "" {
+			sortOpt := cmpopts.SortSlices(func(a, b *composite.ForwardingRule) bool { return a.Name < b.Name })
+
+			if diff := cmp.Diff(tC.want, got, ignoreOpts, sortOpt); diff != "" {
 				t.Errorf("Fetch(%q) returned diff (-want +got):\n%s", tC.link, diff)
 			}
 		})

pkg/neg/controller.go

@@ -205,7 +205,9 @@ func NewController(
 			podInformer.GetIndexer(),
 			cloud,
 			manager,
+			zoneGetter,
 			enableDualStackNEG,
+			flags.F.EnableMultiSubnetCluster && !flags.F.EnableMultiSubnetClusterPhase1,
 			logger,
 		)
 	} else {

pkg/neg/readiness/poller_test.go

@@ -68,7 +68,7 @@ func (p *testPatcher) Eval(t *testing.T, pod string, negKey, bsKey *meta.Key) {
 }
 
 func newFakePoller() (*poller, error) {
-	reflector, err := newTestReadinessReflector(negtypes.NewTestContext())
+	reflector, err := newTestReadinessReflector(negtypes.NewTestContext(), false)
 	if err != nil {
 		return nil, fmt.Errorf("failed to initialize reflector: %s", err)
 	}