Add logging in Infrastructure #2743
Conversation
rudransh-shrivastava
changed the base branch from
main
to
feature/nest-zappa-migration
github-actions
bot
added
docs
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds CloudWatch logging for ElastiCache and VPC flow logs, standardizes log retention to 90 days, and refactors storage into a reusable S3 bucket module with provider/version updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-logging
branch
from
d0b95ea to
b489cad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
infrastructure/README.md (1)
53-55: Add brief explanation for.tfbackendbackend configuration files.Line 55 mentions "set
regionin a.tfbackendfile" but doesn't explain what this file is or how it differs from.tfvars. Add a note explaining that.tfbackendfiles configure Terraform's remote backend storage location (S3 bucket name, DynamoDB table, region). This clarification helps new contributors understand the multi-environment setup.infrastructure/staging/.terraform.lock.hcl (1)
27-29: Simplify redundant Random provider constraints.The Random provider constraints "~> 3.0,
> 3.5" are redundant; the second constraint is a subset of the first. Consider simplifying to just "> 3.5" for clarity.Apply this change to simplify the constraint:
- constraints = "~> 3.0, ~> 3.5" + constraints = "~> 3.5"infrastructure/modules/storage/modules/s3-bucket/outputs.tf (1)
1-4: Consider exporting specific bucket attributes instead of the full resource.The bucket output returns the full
aws_s3_bucket.thisresource object, which allows downstream consumers to access any bucket property but creates tight coupling to the internal resource structure. If consumers only need specific attributes (name, ID, or ARN), consider exporting those explicitly instead.Standard practice:
output "bucket_name" { description = "The name of the S3 bucket" value = aws_s3_bucket.this.id } output "bucket_id" { description = "The ID of the S3 bucket" value = aws_s3_bucket.this.id }How is the bucket output consumed by
infrastructure/modules/storage/outputs.tfor other callers? If only specific attributes are used, refactoring to explicit outputs would reduce coupling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.gitignore(1 hunks)cspell/custom-dict.txt(1 hunks)infrastructure/README.md(2 hunks)infrastructure/backend/.terraform.lock.hcl(1 hunks)infrastructure/backend/main.tf(1 hunks)infrastructure/backend/outputs.tf(1 hunks)infrastructure/backend/variables.tf(1 hunks)infrastructure/modules/cache/main.tf(2 hunks)infrastructure/modules/cache/variables.tf(1 hunks)infrastructure/modules/ecs/modules/task/main.tf(1 hunks)infrastructure/modules/ecs/modules/task/variables.tf(1 hunks)infrastructure/modules/networking/main.tf(2 hunks)infrastructure/modules/networking/variables.tf(1 hunks)infrastructure/modules/storage/main.tf(1 hunks)infrastructure/modules/storage/modules/s3-bucket/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(0 hunks)infrastructure/staging/.terraform.lock.hcl(1 hunks)infrastructure/staging/backend.tf(1 hunks)infrastructure/staging/main.tf(8 hunks)infrastructure/staging/providers.tf(1 hunks)infrastructure/staging/variables.tf(1 hunks)
💤 Files with no reviewable changes (1)
- infrastructure/modules/storage/variables.tf
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 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:
.gitignore
📚 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:
.gitignore
📚 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/staging/providers.tfinfrastructure/staging/variables.tfinfrastructure/staging/backend.tfinfrastructure/staging/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/staging/backend.tfinfrastructure/backend/variables.tf
📚 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.mdinfrastructure/modules/storage/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
⏰ 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 (30)
cspell/custom-dict.txt (1)
130-130: Add custom spell-check entry for Terraform backend files.The
/aws/prefix is the standard naming convention for AWS service log groups, and this dictionary entry appropriately supports that infrastructure pattern.infrastructure/backend/.terraform.lock.hcl (1)
4-6: Verify AWS provider version compatibility.The lockfile pins AWS provider 6.22.0. Confirm this version is compatible with the Terraform 1.14.0 requirement introduced in the backend configuration and with any other infrastructure modules in use.
infrastructure/modules/ecs/modules/task/variables.tf (1)
54-58: Updated default log retention to 90 days, consistent across modules.This aligns with the cache and networking modules' retention configuration. Verify this retention period meets observability and compliance requirements for your environment.
.gitignore (1)
28-28: Appropriately exclude Terraform backend and variable files from version control.The new patterns (
*.tfbackend,*.tfvars) prevent accidental commits of sensitive configuration and credentials, while the negation (!*.tfvars.example) preserves example templates for contributors. This follows Terraform best practices.Also applies to: 31-32
infrastructure/modules/ecs/modules/task/main.tf (1)
12-18: Log group name updated to follow AWS service naming conventions.Log groups created by AWS services follow the standard naming convention like
/aws/lambda/[log-group-name], and the updated path/aws/ecs/...aligns with this convention. Note: existing log groups retain their old naming; only new deployments will use the/aws/ecs/prefix. If you need to migrate historical logs, plan for that separately.infrastructure/modules/networking/variables.tf (1)
17-21: Added log retention variable for VPC flow logs.The new variable enables consistent log retention configuration across modules. Verify that this variable is referenced in the networking module's main.tf for VPC flow log group retention.
infrastructure/backend/outputs.tf (1)
1-9: Standard Terraform backend outputs for state management.The outputs expose the backend infrastructure resources for downstream reference. Verify that the referenced resources (
aws_dynamodb_table.state_lockandaws_s3_bucket.state) are properly defined in infrastructure/backend/main.tf.infrastructure/README.md (1)
18-65: Clear, multi-step infrastructure setup guide with backend/staging separation.The reorganized documentation properly sequences backend provisioning, staging infrastructure, and Zappa deployment. Directory navigation paths use relative commands appropriately. Verify that the backend/ and staging/ directory structure exists as documented.
infrastructure/backend/variables.tf (1)
1-29: Well-structured backend variables.The variables are properly defined with clear descriptions, appropriate types, and sensible defaults. The aws_region default aligns with staging environment configuration.
infrastructure/modules/storage/modules/s3-bucket/variables.tf (1)
1-28: Well-designed module variables with appropriate defaults.The required bucket_name variable forces explicit naming, while force_destroy defaults to false for safety. Reusing lifecycle variables maintains consistency with backend configuration.
infrastructure/backend/main.tf (4)
1-9: Clarify the tight Terraform version constraint.The Terraform version is pinned to exactly 1.14.0 (line 2), which may cause issues when upgrading or working in environments with different Terraform versions. Consider using a range constraint like
~> 1.14.0or>= 1.14.0unless strict pinning is required for backend compatibility.Is the exact Terraform version constraint (1.14.0) intentional, or should it allow patch/minor version flexibility?
66-78: Clarify the purpose of NOSONAR comments.The S3 bucket resources have NOSONAR comments (lines 66 and 73) without explanation. These typically suppress SonarQube security or quality rules. Please clarify which rule(s) are being suppressed and why, or add a comment explaining the rationale.
What is the specific SONAR rule being suppressed? Consider adding an inline comment explaining the suppression if it's intentional (e.g.,
# NOSONAR: S3 bucket naming enforced by Terraform AWS provider).
11-47: Security posture is well-configured.The backend infrastructure properly implements:
- HTTPS-only policy on state bucket (denies non-secure transport).
- Public access blocks on both buckets.
- Server-side encryption with AES256.
- Versioning on both buckets for recovery and audit trail.
- Logging from state bucket to a dedicated logs bucket.
- DynamoDB point-in-time recovery for state locking.
This aligns with security best practices for Terraform backend state management.
80-110: Lifecycle policies properly parameterized.The lifecycle rules correctly use variables (abort_incomplete_multipart_upload_days, noncurrent_version_expiration_days, expire_log_days) for configuration, allowing consumers to adjust retention policies without modifying the module.
infrastructure/staging/backend.tf (2)
1-9: Consider parameterizing hardcoded backend resource names.The backend configuration hardcodes bucket and DynamoDB table names (lines 3-4) to specific values that depend on the
project_namevariable frominfrastructure/backend/main.tf. Ifproject_namechanges or differs across deployments, this configuration will fail.Consider:
- Using Terraform variable interpolation (though backend blocks don't support variables directly).
- Generating this file from a template or script that injects the correct names.
- Documenting the constraint that
project_namemust always be "owasp-nest" for this configuration to work.Is
project_namefixed to "owasp-nest" across all environments, or could it vary? If it can vary, how should the backend reference be updated?
5-7: Backend security configuration is appropriate.Encryption is enabled and the region aligns with staging configuration. The state key prefix "staging/" properly isolates staging state from other environments.
infrastructure/staging/providers.tf (1)
1-3: Clean provider configuration.The AWS provider is properly configured to use the region variable, allowing environment-specific region selection while maintaining a sensible default.
infrastructure/staging/.terraform.lock.hcl (2)
4-6: Verify AWS provider constraint syntax.The AWS provider constraint "~> 6.0, 6.22.0" appears to mix a range constraint with a specific version. Verify with Terraform documentation that this is the intended syntax. Typically, this would pin to 6.22.0, but the mixed constraint may be confusing.
Is the constraint "
> 6.0, 6.22.0" for AWS provider the intended syntax, or should it be simplified to "6.22.0" or "> 6.22.0"?
5-6: Provider version bump aligns with backend infrastructure.The AWS provider version is bumped to 6.22.0, which aligns with the required_providers constraint in infrastructure/backend/main.tf. This ensures consistency across backend and staging configurations.
infrastructure/modules/cache/variables.tf (1)
18-22: Log retention variable properly defined and consistent.The log_retention_in_days variable uses a sensible default of 90 days, aligning with other modules (networking, ecs) and backend logging configuration. This supports the PR objective of adding logging to the cache module.
Verify that the
log_retention_in_daysvariable is consumed ininfrastructure/modules/cache/main.tfto create CloudWatch log groups or logging configurations. Variable definitions alone don't implement logging; ensure the backing resources are created to fulfill the PR objective.infrastructure/modules/storage/modules/s3-bucket/outputs.tf (1)
6-9: ARN output is well-designed.The arn output clearly exports the bucket ARN, which is commonly needed for IAM policies and other configurations.
infrastructure/staging/variables.tf (1)
4-4: Region migration appears intentional and consistent.The region changes from us-east-1 to us-east-2 are properly aligned across availability_zones. Verify that this change is intentional and that backend/provider configuration (providers.tf, backend.tf) align with the new default region.
Also applies to: 10-10
infrastructure/modules/storage/outputs.tf (1)
6-19: Modularization outputs correctly reference new s3-bucket submodule.The refactored outputs correctly derive from module.fixtures_bucket and module.zappa_bucket. Ensure that the s3-bucket submodule (infrastructure/modules/storage/modules/s3-bucket/outputs.tf) correctly exports
bucketandarnattributes.infrastructure/staging/main.tf (2)
21-21: Module source paths correctly updated to reference ../modules.The restructuring from
./modules/<name>to../modules/<name>is consistent across all module blocks, aligning with the repository reorganization where modules are now located one level up from staging.Also applies to: 35-35, 55-55, 69-69, 81-81, 96-96, 108-108
1-10: I apologize for the repeated clone failures. Unfortunately, I'm unable to access the repository to verify this review comment at this time.However, based on my knowledge of Terraform, I can provide context: The review concern is technically valid — in Terraform, child modules do NOT automatically inherit
required_providersdeclarations from parent modules. Ifinfrastructure/modules/cache/main.tfusesrandom_passwordresource, it must explicitly declare the random provider in its ownterraformblock, or receive it via a provider argument from the parent module.To complete the verification, I would need:
The content of
infrastructure/modules/cache/main.tf— particularly:
- Lines 1-20 (to see if it has a
terraformblock withrequired_providers)- Line 42 and surrounding context (to confirm
random_passwordusage)How the cache module is invoked in
infrastructure/staging/main.tf— specifically, whether a provider is passed to itOnce you can provide this information or the repository becomes accessible, I can complete the verification and determine whether:
- The review comment is accurate and identifies a real issue
- The random provider is properly defined and the review comment is incorrect
- There's a valid Terraform pattern being used that the review didn't account for
Would you be able to share these details?
infrastructure/modules/storage/main.tf (2)
11-21: Policy refactoring improves clarity and specificity.The data source rename to
fixtures_read_onlyand the shift from a broad Deny-all policy to an explicits3:GetObjectAllow policy is clearer and more maintainable. The resource scoping viavar.fixtures_s3_bucketis appropriate.
23-46: Modularization successfully eliminates code duplication.The refactoring to use module-based S3 bucket provisioning for both fixtures and zappa buckets reduces duplication and ensures consistent security baselines. Both buckets inherit the ForceHTTPS, versioning, encryption, and public access block configurations from the s3-bucket module.
Verify that the zappa bucket's use case (Zappa deployments) does not require additional IAM policies beyond what Zappa manages at Lambda deployment time (per the Zappa architecture, Lambda IAM roles are handled by Zappa, not Terraform).
infrastructure/modules/storage/modules/s3-bucket/main.tf (1)
11-47: Repository access unavailable; manual verification required.The verification request cannot be completed due to repository cloning failure. The review comment's suggestion to validate variable definitions in
infrastructure/modules/storage/modules/s3-bucket/variables.tfrequires direct codebase inspection, which is currently inaccessible. Additionally, the comment references variables (abort_incomplete_multipart_upload_days,noncurrent_version_expiration_days) that do not appear in the provided code snippet, requiring clarification on whether these are used elsewhere in the module or are orphaned references.infrastructure/modules/cache/main.tf (1)
22-80: The repository clone failed, which prevents me from directly verifying the variable definition. However, I can address the review comment appropriately based on this situation.Verify that
log_retention_in_daysis defined ininfrastructure/modules/cache/variables.tfwith an appropriate default value.The CloudWatch logging configuration for the cache module is well-structured with proper log group creation and delivery configuration to CloudWatch Logs using JSON formatting. However, manual verification is required to confirm that
var.log_retention_in_daysis properly defined in the variables file with a sensible default value (e.g., 7, 14, or 30 days depending on your retention requirements).infrastructure/modules/networking/main.tf (1)
16-77: Unable to complete variable verification due to repository access issues.The codebase could not be accessed for verification. However, based on the code review of lines 16-77, the VPC flow logs infrastructure is properly structured with correctly scoped IAM policies and CloudWatch log group configuration. The conditional requirement remains: verify that
var.log_retention_in_daysis defined with appropriate type (number) and default/required status ininfrastructure/modules/networking/variables.tfbefore merging.
|
Rebasing now... |
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-logging
branch
from
bad1887 to
079221d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrastructure/modules/networking/main.tf (1)
52-60: Consider documenting traffic volume and cost implications of comprehensive flow logging.With traffic_type set to "ALL" and retention at 90 days, VPC Flow Logs may generate substantial volume depending on your traffic patterns. This is likely intentional for observability, but ensure the data volume and associated storage costs are reviewed as part of operational planning.
infrastructure/modules/storage/outputs.tf (1)
11-14: Clarify output value vs. description.The output description says "The name of the S3 bucket" but the value is
module.zappa_bucket.bucket, which exports the full aws_s3_bucket resource object (not the name string). This will work in most Terraform contexts, but for clarity and explicit intent, consider accessing the bucket name directly.Apply this diff to make the output explicit:
output "zappa_s3_bucket" { description = "The name of the S3 bucket for Zappa deployments" - value = module.zappa_bucket.bucket + value = module.zappa_bucket.bucket.id }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
infrastructure/modules/cache/main.tf(2 hunks)infrastructure/modules/cache/variables.tf(1 hunks)infrastructure/modules/ecs/modules/task/main.tf(1 hunks)infrastructure/modules/ecs/modules/task/variables.tf(1 hunks)infrastructure/modules/networking/main.tf(2 hunks)infrastructure/modules/networking/variables.tf(1 hunks)infrastructure/modules/storage/main.tf(1 hunks)infrastructure/modules/storage/modules/s3-bucket/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(0 hunks)
💤 Files with no reviewable changes (1)
- infrastructure/modules/storage/variables.tf
🚧 Files skipped from review as they are similar to previous changes (3)
- infrastructure/modules/storage/modules/s3-bucket/variables.tf
- infrastructure/modules/ecs/modules/task/main.tf
- infrastructure/modules/cache/variables.tf
🧰 Additional context used
🧠 Learnings (4)
📚 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/modules/storage/modules/s3-bucket/main.tfinfrastructure/modules/storage/main.tf
📚 Learning: 2025-11-27T19:38:23.924Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2743
File: infrastructure/modules/storage/main.tf:1-9
Timestamp: 2025-11-27T19:38:23.924Z
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/storage/modules/s3-bucket/main.tfinfrastructure/modules/storage/main.tf
📚 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/modules/storage/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/modules/storage/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 frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (14)
infrastructure/modules/ecs/modules/task/variables.tf (1)
54-58: Consistent retention policy across modules — approved.The update aligns the ECS task log retention with cache and networking modules (all now default to 90 days), establishing a uniform observability retention strategy.
infrastructure/modules/cache/main.tf (2)
22-32: Comprehensive ElastiCache logging setup — approved.Two CloudWatch log groups properly configured for engine and slow logs with consistent naming conventions and tags. Log retention is parameterized via module variable for operational flexibility.
69-80: Log delivery configuration correct for ElastiCache CloudWatch integration.Both engine-log and slow-log are properly configured with appropriate log types, JSON format, and correct destination references. The separate log_delivery_configuration blocks follow Terraform best practices.
infrastructure/modules/networking/main.tf (3)
16-24: IAM assume role policy correctly scoped to VPC Flow Logs service.The policy grants the vpc-flow-logs.amazonaws.com service permission to assume the role, which is the correct and minimal scope for VPC Flow Logs.
26-35: IAM inline policy follows least-privilege principle.The policy grants only the necessary CloudWatch Logs permissions (CreateLogStream, PutLogEvents, DescribeLogStreams) scoped to the flow logs group ARN, with proper wildcard suffix for stream operations.
46-77: VPC Flow Logs infrastructure properly configured end-to-end.CloudWatch log group, IAM role, policy, and policy attachment are all correctly sequenced and referenced. The flow log resource (lines 52–60) properly binds the role, destination, and VPC with traffic_type set to "ALL" for comprehensive monitoring.
infrastructure/modules/networking/variables.tf (1)
17-21: Log retention variable properly aligned with cache and ECS modules.The new variable is consistently defaulted to 90 days, matching the retention policy pattern established across the infrastructure modules. Proper description and type definition.
infrastructure/modules/storage/modules/s3-bucket/outputs.tf (1)
1-9: LGTM!The module outputs correctly expose both the full bucket resource and its ARN, enabling flexible consumption by parent modules and external code.
infrastructure/modules/storage/outputs.tf (1)
6-9: LGTM!The ARN outputs correctly reference the module exports and maintain backward compatibility with the renamed bucket resources.
Also applies to: 16-19
infrastructure/modules/storage/modules/s3-bucket/main.tf (2)
11-36: Robust ForceHTTPS policy.The policy correctly enforces HTTPS-only access for all S3 operations on the bucket and its objects. This is a strong security control that prevents unencrypted data transit.
49-88: LGTM!Comprehensive and security-conscious S3 configuration: public access blocked, server-side encryption enabled, versioning enabled, and lifecycle policies configured. The module is well-structured and reusable.
infrastructure/modules/storage/main.tf (3)
11-21: LGTM!The policy correctly scoped to the fixtures bucket with read-only access. The rename and action restriction appropriately document intent and follow principle of least privilege.
23-41: LGTM!Clean modularization of S3 bucket resources. Both fixtures and zappa buckets are now provisioned via the reusable s3-bucket module with appropriate tags and configuration. This improves maintainability and consistency.
43-46: LGTM!IAM policy correctly references the updated data source and maintains the fixtures-specific read-only access control.
|
rudransh-shrivastava
requested review from
arkid15r and
kasya
as code owners
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Alright, let's try this out 🚀
arkid15r
merged commit e219f12
into
OWASP:feature/nest-zappa-migration
2 participants
Resolves #2738
Proposed change
Add VPC flow logs.
Refactor
cachemodule.Add logs for
cacheandstoragemodules.Update retention period for
ecsmodule.Checklist
make check-testlocally; all checks and tests passed.