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

PIM RoleManagementPolicy Expiration_Admin_Assignment missing on create #2737

Open
dimitar-dimitrow opened this issue Dec 5, 2024 · 7 comments
Labels
kind/bug Some behavior is incorrect or out of spec

Comments

@dimitar-dimitrow
Copy link

dimitar-dimitrow commented Dec 5, 2024

Describe what happened

When I create a RoleManagementPolicy resource the expirationRequired: false configuration of the activeAssignmentRules is not forwarded to azure. This happens only on initial pulumi up.

The HTTP PATCH request content does not contain a rule with id - Expiration_Admin_Assignment, while it contains a rule with id - Expiration_Admin_Eligibility, which is set in similar way.

On a second pulumi up the Expiration_Admin_Assignment is included in the request and is properly updated.

Sample program

  new RoleManagementPolicy(
    `pim-role-policy-jump-box-${roleDefinition.id}`,
    {
      scope: vm_fnd_jump_box.id,
      roleDefinitionId: `/subscriptions/${azureConfig.subscriptionId}/providers/Microsoft.Authorization/roleDefinitions/${roleDefinition.id}`,
      activeAssignmentRules: {
        expirationRequired: false
      },
      eligibleAssignmentRules: {
        expirationRequired: false
      },
      activationRules: {
        requireApproval: true,
        approvalStage: {
          primaryApprovers: policy.approvers.map((approver) => ({
            objectId: approver.objectId,
            type: approver.type
          }))
        }
      },
      notificationRules: {
        activeAssignments: {
          adminNotifications: {
            notificationLevel: policy.adminNotificationLevel,
            defaultRecipients: true
          }
        },
        eligibleAssignments: {
          adminNotifications: {
            notificationLevel: policy.adminNotificationLevel,
            defaultRecipients: true
          }
        },
        eligibleActivations: {
          adminNotifications: {
            notificationLevel: policy.adminNotificationLevel,
            defaultRecipients: true
          }
        }
      }
    }
  );

Log output

I1205 09:52:27.131762 2768753 eventsink.go:59] PATCH /subscriptions/<subscription-id>/resourceGroups/test-weu-fnd-jump-box-tmp0371/providers/Microsoft.Compute/virtualMachines/vm-test-weu-fnd-jump-box-tmp0371/providers/Microsoft.Authorization/roleManagementPolicies/1c0163c0-47e6-4577-8991-ea5c82e286e4?api-version=2020-10-01 HTTP/1.1
I1205 09:52:27.131792 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>PATCH /subscriptions/<subscription-id>/resourceGroups/test-weu-fnd-jump-box-tmp0371/providers/Microsoft.Compute/virtualMachines/vm-test-weu-fnd-jump-box-tmp0371/providers/Microsoft.Authorization/roleManagementPolicies/1c0163c0-47e6-4577-8991-ea5c82e286e4?api-version=2020-10-01 HTTP/1.1<{%reset%}>)
I1205 09:52:27.131979 2768753 eventsink.go:59] Host: management.azure.com
I1205 09:52:27.132002 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>Host: management.azure.com<{%reset%}>)
I1205 09:52:27.132211 2768753 eventsink.go:59] User-Agent: HashiCorp/go-azure-sdk (Go-http-Client/1.1 rolemanagementpolicies/2020-10-01) pulumi-azure/v6.11.0 pid-a90539d8-a7a6-5826-95c4-1fbef22d4b22
I1205 09:52:27.132236 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>User-Agent: HashiCorp/go-azure-sdk (Go-http-Client/1.1 rolemanagementpolicies/2020-10-01) pulumi-azure/v6.11.0 pid-a90539d8-a7a6-5826-95c4-1fbef22d4b22<{%reset%}>)
I1205 09:52:27.132424 2768753 eventsink.go:59] Content-Length: 2202
I1205 09:52:27.132448 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>Content-Length: 2202<{%reset%}>)
I1205 09:52:27.132645 2768753 eventsink.go:59] Content-Type: application/json; charset=utf-8
I1205 09:52:27.132672 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>Content-Type: application/json; charset=utf-8<{%reset%}>)
I1205 09:52:27.132863 2768753 eventsink.go:59] X-Ms-Correlation-Request-Id: b7ac1b5f-281e-857a-362c-60c6e06961ff
I1205 09:52:27.132887 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>X-Ms-Correlation-Request-Id: b7ac1b5f-281e-857a-362c-60c6e06961ff<{%reset%}>)
I1205 09:52:27.133082 2768753 eventsink.go:59] Accept-Encoding: gzip
I1205 09:52:27.133102 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}>Accept-Encoding: gzip<{%reset%}>)
I1205 09:52:27.133296 2768753 eventsink.go:59] 
I1205 09:52:27.133318 2768753 eventsink.go:62] eventSink::Debug(<{%reset%}><{%reset%}>)
I1205 09:52:27.133505 2768753 eventsink.go:59] {"id":"/subscriptions/<subscription-id>/resourceGroups/test-weu-fnd-jump-box-tmp0371/providers/Microsoft.Compute/virtualMachines/vm-test-weu-fnd-jump-box-tmp0371/providers/Microsoft.Authorization/roleManagementPolicies/1c0163c0-47e6-4577-8991-ea5c82e286e4","name":"1c0163c0-47e6-4577-8991-ea5c82e286e4","properties":{"rules":[{"id":"Expiration_Admin_Eligibility","isExpirationRequired":false,"maximumDuration":"P365D","ruleType":"RoleManagementPolicyExpirationRule","target":{"caller":"Admin","enforcedSettings":[],"inheritableSettings":[],"level":"Eligibility","operations":["All"],"targetObjects":[]}},{"id":"Approval_EndUser_Assignment","ruleType":"RoleManagementPolicyApprovalRule","setting":{"approvalMode":"SingleStage","approvalStages":[{"primaryApprovers":[{"id":"<group-id>","userType":"Group"}]}],"isApprovalRequired":true,"isApprovalRequiredForExtension":false,"isRequestorJustificationRequired":true},"target":{"caller":"EndUser","enforcedSettings":[],"inheritableSettings":[],"level":"Assignment","operations":["All"],"targetObjects":[]}},{"id":"Notification_Admin_Admin_Eligibility","isDefaultRecipientsEnabled":true,"notificationLevel":"Critical","notificationType":"Email","recipientType":"Admin","ruleType":"RoleManagementPolicyNotificationRule","target":{"caller":"Admin","enforcedSettings":[],"inheritableSettings":[],"level":"Eligibility","operations":["All"],"targetObjects":[]}},{"id":"Notification_Admin_Admin_Assignment","isDefaultRecipientsEnabled":true,"notificationLevel":"Critical","notificationType":"Email","recipientType":"Admin","ruleType":"RoleManagementPolicyNotificationRule","target":{"caller":"Admin","enforcedSettings":[],"inheritableSettings":[],"level":"Assignment","operations":["All"],"targetObjects":[]}},{"id":"Notification_Admin_EndUser_Assignment","isDefaultRecipientsEnabled":true,"notificationLevel":"Critical","notificationType":"Email","recipientType":"Admin","ruleType":"RoleManagementPolicyNotificationRule","target":{"caller":"EndUser","enforcedSettings":[],"inheritableSettings":[],"level":"Assignment","operations":["All"],"targetObjects":[]}}]},"type":"Microsoft.Authorization/roleManagementPolicies"}

Affected Resource(s)

RoleManagementPolicy

Output of pulumi about

CLI
Version 3.130.0
Go Version go1.22.6
Go Compiler gc

Plugins
KIND NAME VERSION
resource azure 6.11.0
resource azure-native 2.65.0
resource azure-native 2.58.0
resource command 1.0.1
language nodejs unknown

Host
OS ubuntu
Version 22.04
Arch x86_64

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@dimitar-dimitrow dimitar-dimitrow added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Dec 5, 2024
@danielrbradley
Copy link
Member

danielrbradley commented Dec 6, 2024

Hi @dimitar-dimitrow thanks for getting in touch.

It's not immediately obvious at which stage this information might be being dropped. I don't see any issues in the upstream TF provider repo, but it might just have not been spotted. The next state will likely be to have to run the provider under a debugger to step through the transformations.

For reference, here's the implementation of the create in the version you're using (6.11.0):
https://github.com/hashicorp/terraform-provider-azurerm/blob/391b165/internal/services/authorization/role_management_policy_resource.go#L364-L403

One thing that does stand out is that the implementation of the update does have a lot more code around handling aspects like the activeAssignmentRules directly compared to the create. Though the create seems to call through to this code here:
https://github.com/hashicorp/terraform-provider-azurerm/blob/391b165/internal/services/authorization/role_management_policy.go#L105-L115

@danielrbradley danielrbradley removed the needs-triage Needs attention from the triage team label Dec 6, 2024
@danielrbradley
Copy link
Member

Here's what the values look like in the debugger after getting deserialized in the TF provider in the Create() method.

Image

@danielrbradley
Copy link
Member

Looking closer at the upstream code, the codepaths for the create and the update are identical.

@danielrbradley
Copy link
Member

I'm going to have to park this for now. As far as I can see, the transformation from the values in the SDK is accurate to what we pass to the upstream provider. What's not clear is how the upstream provider turns that model into the go-azure-sdk update model. This is done in the buildRoleManagementPolicyForUpdate method.

@danielrbradley danielrbradley changed the title PIM RoleManagementPolicy is not fully applied PIM RoleManagementPolicy Expiration_Admin_Assignment missing on create Dec 6, 2024
@danielrbradley
Copy link
Member

Here's the source for deciding if to add the Expiration_Admin_Assignment entry:
https://github.com/hashicorp/terraform-provider-azurerm/blob/764d84dcbb987458e3a3c0f36dcb48d3d8936af9/internal/services/authorization/role_management_policy.go#L122-L125

When evaluating HasChange(""active_assignment_rules.0.expiration_required") it returns false because the existing value (o) is has a default value of false and the new value (n) is also false:

Image

The Expiration_Admin_Eligibility doesn't have this issue because it's only checking for HasChange("eligible_assignment_rules").

It's most likely also an issue in the upstream provider. Alternatively, there could be a subtle difference between how the default value is populated by the pulumi-terraform-bridge.

danielrbradley added a commit that referenced this issue Dec 6, 2024
@dimitar-dimitrow
Copy link
Author

dimitar-dimitrow commented Dec 9, 2024

@danielrbradley, thanks for the investigation!

Indeed the culprit is the terraform provider. Those policies are generated by azure and filled with default values. As they are not really created by pulumi(terraform) provider, my suggestion was that on create they are loaded(read) from azure and than checked for changes. However, this is not the case and the default/empty golang values are used for initial comparison.

Should I proceed with raising the issue to the terraform repo?

A workaround is to specify a non empty expireAfter field for the activeAssignmentRules. It is evaluated together with expirationRequired, will not match the default golang empty string and the activeAssignmentRules will be included to the PATCH call.

@danielrbradley
Copy link
Member

I think the next step will be to confirm if it's actually a bug when running the provider via Terraform as the bridging process for Pulumi sometimes has slightly different behaviour. If the same bug appears when running via Terraform then we can log this with the provider, otherwise we can log it as an issue in the pulumi-terraform-bridge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec
Projects
None yet
Development

No branches or pull requests

2 participants