Add tests and checks for cache and database module#3333
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIncludes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (3)
infrastructure/Makefile (1)
3-10: Declaretest-infrastructureas.PHONYtarget.Since this target doesn't produce a file, it should be marked as phony to ensure it always runs when invoked.
Suggested fix
MODULES_DIR := infrastructure/modules +.PHONY: test-infrastructure test-infrastructure: @for dir in $(MODULES_DIR)/*/; do \ if [ -d "$$dir/tests" ]; then \ echo "Testing $$dir..."; \ terraform -chdir="$$dir" init -backend=false -input=false && \ terraform -chdir="$$dir" test || exit 1; \ fi \ doneNote: The static analysis warnings about missing
all,clean, andtesttargets are false positives since this is an included Makefile, not a standalone one.infrastructure/modules/cache/tests/cache.tftest.hcl (1)
121-128: Consider set-based comparison for robustness.The
tolist()conversion on a set can produce non-deterministic ordering when multiple subnets are involved. While the current single-subnet test will pass, for future-proofing with multiple subnets, consider using set comparison:♻️ Suggested fix
run "test_subnet_group_uses_provided_subnets" { command = plan assert { - condition = tolist(aws_elasticache_subnet_group.main.subnet_ids) == var.subnet_ids + condition = toset(aws_elasticache_subnet_group.main.subnet_ids) == toset(var.subnet_ids) error_message = "Subnet group must use the provided subnet IDs." } }infrastructure/modules/database/variables.tf (1)
133-141: Validation may conflict with optional proxy creation.The
proxy_security_group_idsvariable requires at least one security group, butcreate_rds_proxydefaults tofalse. This forces users to provide a dummy security group ID even when not creating a proxy.Consider adding a default empty list and moving the non-empty check to a precondition on the RDS proxy resource:
♻️ Suggested approach
variable "proxy_security_group_ids" { description = "A list of security group IDs to associate with the RDS proxy." type = list(string) + default = [] - - validation { - condition = length(var.proxy_security_group_ids) > 0 - error_message = "proxy_security_group_ids must contain at least one security group." - } }Then in
main.tf, add a precondition on the RDS proxy resource:resource "aws_db_proxy" "main" { count = var.create_rds_proxy ? 1 : 0 # ... lifecycle { precondition { condition = length(var.proxy_security_group_ids) > 0 error_message = "proxy_security_group_ids must contain at least one security group when create_rds_proxy is true." } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefileinfrastructure/Makefileinfrastructure/modules/cache/.terraform.lock.hclinfrastructure/modules/cache/tests/cache.tftest.hclinfrastructure/modules/cache/variables.tfinfrastructure/modules/database/.terraform.lock.hclinfrastructure/modules/database/tests/database.tftest.hclinfrastructure/modules/database/variables.tf
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/modules/database/tests/database.tftest.hcl
📚 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/database/.terraform.lock.hclinfrastructure/modules/cache/.terraform.lock.hcl
🪛 checkmake (0.2.2)
infrastructure/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Gitleaks (8.30.0)
infrastructure/modules/cache/tests/cache.tftest.hcl
[high] 107-107: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
(hashicorp-tf-password)
⏰ 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 frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (24)
infrastructure/modules/database/.terraform.lock.hcl (1)
1-45: LGTM!The lockfile correctly pins exact versions for both AWS (6.22.0) and Random (3.7.2) providers, aligning with the team's preference for exact version pinning for maximum reproducibility. Based on learnings from previous reviews.
Makefile (1)
5-5: LGTM!The include directive is correctly placed alongside other sub-Makefiles, maintaining consistency with the existing structure.
infrastructure/modules/database/tests/database.tftest.hcl (5)
1-14: LGTM!Well-structured test variables block with appropriate defaults for plan-based testing.
16-50: Good coverage of security and naming conventions.These tests validate critical security properties (no public access, deletion protection, correct engine) and naming conventions. The assertions are clear and error messages are descriptive.
52-72: Good coverage of conditional password generation.Tests correctly verify the count-based conditional logic for
random_password.db_passwordresource creation.
74-120: Thorough RDS proxy validation.Tests cover proxy creation, engine family, disabled state, and TLS requirement—all essential for validating the conditional proxy configuration.
122-147: LGTM!SSM parameter and storage encryption tests properly validate secrets management (SecureString type, path format) and data-at-rest encryption.
infrastructure/modules/cache/variables.tf (4)
44-47: LGTM!Validation correctly enforces the ElastiCache node type naming convention using a regex anchor.
54-57: LGTM!Appropriate range validations for node count and port number.
Also applies to: 64-67
74-77: LGTM!Requiring at least one security group and subnet ensures the cache cluster can be properly networked.
Also applies to: 101-104
85-88: LGTM!Reasonable bounds for snapshot retention (0-30 days).
infrastructure/modules/cache/.terraform.lock.hcl (1)
1-45: LGTM!The lockfile correctly pins exact versions for both providers (AWS 6.22.0 and Random 3.7.2), which aligns with the team's preference for maximum reproducibility. Based on learnings, this follows the established pattern for Terraform provider version management in this repository.
infrastructure/modules/cache/tests/cache.tftest.hcl (5)
1-11: Good test setup with comprehensive coverage.The test variables are well-structured with appropriate mock values for plan-based testing. The test suite validates critical security properties including encryption, auth token generation, and secure parameter storage.
13-29: LGTM!Auth token tests properly validate that the token is generated and has the expected 32-character length for security.
31-56: LGTM!Encryption tests correctly validate that both at-rest and in-transit encryption are enabled, which are essential security requirements for ElastiCache.
58-78: LGTM!Failover tests correctly validate the conditional behavior where automatic failover is disabled for single-node clusters and enabled for multi-node clusters. The variable override approach for the multi-node scenario is appropriate.
103-119: LGTM!These tests correctly validate that the Redis password is stored as a SecureString in SSM Parameter Store and follows the expected path convention.
Note: The Gitleaks warning on line 107 is a false positive—
django_redis_passwordis a Terraform resource name reference, not an actual password value being exposed.infrastructure/modules/database/variables.tf (7)
16-21: LGTM!The minimum 20 GB validation aligns with AWS RDS requirements for most storage types and database engines.
27-31: LGTM!The 0-35 day range correctly reflects AWS RDS backup retention limits.
66-70: LGTM!The regex validation for the "db." prefix is a good safeguard against accidentally using EC2 instance types.
101-105: LGTM!The validation covers the commonly used SSD-based storage types for RDS.
111-116: LGTM!Requiring at least one subnet ensures proper DB subnet group creation.
147-151: LGTM!The 0-30 day range aligns with AWS Secrets Manager recovery window constraints.
157-161: LGTM!Requiring at least one security group ensures the RDS instance has proper network security configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrastructure/modules/database/variables.tf (1)
101-105: Consider adding "standard" storage type for completeness.The validation covers modern storage types but omits "standard" (magnetic), which AWS still supports for backwards compatibility. If this is intentional to discourage deprecated storage, this is fine as-is.
Optional: Include all valid storage types
validation { - condition = contains(["gp2", "gp3", "io1", "io2"], var.db_storage_type) - error_message = "db_storage_type must be one of: gp2, gp3, io1, io2." + condition = contains(["gp2", "gp3", "io1", "io2", "standard"], var.db_storage_type) + error_message = "db_storage_type must be one of: gp2, gp3, io1, io2, standard." }infrastructure/modules/database/tests/database.tftest.hcl (1)
15-157: Consider adding tests for validation error cases.The test suite comprehensively covers successful scenarios but doesn't test that the new validation blocks correctly reject invalid inputs (e.g.,
db_allocated_storage < 20, invaliddb_storage_type, emptysecurity_group_ids). Terraform test supportsexpect_failuresfor this purpose.Example: Add validation failure tests
run "test_invalid_allocated_storage_rejected" { command = plan variables { db_allocated_storage = 10 } expect_failures = [ var.db_allocated_storage, ] } run "test_invalid_storage_type_rejected" { command = plan variables { db_storage_type = "invalid" } expect_failures = [ var.db_storage_type, ] } run "test_empty_security_groups_rejected" { command = plan variables { security_group_ids = [] } expect_failures = [ var.security_group_ids, ] }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
infrastructure/modules/cache/tests/cache.tftest.hclinfrastructure/modules/database/main.tfinfrastructure/modules/database/tests/database.tftest.hclinfrastructure/modules/database/variables.tf
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/modules/cache/tests/cache.tftest.hcl
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
📚 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/modules/database/tests/database.tftest.hclinfrastructure/modules/database/variables.tf
🪛 Gitleaks (8.30.0)
infrastructure/modules/database/tests/database.tftest.hcl
[high] 64-64: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
(hashicorp-tf-password)
[high] 128-128: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
(hashicorp-tf-password)
⏰ 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 Code Scan
- GitHub Check: Run CI Dependencies Scan
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (9)
infrastructure/modules/database/variables.tf (5)
16-20: LGTM! Validation correctly enforces AWS RDS minimum storage.The 20 GB minimum aligns with AWS RDS requirements for gp2/gp3 storage types.
27-31: LGTM! Validation matches AWS backup retention limits.The 0-35 day range correctly reflects AWS RDS backup retention constraints.
66-70: LGTM! Instance class prefix validation is appropriate.The regex ensures valid RDS instance class naming convention.
111-115: LGTM! Non-empty list validations for required network resources.Both
db_subnet_idsandsecurity_group_idscorrectly enforce at least one entry, which is required for RDS deployment.Also applies to: 153-157
136-136: LGTM! Default change and validation align with the new precondition.The empty default for
proxy_security_group_idscombined with the lifecycle precondition inmain.tfensures proper validation only whencreate_rds_proxyis true. The secret recovery window validation correctly reflects AWS limits.Also applies to: 143-147
infrastructure/modules/database/main.tf (1)
147-153: LGTM! Lifecycle precondition correctly enforces security group requirement.The precondition ensures
proxy_security_group_idsis not empty when creating an RDS proxy, providing a clear error message at plan time rather than failing during apply.infrastructure/modules/database/tests/database.tftest.hcl (3)
1-13: LGTM! Test variables provide good baseline configuration.The common test variables cover all required inputs with sensible defaults for testing.
60-71: Safe to ignore: Gitleaks false positive for test password.The static analysis flagged line 64 as a password exposure risk. This is a false positive—the hardcoded password is intentionally used to test the conditional password generation logic and is not a real secret. Based on learnings, this infrastructure is for quick testing purposes only.
124-131: Safe to ignore: Gitleaks false positive for SSM parameter type check.The static analysis flagged line 128, but this assertion only checks the parameter
typeattribute equals "SecureString"—it doesn't expose any password value.
There was a problem hiding this comment.
I always avoided committing lock files inside modules, as they served no purpose since the main (staging or production) lock file would take precedence.
Committing them seems to cause no problems but improves test speed (terraform init is faster).
There was a problem hiding this comment.
Yeah, it seems it's fine to have them in git
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-cache-db-tests
branch
from
e360c36 to
3cd9741
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @infrastructure/modules/cache/tests/cache.tftest.hcl:
- Around line 1-11: Add an explicit log_retention_in_days variable to the shared
variables block so the test doesn't rely on the module default; define
var.log_retention_in_days with the expected integer value used by
test_log_groups_created (referenced in the test at lines 84 and 89) alongside
the other variables (e.g., add log_retention_in_days = <expected_days>) to make
the test's expectation explicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
infrastructure/modules/cache/tests/cache.tftest.hclinfrastructure/modules/database/main.tfinfrastructure/modules/database/tests/database.tftest.hclinfrastructure/modules/database/variables.tf
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/modules/database/tests/database.tftest.hcl
- infrastructure/modules/database/variables.tf
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🪛 Gitleaks (8.30.0)
infrastructure/modules/cache/tests/cache.tftest.hcl
[high] 107-107: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
(hashicorp-tf-password)
⏰ 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: CodeQL (javascript-typescript)
🔇 Additional comments (2)
infrastructure/modules/database/main.tf (1)
148-154: LGTM!Good addition of a lifecycle precondition to enforce that security groups are provided when creating the RDS proxy. This provides a clear, early failure with an actionable error message rather than letting Terraform fail with a less descriptive error or potentially create an insecure proxy configuration.
infrastructure/modules/cache/tests/cache.tftest.hcl (1)
13-128: Comprehensive test coverage for the cache module.The test suite effectively validates critical security and configuration aspects:
- Auth token generation and length
- Encryption at rest and in transit
- Automatic failover behavior based on node count
- SSM parameter security (SecureString type)
- Naming conventions and path formats
The static analysis warning on line 107 is a false positive—the test asserts that the SSM parameter type is
SecureString, it does not expose any credential values.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @infrastructure/modules/database/variables.tf:
- Around line 28-31: Update the validation block for
var.db_backup_retention_period so the condition allows values up to 35 (change
the upper bound from 30 to 35) and update the error_message to say
"db_backup_retention_period must be between 0 and 35." Locate the validation
block that references var.db_backup_retention_period and modify both the
condition and the error_message accordingly.
- Around line 144-147: The validation for var.secret_recovery_window_in_days
currently allows 0–30 but must enforce AWS Secrets Manager's 7–30 day
constraint; update the validation condition in the validation block for
var.secret_recovery_window_in_days to require var.secret_recovery_window_in_days
>= 7 && var.secret_recovery_window_in_days <= 30 and adjust the error_message to
indicate the valid range is 7–30 days (mention that immediate deletion uses
ForceDeleteWithoutRecovery instead of a 0 value).
🧹 Nitpick comments (2)
infrastructure/Makefile (1)
1-8: Add.PHONYdeclaration for the target.Since
test-infrastructureis not a file but a phony target, it should be declared as such to prevent issues if a file with that name ever exists.♻️ Suggested fix
+.PHONY: test-infrastructure + test-infrastructure: @for dir in infrastructure/modules/*/; do \ if [ -d "$$dir/tests" ]; then \ echo "Testing $$dir..."; \ terraform -chdir="$$dir" init -backend=false -input=false && \ terraform -chdir="$$dir" test || exit 1; \ fi \ doneNote: The checkmake warnings about missing
all,clean, andtesttargets can be safely ignored since this Makefile is included from the root Makefile where those targets likely exist.infrastructure/modules/database/variables.tf (1)
102-105: Consider addingstandardstorage type.AWS RDS also supports
standard(magnetic) storage type. While rarely used for new deployments, excluding it may cause issues if someone needs to use it for specific use cases.♻️ Suggested fix
validation { - condition = contains(["gp2", "gp3", "io1", "io2"], var.db_storage_type) - error_message = "db_storage_type must be one of: gp2, gp3, io1, io2." + condition = contains(["gp2", "gp3", "io1", "io2", "standard"], var.db_storage_type) + error_message = "db_storage_type must be one of: gp2, gp3, io1, io2, standard." }Based on learnings, this infrastructure is for quick testing purposes, so restricting to modern SSD-based storage types is acceptable if intentional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/Makefileinfrastructure/modules/database/variables.tf
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/Makefile
📚 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/Makefile
🪛 checkmake (0.2.2)
infrastructure/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ 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). (2)
- GitHub Check: Run CI Dependencies Scan
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
infrastructure/modules/database/variables.tf (5)
17-20: LGTM!The validation correctly enforces the AWS RDS minimum storage requirement of 20 GB for most database engines.
67-70: LGTM!The regex validation correctly ensures the instance class follows AWS naming conventions.
112-115: LGTM!Good validation to ensure at least one subnet is provided for the DB subnet group.
133-137: LGTM!Setting an explicit empty list default is appropriate. Per the AI summary, there's a corresponding precondition in
main.tfonaws_db_proxythat enforces non-empty security groups whencreate_rds_proxyis true.
154-157: LGTM!Good validation to ensure at least one security group is provided for the RDS database.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @infrastructure/modules/database/variables.tf:
- Around line 143-148: The current validation on variable
secret_recovery_window_in_days permits 1–6 which AWS rejects; update the
validation condition to allow either 0 or 7–30 by changing it to
(var.secret_recovery_window_in_days == 0) || (var.secret_recovery_window_in_days
>= 7 && var.secret_recovery_window_in_days <= 30) and update the error_message
to state "secret_recovery_window_in_days must be 0 or between 7 and 30." Ensure
this change is applied to the validation block that references
secret_recovery_window_in_days.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/modules/database/variables.tf
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2026-01-13T15:15:35.282Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 3333
File: infrastructure/modules/database/variables.tf:144-147
Timestamp: 2026-01-13T15:15:35.282Z
Learning: In Terraform (AWS provider), the aws_secretsmanager_secret resource's recovery_window_in_days can be set to 0 to trigger immediate deletion, mapping to AWS's ForceDeleteWithoutRecovery. This contrasts with the AWS API, which documents RecoveryWindowInDays as 7–30. Review any variable/module usage of recovery_window_in_days in infrastructure/ modules (e.g., infrastructure/modules/database/variables.tf) to ensure this behavior is intentional and documented. If using 0 for immediate deletion, add a clear comment and ensure it aligns with your deletion policies and compliance requirements.
Applied to files:
infrastructure/modules/database/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). (2)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (7)
infrastructure/modules/database/variables.tf (7)
16-21: LGTM!The validation correctly enforces the AWS RDS minimum storage requirement of 20 GB.
27-32: LGTM!The validation range of 0–35 days aligns with AWS RDS backup retention limits.
66-71: LGTM!The regex validation correctly ensures all RDS instance class names start with
db..
101-106: LGTM!The validation covers the modern RDS storage types. Omitting the legacy
standard(magnetic) type is reasonable.
111-116: LGTM!Requiring at least one subnet is appropriate for DB subnet group creation.
136-136: LGTM!The empty list default works correctly with the precondition in
main.tfthat enforces non-empty whencreate_rds_proxyis true.
153-158: LGTM!Requiring at least one security group for RDS is a sensible baseline validation.
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-cache-db-tests
branch
from
fa7fa76 to
814038b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
infrastructure/Makefile (1)
1-8: Add.PHONYdeclaration for the target.The
test-infrastructuretarget doesn't produce a file, so it should be declared as phony to ensure it always runs regardless of filesystem state.Suggested change
+.PHONY: test-infrastructure test-infrastructure: @for dir in infrastructure/modules/*/; do \ if [ -d "$$dir/tests" ]; then \ echo "Testing $$dir..."; \ terraform -chdir="$$dir" init -backend=false -input=false && \ terraform -chdir="$$dir" test || exit 1; \ fi \ doneNote: The static analysis warnings about missing
all,clean, andtesttargets are false positives since this Makefile is included from the root Makefile.infrastructure/modules/database/variables.tf (2)
112-115: Consider requiring at least 2 subnets for Multi-AZ deployments.AWS RDS subnet groups typically require subnets in at least 2 different Availability Zones for Multi-AZ deployments. The current validation allows a single subnet, which would fail during apply for Multi-AZ configurations. Based on learnings, this is testing infrastructure, so this may be intentional.
♻️ Optional: Stricter validation for Multi-AZ support
validation { - condition = length(var.db_subnet_ids) > 0 - error_message = "db_subnet_ids must contain at least one subnet." + condition = length(var.db_subnet_ids) >= 2 + error_message = "db_subnet_ids must contain at least two subnets in different AZs for Multi-AZ support." }
102-105: Consider including "standard" (magnetic) storage type.AWS RDS supports
standard(magnetic) storage type for PostgreSQL, though it is legacy and available only for certain older instance types or regions. Adding it to the validation would prevent unexpected failures if this storage type is needed for backward compatibility or specific use cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
infrastructure/Makefileinfrastructure/modules/cache/tests/cache.tftest.hclinfrastructure/modules/database/main.tfinfrastructure/modules/database/tests/database.tftest.hclinfrastructure/modules/database/variables.tf
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/modules/database/main.tf
- infrastructure/modules/cache/tests/cache.tftest.hcl
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2026-01-13T15:15:35.282Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 3333
File: infrastructure/modules/database/variables.tf:144-147
Timestamp: 2026-01-13T15:15:35.282Z
Learning: In Terraform (AWS provider), the aws_secretsmanager_secret resource's recovery_window_in_days can be set to 0 to trigger immediate deletion, mapping to AWS's ForceDeleteWithoutRecovery. This contrasts with the AWS API, which documents RecoveryWindowInDays as 7–30. Review any variable/module usage of recovery_window_in_days in infrastructure/ modules (e.g., infrastructure/modules/database/variables.tf) to ensure this behavior is intentional and documented. If using 0 for immediate deletion, add a clear comment and ensure it aligns with your deletion policies and compliance requirements.
Applied to files:
infrastructure/modules/database/variables.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/Makefileinfrastructure/modules/database/tests/database.tftest.hcl
📚 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/Makefile
🪛 checkmake (0.2.2)
infrastructure/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Gitleaks (8.30.0)
infrastructure/modules/database/tests/database.tftest.hcl
[high] 64-64: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
(hashicorp-tf-password)
[high] 128-128: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches.
(hashicorp-tf-password)
🔇 Additional comments (15)
infrastructure/modules/database/variables.tf (5)
17-20: LGTM!The validation correctly enforces AWS RDS minimum allocated storage requirement of 20 GB.
28-31: LGTM!The validation correctly matches AWS RDS backup retention period limits (0-35 days).
67-70: LGTM!Good use of regex validation to ensure instance class follows AWS naming convention.
144-147: LGTM!The validation correctly allows 0 for immediate deletion (ForceDeleteWithoutRecovery) or the standard 7-30 day range per AWS Secrets Manager API behavior. Based on learnings, this aligns with the documented Terraform provider behavior.
154-157: LGTM!Requiring at least one security group ensures the RDS instance has network access controls in place.
infrastructure/modules/database/tests/database.tftest.hcl (10)
1-13: LGTM!The shared variables block provides sensible defaults for testing, with all required variables properly defined. The values satisfy the validation constraints defined in
variables.tf.
15-22: LGTM!Good security test ensuring the database is not publicly accessible by default.
24-31: LGTM!Validates consistent naming convention for database identifiers.
33-40: LGTM!Important test verifying deletion protection is enabled by default.
42-49: LGTM!Confirms the database engine is correctly set to PostgreSQL.
51-71: Static analysis false positive - test password is intentional.The Gitleaks warning for line 64 is a false positive. This is a test file where
db_password = "test-password-123"is intentionally used to verify the password-not-generated logic path. The test correctly validates that:
- Password is auto-generated when not provided (lines 51-58)
- Password generation is skipped when explicitly provided (lines 60-71)
73-108: LGTM!Good coverage of RDS proxy creation scenarios, testing both enabled and disabled states with proper security group configuration.
110-122: LGTM!Important security test ensuring TLS is required for proxy connections.
124-140: Static analysis false positive - this checks SSM parameter type, not a password value.The Gitleaks warning for line 128 is a false positive. The test is asserting that the SSM parameter's
typeattribute equals"SecureString", which is a security best practice verification—not password exposure. The test correctly validates that database passwords are stored securely in SSM Parameter Store.
142-158: LGTM!Good tests verifying storage encryption is enabled and subnet group configuration is correct. The use of
toset()for subnet comparison handles ordering differences appropriately.
There was a problem hiding this comment.
Yeah, it seems it's fine to have them in git
arkid15r
merged commit 1966639
into
OWASP:feature/nest-zappa-migration
rudransh-shrivastava
deleted the
feature/nest-zappa-migration-cache-db-tests
branch
2 participants
Proposed change
Resolves #3251
Add tests and checks for:
cachemoduledatabasemoduleAdd a make target
make test-infrastructureChecklist
make check-testlocally: all warnings addressed, tests passed