Merge pull request #1806 from panslava/add-L4Firewall-to-namer

Add L4Firewall to namer, instead of using L4Backend

pkg/loadbalancers/l4.go

@@ -125,9 +125,10 @@ func (l4 *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) *L4ILBSyncR
 	}
 
 	// delete firewall rule allowing load balancer source ranges
-	err = l4.deleteFirewall(name)
+	firewallName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name)
+	err = l4.deleteFirewall(firewallName)
 	if err != nil {
-		klog.Errorf("Failed to delete firewall rule %s for internal loadbalancer service %s, err %v", name, l4.NamespacedName.String(), err)
+		klog.Errorf("Failed to delete firewall rule %s for internal loadbalancer service %s, err %v", firewallName, l4.NamespacedName.String(), err)
 		result.GCEResourceInError = annotations.FirewallRuleResource
 		result.Error = err
 	}
@@ -267,12 +268,13 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service
 		return result
 	}
 	// Add firewall rule for ILB traffic to nodes
+	firewallName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name)
 	nodesFWRParams := firewalls.FirewallParams{
 		PortRanges:        portRanges,
 		SourceRanges:      sourceRanges.StringSlice(),
 		DestinationRanges: []string{fr.IPAddress},
 		Protocol:          string(protocol),
-		Name:              name,
+		Name:              firewallName,
 		NodeNames:         nodeNames,
 		L4Type:            utils.ILB,
 	}
@@ -282,7 +284,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service
 		result.Error = err
 		return result
 	}
-	result.Annotations[annotations.FirewallRuleKey] = name
+	result.Annotations[annotations.FirewallRuleKey] = firewallName
 	result.Annotations[annotations.FirewallRuleForHealthcheckKey] = hcResult.HCFirewallRuleName
 
 	result.MetricsState.InSuccess = true

pkg/loadbalancers/l4_test.go

@@ -1513,7 +1513,7 @@ func verifyForwardingRule(l4 *L4, backendServiceLink string) error {
 }
 
 func verifyNodesFirewall(l4 *L4, nodeNames []string) error {
-	fwName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name)
+	fwName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name)
 	fwDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4.Service.Namespace, l4.Service.Name), "", meta.VersionGA, false, utils.ILB)
 	if err != nil {
 		return fmt.Errorf("failed to create description for resources, err %w", err)
@@ -1564,9 +1564,10 @@ func buildExpectedAnnotations(l4 *L4) map[string]string {
 	}
 
 	hcFwName := l4.namer.L4HealthCheckFirewall(l4.Service.Namespace, l4.Service.Name, isSharedHC)
-
 	expectedAnnotations[annotations.FirewallRuleForHealthcheckKey] = hcFwName
-	expectedAnnotations[annotations.FirewallRuleKey] = backendName
+
+	fwName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name)
+	expectedAnnotations[annotations.FirewallRuleKey] = fwName
 
 	frName := l4.GetFRName()
 	if proto == v1.ProtocolTCP {
@@ -1580,12 +1581,12 @@ func buildExpectedAnnotations(l4 *L4) map[string]string {
 func assertILBResourcesDeleted(t *testing.T, l4 *L4) {
 	t.Helper()
 
-	resourceName := l4.namer.L4Backend(l4.Service.Namespace, l4.Service.Name)
+	nodesFwName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name)
 	hcFwNameShared := l4.namer.L4HealthCheckFirewall(l4.Service.Namespace, l4.Service.Name, true)
 	hcFwNameNonShared := l4.namer.L4HealthCheckFirewall(l4.Service.Namespace, l4.Service.Name, false)
 
 	fwNames := []string{
-		resourceName,
+		nodesFwName,
 		hcFwNameShared,
 		hcFwNameNonShared,
 	}
@@ -1615,14 +1616,14 @@ func assertILBResourcesDeleted(t *testing.T, l4 *L4) {
 		t.Errorf("verifyHealthCheckNotExists(_, %s)", hcNameNonShared)
 	}
 
-	err = verifyBackendServiceNotExists(l4, resourceName)
+	err = verifyBackendServiceNotExists(l4, nodesFwName)
 	if err != nil {
-		t.Errorf("verifyBackendServiceNotExists(_, %s)", resourceName)
+		t.Errorf("verifyBackendServiceNotExists(_, %s)", nodesFwName)
 	}
 
-	err = verifyAddressNotExists(l4, resourceName)
+	err = verifyAddressNotExists(l4, nodesFwName)
 	if err != nil {
-		t.Errorf("verifyAddressNotExists(_, %s)", resourceName)
+		t.Errorf("verifyAddressNotExists(_, %s)", nodesFwName)
 	}
 }
 

pkg/utils/namer/interfaces.go

@@ -87,6 +87,8 @@ type L4ResourcesNamer interface {
 	BackendNamer
 	// L4ForwardingRule returns the name of the forwarding rule for the given service and protocol.
 	L4ForwardingRule(namespace, name, protocol string) string
+	// L4Firewall returns the name of the firewall rule for the given service
+	L4Firewall(namespace, name string) string
 	// L4HealthCheck returns the names of the Healthcheck
 	L4HealthCheck(namespace, name string, shared bool) string
 	// L4HealthCheckFirewall returns the names of the Healthcheck Firewall

pkg/utils/namer/l4_namer.go

@@ -60,6 +60,17 @@ func (namer *L4Namer) L4Backend(namespace, name string) string {
 	}, "-")
 }
 
+// L4Firewall returns the gce Firewall name based on the service namespace and name
+// Naming convention:
+//
+//	k8s2-{uid}-{ns}-{name}-{suffix}
+//
+// Output name is at most 63 characters.
+// This name is identical to L4Backend.
+func (namer *L4Namer) L4Firewall(namespace, name string) string {
+	return namer.L4Backend(namespace, name)
+}
+
 // L4ForwardingRule returns the name of the L4 forwarding rule name based on the service namespace, name and protocol.
 // Naming convention:
 //

pkg/utils/namer/l4_namer_test.go

@@ -17,6 +17,7 @@ func TestL4Namer(t *testing.T) {
 		sharedHC       bool
 		expectFRName   string
 		expectNEGName  string
+		expectFWName   string
 		expectHcFwName string
 		expectHcName   string
 	}{
@@ -28,6 +29,7 @@ func TestL4Namer(t *testing.T) {
 			false,
 			"k8s2-tcp-7kpbhpki-namespace-name-956p2p7x",
 			"k8s2-7kpbhpki-namespace-name-956p2p7x",
+			"k8s2-7kpbhpki-namespace-name-956p2p7x",
 			"k8s2-7kpbhpki-namespace-name-956p2p7x-fw",
 			"k8s2-7kpbhpki-namespace-name-956p2p7x",
 		},
@@ -39,6 +41,7 @@ func TestL4Namer(t *testing.T) {
 			true,
 			"k8s2-tcp-7kpbhpki-namespace-name-956p2p7x",
 			"k8s2-7kpbhpki-namespace-name-956p2p7x",
+			"k8s2-7kpbhpki-namespace-name-956p2p7x",
 			"k8s2-7kpbhpki-l4-shared-hc-fw",
 			"k8s2-7kpbhpki-l4-shared-hc",
 		},
@@ -50,6 +53,7 @@ func TestL4Namer(t *testing.T) {
 			false,
 			"k8s2-udp-7kpbhpki-012345678901234567-01234567890123456-hwm400mg",
 			"k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg",
+			"k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg",
 			"k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm40-fw",
 			"k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg",
 		},
@@ -61,6 +65,7 @@ func TestL4Namer(t *testing.T) {
 			true,
 			"k8s2-udp-7kpbhpki-012345678901234567-01234567890123456-hwm400mg",
 			"k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg",
+			"k8s2-7kpbhpki-01234567890123456789-0123456789012345678-hwm400mg",
 			"k8s2-7kpbhpki-l4-shared-hc-fw",
 			"k8s2-7kpbhpki-l4-shared-hc",
 		},
@@ -70,17 +75,21 @@ func TestL4Namer(t *testing.T) {
 	for _, tc := range testCases {
 		frName := newNamer.L4ForwardingRule(tc.namespace, tc.name, strings.ToLower(tc.proto))
 		negName := newNamer.L4Backend(tc.namespace, tc.name)
+		fwName := newNamer.L4Firewall(tc.namespace, tc.name)
 		hcName := newNamer.L4HealthCheck(tc.namespace, tc.name, tc.sharedHC)
 		hcFwName := newNamer.L4HealthCheckFirewall(tc.namespace, tc.name, tc.sharedHC)
-		if len(frName) > maxResourceNameLength || len(negName) > maxResourceNameLength || len(hcName) > maxResourceNameLength || len(hcFwName) > maxResourceNameLength {
-			t.Errorf("%s: got len(frName) == %v, len(negName) == %v, len(hcName) == %v, len(hcFwName) == %v want <= 63", tc.desc, len(frName), len(negName), len(hcName), len(hcFwName))
+		if len(frName) > maxResourceNameLength || len(negName) > maxResourceNameLength || len(fwName) > maxResourceNameLength || len(hcName) > maxResourceNameLength || len(hcFwName) > maxResourceNameLength {
+			t.Errorf("%s: got len(frName) == %v, len(negName) == %v, len(fwName) == %v, len(hcName) == %v, len(hcFwName) == %v want <= 63", tc.desc, len(frName), len(negName), len(fwName), len(hcName), len(hcFwName))
 		}
 		if frName != tc.expectFRName {
 			t.Errorf("%s ForwardingRuleName: got %q, want %q", tc.desc, frName, tc.expectFRName)
 		}
 		if negName != tc.expectNEGName {
 			t.Errorf("%s VMIPNEGName: got %q, want %q", tc.desc, negName, tc.expectFRName)
 		}
+		if fwName != tc.expectFWName {
+			t.Errorf("%s FirewallName: got %q, want %q", tc.desc, fwName, tc.expectFWName)
+		}
 		if hcFwName != tc.expectHcFwName {
 			t.Errorf("%s FirewallName For Healthcheck: got %q, want %q", tc.desc, hcFwName, tc.expectHcFwName)
 		}