-
Notifications
You must be signed in to change notification settings - Fork 37
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
Support Authentication PIM Role Management Policies #3130
base: master
Are you sure you want to change the base?
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. New resources:
New functions:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3130 +/- ##
==========================================
- Coverage 56.77% 56.47% -0.30%
==========================================
Files 66 67 +1
Lines 8055 8177 +122
==========================================
+ Hits 4573 4618 +45
- Misses 3049 3113 +64
- Partials 433 446 +13 ☔ View full report in Codecov by Sentry. |
540d754
to
46f3a52
Compare
e608d43
to
e34db2f
Compare
var exists bool | ||
if customRes.CanCreate != nil { | ||
err = customRes.CanCreate(ctx, id) | ||
exists = err != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not existing the only kind of err?
Glanced over it but the review effort here is formidable, LMK if you need me as a reviewer, I'm happy to try again. Since there's a lot going on it can be helpful to rebase into step-by-step commits. |
Looks reasonable, except that the Delete operation in the spec isn't actually implemented. Sigh.
…rnal validation via azure SDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the status on this PR? Sorry I didn't get to a review when you put this up.
Guessing the behaviour change is still needed but we might need to rework the changes that touch the custom resources since more recent updates?
Yep, exactly that. |
About
This PR adds support for Role Management Policies, part of Privileged Identity Management (PIM) in the Microsoft.Authentication namespace. It's one part of #2455.
Note that this is about the ARM part of PIM; there's also a Microsoft Graph API part which is not covered by this provider.
This resource wasn't automatically included because it supports only GET and PATCH. The policies are singletons that cannot be created or deleted, only modified via PATCH.
Implementation
Role Management Policies essentially consist of a name which is actually a GUID, and a list of ~20 rules.
Using our existing singleton support
defaults.GetDefaultResourceState
was tricky becauseSo instead, I've implemented a custom resource that captures the original state of a policy when it's first "created", i.e., added to Pulumi state. When a rule or the whole policy is removed from Pulumi, we look up the original state and re-apply it.
Testing
The e2e/integration test for this resource is special because using PIM requires a paid Entra ID P2 license. We don't have one yet so the test fails (example). This PR looks green because the test is marked as long-running and doesn't run in PR workflows. Hence the draft status while I organize the license.