feat(service): deploy application in container apps #1303
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several Bicep files for deploying a container application across different environments (production, staging, test) and updates an existing module to support user-assigned identities. The main Bicep file defines parameters for deployment, including environment variables and resource configurations. Parameter files for production, staging, and test environments are created, allowing for dynamic configuration based on environment variables. Additionally, the workflow file is updated to include a new deployment job for the service, enhancing deployment capabilities. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
.azure/applications/service/prod.bicepparam (1)
8-12: LGTM: Secure handling of sensitive parameters.The secret parameters are appropriately defined and read from environment variables, which is a good security practice. The parameters cover essential resources for the container app deployment, including key vault, container app environment, app insights, and app configuration.
This approach aligns with the PR objective of using a user-assigned identity instead of a managed identity, which should help mitigate potential race conditions during deployment.
Consider adding a brief comment explaining the purpose of each parameter for improved clarity and maintainability. For example:
// Name of the Azure Key Vault containing environment-specific secrets param environmentKeyVaultName = readEnvironmentVariable('AZURE_ENVIRONMENT_KEY_VAULT_NAME').azure/applications/service/test.bicepparam (2)
5-6: LGTM: Good use of environment variables. Consider adding default values.The use of environment variables for
imageTagandrevisionSuffixis a good practice for dynamic configuration. To improve robustness, consider adding default values:param imageTag = readEnvironmentVariable('IMAGE_TAG', 'latest') param revisionSuffix = readEnvironmentVariable('REVISION_SUFFIX', '')This ensures the deployment won't fail if these environment variables are not set, while still allowing for dynamic configuration.
8-12: LGTM: Good use of environment variables for configuration. Some improvements suggested.
The use of environment variables for configuration is a good practice for security and flexibility.
Consider updating the comment to better reflect the nature of these parameters:
// Azure resource configuration
- To improve robustness, consider adding default values or error handling for missing environment variables. For example:
param environmentKeyVaultName = readEnvironmentVariable('AZURE_ENVIRONMENT_KEY_VAULT_NAME', '') param containerAppEnvironmentName = readEnvironmentVariable('AZURE_CONTAINER_APP_ENVIRONMENT_NAME', '') param appInsightConnectionString = readEnvironmentVariable('AZURE_APP_INSIGHTS_CONNECTION_STRING', '') param appConfigurationName = readEnvironmentVariable('AZURE_APP_CONFIGURATION_NAME', '') // Add validation @assert(environmentKeyVaultName != '', 'AZURE_ENVIRONMENT_KEY_VAULT_NAME must be set') @assert(containerAppEnvironmentName != '', 'AZURE_CONTAINER_APP_ENVIRONMENT_NAME must be set') @assert(appInsightConnectionString != '', 'AZURE_APP_INSIGHTS_CONNECTION_STRING must be set') @assert(appConfigurationName != '', 'AZURE_APP_CONFIGURATION_NAME must be set')This approach provides clear error messages if required environment variables are not set.
.azure/applications/service/main.bicep (1)
154-154: Finalize the use of user-assigned identitiesThere's a TODO comment indicating that once all container apps use user-assigned identities, the comment should be removed. Since this app is already using a user-assigned identity, consider addressing this TODO.
Apply this diff to remove the TODO comment:
userAssignedIdentityId: managedIdentity.id - // TODO: Once all container apps use user-assigned identities, remove this comment and ensure userAssignedIdentityId is always provided
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- .azure/applications/service/main.bicep (1 hunks)
- .azure/applications/service/prod.bicepparam (1 hunks)
- .azure/applications/service/staging.bicepparam (1 hunks)
- .azure/applications/service/test.bicepparam (1 hunks)
- .azure/modules/containerApp/main.bicep (3 hunks)
- .github/workflows/workflow-deploy-apps.yml (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
.azure/applications/service/prod.bicepparam (3)
1-1: LGTM: Correct module import.The import statement correctly references the main Bicep module using a relative path, which is appropriate for maintaining file structure consistency.
3-6: LGTM: Well-structured environment-specific parameters.The parameters are appropriately defined:
- Hardcoding 'environment' and 'location' is suitable for a production-specific file.
- Using environment variables for 'imageTag' and 'revisionSuffix' enables dynamic configuration during deployment.
The use of 'REVISION_SUFFIX' aligns with the PR objective of mitigating race conditions during deployment, which is a good practice.
1-12: Overall assessment: Well-structured and aligned with PR objectives.This Bicep parameter file for the production environment is well-structured and follows best practices. It effectively uses a combination of hardcoded values and environment variables to configure the deployment, which aligns well with the PR objectives of enhancing deployment capabilities and addressing potential race conditions.
The file covers all necessary parameters for production deployment, including environment-specific settings and secure handling of sensitive information. The use of environment variables for dynamic configuration and secrets is a good practice that enhances flexibility and security.
To ensure consistency across environments, let's verify the existence of similar parameter files for other environments:
✅ Verification successful
Environment parameter files are consistent across all environments.
All necessary Bicep parameter files for
stagingandtestenvironments exist alongside theprodconfiguration. This ensures consistency and reliability across different deployment environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the existence of similar Bicep parameter files for other environments # Test: Check for staging and test parameter files for env in staging test; do if [ -f ".azure/applications/service/${env}.bicepparam" ]; then echo "${env}.bicepparam exists" else echo "${env}.bicepparam does not exist" fi doneLength of output: 284
.azure/applications/service/test.bicepparam (1)
1-1: LGTM: Correct import of the main Bicep file.The import statement is correctly using a relative path to reference the main Bicep file, following best practices for modular Bicep deployments.
.azure/applications/service/staging.bicepparam (4)
1-1: LGTM: Correct file import.The main Bicep file is properly imported using the 'using' statement, which follows Bicep best practices.
3-4: LGTM: Appropriate environment and location parameters.The 'environment' and 'location' parameters are correctly defined for the staging environment. The use of 'norwayeast' as the location is valid and aligns with the PR objectives.
9-12: LGTM: Secure parameter management for sensitive information.The use of environment variables for sensitive parameters (key vault, container app environment, app insights, and app configuration) is a good security practice and aligns with the PR objectives.
Ensure that all required environment variables are correctly set in your CI/CD pipeline. You can verify this by running the following script in your pipeline:
#!/bin/bash # Description: Verify that required environment variables are set required_vars=( "AZURE_ENVIRONMENT_KEY_VAULT_NAME" "AZURE_CONTAINER_APP_ENVIRONMENT_NAME" "AZURE_APP_INSIGHTS_CONNECTION_STRING" "AZURE_APP_CONFIGURATION_NAME" ) for var in "${required_vars[@]}"; do if [ -z "${!var}" ]; then echo "Error: $var is not set" exit 1 else echo "$var is set (value hidden for security)" fi done echo "All required environment variables are set"Would you like assistance in updating the documentation to reflect these new environment variables and their purpose in the deployment process?
5-6: LGTM: Dynamic parameters for flexible deployment.The use of environment variables for 'imageTag' and 'revisionSuffix' allows for dynamic configuration, which enhances deployment capabilities as per the PR objectives.
Ensure that the 'IMAGE_TAG' and 'REVISION_SUFFIX' environment variables are correctly set in your CI/CD pipeline. You can verify this by running the following script in your pipeline:
.github/workflows/workflow-deploy-apps.yml (1)
Line range hint
1-324: Summary of changes and considerationsThe addition of the new "service" to the deployment matrix is the only change in this workflow file. This modification aligns with the PR objectives and extends the existing deployment process to include the new service.
Consider the following points for the overall implementation:
- Ensure that all necessary Bicep files and configurations for the new service are in place and properly tested.
- Update any relevant documentation to reflect the addition of this new service to the deployment process.
- Consider adding a brief description or comment in the workflow file to explain the purpose of the new service for future reference.
- After deployment, verify that the new service is functioning correctly in all environments.
.azure/modules/containerApp/main.bicep (5)
34-36: OptionaluserAssignedIdentityIdparameter added correctlyThe introduction of the
userAssignedIdentityIdparameter with a default empty string is appropriate. This allows for optional usage of user-assigned identities until it becomes a requirement for all container apps.
57-65: Conditional identity configuration is well-implementedThe
identityConfigvariable correctly sets the identity type based on whetheruserAssignedIdentityIdis provided. This ensures that the container app uses a system-assigned identity by default or a user-assigned identity when specified.
69-69: Identity configuration included in container app resourceAdding
identity: identityConfigto thecontainerAppresource correctly applies the identity settings as per the configured identity type.
95-97: Existing user-assigned identity referenced appropriatelyThe
managedIdentityresource correctly references the existing user-assigned managed identity whenuserAssignedIdentityIdis provided. Extracting the name usinglast(split(userAssignedIdentityId, '/'))is an effective method.
99-99: OutputidentityPrincipalIdconditionally returns the correct principal IDThe output for
identityPrincipalIdappropriately selects the principal ID from the system-assigned identity or the user-assigned identity based on the presence ofuserAssignedIdentityId..azure/applications/service/main.bicep (1)
57-74: Verify Application Insights connection string handlingEnsure that the
appInsightConnectionStringparameter is correctly being passed and used securely. Since it's sensitive information, confirm that it is stored and referenced properly without exposure.Consider running the following script to check for any inadvertent exposures:
| param environment = 'test' | ||
| param location = 'norwayeast' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using environment variables for environment and location.
While the current setup is functional, using environment variables for environment and location parameters could provide more flexibility:
param environment = readEnvironmentVariable('ENVIRONMENT', 'test')
param location = readEnvironmentVariable('AZURE_LOCATION', 'norwayeast')This approach allows for easier configuration changes without modifying the Bicep file, while still maintaining default values.
There was a problem hiding this comment.
the bicepparam file is only used for that environment, so would be overkill to supply environment too right?
I'll make a note of AZURE_LOCATION for later, good point!
There was a problem hiding this comment.
@arealmaas, you make a great point—it might be unnecessary to supply the environment parameter via an environment variable since the bicepparam file is only used for that specific environment. Thanks for considering the suggestion about AZURE_LOCATION!
(_/_)
(•︵•)
✏️ Learnings added
Learnt from: arealmaas
PR: digdir/dialogporten#1303
File: .azure/applications/service/test.bicepparam:3-4
Timestamp: 2024-10-16T08:43:00.537Z
Learning: When working with environment-specific bicepparam files, it's unnecessary to supply the `environment` parameter via environment variables because each bicepparam file is only used for that specific environment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @secure() | ||
| param containerAppEnvironmentName string |
There was a problem hiding this comment.
Remove @secure() from non-sensitive parameter
The containerAppEnvironmentName parameter is annotated with @secure(), which is typically used for sensitive data like passwords or connection strings. Since this parameter represents a resource name and is not sensitive, consider removing the @secure() attribute to avoid unnecessary restrictions.
Apply this diff to remove the @secure() attribute:
@description('The name of the container app environment')
@minLength(3)
-@secure()
param containerAppEnvironmentName string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @secure() | |
| param containerAppEnvironmentName string | |
| @description('The name of the container app environment') | |
| @minLength(3) | |
| param containerAppEnvironmentName string |
| output name string = containerApp.outputs.name | ||
| output revisionName string = containerApp.outputs.revisionName |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider outputting the managed identity principal ID
If other resources need to reference the managed identity's principal ID, it might be helpful to output it.
You can add the following output:
output name string = containerApp.outputs.name
output revisionName string = containerApp.outputs.revisionName
+output managedIdentityPrincipalId string = managedIdentity.properties.principalIdCommittable suggestion was skipped due to low confidence.
|
🤖 I have created a release *beep* *boop* --- ## [1.25.0](v1.24.0...v1.25.0) (2024-10-17) ### Features * **applications:** add scalers for cpu and memory ([#1295](#1295)) ([eb0f19b](eb0f19b)) * **infrastructure:** create new yt01 app environment ([#1291](#1291)) ([1a1ccc0](1a1ccc0)) * **service:** add permissions for service-bus ([#1305](#1305)) ([7bf4177](7bf4177)) * **service:** deploy application in container apps ([#1303](#1303)) ([a309044](a309044)) ### Bug Fixes * **applications:** add missing property for scale configuration ([3ffb724](3ffb724)) * **applications:** use correct scale configuration ([#1311](#1311)) ([b8fb3cc](b8fb3cc)) * Fix ID-porten acr claim parsing ([#1299](#1299)) ([8b8862f](8b8862f)) * **service:** ensure default credentials work ([#1306](#1306)) ([b1e6a14](b1e6a14)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Documentation