From 6c9120ac2f789f335566d4915691e33d41da6b62 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Wed, 14 Sep 2022 13:34:10 +0000 Subject: [PATCH] Fix deletion of address in l4, remove frName argument We were trying to delete address with backend service name, but we only reserve address with forwarding rule name Remove frName argument from ensureForwardingRule, and instead construct the name on the fly --- pkg/loadbalancers/forwarding_rules.go | 13 +++++++------ pkg/loadbalancers/l4.go | 9 ++++----- pkg/loadbalancers/l4_test.go | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index e4f61d80f4..1c1a7f7af1 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -195,9 +195,10 @@ func (l7 *L7) getEffectiveIP() (string, bool, error) { // ensureForwardingRule creates a forwarding rule with the given name, if it does not exist. It updates the existing // forwarding rule if needed. -func (l4 *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.ILBOptions, existingFwdRule *composite.ForwardingRule) (*composite.ForwardingRule, error) { +func (l4 *L4) ensureForwardingRule(bsLink string, options gce.ILBOptions, existingFwdRule *composite.ForwardingRule) (*composite.ForwardingRule, error) { // version used for creating the existing forwarding rule. version := meta.VersionGA + frName := l4.GetFRName() if l4.cloud.IsLegacyNetwork() { l4.recorder.Event(l4.Service, v1.EventTypeWarning, "ILBOptionsIgnored", "Internal LoadBalancer options are not supported with Legacy Networks.") @@ -218,20 +219,20 @@ func (l4 *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce. // Determine IP which will be used for this LB. If no forwarding rule has been established // or specified in the Service spec, then requestedIP = "". ipToUse := l4lbIPToUse(l4.Service, existingFwdRule, subnetworkURL) - klog.V(2).Infof("ensureForwardingRule(%v): Using subnet %q for LoadBalancer IP %s", loadBalancerName, subnetworkURL, ipToUse) + klog.V(2).Infof("ensureForwardingRule(%v): Using subnet %q for LoadBalancer IP %s", frName, subnetworkURL, ipToUse) var addrMgr *addressManager // If the network is not a legacy network, use the address manager if !l4.cloud.IsLegacyNetwork() { nm := types.NamespacedName{Namespace: l4.Service.Namespace, Name: l4.Service.Name}.String() // ILB can be created only in Premium Tier - addrMgr = newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, loadBalancerName, ipToUse, cloud.SchemeInternal, cloud.NetworkTierPremium) + addrMgr = newAddressManager(l4.cloud, nm, l4.cloud.Region(), subnetworkURL, frName, ipToUse, cloud.SchemeInternal, cloud.NetworkTierPremium) var err error ipToUse, _, err = addrMgr.HoldAddress() if err != nil { return nil, err } - klog.V(2).Infof("ensureForwardingRule(%v): reserved IP %q for the forwarding rule", loadBalancerName, ipToUse) + klog.V(2).Infof("ensureForwardingRule(%v): reserved IP %q for the forwarding rule", frName, ipToUse) defer func() { // Release the address that was reserved, in all cases. If the forwarding rule was successfully created, // the ephemeral IP is not needed anymore. If it was not created, the address should be released to prevent leaks. @@ -248,12 +249,12 @@ func (l4 *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce. frDesc, err := utils.MakeL4LBServiceDescription(utils.ServiceKeyFunc(l4.Service.Namespace, l4.Service.Name), ipToUse, version, false, utils.ILB) if err != nil { - return nil, fmt.Errorf("Failed to compute description for forwarding rule %s, err: %w", loadBalancerName, + return nil, fmt.Errorf("Failed to compute description for forwarding rule %s, err: %w", frName, err) } fr := &composite.ForwardingRule{ - Name: loadBalancerName, + Name: frName, IPAddress: ipToUse, Ports: ports, IPProtocol: string(protocol), diff --git a/pkg/loadbalancers/l4.go b/pkg/loadbalancers/l4.go index c67b7a7b35..d6a646af42 100644 --- a/pkg/loadbalancers/l4.go +++ b/pkg/loadbalancers/l4.go @@ -118,7 +118,7 @@ func (l4 *L4) EnsureInternalLoadBalancerDeleted(svc *corev1.Service) *L4ILBSyncR result.Error = err result.GCEResourceInError = annotations.ForwardingRuleResource } - if err = ensureAddressDeleted(l4.cloud, name, l4.cloud.Region()); err != nil { + if err = ensureAddressDeleted(l4.cloud, frName, l4.cloud.Region()); err != nil { klog.Errorf("Failed to delete address for internal loadbalancer service %s, err %v", l4.NamespacedName.String(), err) result.Error = err result.GCEResourceInError = annotations.AddressResource @@ -247,8 +247,7 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service } result.Annotations[annotations.BackendServiceKey] = name // create fr rule - frName := l4.GetFRName() - fr, err := l4.ensureForwardingRule(frName, bs.SelfLink, options, existingFR) + fr, err := l4.ensureForwardingRule(bs.SelfLink, options, existingFR) if err != nil { klog.Errorf("EnsureInternalLoadBalancer: Failed to create forwarding rule - %v", err) result.GCEResourceInError = annotations.ForwardingRuleResource @@ -256,9 +255,9 @@ func (l4 *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service return result } if fr.IPProtocol == string(corev1.ProtocolTCP) { - result.Annotations[annotations.TCPForwardingRuleKey] = frName + result.Annotations[annotations.TCPForwardingRuleKey] = fr.Name } else { - result.Annotations[annotations.UDPForwardingRuleKey] = frName + result.Annotations[annotations.UDPForwardingRuleKey] = fr.Name } // ensure firewalls diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index ec2e5a49ce..336435a6a8 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -1621,9 +1621,9 @@ func assertILBResourcesDeleted(t *testing.T, l4 *L4) { t.Errorf("verifyBackendServiceNotExists(_, %s)", nodesFwName) } - err = verifyAddressNotExists(l4, nodesFwName) + err = verifyAddressNotExists(l4, frName) if err != nil { - t.Errorf("verifyAddressNotExists(_, %s)", nodesFwName) + t.Errorf("verifyAddressNotExists(_, %s)", frName) } }