From eed98104af9d1d151db91b250fa9e9499341d4ae Mon Sep 17 00:00:00 2001 From: Phillip Stagnet Date: Thu, 13 Aug 2020 16:41:40 +0200 Subject: [PATCH] Allow 404 error on lb deletion in azure When trying to delete a loadbalancer after all resources in azure including the resource group have been deleted we currently get an an error when trying to delete the loadbalancer afterwards because the resource group hasn't been found. Change EnsureLoadBalancerDeleted function to not fail when the resource group has already been deleted. --- .../azure/azure_loadbalancer.go | 5 +++-- .../azure/azure_loadbalancer_test.go | 10 ++++++++++ .../azure/retry/azure_error.go | 17 +++++++++++++++++ .../azure/retry/azure_error_test.go | 14 ++++++++++++++ 4 files changed, 44 insertions(+), 2 deletions(-) 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 9b150397d02..9744507c8cb 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 @@ -36,6 +36,7 @@ import ( servicehelpers "k8s.io/cloud-provider/service/helpers" "k8s.io/klog/v2" azcache "k8s.io/legacy-cloud-providers/azure/cache" + "k8s.io/legacy-cloud-providers/azure/retry" utilnet "k8s.io/utils/net" ) @@ -216,7 +217,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri klog.V(5).Infof("Delete service (%s): START clusterName=%q", serviceName, clusterName) serviceIPToCleanup, err := az.findServiceIPAddress(ctx, clusterName, service, isInternal) - if err != nil { + if err != nil && !retry.HasStatusForbiddenOrIgnoredError(err) { return err } @@ -225,7 +226,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri return err } - if _, err := az.reconcileLoadBalancer(clusterName, service, nil, false /* wantLb */); err != nil { + if _, err := az.reconcileLoadBalancer(clusterName, service, nil, false /* wantLb */); err != nil && !retry.HasStatusForbiddenOrIgnoredError(err) { return err } 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 ceb249d3e12..f7eaffd730e 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 @@ -366,6 +366,7 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) { service v1.Service isInternalSvc bool expectCreateError bool + wrongRGAtDelete bool }{ { desc: "external service should be created and deleted successfully", @@ -385,6 +386,12 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) { service: getResourceGroupTestService("service4", "random-rg", "1.2.3.4", 80), expectCreateError: true, }, + { + desc: "annotated service with different resourceGroup shouldn't be created but should be deleted successfully", + service: getResourceGroupTestService("service5", "random-rg", "", 80), + expectCreateError: true, + wrongRGAtDelete: true, + }, } ctrl := gomock.NewController(t) @@ -419,6 +426,9 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) { expectedLBs = make([]network.LoadBalancer, 0) setMockLBs(az, ctrl, &expectedLBs, "service", 1, i+1, c.isInternalSvc) // finally, delete it. + if c.wrongRGAtDelete { + az.LoadBalancerResourceGroup = "nil" + } err = az.EnsureLoadBalancerDeleted(context.TODO(), testClusterName, &c.service) expectedLBs = make([]network.LoadBalancer, 0) mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error.go b/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error.go index 02403e7ef01..a0cedbef2d4 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error.go @@ -286,3 +286,20 @@ func IsErrorRetriable(err error) bool { return strings.Contains(err.Error(), "Retriable: true") } + +// HasStatusForbiddenOrIgnoredError return true if the given error code is part of the error message +// This should only be used when trying to delete resources +func HasStatusForbiddenOrIgnoredError(err error) bool { + if err == nil { + return false + } + + if strings.Contains(err.Error(), fmt.Sprintf("HTTPStatusCode: %d", http.StatusNotFound)) { + return true + } + + if strings.Contains(err.Error(), fmt.Sprintf("HTTPStatusCode: %d", http.StatusForbidden)) { + return true + } + return false +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error_test.go index ff905cb4209..eafb772172b 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/retry/azure_error_test.go @@ -352,3 +352,17 @@ func TestIsErrorRetriable(t *testing.T) { result = IsErrorRetriable(fmt.Errorf("Retriable: true")) assert.Equal(t, true, result) } + +func TestHasErrorCode(t *testing.T) { + // false case + result := HasStatusForbiddenOrIgnoredError(fmt.Errorf("HTTPStatusCode: 408")) + assert.False(t, result) + + // true case 404 + result = HasStatusForbiddenOrIgnoredError(fmt.Errorf("HTTPStatusCode: %d", http.StatusNotFound)) + assert.True(t, result) + + // true case 403 + result = HasStatusForbiddenOrIgnoredError(fmt.Errorf("HTTPStatusCode: %d", http.StatusForbidden)) + assert.True(t, result) +}