Make Improvements to Infrastructure Code #2812
Conversation
rudransh-shrivastava
changed the base branch from
main
to
feature/nest-zappa-migration
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughUpdated Terraform/provider constraints, moved DB/Redis secrets into SSM parameters and switched module interfaces to ARN form, removed module lockfiles, refactored security-group egress into explicit rules, added variable validations and TFLint/pre-commit integration, plus documentation and tagging/description standardization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/modules/security/main.tf (1)
141-150: Consider standardizing attribute ordering across security group rules.The egress rules use consistent patterns but attribute order varies slightly. For example, some resources order attributes as
count, description, from_port...while others usecount, type, description, from_port.... Standardizing the order (e.g.,count, description, type, from_port, to_port, protocol, cidr_blocks/source_security_group_id, security_group_id) would improve maintainability.Also applies to: 174-182
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
infrastructure/modules/cache/.terraform.lock.hcl(0 hunks)infrastructure/modules/cache/main.tf(1 hunks)infrastructure/modules/database/.terraform.lock.hcl(0 hunks)infrastructure/modules/database/main.tf(1 hunks)infrastructure/modules/ecs/.terraform.lock.hcl(0 hunks)infrastructure/modules/ecs/main.tf(1 hunks)infrastructure/modules/ecs/modules/task/.terraform.lock.hcl(0 hunks)infrastructure/modules/ecs/modules/task/main.tf(1 hunks)infrastructure/modules/parameters/main.tf(1 hunks)infrastructure/modules/security/main.tf(6 hunks)infrastructure/modules/storage/.terraform.lock.hcl(0 hunks)infrastructure/staging/.terraform.lock.hcl(2 hunks)
💤 Files with no reviewable changes (5)
- infrastructure/modules/cache/.terraform.lock.hcl
- infrastructure/modules/ecs/.terraform.lock.hcl
- infrastructure/modules/database/.terraform.lock.hcl
- infrastructure/modules/ecs/modules/task/.terraform.lock.hcl
- infrastructure/modules/storage/.terraform.lock.hcl
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/cache/main.tf:30-30
Timestamp: 2025-10-17T15:25:34.963Z
Learning: The infrastructure/Terraform code in the OWASP Nest repository under the `infrastructure/` directory is intended for quick testing purposes only, not for production deployment.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2743
File: infrastructure/modules/storage/main.tf:1-9
Timestamp: 2025-11-27T19:38:23.956Z
Learning: In the OWASP/Nest infrastructure code (infrastructure/ directory), the team prefers exact version pinning for Terraform (e.g., "1.14.0") and AWS provider (e.g., "6.22.0") over semantic versioning constraints (e.g., "~> 1.14" or "~> 6.22"). This is a deliberate choice for maximum reproducibility and explicit control over version updates in their testing environment.
📚 Learning: 2025-11-27T19:38:23.956Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2743
File: infrastructure/modules/storage/main.tf:1-9
Timestamp: 2025-11-27T19:38:23.956Z
Learning: In the OWASP/Nest infrastructure code (infrastructure/ directory), the team prefers exact version pinning for Terraform (e.g., "1.14.0") and AWS provider (e.g., "6.22.0") over semantic versioning constraints (e.g., "~> 1.14" or "~> 6.22"). This is a deliberate choice for maximum reproducibility and explicit control over version updates in their testing environment.
Applied to files:
infrastructure/modules/ecs/main.tfinfrastructure/modules/database/main.tfinfrastructure/staging/.terraform.lock.hclinfrastructure/modules/cache/main.tfinfrastructure/modules/ecs/modules/task/main.tfinfrastructure/modules/parameters/main.tf
📚 Learning: 2025-11-08T11:16:25.725Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.725Z
Learning: The parameters module in infrastructure/modules/parameters/ is currently configured for staging environment only. The `configuration` and `settings_module` variables default to "Staging" and "settings.staging" respectively, and users can update parameter values via the AWS Parameter Store console. The lifecycle.ignore_changes blocks on these parameters support manual console updates without Terraform reverting them.
Applied to files:
infrastructure/modules/parameters/main.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). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (11)
infrastructure/modules/database/main.tf (1)
2-2: Version constraints properly tightened to exact pinning.All three version constraints (Terraform, AWS provider, Random provider) have been correctly updated to exact versions, aligning with the PR objective for stricter versioning and the team's documented preference for reproducibility in the testing infrastructure. Changes are syntactically valid and consistent across modules.
Also applies to: 7-7, 11-11
infrastructure/staging/.terraform.lock.hcl (1)
6-6: Lock file constraints correctly reflect module-level version pinning.The provider constraint strings have been properly updated to match the exact versions specified in module configurations. Presence of valid hashes confirms these versions are available in the Terraform registry. This file will be automatically regenerated by
terraform initif needed.Also applies to: 29-29
infrastructure/modules/parameters/main.tf (1)
2-2: Version constraints consistently updated for reproducibility.Terraform and provider version constraints have been properly pinned to exact versions (1.14.0, 6.22.0, 3.7.2), maintaining consistency with other modules in the PR and supporting the PR objective of enforcing stricter versioning.
Also applies to: 7-7, 11-11
infrastructure/modules/ecs/main.tf (1)
2-2: Version constraints properly pinned for ECS module.Terraform and AWS provider versions have been updated to exact versions (1.14.0 and 6.22.0). This module correctly declares only the AWS provider as a requirement, which is appropriate for its resource definitions.
Also applies to: 7-7
infrastructure/modules/ecs/modules/task/main.tf (1)
2-2: Version constraints correctly pinned for ECS task submodule.Terraform and AWS provider versions have been appropriately updated to exact versions. This submodule correctly depends only on the AWS provider, which aligns with its resource scope (CloudWatch and ECS task definitions).
Also applies to: 7-7
infrastructure/modules/cache/main.tf (1)
2-2: Cache module version constraints properly tightened.All three Terraform and provider version constraints have been updated to exact versions (1.14.0, 6.22.0, 3.7.2). The module appropriately declares both AWS and Random providers, which are both required for ElastiCache configuration and auth token generation.
Also applies to: 7-7, 11-11
infrastructure/modules/security/main.tf (5)
1-10: Version pinning looks solid.Exact version constraints align with your team's preference for reproducibility and explicit control.
Based on learnings, this approach matches the team's deliberate choice for maximum reproducibility in the testing environment.
108-172: Conditional RDS logic is well-structured.The mutual exclusivity between direct RDS connections (lines 108–128) and RDS Proxy connections (lines 130–172) is correctly implemented using
countconditions. This approach should also resolve the terraform drift issue mentioned in the PR by providing explicit resource lifecycle management instead of inline blocks.
184-202: Verify thatvar.redis_portvariable is defined invariables.tf.The Redis ingress rules (lines 184–202) reference
var.redis_port. Confirm this variable is defined with an appropriate default value (typically6379for Redis).
58-106: Verify that variablevar.default_egress_cidr_blocksis defined invariables.tf.The egress rules on lines 58–66, 78–86, and 98–106 reference
var.default_egress_cidr_blocks, which must be defined upstream. Confirm that the variable exists and contains the intended CIDR blocks (e.g.,["0.0.0.0/0"]for full internet egress).
68-76: Verify that variablevar.vpc_endpoint_sg_idis defined.The VPC endpoint rules reference
var.vpc_endpoint_sg_idon lines 73 and 93, which should point to the VPC endpoint security group. Confirm this variable is defined invariables.tf.Also applies to: 88-96
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-improvements
branch
from
7ab99cf to
e66427b
Compare
github-actions
bot
added
docs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
infrastructure/modules/database/variables.tf (1)
1-116: Consider establishing a consistent description pattern across all variables.While the changes to the boolean variable descriptions are good, the file still shows mixed description styles across different variable types (some starting with "The", others with "A", "Whether", or "List of"). As part of the broader "Consistent descriptions" objective, consider establishing a style guide for variable descriptions (e.g., boolean variables always use "Whether...", and apply it consistently across this module and others.
infrastructure/modules/ecs/variables.tf (2)
1-4: Complete the variable description consistency pass across the file.The
aws_regiondescription now ends with a period, which improves consistency. However, many other descriptions in this file lack periods (e.g.,ecs_sg_id,environment,fixtures_read_only_policy_arn, etc.). Since "Consistent descriptions" is a stated PR objective, consider completing this formatting pass for all variable descriptions in one go.Example fix to apply consistently across the file:
variable "ecs_sg_id" { - description = "The ID of the security group for the ECS tasks" + description = "The ID of the security group for the ECS tasks." type = string } variable "environment" { - description = "The environment (e.g., staging, production)" + description = "The environment (e.g., staging, production)." type = string }Based on the PR objectives, this aligns with the goal to ensure consistent resource descriptions across the infrastructure code.
85-88: Note: Variable validation blocks are pending per PR objectives.The PR objectives mark "Add validation for variables" as pending. Consider adding validation blocks where appropriate (e.g., validating that
project_nameandenvironmentfollow naming conventions, or that task CPU/memory values meet expected ranges), when that work is prioritized.Example (if needed):
variable "project_name" { description = "The name of the project." type = string validation { condition = length(var.project_name) > 0 error_message = "Project name must not be empty." } }infrastructure/README.md (1)
20-24: Markdown linting violations: nested list indentation (MD007).The nested bullet points under numbered sections use 2-space indentation, which improves visual hierarchy but violates markdownlint's MD007 rule (expects 0 indentation). While this formatting choice enhances readability, it conflicts with strict GitHub-flavored Markdown compliance.
Decide whether to:
- Accept the style trade-off for improved readability, and update the linting configuration to allow/disable MD007, or
- Reformat all nested lists to start at column 0 and rely on numbered sub-items (e.g., "1.1", "1.2") or dashes at root level for hierarchy.
Also applies to: 29-33, 35-39, 49-53, 55-59, 61-65, 67-71, 73-77, 85-89, 91-95, 99-100
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
infrastructure/README.md(2 hunks)infrastructure/backend/variables.tf(1 hunks)infrastructure/modules/cache/variables.tf(1 hunks)infrastructure/modules/database/variables.tf(2 hunks)infrastructure/modules/ecs/variables.tf(2 hunks)infrastructure/modules/networking/modules/nacl/variables.tf(1 hunks)infrastructure/modules/networking/modules/vpc-endpoint/variables.tf(1 hunks)infrastructure/modules/security/variables.tf(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- infrastructure/modules/networking/modules/vpc-endpoint/variables.tf
- infrastructure/backend/variables.tf
- infrastructure/modules/security/variables.tf
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/cache/main.tf:30-30
Timestamp: 2025-10-17T15:25:34.963Z
Learning: The infrastructure/Terraform code in the OWASP Nest repository under the `infrastructure/` directory is intended for quick testing purposes only, not for production deployment.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2743
File: infrastructure/modules/storage/main.tf:1-9
Timestamp: 2025-11-27T19:38:23.956Z
Learning: In the OWASP/Nest infrastructure code (infrastructure/ directory), the team prefers exact version pinning for Terraform (e.g., "1.14.0") and AWS provider (e.g., "6.22.0") over semantic versioning constraints (e.g., "~> 1.14" or "~> 6.22"). This is a deliberate choice for maximum reproducibility and explicit control over version updates in their testing environment.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/providers.tf:1-3
Timestamp: 2025-10-17T15:25:55.624Z
Learning: The infrastructure code in the OWASP/Nest repository (infrastructure/ directory) is intended for quick testing purposes only, not for production deployment.
📚 Learning: 2025-10-17T15:25:53.713Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/database/main.tf:22-60
Timestamp: 2025-10-17T15:25:53.713Z
Learning: The infrastructure code in the `infrastructure/` directory is intended for quick testing purposes only, not production-grade deployment. Production-grade security hardening controls (such as IAM database authentication, deletion protection, Performance Insights KMS encryption) are not required for this testing infrastructure.
Applied to files:
infrastructure/README.md
📚 Learning: 2025-10-17T15:25:34.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/cache/main.tf:30-30
Timestamp: 2025-10-17T15:25:34.963Z
Learning: The infrastructure/Terraform code in the OWASP Nest repository under the `infrastructure/` directory is intended for quick testing purposes only, not for production deployment.
Applied to files:
infrastructure/README.md
📚 Learning: 2025-10-17T15:25:55.624Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/providers.tf:1-3
Timestamp: 2025-10-17T15:25:55.624Z
Learning: The infrastructure code in the OWASP/Nest repository (infrastructure/ directory) is intended for quick testing purposes only, not for production deployment.
Applied to files:
infrastructure/README.md
📚 Learning: 2025-11-23T11:52:15.463Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2699
File: backend/wsgi.py:13-13
Timestamp: 2025-11-23T11:52:15.463Z
Learning: In the OWASP Nest project, the SSM parameter store setup in backend/wsgi.py (using boto3 to fetch parameters from AWS Systems Manager) is designed for staging and production environments, not just for testing purposes.
Applied to files:
infrastructure/README.md
🪛 LanguageTool
infrastructure/README.md
[style] ~234-~234: Using many exclamation marks might seem excessive (in this case: 13 exclamation marks for a text that’s 4635 characters long)
Context: ...ets and ECR repositories are empty. > [!NOTE] > Some resources have `prevent_des...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
infrastructure/README.md
20-20: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
43-43: Blank line inside blockquote
(MD028, no-blanks-blockquote)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
81-81: Blank line inside blockquote
(MD028, no-blanks-blockquote)
85-85: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
91-91: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
99-99: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
119-119: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
120-120: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
121-121: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
129-129: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
169-169: Blank line inside blockquote
(MD028, no-blanks-blockquote)
177-177: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
183-183: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
203-203: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
215-215: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
216-216: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
217-217: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
218-218: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
219-219: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ 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). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (5)
infrastructure/modules/networking/modules/nacl/variables.tf (1)
2-2: Excellent consistency improvements to variable descriptions.All variable descriptions have been updated to follow a consistent, more descriptive format. Particularly nice touches:
- Complete phrasing with article + noun structure ("A map of...", "The CIDR block for...")
- Helpful example for the
environmentvariable showing expected values- Clear, unambiguous language across all descriptions
These changes improve developer experience and documentation quality with zero behavioral impact.
Also applies to: 7-7, 12-12, 17-17, 22-22, 27-27, 32-32
infrastructure/modules/database/variables.tf (1)
31-31: Approve description simplifications for boolean variables.The descriptions for
db_copy_tags_to_snapshot(line 31) anddb_skip_final_snapshot(line 71) have been simplified by removing verbose prefixes ("Specifies whether", "Determines whether") and using "Whether" directly. This improves readability and aligns with the PR objective of ensuring consistent descriptions.Also applies to: 71-71
infrastructure/modules/cache/variables.tf (1)
2-2: Description improvement aligns with documentation goals.The simplified description is more concise and clearer. This change moves the documentation toward consistency as outlined in the PR objectives.
infrastructure/README.md (2)
234-235: Disregard linting warning about excessive exclamation marks.LanguageTool's EN_EXCESSIVE_EXCLAMATION flag is a false positive caused by the
[!NOTE]and[!WARNING]GFM alert syntax. These exclamation marks are part of standard GitHub-flavored Markdown alert formatting, not excessive punctuation in prose.
1-255: Documentation structure and clarity are strong.The README provides clear, step-by-step setup instructions with helpful note blocks and explicit examples. The expanded format (e.g., breaking commands into sub-steps and adding context) improves accessibility for users unfamiliar with infrastructure provisioning.
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-improvements
branch
from
28c89b2 to
d6e72a7
Compare
rudransh-shrivastava
changed the title
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrastructure/modules/ecs/variables.tf (1)
1-124: Consider adding input validation blocks for required variables.The PR objectives mention adding validation for variables, but this file only updates descriptions with trailing periods. Consider adding validation blocks for variables like
ecs_sg_id,fixtures_bucket_name, and task resource specifications to catch invalid inputs early. For example:variable "ecs_sg_id" { description = "The ID of the security group for the ECS tasks." type = string validation { condition = can(regex("^sg-[0-9a-f]{8}$|^sg-[0-9a-f]{17}$", var.ecs_sg_id)) error_message = "Must be a valid security group ID." } }infrastructure/modules/cache/variables.tf (1)
1-75: Consider adding input validation for Redis configuration variables.Similar to the ECS module, variable descriptions have been standardized with trailing periods, but the PR objectives mention adding variable validation. Consider adding validation for critical variables like
redis_port,redis_num_cache_nodes, andredis_engine_versionto ensure valid inputs. For example:variable "redis_port" { description = "The port for the Redis cache." type = number validation { condition = var.redis_port >= 1024 && var.redis_port <= 65535 error_message = "Redis port must be between 1024 and 65535." } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.pre-commit-config.yaml(1 hunks)infrastructure/.tflint.hcl(1 hunks)infrastructure/README.md(2 hunks)infrastructure/backend/main.tf(4 hunks)infrastructure/backend/variables.tf(1 hunks)infrastructure/modules/cache/.terraform.lock.hcl(0 hunks)infrastructure/modules/cache/main.tf(2 hunks)infrastructure/modules/cache/outputs.tf(1 hunks)infrastructure/modules/cache/variables.tf(4 hunks)infrastructure/modules/database/.terraform.lock.hcl(0 hunks)infrastructure/modules/database/main.tf(3 hunks)infrastructure/modules/database/outputs.tf(1 hunks)infrastructure/modules/database/variables.tf(5 hunks)infrastructure/modules/ecs/.terraform.lock.hcl(0 hunks)infrastructure/modules/ecs/main.tf(4 hunks)infrastructure/modules/ecs/modules/task/.terraform.lock.hcl(0 hunks)infrastructure/modules/ecs/modules/task/main.tf(1 hunks)infrastructure/modules/ecs/outputs.tf(1 hunks)infrastructure/modules/ecs/variables.tf(2 hunks)infrastructure/modules/networking/modules/nacl/variables.tf(1 hunks)infrastructure/modules/networking/modules/vpc-endpoint/outputs.tf(1 hunks)infrastructure/modules/networking/modules/vpc-endpoint/variables.tf(1 hunks)infrastructure/modules/networking/outputs.tf(1 hunks)infrastructure/modules/networking/variables.tf(3 hunks)infrastructure/modules/parameters/main.tf(1 hunks)infrastructure/modules/parameters/outputs.tf(1 hunks)infrastructure/modules/parameters/variables.tf(2 hunks)infrastructure/modules/security/main.tf(4 hunks)infrastructure/modules/security/outputs.tf(1 hunks)infrastructure/modules/security/variables.tf(2 hunks)infrastructure/modules/storage/.terraform.lock.hcl(0 hunks)infrastructure/modules/storage/main.tf(1 hunks)infrastructure/modules/storage/modules/s3-bucket/outputs.tf(1 hunks)infrastructure/modules/storage/modules/s3-bucket/variables.tf(1 hunks)infrastructure/modules/storage/outputs.tf(1 hunks)infrastructure/modules/storage/variables.tf(1 hunks)infrastructure/staging/.terraform.lock.hcl(2 hunks)infrastructure/staging/main.tf(2 hunks)infrastructure/staging/outputs.tf(1 hunks)infrastructure/staging/variables.tf(4 hunks)
💤 Files with no reviewable changes (5)
- infrastructure/modules/storage/.terraform.lock.hcl
- infrastructure/modules/cache/.terraform.lock.hcl
- infrastructure/modules/ecs/modules/task/.terraform.lock.hcl
- infrastructure/modules/database/.terraform.lock.hcl
- infrastructure/modules/ecs/.terraform.lock.hcl
✅ Files skipped from review due to trivial changes (2)
- infrastructure/modules/storage/outputs.tf
- infrastructure/modules/storage/modules/s3-bucket/variables.tf
🚧 Files skipped from review as they are similar to previous changes (14)
- infrastructure/modules/ecs/modules/task/main.tf
- infrastructure/modules/networking/variables.tf
- infrastructure/modules/database/outputs.tf
- infrastructure/modules/networking/outputs.tf
- infrastructure/modules/parameters/variables.tf
- infrastructure/modules/networking/modules/nacl/variables.tf
- infrastructure/modules/ecs/outputs.tf
- infrastructure/.tflint.hcl
- infrastructure/modules/networking/modules/vpc-endpoint/outputs.tf
- infrastructure/staging/.terraform.lock.hcl
- infrastructure/modules/storage/modules/s3-bucket/outputs.tf
- infrastructure/staging/main.tf
- infrastructure/modules/security/variables.tf
- infrastructure/staging/variables.tf
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/cache/main.tf:30-30
Timestamp: 2025-10-17T15:25:34.963Z
Learning: The infrastructure/Terraform code in the OWASP Nest repository under the `infrastructure/` directory is intended for quick testing purposes only, not for production deployment.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2743
File: infrastructure/modules/storage/main.tf:1-9
Timestamp: 2025-11-27T19:38:23.956Z
Learning: In the OWASP/Nest infrastructure code (infrastructure/ directory), the team prefers exact version pinning for Terraform (e.g., "1.14.0") and AWS provider (e.g., "6.22.0") over semantic versioning constraints (e.g., "~> 1.14" or "~> 6.22"). This is a deliberate choice for maximum reproducibility and explicit control over version updates in their testing environment.
📚 Learning: 2025-11-08T11:16:25.725Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.725Z
Learning: The parameters module in infrastructure/modules/parameters/ is currently configured for staging environment only. The `configuration` and `settings_module` variables default to "Staging" and "settings.staging" respectively, and users can update parameter values via the AWS Parameter Store console. The lifecycle.ignore_changes blocks on these parameters support manual console updates without Terraform reverting them.
Applied to files:
infrastructure/modules/cache/variables.tfinfrastructure/modules/parameters/main.tfinfrastructure/modules/cache/main.tf
📚 Learning: 2025-11-27T19:38:23.956Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2743
File: infrastructure/modules/storage/main.tf:1-9
Timestamp: 2025-11-27T19:38:23.956Z
Learning: In the OWASP/Nest infrastructure code (infrastructure/ directory), the team prefers exact version pinning for Terraform (e.g., "1.14.0") and AWS provider (e.g., "6.22.0") over semantic versioning constraints (e.g., "~> 1.14" or "~> 6.22"). This is a deliberate choice for maximum reproducibility and explicit control over version updates in their testing environment.
Applied to files:
infrastructure/modules/parameters/main.tfinfrastructure/modules/ecs/main.tfinfrastructure/modules/database/main.tfinfrastructure/modules/cache/main.tf
📚 Learning: 2025-11-23T11:52:15.463Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2699
File: backend/wsgi.py:13-13
Timestamp: 2025-11-23T11:52:15.463Z
Learning: In the OWASP Nest project, the SSM parameter store setup in backend/wsgi.py (using boto3 to fetch parameters from AWS Systems Manager) is designed for staging and production environments, not just for testing purposes.
Applied to files:
infrastructure/modules/parameters/outputs.tfinfrastructure/README.mdinfrastructure/modules/cache/main.tf
📚 Learning: 2025-10-17T15:25:53.713Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/database/main.tf:22-60
Timestamp: 2025-10-17T15:25:53.713Z
Learning: The infrastructure code in the `infrastructure/` directory is intended for quick testing purposes only, not production-grade deployment. Production-grade security hardening controls (such as IAM database authentication, deletion protection, Performance Insights KMS encryption) are not required for this testing infrastructure.
Applied to files:
infrastructure/README.md
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
infrastructure/README.md
📚 Learning: 2025-10-17T15:25:34.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/cache/main.tf:30-30
Timestamp: 2025-10-17T15:25:34.963Z
Learning: The infrastructure/Terraform code in the OWASP Nest repository under the `infrastructure/` directory is intended for quick testing purposes only, not for production deployment.
Applied to files:
infrastructure/README.md
📚 Learning: 2025-10-17T15:25:55.624Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/providers.tf:1-3
Timestamp: 2025-10-17T15:25:55.624Z
Learning: The infrastructure code in the OWASP/Nest repository (infrastructure/ directory) is intended for quick testing purposes only, not for production deployment.
Applied to files:
infrastructure/README.md
🪛 Checkov (3.2.334)
infrastructure/modules/database/main.tf
[high] 80-86: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
infrastructure/modules/cache/main.tf
[high] 86-92: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
🪛 LanguageTool
infrastructure/README.md
[style] ~234-~234: Using many exclamation marks might seem excessive (in this case: 13 exclamation marks for a text that’s 4635 characters long)
Context: ...ets and ECR repositories are empty. > [!NOTE] > Some resources have `prevent_des...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
infrastructure/README.md
20-20: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
43-43: Blank line inside blockquote
(MD028, no-blanks-blockquote)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
81-81: Blank line inside blockquote
(MD028, no-blanks-blockquote)
85-85: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
91-91: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
99-99: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
119-119: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
120-120: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
121-121: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
129-129: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
169-169: Blank line inside blockquote
(MD028, no-blanks-blockquote)
177-177: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
183-183: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
203-203: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
215-215: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
216-216: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
217-217: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
218-218: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
219-219: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ 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). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (29)
infrastructure/modules/storage/variables.tf (1)
8-8: ✓ Consistent description formatting applied across all variables.All variable descriptions have been updated to end with periods, improving consistency and alignment with documentation standards. These formatting changes are minor but beneficial for maintainability.
Also applies to: 13-13, 18-18, 23-23
.pre-commit-config.yaml (1)
19-20: Verify that the.tflint.hclfile exists and is properly formatted.The
__GIT_WORKING_DIR__placeholder and--args=--config=syntax are valid and officially supported by pre-commit-terraform (documented in their README with examples matching this exact pattern). However, confirm thatinfrastructure/.tflint.hclexists and contains valid HCL configuration before merging.Likely an incorrect or invalid review comment.
infrastructure/backend/main.tf (2)
15-21: Well-structured tagging pattern with centralized locals.The introduction of
locals.common_tagsprovides a consistent, reusable tagging foundation across backend resources. The use ofmerge()to combine common tags with resource-specific Name tags is a clean, extensible pattern that aligns with the PR's tagging standardization objective.
2-2: Version pinning approach aligns with project standards.Exact version pinning (e.g.,
"1.14.0"instead of"~> 1.14") ensures reproducibility in this testing environment. Based on learnings, this deliberate choice is consistent with OWASP/Nest infrastructure practices.Also applies to: 6-6, 10-10
infrastructure/backend/variables.tf (1)
2-2: Description improved for clarity.The revised description is more concise and direct, removing unnecessary phrasing. This aligns with the PR's objective to standardize variable descriptions across modules.
infrastructure/README.md (1)
1-255: Documentation formatting improvements enhance readability.The README has been updated with improved indentation, clearer step sequencing, and better organization of code blocks and notes. These changes align with the PR's broader consistency and clarity improvements. The formatting changes use GitHub's standard alert syntax (
> [!NOTE],> [!WARNING]), which is correct and renders properly on GitHub.infrastructure/modules/cache/main.tf (2)
2-2: Version pinning follows project standard.Exact version constraints align with OWASP/Nest infrastructure practices for reproducibility.
Also applies to: 7-7, 11-11
86-92: Verify KMS CMK requirement for SSM parameters in testing environment.The new
aws_ssm_parameterresource usestype = "SecureString", which encrypts the value with AWS's managed key by default. However, Checkov flagged this with CKV_AWS_337, which requires a customer-managed KMS key.Given that this infrastructure is for testing purposes (per learnings), clarify whether the additional overhead of managing a customer-managed KMS key is necessary. If the testing environment does not require this level of encryption, you may suppress this check via a Checkov comment or update project guidelines.
infrastructure/modules/networking/modules/vpc-endpoint/variables.tf (1)
2-2: Variable descriptions improved for consistency and clarity.The updated descriptions are more explicit, include proper articles, and maintain consistent grammar. Examples help clarify variable content (e.g., "The environment (e.g., staging, production)."). These changes align with the PR's descriptive consistency objective across the infrastructure codebase.
Also applies to: 7-7, 12-12, 17-17, 22-22, 27-27, 32-32, 37-37, 42-42
infrastructure/modules/storage/main.tf (1)
53-53: Consistent tagging applied to IAM policy.Adding
tags = var.common_tagsto theaws_iam_policyresource extends the PR's tagging standardization objective to IAM resources, improving resource discoverability and cost allocation across the infrastructure.infrastructure/modules/ecs/main.tf (2)
2-2: Version pinning aligns with project standards.Exact version constraints ensure reproducible builds in the testing environment.
Also applies to: 7-7
83-83: Consistent tagging applied across IAM policies.All three IAM policies (
ecs_tasks_execution_role_ssm_policy,ecs_tasks_execution_policy,event_bridge_ecs_policy) now includetags = var.common_tags, aligning with the PR's tagging standardization objective. This enables better resource tracking and cost allocation across the ECS infrastructure.Also applies to: 119-119, 200-200
infrastructure/modules/cache/outputs.tf (2)
7-7: Minor description improvement for consistency.Adding a period to the
redis_primary_endpointdescription maintains consistency with the descriptive standardization throughout the PR.
1-4: Secrets exposed via ARN instead of direct output.The new
redis_password_arnoutput follows the PR's objective to use secrets' ARN instead of exposing raw values, improving security by allowing downstream consumers to fetch the actual secret from AWS Systems Manager Parameter Store.Verify that all downstream consumers (e.g.,
parametersmodule, ECS task definitions) have been updated to referenceredis_password_arninstead of the previously removedredis_auth_tokenoutput.infrastructure/modules/parameters/main.tf (2)
1-14: Exact version pinning follows team conventions and ensures reproducibility.The provider versions are correctly pinned to exact versions (Terraform 1.14.0, AWS 6.22.0, Random 3.7.2), which aligns with the team's preference for maximum reproducibility in the testing environment. This is the right approach for this codebase.
16-175: SSM parameter configuration and tagging are consistent.All SSM parameters properly apply
tags = var.common_tagsand uselifecycle.ignore_changes = [value]to allow manual updates via AWS console without Terraform conflicts. This approach is consistent with the established pattern in the parameters module and supports the staged deployment workflow.infrastructure/modules/parameters/outputs.tf (1)
1-22: ARN-based credential references decouple password management from parameter distribution.The output has been renamed to
django_ssm_parameter_arnsand now referencesvar.db_password_arnandvar.redis_password_arninstead of managing passwords directly. This approach decentralizes password generation and management to the database and cache modules, while this module acts as a parameter aggregator. Verify that the database and cache modules correctly export these ARN outputs and that the staging module properly wires them as inputs.infrastructure/modules/database/main.tf (3)
1-14: Exact version pinning provides reproducibility consistency.Provider versions are correctly exact-pinned across Terraform and AWS provider, aligning with team conventions. This ensures consistent behavior across deployments in the testing environment.
16-35: Password generation with fallback to input variable is sound, with appropriate character restrictions.The locals block (lines 16–19) implements a sensible pattern: generate a random password only if none is provided, otherwise use the input. The
random_passwordresource explicitly avoids problematic special characters that could cause issues with database connection strings or shell escaping. This is a pragmatic approach for handling secrets in infrastructure code.
80-86: KMS CMK encryption for SSM parameters is planned as a follow-up enhancement.The
aws_ssm_parameterresource currently uses AWS-managed keys. While Checkov flags this (CKV_AWS_337) as a security best practice, KMS CMK encryption for sensitive SSM parameters in this module is planned to be implemented after S3 state management is completed. For the current testing infrastructure, AWS-managed keys are acceptable as an interim measure.Ensure this work is tracked in the project roadmap and prioritize implementation once S3 state management is in place.
infrastructure/staging/outputs.tf (1)
1-19: Output descriptions are now consistently punctuated.The descriptions have been standardized with trailing periods, improving consistency across the infrastructure codebase. No functional changes to output values.
infrastructure/modules/security/outputs.tf (1)
1-24: Output descriptions are now consistently formatted.Descriptions have been updated with trailing periods for consistency across the security module outputs. No functional changes to the security group ID references.
infrastructure/modules/security/main.tf (4)
12-56: Security groups now defined without inline rules to prevent Terraform drift.The security groups are now defined with empty inline rule blocks, allowing external
aws_security_group_ruleresources to manage all ingress and egress rules independently. This approach directly addresses the PR objective to "fix bug where security module updates aws_security_group.x each apply despite no changes." By separating rule management, Terraform will no longer detect unintended drift when rules are managed externally.
58-96: Review egress rule scope and alignment with "tighten egress" PR objective.The new
ecs_egress_allandlambda_egress_allrules (lines 58–66, 78–86) allow all outbound traffic (protocol = "-1", ports 0–65535) to CIDR blocks defined invar.default_egress_cidr_blocks. The PR objectives state "Tighten egress rules (currently all outbound traffic allowed)," but these rules still permit unrestricted outbound traffic.Verify that:
var.default_egress_cidr_blocksis configured restrictively in the staging environment (e.g., not["0.0.0.0/0"])- These rules are intentionally broad for testing purposes, or if they should be tightened further
If the intent is to restrict egress to specific destinations (e.g., only to RDS, Redis, and VPC endpoints), consider removing these catch-all rules and explicitly list allowed destinations instead.
98-162: RDS and RDS Proxy rules use count-based conditionals correctly.Rules for RDS and RDS Proxy ingress/egress properly use
countto depend onvar.create_rds_proxy:
- When
create_rds_proxy = false: RDS accepts direct connections from ECS and Lambda (lines 98–118)- When
create_rds_proxy = true: RDS accepts only from RDS Proxy, and RDS Proxy accepts from ECS/Lambda with egress to RDSThis conditional approach ensures correct rule configuration based on the deployment topology.
164-182: Lambda-to-Redis ingress rule is a new addition; verify the requirement.A new security group rule (lines 174–182) explicitly allows Lambda to connect to Redis on the Redis port. This is a functional addition not visible in prior versions. Confirm:
- Lambda functions in this deployment actually need direct Redis access
- If this was previously missing and caused issues, document the fix in the commit message
infrastructure/modules/database/variables.tf (3)
7-122: Description consistency improvements align with PR objectives.All variable descriptions have been updated to follow a consistent format with trailing periods and clear, standardized wording. This improves documentation clarity and maintainability across the module.
70-74: Verify password handling withdefault = null.The
db_passwordvariable now hassensitive = trueanddefault = null, which aligns with the PR objective to "Use secrets' ARN instead of outputs." Confirm that:
- The module's
main.tfcorrectly handles thenulldefault and retrieves the actual password from AWS Secrets Manager using an ARN.- The RDS resource creation doesn't fail if no password is explicitly provided (i.e., the password generation or retrieval from Secrets Manager is mandatory in the implementation).
1-122: Verify variable validation blocks.The PR summary states "Add validation for variables — done," but the provided file does not contain
validationblocks. Confirm whether:
- Variable validation constraints were added to this file but are not shown in the provided context.
- Validation was implemented in
main.tfvia resource-level constraints instead.- Validation is enforced elsewhere (e.g., in a
.tfvarsfile or external tooling like TFLint).Run
rg -n 'validation' infrastructure/modules/database/to search for validation blocks across all files in the database module.
2 participants
Resolves #2809
Proposed change
pre-commithooks..terraform.lock.hclfrom modules.securitymodule updatesmodule.security.aws_security_group.xeach timeterraform applyis used, despite no changes.Checklist
make check-testlocally; all checks and tests passed.