-
-
Notifications
You must be signed in to change notification settings - Fork 253
docs: web acl id can be an ARN for newer WAFs #349
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
base: main
Are you sure you want to change the base?
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 description for the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes ✨ 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. Comment |
|
/terratest |
There was a problem hiding this 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
🧹 Nitpick comments (1)
variables.tf (1)
356-360: Consider adding a validation block to catch obvious mis-inputs earlyTerraform will still accept any string here and users won’t discover a bad format until the CloudFront call fails. A lightweight validation guard can fail fast:
variable "web_acl_id" { type = string default = "" description = "ID or ARN of the AWS WAF web ACL that is associated with the distribution. NOTE: If using the latest version of WAF (WAFv2), be sure to use the ARN. If using WAF Classic, use the ID. https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudfront_distribution#web_acl_id-3" + + validation { + # Accept empty string, a WAFv2 ARN, or a 32-char hex ID for WAF Classic + condition = var.web_acl_id == "" || + can(regex("^arn:aws[\\w-]*:waf.*:.*:.*:webacl/.*", var.web_acl_id)) || + can(regex("^[a-f0-9]{32}$", var.web_acl_id)) + error_message = "Provide either a full ARN for WAFv2 or the 32-character ID for WAF Classic." + } }This is optional, but it tightens feedback loops and aligns with other validations already present in the module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by!README.md
📒 Files selected for processing (1)
variables.tf(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jwadolowski
PR: cloudposse/terraform-aws-cloudfront-s3-cdn#340
File: main.tf:570-578
Timestamp: 2025-07-31T11:46:57.218Z
Learning: In terraform-aws-cloudfront-s3-cdn module, custom_origin_config in the custom_origins variable was always a required parameter, not optional. CloudFront origins require either custom_origin_config or s3_origin_config but not both, so null was never a valid value for custom_origin_config in custom origins.
Learnt from: jwadolowski
PR: cloudposse/terraform-aws-cloudfront-s3-cdn#340
File: main.tf:570-578
Timestamp: 2025-07-31T11:46:57.218Z
Learning: In terraform-aws-cloudfront-s3-cdn module, custom_origin_config in the custom_origins variable was always a required parameter, not optional. CloudFront origins require either custom_origin_config or s3_origin_config but not both, so null was never a valid value for custom_origin_config in custom origins.
📚 Learning: in terraform-aws-cloudfront-s3-cdn module, custom_origin_config in the custom_origins variable was a...
Learnt from: jwadolowski
PR: cloudposse/terraform-aws-cloudfront-s3-cdn#340
File: main.tf:570-578
Timestamp: 2025-07-31T11:46:57.218Z
Learning: In terraform-aws-cloudfront-s3-cdn module, custom_origin_config in the custom_origins variable was always a required parameter, not optional. CloudFront origins require either custom_origin_config or s3_origin_config but not both, so null was never a valid value for custom_origin_config in custom origins.
Applied to files:
variables.tf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
variables.tf (1)
359-360: Documentation clarification looks goodThe note about ARN vs ID removes an often-hit stumbling block for users and the link to the provider docs is a nice touch.
|
/terratest |
As per the AWS provider docs. Updating description here so consumers don't get confused why WAF association fails when providing the ID.
ID or ARN of the AWS WAF web ACL that is associated with the distribution. NOTE: If using the latest version of WAF (WAFv2), be sure to use the ARN. If using WAF Classic, use the ID. https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudfront_distribution#web_acl_id-3