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.

Sign up for GitHub

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

arm_role_assignment: add principal_type and skip_service_principal_aad_check properties #4168

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
855ee7e
Expose Principal Type attribute
WodansSon Aug 27, 2019
84029ed
Update azurerm/resource_arm_role_assignment_test.go
WodansSon Aug 28, 2019
19e6594
Update azurerm/resource_arm_role_assignment_test.go
WodansSon Aug 28, 2019
c988716
Updates per PR comments
WodansSon Aug 28, 2019
b10b749
Merge branch 'e_principal-type' of https://github.com/terraform-provi…
WodansSon Aug 28, 2019
18bdc0f
Missed a comma
WodansSon Aug 28, 2019
808509e
Revert name change
WodansSon Aug 28, 2019
9daeb7a
Update per PR review
WodansSon Aug 30, 2019
13173f9
Change of direction
WodansSon Aug 30, 2019
fc6eecb
Removing unused resource
WodansSon Aug 30, 2019
7b3452d
added set to read CRUD
WodansSon Aug 30, 2019
934b93c
Update read
WodansSon Aug 30, 2019
1d42907
gofmt
WodansSon Aug 31, 2019
1e3a823
Changes per PR comments
WodansSon Sep 4, 2019
865796d
Added principal_type as a computed attribute
WodansSon Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 37 additions & 4 deletions azurerm/resource_arm_role_assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
)

Expand Down Expand Up @@ -64,6 +65,24 @@ func resourceArmRoleAssignment() *schema.Resource {
Required: true,
ForceNew: true,
},

"principal_type": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
string(authorization.Application),
string(authorization.DirectoryObjectOrGroup),
string(authorization.DirectoryRoleTemplate),
string(authorization.Everyone),
string(authorization.ForeignGroup),
string(authorization.Group),
string(authorization.MSI),
string(authorization.ServicePrincipal),
string(authorization.Unknown),
Copy link
Contributor

Choose a reason for hiding this comment

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

presumably Unknown shouldn't be in this list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it was on purpose Unknown is actually a valid value,

Copy link
Contributor

Choose a reason for hiding this comment

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

just because it's in the list supported by the API doesn't mean it should be exposed to users unfortunately, Unknown is generally present to indicate a value that's unsupported on this API version (for example if it's been created on a newer/older version which is no longer supported)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tombuildsstuff I have asked the service team about the expected behavior and the purpose of the Unknown value.

string(authorization.User),
}, false),
},
},
}
}
Expand Down Expand Up @@ -118,11 +137,19 @@ func resourceArmRoleAssignmentCreate(d *schema.ResourceData, meta interface{}) e
}
}

roleAssignmentProperties := authorization.RoleAssignmentProperties{
RoleDefinitionID: utils.String(roleDefinitionId),
PrincipalID: utils.String(principalId),
}

principalType := d.Get("principal_type").(string)

if principalType != "" {
roleAssignmentProperties.PrincipalType = authorization.PrincipalType(principalType)
}

properties := authorization.RoleAssignmentCreateParameters{
RoleAssignmentProperties: &authorization.RoleAssignmentProperties{
RoleDefinitionID: utils.String(roleDefinitionId),
PrincipalID: utils.String(principalId),
},
RoleAssignmentProperties: &roleAssignmentProperties,
}

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

principalType := d.Get("principal_type").(string)

if principalType != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we just mark the property as computed instead so its populated with whatever the default is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is my understanding that this needs to be defined by the user,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we shouldn't need this check at all? because right now we don't set it, the new property is optional with no default, so the user has to explicitly set it. And if in the case of the user not setting it there is nothing to read back then? If there is we can just mark it as computed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okie-dokie... fixed. :)

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 {
roleResp, err := roleDefinitionsClient.GetByID(ctx, *roleId)
Expand Down
50 changes: 47 additions & 3 deletions azurerm/resource_arm_role_assignment_test.go
View file
Open in desktop
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 @@ -219,6 +220,27 @@ func testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal(t *testing.T
})
}

func testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType(t *testing.T) {
resourceName := "azurerm_role_assignment.test"
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"),
resource.TestCheckResourceAttr(resourceName, "principal_type", "ServicePrincipal"),
),
},
},
})
}

func testAccAzureRMActiveDirectoryServicePrincipal_group(t *testing.T) {
ri := tf.AccRandTimeInt()
id := uuid.New().String()
Expand Down Expand Up @@ -440,6 +462,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}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we fi the formatting here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kt I take that back... no we can't, if you do you get this error:

 Error: authorization.RoleAssignmentsClient#Create: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidRoleAssignmentId" Message="The role assignment ID 'acctestRA21652c15-15eb-45b3-8a6c-b31feaa9e2df' is not valid. The role assignment ID must be a GUID."

Copy link
Collaborator

Choose a reason for hiding this comment

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

switching from tabs to spaces gives you that error??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, switching form %s to testacc%s gives me that error.

principal_type = "ServicePrincipal"
}
`, rInt, roleAssignmentID)
}

func testAccAzureRMRoleAssignment_group(rInt int, roleAssignmentID string) string {
return fmt.Sprintf(`
data "azurerm_subscription" "current" {}
Expand All @@ -448,7 +492,7 @@ resource "azuread_group" "test" {
name = "acctestspa-%d"
}

resource "azurerm_role_assignment" "test" {
resource ServicePrincipal {
name = "%s"
scope = "${data.azurerm_subscription.current.id}"
role_definition_name = "Reader"
Expand Down
3 changes: 3 additions & 0 deletions website/docs/r/role_assignment.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ resource "azurerm_role_assignment" "test" {
scope = "${data.azurerm_subscription.primary.id}"
role_definition_name = "Reader"
principal_id = "${data.azurerm_client_config.test.service_principal_object_id}"
principal_type = "ServicePrincipal"
}
```

Expand Down Expand Up @@ -133,6 +134,8 @@ The following arguments are supported:

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

* `PrincipalType` - (Optional) The principal type of the assigned `principal_id`. Possible values include: `User`, `Group`, `ServicePrincipal`, `Unknown`, `DirectoryRoleTemplate`, `ForeignGroup`, `Application`, `MSI`, `DirectoryObjectOrGroup`, `Everyone`.

## Attributes Reference

The following attributes are exported:
Expand Down