Release reserved GCE IP address after ensure completes.

Previous behavior was to release the ip only upon success, but it should be released
on failure as well. Otherwise, an IP address will be unnecessarily consumed due to an
in-error service.
This commit is contained in:
Pavithra Ramesh 2021-01-05 14:12:03 -08:00
parent c4aca30005
commit 690189178d
3 changed files with 21 additions and 7 deletions

View File

@ -153,6 +153,12 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
return nil, err
}
klog.V(2).Infof("ensureInternalLoadBalancer(%v): reserved IP %q for the forwarding rule", loadBalancerName, ipToUse)
defer func() {
// Release the address if all resources were created successfully, or if we error out.
if err := addrMgr.ReleaseAddress(); err != nil {
klog.Errorf("ensureInternalLoadBalancer: failed to release address reservation, possibly causing an orphan: %v", err)
}
}()
}
// Ensure firewall rules if necessary
@ -213,13 +219,6 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
g.clearPreviousInternalResources(svc, loadBalancerName, existingBackendService, backendServiceName, hcName)
}
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: failed to release address reservation, possibly causing an orphan: %v", err)
}
}
// Get the most recent forwarding rule for the address.
updatedFwdRule, err := g.GetRegionForwardingRule(loadBalancerName, g.region)
if err != nil {

View File

@ -949,6 +949,11 @@ func TestEnsureInternalLoadBalancerErrors(t *testing.T) {
)
assert.Error(t, err, "Should return an error when "+desc)
assert.Nil(t, status, "Should not return a status when "+desc)
// ensure that the temporarily reserved IP address is released upon sync errors
ip, err := gce.GetRegionAddress(gce.GetLoadBalancerName(context.TODO(), params.clusterName, params.service), gce.region)
require.Error(t, err)
assert.Nil(t, ip)
})
}
}

View File

@ -249,6 +249,11 @@ func assertInternalLbResources(t *testing.T, gce *Cloud, apiService *v1.Service,
assert.Equal(t, backendServiceLink, fwdRule.BackendService)
// if no Subnetwork specified, defaults to the GCE NetworkURL
assert.Equal(t, gce.NetworkURL(), fwdRule.Subnetwork)
// Check that the IP address has been released. IP is only reserved until ensure function exits.
ip, err := gce.GetRegionAddress(lbName, gce.region)
require.Error(t, err)
assert.Nil(t, ip)
}
func assertInternalLbResourcesDeleted(t *testing.T, gce *Cloud, apiService *v1.Service, vals TestClusterValues, firewallsDeleted bool) {
@ -287,6 +292,11 @@ func assertInternalLbResourcesDeleted(t *testing.T, gce *Cloud, apiService *v1.S
healthcheck, err := gce.GetHealthCheck(hcName)
require.Error(t, err)
assert.Nil(t, healthcheck)
// Check that the IP address has been released
ip, err := gce.GetRegionAddress(lbName, gce.region)
require.Error(t, err)
assert.Nil(t, ip)
}
func checkEvent(t *testing.T, recorder *record.FakeRecorder, expected string, shouldMatch bool) bool {