Feature/iac scaffolding : Foundational Network Module #2310
Conversation
…ional structure and documentation for the new Terraform-based IaC
In this commit I introduced the initial, production-grade Terraform module for the core AWS network infrastructure. Key features of this module include: - Creates a new VPC with public and private subnets across multiple Availability Zones for high availability. - Implements a single, cost-effective, and resilient NAT Gateway for private subnet egress. - Provisions a public Application Load Balancer (ALB) with a secure default configuration and an HTTPS redirect. - Establishes a secure-by-default posture with a locked-down ALB security group.
Summary by CodeRabbit
WalkthroughAdds Terraform pre-commit hooks, a new Network Terraform module (VPC, subnets, NAT, routing, ALB, flow logs, S3 log buckets), module variables/outputs and versions, an operational guide, repo ignores and cspell entries, and Trivy misconfiguration ignores. No public API/signature changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 8
🧹 Nitpick comments (6)
.pre-commit-config.yaml (1)
90-100: Add terraform_docs and prefer types-based matching over regex.
- Add the terraform_docs hook to keep variables/outputs documented and in sync.
- Prefer types_or: [terraform] over files regex to catch all Terraform sources consistently (including nested paths).
Apply this diff:
- repo: https://github.com/antonbabenko/pre-commit-terraform - rev: v1.92.0 + rev: v1.92.0 hooks: + - id: terraform_docs + args: + - --hook-config=--create-file-if-not-exist=true - id: terraform_fmt - files: \.tf$ + types_or: + - terraform - id: terraform_validate - files: \.tf$ + types_or: + - terraform - id: terraform_tflint - files: \.tf$ + types_or: + - terraform - id: terraform_trivy - files: \.tf$ + types_or: + - terraformTerraform/Operational-Guide.md (1)
18-21: Fix module path to match repository structure.Doc lists modules/network but the added code resides under modules/01-Network. Align the path or rename for consistency.
Terraform/modules/01-Network/variables.tf (3)
19-23: Validate vpc_cidr format.Add a simple CIDR validation to catch typos early.
variable "vpc_cidr" { description = "The CIDR block for the VPC." type = string default = "10.0.0.0/16" + validation { + condition = can(cidrhost(var.vpc_cidr, 0)) + error_message = "vpc_cidr must be a valid IPv4 CIDR block." + } }
25-28: Ensure at least one AZ provided.Add length check to avoid empty lists.
variable "availability_zones" { description = "A list of Availability Zones to deploy resources into. Must match the number of subnets. e.g., [\"us-east-1a\", \"us-east-1b\"]" type = list(string) + validation { + condition = length(var.availability_zones) > 0 + error_message = "At least one availability zone must be provided." + } }
48-52: Optionally add a toggle for per‑AZ NAT gateways.Expose a boolean to support HA NAT as an opt-in later.
variable "enable_multi_az_nat_gateways" { description = "If true, create a NAT Gateway in each public subnet (higher availability, higher cost)." type = bool default = false }Terraform/modules/01-Network/main.tf (1)
65-73: Allocate the EIP in VPC scope explicitly.Set domain = "vpc" to ensure correct allocation context for the NAT.
resource "aws_eip" "nat" { - # Only one EIP is needed for the single NAT Gateway. + domain = "vpc" # Ensure EIP is allocated in the VPC scope + # Only one EIP is needed for the single NAT Gateway. tags = merge(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.pre-commit-config.yaml(1 hunks)Terraform/Operational-Guide.md(1 hunks)Terraform/modules/01-Network/main.tf(1 hunks)Terraform/modules/01-Network/outputs.tf(1 hunks)Terraform/modules/01-Network/variables.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 179-195: Ensure that ALB drops HTTP headers
(CKV_AWS_131)
[high] 212-227: Ensure that load balancer is using at least TLS 1.2
(CKV_AWS_103)
🔇 Additional comments (1)
Terraform/modules/01-Network/outputs.tf (1)
1-29: Outputs look good.Clear and minimal set to enable composition by downstream modules.
Terraform/Operational-Guide.md
Outdated
| cd terraform/environments/dev | ||
| ``` | ||
There was a problem hiding this comment.
🧩 Analysis chain
Verify environment directory path/casing.
Doc uses terraform/environments/dev, while repo paths appear under Terraform/. Ensure the path exists and matches case-sensitive filesystems.
🏁 Script executed:
#!/bin/bash
# Discover environment and module paths (case-insensitive) in repo
fd -i -t d '^environments$' -d 4
fd -i -t d '01-Network' -d 6Length of output: 53
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Listing Terraform/environments =="
ls -la Terraform/environments || true
echo
echo "== Searching for 'dev' directories under Terraform =="
fd -t d -i dev Terraform -d 4 || true
echo
echo "== Showing Operational-Guide.md around lines 100-120 =="
sed -n '100,120p' Terraform/Operational-Guide.md || trueLength of output: 991
Fix path casing and missing dev environment in docs
Terraform/Operational-Guide.md (lines 111–113) shows cd terraform/environments/dev, but the repo has Terraform/environments/ (only .gitkeep — no dev directory). Update the doc to use the correct path/casing (e.g., cd Terraform/environments/<env>) and either document creating the environment directory or add Terraform/environments/dev/ to the repo.
🤖 Prompt for AI Agents
In Terraform/Operational-Guide.md around lines 111 to 113, the documented path
uses the wrong casing and references a non-existent dev directory; update the
path to use the actual repo casing and a placeholder (e.g., cd
Terraform/environments/<env>) and either add a short step that instructs users
to create the environment directory (mkdir -p Terraform/environments/dev &&
touch Terraform/environments/dev/.gitkeep) or add Terraform/environments/dev/ to
the repository so the example works as written; ensure the text matches the
repository layout and casing consistently.
|
I'm reviewing all the suggestions now and will push up a new commit with the fixes shortly. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Terraform/Operational-Guide.md (1)
134-139: Add a concrete remote backend example to avoid local state.Without a backend block, init will default to local state. Provide a sample backend to match the S3/DynamoDB created above.
Add this as backend.tf in the environment directory:
terraform { backend "s3" { bucket = env.TF_STATE_BUCKET key = "owasp-nest/${env.ENVIRONMENT}/terraform.tfstate" region = env.AWS_REGION dynamodb_table = env.TF_STATE_LOCK_TABLE encrypt = true } }Alternatively, document using a backend config file (e.g., -backend-config=backend.hcl).
Terraform/modules/01-Network/variables.tf (1)
60-70: Expose a few config toggles to avoid hard-coding policy and behavior.Make TLS policy, ALB deletion protection, NAT topology, and ALB log retention configurable.
variable "alb_access_logs_bucket_name" { description = "The name of the S3 bucket to store ALB access logs. Must be globally unique. If left empty, a name will be generated." type = string default = "" } + +variable "alb_ssl_policy" { + description = "SSL policy for the ALB HTTPS listener." + type = string + default = "ELBSecurityPolicy-TLS13-1-2-2021-06" +} + +variable "alb_deletion_protection_enabled" { + description = "Explicitly enable ALB deletion protection. If not set, production is enabled by default in main.tf." + type = bool + default = false +} + +variable "nat_per_az" { + description = "If true, create one NAT Gateway per AZ for higher availability (higher cost). If false, use a single NAT GW." + type = bool + default = false +} + +variable "alb_logs_retention_days" { + description = "Days to retain ALB access logs in S3." + type = number + default = 90 +}Follow-up: wire these variables in main.tf where applicable.
Terraform/modules/01-Network/main.tf (2)
231-233: Make ALB deletion protection controllable by variable with sane default.Allows overriding in non-prod and forces on in prod by default.
- enable_deletion_protection = var.environment == "prod" ? true : false + enable_deletion_protection = var.alb_deletion_protection_enabled || (var.environment == "prod")
263-269: LGTM: TLS 1.2/1.3 policy enforced.Updated ssl_policy to a modern policy. Consider sourcing from var.alb_ssl_policy for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Terraform/Operational-Guide.md(1 hunks)Terraform/modules/01-Network/main.tf(1 hunks)Terraform/modules/01-Network/variables.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 141-154: Ensure that an S3 bucket has a lifecycle configuration
(CKV2_AWS_61)
🔇 Additional comments (6)
Terraform/Operational-Guide.md (1)
49-70: Good fix: us-east-1 handling + hardened state bucket.Conditional create-bucket for us-east-1, versioning, public access block, and SSE are all correct and align with best practices.
Terraform/modules/01-Network/variables.tf (2)
34-36: Good validation on public subnet CIDRs vs AZs.Prevents index mismatches at apply time.
43-45: Good validation on private subnet CIDRs vs AZs.Avoids out-of-range indexing and enforces module contract.
Terraform/modules/01-Network/main.tf (3)
75-89: Good: clear note on single-NAT SPOF trade-off.Accurately documents availability/cost implications.
223-246: Ensure provider compatibility for “drop_invalid_header_fields”.Some provider versions require setting this via load_balancer_attributes. If your version errors on the top-level arg, switch to the attribute block below.
resource "aws_lb" "main" { @@ - drop_invalid_header_fields = true + load_balancer_attributes { + key = "routing.http.drop_invalid_header_fields.enabled" + value = "true" + }Please confirm your aws provider version supports the chosen form.
234-238: Avoid null bucket when logging is disabled.Use a dynamic block so bucket is only referenced when enabled (prevents accidental eval of a zero-count reference).
- access_logs { - bucket = var.enable_alb_access_logs ? aws_s3_bucket.alb_access_logs[0].bucket : null - enabled = var.enable_alb_access_logs - prefix = "alb" - } + dynamic "access_logs" { + for_each = var.enable_alb_access_logs ? [1] : [] + content { + bucket = aws_s3_bucket.alb_access_logs[0].bucket + enabled = true + prefix = "alb" + } + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Terraform/modules/01-Network/main.tf (4)
62-64: Fix misleading resilience claim for single NAT/single RT.Single NAT + single private route table is cost‑optimized, not more resilient. It increases AZ blast radius and adds cross‑AZ egress charges when other AZs traverse the NAT’s AZ.
Apply this diff to correct the comment:
-# We create a SINGLE NAT Gateway and a SINGLE private route table. This is more -# resilient, cost-effective, and simpler to manage than a per-AZ NAT Gateway. +# We create a SINGLE NAT Gateway and a SINGLE private route table. This is more +# cost-effective and simpler to manage than per-AZ NAT Gateways, but less resilient +# (AZ egress SPOF) and may incur cross-AZ data transfer for other AZs.
76-89: Optional: add per‑AZ NAT support to avoid egress SPOF and cross‑AZ charges.Expose a toggle to create one NAT per AZ and per‑AZ private route tables; keep current single‑NAT as default.
Example (outside this file; minimal sketch):
variable "enable_multi_az_nat" { type = bool, default = false } resource "aws_nat_gateway" "az" { count = var.enable_multi_az_nat ? length(aws_subnet.public) : 0 allocation_id = aws_eip.nat_multi[count.index].id subnet_id = aws_subnet.public[count.index].id depends_on = [aws_internet_gateway.main] tags = merge(var.tags, { Name = "${var.project_prefix}-${var.environment}-nat-gw-${count.index}" }) } resource "aws_route_table" "private_per_az" { count = var.enable_multi_az_nat ? length(aws_subnet.private) : 0 vpc_id = aws_vpc.main.id route { cidr_block = "0.0.0.0/0" nat_gateway_id = aws_nat_gateway.az[count.index].id } tags = merge(var.tags, { Name = "${var.project_prefix}-${var.environment}-private-rt-${count.index}" }) }
31-44: Validate CIDR/AZ list lengths to prevent index errors.If list lengths diverge, Terraform panics at index access.
Add validations in variables.tf (outside this file):
variable "availability_zones" { type = list(string) validation { condition = length(var.availability_zones) == length(var.public_subnet_cidrs) error_message = "public_subnet_cidrs must match availability_zones length." } validation { condition = length(var.availability_zones) == length(var.private_subnet_cidrs) error_message = "private_subnet_cidrs must match availability_zones length." } }Also applies to: 46-58
108-113: Use for_each to avoid index coupling on associations.for_each is safer and clearer than count-index ties.
-resource "aws_route_table_association" "public" { - count = length(aws_subnet.public) - subnet_id = aws_subnet.public[count.index].id - route_table_id = aws_route_table.public.id -} +resource "aws_route_table_association" "public" { + for_each = { for s in aws_subnet.public : s.id => s } + subnet_id = each.value.id + route_table_id = aws_route_table.public.id +} @@ -resource "aws_route_table_association" "private" { - count = length(aws_subnet.private) - subnet_id = aws_subnet.private[count.index].id - route_table_id = aws_route_table.private.id -} +resource "aws_route_table_association" "private" { + for_each = { for s in aws_subnet.private : s.id => s } + subnet_id = each.value.id + route_table_id = aws_route_table.private.id +}Also applies to: 132-137
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Terraform/modules/01-Network/main.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 148-161: Ensure that an S3 bucket has a lifecycle configuration
(CKV2_AWS_61)
[medium] 174-185: Ensure that an S3 bucket has a lifecycle configuration
(CKV2_AWS_61)
🔇 Additional comments (6)
Terraform/modules/01-Network/main.tf (6)
148-161: Harden ALB logs bucket: SSE, ownership controls, lifecycle.Add SSE-S3, BucketOwnerPreferred, and lifecycle to meet checks and best practices.
resource "aws_s3_bucket" "alb_access_logs" { # Only create this bucket if logging is enabled count = var.enable_alb_access_logs ? 1 : 0 @@ ) } + +resource "aws_s3_bucket_server_side_encryption_configuration" "alb_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.alb_access_logs[0].id + rule { + apply_server_side_encryption_by_default { + sse_algorithm = "AES256" + } + } +} + +resource "aws_s3_bucket_ownership_controls" "alb_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.alb_access_logs[0].id + rule { object_ownership = "BucketOwnerPreferred" } +} + +resource "aws_s3_bucket_lifecycle_configuration" "alb_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.alb_access_logs[0].id + rule { + id = "expire-alb-logs" + status = "Enabled" + expiration { days = var.alb_logs_retention_days } + } +}Note: Checkov flags CKV2_AWS_61 for lifecycle. [Based on static analysis hints]
173-185: Also harden the S3 server access logs bucket.Apply public access block, SSE, ownership, lifecycle to the target logs bucket too.
resource "aws_s3_bucket" "s3_server_access_logs" { count = var.enable_alb_access_logs ? 1 : 0 @@ ) } + +resource "aws_s3_bucket_public_access_block" "s3_server_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.s3_server_access_logs[0].id + block_public_acls = true + block_public_policy = true + ignore_public_acls = true + restrict_public_buckets = true +} + +resource "aws_s3_bucket_server_side_encryption_configuration" "s3_server_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.s3_server_access_logs[0].id + rule { + apply_server_side_encryption_by_default { sse_algorithm = "AES256" } + } +} + +resource "aws_s3_bucket_ownership_controls" "s3_server_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.s3_server_access_logs[0].id + rule { object_ownership = "BucketOwnerPreferred" } +} + +resource "aws_s3_bucket_lifecycle_configuration" "s3_server_access_logs" { + count = var.enable_alb_access_logs ? 1 : 0 + bucket = aws_s3_bucket.s3_server_access_logs[0].id + rule { + id = "expire-s3-logs" + status = "Enabled" + expiration { days = var.alb_logs_retention_days } + } +}
280-303: ALB config looks good (HTTPS, deletion protection gating, header hardening).LGTM.
320-326: TLS policy upgraded to TLS1.2/1.3.Good security posture.
65-73: Set EIP domain to “vpc” for NAT Gateway.Without domain="vpc", allocation can misbehave in edge cases and fail NAT creation.
resource "aws_eip" "nat" { + domain = "vpc" # Only one EIP is needed for the single NAT Gateway. tags = merge(
196-230: Fix ALB logs bucket policy (invalid Effect, wrong resource path, missing GetBucketAcl).Effect must be “Allow”. Resource must point to AWSLogs//*, and ALB needs s3:GetBucketAcl. Add ACL condition.
data "aws_iam_policy_document" "alb_access_logs" { count = var.enable_alb_access_logs ? 1 : 0 # This statement allows the ALB service to write logs to the bucket. statement { - effect = "AllowALBToWriteLogs" + sid = "AWSLogDeliveryWrite" + effect = "Allow" actions = ["s3:PutObject"] - resources = ["${aws_s3_bucket.alb_access_logs[0].arn}/*"] + resources = ["${aws_s3_bucket.alb_access_logs[0].arn}/AWSLogs/${data.aws_caller_identity.current.account_id}/*"] principals { type = "AWS" - identifiers = [data.aws_elb_service_account.current.arn] + identifiers = [data.aws_elb_service_account.current.arn] } + condition { + test = "StringEquals" + variable = "s3:x-amz-acl" + values = ["bucket-owner-full-control"] + } } - # This statement denies any access to the bucket over insecure HTTP. + # Allow the ALB service to read bucket ACL for delivery checks. + statement { + sid = "AWSLogDeliveryCheck" + effect = "Allow" + actions = ["s3:GetBucketAcl"] + resources = [aws_s3_bucket.alb_access_logs[0].arn] + principals { + type = "AWS" + identifiers = [data.aws_elb_service_account.current.arn] + } + } + + # This statement denies any access to the bucket over insecure HTTP. statement { sid = "DenyInsecureTransport" effect = "Deny" actions = ["s3:*"] resources = ["${aws_s3_bucket.alb_access_logs[0].arn}/*"] principals { type = "*" identifiers = ["*"] } condition { test = "Bool" variable = "aws:SecureTransport" values = ["false"] } } }
This commit resolves all SonarQube security hotspots for the S3 buckets by: - Adding Public Access Block configurations to both log buckets. - Enforcing HTTPS-only access via bucket policies on both buckets. - Enabling server-access logging for both buckets. - Adding versioning, encryption, and lifecycle policies for a complete production-ready configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Terraform/modules/01-Network/main.tf (2)
65-72: Explicitly scope the NAT EIP to the VPC domain.Setting
domain = "vpc"avoids legacy Classic allocations and is the documented requirement for NAT gateways. Please add the explicit domain for clarity and robustness.resource "aws_eip" "nat" { + domain = "vpc" tags = merge( var.tags, { Name = "${var.project_prefix}-${var.environment}-nat-eip" } ) }
244-287: Add abort rules for incomplete multipart uploads.Lifecycle policies should clean up failed multipart uploads to satisfy CKV_AWS_300 and prevent unbounded storage costs.
noncurrent_version_expiration { noncurrent_days = 30 } + + abort_incomplete_multipart_upload { + days_after_initiation = 7 + } } } @@ noncurrent_version_expiration { noncurrent_days = 30 } + + abort_incomplete_multipart_upload { + days_after_initiation = 7 + } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Terraform/modules/01-Network/main.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 240-266: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
[medium] 268-289: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
nishkersh
force-pushed
the
feature/iac-scaffolding
branch
from
19eedd1 to
a0706b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Terraform/modules/01-Network/main.tf (2)
65-72: Add explicit VPC domain to EIP resource.The EIP resource lacks the
domain = "vpc"attribute. Without this, AWS may allocate an EIP in the EC2-Classic domain, which will not work correctly with the NAT Gateway. This is a required configuration for VPC-based NAT.Apply this diff to add the domain attribute:
resource "aws_eip" "nat" { + domain = "vpc" + tags = merge(
62-63: Correct misleading NAT Gateway documentation.The comment incorrectly claims the single NAT Gateway is "more resilient." In reality, placing a single NAT in one AZ creates an egress single-point-of-failure for all private subnets if that AZ or the NAT fails. This is a cost optimization, not a resilience improvement. Update the comment to reflect this trade-off so operators can make informed choices.
Apply this diff to correct the comment:
-# We create a SINGLE NAT Gateway and a SINGLE private route table. This is more -# resilient, cost-effective, and simpler to manage than a per-AZ NAT Gateway. +# We create a SINGLE NAT Gateway and a SINGLE private route table. This is a cost +# optimization but introduces a single-AZ egress SPOF compared to per-AZ NAT gateways. +# Scale to one NAT per AZ if higher availability is required.
🧹 Nitpick comments (1)
Terraform/modules/01-Network/main.tf (1)
240-266: Add abort incomplete multipart uploads rule to S3 lifecycle policies.Static analysis flags that S3 lifecycle configurations should include rules to abort incomplete multipart uploads (CKV_AWS_300). This prevents accumulated orphaned uploads from consuming storage and incurring unexpected costs.
Add an
abort_incomplete_multipart_uploadrule to both lifecycle configurations:resource "aws_s3_bucket_lifecycle_configuration" "alb_access_logs" { count = var.enable_alb_access_logs ? 1 : 0 bucket = aws_s3_bucket.alb_access_logs[0].id rule { id = "expire-old-logs" status = "Enabled" + abort_incomplete_multipart_upload { + days_after_initiation = 7 + } + transition { days = 30 storage_class = "STANDARD_IA" } ... } } resource "aws_s3_bucket_lifecycle_configuration" "s3_server_access_logs" { count = var.enable_alb_access_logs ? 1 : 0 bucket = aws_s3_bucket.s3_server_access_logs[0].id rule { id = "expire-old-logs" status = "Enabled" + abort_incomplete_multipart_upload { + days_after_initiation = 7 + } + transition { days = 30 storage_class = "STANDARD_IA" } ... } }Also applies to: 268-289
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Terraform/modules/01-Network/main.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 240-266: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
[medium] 268-289: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
🔇 Additional comments (2)
Terraform/modules/01-Network/main.tf (2)
478-478: Excellent security hardening improvements.The ALB resource now includes
drop_invalid_header_fields = true(satisfying CKV_AWS_131) and the HTTPS listener usesssl_policy = "ELBSecurityPolicy-TLS13-1-2-2021-06"(enforcing TLS 1.2+, satisfying CKV_AWS_103). These are solid security posture improvements over the defaults. Well done addressing the previous security findings.Also applies to: 524-524
313-408: ALB and S3 bucket policies correctly use AWS service accounts.The IAM policy documents now properly reference
data.aws_elb_service_account.current.arnanddata.aws_caller_identity.current.account_id, eliminating the hard-coded placeholders that would break logging. The policies also correctly include both PutObject and GetBucketAcl permissions, and enforce HTTPS-only access via the DenyInsecureTransport statements. This is a solid foundation for ALB logging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Terraform/modules/01-Network/main.tf (1)
66-73: Add missingdomain = "vpc"to EIP resource.The EIP is missing the explicit
domainattribute. AWS may allocate a classical EIP instead of a VPC-scoped EIP, causing the NAT Gateway attachment to fail. This was flagged in a previous review but is still not present in the code.Apply this diff:
resource "aws_eip" "nat" { + domain = "vpc" tags = merge(
🧹 Nitpick comments (1)
Terraform/modules/01-Network/main.tf (1)
241-267: Addabort_incomplete_multipart_uploadto S3 lifecycle rules.Both lifecycle configurations lack rules to abort incomplete multipart uploads, which can accumulate abandoned parts and incur unexpected storage charges. Checkov flag CKV_AWS_300 recommends setting this for cost management and compliance.
Apply this diff to the
aws_s3_bucket_lifecycle_configurationfor ALB logs (lines 241–267):resource "aws_s3_bucket_lifecycle_configuration" "alb_access_logs" { count = var.enable_alb_access_logs ? 1 : 0 bucket = aws_s3_bucket.alb_access_logs[0].id rule { id = "expire-old-logs" status = "Enabled" + abort_incomplete_multipart_upload { + days_after_initiation = 7 + } + transition { days = 30 storage_class = "STANDARD_IA" } ...Apply the same pattern to
aws_s3_bucket_lifecycle_configurationfor S3 server logs (lines 269–290), adjustingdays_after_initiationas desired (e.g., 7 days is a common default).Also applies to: 269-290
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)Terraform/modules/01-Network/main.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 241-267: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
[medium] 269-290: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
🔇 Additional comments (1)
Terraform/modules/01-Network/main.tf (1)
1-536: Module structure and security posture are well-designed.The overall architecture is clean and production-grade:
- VPC, subnets, and routing are correctly configured across multiple AZs with cost-optimized single NAT Gateway and explicit SPOF documentation.
- Security group properly enforces HTTPS with HTTP→HTTPS redirect.
- ALB is hardened with TLS 1.2+ enforcement, drop invalid headers, and deletion protection for production.
- S3 logging buckets have strong security controls: public access blocks, encryption, versioning, bucket policies with secure transport enforcement, and proper service principals.
- Tagging and dependencies are consistently applied throughout.
Once the EIP
domainissue is fixed and lifecycle rules are updated, this module will be ready.
This commit resolves all failing CI checks from the initial PR. - Updates the cspell dictionary to recognize common IaC acronyms. - Configures .pre-commit-config.yaml to correctly run Terraform, TFLint, and Trivy hooks. - Updates trivyignore.yaml to manage acceptable risks for the PoC. - Applies all automatic formatting fixes from the linting tools. - Edited .gitignore for Terraform-specific files (.terraform/, *.tfstate).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Terraform/modules/01-Network/main.tf (1)
73-80: Add explicitdomain = "vpc"to EIP resource.The
aws_eipresource lacks the explicitdomain = "vpc"attribute. While not always problematic in modern AWS, omitting this can cause default EC2-Classic allocation behavior in older regions or configurations. Explicitly settingdomain = "vpc"aligns with best practices and ensures the EIP is allocated in the correct scope for NAT Gateway use.Apply this diff:
resource "aws_eip" "nat" { + domain = "vpc" + tags = merge(
🧹 Nitpick comments (3)
trivyignore.yaml (1)
10-14: Document a plan to address Trivy ignores before production.The four misconfiguration ignores are justified for the PoC phase, but there is no documented plan to re-evaluate or remediate them before production deployment. Consider adding a tracking issue or ADR to outline the hardening roadmap (e.g., enable per-AZ NATs for AVD-AWS-0164, migrate to KMS for AVD-AWS-0132, etc.).
Would you like me to help draft a hardening checklist or migration plan for these PoC trade-offs?
Terraform/modules/01-Network/outputs.tf (1)
1-29: Module outputs are well-designed for downstream consumption.The six outputs expose the key resources (VPC, subnets, ALB, security groups, listeners) needed by downstream compute and database modules, with clear descriptions and no sensitive data leakage.
Optional enhancement: Consider adding
nat_eipoutput (EIP public IP) for scenarios where downstream modules or operators need to whitelist outbound IP ranges.Terraform/modules/01-Network/main.tf (1)
143-184: VPC Flow Logs are appropriately configured for observability.CloudWatch integration with a reasonable 30-day retention and proper IAM role setup satisfies compliance/observability requirements. The note about scoping down the IAM policy for production is acknowledged.
Consider making retention_in_days configurable via a variable for consistency with other PoC parameters, but the hardcoded 30-day default is reasonable for MVP.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks).pre-commit-config.yaml(1 hunks)Terraform/modules/01-Network/main.tf(1 hunks)Terraform/modules/01-Network/outputs.tf(1 hunks)Terraform/modules/01-Network/variables.tf(1 hunks)Terraform/modules/01-Network/versions.tf(1 hunks)cspell/custom-dict.txt(5 hunks)trivyignore.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Terraform/modules/01-Network/variables.tf
- .pre-commit-config.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
Terraform/modules/01-Network/main.tf
[medium] 291-317: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
[medium] 319-340: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
🔇 Additional comments (11)
cspell/custom-dict.txt (1)
10-10: Dictionary entries correctly ordered and aligned with PR scope.All six new entries (ELB, SPOF, elb, igw, tfstate, tfvars) are correctly positioned in alphabetical order and directly support the Terraform network infrastructure module being introduced in this PR. The entries prevent false positives for AWS networking components (ELB, igw), infrastructure concepts (SPOF), and Terraform-specific configuration file patterns (tfstate, tfvars).
Also applies to: 27-27, 60-60, 72-72, 124-124, 125-125
.gitignore (1)
20-25: Terraform ignore patterns are well-formed and comprehensive.The added patterns properly exclude local Terraform working directories, state files, and variable files while preserving the
.tfvars.exampletemplate. This aligns with Terraform best practices and prevents accidental secret/state leakage.Terraform/modules/01-Network/versions.tf (1)
1-10: Version constraints are appropriate and reproducible.Terraform >= 1.3.0 and AWS provider ~> 6.14.1 provide stability while allowing patch updates. This balances reproducibility with access to bug fixes and minor improvements.
Terraform/modules/01-Network/main.tf (8)
1-32: VPC and Internet Gateway configuration is correct.Precondition validation, DNS settings, and tagging are all appropriate. The defensive precondition catches array mismatches early.
34-65: Multi-AZ subnet configuration is well-structured.The count pattern with variable-driven AZ assignment, proper
map_public_ip_on_launchsettings, and AZ-aware tagging enable flexible, multi-AZ deployments.
96-141: Routing configuration is sound.Public and private route tables with proper associations and NAT Gateway egress are correctly implemented. The single-NAT architecture is now clearly documented as a cost optimization with AZ-level SPOF trade-off.
187-243: S3 bucket access blocks and data sources are correctly implemented.Public access blocks comprehensively restrict unauthorized access, and data sources correctly reference regional ELB service accounts and caller identity. The nested bucket structure (ALB logs → S3 logs) with count conditionals is properly configured.
245-288: S3 versioning and encryption are correctly configured.Versioning is enabled for log auditability, and SSE-AES256 encryption is appropriately applied. The critical issue (bucket_key_enabled with SSE-S3) has been properly resolved.
342-481: S3 bucket logging and IAM policies are security-hardened and correctly structured.Nested bucket logging, strict HTTPS-only access enforcement, and proper principal references (regional ELB service account and S3 logging service) are all correctly implemented. Dependencies ensure proper resource creation order.
483-555: ALB and security group configuration properly harden ingress/egress controls.HTTP header validation (
drop_invalid_header_fields = true), restricted egress to VPC CIDR only, and dynamic access logging with proper dependencies are all correctly implemented. The conditional deletion protection for production is appropriate.
557-587: ALB listener configuration enforces modern TLS and proper HTTP handling.HTTP listeners correctly redirect to HTTPS with status 301, and HTTPS listeners enforce TLS 1.2+ (
ELBSecurityPolicy-TLS13-1-2-2021-06). The 404 default response appropriately defers target routing to downstream modules.
| # BUCKETS LIFECYCLE POLICIES | ||
| resource "aws_s3_bucket_lifecycle_configuration" "alb_access_logs" { | ||
| count = var.enable_alb_access_logs ? 1 : 0 | ||
| bucket = aws_s3_bucket.alb_access_logs[0].id | ||
|
|
||
| rule { | ||
| id = "expire-old-logs" | ||
| status = "Enabled" | ||
|
|
||
| transition { | ||
| days = 30 | ||
| storage_class = "STANDARD_IA" | ||
| } | ||
|
|
||
| transition { | ||
| days = 90 | ||
| storage_class = "GLACIER" | ||
| } | ||
|
|
||
| expiration { | ||
| days = 365 | ||
| } | ||
|
|
||
| noncurrent_version_expiration { | ||
| noncurrent_days = 30 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| resource "aws_s3_bucket_lifecycle_configuration" "s3_server_access_logs" { | ||
| count = var.enable_alb_access_logs ? 1 : 0 | ||
| bucket = aws_s3_bucket.s3_server_access_logs[0].id | ||
|
|
||
| rule { | ||
| id = "expire-old-logs" | ||
| status = "Enabled" | ||
|
|
||
| transition { | ||
| days = 30 | ||
| storage_class = "STANDARD_IA" | ||
| } | ||
|
|
||
| expiration { | ||
| days = 180 | ||
| } | ||
|
|
||
| noncurrent_version_expiration { | ||
| noncurrent_days = 30 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add abort incomplete multipart upload rules to S3 lifecycle policies.
Both lifecycle configurations lack rules to clean up incomplete multipart uploads, which can accumulate and consume storage without billing visibility. Checkov (CKV_AWS_300) correctly flags this omission.
Apply this diff to both lifecycle resources:
resource "aws_s3_bucket_lifecycle_configuration" "alb_access_logs" {
count = var.enable_alb_access_logs ? 1 : 0
bucket = aws_s3_bucket.alb_access_logs[0].id
rule {
id = "expire-old-logs"
status = "Enabled"
+
+ abort_incomplete_multipart_upload {
+ days_after_initiation = 7
+ }
transition { resource "aws_s3_bucket_lifecycle_configuration" "s3_server_access_logs" {
count = var.enable_alb_access_logs ? 1 : 0
bucket = aws_s3_bucket.s3_server_access_logs[0].id
rule {
id = "expire-old-logs"
status = "Enabled"
+
+ abort_incomplete_multipart_upload {
+ days_after_initiation = 7
+ }
transition {🧰 Tools
🪛 Checkov (3.2.334)
[medium] 291-317: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
[medium] 319-340: Ensure S3 lifecycle configuration sets period for aborting failed uploads
(CKV_AWS_300)
🤖 Prompt for AI Agents
In Terraform/modules/01-Network/main.tf around lines 290 to 340, both
aws_s3_bucket_lifecycle_configuration resources (alb_access_logs and
s3_server_access_logs) are missing abort_incomplete_multipart_upload rules; add
an abort_incomplete_multipart_upload block inside each rule (e.g.
abort_incomplete_multipart_upload { days_after_initiation = 7 }) to ensure
incomplete multipart uploads are cleaned up and satisfy CKV_AWS_300.
d57f2e4 to
6621967
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cspell/custom-dict.txt(6 hunks)
🔇 Additional comments (1)
cspell/custom-dict.txt (1)
1-1: New dictionary entries are correctly ordered and appropriately aligned with infrastructure changes.All seven new entries—AVD, ELB, SPOF, elb, igw, tfstate, and tfvars—are positioned correctly in alphabetical order and directly support the Terraform network module additions in this PR. These terms represent infrastructure components, Terraform state/configuration files, and operational concepts that will appear in the IaC code and documentation.
Also applies to: 11-11, 28-28, 61-61, 73-73, 125-125, 126-126
| CISSP | ||
| Cañón | ||
| DRF | ||
| ELB |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the duplication of ELB (uppercase) and elb (lowercase).
The dictionary now contains both ELB at line 11 and elb at line 61. While this may be intentional (e.g., ELB for documentation/comments and elb for Terraform resource types), it's worth verifying that both forms are actually used in the codebase to avoid maintaining unnecessary duplicates.
Also applies to: 61-61
🏁 Script executed:
#!/bin/bash
# Description: Verify dual usage of ELB and elb in the codebase to confirm intentional duplication.
# Search for uppercase ELB usage
echo "=== Uppercase ELB usage ==="
rg -C2 '\bELB\b' --type-list | head -20
# Search for lowercase elb usage
echo "=== Lowercase elb usage ==="
rg -C2 '\belb\b' --type-list | head -20Length of output: 835
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Description: Verify dual usage of ELB and elb in the codebase to confirm intentional duplication.
# Search for uppercase ELB usage
echo "=== Uppercase ELB usage ==="
rg -C2 '\bELB\b' || echo "No matches found"
# Search for lowercase elb usage
echo ""
echo "=== Lowercase elb usage ==="
rg -C2 '\belb\b' || echo "No matches found"
# Also check the custom-dict.txt file itself to see the entries
echo ""
echo "=== Entries in custom-dict.txt ==="
grep -n "ELB\|elb" cspell/custom-dict.txtLength of output: 756
Remove the unnecessary lowercase elb entry (line 61).
The codebase uses uppercase ELB in Terraform/modules/01-Network/main.tf (in both a comment and the aws_elb_service_account resource reference), which justifies keeping the ELB entry at line 11. However, the lowercase elb entry at line 61 has no actual usage in the codebase and should be removed to avoid maintaining unnecessary dictionary entries.
🤖 Prompt for AI Agents
In cspell/custom-dict.txt around lines 11 and 61, keep the existing uppercase
"ELB" at line 11 but remove the duplicate lowercase "elb" entry at line 61; edit
the file to delete the lowercase line and save, then run the repo spell/check
pipeline (or cspell) to confirm no regressions.
|
Hi @arkid15r , I've pushed the final set of fixes. All pre-commit checks are now passing locally. |
|
2 participants
Proposed change
This is the first major PR for the IaC PoC as discussed in Resolves #2215. It establishes the foundational, production-grade network infrastructure for the Nest application on AWS.
This PR introduces the first reusable Terraform module:
modules/01-Network.Key Architectural Features:
variables.tfandoutputs.tf) and is ready to be consumed by other modules (likecomputeanddatabase).Next Steps:
Once this foundational network is approved and merged, I will proceed with the
databaseandstoragemodules as planned.Looking forward to your feedback!
Checklist
make check-testlocally; all checks and tests passed.