Feature/fargate

[teaguru]

No description provided.

[shane-nzer]

AI gen'd, human reviewed:

Review Summary

This PR adds Fargate support to the ECS cluster module. There's one critical issue that needs to be fixed before merging, plus some suggestions for improvement.

---

Critical Issue

Invalid capacity provider strategy for fargate_spot_with_fallback (main.tf:183-217)

The current implementation creates two separate default_capacity_provider_strategy blocks:

dynamic "default_capacity_provider_strategy" {
  for_each = var.cluster_strategy_type == "fargate_spot_with_fallback" ? [1] : []
  content {
    base              = 1
    weight            = 0
    capacity_provider = "FARGATE"
  }
}

dynamic "default_capacity_provider_strategy" {
  for_each = var.cluster_strategy_type == "fargate_spot_with_fallback" ? [1] : []
  content {
    base              = 0
    weight            = 1
    capacity_provider = "FARGATE_SPOT"
  }
}

Problem: AWS ECS expects a single strategy block with multiple provider entries, not multiple strategy blocks. This will likely fail during terraform apply.

Suggested fix:

dynamic "default_capacity_provider_strategy" {
  for_each = var.cluster_strategy_type == "fargate_spot_with_fallback" ? [
    { base = 1, weight = 0, provider = "FARGATE" },
    { base = 0, weight = 1, provider = "FARGATE_SPOT" }
  ] : []
  content {
    base              = default_capacity_provider_strategy.value.base
    weight            = default_capacity_provider_strategy.value.weight
    capacity_provider = default_capacity_provider_strategy.value.provider
  }
}

---

Medium Priority

1. Inconsistent "default" strategy logic (main.tf:177)

The logic for the "default" strategy type still uses the old EC2-or-Fargate conditional:

capacity_provider = var.instances_autoscale_max > 0 && var.instances_desired == 0 ? aws_ecs_capacity_provider.autoscale[0].name : "FARGATE"

This doesn't align well with the new explicit cluster_strategy_type variable. Consider simplifying "default" to consistently use FARGATE, or document why this dynamic behavior exists.

2. Missing KMS encryption for CloudWatch Logs (main.tf:151)

The log group doesn't specify KMS encryption:

resource "aws_cloudwatch_log_group" "ecs_exec_logs" {
  count             = var.cluster_logging ? 1 : 0
  name              = "/ecs/${var.cluster_name}/exec"
  retention_in_days = var.cluster_log_retention_days
}

Consider adding a kms_key_id parameter for consistency with the existing disk_kms_key_id variable (especially for compliance requirements).

---

Minor Suggestions

1. DRY improvement - The log group path /ecs/${var.cluster_name}/exec appears in both main.tf:161 and the migration guide. Consider using a locals block.

2. Variable validation - Could add validation to prevent confusing combinations (e.g., cluster_strategy_type = "fargate" when EC2 instances are configured).

[teaguru]

Thanks, Shane — noted.
immplemented that
dynamic "default_capacity_provider_strategy" {
for_each = var.cluster_strategy_type == "fargate_spot_with_fallback" ? [
{ base = 1, weight = 0, provider = "FARGATE" },
{ base = 0, weight = 1, provider = "FARGATE_SPOT" }
] : []
content {
base = default_capacity_provider_strategy.value.base
weight = default_capacity_provider_strategy.value.weight
capacity_provider = default_capacity_provider_strategy.value.provider
}
}