azurerm_role_assignment - fix race condition in read after create

azurerm/internal/services/authorization/parse/role_assignment.go

@@ -0,0 +1,47 @@
+package parse
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
+)
+
+type RoleAssignmentID struct {
+	SubscriptionID  string
+	ResourceGroup   string
+	ManagementGroup string
+	Name            string
+}
+
+func RoleAssignmentId(input string) (*RoleAssignmentID, error) {
+	if len(input) == 0 {
+		return nil, fmt.Errorf("Role Assignment ID is empty string")
+	}
+
+	roleAssignmentId := RoleAssignmentID{}
+
+	switch {
+	case strings.HasPrefix(input, "/subscriptions/"):
+		id, err := azure.ParseAzureResourceID(input)
+		if err != nil {
+			return nil, fmt.Errorf("could not parse %q as Azure resource ID", input)
+		}
+		roleAssignmentId.SubscriptionID = id.SubscriptionID
+		roleAssignmentId.ResourceGroup = id.ResourceGroup
+		if roleAssignmentId.Name, err = id.PopSegment("roleAssignments"); err != nil {
+			return nil, err
+		}
+	case strings.HasPrefix(input, "/providers/Microsoft.Management/"):
+		idParts := strings.Split(input, "/providers/Microsoft.Authorization/roleAssignments/")
+		if len(idParts) != 2 {
+			return nil, fmt.Errorf("could not parse Role Assignment ID %q for Management Group", input)
+		}
+		roleAssignmentId.Name = idParts[1]
+		roleAssignmentId.ManagementGroup = strings.Trim(idParts[0], "/providers/Microsoft.Management/managementGroups/")
+	default:
+		return nil, fmt.Errorf("could not parse Role Assignment ID %q", input)
+	}
+
+	return &roleAssignmentId, nil
+}

azurerm/internal/services/authorization/role_assignment_resource.go

@@ -150,10 +150,10 @@ func resourceArmRoleAssignmentCreate(d *schema.ResourceData, meta interface{}) e
 		properties.RoleAssignmentProperties.PrincipalType = authorization.ServicePrincipal
 	}
 
-	// TODO: we should switch this to use the timeout
-	if err := resource.Retry(300*time.Second, retryRoleAssignmentsClient(d, scope, name, properties, meta)); err != nil {
+	if err := resource.Retry(d.Timeout(schema.TimeoutCreate), retryRoleAssignmentsClient(d, scope, name, properties, meta)); err != nil {
 		return err
 	}
+
 	read, err := roleAssignmentsClient.Get(ctx, scope, name)
 	if err != nil {
 		return err
@@ -162,23 +162,6 @@ func resourceArmRoleAssignmentCreate(d *schema.ResourceData, meta interface{}) e
 		return fmt.Errorf("Cannot read Role Assignment ID for %q (Scope %q)", name, scope)
 	}
 
-	stateConf := &resource.StateChangeConf{
-		Pending: []string{
-			"pending",
-		},
-		Target: []string{
-			"ready",
-		},
-		Refresh:                   roleAssignmentCreateStateRefreshFunc(ctx, roleAssignmentsClient, *read.ID),
-		MinTimeout:                5 * time.Second,
-		ContinuousTargetOccurence: 5,
-		Timeout:                   d.Timeout(schema.TimeoutCreate),
-	}
-
-	if _, err := stateConf.WaitForState(); err != nil {
-		return fmt.Errorf("failed waiting for Role Assignment %q to finish replicating: %+v", name, err)
-	}
-
 	d.SetId(*read.ID)
 	return resourceArmRoleAssignmentRead(d, meta)
 }
@@ -262,6 +245,27 @@ func retryRoleAssignmentsClient(d *schema.ResourceData, scope string, name strin
 			return resource.NonRetryableError(err)
 		}
 
+		if resp.ID == nil {
+			return resource.NonRetryableError(fmt.Errorf("creation of Role Assignment %q did not return an id value", name))
+		}
+
+		stateConf := &resource.StateChangeConf{
+			Pending: []string{
+				"pending",
+			},
+			Target: []string{
+				"ready",
+			},
+			Refresh:                   roleAssignmentCreateStateRefreshFunc(ctx, roleAssignmentsClient, *resp.ID),
+			MinTimeout:                5 * time.Second,
+			ContinuousTargetOccurence: 5,
+			Timeout:                   d.Timeout(schema.TimeoutCreate),
+		}
+
+		if _, err := stateConf.WaitForState(); err != nil {
+			return resource.NonRetryableError(fmt.Errorf("failed waiting for Role Assignment %q to finish replicating: %+v", name, err))
+		}
+
 		return nil
 	}
 }

azurerm/internal/services/authorization/role_assignment_resource_test.go

@@ -22,11 +22,19 @@ func TestAccRoleAssignment(t *testing.T) {
 	// Azure only being happy about provisioning a couple at a time
 	acceptance.RunTestsInSequence(t, map[string]map[string]func(t *testing.T){
 		"basic": {
-			"emptyName":      testAccRoleAssignment_emptyName,
-			"roleName":       testAccRoleAssignment_roleName,
-			"dataActions":    testAccRoleAssignment_dataActions,
-			"builtin":        testAccRoleAssignment_builtin,
-			"custom":         testAccRoleAssignment_custom,
+			"roleName": testAccRoleAssignment_roleName,
+			"custom":   testAccRoleAssignment_custom,
+		},
+		"basic_empty_name": {
+			"emptyName": testAccRoleAssignment_emptyName,
+		},
+		"built_in": {
+			"builtin": testAccRoleAssignment_builtin,
+		},
+		"data_actions": {
+			"dataActions": testAccRoleAssignment_dataActions,
+		},
+		"requires_import": {
 			"requiresImport": testAccRoleAssignment_requiresImport,
 		},
 		"assignment": {
@@ -41,15 +49,15 @@ func TestAccRoleAssignment(t *testing.T) {
 }
 
 func testAccRoleAssignment_emptyName(t *testing.T) {
-	data := acceptance.BuildTestData(t, "azurerm_role_definition", "test")
+	data := acceptance.BuildTestData(t, "azurerm_role_assignment", "test")
 	r := RoleAssignmentResource{}
 
 	data.ResourceTest(t, r, []resource.TestStep{
 		{
 			Config: r.emptyNameConfig(),
 			Check: resource.ComposeTestCheckFunc(
 				check.That(data.ResourceName).ExistsInAzure(r),
-				resource.TestCheckResourceAttrSet(data.ResourceName, "name"),
+				check.That(data.ResourceName).Key("name").Exists(),
 			),
 		},
 		data.ImportStep("skip_service_principal_aad_check"),
@@ -67,8 +75,8 @@ func testAccRoleAssignment_roleName(t *testing.T) {
 			Config: r.roleNameConfig(id),
 			Check: resource.ComposeTestCheckFunc(
 				check.That(data.ResourceName).ExistsInAzure(r),
-				resource.TestCheckResourceAttrSet(data.ResourceName, "role_definition_id"),
-				resource.TestCheckResourceAttr(data.ResourceName, "role_definition_name", "Log Analytics Reader"),
+				check.That(data.ResourceName).Key("role_definition_id").Exists(),
+				check.That(data.ResourceName).Key("role_definition_name").HasValue("Log Analytics Reader"),
 			),
 		},
 		data.ImportStep("skip_service_principal_aad_check"),
@@ -86,8 +94,8 @@ func testAccRoleAssignment_requiresImport(t *testing.T) {
 			Config: r.roleNameConfig(id),
 			Check: resource.ComposeTestCheckFunc(
 				check.That(data.ResourceName).ExistsInAzure(r),
-				resource.TestCheckResourceAttrSet(data.ResourceName, "role_definition_id"),
-				resource.TestCheckResourceAttr(data.ResourceName, "role_definition_name", "Log Analytics Reader"),
+				check.That(data.ResourceName).Key("role_definition_id").Exists(),
+				check.That(data.ResourceName).Key("role_definition_name").HasValue("Log Analytics Reader"),
 			),
 		},
 		{
@@ -221,17 +229,17 @@ func testAccRoleAssignment_managementGroup(t *testing.T) {
 }
 
 func (r RoleAssignmentResource) Exists(ctx context.Context, client *clients.Client, state *terraform.InstanceState) (*bool, error) {
-	id, err := parse.RoleDefinitionId(state.ID)
+	id, err := parse.RoleAssignmentId(state.ID)
 	if err != nil {
 		return nil, err
 	}
 
-	resp, err := client.Authorization.RoleAssignmentsClient.Get(ctx, id.Scope, id.RoleID)
+	resp, err := client.Authorization.RoleAssignmentsClient.GetByID(ctx, state.ID)
 	if err != nil {
 		if utils.ResponseWasNotFound(resp.Response) {
 			return utils.Bool(false), nil
 		}
-		return nil, fmt.Errorf("retrieving Role Assignment for role %q (Scope %q): %+v", id.RoleID, id.Scope, err)
+		return nil, fmt.Errorf("retrieving Role Assignment for role %q: %+v", id.Name, err)
 	}
 	return utils.Bool(true), nil
 }
@@ -281,7 +289,7 @@ func (RoleAssignmentResource) requiresImportConfig(id string) string {
 
 resource "azurerm_role_assignment" "import" {
   name                 = azurerm_role_assignment.test.name
-  scope                = azurerm_role_assignment.test.id
+  scope                = azurerm_role_assignment.test.scope
   role_definition_name = azurerm_role_assignment.test.role_definition_name
   principal_id         = azurerm_role_assignment.test.principal_id
 }