diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index dcbde5d78c0..05aa5adfa4d 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/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)" diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index 22284569ba0..808b3579010 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/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) + } +}