Skip to content

Commit

Permalink
Allow 404 error on lb deletion in azure
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
phiphi282 authored and andyzhangx committed Sep 11, 2020
1 parent c60398c commit b1b34b0
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 2 deletions.
Expand Up @@ -36,6 +36,7 @@ import (
servicehelpers "k8s.io/cloud-provider/service/helpers"
"k8s.io/klog"
azcache "k8s.io/legacy-cloud-providers/azure/cache"
"k8s.io/legacy-cloud-providers/azure/retry"
utilnet "k8s.io/utils/net"
)

Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
Expand Up @@ -360,6 +360,7 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
desc string
service v1.Service
expectCreateError bool
wrongRGAtDelete bool
}{
{
desc: "external service should be created and deleted successfully",
Expand All @@ -378,6 +379,12 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
service: getResourceGroupTestService("test4", "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)
Expand All @@ -404,6 +411,9 @@ func TestEnsureLoadBalancerDeleted(t *testing.T) {
}

// finally, delete it.
if c.wrongRGAtDelete {
az.LoadBalancerResourceGroup = "nil"
}
err = az.EnsureLoadBalancerDeleted(context.TODO(), testClusterName, &c.service)
assert.Nil(t, err, "TestCase[%d]: %s", i, c.desc)
result, rerr := az.LoadBalancerClient.List(context.Background(), az.Config.ResourceGroup)
Expand Down
Expand Up @@ -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
}
Expand Up @@ -290,3 +290,27 @@ func TestIsThrottled(t *testing.T) {
assert.Equal(t, test.expected, real)
}
}

func TestIsErrorRetriable(t *testing.T) {
// flase case
result := IsErrorRetriable(nil)
assert.Equal(t, false, result)

// true case
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)
}

0 comments on commit b1b34b0

Please sign in to comment.