Add tests for frontend module#3581
Conversation
…igration-frontend-tests
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces a new KMS module and wires its key across multiple modules; refactors the frontend to use ECR/ECS with new task/service and IAM roles; enables KMS encryption for multiple CloudWatch log groups and secrets; adds VPC flow logs and routing resources; simplifies ALB/HTTPS logic; updates staging and CI variable handling; and adds TFTest suites for frontend and KMS. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/modules/frontend/main.tf (1)
14-20: Add KMS encryption to the CloudWatch log group.The log group is currently unencrypted. Add
kms_key_id = var.cloudwatch_log_kms_key_idto the resource and define the corresponding variable. Update the test to verify encryption is applied.Note: This same pattern applies across other CloudWatch log groups in the infrastructure (networking, cache, ecs/task modules). Consider a coordinated rollout if this is part of a broader encryption initiative.
Suggested change
resource "aws_cloudwatch_log_group" "frontend" { name = "/aws/ecs/${var.project_name}-${var.environment}-frontend" retention_in_days = var.log_retention_in_days + kms_key_id = var.cloudwatch_log_kms_key_id tags = merge(var.common_tags, { Name = "${var.project_name}-${var.environment}-frontend-logs" }) }
🤖 Fix all issues with AI agents
In `@infrastructure/modules/frontend/main.tf`:
- Around line 22-33: Update the ECR repository resource
aws_ecr_repository.frontend to set image_tag_mutability = "IMMUTABLE" (replacing
"MUTABLE") and update the test named
test_frontend_ecr_repository_image_tag_mutability in
infrastructure/modules/frontend/tests/frontend.tftest.hcl to assert
image_tag_mutability == "IMMUTABLE" so the test reflects the hardened
configuration.
|
It looks |
|
There are some suggestions that are valid and I can incorporate. Like setting up AWS KMS encryption key and not using |
|
Hey @arkid15r, some questions related to the semgrep scan:
|
Yes if possible. We use http->https redirect for current Nginx based solution AFAIR.
Feel free to suppress it while leaving a TODO. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infrastructure/modules/ecs/modules/task/main.tf (1)
12-19: Add validation forkms_key_arnvariable and consider making it optional.KMS encryption for CloudWatch logs is a good security improvement. However, the
kms_key_arnvariable (defined in variables.tf:60-63) lacks ARN format validation and has no default value, making it a required parameter with no fallback to AWS-managed encryption.Consider either:
- Adding validation to enforce ARN format (e.g.,
validation { condition = ... }), or- Making it optional with a
default = nullto allow fallback to AWS-managed keys when KMS is not requiredThis would improve robustness and flexibility of the module.
infrastructure/modules/alb/main.tf (1)
130-142: Add ACM certificate validation dependency to prevent apply failures.The HTTPS listener references a newly created ACM certificate without waiting for it to reach ISSUED status. AWS ALBs reject PENDING_VALIDATION certificates—the first apply will fail with a validation error, requiring manual DNS validation and a second apply. Either add
aws_acm_certificate_validationto wait for issuance before creating the listener, or document that a two-phase deployment workflow is required (create cert → complete DNS validation externally → re-apply).
🤖 Fix all issues with AI agents
In `@infrastructure/modules/alb/outputs.tf`:
- Around line 36-43: The output description for https_listener_arn is stale — it
says "(null if HTTPS disabled)" but nginx now always creates HTTPS; update the
description for the output "https_listener_arn" (value
aws_lb_listener.https.arn) to remove the null caveat and reflect that the ARN is
always provided (e.g., "The ARN of the HTTPS listener.").
In `@infrastructure/modules/database/main.tf`:
- Around line 63-67: The RDS proxy cannot decrypt the secret stored in
aws_secretsmanager_secret.db_credentials because its IAM policy only includes
secretsmanager:GetSecretValue; update the IAM policy or role attached to the RDS
proxy (the policy/resource that grants the proxy access, e.g.,
aws_iam_policy.rds_proxy_policy or aws_iam_role.rds_proxy_role) to also allow
"kms:Decrypt" on the CMK used by var.kms_key_arn (or the aws_kms_key resource if
defined), ensuring the policy includes kms:Decrypt with the correct resource ARN
so the proxy can decrypt the secret.
🧹 Nitpick comments (2)
infrastructure/modules/networking/variables.tf (1)
58-61: Add light validation forkms_key_arnto fail fast.
A minimal check helps catch empty or malformed values before they reach resource configuration.Suggested validation
variable "kms_key_arn" { description = "The ARN of the KMS key." type = string + + validation { + condition = can(regex("^arn:", var.kms_key_arn)) + error_message = "kms_key_arn must be a valid KMS key ARN." + } }infrastructure/modules/database/main.tf (1)
45-45: Consider documenting or restoring configurability for log exports.
Hard-coding the exports removes the previous knob; if intentional, a short note in module docs helps prevent surprises.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/nest-zappa-migration #3581 +/- ##
===============================================================
Coverage ? 85.50%
===============================================================
Files ? 463
Lines ? 14271
Branches ? 1900
===============================================================
Hits ? 12202
Misses ? 1689
Partials ? 380
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Hey @arkid15r , I've resolved the semgrep scans. You can either: back up the current state > recreate the backend > initialize with the new backend > push the backed-up state. Commands to push and pull state: Or just recreate the backend directly if it has no existing state. |
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-frontend-tests
branch
from
77b6df2 to
3bea5cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@infrastructure/backend/main.tf`:
- Around line 91-94: The KMS key used by module.kms (referenced as
module.kms.key_arn) needs a policy statement allowing the DynamoDB service
principal to use the key; update the KMS key policy attached to your KMS
resource (e.g., the aws_kms_key or aws_kms_key_policy for the key created in the
kms module) to add a Statement that grants Principal service
"dynamodb.amazonaws.com" the actions "kms:Decrypt", "kms:Encrypt",
"kms:GenerateDataKey*" and "kms:Describe*" on the key resource (or "*" if the
policy is scoped broadly); add any necessary Conditions (e.g., SourceArn) if you
want to restrict to the specific DynamoDB table, then reapply Terraform so
module.kms.key_arn can be used by the encrypted DynamoDB table.
In `@infrastructure/staging/main.tf`:
- Around line 143-160: Add a validation block to the variable "domain_name"
declaration to reject empty strings; specifically add a validation that checks
length(var.domain_name) > 0 and provides a clear error_message (e.g.,
"domain_name must not be empty."), since main.tf interpolates domain_name into
allowed_origins, nextauth_url, server_csrf_url, server_graphql_url and
allowed_hosts and empty values would produce invalid URLs.
🧹 Nitpick comments (1)
infrastructure/modules/database/main.tf (1)
45-45: Hardcoded log exports remove configurability.The
enabled_cloudwatch_logs_exportsis now hardcoded to["postgresql", "upgrade"]instead of using a variable. This reduces flexibility for environments that may need different log types (e.g., adding"error"logs).Consider if this is intentional or if the variable should be retained for flexibility.
rudransh-shrivastava
force-pushed
the
feature/nest-zappa-migration-frontend-tests
branch
from
d70e384 to
ec6e4e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrastructure/modules/networking/modules/nacl/tests/nacl.tftest.hcl`:
- Around line 47-63: The test error messages are inconsistent with the assert
conditions in run "test_private_nacl_associations_count" and run
"test_public_nacl_associations_count": the asserts check
length(aws_network_acl_association.private) == 2 and
length(aws_network_acl_association.public) == 2 but the error_message texts say
"should have 3 subnet associations." Update the error_message strings to match
the expected count of 2 (e.g., "Private NACL should have 2 subnet associations."
and "Public NACL should have 2 subnet associations.") or, if the correct
expectation is 3, change the assert conditions to == 3 for
aws_network_acl_association.private and aws_network_acl_association.public so
the condition and message agree.
infrastructure/modules/networking/modules/nacl/tests/nacl.tftest.hcl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrastructure/modules/frontend/tests/frontend.tftest.hcl`:
- Around line 1-15: The test fixture declares aws_region = "us-east-2" but
target_group_arn embeds "us-east-1", causing inconsistent region data; update
the value of target_group_arn in the variables block so its region matches
aws_region (change the ARN region segment from us-east-1 to us-east-2) or
alternatively set aws_region to "us-east-1" so both match; edit the variables
entry named target_group_arn to reflect the chosen region consistently with
aws_region.
🧹 Nitpick comments (2)
infrastructure/modules/cache/tests/cache.tftest.hcl (1)
4-4: Add assertions that log groups use the provided KMS key.
You addedkms_key_arn, but the test suite never asserts it’s applied. This could let a misconfiguration slip through.✅ Suggested test additions
run "test_log_groups_created" { command = plan assert { condition = aws_cloudwatch_log_group.engine_log.retention_in_days == var.log_retention_in_days error_message = "Engine log group must be created with correct retention." } + assert { + condition = aws_cloudwatch_log_group.engine_log.kms_key_id == var.kms_key_arn + error_message = "Engine log group must be encrypted with the provided KMS key." + } + assert { condition = aws_cloudwatch_log_group.slow_log.retention_in_days == var.log_retention_in_days error_message = "Slow log group must be created with correct retention." } + + assert { + condition = aws_cloudwatch_log_group.slow_log.kms_key_id == var.kms_key_arn + error_message = "Slow log group must be encrypted with the provided KMS key." + } }Also applies to: 82-93
infrastructure/modules/frontend/tests/frontend.tftest.hcl (1)
53-60: Consider parameterizing image tag mutability.Hard-coding
"MUTABLE"in the test makes it harder to harden later (e.g., to"IMMUTABLE"). If the module can support a toggle, this test could assert against a variable and/or add a second scenario.
|
| DOMAIN_NAME: ${{ vars.DOMAIN_NAME }} | ||
| ECS_USE_FARGATE_SPOT: true | ||
| ECS_USE_PUBLIC_SUBNETS: false | ||
| ENVIRONMENT: "staging" |
There was a problem hiding this comment.
Will update in the testing CI/CD PR.
| temp_path = Path(temp_dir) | ||
| new_archive_path = temp_path / "new.tar.gz" | ||
|
|
||
| # nosemgrep: trailofbits.python.tarfile-extractall-traversal.tarfile-extractall-traversal # noqa: ERA001, E501 |
There was a problem hiding this comment.
I suggest using NOSEMGREP for consistency w/ Sonar.
arkid15r
merged commit 0e67f96
into
OWASP:feature/nest-zappa-migration
2 participants
Proposed change
Resolves #3546
Add tests for frontend module
Add kms module.
Resolve semgrep scans
Checklist
make check-testlocally: all warnings addressed, tests passed