Fixing destroy when role scope is a Management Group

azurerm/internal/services/authorization/role_definition_resource.go

@@ -97,7 +97,8 @@ func resourceArmRoleDefinition() *schema.Resource {
 
 			"assignable_scopes": {
 				Type:     schema.TypeList,
-				Required: true,
+				Optional: true,
+				Computed: true,
 				Elem: &schema.Schema{
 					Type: schema.TypeString,
 				},
@@ -184,13 +185,17 @@ func resourceArmRoleDefinitionRead(d *schema.ResourceData, meta interface{}) err
 	}
 
 	if id := resp.ID; id != nil {
-		roleDefinitionId, err := parseRoleDefinitionId(*id)
-		if err != nil {
-			return fmt.Errorf("Error parsing Role Definition ID: %+v", err)
-		}
-		if roleDefinitionId != nil {
-			d.Set("scope", roleDefinitionId.scope)
-			d.Set("role_definition_id", roleDefinitionId.roleDefinitionId)
+		if resp.RoleDefinitionProperties != nil && resp.RoleDefinitionProperties.AssignableScopes != nil {
+			roleDefinitionId, err := parseRoleDefinitionId(*id, *resp.RoleDefinitionProperties.AssignableScopes)
+			if err != nil {
+				return fmt.Errorf("Error parsing Role Definition ID: %+v", err)
+			}
+			if roleDefinitionId != nil {
+				d.Set("scope", roleDefinitionId.scope)
+				d.Set("role_definition_id", roleDefinitionId.roleDefinitionId)
+			}
+		} else {
+			return fmt.Errorf("assignable scopes not found for Role Definition with id %q", d.Id())
 		}
 	}
 
@@ -217,7 +222,9 @@ func resourceArmRoleDefinitionDelete(d *schema.ResourceData, meta interface{}) e
 	ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d)
 	defer cancel()
 
-	id, err := parseRoleDefinitionId(d.Id())
+	scope := d.Get("scope").(string)
+
+	id, err := parseRoleDefinitionId(d.Id(), []string{scope})
 	if err != nil {
 		return err
 	}
@@ -277,9 +284,15 @@ func expandRoleDefinitionPermissions(d *schema.ResourceData) []authorization.Per
 func expandRoleDefinitionAssignableScopes(d *schema.ResourceData) []string {
 	scopes := make([]string, 0)
 
+	// The first scope in the list must be the target scope as it it not returned in any API call
+	assignedScope := d.Get("scope").(string)
+	scopes = append(scopes, assignedScope)
 	assignableScopes := d.Get("assignable_scopes").([]interface{})
 	for _, scope := range assignableScopes {
-		scopes = append(scopes, scope.(string))
+		// Ensure the assigned scope is not duplicated in the list if also specified in `assignable_scopes`
+		if scope != assignedScope {
+			scopes = append(scopes, scope.(string))
+		}
 	}
 
 	return scopes
@@ -346,16 +359,17 @@ type roleDefinitionId struct {
 	roleDefinitionId string
 }
 
-func parseRoleDefinitionId(input string) (*roleDefinitionId, error) {
+func parseRoleDefinitionId(input string, scopes []string) (*roleDefinitionId, error) {
 	segments := strings.Split(input, "/providers/Microsoft.Authorization/roleDefinitions/")
+
 	if len(segments) != 2 {
 		return nil, fmt.Errorf("Expected Role Definition ID to be in the format `{scope}/providers/Microsoft.Authorization/roleDefinitions/{name}` but got %q", input)
 	}
 
-	// {scope}/providers/Microsoft.Authorization/roleDefinitions/{roleDefinitionId}
 	id := roleDefinitionId{
-		scope:            segments[0],
+		scope:            scopes[0],
 		roleDefinitionId: segments[1],
 	}
+
 	return &id, nil
 }

azurerm/internal/services/authorization/tests/role_definition_resource_test.go

@@ -26,12 +26,7 @@ func TestAccAzureRMRoleDefinition_basic(t *testing.T) {
 					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
 				),
 			},
-			{
-				ResourceName:            data.ResourceName,
-				ImportState:             true,
-				ImportStateVerify:       true,
-				ImportStateVerifyIgnore: []string{"role_definition_id", "scope"},
-			},
+			data.ImportStep(),
 		},
 	})
 }
@@ -91,23 +86,16 @@ func TestAccAzureRMRoleDefinition_update(t *testing.T) {
 				Config: testAccAzureRMRoleDefinition_basic(id, data),
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.0", "*"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.not_actions.#", "0"),
 				),
 			},
+			data.ImportStep(),
 			{
 				Config: testAccAzureRMRoleDefinition_updated(id, data),
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.0", "*"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.not_actions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.not_actions.0", "Microsoft.Authorization/*/read"),
 				),
 			},
+			data.ImportStep(),
 		},
 	})
 }
@@ -124,23 +112,16 @@ func TestAccAzureRMRoleDefinition_updateEmptyId(t *testing.T) {
 				Config: testAccAzureRMRoleDefinition_emptyId(data),
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.0", "*"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.not_actions.#", "0"),
 				),
 			},
+			data.ImportStep(),
 			{
 				Config: testAccAzureRMRoleDefinition_updateEmptyId(data),
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.actions.0", "*"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.not_actions.#", "1"),
-					resource.TestCheckResourceAttr(data.ResourceName, "permissions.0.not_actions.0", "Microsoft.Authorization/*/read"),
 				),
 			},
+			data.ImportStep(),
 		},
 	})
 }
@@ -157,10 +138,28 @@ func TestAccAzureRMRoleDefinition_emptyName(t *testing.T) {
 				Config: testAccAzureRMRoleDefinition_emptyId(data),
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
-					resource.TestCheckResourceAttrSet(data.ResourceName, "id"),
-					resource.TestCheckResourceAttrSet(data.ResourceName, "name"),
 				),
 			},
+			data.ImportStep(),
+		},
+	})
+}
+
+func TestAccAzureRMRoleDefinition_managementGroup(t *testing.T) {
+	data := acceptance.BuildTestData(t, "azurerm_role_definition", "test")
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { acceptance.PreCheck(t) },
+		Providers:    acceptance.SupportedProviders,
+		CheckDestroy: testCheckAzureRMRoleDefinitionDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAzureRMRoleDefinition_managementGroup(uuid.New().String(), data),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMRoleDefinitionExists(data.ResourceName),
+				),
+			},
+			data.ImportStep("scope"),
 		},
 	})
 }
@@ -235,10 +234,6 @@ resource "azurerm_role_definition" "test" {
     actions     = ["*"]
     not_actions = []
   }
-
-  assignable_scopes = [
-    data.azurerm_subscription.primary.id,
-  ]
 }
 `, id, data.RandomInteger)
 }
@@ -369,3 +364,33 @@ resource "azurerm_role_definition" "test" {
 }
 `, data.RandomInteger)
 }
+
+func testAccAzureRMRoleDefinition_managementGroup(id string, data acceptance.TestData) string {
+	return fmt.Sprintf(`
+provider "azurerm" {
+  features {}
+}
+
+data "azurerm_subscription" "primary" {
+}
+
+resource "azurerm_management_group" "test" {
+}
+
+resource "azurerm_role_definition" "test" {
+  role_definition_id = "%s"
+  name               = "acctestrd-%d"
+  scope              = azurerm_management_group.test.id
+
+  permissions {
+    actions     = ["*"]
+    not_actions = []
+  }
+
+  assignable_scopes = [
+    azurerm_management_group.test.id,
+    data.azurerm_subscription.primary.id,
+  ]
+}
+`, id, data.RandomInteger)
+}

website/docs/r/role_definition.html.markdown

@@ -47,7 +47,9 @@ The following arguments are supported:
 
 * `permissions` - (Required) A `permissions` block as defined below.
 
-* `assignable_scopes` - (Required) One or more assignable scopes for this Role Definition, such as `/subscriptions/0b1f6471-1bf0-4dda-aec3-111122223333`, `/subscriptions/0b1f6471-1bf0-4dda-aec3-111122223333/resourceGroups/myGroup`, or `/subscriptions/0b1f6471-1bf0-4dda-aec3-111122223333/resourceGroups/myGroup/providers/Microsoft.Compute/virtualMachines/myVM`.
+* `assignable_scopes` - (Optional) One or more assignable scopes for this Role Definition, such as `/subscriptions/0b1f6471-1bf0-4dda-aec3-111122223333`, `/subscriptions/0b1f6471-1bf0-4dda-aec3-111122223333/resourceGroups/myGroup`, or `/subscriptions/0b1f6471-1bf0-4dda-aec3-111122223333/resourceGroups/myGroup/providers/Microsoft.Compute/virtualMachines/myVM`.
+
+~> **NOTE:** The value for `scope` is automatically included in this list.
 
 A `permissions` block as the following properties: