Skip to content

Commit

Permalink
Fix deletion of address in l4, remove frName argument
Browse files Browse the repository at this point in the history
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
  • Loading branch information
panslava committed Sep 15, 2022
1 parent b69ecdd commit 6c9120a
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 13 deletions.
13 changes: 7 additions & 6 deletions pkg/loadbalancers/forwarding_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand All @@ -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.
Expand All @@ -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),
Expand Down
9 changes: 4 additions & 5 deletions pkg/loadbalancers/l4.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -247,18 +247,17 @@ 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
result.Error = err
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
Expand Down
4 changes: 2 additions & 2 deletions pkg/loadbalancers/l4_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down

0 comments on commit 6c9120a

Please sign in to comment.