Merge pull request #82890 from chewong/orphaned-public-ips

Fix #80571: Update service controller to prevent orphaned public IP addresses

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go

@@ -104,18 +104,38 @@ const (
 	clusterNameKey = "kubernetes-cluster-name"
 )
 
-// GetLoadBalancer returns whether the specified load balancer exists, and
+// GetLoadBalancer returns whether the specified load balancer and its components exist, and
 // if so, what its status is.
 func (az *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, service *v1.Service) (status *v1.LoadBalancerStatus, exists bool, err error) {
-	_, status, exists, err = az.getServiceLoadBalancer(service, clusterName, nil, false)
+	// Since public IP is not a part of the load balancer on Azure,
+	// there is a chance that we could orphan public IP resources while we delete the load blanacer (kubernetes/kubernetes#80571).
+	// We need to make sure the existence of the load balancer depends on the load balancer resource and public IP resource on Azure.
+	existsPip := func() bool {
+		pipName, _, err := az.determinePublicIPName(clusterName, service)
+		if err != nil {
+			return false
+		}
+		pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
+		_, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)
+		if err != nil {
+			return false
+		}
+		return existsPip
+	}()
+
+	_, status, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false)
 	if err != nil {
-		return nil, false, err
+		return nil, existsPip, err
 	}
-	if !exists {
+
+	// Return exists = false only if the load balancer and the public IP are not found on Azure
+	if !existsLb && !existsPip {
 		serviceName := getServiceName(service)
 		klog.V(5).Infof("getloadbalancer (cluster:%s) (service:%s) - doesn't exist", clusterName, serviceName)
 		return nil, false, nil
 	}
+
+	// Return exists = true if either the load balancer or the public IP (or both) exists
 	return status, true, nil
 }
 
@@ -169,6 +189,10 @@ func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, ser
 
 // UpdateLoadBalancer updates hosts under the specified load balancer.
 func (az *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) error {
+	if !az.shouldUpdateLoadBalancer(clusterName, service) {
+		klog.V(2).Infof("UpdateLoadBalancer: skipping service %s because it is either being deleted or does not exist anymore", service.Name)
+		return nil
+	}
 	_, err := az.EnsureLoadBalancer(ctx, clusterName, service, nodes)
 	return err
 }
@@ -475,7 +499,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s
 		return service.Spec.LoadBalancerIP, nil
 	}
 
-	lbStatus, existsLb, err := az.GetLoadBalancer(ctx, clusterName, service)
+	_, lbStatus, existsLb, err := az.getServiceLoadBalancer(service, clusterName, nil, false)
 	if err != nil {
 		return "", err
 	}
@@ -1283,6 +1307,11 @@ func (az *Cloud) reconcileSecurityGroup(clusterName string, service *v1.Service,
 	return &sg, nil
 }
 
+func (az *Cloud) shouldUpdateLoadBalancer(clusterName string, service *v1.Service) bool {
+	_, _, existsLb, _ := az.getServiceLoadBalancer(service, clusterName, nil, false)
+	return existsLb && service.ObjectMeta.DeletionTimestamp == nil
+}
+
 func logSafe(s *string) string {
 	if s == nil {
 		return "(nil)"

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go

@@ -25,6 +25,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network"
 	"github.com/Azure/go-autorest/autorest/to"
@@ -1895,3 +1896,66 @@ func TestEnsurePublicIPExists(t *testing.T) {
 		assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
 	}
 }
+
+func TestShouldUpdateLoadBalancer(t *testing.T) {
+	testCases := []struct {
+		desc                   string
+		lbHasDeletionTimestamp bool
+		existsLb               bool
+		expectedOutput         bool
+	}{
+		{
+			desc:                   "should update a load balancer that does not have a deletion timestamp and exists in Azure",
+			lbHasDeletionTimestamp: false,
+			existsLb:               true,
+			expectedOutput:         true,
+		},
+		{
+			desc:                   "should not update a load balancer that is being deleted / already deleted in K8s",
+			lbHasDeletionTimestamp: true,
+			existsLb:               true,
+			expectedOutput:         false,
+		},
+		{
+			desc:                   "should not update a load balancer that does not exist in Azure",
+			lbHasDeletionTimestamp: false,
+			existsLb:               false,
+			expectedOutput:         false,
+		},
+		{
+			desc:                   "should not update a load balancer that has a deletion timestamp and does not exist in Azure",
+			lbHasDeletionTimestamp: true,
+			existsLb:               false,
+			expectedOutput:         false,
+		},
+	}
+
+	for i, test := range testCases {
+		az := getTestCloud()
+		service := getTestService("test1", v1.ProtocolTCP, nil, 80)
+		if test.lbHasDeletionTimestamp {
+			service.ObjectMeta.DeletionTimestamp = &metav1.Time{time.Now()}
+		}
+		if test.existsLb {
+			lb := network.LoadBalancer{
+				Name: to.StringPtr("lb1"),
+				LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
+					FrontendIPConfigurations: &[]network.FrontendIPConfiguration{
+						{
+							Name: to.StringPtr("atest1"),
+							FrontendIPConfigurationPropertiesFormat: &network.FrontendIPConfigurationPropertiesFormat{
+								PublicIPAddress: &network.PublicIPAddress{ID: to.StringPtr("id1")},
+							},
+						},
+					},
+				},
+			}
+			_, err := az.LoadBalancerClient.CreateOrUpdate(context.TODO(), "rg", *lb.Name, lb, "")
+			if err != nil {
+				t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
+			}
+		}
+		shouldUpdateLoadBalancer := az.shouldUpdateLoadBalancer(testClusterName, &service)
+		assert.Equal(t, test.expectedOutput, shouldUpdateLoadBalancer, "TestCase[%d]: %s", i, test.desc)
+	}
+}