diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index 9c2d84006b5..0f5e9fc1b08 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -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 { diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index 2c1212f62f8..ff3386ee195 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go @@ -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) }) } } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go index 1dd0fd02c49..911c37bff07 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go @@ -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 {