Update module #28
Update module #28
Conversation
|
Warning Rate limit exceeded@spy86 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (5)
WalkthroughThe changes significantly expand the configuration and feature set for an Azure Container Registry Terraform module. The resource definition, variables, and outputs are refactored to support advanced options such as georeplication, network rules, policies, identities, and encryption. Numerous new variables and dynamic blocks are introduced to enable conditional and complex configurations, especially for Premium SKU features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terraform
participant AzureRM
User->>Terraform: Configure variables (including advanced features)
Terraform->>AzureRM: Create/Update azurerm_container_registry with dynamic blocks
AzureRM-->>Terraform: Provisioned Container Registry (with optional features)
Terraform-->>User: Output admin credentials and registry info (conditionally)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
variables.tf (3)
16-23: Make SKU validation case-insensitive.Users often pass
"premium"or"STANDARD".contains()is case-sensitive, so the current rule will reject otherwise valid input.- validation { - condition = contains(["Basic", "Standard", "Premium"], var.acr_tier) - error_message = "The SKU must be one of: Basic, Standard, Premium." - } + validation { + condition = contains(["basic", "standard", "premium"], lower(var.acr_tier)) + error_message = "The SKU must be one of: Basic, Standard, Premium (case-insensitive)." + }
68-76: Apply the same case-insensitive pattern tonetwork_rule_bypass_option.Keeps behaviour consistent across variables and avoids needless errors.
89-97: Consider IP/CIDR validation fornetwork_rule_set.ip_rules.Passing an invalid IP string will only surface during
apply. A simple regex orcan(cidrhost())-based test can catch formatting mistakes early.acr.tf (1)
71-77: Encryption block requires Premium SKU & Customer-Managed Key prerequisites.Provider will reject this block unless:
skuisPremium- The Key Vault key has
cryptopermissions granted to the registry’s identity.Consider adding documentation or validation to prevent mis-configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
acr.tf(1 hunks)output.tf(1 hunks)variables.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
acr.tf
[MEDIUM] 1-80: Enable vulnerability scanning for container images.
(CKV_AZURE_163)
[MEDIUM] 1-80: Ensure geo-replicated container registries to match multi-region container deployments.
(CKV_AZURE_165)
[MEDIUM] 1-80: Ensures that ACR uses signed/trusted images
(CKV_AZURE_164)
🔇 Additional comments (2)
variables.tf (1)
116-123: Validateidentity.typeagainst provider-supported values.
SystemAssigned,UserAssigned, orSystemAssigned, UserAssignedare the only accepted strings. Add avalidationblock to fail fast on typos.acr.tf (1)
31-45: Null-safe loops for empty IP/VNet rule lists.If
ip_rulesorvirtual_network_rulesisnull, Terraform will error with “Invalid for_each argument”. Usetoset()or a coalesce to empty list.-for_each = network_rule_set.value.ip_rules +for_each = network_rule_set.value.ip_rules != null ? network_rule_set.value.ip_rules : []Apply the same pattern to
virtual_network_rules.
acr.tf
Outdated
| dynamic "georeplication" { | ||
| for_each = var.georeplications | ||
| content { | ||
| location = georeplication.value.location | ||
| zone_redundancy_enabled = georeplication.value.zone_redundancy_enabled | ||
| regional_endpoint_enabled = georeplication.value.regional_endpoint_enabled | ||
| tags = merge(var.default_tags, georeplication.value.tags) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard georeplication against non-Premium SKUs.
georeplication is only valid when sku = "Premium". Without a conditional, a non-Premium deploy will fail.
-for_each = var.georeplications
+for_each = var.acr_tier == "Premium" ? var.georeplications : []📝 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.
| dynamic "georeplication" { | |
| for_each = var.georeplications | |
| content { | |
| location = georeplication.value.location | |
| zone_redundancy_enabled = georeplication.value.zone_redundancy_enabled | |
| regional_endpoint_enabled = georeplication.value.regional_endpoint_enabled | |
| tags = merge(var.default_tags, georeplication.value.tags) | |
| } | |
| } | |
| dynamic "georeplication" { | |
| - for_each = var.georeplications | |
| + for_each = var.acr_tier == "Premium" ? var.georeplications : [] | |
| content { | |
| location = georeplication.value.location | |
| zone_redundancy_enabled = georeplication.value.zone_redundancy_enabled | |
| regional_endpoint_enabled = georeplication.value.regional_endpoint_enabled | |
| tags = merge(var.default_tags, georeplication.value.tags) | |
| } | |
| } |
🤖 Prompt for AI Agents
In acr.tf around lines 15 to 23, the georeplication block is applied
unconditionally, but it should only be included when the SKU is "Premium". Add a
conditional expression to the dynamic "georeplication" block to ensure it only
executes if var.sku equals "Premium", preventing deployment failures for
non-Premium SKUs.
| output "admin_username" { | ||
| description = "The Username associated with the Container Registry Admin account - if the admin account is enabled." | ||
| value = azurerm_container_registry.main.admin_username | ||
| sensitive = true | ||
| value = azurerm_container_registry.main.admin_enabled ? azurerm_container_registry.main.admin_username : null | ||
| sensitive = false | ||
| } |
There was a problem hiding this comment.
admin_username should still be marked sensitive.
Although a username is less critical than a password, Azure classifies it as sensitive and the provider already flags it so. Overriding it to false leaks the value into state diff, logs, and remote outputs.
-output "admin_username" {
- description = "The Username associated with the Container Registry Admin account - if the admin account is enabled."
- value = azurerm_container_registry.main.admin_enabled ? azurerm_container_registry.main.admin_username : null
- sensitive = false
-}
+output "admin_username" {
+ description = "The Username associated with the Container Registry Admin account - if the admin account is enabled."
+ value = azurerm_container_registry.main.admin_enabled ? azurerm_container_registry.main.admin_username : null
+ sensitive = true
+}📝 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.
| output "admin_username" { | |
| description = "The Username associated with the Container Registry Admin account - if the admin account is enabled." | |
| value = azurerm_container_registry.main.admin_username | |
| sensitive = true | |
| value = azurerm_container_registry.main.admin_enabled ? azurerm_container_registry.main.admin_username : null | |
| sensitive = false | |
| } | |
| output "admin_username" { | |
| description = "The Username associated with the Container Registry Admin account - if the admin account is enabled." | |
| value = azurerm_container_registry.main.admin_enabled ? azurerm_container_registry.main.admin_username : null | |
| sensitive = true | |
| } |
🤖 Prompt for AI Agents
In output.tf around lines 13 to 17, the output "admin_username" is incorrectly
marked as not sensitive, which can expose sensitive information in state files
and logs. Change the sensitive attribute from false to true to ensure the
username is treated as sensitive and properly protected.
| variable "zone_redundancy_enabled" { | ||
| description = "Whether zone redundancy is enabled for the Container Registry (Premium SKU only)." | ||
| type = bool | ||
| default = false | ||
| } | ||
|
|
||
| variable "export_policy_enabled" { | ||
| description = "Whether export policy is enabled for the Container Registry." | ||
| type = bool | ||
| default = true | ||
| } | ||
|
|
||
| variable "anonymous_pull_enabled" { | ||
| description = "Whether anonymous pull access is enabled for the Container Registry (Premium SKU only)." | ||
| type = bool | ||
| default = false | ||
| } | ||
|
|
||
| variable "data_endpoint_enabled" { | ||
| description = "Whether data endpoint is enabled for the Container Registry (Premium SKU only)." | ||
| type = bool | ||
| default = false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to block Premium-only flags when acr_tier ≠ "Premium".
zone_redundancy_enabled, anonymous_pull_enabled, and data_endpoint_enabled are only accepted by the provider when the registry SKU is Premium.
Turning any of them on while var.acr_tier is Basic/Standard will make the plan/apply fail.
variable "zone_redundancy_enabled" {
description = "Whether zone redundancy is enabled for the Container Registry (Premium SKU only)."
type = bool
default = false
+ validation {
+ condition = !(var.zone_redundancy_enabled && var.acr_tier != "Premium")
+ error_message = "zone_redundancy_enabled can only be true when acr_tier is \"Premium\"."
+ }
}Replicate the same conditional check for the other Premium-only switches to fail fast and make the UX explicit.
🤖 Prompt for AI Agents
In variables.tf between lines 44 and 66, add validation blocks to the variables
zone_redundancy_enabled, anonymous_pull_enabled, and data_endpoint_enabled to
ensure they can only be true if var.acr_tier is "Premium". Implement a condition
in each variable's validation that returns an error message if the variable is
true while var.acr_tier is not "Premium", thereby preventing invalid
configurations early during plan/apply.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
acr.tf (1)
17-25: Georeplication still unguarded for non-Premium SKU
Thegeoreplicationsblock will makeplan/applyfail whenvar.acr_tier≠"Premium". Wrap thefor_eachwith an SKU check, as suggested previously.-for_each = var.georeplications +for_each = var.acr_tier == "Premium" ? var.georeplications : []variables.tf (1)
44-66: Missing validation for Premium-only switches
zone_redundancy_enabled,anonymous_pull_enabled, anddata_endpoint_enabledremain unchecked againstvar.acr_tier. Add the validation blocks recommended in the earlier review so invalid combos fail fast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
acr.tf(1 hunks)output.tf(1 hunks)variables.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- output.tf
🧰 Additional context used
🪛 Checkov (3.2.334)
acr.tf
[MEDIUM] 1-67: Enable vulnerability scanning for container images.
(CKV_AZURE_163)
[MEDIUM] 1-67: Ensure geo-replicated container registries to match multi-region container deployments.
(CKV_AZURE_165)
[MEDIUM] 1-67: Ensures that ACR uses signed/trusted images
(CKV_AZURE_164)
acr.tf
Outdated
| retention_policy_in_days = var.retention_policy != null ? var.retention_policy.days : null | ||
| trust_policy_enabled = var.trust_policy != null ? var.trust_policy.enabled : false |
There was a problem hiding this comment.
Retention & Trust policy attributes will fail plan – use proper nested blocks
retention_policy_in_days and trust_policy_enabled are not valid top-level arguments for azurerm_container_registry. In addition, var.retention_policy is declared as a number, so the .days accessor will raise a type error. Switch to the provider-expected nested blocks (and align variable types) to avoid a hard failure at terraform plan.
- retention_policy_in_days = var.retention_policy != null ? var.retention_policy.days : null
- trust_policy_enabled = var.trust_policy != null ? var.trust_policy.enabled : false
+
+ dynamic "retention_policy" {
+ for_each = var.retention_policy != null ? [var.retention_policy] : []
+ content {
+ days = retention_policy.value.days
+ status = "Enabled"
+ }
+ }
+
+ dynamic "trust_policy" {
+ for_each = var.trust_policy != null ? [var.trust_policy] : []
+ content {
+ enabled = trust_policy.value.enabled
+ }
+ }📝 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.
| retention_policy_in_days = var.retention_policy != null ? var.retention_policy.days : null | |
| trust_policy_enabled = var.trust_policy != null ? var.trust_policy.enabled : false | |
| dynamic "retention_policy" { | |
| for_each = var.retention_policy != null ? [var.retention_policy] : [] | |
| content { | |
| days = retention_policy.value.days | |
| status = "Enabled" | |
| } | |
| } | |
| dynamic "trust_policy" { | |
| for_each = var.trust_policy != null ? [var.trust_policy] : [] | |
| content { | |
| enabled = trust_policy.value.enabled | |
| } | |
| } |
🤖 Prompt for AI Agents
In acr.tf around lines 14 to 15, the attributes retention_policy_in_days and
trust_policy_enabled are incorrectly used as top-level arguments and
var.retention_policy is a number, causing type errors. Replace these with the
correct nested blocks for retention_policy and trust_policy as expected by the
azurerm_container_registry resource, and update the variable types accordingly
to match the nested block structure to prevent terraform plan failures.
| dynamic "encryption" { | ||
| for_each = var.encryption != null ? [var.encryption] : [] | ||
| content { | ||
| key_vault_key_id = encryption.value.key_vault_key_id | ||
| identity_client_id = encryption.value.identity_client_id | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Premium-only encryption block needs an SKU gate
encryption is accepted only on Premium registries. Protect the dynamic block so Basic/Standard deployments remain valid.
-for_each = var.encryption != null ? [var.encryption] : []
+for_each = var.acr_tier == "Premium" && var.encryption != null ? [var.encryption] : []📝 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.
| dynamic "encryption" { | |
| for_each = var.encryption != null ? [var.encryption] : [] | |
| content { | |
| key_vault_key_id = encryption.value.key_vault_key_id | |
| identity_client_id = encryption.value.identity_client_id | |
| } | |
| } | |
| dynamic "encryption" { | |
| - for_each = var.encryption != null ? [var.encryption] : [] | |
| + for_each = var.acr_tier == "Premium" && var.encryption != null ? [var.encryption] : [] | |
| content { | |
| key_vault_key_id = encryption.value.key_vault_key_id | |
| identity_client_id = encryption.value.identity_client_id | |
| } | |
| } |
🤖 Prompt for AI Agents
In acr.tf around lines 58 to 64, the dynamic "encryption" block must be
conditionally included only for Premium SKU registries. Add a condition to check
if the SKU is Premium before including the encryption block, ensuring Basic and
Standard SKUs do not attempt to use encryption and cause deployment errors.
| variable "georeplications" { | ||
| description = "A list of georeplication configurations for the Container Registry (Premium SKU only)." | ||
| type = list(object({ | ||
| location = string | ||
| zone_redundancy_enabled = bool | ||
| regional_endpoint_enabled = bool | ||
| tags = map(string) | ||
| })) | ||
| default = [] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard georeplication variable against non-Premium usage
As with the resource block, the variable should surface an explicit validation that forbids non-empty georeplications when acr_tier is not "Premium". This prevents silent misconfiguration at module entry.
🤖 Prompt for AI Agents
In variables.tf around lines 78 to 87, add a validation block to the
georeplications variable that checks if acr_tier is "Premium" when
georeplications is not empty. This validation should produce an error if
georeplications is set but acr_tier is not "Premium", preventing
misconfiguration. Use Terraform's variable validation syntax to enforce this
condition explicitly.
| type = object({ | ||
| default_action = string | ||
| ip_rules = list(string) | ||
| virtual_network_rules = list(string) | ||
| }) | ||
| default = null | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Network rule set needs SKU validation
network_rule_set is not supported on Basic/Standard SKUs. Add a validation similar to:
validation {
condition = var.network_rule_set == null || var.acr_tier == "Premium"
error_message = "network_rule_set can only be set when acr_tier is \"Premium\"."
}🤖 Prompt for AI Agents
In variables.tf around lines 90 to 97, the network_rule_set variable lacks SKU
validation and is not supported on Basic/Standard SKUs. Add a validation block
to ensure network_rule_set is either null or acr_tier is "Premium". This
involves adding a validation condition that checks if var.network_rule_set is
null or var.acr_tier equals "Premium", and an error_message stating that
network_rule_set can only be set when acr_tier is "Premium".
| description = "Number of days to retain untagged manifests in the Container Registry (Premium SKU only). Set to null to disable." | ||
| type = number | ||
| default = null | ||
| } | ||
|
|
There was a problem hiding this comment.
Variable shape does not match usage – switch to an object
acr.tf expects a .days field, but the variable is defined as a plain number. This type mismatch will break the module. Define the variable as an object with a days attribute (or change the usage).
-variable "retention_policy" {
- description = "Number of days to retain untagged manifests in the Container Registry (Premium SKU only). Set to null to disable."
- type = number
- default = null
-}
+variable "retention_policy" {
+ description = "Retention policy configuration for untagged manifests (Premium SKU only)."
+ type = object({
+ days = number
+ })
+ default = null
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In variables.tf around lines 99 to 103, the variable "retention_policy" is
defined as a number but is used in acr.tf expecting an object with a "days"
field. To fix this, redefine "retention_policy" as an object type with a "days"
attribute instead of a plain number, ensuring the variable shape matches its
usage.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation