Merge pull request kubernetes#75256 from feiskyer/fix-75198

Ensure Azure load balancer cleaned up on 404 or 403
yibozhuang · Mar 12, 2019 · 9cbccd3 · 9cbccd3
2 parents a213886 + 88907da
commit 9cbccd3
Show file tree

Hide file tree

Showing 4 changed files with 106 additions and 6 deletions.
diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go
@@ -169,27 +169,47 @@ func (az *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, ser
 func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *v1.Service) error {
 	isInternal := requiresInternalLoadBalancer(service)
 	serviceName := getServiceName(service)
-	klog.V(5).Infof("delete(%s): START clusterName=%q", serviceName, clusterName)
+	klog.V(5).Infof("Delete service (%s): START clusterName=%q", serviceName, clusterName)
+
+	ignoreErrors := func(err error) error {
+		if ignoreStatusNotFoundFromError(err) == nil {
+			klog.V(5).Infof("EnsureLoadBalancerDeleted: ignoring StatusNotFound error because the resource doesn't exist (%v)", err)
+			return nil
+		}
+
+		if ignoreStatusForbiddenFromError(err) == nil {
+			klog.V(5).Infof("EnsureLoadBalancerDeleted: ignoring StatusForbidden error (%v). This may be caused by wrong configuration via service annotations", err)
+			return nil
+		}
+
+		return err
+	}
 
 	serviceIPToCleanup, err := az.findServiceIPAddress(ctx, clusterName, service, isInternal)
-	if err != nil {
+	if ignoreErrors(err) != nil {
 		return err
 	}
 
 	klog.V(2).Infof("EnsureLoadBalancerDeleted: reconciling security group for service %q with IP %q, wantLb = false", serviceName, serviceIPToCleanup)
 	if _, err := az.reconcileSecurityGroup(clusterName, service, &serviceIPToCleanup, false /* wantLb */); err != nil {
-		return err
+		if ignoreErrors(err) != nil {
+			return err
+		}
 	}
 
 	if _, err := az.reconcileLoadBalancer(clusterName, service, nil, false /* wantLb */); err != nil {
-		return err
+		if ignoreErrors(err) != nil {
+			return err
+		}
 	}
 
 	if _, err := az.reconcilePublicIP(clusterName, service, nil, false /* wantLb */); err != nil {
-		return err
+		if ignoreErrors(err) != nil {
+			return err
+		}
 	}
 
-	klog.V(2).Infof("delete(%s): FINISH", serviceName)
+	klog.V(2).Infof("Delete service (%s): FINISH", serviceName)
 	return nil
 }
 

diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer_test.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package azure
 
 import (
+	"context"
 	"fmt"
 	"reflect"
 	"testing"
@@ -308,3 +309,62 @@ func TestSubnet(t *testing.T) {
 		assert.Equal(t, c.expected, real, fmt.Sprintf("TestCase[%d]: %s", i, c.desc))
 	}
 }
+
+func TestEnsureLoadBalancerDeleted(t *testing.T) {
+	const vmCount = 8
+	const availabilitySetCount = 4
+	const serviceCount = 9
+
+	tests := []struct {
+		desc              string
+		service           v1.Service
+		expectCreateError bool
+	}{
+		{
+			desc:    "external service should be created and deleted successfully",
+			service: getTestService("test1", v1.ProtocolTCP, 80),
+		},
+		{
+			desc:    "internal service should be created and deleted successfully",
+			service: getInternalTestService("test2", 80),
+		},
+		{
+			desc:    "annotated service with same resourceGroup should be created and deleted successfully",
+			service: getResourceGroupTestService("test3", "rg", "", 80),
+		},
+		{
+			desc:              "annotated service with different resourceGroup shouldn't be created but should be deleted successfully",
+			service:           getResourceGroupTestService("test4", "random-rg", "1.2.3.4", 80),
+			expectCreateError: true,
+		},
+	}
+
+	az := getTestCloud()
+	for i, c := range tests {
+		clusterResources := getClusterResources(az, vmCount, availabilitySetCount)
+		getTestSecurityGroup(az)
+		if c.service.Annotations[ServiceAnnotationLoadBalancerInternal] == "true" {
+			addTestSubnet(t, az, &c.service)
+		}
+
+		// create the service first.
+		lbStatus, err := az.EnsureLoadBalancer(context.TODO(), testClusterName, &c.service, clusterResources.nodes)
+		if c.expectCreateError {
+			assert.NotNil(t, err, "TestCase[%d]: %s", i, c.desc)
+		} else {
+			assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
+			assert.NotNil(t, lbStatus, "TestCase[%d]: %s", i, c.desc)
+			result, err := az.LoadBalancerClient.List(context.TODO(), az.Config.ResourceGroup)
+			assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
+			assert.Equal(t, len(result), 1, "TestCase[%d]: %s", i, c.desc)
+			assert.Equal(t, len(*result[0].LoadBalancingRules), 1, "TestCase[%d]: %s", i, c.desc)
+		}
+
+		// finally, delete it.
+		err = az.EnsureLoadBalancerDeleted(context.TODO(), testClusterName, &c.service)
+		assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
+		result, err := az.LoadBalancerClient.List(context.Background(), az.Config.ResourceGroup)
+		assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
+		assert.Equal(t, len(result), 0, "TestCase[%d]: %s", i, c.desc)
+	}
+}
diff --git a/pkg/cloudprovider/providers/azure/azure_test.go b/pkg/cloudprovider/providers/azure/azure_test.go
@@ -1137,6 +1137,13 @@ func getInternalTestService(identifier string, requestedPorts ...int32) v1.Servi
 	return svc
 }
 
+func getResourceGroupTestService(identifier, resourceGroup, loadBalancerIP string, requestedPorts ...int32) v1.Service {
+	svc := getTestService(identifier, v1.ProtocolTCP, requestedPorts...)
+	svc.Spec.LoadBalancerIP = loadBalancerIP
+	svc.Annotations[ServiceAnnotationLoadBalancerResourceGroup] = resourceGroup
+	return svc
+}
+
 func setLoadBalancerModeAnnotation(service *v1.Service, lbMode string) {
 	service.Annotations[ServiceAnnotationLoadBalancerMode] = lbMode
 }

diff --git a/pkg/cloudprovider/providers/azure/azure_wrap.go b/pkg/cloudprovider/providers/azure/azure_wrap.go
@@ -71,6 +71,19 @@ func ignoreStatusNotFoundFromError(err error) error {
 	return err
 }
 
+// ignoreStatusForbiddenFromError returns nil if the status code is StatusForbidden.
+// This happens when AuthorizationFailed is reported from Azure API.
+func ignoreStatusForbiddenFromError(err error) error {
+	if err == nil {
+		return nil
+	}
+	v, ok := err.(autorest.DetailedError)
+	if ok && v.StatusCode == http.StatusForbidden {
+		return nil
+	}
+	return err
+}
+
 /// getVirtualMachine calls 'VirtualMachinesClient.Get' with a timed cache
 /// The service side has throttling control that delays responses if there're multiple requests onto certain vm
 /// resource request in short period.