From a337a2a18e98eafc07196e2c5953845f3f344b5b Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Mon, 12 Sep 2022 14:26:51 +0000 Subject: [PATCH] Add L4Firewall to namer, instead of using L4Backend --- pkg/loadbalancers/l4.go | 10 ++++++---- pkg/loadbalancers/l4_test.go | 19 ++++++++++--------- pkg/utils/namer/interfaces.go | 2 ++ pkg/utils/namer/l4_namer.go | 11 +++++++++++ pkg/utils/namer/l4_namer_test.go | 13 +++++++++++-- 5 files changed, 40 insertions(+), 15 deletions(-) diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index 697b39c559..fe16d7d60a 100644 --- a/pkg/loadbalancers/l4.go +++ b/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 diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 8ceef1bf01..b5f1ecdd43 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/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) } } diff --git a/pkg/utils/namer/interfaces.go b/pkg/utils/namer/interfaces.go index 8056e6038b..51a294ed0d 100644 --- a/pkg/utils/namer/interfaces.go +++ b/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 diff --git a/pkg/utils/namer/l4_namer.go b/pkg/utils/namer/l4_namer.go index 6cbe8fbfa6..63d2a4af03 100644 --- a/pkg/utils/namer/l4_namer.go +++ b/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: // diff --git a/pkg/utils/namer/l4_namer_test.go b/pkg/utils/namer/l4_namer_test.go index e16ff4186e..d1bf0e3c95 100644 --- a/pkg/utils/namer/l4_namer_test.go +++ b/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,10 +75,11 @@ 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) @@ -81,6 +87,9 @@ func TestL4Namer(t *testing.T) { 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) }