Skip to content

Conversation

@leek
Copy link
Owner

@leek leek commented Oct 20, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Added www subdomain routing with automatic redirect to primary domain
    • Integrated VPC endpoints for private AWS service connectivity (SSM, ECR, CloudWatch, SQS, S3)
    • Added ECS deployment circuit breaker for automatic rollback on failures
  • Improvements

    • Enhanced bastion server setup with comprehensive logging and retry mechanisms
    • Optimized load balancer health checks and connection timeouts
    • Updated infrastructure naming to support flexible multi-application deployments

@leek leek requested a review from Copilot October 20, 2025 21:11
@leek leek self-assigned this Oct 20, 2025
@leek leek added the enhancement New feature or request label Oct 20, 2025
@gemini-code-assist
Copy link

Summary of Changes

Hello @leek, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on a series of miscellaneous updates and improvements to the Terraform infrastructure. The changes aim to enhance the reliability of bastion host provisioning, optimize application deployment strategies, improve domain routing, and bolster network security through the implementation of VPC endpoints. These updates collectively contribute to a more robust and efficient cloud environment.

Highlights

  • Bastion Host Bootstrap Improvements: The bastion host user data script has been significantly refactored to include robust logging, timestamping, and a retry mechanism for all installation commands, enhancing reliability and debuggability.
  • ECS Service Deployment Optimization: Deployment configurations for the ECS service have been added to minimize ENI requirements during rolling updates, improving efficiency and stability.
  • WWW to Non-WWW Redirect: An Application Load Balancer (ALB) listener rule has been implemented to automatically redirect traffic from the 'www' subdomain to the bare domain, ensuring consistent access.
  • VPC Endpoints Configuration: Multiple VPC Interface Endpoints for AWS services like SSM, ECR, CloudWatch Logs, and SQS, along with a Gateway Endpoint for S3, have been configured to enhance security and reduce data transfer costs by keeping traffic within the AWS network.
  • Terraform State Management Updates: The GitHub Actions IAM policy now explicitly references hardcoded names for the Terraform state S3 bucket and DynamoDB lock table, providing clearer permissions.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces random updates and improvements across the Terraform infrastructure, focusing on VPC endpoints, load balancing enhancements, and bastion host reliability improvements.

Key Changes:

  • Added VPC endpoints for AWS services (SSM, ECR, CloudWatch Logs, SQS, S3) to enable private connectivity
  • Implemented www to non-www domain redirect functionality in the load balancer
  • Enhanced bastion host bootstrap script with retry logic, proper error handling, and idempotency

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
terraform/modules/security/main.tf Hardcoded application name to "laravel" in IAM policy resource ARNs
terraform/modules/networking/main.tf Added VPC interface and gateway endpoints with security group configuration
terraform/modules/load_balancer/variables.tf Added domain_name variable for redirect functionality
terraform/modules/load_balancer/main.tf Reduced health check intervals, added www redirect listener rule
terraform/modules/dns/main.tf Added www subdomain DNS record for redirect support
terraform/modules/compute/main.tf Added ECS deployment configuration with circuit breaker
terraform/modules/bastion/user_data.sh Rewrote bootstrap script with retry logic, logging, and idempotency
terraform/main.tf Passed domain_name variable to load_balancer module
terraform/.gitignore Added plan.out to ignore list
Files not reviewed (1)
  • terraform/.terraform.lock.hcl: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a variety of improvements across the Terraform infrastructure. Key changes include making the bastion host setup script more robust and idempotent, adding VPC endpoints for better security and performance, and configuring a www-to-non-www redirect at the load balancer. However, there are several critical issues in the bastion user data script due to incorrect variable handling that will cause the setup to fail. Additionally, the Terraform provider lock file has been removed, which is not recommended as it can lead to inconsistent builds. There are also some minor issues with hardcoded values and missing newlines at the end of files.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Updates Terraform infrastructure configuration to standardize application naming, add domain-based www redirection, enhance container deployment settings, establish VPC endpoints for private service access, and refactor bastion bootstrap process with improved logging and retry mechanisms.

Changes

Cohort / File(s) Summary
Gitignore and Dependencies
terraform/.gitignore, terraform/.terraform.lock.hcl
Added plan.out to gitignore; removed AWS and random provider lock entries.
Application Naming Standardization
terraform/variables.tf, terraform/versions.tf, terraform/modules/security/main.tf
Updated default value for app_name variable; parameterized S3 backend bucket and key using var.app_name; refactored IAM user and policy naming to use app_name instead of hardcoded "laravel".
Domain and Load Balancer Configuration
terraform/main.tf, terraform/modules/load_balancer/variables.tf, terraform/modules/load_balancer/main.tf
Added domain_name input to load_balancer module; introduced domain_name variable and www-redirect logic with conditional ALB listener rule; adjusted health check interval from 30 to 15 seconds and deregistration delay from 30 to 15 seconds; added tags to HTTPS listener.
DNS Configuration
terraform/modules/dns/main.tf
Added Route53 A record resource for www subdomain with alias to ALB.
Container and Network Infrastructure
terraform/modules/compute/main.tf, terraform/modules/networking/main.tf
Added ECS service deployment configuration (minimum healthy percent, maximum percent, circuit breaker with rollback); introduced VPC endpoints for SSM, EC2 messages, ECR, CloudWatch Logs, SQS, and S3 Gateway with corresponding security group.
Bastion Bootstrap Process
terraform/modules/bastion/user_data.sh
Replaced inline installation with controlled bootstrap workflow; added strict shell options, centralized logging, retry utilities, and support for optional MySQL and Redis clients; refactored AWS CLI v2, Terraform, Docker, and Docker Compose installation steps with retries and verification; updated RDS polling and MySQL user setup flow.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant ALB as ALB
    participant EC2 as EC2 Service
    
    rect rgb(200, 220, 255)
    Note over Client,ALB: WWW Redirect (New)
    Client->>ALB: GET http://www.domain.com
    ALB->>Client: 301 HTTPS Redirect to domain.com
    end
    
    rect rgb(220, 235, 220)
    Note over Client,ALB: Standard Request (Existing)
    Client->>ALB: GET https://domain.com
    ALB->>EC2: Route to target
    EC2-->>ALB: Response
    ALB-->>Client: 200 OK
    end
Loading
sequenceDiagram
    participant Instance as Bastion Instance
    participant Bootstrap as Bootstrap Script
    participant System as System Services
    participant RDS as RDS
    participant Secrets as Secrets Manager
    participant Logs as CloudWatch Logs
    
    rect rgb(200, 220, 255)
    Note over Instance,Bootstrap: Bootstrap Initialization (New)
    Instance->>Bootstrap: Execute user_data in background (nohup)
    Bootstrap->>Bootstrap: Set strict shell options (set -euo pipefail)
    Bootstrap->>System: Install base utilities with retry
    end
    
    rect rgb(220, 235, 220)
    Note over Bootstrap,System: Tool Installation (New)
    Bootstrap->>System: Install AWS CLI v2 with retry + logging
    Bootstrap->>System: Install Terraform with retry + logging
    Bootstrap->>System: Install Docker & Docker Compose with retry + logging
    Bootstrap->>Logs: Log progress with timestamps
    end
    
    rect rgb(235, 220, 220)
    Note over Bootstrap,RDS: Database Setup (New)
    Bootstrap->>Secrets: Fetch master password
    Bootstrap->>RDS: Poll RDS readiness with retry cap
    Bootstrap->>RDS: Create users and grant privileges
    Bootstrap->>Logs: Verify success and log outcome
    Bootstrap->>System: Clear secrets from memory
    end
    
    rect rgb(200, 220, 255)
    Note over Bootstrap,Logs: Completion (New)
    Bootstrap->>System: Create bootstrap completion flag
    Bootstrap->>Logs: Log bootstrap completion
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

The diff spans multiple infrastructure layers with heterogeneous changes: extensive bastion bootstrap rewrite with dense shell scripting logic, new VPC endpoint infrastructure requiring security validation, application naming refactoring across multiple files affecting IAM and S3 backend, domain-based redirect logic in load balancer, and ECS deployment parameter updates. Changes demand separate reasoning for each functional area and careful review of the bootstrap script logic, naming consistency, and infrastructure interconnections.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The PR title "feat(misc): random updates and improvements" uses vague, non-descriptive terminology that fails to convey meaningful information about the changeset. While the changes are genuinely related to updates (domain name support, VPC endpoints, bootstrap scripting improvements, parameterized naming, and deployment configurations), the title does not communicate what is actually being changed or why. The use of "misc" and "random updates" exemplifies the type of non-specific language that obscures the pull request's purpose, making it difficult for teammates reviewing history to understand the primary intent without reading the full changeset. Consider revising the title to be more specific about the primary changes or key themes. For example, the title could focus on a main feature ("feat: add domain name and www redirect support"), reference the major infrastructure addition ("feat: add VPC endpoints for service communication"), or identify the bootstrap improvements ("feat: refactor bastion bootstrap script with retry logic"). A more specific title would help reviewers quickly understand the changeset's scope and purpose.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/misc-updates

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

♻️ Duplicate comments (3)
terraform/modules/bastion/user_data.sh (3)

33-33: Incorrect variable expansion in retry log message - duplicate of past review.

The double dollar sign before {delay} is incorrect. In bash, $$ expands to the process ID, not a literal dollar sign.

The fix remains the same as the previous review:

-    log "Command failed (attempt $n/$attempts): $*; retrying in $${delay}s"
+    log "Command failed (attempt $n/$attempts): $*; retrying in ${delay}s"

83-83: Incorrect variable expansion in Docker Compose version reference.

Line 83 uses $${DOCKER_COMPOSE_VERSION}, which will expand to the process ID plus literal text instead of the version variable (e.g., v12345{DOCKER_COMPOSE_VERSION} instead of v2.34.1), resulting in a failed download.

Apply this diff to fix the variable reference:

-retry 5 30 curl -sSL "https://github.com/docker/compose/releases/download/v$${DOCKER_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose
+retry 5 30 curl -sSL "https://github.com/docker/compose/releases/download/v${DOCKER_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose

69-72: Incorrect variable expansion in Terraform version references.

Lines 69, 70, and 72 use $${TERRAFORM_VERSION}, which expands incorrectly in bash. The $$ becomes the current process ID, producing malformed URLs like terraform/12345{TERRAFORM_VERSION}/... instead of terraform/1.10.5/....

Apply this diff to fix all three occurrences:

-retry 5 30 bash -c "cd /tmp && wget -q \"https://releases.hashicorp.com/terraform/$${TERRAFORM_VERSION}/terraform_$${TERRAFORM_VERSION}_linux_amd64.zip\""
-retry 5 30 bash -c "cd /tmp && unzip -o \"terraform_$${TERRAFORM_VERSION}_linux_amd64.zip\""
-rm -f /tmp/terraform_$${TERRAFORM_VERSION}_linux_amd64.zip
+retry 5 30 bash -c "cd /tmp && wget -q \"https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_linux_amd64.zip\""
+retry 5 30 bash -c "cd /tmp && unzip -o \"terraform_${TERRAFORM_VERSION}_linux_amd64.zip\""
+rm -f /tmp/terraform_${TERRAFORM_VERSION}_linux_amd64.zip
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a78abd and 11a4272.

📒 Files selected for processing (12)
  • terraform/.gitignore (1 hunks)
  • terraform/.terraform.lock.hcl (0 hunks)
  • terraform/main.tf (1 hunks)
  • terraform/modules/bastion/user_data.sh (2 hunks)
  • terraform/modules/compute/main.tf (1 hunks)
  • terraform/modules/dns/main.tf (1 hunks)
  • terraform/modules/load_balancer/main.tf (9 hunks)
  • terraform/modules/load_balancer/variables.tf (2 hunks)
  • terraform/modules/networking/main.tf (2 hunks)
  • terraform/modules/security/main.tf (2 hunks)
  • terraform/variables.tf (1 hunks)
  • terraform/versions.tf (1 hunks)
💤 Files with no reviewable changes (1)
  • terraform/.terraform.lock.hcl
🧰 Additional context used
🪛 Checkov (3.2.334)
terraform/modules/networking/main.tf

[medium] 140-157: Ensure Terraform module sources use a commit hash

(CKV_TF_1)

🪛 Shellcheck (0.11.0)
terraform/modules/bastion/user_data.sh

[warning] 5-5: LOG_FILE appears unused. Verify use (or export if used externally).

(SC2034)

🔇 Additional comments (14)
terraform/modules/networking/main.tf (3)

13-13: Cosmetic formatting tweaks approved.

The adjustments to the VPC module inputs (AZs slicing and formatting of gateway flags) are non-breaking and improve clarity.

Also applies to: 17-21


139-157: Security group configuration is appropriate; version constraint follows existing patterns.

The ingress rule correctly limits HTTPS traffic to the VPC CIDR, which is the proper security posture for interface endpoints. Note that static analysis flags the module version constraint (version = "~> 5.0"); however, this is consistent with all other module definitions in the file. If your organization prefers pinning to specific commit hashes per CKV_TF_1, consider standardizing this across all module blocks.


264-274: Verify intentionality of S3 endpoint on both private and public route tables.

The S3 gateway endpoint is configured to route through both private and public route tables via concat(). While this configuration can reduce NAT gateway costs by allowing direct S3 access from both layers, confirm this is the intended behavior for your architecture. If only private resources need S3 access, consider limiting to module.vpc.private_route_table_ids alone to follow the principle of least privilege.

terraform/.gitignore (1)

72-72: LGTM!

Adding plan.out to the gitignore list is appropriate and consistent with existing Terraform plan patterns.

terraform/modules/compute/main.tf (1)

213-220: LGTM!

The deployment configuration improves resilience with circuit breaker-backed rollback on failure and optimizes ENI usage with a 50% minimum healthy threshold. This is a solid operational enhancement for ECS service deployments.

terraform/main.tf (1)

184-184: LGTM!

Passing domain_name to the load_balancer module enables the new domain-based redirect logic and aligns the root configuration with the module's domain handling.

terraform/modules/dns/main.tf (1)

31-42: LGTM!

The new www A record mirrors the existing patterns and properly routes www.${domain_name} to the ALB for handling the domain-based redirect logic.

terraform/modules/load_balancer/variables.tf (1)

16-19: LGTM!

Adding domain_name as a required variable enables domain-aware load balancer routing and aligns with the PR's domain-based redirect feature.

terraform/modules/load_balancer/main.tf (6)

38-39: Cosmetic formatting applied to visibility_config blocks.

The alignment adjustments to metric names across multiple visibility_config blocks are purely cosmetic with no semantic impact. If these changes are governed by an automated formatter or linting rules, confirm that tooling is configured in the CI/CD pipeline.

Also applies to: 61-62, 84-85, 107-108, 130-131, 137-138


150-152: Clean local variable for domain detection.

The local centralizes the domain detection logic and properly normalizes case. Good practice.


185-185: Verify the rationale for reducing health check interval from 30s to 15s.

Halving the health check interval doubles the frequency of health check requests to targets and increases CloudWatch metrics API calls. Confirm this change aligns with operational requirements and has been performance-tested.


194-194: Verify the rationale for reducing deregistration_delay from 30s to 15s.

Halving the connection draining timeout may cause in-flight requests to be terminated before completion during target deregistration. Confirm this aligns with deployment and graceful shutdown strategies, especially for long-running requests.


234-234: No issues found.

The aws_lb_listener resource supports a tags argument (and exposes tags_all), so the code change at line 234 is correct.


237-263: No issues found—code is correct as-is.

Verification confirms all concerns are addressed: var.domain_name is defined in variables.tf, the conditional logic correctly handles both www and non-www domains, there are no priority conflicts (this is the only listener rule on the https listener), and the ALB redirect syntax is valid. The rule already includes an explanatory comment.

set -euo pipefail

BOOTSTRAP_SCRIPT="/usr/local/bin/bastion-bootstrap.sh"
LOG_FILE="/var/log/bastion-bootstrap.log"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove unused LOG_FILE variable from outer script.

The outer script declares LOG_FILE at line 5 but never uses it—the same variable is re-declared and used inside the bootstrap script. This is dead code and should be removed.

Apply this diff:

 BOOTSTRAP_SCRIPT="/usr/local/bin/bastion-bootstrap.sh"
-LOG_FILE="/var/log/bastion-bootstrap.log"
 
 cat > "$BOOTSTRAP_SCRIPT" <<'SCRIPT'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
LOG_FILE="/var/log/bastion-bootstrap.log"
BOOTSTRAP_SCRIPT="/usr/local/bin/bastion-bootstrap.sh"
cat > "$BOOTSTRAP_SCRIPT" <<'SCRIPT'
🧰 Tools
🪛 Shellcheck (0.11.0)

[warning] 5-5: LOG_FILE appears unused. Verify use (or export if used externally).

(SC2034)

🤖 Prompt for AI Agents
In terraform/modules/bastion/user_data.sh around line 5, the script declares
LOG_FILE="/var/log/bastion-bootstrap.log" but that outer declaration is never
used because the bootstrap script re-declares LOG_FILE; remove this unused OUTER
LOG_FILE assignment (delete the line) so only the bootstrap-local LOG_FILE
remains, and run a quick grep to ensure no other references depend on the
removed variable.

Comment on lines +163 to +258
locals {
interface_endpoint_tags = merge(var.common_tags, {
TerraformComponent = "vpc-endpoints"
})
}

resource "aws_vpc_endpoint" "ssm" {
service_name = "com.amazonaws.${var.aws_region}.ssm"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-ssm-endpoint"
})
}

resource "aws_vpc_endpoint" "ssmmessages" {
service_name = "com.amazonaws.${var.aws_region}.ssmmessages"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-ssmmessages-endpoint"
})
}

resource "aws_vpc_endpoint" "ec2messages" {
service_name = "com.amazonaws.${var.aws_region}.ec2messages"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-ec2messages-endpoint"
})
}

resource "aws_vpc_endpoint" "ecr_api" {
service_name = "com.amazonaws.${var.aws_region}.ecr.api"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-ecr-api-endpoint"
})
}

resource "aws_vpc_endpoint" "ecr_dkr" {
service_name = "com.amazonaws.${var.aws_region}.ecr.dkr"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-ecr-dkr-endpoint"
})
}

resource "aws_vpc_endpoint" "logs" {
service_name = "com.amazonaws.${var.aws_region}.logs"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-logs-endpoint"
})
}

resource "aws_vpc_endpoint" "sqs" {
service_name = "com.amazonaws.${var.aws_region}.sqs"
vpc_id = module.vpc.vpc_id
vpc_endpoint_type = "Interface"
subnet_ids = module.vpc.private_subnets
security_group_ids = [module.vpc_endpoints_security_group.security_group_id]
private_dns_enabled = true

tags = merge(local.interface_endpoint_tags, {
Name = "${var.app_name}-${var.environment}-sqs-endpoint"
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Refactor repetitive VPC endpoint definitions using for_each to improve maintainability.

The seven interface endpoint resources are nearly identical, differing only in their service name and endpoint name. This duplication increases maintenance burden and the risk of configuration inconsistencies. Consolidate these into a single resource block using for_each to make future updates simpler and less error-prone.

Apply this refactor to eliminate duplication:

locals {
  interface_endpoint_tags = merge(var.common_tags, {
    TerraformComponent = "vpc-endpoints"
  })

  # Map of VPC interface endpoints to create
  interface_endpoints = {
    ssm = {
      service_name = "com.amazonaws.${var.aws_region}.ssm"
    }
    ssmmessages = {
      service_name = "com.amazonaws.${var.aws_region}.ssmmessages"
    }
    ec2messages = {
      service_name = "com.amazonaws.${var.aws_region}.ec2messages"
    }
    ecr_api = {
      service_name = "com.amazonaws.${var.aws_region}.ecr.api"
    }
    ecr_dkr = {
      service_name = "com.amazonaws.${var.aws_region}.ecr.dkr"
    }
    logs = {
      service_name = "com.amazonaws.${var.aws_region}.logs"
    }
    sqs = {
      service_name = "com.amazonaws.${var.aws_region}.sqs"
    }
  }
}

resource "aws_vpc_endpoint" "interface_endpoints" {
  for_each = local.interface_endpoints

  service_name        = each.value.service_name
  vpc_id              = module.vpc.vpc_id
  vpc_endpoint_type   = "Interface"
  subnet_ids          = module.vpc.private_subnets
  security_group_ids  = [module.vpc_endpoints_security_group.security_group_id]
  private_dns_enabled = true

  tags = merge(local.interface_endpoint_tags, {
    Name = "${var.app_name}-${var.environment}-${each.key}-endpoint"
  })

  depends_on = [module.vpc_endpoints_security_group]
}

This approach makes it simple to add or remove endpoints in the future by modifying the local.interface_endpoints map.

🤖 Prompt for AI Agents
In terraform/modules/networking/main.tf around lines 163 to 258, multiple
nearly-identical aws_vpc_endpoint "Interface" resources (ssm, ssmmessages,
ec2messages, ecr_api, ecr_dkr, logs, sqs) are duplicated; refactor by creating a
local map (e.g., local.interface_endpoints) that maps endpoint keys to their
service_name, replace the seven resources with a single aws_vpc_endpoint
resource using for_each = local.interface_endpoints, reference
each.value.service_name for service_name and use
"${var.app_name}-${var.environment}-${each.key}-endpoint" for Name tag, keep
common attributes (vpc_id, vpc_endpoint_type, subnet_ids, security_group_ids,
private_dns_enabled, tags) and add depends_on =
[module.vpc_endpoints_security_group].

Comment on lines 432 to 434
resource "aws_iam_user_policy" "laravel_app_user_policy" {
name = "${var.app_name}-${var.environment}-laravel-user-policy"
name = "${var.app_name}-${var.environment}-${var.app_name}-user-policy"
user = aws_iam_user.laravel_app_user.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Redundant app_name in IAM policy naming.

Line 433 duplicates var.app_name in the policy name: ${var.app_name}-${var.environment}-${var.app_name}-user-policy.

Remove the duplicate.

Apply this diff:

 resource "aws_iam_user_policy" "laravel_app_user_policy" {
-  name = "${var.app_name}-${var.environment}-${var.app_name}-user-policy"
+  name = "${var.app_name}-${var.environment}-user-policy"
   user = aws_iam_user.laravel_app_user.name
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resource "aws_iam_user_policy" "laravel_app_user_policy" {
name = "${var.app_name}-${var.environment}-laravel-user-policy"
name = "${var.app_name}-${var.environment}-${var.app_name}-user-policy"
user = aws_iam_user.laravel_app_user.name
resource "aws_iam_user_policy" "laravel_app_user_policy" {
name = "${var.app_name}-${var.environment}-user-policy"
user = aws_iam_user.laravel_app_user.name
🧰 Tools
🪛 Checkov (3.2.334)

[high] 432-513: Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions

(CKV_AWS_355)


[high] 432-513: Ensure IAM policies does not allow write access without constraints

(CKV_AWS_290)

🤖 Prompt for AI Agents
In terraform/modules/security/main.tf around lines 432 to 434 the
aws_iam_user_policy resource name duplicates var.app_name resulting in
"${var.app_name}-${var.environment}-${var.app_name}-user-policy"; remove the
duplicate so the name becomes a single app_name (e.g.
"${var.app_name}-${var.environment}-user-policy") and update the resource
accordingly, then run terraform fmt to keep formatting consistent.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants