Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arm_role_assignment: add principal_type and skip_service_principal_aad_check properties #4168

Merged
merged 15 commits into from
Sep 7, 2019
Merged
19 changes: 19 additions & 0 deletions azurerm/resource_arm_role_assignment.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ func resourceArmRoleAssignment() *schema.Resource {
Required: true,
ForceNew: true,
},

"principal_type": {
Type: schema.TypeString,
Computed: true,
},

"skip_service_principal_aad_check": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about this, I'm fine with this approach, but is there any way that we could look this information up so that users don't need to specify it?

Copy link
Collaborator

@katbyte katbyte Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this is all a hack around broken AAD replication, and that there might be other reasons to set the type property (or at the least consume it) i'm more inclined to just expose the SDK/API type field and users can set it as desired.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the service team:

  • The only valid values that are publicly exposed by our APIs are: User, Group, and ServicePrincipal.

  • This property is more relevant on the read path than write. For writes, the only relevant scenario is for ServicePrincipal. I see no value add in specifying PrincipalType as User or Group. The service anyways checks against AAD and fails the call if the principal is not found or the principal type in AAD doesn’t match what the request specified. Only for ServicePrincipal, if AAD returns principal not found, then role assignment create still proceeds in the new api-version.

  • Don’t set this parameter unless explicitly dealing with newly created service principal.

  • As stated above this attribute makes more sense in the read scenario, the API will return User, Group, ServicePrincipal based on the type of the principal in AAD.

Type: schema.TypeBool,
Optional: true,
ForceNew: true,
Default: false,
},
},
}
}
Expand Down Expand Up @@ -125,6 +137,12 @@ func resourceArmRoleAssignmentCreate(d *schema.ResourceData, meta interface{}) e
},
}

skipPrincipalCheck := d.Get("skip_service_principal_aad_check").(bool)

if skipPrincipalCheck {
properties.RoleAssignmentProperties.PrincipalType = authorization.ServicePrincipal
}

if err := resource.Retry(300*time.Second, retryRoleAssignmentsClient(scope, name, properties, meta)); err != nil {
return err
}
Expand Down Expand Up @@ -163,6 +181,7 @@ func resourceArmRoleAssignmentRead(d *schema.ResourceData, meta interface{}) err
d.Set("scope", props.Scope)
d.Set("role_definition_id", props.RoleDefinitionID)
d.Set("principal_id", props.PrincipalID)
d.Set("principal_type", props.PrincipalType)

//allows for import when role name is used (also if the role name changes a plan will show a diff)
if roleId := props.RoleDefinitionID; roleId != nil {
Expand Down
73 changes: 66 additions & 7 deletions azurerm/resource_arm_role_assignment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ func TestAccAzureRMRoleAssignment(t *testing.T) {
"requiresImport": testAccAzureRMRoleAssignment_requiresImport,
},
"assignment": {
"sp": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal,
"group": testAccAzureRMActiveDirectoryServicePrincipal_group,
"sp": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal,
"spType": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be better as:

Suggested change
"spType": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType,
"spSkip": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithAadSkip,

"group": testAccAzureRMActiveDirectoryServicePrincipal_group,
},
"management": {
"assign": testAccAzureRMRoleAssignment_managementGroup,
Expand Down Expand Up @@ -64,6 +65,9 @@ func testAccAzureRMRoleAssignment_emptyName(t *testing.T) {
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"skip_service_principal_aad_check",
},
},
},
})
Expand All @@ -90,6 +94,9 @@ func testAccAzureRMRoleAssignment_roleName(t *testing.T) {
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"skip_service_principal_aad_check",
},
},
},
})
Expand Down Expand Up @@ -145,6 +152,9 @@ func testAccAzureRMRoleAssignment_dataActions(t *testing.T) {
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"skip_service_principal_aad_check",
},
},
},
})
Expand All @@ -169,6 +179,9 @@ func testAccAzureRMRoleAssignment_builtin(t *testing.T) {
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"skip_service_principal_aad_check",
},
},
},
})
Expand All @@ -195,12 +208,16 @@ func testAccAzureRMRoleAssignment_custom(t *testing.T) {
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"skip_service_principal_aad_check",
},
},
},
})
}

func testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal(t *testing.T) {
resourceName := "azurerm_role_assignment.test"
ri := tf.AccRandTimeInt()
id := uuid.New().String()

Expand All @@ -211,6 +228,26 @@ func testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal(t *testing.T
Steps: []resource.TestStep{
{
Config: testAccAzureRMRoleAssignment_servicePrincipal(ri, id),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMRoleAssignmentExists("azurerm_role_assignment.test"),
resource.TestCheckResourceAttr(resourceName, "principal_type", "ServicePrincipal"),
),
},
},
})
}

func testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType(t *testing.T) {
ri := tf.AccRandTimeInt()
id := uuid.New().String()

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMRoleAssignmentDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMRoleAssignment_servicePrincipalWithType(ri, id),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMRoleAssignmentExists("azurerm_role_assignment.test"),
),
Expand Down Expand Up @@ -440,6 +477,28 @@ resource "azurerm_role_assignment" "test" {
`, rInt, roleAssignmentID)
}

func testAccAzureRMRoleAssignment_servicePrincipalWithType(rInt int, roleAssignmentID string) string {
return fmt.Sprintf(`
data "azurerm_subscription" "current" {}

resource "azuread_application" "test" {
name = "acctestspa-%d"
}

resource "azuread_service_principal" "test" {
application_id = "${azuread_application.test.application_id}"
}

resource "azurerm_role_assignment" "test" {
name = "%s"
scope = "${data.azurerm_subscription.current.id}"
role_definition_name = "Reader"
principal_id = "${azuread_service_principal.test.id}"
skip_service_principal_aad_check = true
}
`, rInt, roleAssignmentID)
}

func testAccAzureRMRoleAssignment_group(rInt int, roleAssignmentID string) string {
return fmt.Sprintf(`
data "azurerm_subscription" "current" {}
Expand All @@ -464,17 +523,17 @@ data "azurerm_subscription" "primary" {}
data "azurerm_client_config" "test" {}

data "azurerm_role_definition" "test" {
name = "Monitoring Reader"
name = "Monitoring Reader"
}

resource "azurerm_management_group" "test" {
group_id = "%s"
group_id = "%s"
}

resource "azurerm_role_assignment" "test" {
scope = "${azurerm_management_group.test.id}"
role_definition_id = "${data.azurerm_role_definition.test.id}"
principal_id = "${data.azurerm_client_config.test.service_principal_object_id}"
scope = "${azurerm_management_group.test.id}"
role_definition_id = "${data.azurerm_role_definition.test.id}"
principal_id = "${data.azurerm_client_config.test.service_principal_object_id}"
}
`, groupId)
}
4 changes: 4 additions & 0 deletions website/docs/r/role_assignment.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,16 @@ The following arguments are supported:

~> **NOTE:** The Principal ID is also known as the Object ID (ie not the "Application ID" for applications).

* `skip_service_principal_aad_check` - (Optional) If the `principal_id` is a newly provisioned `Service Principal` set this value to `true` to skip the `Azure Active Directory` check which may fail due to replication lag. This argument is only valid if the `principal_id` is a `Service Principal` identity. If it is not a `Service Principal` identity it will cause the role assignment to fail. Defaults to `false`.

## Attributes Reference

The following attributes are exported:

* `id` - The Role Assignment ID.

* `principal_type` - The type of the `principal_id`, e.g. User, Group, Service Principal, Application, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave this as:

Suggested change
* `principal_type` - The type of the `principal_id`, e.g. User, Group, Service Principal, Application, etc.
* `principal_type` - The type of the `principal_id`.


## Import

Role Assignments can be imported using the `resource id`, e.g.
Expand Down