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

feat: create azurerm_pim_active_role_assignment and azurerm_pim_eligible_role_assignment resource #20731

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

josh-barker
Copy link
Contributor

@josh-barker josh-barker commented Mar 2, 2023

This PR creates 2 x resources for Azure PIM role assignments - eligible and active

It relates to #20496

There is a test case that depends on the above PR so that the role settings can be set to allow permanent assignments.

FYI @manicminer

@josh-barker josh-barker force-pushed the feat/pim-role-assignment branch 5 times, most recently from 63b25b7 to b881ebe Compare March 6, 2023 01:00
@josh-barker josh-barker marked this pull request as ready for review March 6, 2023 01:11
@josh-barker josh-barker force-pushed the feat/pim-role-assignment branch from b881ebe to fd10160 Compare March 6, 2023 01:13
@manicminer manicminer self-requested a review March 6, 2023 13:41
@manicminer manicminer self-assigned this Mar 6, 2023
@ealdaon

This comment was marked as off-topic.

@stephybun stephybun assigned katbyte and unassigned manicminer Jun 12, 2023
@katbyte
Copy link
Collaborator

katbyte commented Jun 13, 2023

@josh-barker & @ealdaon - we are currently waiting on some AAD licences so we can properly test this, hopefully within the next few weeks

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.

Ran the tests today and they are failing with:

  Error: retrieving Role Management Policy: (Principal Id "3aa04c8c-5a75-4e5e-9117-1b7cf6f33e21" / Scope "/subscriptions/*******/resourceGroups/acctestRG-230614155046681271/providers/Microsoft.Network/virtualNetworks/amtestVNET1-230614155046681271" / Role Definition Id "/subscriptions/*******/providers/Microsoft.Authorization/roleDefinitions/3e5e47e6-65f7-47ef-90b5-e5dd4d455f24"): roleassignmentschedulerequests.RoleAssignmentScheduleRequestsClient#Get: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InsufficientPermissions" Message="The requestor 3aa04c8c-5a75-4e5e-9117-1b7cf6f33e21 does not have permissions for this request. Please use $filter=asTarget() to filter on the requestor's assignments."

not sure if this is something we can configure/fix in the test config?

@josh-barker
Copy link
Contributor Author

Hey @katbyte , I've run some tests and have found that the service principal needs either Owner or
User Access Administrator + Contributor, as it is creating resources & role assignments.

I saw the same error as you when it didn't have those permissions.

Let me know if you need anything else!

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 @josh-barker - we've got the tests passing now so LGTM! 🏗️

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.

Was doing one last skim and spotted one minor charge we should make: ticket block properties can drop the ticket_ otherwise we would have ticket.ticket_*
once thats done this should be good to merge for this weeks release

website/docs/r/pim_eligible_role_assignment.html.markdown Outdated Show resolved Hide resolved
website/docs/r/pim_active_role_assignment.html.markdown Outdated Show resolved Hide resolved
@josh-barker josh-barker force-pushed the feat/pim-role-assignment branch from 6fcabb5 to 892772f Compare June 28, 2023 23:51
@josh-barker josh-barker requested a review from katbyte June 29, 2023 00:18
@josh-barker
Copy link
Contributor Author

Hey @katbyte , I had to rebase from main and update some of the code to match the latest sdk generation. Let me know if you need any more adjustments on that.

Thanks!

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 @josh-barker ! LGTM now 🌩️

@katbyte katbyte merged commit d769727 into hashicorp:main Jul 6, 2023
@github-actions github-actions bot added this to the v3.64.0 milestone Jul 6, 2023
katbyte added a commit that referenced this pull request Jul 6, 2023
@josh-barker
Copy link
Contributor Author

Thanks @katbyte & @manicminer for your help on this one!!

@josh-barker josh-barker deleted the feat/pim-role-assignment branch July 7, 2023 05:14
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024
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.

4 participants