Integrate AWS Parameter Store for Zappa/Infrastructure deployments #2551
Conversation
Summary by CodeRabbitRelease Notes
WalkthroughIntroduces a new SSM Parameters module exposing ARNs, moves Django config into SSM, replaces container env vars with SSM-backed secrets, renames Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
|
Test: This PR must be linked to an issue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
infrastructure/modules/parameters/variables.tf (2)
1-52: Add validation to prevent invalid parameter values.The required variables accept any string values. Consider adding validation blocks for database port numbers and environment names to catch configuration errors early:
validation { condition = try(tonumber(var.db_port) > 0 && tonumber(var.db_port) < 65536, false) error_message = "db_port must be a valid port number (1-65535)." }However, note that
db_portis already a string here, so validation would require conversion.
23-26: Consider explicit type conversion for clarity:db_port = tostring(var.db_port)The root module (
infrastructure/variables.tfline 50) definesdb_portasnumber, but it's passed directly to this parameters module (line 90 ininfrastructure/main.tf) which expectsstring. While Terraform implicitly coerces the number to string and the code works, making the conversion explicit in the module call would improve clarity.Since SSM Parameter Store values are always strings, keeping
db_portasstringhere is correct—just add explicit conversion at the call site:db_port = tostring(var.db_port)ininfrastructure/main.tfline 90.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
infrastructure/main.tf(3 hunks)infrastructure/modules/cache/main.tf(2 hunks)infrastructure/modules/cache/variables.tf(0 hunks)infrastructure/modules/database/main.tf(2 hunks)infrastructure/modules/database/variables.tf(1 hunks)infrastructure/modules/ecs/main.tf(10 hunks)infrastructure/modules/ecs/modules/task/main.tf(1 hunks)infrastructure/modules/ecs/modules/task/variables.tf(1 hunks)infrastructure/modules/ecs/variables.tf(1 hunks)infrastructure/modules/parameters/main.tf(1 hunks)infrastructure/modules/parameters/outputs.tf(1 hunks)infrastructure/modules/parameters/variables.tf(1 hunks)infrastructure/outputs.tf(0 hunks)infrastructure/variables.tf(1 hunks)
💤 Files with no reviewable changes (2)
- infrastructure/modules/cache/variables.tf
- infrastructure/outputs.tf
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
🪛 Checkov (3.2.334)
infrastructure/modules/parameters/main.tf
[high] 16-26: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 28-38: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 52-62: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 64-74: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 104-110: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 128-138: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 148-154: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 156-162: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 176-186: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 188-198: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 200-210: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
⏰ 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). (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (18)
infrastructure/modules/database/variables.tf (1)
81-84: Consistent variable naming across module boundaries.This rename improves naming consistency with other modules and external references. The change is straightforward and the interface remains clear.
infrastructure/modules/database/main.tf (1)
58-58: Variable references properly updated.Both occurrences of
var.db_usernamehave been correctly updated tovar.db_user, maintaining consistency with the variable declaration. The Secrets Manager integration is properly configured with RDS Proxy.Also applies to: 74-74
infrastructure/modules/ecs/modules/task/main.tf (1)
46-49: Verify ECS task execution role has SSM parameter read permissions.The change from inline environment variables to SSM parameter references requires the ECS task execution role to have
ssm:GetParametersandssm:GetParameterpermissions for the referenced parameter ARNs. Ensure the task execution role policy grants access to these resources.infrastructure/modules/ecs/modules/task/variables.tf (1)
17-21: Clear variable rename with updated documentation.The rename from
container_environmenttocontainer_parameters_arnsaccurately reflects the new purpose of passing SSM parameter ARNs. The updated description clearly communicates the expected format.infrastructure/modules/cache/main.tf (1)
17-19: Behavioral change: Redis auth token generation is now mandatory.Previously, the module conditionally generated the auth token based on
generate_redis_auth_token. Now the token is always generated (line 31:count = 1) and the local.redis_auth_token always references the generated token (line 18). This removes the ability to provide an external token and simplifies the configuration. Ensure this aligns with your infrastructure strategy and update any documentation/runbooks accordingly.Also applies to: 31-31
infrastructure/modules/ecs/variables.tf (1)
12-16: Correct removal of sensitive flag from parameter ARN variable.ARNs themselves are not sensitive data—they're resource identifiers. The removal of the
sensitiveflag is appropriate; only actual secrets should be marked sensitive, not references to them.infrastructure/main.tf (2)
83-96: New parameters module orchestrates SSM parameter storage.The new
parametersmodule is properly positioned as a central hub, consuming infrastructure outputs (DB credentials, Redis endpoint, password) and creating SSM parameters for application consumption. The dependency flow is clear:database→parameters→ecs.Verify that referenced outputs exist:
module.database.db_proxy_endpoint(line 87)module.database.db_password(line 89)module.cache.redis_primary_endpoint(line 94)module.cache.redis_auth_token(line 95)
62-62: Verify container_parameters_arns output structure.The
container_parameters_arnsis passed to the ECS module (line 62) frommodule.parameters.ssm_parameter_arns. Ensure this output is a map where keys are environment variable names and values are SSM parameter ARNs, matching the expected format for the ECS task'ssecretsblock.infrastructure/modules/parameters/main.tf (3)
156-162: Verify Django secret key generation entropy and configuration.The Django secret key is generated with
length = 50andspecial = true. Verify this meets your Django security requirements:
- Django recommends using
django-insecure-prefix for development, but production should use a strong random key- Length of 50 with special characters is generally appropriate
- Confirm this aligns with your Django configuration validation
1-14: Verify module input variables are properly defined.The module references
var.db_host,var.db_name,var.db_password,var.redis_host, andvar.redis_password, among others. Ensureinfrastructure/modules/parameters/variables.tfdefines all required variables with appropriatesensitive = trueflags on credentials.
16-74: [SECURITY] SSM parameters should use customer-managed KMS CMK encryption.All
SecureStringparameters (and ideallyStringparameters containing sensitive data) are currently encrypted with the default AWS-managed key. For production deployments, use customer-managed KMS CMK encryption by adding thekms_key_idattribute:resource "aws_ssm_parameter" "django_db_password" { description = "Database password generated by Terraform." name = "/${var.project_name}/${var.environment}/DJANGO_DB_PASSWORD" tags = var.common_tags type = "SecureString" + kms_key_id = aws_kms_key.ssm_parameters.id value = var.db_password }This aligns with AWS security best practices and resolves Checkov's CKV_AWS_337 violations. You'll need to create a KMS key resource for SSM parameter encryption.
⛔ Skipped due to 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.infrastructure/modules/ecs/main.tf (3)
12-12: Data source for account ID injection is correct.The
data.aws_caller_identitydata source is needed to construct the least-privilege resource ARN for the SSM policy. This approach is sound.
125-125: Module parameter updates are consistent across all ECS task definitions.All 6 ECS task modules (sync_data, owasp_update_project_health_metrics, owasp_update_project_health_scores, migrate, load_data, index_data) have been updated to use
container_parameters_arnsinstead of the previouscontainer_environmentapproach.Also applies to: 146-146, 167-167, 188-188, 217-217, 246-246
46-61: All verifications passed—task module is properly integrated.The task module at
infrastructure/modules/ecs/modules/task/correctly:
- Defines the
container_parameters_arnsinput variable (type:map(string))- Passes these ARNs to the ECS task definition via the
secretsfield, which is the correct mechanism for SSM parameter referencesThe IAM policy (lines 46-61) grants
ssm:GetParameterson the parameter path${project_name}/${environment}/*, which properly supports this configuration. No additional changes are required.infrastructure/variables.tf (3)
68-76: Environment validation block is a good addition.The validation restricting
environmentto["staging", "production"]prevents invalid deployments and enforces consistency. This is a best practice for infrastructure-as-code.
1-142: No downstream impact from redis_auth_token removal – properly replaced via module interface.Verification confirms the removal is safe: the
redis_auth_tokenis still generated internally in the cache module and passed to the parameters module asredis_password, which correctly defines it as an input variable. The parameters module then stores it in SSM Parameter Store. The integration chain remains intact with no breaking changes.
62-66: All changes verified and correctly implemented.The variable rename from
db_usernametodb_useris consistent across all infrastructure code: it's properly referenced ininfrastructure/main.tf(lines 50, 91) and passed to both the parameters and database modules. The removal ofredis_auth_tokenfrom variables is safe—it's generated internally within the cache module, not a variable. The 18 Django environment variables have been correctly migrated to SSM Parameter Store resources in the parameters module (19 total parameters), withdjango_db_userproperly using the renamedvar.db_uservariable.infrastructure/modules/parameters/outputs.tf (1)
1-24: All SSM parameter resources are properly defined.Verification confirms that all 19 aws_ssm_parameter resources referenced in outputs.tf are correctly defined in infrastructure/modules/parameters/main.tf with matching names. No resource references will fail during Terraform apply.
rudransh-shrivastava
changed the base branch from
feature/nest-zappa-migration
to
main
rudransh-shrivastava
changed the base branch from
main
to
feature/nest-zappa-migration
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration
branch
from
5a804a6 to
3d9af1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/modules/security/main.tf (1)
33-113: Add conditional ingress rule for direct Lambda→RDS access when RDS Proxy is disabled.Verification confirms no direct Lambda→RDS ingress rule exists in the security module. Both RDS-related ingress rules are gated behind
var.create_rds_proxy ? 1 : 0:
rds_from_proxy(line 94): RDS Proxy → RDSrds_proxy_from_lambda(line 105): Lambda → RDS ProxyWhen
create_rds_proxy = false, Lambda cannot access RDS through either path. Add a thirdaws_security_group_ruleto allow direct Lambda→RDS access when the proxy is disabled:resource "aws_security_group_rule" "rds_from_lambda" { count = var.create_rds_proxy ? 0 : 1 type = "ingress" description = "PostgreSQL from Lambda" from_port = var.db_port to_port = var.db_port protocol = "tcp" security_group_id = aws_security_group.rds.id source_security_group_id = aws_security_group.lambda.id }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/settings/staging.py(0 hunks)infrastructure/README.md(1 hunks)infrastructure/main.tf(4 hunks)infrastructure/modules/database/main.tf(4 hunks)infrastructure/modules/database/outputs.tf(1 hunks)infrastructure/modules/database/variables.tf(2 hunks)infrastructure/modules/parameters/main.tf(1 hunks)infrastructure/modules/parameters/outputs.tf(1 hunks)infrastructure/modules/security/main.tf(3 hunks)infrastructure/modules/security/outputs.tf(1 hunks)infrastructure/modules/security/variables.tf(1 hunks)infrastructure/variables.tf(2 hunks)
💤 Files with no reviewable changes (1)
- backend/settings/staging.py
✅ Files skipped from review due to trivial changes (1)
- infrastructure/README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
📚 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/parameters/main.tf
🪛 Checkov (3.2.334)
infrastructure/modules/parameters/main.tf
[high] 16-26: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 28-38: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 80-86: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 104-114: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 124-130: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 132-138: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 152-162: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 164-174: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 176-186: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
⏰ 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 (14)
infrastructure/variables.tf (2)
68-72:db_userrename is well-aligned across variable surface.The rename from
db_usernametodb_useris consistent with the database module updates and parameter module expectations. This aligns well with the broader db_user wiring ininfrastructure/main.tfline 92 and the parameters module.
74-82: Environment validation block strengthens config robustness.The explicit validation enforcing
stagingorproductionis a good defensive measure and aligns with the environment-driven parameter scoping in the new parameters module.infrastructure/modules/database/outputs.tf (1)
7-10: Conditional proxy endpoint selection is properly implemented.The ternary correctly indexes the proxy endpoint with
[0]when enabled and falls back to the direct RDS instance endpoint when disabled. Since the RDS instance itself is not count-based, there is no risk of null reference.infrastructure/modules/database/variables.tf (1)
87-90:db_userrename is consistent across modules.The variable name aligns with usage in
infrastructure/main.tfline 92 and the parameters module wiring. Good consistency.infrastructure/modules/security/outputs.tf (1)
6-9: Verify database module handles nullproxy_security_group_idsgracefully.The output correctly returns
nullwhen RDS proxy is disabled. However,infrastructure/main.tfline 54 passes this to the database module:proxy_security_group_ids = [module.security.rds_proxy_sg_id]. When the proxy is disabled, this becomes[null], which may be passed toaws_db_proxy.mainas a list with a null element (line 137 in database/main.tf).Verify that the database module's
aws_db_proxy.mainresource handlesvpc_security_group_ids = [null]gracefully or is only created whencreate_rds_proxy = true. If the proxy is only created whencreate_rds_proxy = true, the null is not a problem. Otherwise, Terraform may reject the null value in the security group list.infrastructure/modules/database/main.tf (2)
58-58:db_uservariable references are correctly applied throughout resource definitions.Both the
aws_db_instance.mainresource (line 58) and theaws_secretsmanager_secret_version(line 74) correctly referencevar.db_user. This ensures that the database credentials stored in Secrets Manager match the RDS instance configuration.Also applies to: 74-74
79-139: RDS proxy resource structure and count logic are sound, but default value mismatch (already flagged) must be resolved.All proxy resources (IAM role, IAM policy, proxy itself, target group, proxy target) correctly use
count = var.create_rds_proxy ? 1 : 0. All indexed references correctly use[0]to access the single created resource. The IAM policy safely referencesaws_secretsmanager_secret.db_credentials.arn, which always exists regardless of proxy status.However, this approval is contingent on resolving the
create_rds_proxydefault value conflict flagged ininfrastructure/modules/database/variables.tfline 10. Once the default is aligned tofalse, the proxy resources will only be created when explicitly enabled, which is the correct behavior.infrastructure/modules/parameters/outputs.tf (1)
1-22: SSM parameter ARN output comprehensively exposes Django configuration.The new
ssm_parameter_arnsoutput maps 17 Django-related environment variables to their corresponding AWS SSM Parameter Store ARNs. This replaces the previousdjango_environment_variableslocal-variable approach and enables ECS tasks to fetch parameters securely from SSM at runtime via IAM policies rather than embedding values in task definitions or environment variables.The mapping includes essential Django config: database credentials/endpoint, Redis, Algolia, OpenAI, Sentry, Slack, and general Django settings.
Verify that all 17 SSM parameters referenced here are actually created in
infrastructure/modules/parameters/main.tfand that the ECS module (infrastructure/modules/ecs/main.tfor its task definition module) correctly consumescontainer_parameters_arnsto inject these parameters into task definitions.infrastructure/main.tf (4)
84-97: Newparametersmodule cleanly centralizes environment/config.The module orchestrates all Django configuration into AWS SSM parameters. The wiring sources credentials and endpoints from upstream modules (database, cache) and exposes them as ARNs to the ECS module via
container_parameters_arns. This implements the core objective of the PR: moving from environment variables to AWS-managed parameter storage.
42-42:create_rds_proxyanddb_userpropagation across modules is structurally correct.The root-level
create_rds_proxyis passed to bothdatabaseandsecuritymodules (lines 42, 103), anddb_useris correctly renamed throughout (line 51). This ensures coordinated provisioning of RDS proxy resources.Caveat: This approval is contingent on resolving the default value mismatch in
infrastructure/modules/database/variables.tfline 10 (defaults totrueinstead offalse). Once aligned, the propagation will work as intended.Also applies to: 51-51, 54-54, 103-103
58-70: ECS module migration fromdjango_environment_variablestocontainer_parameters_arnsis correct.Line 63 now wires
container_parameters_arns = module.parameters.ssm_parameter_arns, replacing the previous hardcoded environment variables. This enables ECS tasks to fetch config from SSM Parameter Store at runtime via IAM policy, eliminating the need to embed sensitive values in task definitions.Verify that the ECS module (specifically the task definition resources) has been updated to consume
container_parameters_arnsand apply IAM policies to allow SSM parameter access. This is a material security improvement but requires corresponding changes in the ECS module to function.
88-88: Parameter module sources are correctly linked to upstream module outputs.Line 88 sources
db_hostfrommodule.database.db_proxy_endpoint, which is conditional (returns proxy endpoint when enabled, direct RDS endpoint when disabled). Line 96 sourcesredis_passwordfrommodule.cache.redis_auth_token.Verify that the cache module still exposes
redis_auth_tokenoutput (or check if it was renamed/removed in parallel changes not shown in the provided code). If the cache module output was removed or renamed, this wiring will fail.Also applies to: 96-96
infrastructure/modules/parameters/main.tf (2)
188-191: Random string generation for Django SECRET_KEY is appropriate.The 50-character length with special characters aligns with Django best practices for a cryptographically strong secret key.
16-186: Verify whether KMS CMK encryption for SSM parameters should be prioritized now or deferred for testing infrastructure.The technical issue is confirmed: all 9 SecureString parameters lack
kms_key_id, which Checkov CKV_AWS_337 flags as a security concern. However, based on learnings from PR #2431, this infrastructure is designated for testing purposes only, where production-grade security hardening is intentionally deferred.Decide whether:
- KMS CMK encryption should be implemented now as a mandatory security baseline, or
- It should be deferred alongside other hardening controls (IAM database authentication, deletion protection, Performance Insights encryption) until production deployment.
If implementing now, apply
kms_key_idto these 9 SecureString parameters:django_algolia_application_id,django_algolia_write_api_key,django_db_password,django_open_ai_secret_key,django_redis_password,django_secret_key,django_sentry_dsn,django_slack_bot_token,django_slack_signing_secret.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/wsgi.py (1)
16-21: Set the paginator to recurse so deeper parameters loadRight now
get_parameters_by_pathreturns only the immediate children beneath the supplied path. If we later group secrets into nested subpaths (for example/owasp-nest/staging/django/...), those entries will never reachos.environ. PassingRecursive=Truekeeps the current behavior while future-proofing this loader.Apply this diff:
- response_iterator = paginator.paginate(Path=ssm_param_path, WithDecryption=True) + response_iterator = paginator.paginate( + Path=ssm_param_path, + WithDecryption=True, + Recursive=True, + )infrastructure/README.md (1)
58-62: Spell out which SSM path needs updatingCould we add a sentence here that points folks at the
/owasp-nest/<stage>Parameter Store path (or whatever stage-specific prefix they just provisioned) and reminds them those placeholder values were created as SecureStrings? That keeps new contributors from hunting around the console for the right entries.Consider appending something like “In the
/owasp-nest/<stage>hierarchy you just provisioned, edit each DJANGO_* parameter (they currently readto-be-set-in-aws-console) and save the real values as SecureString entries.”
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/wsgi.py(1 hunks)backend/zappa_settings.example.json(1 hunks)infrastructure/README.md(3 hunks)infrastructure/terraform.tfvars.example(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
📚 Learning: 2025-10-23T19:22:23.811Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
Applied to files:
infrastructure/README.md
📚 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/terraform.tfvars.example
⏰ 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 (4)
infrastructure/terraform.tfvars.example (3)
3-3: LGTM! Appropriate default for testing environment.Setting
create_rds_proxytofalseis a sensible default for testing/staging environments, as RDS Proxy adds additional cost and complexity.
4-4: LGTM! Acceptable for testing infrastructure.Setting
db_backup_retention_periodto0disables automated backups, which is appropriate for testing environments. Based on learnings, this infrastructure is intended for quick testing purposes only.
5-6: LGTM! Clear configuration values.The database name and user configuration is clear and follows good naming conventions. Note that sensitive credentials (passwords) have been correctly moved to AWS SSM Parameter Store per the PR objectives.
backend/zappa_settings.example.json (1)
4-19: Nice use of native Lambda environment variablesSwitching to
aws_environment_variablesensures the Parameter Store path is injected into the Lambda runtime itself, exactly as Zappa recommends for sensitive configuration.
|
|
||
| AWS_ACCESS_KEY_ID = values.SecretValue() | ||
| AWS_SECRET_ACCESS_KEY = values.SecretValue() | ||
| AWS_STORAGE_BUCKET_NAME = "owasp-nest" |
There was a problem hiding this comment.
I'm not entirely sure that we will need these, as we can use IAM. I plan to cover collectstatic in a different PR (related to ECS tasks).
There was a problem hiding this comment.
Is there anything other than collectstatic that uses the S3 bucket?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
infrastructure/modules/parameters/main.tf (2)
80-86: Addlifecycle.ignore_changesfor database password consistency.The
django_db_passwordparameter is aSecureStringbut lackslifecycle.ignore_changes, unlike other sensitive parameters (django_algolia_application_id,django_open_ai_secret_key, etc.). This creates an inconsistency: if the password is manually updated in the AWS console (common practice for production), Terraform will detect drift on the nextplan/apply.Add the lifecycle block for consistency:
resource "aws_ssm_parameter" "django_db_password" { description = "Database password generated by Terraform." name = "/${var.project_name}/${var.environment}/DJANGO_DB_PASSWORD" tags = var.common_tags type = "SecureString" value = var.db_password + + lifecycle { + ignore_changes = [value] + } }
16-186: Document the placeholder and lifecycle pattern for future maintainers.The module uses a clear pattern for externally-sourced or manually-updated secrets: placeholder values ("to-be-set-in-aws-console") combined with
lifecycle.ignore_changes = [value]. This is a reasonable approach for testing infrastructure, but consider adding an inline comment or updating the module's README to explain:
- Which parameters require manual setup in AWS console
- Why
lifecycle.ignore_changesis applied (to protect manual updates from Terraform drift detection)- How to verify all secrets have been properly configured before deployment
This will reduce confusion for future maintainers and operators.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/modules/parameters/main.tf(1 hunks)infrastructure/modules/parameters/variables.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/modules/parameters/variables.tf
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
📚 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/parameters/main.tf
🪛 Checkov (3.2.334)
infrastructure/modules/parameters/main.tf
[high] 16-26: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 28-38: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 80-86: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 104-114: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 124-130: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 132-138: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 152-162: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 164-174: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 176-186: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
⏰ 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/parameters/main.tf (2)
1-14: Provider configuration looks good.Terraform and provider versions are appropriately specified for this module.
188-191: Random secret key generation is appropriate.The
random_string.django_secret_keyresource is properly configured (length=50, special=true) to generate a cryptographically suitable DjangoSECRET_KEYvalue.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/modules/database/outputs.tf (1)
7-10: Output description could be more inclusive.The output now conditionally returns either an RDS proxy or RDS instance endpoint, but the description states "The endpoint of the RDS proxy". Consider updating it to reflect the dual nature:
output "db_proxy_endpoint" { - description = "The endpoint of the RDS proxy" + description = "The database endpoint (RDS proxy if enabled, otherwise RDS instance)" value = var.create_rds_proxy ? aws_db_proxy.main[0].endpoint : aws_db_instance.main.address }This clarifies the conditional behavior for downstream module consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/modules/database/outputs.tf(1 hunks)infrastructure/modules/security/main.tf(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:16-26
Timestamp: 2025-11-08T11:43:19.257Z
Learning: KMS CMK encryption for SSM SecureString parameters in infrastructure/modules/parameters/ is planned to be implemented after S3 state management is completed. Currently using AWS-managed keys for the testing infrastructure.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.715Z
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.
⏰ 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 (2)
infrastructure/modules/security/main.tf (2)
33-66: Security groups correctly restructured for conditional proxy support.The RDS and RDS Proxy security groups are properly set up with the
create_rds_proxyflag:
aws_security_group.rds(line 33): Always created, serves as primary database security group.aws_security_group.rds_proxy(line 50): Conditionally created withcount = var.create_rds_proxy ? 1 : 0, only exists when proxy is enabled.Naming and descriptions are clear and distinguish the two roles appropriately.
93-124: Security group rules correctly implement conditional proxy routing.The three new
aws_security_group_ruleresources enforce the correct traffic flow for both scenarios:
rds_from_lambda(line 93, count=0 when proxy enabled): Direct Lambda→RDS connection when proxy is disabled. Safely referencesaws_security_group.rds.idandaws_security_group.lambda.id, both of which always exist.
rds_from_proxy(line 104, count=1 when proxy enabled): Proxy→RDS connection when proxy is enabled. Correctly uses array indexingaws_security_group.rds_proxy[0].idwhich is only valid when the proxy security group is created.
rds_proxy_from_lambda(line 115, count=1 when proxy enabled): Lambda→Proxy connection when proxy is enabled. Again usesaws_security_group.rds_proxy[0].idwith correct array indexing.The mutually exclusive count conditions align perfectly with the two deployment scenarios. Extracting these rules from inline ingress blocks is a best practice that avoids configuration conflicts.
|
|
||
| AWS_ACCESS_KEY_ID = values.SecretValue() | ||
| AWS_SECRET_ACCESS_KEY = values.SecretValue() | ||
| AWS_STORAGE_BUCKET_NAME = "owasp-nest" |
| if not ssm_param_path: | ||
| return | ||
|
|
||
| from pathlib import Path |
There was a problem hiding this comment.
Any reason to import it here instead of top level?
(ai assisted?)
There was a problem hiding this comment.
Oh no particular reason, I think we can load pathlib at top level.
There was a problem hiding this comment.
I see you've fixed it in another PR, Thank you!
There was a problem hiding this comment.
The code was taken from this discussion: Miserlou/Zappa#1432
arkid15r
merged commit 0268515
into
OWASP:feature/nest-zappa-migration
…2551) * Use AWS SSM Parameter Store to handle environment variables * Use focused policy for read access * Update documentation * Add flag for create_rds_proxy * set default value of create_rds_proxy to false * Populate Zappa/Lambda environment variables from ssm/parameter store * Update documentation * Update example * add default configurations * add security group db from lambda
2 participants
Proposed change
Resolves #2538
*.example(s).create_rds_proxy.Checklist
make check-testlocally; all checks and tests passed.