feat(origins): added optional origin_shield section for additional origins #345
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce support for configuring the CloudFront Origin Shield feature for both custom and S3 origins within the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. ✨ 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. 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
CodeRabbit Configuration File (
|
|
How do I properly regenerate README.md? I can't figure this out from inline comments, which mention |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
main.tf(2 hunks)variables.tf(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
variables.tf (1)
467-471: Ensure Terraform version supportsoptionaltype syntax.
The use ofoptional(object({...}), null)requires Terraform ≥1.5.0. Please confirm that your module’srequired_versionconstraint (e.g., inversions.tf) is updated to include Terraform 1.5.0 or newer to avoid breaking users on earlier versions.
| origin_shield = optional(object({ | ||
| enabled = optional(bool, false) | ||
| region = optional(string, null) | ||
| }), null) | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add fallback for unset origin_shield.region.
If origin_shield.enabled = true but region remains null, downstream will configure origin_shield_region = null, which is invalid. Consider defaulting region to the computed local.origin_shield_region when it’s not provided.
🤖 Prompt for AI Agents
In variables.tf around lines 489 to 493, the origin_shield.region can be null
even when origin_shield.enabled is true, causing invalid downstream
configuration. Modify the code to provide a fallback so that if
origin_shield.region is null and origin_shield.enabled is true,
origin_shield.region defaults to local.origin_shield_region. This ensures a
valid region value is always set when origin_shield is enabled.
| dynamic "origin_shield" { | ||
| for_each = origin.value.origin_shield != null ? [origin.value.origin_shield] : [] | ||
| content { | ||
| enabled = origin_shield.value.enabled | ||
| origin_shield_region = origin_shield.value.region | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply region fallback and filter disabled shields for custom origins.
Only include the block when enabled is true and coalesce a null region to local.origin_shield_region:
-for_each = origin.value.origin_shield != null ? [origin.value.origin_shield] : []
+for_each = origin.value.origin_shield.enabled ? [origin.value.origin_shield] : []
content {
enabled = origin_shield.value.enabled
- origin_shield_region = origin_shield.value.region
+ origin_shield_region = coalesce(origin_shield.value.region, local.origin_shield_region)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In main.tf around lines 574 to 580, the dynamic block for origin_shield should
only be included if the shield is enabled. Update the for_each expression to
filter out any origin_shield where enabled is false, and use a coalesce function
to set origin_shield_region to local.origin_shield_region if the region is null.
This ensures the block is conditionally included and applies the correct region
fallback.
| dynamic "origin_shield" { | ||
| for_each = origin.value.origin_shield != null ? [origin.value.origin_shield] : [] | ||
| content { | ||
| enabled = origin_shield.value.enabled | ||
| origin_shield_region = origin_shield.value.region | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply region fallback and filter disabled shields for S3 origins.
Mirror the custom‐origin logic: only render when enabled is true and fallback null region:
-for_each = origin.value.origin_shield != null ? [origin.value.origin_shield] : []
+for_each = origin.value.origin_shield.enabled ? [origin.value.origin_shield] : []
content {
enabled = origin_shield.value.enabled
- origin_shield_region = origin_shield.value.region
+ origin_shield_region = coalesce(origin_shield.value.region, local.origin_shield_region)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In main.tf around lines 601 to 607, the origin_shield block should only be
rendered if origin_shield is enabled and the region should fallback to null if
not set. Update the for_each condition to filter out disabled origin_shield
entries and modify the origin_shield_region assignment to use a fallback to null
when the region is not specified, matching the custom-origin logic.
|
While improvements provided by AI are reasonable, I believe that this also should be implemented in upstream module cloudposse/terraform-aws-cloudfront-cdn, as I replicated code from that repo. |
…igins custom_origins & s3_origins
atimofeev
force-pushed
the
custom-origin-shield
branch
from
5de57d3 to
f775824
Compare
what
Added
origin_shieldconfiguration block tos3_originsandcustom_origins.why
We should be able to enable
origin_shieldfor selected additional origins.references
closes #341