`arm_role_assignment`: add `principal_type` and `skip_service_principal_aad_check` properties #4168

WodansSon · 2019-08-27T23:50:48Z

Including the PrincipalType parameter will eliminate a race condition where sometimes if you try to create an identity and then assign it immediately, the assignment fails because the identity hasn’t replicated in AAD yet.

tombuildsstuff

hi @jeffreyCline

Thanks for this PR - I've left some comments inline but this otherwise LGTM.

Thanks!

azurerm/resource_arm_role_assignment_test.go

tombuildsstuff · 2019-08-28T09:24:47Z

azurerm/resource_arm_role_assignment.go

+					string(authorization.Group),
+					string(authorization.MSI),
+					string(authorization.ServicePrincipal),
+					string(authorization.Unknown),


presumably Unknown shouldn't be in this list?

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

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)

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

website/docs/r/role_assignment.html.markdown

katbyte

Thanks for the PR @jeffreyCline,

I've left some comments inline that i think should be addressed

azurerm/resource_arm_role_assignment.go

katbyte · 2019-08-28T20:05:27Z

azurerm/resource_arm_role_assignment.go

@@ -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 != "" {


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

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

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.

Okie-dokie... fixed. :)

katbyte · 2019-08-28T20:06:16Z

azurerm/resource_arm_role_assignment_test.go

+  name                 = "%s"
+  scope                = "${data.azurerm_subscription.current.id}"
+  role_definition_name = "Reader"
+	principal_id         = "${azuread_service_principal.test.id}"


Could we fi the formatting here?

@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."

switching from tabs to spaces gives you that error??

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

azurerm/resource_arm_role_assignment_test.go

katbyte · 2019-08-28T20:10:19Z

Could you also explain how exposing this property fixing AD replication eventually consistency? Those two things seem unrelated.

Co-Authored-By: Tom Harvey <[email protected]>

Co-Authored-By: kt <[email protected]>

…ders/terraform-provider-azurerm into e_principal-type

WodansSon · 2019-08-28T23:05:47Z

Could you also explain how exposing this property fixing AD replication eventually consistency? Those two things seem unrelated.

per conversation with the service team: "...there is one more parameter that I recommend you set on the role assignment create, including this parameter will eliminate a race condition where sometimes if you try to create an identity and then assign it immediately, the assignment fails because the identity hasn’t replicated in AAD yet.

In a REST call to ARM you would include:

“principalType”:”ServicePrincipal”

… in the body of the PUT request to create the role assignment. I’m not sure what that translates to Terraform-wise."

tombuildsstuff · 2019-08-29T15:28:12Z

@jeffreyCline

..there is one more parameter that I recommend you set on the role assignment create, including this parameter will eliminate a race condition where sometimes if you try to create an identity and then assign it immediately, the assignment fails because the identity hasn’t replicated in AAD yet.

Sounds like we want to default this to ServicePrincipal then?

WodansSon · 2019-08-30T00:46:09Z

@tombuildsstuff

Sounds like we want to default this to ServicePrincipal then?

This attribute maps to the type of the Principal. If you are dealing with newly provisioned managed identities then setting to ServicePrincipal should be fine. In other cases, setting this to ServicePrincipal can lead to errors. I would suggest setting it only where needed. This is only needed for cases where principal is newly created and used within say 2-5 mins. of creation.

tombuildsstuff

hey @jeffreyCline

Thanks for pushing those changes - this now LGTM but I had one question around whether we could infer this field rather than making users specify it, WDYT?

Thanks!

azurerm/resource_arm_role_assignment.go

tombuildsstuff · 2019-09-04T08:46:13Z

azurerm/resource_arm_role_assignment.go

@@ -64,6 +64,13 @@ func resourceArmRoleAssignment() *schema.Resource {
 				Required: true,
 				ForceNew: true,
 			},
+
+			"skip_service_principal_aad_check": {


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?

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.

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.

WodansSon · 2019-09-04T21:07:10Z

hey @jeffreyCline
Thanks for pushing those changes - this now LGTM but I had one question around whether we could infer this field rather than making users specify it, WDYT?
Thanks!

If we wanted to do that then I believe we would have to do a get on the service principal against AAD and see if it comes back with something or not, if the service principal returns not found we could then add this value to the call automatically and avoid having this value at all. Just depends on how you want to do it.

katbyte

Aside from two minor comments LGTM now @jeffreyCline

katbyte · 2019-09-07T00:10:56Z

website/docs/r/role_assignment.html.markdown

 ## 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.


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`.

katbyte · 2019-09-07T00:17:13Z

azurerm/resource_arm_role_assignment_test.go

-			"sp":    testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal,
-			"group": testAccAzureRMActiveDirectoryServicePrincipal_group,
+			"sp":     testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipal,
+			"spType": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType,


this might be better as:

Suggested change

"spType": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType,

"spSkip": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithAadSkip,

add `pricipal_type` and `skip_servicepricipal_aad_check` properties

ghost · 2019-09-18T12:11:17Z

This has been released in version 1.34.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.34.0"
}
# ... other configuration ...

ghost · 2019-10-07T14:50:44Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

Expose Principal Type attribute

855ee7e

WodansSon added enhancement documentation service/roles labels Aug 27, 2019

ghost added the size/M label Aug 27, 2019

WodansSon requested a review from katbyte August 27, 2019 23:52

tombuildsstuff suggested changes Aug 28, 2019

View reviewed changes

tombuildsstuff added the waiting-response label Aug 28, 2019

tombuildsstuff added this to the v1.34.0 milestone Aug 28, 2019

katbyte requested changes Aug 28, 2019

View reviewed changes

WodansSon and others added 4 commits August 28, 2019 14:56

Update azurerm/resource_arm_role_assignment_test.go

84029ed

Co-Authored-By: Tom Harvey <[email protected]>

Update azurerm/resource_arm_role_assignment_test.go

19e6594

Co-Authored-By: kt <[email protected]>

Updates per PR comments

c988716

Merge branch 'e_principal-type' of https://github.com/terraform-provi…

b10b749

…ders/terraform-provider-azurerm into e_principal-type

WodansSon removed the waiting-response label Aug 28, 2019

Missed a comma

18bdc0f

Revert name change

808509e

tombuildsstuff added the waiting-response label Aug 29, 2019

WodansSon removed the waiting-response label Aug 30, 2019

WodansSon added 6 commits August 29, 2019 19:37

Update per PR review

9daeb7a

Change of direction

13173f9

Removing unused resource

fc6eecb

added set to read CRUD

7b3452d

Update read

934b93c

gofmt

1d42907

tombuildsstuff approved these changes Sep 4, 2019

View reviewed changes

WodansSon added 2 commits September 4, 2019 14:30

Changes per PR comments

1e3a823

Added principal_type as a computed attribute

865796d

katbyte approved these changes Sep 7, 2019

View reviewed changes

WodansSon changed the title ~~Enhancement arm_role_assignment: Expose Principal Type attribute~~ arm_role_assignment: add pricipal_type and skip_servicepricipal_aad_check properties Sep 7, 2019

WodansSon merged commit 06eae21 into master Sep 7, 2019

WodansSon deleted the e_principal-type branch September 7, 2019 02:05

WodansSon added a commit that referenced this pull request Sep 7, 2019

arm_role_assignment [GH-4168]

9b7536b

add `pricipal_type` and `skip_servicepricipal_aad_check` properties

WodansSon changed the title ~~arm_role_assignment: add pricipal_type and skip_servicepricipal_aad_check properties~~ arm_role_assignment: add principall_type and skip_service_principal_aad_check properties Sep 7, 2019

WodansSon changed the title ~~arm_role_assignment: add principall_type and skip_service_principal_aad_check properties~~ arm_role_assignment: add principal_type and skip_service_principal_aad_check properties Sep 7, 2019

tdmalone mentioned this pull request Sep 20, 2019

Addition of skip_service_principal_aad_check in v1.34.0 forces recreation of azurerm_role_assignment #4393

Closed

ghost locked and limited conversation to collaborators Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`arm_role_assignment`: add `principal_type` and `skip_service_principal_aad_check` properties #4168

`arm_role_assignment`: add `principal_type` and `skip_service_principal_aad_check` properties #4168

WodansSon commented Aug 27, 2019

tombuildsstuff left a comment

tombuildsstuff Aug 28, 2019

WodansSon Aug 28, 2019

tombuildsstuff Aug 29, 2019

WodansSon Aug 30, 2019

katbyte left a comment

katbyte Aug 28, 2019

WodansSon Aug 28, 2019

katbyte Aug 29, 2019

WodansSon Aug 30, 2019

katbyte Aug 28, 2019

WodansSon Aug 28, 2019

WodansSon Aug 28, 2019

katbyte Aug 29, 2019

WodansSon Aug 29, 2019

katbyte commented Aug 28, 2019

WodansSon commented Aug 28, 2019

tombuildsstuff commented Aug 29, 2019

WodansSon commented Aug 30, 2019

tombuildsstuff left a comment

tombuildsstuff Sep 4, 2019

katbyte Sep 4, 2019 •

edited

Loading

WodansSon Sep 5, 2019

WodansSon commented Sep 4, 2019

katbyte left a comment

katbyte Sep 7, 2019

katbyte Sep 7, 2019

ghost commented Sep 18, 2019

ghost commented Oct 7, 2019

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

	"spType": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithType,
	"spSkip": testAccAzureRMActiveDirectoryServicePrincipal_servicePrincipalWithAadSkip,

arm_role_assignment: add principal_type and skip_service_principal_aad_check properties #4168

arm_role_assignment: add principal_type and skip_service_principal_aad_check properties #4168

Conversation

WodansSon commented Aug 27, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Aug 28, 2019

WodansSon commented Aug 28, 2019

tombuildsstuff commented Aug 29, 2019

WodansSon commented Aug 30, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon commented Sep 4, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 18, 2019

ghost commented Oct 7, 2019

`arm_role_assignment`: add `principal_type` and `skip_service_principal_aad_check` properties #4168

`arm_role_assignment`: add `principal_type` and `skip_service_principal_aad_check` properties #4168

katbyte Sep 4, 2019 •

edited

Loading