Merge pull request #93962 from phiphi282/phiphi/fix-azure-delete-lb

Fix deleting loadbalancer after resource group in azure

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
 	}
 

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)

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
+}

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)
+}