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

Conversation

WodansSon
Copy link
Collaborator

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.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @jeffreyCline

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

Thanks!

azurerm/resource_arm_role_assignment_test.go Outdated Show resolved Hide resolved
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.

website/docs/r/role_assignment.html.markdown Outdated Show resolved Hide resolved
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jeffreyCline,

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

azurerm/resource_arm_role_assignment.go Outdated Show resolved Hide resolved
azurerm/resource_arm_role_assignment.go Outdated Show resolved Hide resolved
azurerm/resource_arm_role_assignment.go Outdated Show resolved Hide resolved
@@ -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. :)

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.

azurerm/resource_arm_role_assignment_test.go Outdated Show resolved Hide resolved
@katbyte
Copy link
Collaborator

katbyte commented Aug 28, 2019

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

@WodansSon
Copy link
Collaborator Author

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
Copy link
Contributor

@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
Copy link
Collaborator Author

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

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -64,6 +64,13 @@ func resourceArmRoleAssignment() *schema.Resource {
Required: true,
ForceNew: 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.

@WodansSon
Copy link
Collaborator Author

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.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Aside from two minor comments LGTM now @jeffreyCline

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

"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,

@WodansSon 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 WodansSon merged commit 06eae21 into master Sep 7, 2019
@WodansSon WodansSon deleted the e_principal-type branch September 7, 2019 02:05
WodansSon added a commit that referenced this pull request Sep 7, 2019
add `pricipal_type` and `skip_servicepricipal_aad_check` properties
@WodansSon 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 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
@ghost
Copy link

ghost commented Sep 18, 2019

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
Copy link

ghost commented Oct 7, 2019

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!

@ghost ghost locked and limited conversation to collaborators Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants