Merge pull request #1 from omattsson/add_rbac_retry

Add rbac retry

Author: omattsson

azurehelper/azure_blob.go

@@ -5,10 +5,8 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
 	"strings"
-	"time"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
@@ -18,25 +16,6 @@ import (
 	"github.com/gruntwork-io/terragrunt/pkg/log"
 )
 
-// waitForStorageAccountDNS waits for the storage account DNS to propagate and become available.
-func waitForStorageAccountDNS(name string, maxWait time.Duration) error {
-	url := name + ".blob.core.windows.net"
-	deadline := time.Now().Add(maxWait)
-
-	for {
-		_, err := net.LookupHost(url)
-		if err == nil {
-			return nil // DNS is ready
-		}
-
-		if time.Now().After(deadline) {
-			return fmt.Errorf("storage account DNS not available after %s: %w", maxWait, err)
-		}
-
-		time.Sleep(3 * time.Second) //nolint: mnd
-	}
-}
-
 // BlobServiceClient wraps Azure's azblob client to provide a simpler interface for our needs.
 type BlobServiceClient struct {
 	client *azblob.Client
@@ -88,40 +67,13 @@ func (e *AzureResponseError) Error() string {
 	return fmt.Sprintf("Azure API error (StatusCode=%d, ErrorCode=%s): %s", e.StatusCode, e.ErrorCode, e.Message)
 }
 
-// IsVersioningEnabled checks if versioning is enabled for a blob storage account.
-// In Azure Blob Storage, versioning is a storage account level setting and cannot be
-// configured at the container level. This method always returns true as versioning
-// state needs to be checked at the storage account level.
-func (c *BlobServiceClient) IsVersioningEnabled(ctx context.Context, containerName string) (bool, error) {
-	l := log.Default()
-	l.Warnf("Warning: Blob versioning in Azure Storage is a storage account level setting, not a container level setting")
-
-	return true, nil
-}
-
-// EnableVersioningIfNecessary is deprecated as versioning is a storage account level setting.
-func (c *BlobServiceClient) EnableVersioningIfNecessary(ctx context.Context, l log.Logger, containerName string) error {
-	l.Warnf("Warning: Blob versioning in Azure Storage is a storage account level setting and cannot be configured at container level")
-
-	return nil
-}
-
 // CreateBlobServiceClient creates a new Azure Blob Service client using the configuration from the backend.
 func CreateBlobServiceClient(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, config map[string]interface{}) (*BlobServiceClient, error) {
 	storageAccountName, okStorageAccountName := config["storage_account_name"].(string)
 	if !okStorageAccountName || storageAccountName == "" {
 		return nil, errors.Errorf("storage_account_name is required")
 	}
 
-	// Wait for DNS propagation only if explicitly requested via config
-	// This is useful when the storage account was just created and DNS might not be propagated yet
-	if waitForDNS, ok := config["wait_for_dns"].(bool); ok && waitForDNS {
-		l.Infof("Waiting for DNS propagation for storage account %s", storageAccountName)
-
-		if err := waitForStorageAccountDNS(storageAccountName, 60*time.Second); err != nil { //nolint: mnd
-			l.Warnf("DNS propagation wait failed for %s: %v", storageAccountName, err)
-		}
-	}
 	// Extract resource group and subscription ID if provided
 	resourceGroupName, _ := config["resource_group_name"].(string)
 	subscriptionID, _ := config["subscription_id"].(string)

azurehelper/azure_constants.go

@@ -28,4 +28,9 @@ const (
 	AccessTierHot     = "Hot"
 	AccessTierCool    = "Cool"
 	AccessTierPremium = "Premium"
+
+	// RBAC propagation retry configuration
+	RbacRetryDelay    = 3 * time.Second
+	RbacMaxRetries    = 5
+	RbacRetryAttempts = RbacMaxRetries + 1 // Total attempts including first try
 )

azurehelper/azure_rbac_constants_test.go

@@ -0,0 +1,150 @@
+package azurehelper_test
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/gruntwork-io/terragrunt/azurehelper"
+	"github.com/stretchr/testify/assert"
+)
+
+// TestRBACRetryConstants ensures the RBAC retry constants have the expected values
+// and that the retry attempts is correctly calculated from max retries
+func TestRBACRetryConstants(t *testing.T) {
+	t.Parallel()
+	// Test RBAC delay is 3 seconds
+	assert.Equal(t, 3*time.Second, azurehelper.RbacRetryDelay, "RBAC retry delay should be 3 seconds")
+
+	// Test RBAC max retries is 5
+	assert.Equal(t, 5, azurehelper.RbacMaxRetries, "RBAC max retries should be 5")
+
+	// Test that retry attempts equals max retries + 1 (for the initial attempt)
+	assert.Equal(t, azurehelper.RbacMaxRetries+1, azurehelper.RbacRetryAttempts,
+		"RBAC retry attempts should equal RbacMaxRetries+1")
+
+	// Test the specific expected values
+	assert.Equal(t, 6, azurehelper.RbacRetryAttempts, "RBAC retry attempts should be 6 (5 retries + initial attempt)")
+
+	// The constants are defined in the package, so we can't modify them for testing
+	// Instead, we'll just verify the current values match our expectations
+
+	// Test different values of the relationship
+	testCases := []struct {
+		maxRetries       int
+		expectedAttempts int
+	}{
+		{3, 4},
+		{5, 6}, // Current value
+		{10, 11},
+		{0, 1}, // Edge case: no retries means 1 attempt
+	}
+
+	for _, tc := range testCases {
+		calculatedAttempts := tc.maxRetries + 1
+		assert.Equal(t, tc.expectedAttempts, calculatedAttempts,
+			"When max retries is %d, attempts should be %d", tc.maxRetries, tc.expectedAttempts)
+	}
+}
+
+// TestIsPermissionError tests the isPermissionError function with various error types
+// nolint: govet
+func TestIsPermissionError(t *testing.T) {
+	t.Parallel()
+	// Create storage account client for testing
+	client := &azurehelper.StorageAccountClient{}
+
+	// Test cases for various error messages
+	testCases := []struct {
+		name     string
+		input    error
+		expected bool
+	}{
+		{
+			name:     "nil error",
+			input:    nil,
+			expected: false,
+		},
+		{
+			name:     "basic authorization error",
+			input:    errors.New("operation failed due to authorization failed"),
+			expected: true,
+		},
+		{
+			name:     "permission denied error",
+			input:    errors.New("permission denied for operation"),
+			expected: true,
+		},
+		{
+			name:     "forbidden error",
+			input:    errors.New("server returned status code 403 forbidden"),
+			expected: true,
+		},
+		{
+			name:     "access denied error",
+			input:    errors.New("access denied to resource"),
+			expected: true,
+		},
+		{
+			name:     "not authorized error",
+			input:    errors.New("client is not authorized to perform this action"),
+			expected: true,
+		},
+		{
+			name:     "role assignment error",
+			input:    errors.New("waiting for role assignment to complete"),
+			expected: true,
+		},
+		{
+			name:     "storage blob data owner error",
+			input:    errors.New("requires storage blob data owner role"),
+			expected: true,
+		},
+		{
+			name:     "unrelated error",
+			input:    errors.New("connection timed out"),
+			expected: false,
+		},
+		{
+			name:     "network error",
+			input:    errors.New("network connectivity issues"),
+			expected: false,
+		},
+		{
+			name:     "validation error",
+			input:    errors.New("validation failed for resource"),
+			expected: false,
+		},
+	}
+
+	// Run test cases
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			result := client.IsPermissionError(tc.input)
+			assert.Equal(t, tc.expected, result,
+				"IsPermissionError should return %v for error: %v", tc.expected, tc.input)
+		})
+	}
+
+	// Test with wrapped errors
+	t.Run("WrappedPermissionError", func(t *testing.T) {
+		t.Parallel()
+		innerErr := errors.New("permission denied for storage account")
+		wrappedErr := fmt.Errorf("operation failed: %w", innerErr)
+
+		assert.True(t, client.IsPermissionError(wrappedErr),
+			"Should detect permission error in wrapped error")
+	})
+
+	// Test with non-permission wrapped errors
+	t.Run("WrappedNonPermissionError", func(t *testing.T) {
+		t.Parallel()
+		innerErr := errors.New("resource not found")
+		wrappedErr2 := fmt.Errorf("operation failed: %w", innerErr)
+
+		assert.False(t, client.IsPermissionError(wrappedErr2),
+			"Should not detect permission error in unrelated wrapped error")
+	})
+}