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: New module avm/ptn/authorization/pim-role-assignment #4431

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

sebassem
Copy link
Contributor

@sebassem sebassem commented Feb 11, 2025

Description

New pattern for PIM role assignments

  • @Azure/avm-core-team-technical-bicep TODO: Add custom secret 'userPrinicipalId' to AVM CI using user "AVM CI Validation User 001"

Pipeline Reference

Pipeline
avm.ptn.authorization.pim-role-assignment

Type of Change

  • Update to CI Environment or utilities (Non-module affecting changes)
  • Azure Verified Module updates:
    • Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
      • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
      • The bug was found by the module author, and no one has opened an issue to report it yet.
    • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
    • Breaking changes and I have bumped the MAJOR version in version.json.
    • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • I have run Set-AVMModule locally to generate the supporting module files.
  • My corresponding pipelines / checks run clean and green without any errors or warnings

…ncies and adding new test modules for various scopes
…' to 'Contributor' for consistency across documentation and tests
@sebassem sebassem requested review from a team as code owners February 11, 2025 12:26
@avm-team-linter avm-team-linter bot added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Feb 11, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 Maintainers need to triage still label Feb 11, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue label Feb 11, 2025
Comment on lines +9 to +10
/avm/ptn/authorization/role-assignment/ @Azure/avm-ptn-authorization-roleassignment-module-owners-bicep @Azure/avm-module-reviewers-bicep
/avm/ptn/authorization/pim-role-assignment/ @Azure/avm-ptn-authorization-pimroleassignment-module-owners-bicep @Azure/avm-module-reviewers-bicep
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there one two many lines? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry I don't get the problem with this one 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently shows errors as the teams are either not existing - or are not yet confirmed in the hierachy of teams (documented in the contribution guide)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the teams are created, I can see them on GitHub teams. So maybe not approved yet ?

Comment on lines 141 to 145
$null = New-AzRoleAssignmentScheduleRequest -Name $guid `
-Scope $scope `
-PrincipalId $pimRoleAssignmentPrinicpalId `
-RequestType AdminRemove `
-RoleDefinitionId $pimRoleAssignmentRoleDefinitionId
Copy link
Contributor

@AlexanderSehr AlexanderSehr Feb 12, 2025

Choose a reason for hiding this comment

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

Suggested change
$null = New-AzRoleAssignmentScheduleRequest -Name $guid `
-Scope $scope `
-PrincipalId $pimRoleAssignmentPrinicpalId `
-RequestType AdminRemove `
-RoleDefinitionId $pimRoleAssignmentRoleDefinitionId
# To remove a scheduled request, a 'AdminRemove' request must be submitted
$removalInputObject = @{
Name = $guid
Scope = $scope
PrincipalId = $pimRoleAssignmentPrinicpalId
RequestType = 'AdminRemove'
RoleDefinitionId = $pimRoleAssignmentRoleDefinitionId
}
$null = New-AzRoleAssignmentScheduleRequest @removalInputObject

Splatting (using the backticks) is a bit of a 'debated' practice 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update to the other case :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

Comment on lines 18 to 20
@description('Required. Principle ID of the user. This value is tenant-specific and must be stored in the CI Key Vault in a secret named \'userPrinicipalId\'.')
@secure()
param userPrinicipalId string = ''
Copy link
Contributor

@AlexanderSehr AlexanderSehr Feb 13, 2025

Choose a reason for hiding this comment

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

Suggested change
@description('Required. Principle ID of the user. This value is tenant-specific and must be stored in the CI Key Vault in a secret named \'userPrinicipalId\'.')
@secure()
param userPrinicipalId string = ''
@description('Required. Principle ID of the user. This value is tenant-specific and must be stored in the CI Key Vault in a secret named \'CI-testUserObjectId\'.')
@secure()
param testUserObjectIdstring = ''

Would recommend to rename the secret slightly. Also, please apply the update in the description to all test files using this secret. It must have the CI- prefix

Copy link
Contributor

Choose a reason for hiding this comment

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

Ref:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Just created the secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

@sebassem sebassem requested review from a team as code owners February 13, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Core Team 🧞 This item needs the AVM Core Team to review it Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants