diff --git a/pkg/cloudprovider/providers/azure/azure_loadbalancer.go b/pkg/cloudprovider/providers/azure/azure_loadbalancer.go index 222d92d519..9a114d5ec0 100644 --- 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 index 7dda564e7d..f1368575b6 100644 --- 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 index 523809f3b3..e30b0d9297 100644 --- 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 index 95e8c7dac8..277c6debbd 100644 --- 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.