Skip to content

Conversation

@rudransh-shrivastava
Copy link
Collaborator

@rudransh-shrivastava rudransh-shrivastava commented Dec 1, 2025

Resolves #2761

Proposed change

Make improvements to the networking module:

  • Add NACLs.
  • Add VPC endpoints.
  • Fix traffic going through NAT instead of VPC endpoints.

Checklist

  • I've read and followed the contributing guidelines.
  • I've run make check-test locally; all checks and tests passed.

@rudransh-shrivastava rudransh-shrivastava changed the base branch from main to feature/nest-zappa-migration December 1, 2025 16:31
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 1, 2025

Summary by CodeRabbit

  • New Features

    • Added network access control lists (ACLs) for private and public subnets with inbound and outbound traffic rules
    • Added VPC endpoints for AWS services: CloudWatch Logs, ECR, S3, Secrets Manager, and SSM
    • Added security group configuration for VPC endpoint access with HTTPS ingress rules
  • Chores

    • Updated Terraform to version 1.14.0 and AWS provider to 6.22.0
    • Removed unused provider dependencies

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Adds NACLs and VPC endpoints to the networking module, introduces corresponding variables/outputs and security-group egress rules, updates Terraform and AWS provider constraints, wires new module inputs/outputs into staging/security, and removes two module-level .terraform.lock.hcl files.

Changes

Cohort / File(s) Change Summary
Version & provider updates
infrastructure/modules/networking/main.tf, infrastructure/modules/security/main.tf
Bumped Terraform required_version to 1.14.0 and updated AWS provider constraint to 6.22.0. Removed random provider block from security module.
NACL module
infrastructure/modules/networking/modules/nacl/main.tf, infrastructure/modules/networking/modules/nacl/variables.tf
Added nacl module creating private/public aws_network_acl, subnet associations, and multiple inbound/outbound aws_network_acl_rules; added seven input variables (common_tags, environment, private_subnet_ids, project_name, public_subnet_ids, vpc_cidr, vpc_id).
VPC endpoint module
infrastructure/modules/networking/modules/vpc-endpoint/main.tf, .../variables.tf, .../outputs.tf
Added vpc-endpoint module: security group + ingress rule, interface endpoints (CloudWatch Logs, ECR API/DKR, Secrets Manager, SSM), gateway S3 endpoint with route table associations; added nine variables and exported security_group_id.
Module wiring & variables
infrastructure/modules/networking/main.tf, infrastructure/modules/networking/variables.tf, infrastructure/modules/networking/outputs.tf, infrastructure/modules/security/main.tf, infrastructure/modules/security/variables.tf, infrastructure/staging/main.tf
Declared nacl and vpc_endpoint modules; added aws_region var to networking and vpc_endpoint_sg_id var to security; wired module.vpc_endpoint.security_group_id into security module and passed aws_region from staging; added two egress SG rules for ECS and Lambda to the VPC endpoint SG.
Lockfile removals
infrastructure/modules/networking/.terraform.lock.hcl, infrastructure/modules/security/.terraform.lock.hcl
Deleted module-level .terraform.lock.hcl files (removed recorded provider versions and hashes).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to NACL rule priorities, stateless behavior, and port ranges.
  • Validate VPC endpoint interface settings (subnet associations, private_dns_enabled) and the endpoint SG ingress rule.
  • Verify security module egress rules reference the new VPC endpoint SG correctly and that the module wiring in staging matches expected inputs/outputs.

Possibly related PRs

  • #2431 — Adds similar VPC endpoints/security-group wiring and exposes a VPC endpoint SG for consumption by security rules.
  • #2685 — Modifies security module SG configuration and outputs; overlaps on SG wiring and egress rules to VPC endpoints.
  • #2717 — Updates security/network wiring for ECS-to-VPC-endpoint connectivity and related SG rules.

Suggested labels

backend, docs

Suggested reviewers

  • arkid15r
  • kasya

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Improve networking Module' is vague and generic, using broad terms that don't clearly convey the specific technical changes made in the PR. Consider a more specific title that highlights the main changes, such as 'Add NACLs and VPC endpoints to networking module' or 'Add network security and VPC endpoint support'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly relates to the changeset, specifically mentioning the three main changes: adding NACLs, adding VPC endpoints, and fixing traffic routing.
Linked Issues check ✅ Passed The PR successfully implements all coding requirements from issue #2761: adds Network ACLs module, adds VPC endpoints module, and refactors networking module to support both features.
Out of Scope Changes check ✅ Passed The PR includes provider version updates and lockfile deletions that are not explicitly mentioned in the issue requirements but are necessary infrastructure maintenance changes supporting the feature additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 987f464 and 2839742.

📒 Files selected for processing (13)
  • infrastructure/modules/networking/.terraform.lock.hcl (0 hunks)
  • infrastructure/modules/networking/main.tf (2 hunks)
  • infrastructure/modules/networking/modules/nacl/main.tf (1 hunks)
  • infrastructure/modules/networking/modules/nacl/variables.tf (1 hunks)
  • infrastructure/modules/networking/modules/vpc-endpoint/main.tf (1 hunks)
  • infrastructure/modules/networking/modules/vpc-endpoint/outputs.tf (1 hunks)
  • infrastructure/modules/networking/modules/vpc-endpoint/variables.tf (1 hunks)
  • infrastructure/modules/networking/outputs.tf (1 hunks)
  • infrastructure/modules/networking/variables.tf (1 hunks)
  • infrastructure/modules/security/.terraform.lock.hcl (0 hunks)
  • infrastructure/modules/security/main.tf (2 hunks)
  • infrastructure/modules/security/variables.tf (1 hunks)
  • infrastructure/staging/main.tf (2 hunks)
💤 Files with no reviewable changes (2)
  • infrastructure/modules/security/.terraform.lock.hcl
  • infrastructure/modules/networking/.terraform.lock.hcl
🚧 Files skipped from review as they are similar to previous changes (8)
  • infrastructure/modules/security/variables.tf
  • infrastructure/modules/networking/main.tf
  • infrastructure/modules/networking/outputs.tf
  • infrastructure/modules/networking/modules/nacl/main.tf
  • infrastructure/modules/security/main.tf
  • infrastructure/modules/networking/modules/vpc-endpoint/outputs.tf
  • infrastructure/modules/networking/modules/nacl/variables.tf
  • infrastructure/modules/networking/variables.tf
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-08T11:16:25.725Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.725Z
Learning: The parameters module in infrastructure/modules/parameters/ is currently configured for staging environment only. The `configuration` and `settings_module` variables default to "Staging" and "settings.staging" respectively, and users can update parameter values via the AWS Parameter Store console. The lifecycle.ignore_changes blocks on these parameters support manual console updates without Terraform reverting them.

Applied to files:

  • infrastructure/modules/networking/modules/vpc-endpoint/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). (4)
  • GitHub Check: Run frontend unit tests
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run backend tests
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
infrastructure/staging/main.tf (2)

71-71: aws_region propagation to networking module is correct.

This input feeds the vpc_endpoint submodule's need to construct region-specific AWS service names (e.g., com.amazonaws.${var.aws_region}.logs). Verify that the networking module declares aws_region as a variable and passes it to the vpc_endpoint submodule.


99-107: Verify vpc_endpoint_sg_id wiring with security module.

Line 105 passes module.networking.vpc_endpoint_security_group_id to the security module. Ensure this output is correctly exposed by the networking module and that the security module's variables define vpc_endpoint_sg_id as a valid input for attaching additional security group rules.

infrastructure/modules/networking/modules/vpc-endpoint/variables.tf (1)

1-44: Variable declarations are well-structured and complete.

All nine variables are properly typed, described, and align with the vpc-endpoint submodule's resource dependencies. No concerns identified.

infrastructure/modules/networking/modules/vpc-endpoint/main.tf (4)

12-29: VPC endpoint security group configuration is appropriate.

The security group correctly allows HTTPS (443) ingress from the entire VPC CIDR. This enables private communication between resources in the VPC and the VPC endpoints. The naming and tagging are consistent with project conventions.


31-98: VPC endpoint configurations follow AWS best practices.

  • Interface endpoints (cloudwatch_logs, ecr_api, ecr_dkr, secretsmanager, ssm): Correctly configured with private_dns_enabled=true, attached to the security group, and placed in private subnets.
  • S3 Gateway endpoint: Correctly omits security group attachment (Gateway endpoints do not support security groups) and uses vpc_endpoint_type = "Gateway".
  • Service names: Correctly constructed with region interpolation (e.g., com.amazonaws.${var.aws_region}.logs).

All endpoints are properly tagged with consistent naming conventions.


100-108: S3 endpoint route table associations are correctly configured.

The S3 Gateway endpoint is associated with both private (line 100–103) and public (line 105–108) route tables, allowing resources in all subnets to use the endpoint. This approach using aws_vpc_endpoint_route_table_association resources provides flexibility compared to direct route_table_ids property and is a valid pattern.


1-10: No action required. The exact version pinning for Terraform (1.14.0) and AWS provider (6.22.0) is intentional and aligns with OWASP/Nest's infrastructure policy, which prioritizes reproducibility and explicit version control in the testing environment. Both versions are stable and current as of December 2025. Exact provider version pinning is a recommended security practice when paired with staging environment testing, which your workflow includes.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rudransh-shrivastava rudransh-shrivastava marked this pull request as ready for review December 1, 2025 17:08
@rudransh-shrivastava rudransh-shrivastava changed the title Improve network Module Improve networking Module Dec 1, 2025
@rudransh-shrivastava rudransh-shrivastava force-pushed the feature/nest-zappa-migration-network-improvement branch from 987f464 to 2839742 Compare December 3, 2025 14:36
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 3, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve networking Module

1 participant