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

chore: use user assigned identity for all applications #1580

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

arealmaas
Copy link
Collaborator

@arealmaas arealmaas commented Dec 10, 2024

Description

Related Issue(s)

  • #N/A

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

  • Documentation is updated (either in docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)

@arealmaas arealmaas requested review from a team as code owners December 10, 2024 15:02
Copy link
Contributor

coderabbitai bot commented Dec 10, 2024

Warning

Rate limit exceeded

@arealmaas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 33 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between f358855 and 506ad43.

📒 Files selected for processing (1)
  • .azure/applications/sync-resource-policy-information-job/main.bicep (3 hunks)
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several changes across multiple Bicep files to implement managed identities for various Azure applications. Each application configuration is updated to include a new managed identity resource, enhancing security by allowing applications to authenticate to Azure services without managing credentials. Environment variables are modified to reference the new managed identity's client ID, and access policies are updated to use the managed identity's principal ID instead of previous outputs. This ensures that the correct identity is utilized for resource access.

Changes

File Path Change Summary
.azure/applications/graphql/main.bicep Added managedIdentity resource; updated userAssignedIdentityId, principalIds for access policies.
.azure/applications/sync-subject-resource-mappings-job/main.bicep Added managedIdentity resource; updated userAssignedIdentityId, principalIds, and output for identityPrincipalId.
.azure/applications/web-api-eu/main.bicep Added managedIdentity resource; updated userAssignedIdentityId, principalIds for access policies.
.azure/applications/web-api-migration-job/main.bicep Added managedIdentity resource; updated userAssignedIdentityId, principalIds, and output for identityPrincipalId.
.azure/applications/web-api-so/main.bicep Added managedIdentity resource; updated userAssignedIdentityId, principalIds for access policies.
.azure/modules/containerApp/main.bicep Updated userAssignedIdentityId to be required; simplified identity configuration logic; updated output for identityPrincipalId.
.azure/modules/containerAppJob/main.bicep Added userAssignedIdentityId parameter; modified identity assignment logic; updated output for identityPrincipalId.

Possibly related PRs

Suggested reviewers

  • oskogstad
  • MagnusSandgren

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
.azure/applications/web-api-eu/main.bicep (1)

63-67: LGTM! Managed identity properly implemented for Web API EU application.

The implementation follows the established pattern consistently:

  1. Managed identity resource
  2. Azure SDK environment variable
  3. Container app integration
  4. Key Vault and App Configuration access policies

The consistent implementation across all applications is commendable as it simplifies maintenance and reduces the likelihood of configuration errors.

The consistent pattern used across all applications for implementing managed identities is a good architectural choice. This uniformity:

  • Simplifies maintenance
  • Makes it easier to onboard new team members
  • Reduces the likelihood of configuration errors
  • Makes it easier to automate and script infrastructure changes

Also applies to: 86-89, 172-172, 180-180, 188-188

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c5f3bfb and 6a38937.

📒 Files selected for processing (7)
  • .azure/applications/graphql/main.bicep (3 hunks)
  • .azure/applications/sync-subject-resource-mappings-job/main.bicep (3 hunks)
  • .azure/applications/web-api-eu/main.bicep (3 hunks)
  • .azure/applications/web-api-migration-job/main.bicep (2 hunks)
  • .azure/applications/web-api-so/main.bicep (3 hunks)
  • .azure/modules/containerApp/main.bicep (3 hunks)
  • .azure/modules/containerAppJob/main.bicep (3 hunks)
🧰 Additional context used
📓 Learnings (4)
.azure/applications/graphql/main.bicep (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1303
File: .azure/applications/service/main.bicep:116-120
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In the Bicep file `.azure/applications/service/main.bicep`, the `managedIdentity` resource is created alongside the container app and does not exist prior to deployment.
.azure/applications/web-api-migration-job/main.bicep (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1303
File: .azure/applications/service/main.bicep:116-120
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In the Bicep file `.azure/applications/service/main.bicep`, the `managedIdentity` resource is created alongside the container app and does not exist prior to deployment.
.azure/applications/web-api-eu/main.bicep (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1303
File: .azure/applications/service/main.bicep:116-120
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In the Bicep file `.azure/applications/service/main.bicep`, the `managedIdentity` resource is created alongside the container app and does not exist prior to deployment.
.azure/applications/web-api-so/main.bicep (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1303
File: .azure/applications/service/main.bicep:116-120
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In the Bicep file `.azure/applications/service/main.bicep`, the `managedIdentity` resource is created alongside the container app and does not exist prior to deployment.
🔇 Additional comments (23)
.azure/applications/web-api-so/main.bicep (5)

91-96: Managed Identity Resource Declaration

The managedIdentity resource is correctly declared and will be created alongside the container app. This adheres to best practices for using user-assigned managed identities in Azure deployments.


114-117: Adding AZURE_CLIENT_ID Environment Variable

The environment variable AZURE_CLIENT_ID is correctly set to the managed identity's client ID. This ensures that the application can authenticate using the user-assigned managed identity.


173-173: Passing User-Assigned Identity ID to Container App Module

The userAssignedIdentityId parameter is appropriately passed to the containerApp module. This links the container app to the newly created managed identity, enabling it to utilize the managed identity for authentication.


181-181: Updating Key Vault Access Policy Principal IDs

The principalIds parameter for the Key Vault reader access policy is correctly updated to use the managed identity's principalId. This grants the necessary access permissions to the managed identity.


189-189: Updating App Configuration Access Policy Principal IDs

Similarly, the principalIds parameter for the App Configuration reader access policy is updated to use the managed identity's principalId. This ensures the managed identity has the required access to App Configuration.

.azure/modules/containerAppJob/main.bicep (4)

28-31: Adding userAssignedIdentityId Parameter

The userAssignedIdentityId parameter is correctly added as a required parameter with a minimum length constraint. This allows the module to accept an ID for a user-assigned managed identity.


49-52: Referencing Existing Managed Identity Resource

The managedIdentity resource is referenced as an existing resource using the provided userAssignedIdentityId. This ensures that the job uses the specified user-assigned managed identity.


57-60: Configuring Job Identity to Use User-Assigned Identity

The identity configuration for the job resource is correctly set to 'UserAssigned', and the userAssignedIdentities are properly specified. This enables the job to authenticate using the provided managed identity.


86-86: Updating Output identityPrincipalId

The output identityPrincipalId is updated to reference managedIdentity.properties.principalId. This change ensures that the output provides the principal ID of the user-assigned managed identity.

.azure/applications/web-api-migration-job/main.bicep (5)

37-42: Adding Managed Identity Resource

The managedIdentity resource is correctly declared to create a user-assigned managed identity for the migration job. This follows best practices for secure authentication.


48-51: Setting AZURE_CLIENT_ID Environment Variable

The environment variable AZURE_CLIENT_ID is appropriately set to the managed identity's client ID. This allows the migration job to utilize the managed identity for authentication.


75-75: Passing User-Assigned Identity ID to Migration Job Module

The userAssignedIdentityId parameter is correctly passed to the migrationJob module, linking the job to the newly created managed identity.


83-83: Updating Key Vault Access Policy Principal IDs

The principalIds parameter in the keyVaultReaderAccessPolicy module is updated to use managedIdentity.properties.principalId. This grants the managed identity the necessary access to Key Vault.


87-87: Updating Output identityPrincipalId

The output identityPrincipalId is updated to managedIdentity.properties.principalId, ensuring that it references the principal ID of the user-assigned managed identity.

.azure/modules/containerApp/main.bicep (4)

61-63: Making userAssignedIdentityId Parameter Required

The userAssignedIdentityId parameter is now required with a minimum length constraint. This change enforces the provision of a user-assigned managed identity ID when using this module.


84-85: Referencing Existing Managed Identity Resource

The managedIdentity resource is correctly referenced as an existing resource using the userAssignedIdentityId. This ensures the container app utilizes the specified managed identity.


91-96: Configuring Container App to Use User-Assigned Identity

The identity configuration of the containerApp resource is correctly set to 'UserAssigned', and the userAssignedIdentities are properly specified. This links the container app to the managed identity for authentication.


119-119: Updating Output identityPrincipalId

The output identityPrincipalId now references managedIdentity.properties.principalId, ensuring consistency with the use of the user-assigned managed identity.

.azure/applications/sync-subject-resource-mappings-job/main.bicep (4)

49-53: LGTM! Managed identity resource properly configured.

The managed identity resource is correctly defined with appropriate naming convention and inherits location and tags from parameters.


72-75: LGTM! Environment variable correctly configured for managed identity.

The AZURE_CLIENT_ID environment variable is properly set to use the managed identity's client ID, following Azure SDK conventions.


107-107: LGTM! Container app properly configured to use managed identity.

The container app module is correctly configured to use the managed identity through the userAssignedIdentityId parameter.


115-115: LGTM! Access policy and output properly updated for managed identity.

The Key Vault access policy and output are correctly updated to use the managed identity's principal ID.

Also applies to: 119-119

.azure/applications/graphql/main.bicep (1)

64-68: LGTM! Managed identity properly implemented for GraphQL application.

All components are correctly configured:

  1. Managed identity resource with proper naming
  2. Environment variable for Azure SDK authentication
  3. Container app integration
  4. Access policies for both Key Vault and App Configuration

The implementation follows the same pattern as other applications, maintaining consistency across the codebase.

Also applies to: 83-86, 170-170, 178-178, 186-186

oskogstad
oskogstad previously approved these changes Dec 11, 2024
@arealmaas arealmaas merged commit fb740cd into main Dec 20, 2024
26 checks passed
@arealmaas arealmaas deleted the chore/user-assigned-identity branch December 20, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants