diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index a70ca157e0..1140bfc267 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -261,6 +261,13 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I return nil, err } klog.V(2).Infof("ensureForwardingRule(%v): reserved IP %q for the forwarding rule", loadBalancerName, 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. + if err := addrMgr.ReleaseAddress(); err != nil { + klog.Errorf("ensureInternalLoadBalancer: failed to release address reservation, possibly causing an orphan: %v", err) + } + }() } ports, _, protocol := utils.GetPortsAndProtocol(l.Service.Spec.Ports) @@ -305,14 +312,6 @@ func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.I if err = composite.CreateForwardingRule(l.cloud, key, fr); err != nil { return nil, err } - if addrMgr != nil { - // Now that the controller knows the forwarding rule exists, we can release the address. - if err := addrMgr.ReleaseAddress(); err != nil { - klog.Errorf("ensureInternalLoadBalancer: - %s, failed to release address reservation, possibly causing an orphan: %v", fr.Name, err) - } - } - l.recorder.Eventf(l.Service, corev1.EventTypeNormal, events.SyncIngress, "ForwardingRule %q created", key.Name) - return composite.GetForwardingRule(l.cloud, key, fr.Version) } diff --git a/pkg/loadbalancers/l4_test.go b/pkg/loadbalancers/l4_test.go index 8fa6f013bf..0b8e05bb1e 100644 --- a/pkg/loadbalancers/l4_test.go +++ b/pkg/loadbalancers/l4_test.go @@ -133,6 +133,15 @@ func TestEnsureInternalLoadBalancer(t *testing.T) { t.Errorf("Got empty loadBalancer status using handler %v", l) } assertInternalLbResources(t, svc, l, nodeNames) + // Simulate a periodic sync + status, err = l.EnsureInternalLoadBalancer(nodeNames, svc) + if err != nil { + t.Errorf("Failed to ensure loadBalancer, err %v", err) + } + if len(status.Ingress) == 0 { + t.Errorf("Got empty loadBalancer status using handler %v", l) + } + assertInternalLbResources(t, svc, l, nodeNames) } func TestEnsureInternalLoadBalancerTypeChange(t *testing.T) { @@ -1107,6 +1116,6 @@ func assertInternalLbResourcesDeleted(t *testing.T, apiService *v1.Service, fire } addr, err := l.cloud.GetRegionAddress(resourceName, l.cloud.Region()) if err == nil || addr != nil { - t.Errorf("Expected error when looking up backend service after deletion") + t.Errorf("Expected error when looking up IP address after deletion") } }