cloudfront migration #454
cloudfront migration #454
Conversation
rauneetverma
requested review from
anshulsao,
ishaankalra,
pramodh-ayyappan,
rr0hit,
unni-facets and
vishnukv-facets
as code owners
WalkthroughThis change introduces a comprehensive AWS CloudFront distribution module. It significantly expands the CloudFront facets schema with new configuration options and sample updates, and adds new Terraform files for locals, main configuration, outputs, and variables. The module supports advanced CloudFront features, custom cache policies, and detailed output attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terraform
participant CloudFront Module
participant AWS
User->>Terraform: Provide inputs/variables
Terraform->>CloudFront Module: Pass configuration (locals, variables)
CloudFront Module->>AWS: Create CloudFront distribution and cache policies
AWS-->>CloudFront Module: Return distribution and policy details
CloudFront Module-->>Terraform: Output distribution attributes
Terraform-->>User: Expose outputs (ARN, domain, OAI, etc.)
Estimated code review effort4 (~90 minutes) Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
modules/cloudfront/default/0.1/facets.yaml (1)
155-160: Broken YAML – unmatched quote in regex key
cache_policies.patternPropertieskey ends with'$':, leaving an additional single-quote before the colon.
YAML loaders will fail to parse this file.- ^[a-zA-Z0-9_.-]*$': + '^[a-zA-Z0-9_.-]*$':
🧹 Nitpick comments (2)
modules/cloudfront/default/0.1/variables.tf (1)
4-7: Unused inputinputs– delete or wire-upThe
inputsvariable is declared but never referenced across the module.
Dead variables add cognitive load and invite drift.modules/cloudfront/default/0.1/outputs.tf (1)
16-80: Adddescription(andsensitivewhere applicable) to all output blocks.Terraform best practice is to document every output for module consumers; otherwise
terraform providers schema -jsonrenders empty descriptions. Also consider marking potentially sensitive fields (etag,caller_reference) withsensitive = true.-output "cloudfront_distribution_etag" { - value = module.cloudfront.cloudfront_distribution_etag -} +output "cloudfront_distribution_etag" { + description = "ETag of the CloudFront distribution." + value = module.cloudfront.cloudfront_distribution_etag + sensitive = true +}Repeat for the remaining outputs.
Would you like a PR-ready diff that bulk-adds descriptions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
modules/cloudfront/default/0.1/facets.yaml(11 hunks)modules/cloudfront/default/0.1/locals.tf(1 hunks)modules/cloudfront/default/0.1/main.tf(1 hunks)modules/cloudfront/default/0.1/outputs.tf(1 hunks)modules/cloudfront/default/0.1/variables.tf(1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: sanmesh-kakade
PR: Facets-cloud/facets-modules#393
File: modules/mongo_user/default/0.3/main.tf:63-110
Timestamp: 2025-06-03T11:10:34.578Z
Learning: For the Facets-cloud/facets-modules repository, module sources without specific version tags default to master branch and this is the preferred approach by the team.
modules/cloudfront/default/0.1/variables.tf (1)
Learnt from: sanmesh-kakade
PR: #393
File: modules/mongo_user/default/0.3/main.tf:63-110
Timestamp: 2025-06-03T11:10:34.578Z
Learning: In the mongo_user module at modules/mongo_user/default/0.3/, the inputs variable is properly declared with type object containing mongo_details.interfaces.writer.connection_string structure, making var.inputs.mongo_details.interfaces.writer.connection_string a valid reference.
🪛 Checkov (3.2.334)
modules/cloudfront/default/0.1/main.tf
[MEDIUM] 1-9: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
[HIGH] 1-9: Ensure Terraform module sources use a tag with a version number
(CKV_TF_2)
[MEDIUM] 12-43: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sanmesh-kakade
PR: Facets-cloud/facets-modules#393
File: modules/mongo_user/default/0.3/main.tf:63-110
Timestamp: 2025-06-03T11:10:34.578Z
Learning: For the Facets-cloud/facets-modules repository, module sources without specific version tags default to master branch and this is the preferred approach by the team.
modules/cloudfront/default/0.1/variables.tf (1)
Learnt from: sanmesh-kakade
PR: #393
File: modules/mongo_user/default/0.3/main.tf:63-110
Timestamp: 2025-06-03T11:10:34.578Z
Learning: In the mongo_user module at modules/mongo_user/default/0.3/, the inputs variable is properly declared with type object containing mongo_details.interfaces.writer.connection_string structure, making var.inputs.mongo_details.interfaces.writer.connection_string a valid reference.
🪛 Checkov (3.2.334)
modules/cloudfront/default/0.1/main.tf
[MEDIUM] 1-9: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
[HIGH] 1-9: Ensure Terraform module sources use a tag with a version number
(CKV_TF_2)
[MEDIUM] 12-43: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
🔇 Additional comments (1)
modules/cloudfront/default/0.1/facets.yaml (1)
391-397: Property name mismatch:aws_waf_idvs samplewaf_id
locals.tf/main.tflook foraws_waf_id, but the sample and facets spec footer usewaf_id.
Align the field name to avoid silent nulls.
| locals { | ||
| spec = lookup(var.instance, "spec", {}) | ||
| advanced = lookup(lookup(var.instance, "advanced", {}), "cloudfront", {}) | ||
| user_defined_tags = lookup(local.advanced, "tags", {}) | ||
| tags = merge(local.user_defined_tags, var.environment.cloud_tags) | ||
| create_origin_access_identity = lookup(local.advanced, "create_origin_access_identity", true) | ||
|
|
There was a problem hiding this comment.
Hard dependency on environment.cloud_tags
Same concern as in variables.tf: if callers omit cloud_tags, evaluation fails.
Either enforce via variable type (object({ namespace = string, cloud_tags = map(string) })) or keep defensive lookup(var.environment, "cloud_tags", {}).
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/locals.tf around lines 1 to 7, the code
assumes var.environment.cloud_tags always exists, causing failures if omitted.
Fix this by replacing direct access to var.environment.cloud_tags with a
defensive lookup that provides an empty map as a default, e.g., use
lookup(var.environment, "cloud_tags", {}) instead of var.environment.cloud_tags
to avoid evaluation errors when cloud_tags is missing.
| forwarded_values = [ | ||
| for k, v in lookup(local.spec.default_cache_behavior, "forwarded_values", {}) : { | ||
| query_string = lookup(v, "query_string", false) | ||
| query_string_cache_keys = lookup(v, "query_string_cache_keys", []) | ||
| headers = lookup(v, "headers", []) | ||
| cookies_forward = lookup(v, "cookies_forward", "none") | ||
| cookies_whitelisted_names = lookup(v, "cookies_whitelisted_names", null) | ||
| } | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
forwarded_values must be an object, not a list
terraform-aws-modules/cloudfront/aws expects:
forwarded_values = {
query_string = false
cookies = { forward = "none" }
}Supplying a list will raise a type error at plan time.
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/locals.tf around lines 85 to 93, the
forwarded_values is defined as a list comprehension but it must be an object to
match the expected type by terraform-aws-modules/cloudfront/aws. Change the
forwarded_values definition from a list to a single object with keys like
query_string and cookies, ensuring cookies is an object with a forward key,
instead of a list of objects.
| lambda_function_association = { | ||
| for k, v in lookup(local.raw_ordered_cache_behaviors[k], "lambda_function_association", {}) : k => { | ||
| event_type = v.event_type | ||
| lambda_arn = v.lambda_arn | ||
| include_body = lookup(v, "include_body", null) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
lambda_function_association should be a list, not a map
You convert the map into another map, but the downstream module schema is list(object({...})).
Wrap the comprehension in to_list(values(...)) or replicate the pattern used for the default cache behaviour.
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/locals.tf around lines 143 to 150, the
lambda_function_association is currently constructed as a map, but the
downstream module expects a list of objects. To fix this, convert the map
comprehension result to a list by wrapping it with to_list(values(...)) or use a
similar pattern as the default cache behaviour to produce a list instead of a
map.
| query_strings_config { | ||
| query_string_behavior = lookup(lookup(lookup(each.value, "local.parameters_in_cache_key_and_forwarded_to_origin", {}), "query_strings_config", {}), "query_string_behavior", "none") | ||
| query_strings { | ||
| items = lookup(lookup(lookup(lookup(each.value, "parameters_in_cache_key_and_forwarded_to_origin", {}), "query_strings_config", {}), "query_strings", {}), "items", []) |
There was a problem hiding this comment.
Typo in lookup path drops query_strings_config entirely
"local.parameters_in_cache_key_and_forwarded_to_origin" includes the local. prefix inside the key name, so the lookup always falls back to {} and query_string_behavior defaults to "none".
- query_string_behavior = lookup(lookup(lookup(each.value, "local.parameters_in_cache_key_and_forwarded_to_origin", {}), "query_strings_config", {}), "query_string_behavior", "none")
+ query_string_behavior = lookup(lookup(lookup(each.value, "parameters_in_cache_key_and_forwarded_to_origin", {}), "query_strings_config", {}), "query_string_behavior", "none")📝 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.
| query_strings_config { | |
| query_string_behavior = lookup(lookup(lookup(each.value, "local.parameters_in_cache_key_and_forwarded_to_origin", {}), "query_strings_config", {}), "query_string_behavior", "none") | |
| query_strings { | |
| items = lookup(lookup(lookup(lookup(each.value, "parameters_in_cache_key_and_forwarded_to_origin", {}), "query_strings_config", {}), "query_strings", {}), "items", []) | |
| query_strings_config { | |
| query_string_behavior = lookup( | |
| lookup( | |
| lookup(each.value, "parameters_in_cache_key_and_forwarded_to_origin", {}), | |
| "query_strings_config", | |
| {} | |
| ), | |
| "query_string_behavior", | |
| "none" | |
| ) | |
| query_strings { | |
| items = lookup( | |
| lookup( | |
| lookup( | |
| lookup(each.value, "parameters_in_cache_key_and_forwarded_to_origin", {}), | |
| "query_strings_config", | |
| {} | |
| ), | |
| "query_strings", | |
| {} | |
| ), | |
| "items", | |
| [] | |
| ) |
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/main.tf around lines 64 to 67, the lookup path
mistakenly includes the prefix "local." in the key name, causing it to always
fallback to an empty map and default the query_string_behavior to "none". Remove
the "local." prefix from the first lookup key so it correctly accesses
"parameters_in_cache_key_and_forwarded_to_origin" and retrieves the nested
"query_strings_config" and "query_string_behavior" values.
| locals { | ||
| output_interfaces = {} | ||
| output_attributes = { |
There was a problem hiding this comment.
Remove or wire up unused output_interfaces.
local.output_interfaces is declared but never referenced. Dead code adds noise and can confuse future maintainers. Either delete it or hook it into an actual output block if you planned to expose interface details.
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/outputs.tf at lines 2 to 4, the local variable
output_interfaces is declared but never used. To fix this, either remove the
output_interfaces declaration entirely to eliminate dead code or connect it to
an output block if it was intended to expose interface details, ensuring it is
properly referenced in the Terraform outputs.
| output_attributes = { | ||
| cloudfront_distribution_arn = module.cloudfront.cloudfront_distribution_arn | ||
| cloudfront_distribution_domain_name = module.cloudfront.cloudfront_distribution_domain_name | ||
| cloudfront_distribution_etag = module.cloudfront.cloudfront_distribution_etag | ||
| cloudfront_distribution_id = module.cloudfront.cloudfront_distribution_id | ||
| cloudfront_distribution_last_modified_time = module.cloudfront.cloudfront_distribution_last_modified_time | ||
| cloudfront_origin_access_identities = { | ||
| for k, v in module.cloudfront.cloudfront_origin_access_identities : k => v.iam_arn | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose the aggregated output_attributes map as a single output.
You’re already collecting all key distribution attributes in local.output_attributes, but that map isn’t exported. Down-stream stacks will still have to reference the individual outputs below, defeating the purpose of the aggregation.
+output "cloudfront_attributes" {
+ description = "Aggregated CloudFront distribution attributes."
+ value = local.output_attributes
+}This reduces boilerplate and keeps the public interface future-proof; adding a new attribute only requires updating the local, not multiple outputs.
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/outputs.tf around lines 4 to 14, the
aggregated map output_attributes is defined but not exposed as an output. To fix
this, define a new output block that exports the entire output_attributes map so
downstream stacks can reference it directly. This will reduce boilerplate and
make the interface easier to maintain by exposing the aggregated attributes as a
single output.
| variable "environment" { | ||
| type = any | ||
| default = { | ||
| namespace = "default" | ||
| } | ||
| } |
There was a problem hiding this comment.
Default environment map is missing cloud_tags, causing runtime errors
locals.tf (l.5) assumes var.environment.cloud_tags always exists.
With the current default, terraform plan will abort with: “Invalid index; the given key does not identify an element in this collection value.”
-variable "environment" {
- type = any
- default = {
- namespace = "default"
- }
-}
+variable "environment" {
+ type = any
+ default = {
+ namespace = "default"
+ cloud_tags = {} # <- ensure attribute is present
+ }
+}📝 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.
| variable "environment" { | |
| type = any | |
| default = { | |
| namespace = "default" | |
| } | |
| } | |
| variable "environment" { | |
| type = any | |
| default = { | |
| namespace = "default" | |
| cloud_tags = {} # <- ensure attribute is present | |
| } | |
| } |
🤖 Prompt for AI Agents
In modules/cloudfront/default/0.1/variables.tf around lines 23 to 28, the
default value for the variable "environment" is missing the "cloud_tags" key,
which causes runtime errors when locals.tf line 5 tries to access
var.environment.cloud_tags. To fix this, add a "cloud_tags" key with an
appropriate default value (e.g., an empty map) to the default environment map to
ensure the key always exists.
Description
Related issues
Type of change
Checklist
developbranchTesting
Reviewer instructions
Summary by CodeRabbit
New Features
Documentation