`azurerm_role_management_policy` New resource & data source #25900

oWretch · 2024-05-08T09:51:49Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Add support for managing the policies for PIM assignments to Azure resources. Based on the work in hashicorp/terraform-provider-azuread#1327.

Closes #23295 as this has not been updated for 7 months and has merge conflicts with main. I also thought code consistency would be nice between providers. Also closes #20496.

Fixes #19912, fixes #22766, fixes #23458, fixes hashicorp/terraform-provider-azuread#1186

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

terraform-provider-azurerm % make acctests SERVICE='authorization' TESTARGS='-run="TestRoleManagementPolicy(DataSource)?_"' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/authorization -run="TestRoleManagementPolicy(DataSource)?_" -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestRoleManagementPolicyDataSource_resourceGroup
=== PAUSE TestRoleManagementPolicyDataSource_resourceGroup
=== RUN   TestRoleManagementPolicyDataSource_managementGroup
=== PAUSE TestRoleManagementPolicyDataSource_managementGroup
=== RUN   TestRoleManagementPolicy_resourceGroup
=== PAUSE TestRoleManagementPolicy_resourceGroup
=== RUN   TestRoleManagementPolicy_managementGroup
=== PAUSE TestRoleManagementPolicy_managementGroup
=== CONT  TestRoleManagementPolicyDataSource_resourceGroup
=== CONT  TestRoleManagementPolicy_resourceGroup
=== CONT  TestRoleManagementPolicy_managementGroup
=== CONT  TestRoleManagementPolicyDataSource_managementGroup
--- PASS: TestRoleManagementPolicyDataSource_managementGroup (87.60s)
--- PASS: TestRoleManagementPolicy_managementGroup (126.46s)
--- PASS: TestRoleManagementPolicyDataSource_resourceGroup (134.63s)
--- PASS: TestRoleManagementPolicy_resourceGroup (241.30s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization 244.468s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_role_management_policy - new resource [azurerm_role_management_policy New resource & data source #25900]
azurerm_role_management_policy - new data source [azurerm_role_management_policy New resource & data source #25900]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

manicminer

Thanks for working on this @oWretch. Whilst this duplicates #23295, this does appear to be more developed so I'll close that one out in favor of this PR.

I've made some comments inline, if you can take a look at these I'll happily circle back for another review.

Thanks!

internal/services/authorization/parse/role_management_policy.go

internal/services/authorization/role_management_policy_data_source.go

website/docs/r/role_management_policy.html.markdown

internal/services/authorization/role_management_policy_resource.go

website/docs/r/role_management_policy.html.markdown

oWretch · 2024-06-04T10:46:45Z

@manicminer I realised today I hadn't answered all your feedback. I think I have it all updated now.

manicminer

Hi @oWretch, thanks for circling back and making those changes. The updated buildRoleManagementPolicyForUpdate() function looks great 👍

I've spent some time reviewing and testing, and whilst I originally had some more changes to request, once I starting digging a bit more into the APIs I realised that it would be dificult to communicate - so I've gone ahead and made some updates to get this ready, which I've summarized here:

I noticed that the response payloads to the List operations to retrieve the policy assignments were coming in at 4-8MB each (!!). It also seems that the SDK doesn't support filtering on the roleManagementPolicyAssignments endpoint (presumably the API specs don't advertise it), so I reworked the resource to use only the roleManagementPolicies endpoint (incidentally, the unfiltered List response payloads here are also in the multi-MB range, though a bit less than roleManagementPolicyAssignments), where we can use a $filter query parameter to filter on the role definition ID.
Doing this brings down the List responses are to a few KB, drastically improving performance of the resource, but also means that we also need to save the role definition ID to state as part of the resource ID. I've added a custom ID type RoleManagementPolicyId which captures the scope and the role_definition_id, this enables us to perform a quick search with the new FindRoleManagementPolicyId() function whenever we need to get the latest policy. Given that the policy ID changes every time it is modified, this also provides us with a stable resource ID.
The data source however, can continue to use the rolemanagementpolicies.ScopedRoleManagementPolicyId type since it does not matter if this changes at refresh time.
Validating role_definition_id is tricky since it seems certain role definitions (looking at management groups in particular) are not scoped at all, so the ID types validation in the SDK fails to parse these, as does parse.RoleDefinitionId which is used in the azurerm_role_definition resource. Some definite inconsistency from the APIs here. I've opted to forego validation here (save for ensuring it's not empty), and I will follow up with another PR to consolidate the scattered logic around role definition IDs in the authorization package - else we will end up with yet another ID type to add to the confusion.
I've updated the validation for scope to only support management groups, subscriptions and resource groups at this time. If it becomes clear that other resource types expose role management policies, we can add support for those individually (ensuring we have test coverage as we go).
I removed the display_name attribute from the resource and the data source, since in my testing this doesn't actually exist and seems to be a ghost field from the API specs.
I've added test cases for subscription policies since it's important to ensure coverage for these too.

With all that said, since I've made considerable changes, and as I have been staring at this for awhile, I'm going to request a secondary review from another contributor. Pending their review, I'm happy to merge this. Thanks again for your work on this!

…liability - Introduce a custom ID type `RoleManagementPolicyId`, so we can reduce API calls and filter on the `roleManagementPolicies` endpoint. - Add test cases for role management policies on subscriptions. - Tidy up validation across the resource and data source. - Remove `display_name` attribute as this is never returned by the API.

mbfrahry

Hey @manicminer, gave it a review and it looks good! Just had a couple comments to confirm but other than that, I think we're good!

mbfrahry · 2024-06-13T16:38:51Z

internal/services/authorization/role_management_policy.go

+					claimValue = model.ActivationRules[0].RequireConditionalAccessContext
+				}
+			} else {
+				isEnabled = false


We could simplify this by starting with isEnabled is false

Good shout, will tidy this up 👍

mbfrahry · 2024-06-13T16:46:48Z

internal/services/authorization/role_management_policy.go

+		defaultRecipients = data.DefaultRecipients
+	}
+	if recipientChange {
+		additionalRecipients = data.AdditionalRecipients


Are we losing recipient information with this? We're doing quite a bit of work here and then overwriting it if recipientChange is true. Did we want to append instead of overwrite or is this correct?

I think this is OK, on lines 560-566 we're populating with any existing additional recipients as retrieved from the policy prior to updating, and here we're selectively replacing them if the property changed in the config (it seems they'll be removed if you just don't specify them in the Update request).

manicminer · 2024-06-13T21:10:14Z

Test results

oWretch · 2024-06-13T21:57:33Z

Thanks for that thorough review @manicminer. And I agree with your sentiments - the Azure API for role management could be far better than it is. One might think three versions would be enough to sort some of that out 🙂

<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azurerm" updated from "3.107.0" to "3.108.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.108.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.108.0
FEATURES:

* **New Data Source:** `azurerm_role_management_policy` ([#25900](hashicorp/terraform-provider-azurerm#25900 **New Resource:** `azurerm_role_management_policy` ([#25900](https://github.com/hashicorp/terraform-provider-azurerm/issues/25900))

ENHANCEMENTS:

* provider: support subscription ID hinting when using Azure CLI authentication ([#26282](hashicorp/terraform-provider-azurerm#26282 `serviceconnector`: updating to use API Version `2024-04-01` ([#26248](hashicorp/terraform-provider-azurerm#26248 `azurerm_container_groups` - can now be created with a User Assigned Identity when running Windows ([#26308](hashicorp/terraform-provider-azurerm#26308 `azurerm_kubernetes_cluster` - updating the `network_profile.network_policy` property to `azure` and `calico` when it hasn't been previously set is supported ([#26176](hashicorp/terraform-provider-azurerm#26176 `azurerm_kubernetes_cluster` - respect Pod Distruption Budgets when rotating the `default_node_pool` ([#26274](hashicorp/terraform-provider-azurerm#26274 `azurerm_lb_backend_address_pool` - support for the `synchronous_mode` property ([#26309](hashicorp/terraform-provider-azurerm#26309 `azurerm_private_endpoint` - support symultaneous creation of multiple resources of this type per subnet ([#26006](https://github.com/hashicorp/terraform-provider-azurerm/issues/26006))

BUG FIXES:

* `azurerm_express_route_circuit_peering`, `azurerm_express_route_circuit`, `azurerm_express_route_gateway`, `azurerm_express_route_port` - split create and update ([#26237](hashicorp/terraform-provider-azurerm#26237 `azurerm_lb_backend_address_pool_address` - when using this resource, values are no longer reset on `azurerm_lb_backend_address_pool` ([#26264](hashicorp/terraform-provider-azurerm#26264 `azurerm_route_filter` - spliting create and update so lifecycle ignore changes works correctly ([#26266](hashicorp/terraform-provider-azurerm#26266 `azurerm_route_server` - spliting create and update so lifecycle ignore changes works correctly ([#26266](hashicorp/terraform-provider-azurerm#26266 `azurerm_synapse_workspace` - updates the client used in all operations of `azurerm_synapse_workspace_sql_aad_admin` to prevent this resource from modifying the same resource as `azurerm_synapse_workspace_aad_admin` ([#26317](hashicorp/terraform-provider-azurerm#26317 `azurerm_virtual_network` - correctly parse network securty group IDs ([#26283](https://github.com/hashicorp/terraform-provider-azurerm/issues/26283))

DEPRECATIONS:

* Data Source: `azurerm_network_interface` - the `enable_ip_forwarding` and `enable_accelerated_networking` properties have been deprecated and superseded by the `ip_forwarding_enabled` and `accelerated_networking_enabled` properties ([#26293](hashicorp/terraform-provider-azurerm#26293 `azurerm_api_management` - the `policy` block has been deprecated is superseded by the `azurerm_api_management_policy` resource ([#26305](hashicorp/terraform-provider-azurerm#26305 `azurerm_kubernetes_cluster` - the `ebpf_data_plane` property has been deprecated and superseded by the `network_data_plane` property ([#26251](hashicorp/terraform-provider-azurerm#26251 `azurerm_network_interface` - the `enable_ip_forwarding` and `enable_accelerated_networking` properties have been deprecated and superseded by the `ip_forwarding_enabled` and `accelerated_networking_enabled` properties ([#26293](hashicorp/terraform-provider-azurerm#26293 `azurerm_synapse_workspace` - the `aad_admin` and `sql_aad_admin` blocks have been deprecated and superseded by the `azurerm_synapse_workspace_aad_admin` and `azurerm_synapse_workspace_sql_aad_admin` resources ([#26317](https://github.com/hashicorp/terraform-provider-azurerm/issues/26317))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/244/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>

celsocoutinho-tangany · 2024-06-19T13:34:51Z

This is awesome! But is it fair to assume that it is currently not possible to provision azurerm_role_management_policy on a resource-level, rather than on a RG, subscription or MG level?

manicminer · 2024-06-19T13:57:48Z

@celsocoutinho-tangany Currently yes, the resource only supports management group, subscription or resource group level. We had some challenging API interactions when trying to scope more specifically than that. I'll try to get those resolved in time, once the current implementation of this resource has had a break-in period to weed out any bugs.

github-actions · 2024-07-20T02:07:12Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added dependencies service/authorization size/XL waiting-response labels May 8, 2024

oWretch force-pushed the f/pim-policies branch from 5078a1d to 502a588 Compare May 8, 2024 10:24

github-actions bot added waiting-response and removed waiting-response labels May 8, 2024

oWretch force-pushed the f/pim-policies branch from 502a588 to 4fbacb5 Compare May 8, 2024 10:32

github-actions bot removed the waiting-response label May 8, 2024

oWretch force-pushed the f/pim-policies branch from 4fbacb5 to 480554f Compare May 8, 2024 10:34

github-actions bot added the waiting-response label May 8, 2024

oWretch force-pushed the f/pim-policies branch from 480554f to a100d85 Compare May 9, 2024 02:58

github-actions bot removed the waiting-response label May 9, 2024

oWretch marked this pull request as ready for review May 9, 2024 02:58

github-actions bot added documentation waiting-response labels May 9, 2024

oWretch force-pushed the f/pim-policies branch from a100d85 to 789028f Compare May 9, 2024 03:02

github-actions bot removed the waiting-response label May 9, 2024

manicminer suggested changes May 9, 2024

View reviewed changes

github-actions bot added the waiting-response label May 9, 2024

manicminer mentioned this pull request May 9, 2024

feat: role management policy resource #23295

Closed

github-actions bot removed the waiting-response label May 9, 2024

oWretch requested a review from manicminer May 9, 2024 22:44

This was referenced May 21, 2024

Manage azure role settings hashicorp/terraform-provider-azuread#1186

Closed

Add support to modify EntraID directory role settings in PIM hashicorp/terraform-provider-azuread#1390

Open

github-actions bot added the waiting-response label Jun 4, 2024

oWretch added 3 commits June 4, 2024 22:49

Add role management policies clients

fa7303d

Create azurerm_role_management_policy resource

b074787

Create azurerm_role_management_policy data source

27e9c1b

oWretch added 3 commits June 4, 2024 22:49

Add documentation

433fcba

Update based on code review

63a426d

Update based on feedback

c494515

oWretch force-pushed the f/pim-policies branch from be4c521 to c494515 Compare June 4, 2024 10:49

github-actions bot added waiting-response and removed waiting-response labels Jun 4, 2024

Update deps

22b839a

github-actions bot added waiting-response and removed waiting-response labels Jun 4, 2024

Fix go linting

77fbaa0

github-actions bot removed the waiting-response label Jun 4, 2024

manicminer approved these changes Jun 13, 2024

View reviewed changes

mbfrahry approved these changes Jun 13, 2024

View reviewed changes

github-actions bot added the waiting-response label Jun 13, 2024

azurerm_role_management_policy: address review comments

f0b8cc0

github-actions bot removed the waiting-response label Jun 13, 2024

azurerm_role_management_policy: fix up test names, update docs

e4d9281

manicminer merged commit 2f13594 into hashicorp:main Jun 13, 2024
33 checks passed

github-actions bot added this to the v3.108.0 milestone Jun 13, 2024

manicminer added a commit that referenced this pull request Jun 13, 2024

Changelog for #25900

fa66cdb

oWretch deleted the f/pim-policies branch June 13, 2024 21:57

kbcz1989 mentioned this pull request Jun 17, 2024

update role_management_policy import docs #26356

Merged

14 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_role_management_policy` New resource & data source #25900

`azurerm_role_management_policy` New resource & data source #25900

oWretch commented May 8, 2024 •

edited by rcskosir

Loading

manicminer left a comment

oWretch commented Jun 4, 2024

manicminer left a comment •

edited

Loading

mbfrahry left a comment

mbfrahry Jun 13, 2024

manicminer Jun 13, 2024

mbfrahry Jun 13, 2024

manicminer Jun 13, 2024

manicminer commented Jun 13, 2024

oWretch commented Jun 13, 2024

celsocoutinho-tangany commented Jun 19, 2024

manicminer commented Jun 19, 2024

github-actions bot commented Jul 20, 2024

azurerm_role_management_policy New resource & data source #25900

azurerm_role_management_policy New resource & data source #25900

Conversation

oWretch commented May 8, 2024 • edited by rcskosir Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

manicminer left a comment

Choose a reason for hiding this comment

oWretch commented Jun 4, 2024

manicminer left a comment • edited Loading

Choose a reason for hiding this comment

mbfrahry left a comment

Choose a reason for hiding this comment

mbfrahry Jun 13, 2024

Choose a reason for hiding this comment

manicminer Jun 13, 2024

Choose a reason for hiding this comment

mbfrahry Jun 13, 2024

Choose a reason for hiding this comment

manicminer Jun 13, 2024

Choose a reason for hiding this comment

manicminer commented Jun 13, 2024

oWretch commented Jun 13, 2024

celsocoutinho-tangany commented Jun 19, 2024

manicminer commented Jun 19, 2024

github-actions bot commented Jul 20, 2024

`azurerm_role_management_policy` New resource & data source #25900

`azurerm_role_management_policy` New resource & data source #25900

oWretch commented May 8, 2024 •

edited by rcskosir

Loading

manicminer left a comment •

edited

Loading