improvement CICD Perma drift resolution, feature enhancements to control over ECS Service deployment (#77)

* update to avoid permadrift

* Update src/variables.tf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* code suggestion improvements

* cleanup 1

* Update to improve functionality and permadrift

* <output	Update changelog to include this PR.</output>

* Update src/variables.tf

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* cleanup from pr comments

* update ecs logic to fix tests

* terraform fmt -recursive

* try again

* Coder rabbit suggestions

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Author: 3 people

README.md

@@ -1,3 +1,25 @@
+## PR [#77](https://github.com/cloudposse-terraform-components/aws-ecs-service/pull/77)
+
+### Added
+
+- Added `additional_lb_target_groups` variable to support registering multiple container ports to load balancer target groups
+  - Enables sidecar containers (e.g., stunnel, envoy) to be registered to separate target groups
+  - Each entry requires a unique `target_group_arn` to prevent all additional ports from being wired to the same target group
+  - Includes validation to ensure valid target group ARN format
+- Added `additional_security_groups` variable to attach additional security groups to the ECS service
+- Added `security_group_id` field to `custom_security_group_rules` to allow rules to target different security groups
+
+### Changed
+
+- Updated `task_template` output to use S3 task definition when available via merge strategy
+  - When `task-definition.json` exists in S3 (created by CI/CD), it takes precedence
+  - Falls back to Terraform-created task definition when S3 version doesn't exist
+  - Eliminates Terraform drift when CI/CD manages task definitions
+- Fixed deprecation warnings:
+  - Replaced `inline_policy` in `aws_iam_role.github_actions` with separate `aws_iam_role_policy` resource
+  - Replaced deprecated `aws_s3_bucket_object` with `aws_s3_object`
+- Updated SSM parameter key format to include attributes in the path for better organization
+
 ## PR [#1008](https://github.com/cloudposse/terraform-aws-components/pull/1008)
 
 ### Possible Breaking Change

src/CHANGELOG.md

@@ -103,7 +103,18 @@ data "aws_iam_policy_document" "github_actions_iam_ecspresso_policy" {
       "ecs:RunTask",
     ]
     resources = [
-      format("arn:%s:ecs:%s:%s:task-definition/%s:*", local.aws_partition, var.region, local.account_id, join("", module.ecs_alb_service_task[*].task_definition_family)),
+      format(
+        "arn:%s:ecs:%s:%s:task-definition/%s:*",
+        local.aws_partition,
+        var.region,
+        local.account_id,
+        try(one(module.ecs_alb_service_task[*].task_definition_family), null) != null
+        ? try(one(module.ecs_alb_service_task[*].task_definition_family), null)
+        : (try(one(data.aws_ecs_task_definition.created_task[*].family), null) != null
+          ? one(data.aws_ecs_task_definition.created_task[*].family)
+          : module.this.id
+        )
+      ),
     ]
   }
 

src/README.md

@@ -54,11 +54,13 @@ resource "aws_iam_role" "github_actions" {
   count              = local.github_actions_iam_role_enabled ? 1 : 0
   name               = module.gha_role_name.id
   assume_role_policy = module.gha_assume_role.github_assume_role_policy
+}
 
-  inline_policy {
-    name   = module.gha_role_name.id
-    policy = local.github_actions_iam_policy
-  }
+resource "aws_iam_role_policy" "github_actions" {
+  count  = local.github_actions_iam_role_enabled ? 1 : 0
+  name   = module.gha_role_name.id
+  role   = aws_iam_role.github_actions[0].id
+  policy = local.github_actions_iam_policy
 }
 
 output "github_actions_iam_role_arn" {

src/github-actions-iam-policy.tf

@@ -10,9 +10,11 @@ locals {
 
   assign_public_ip = try(local.task["assign_public_ip"], false)
 
-  container_definition = concat([
-    for container in module.container_definition :
-    container.json_map_object
+  container_definition = concat(
+    [
+
+      for container in module.container_definition :
+      container.json_map_object
     ],
     [
       for container in module.datadog_container_definition :
@@ -129,16 +131,16 @@ locals {
     local.container_aliases[item.name] => { container_definition = item }
   }
 
-  containers_priority_terraform = {
+  containers_priority_terraform = local.enabled ? {
     for name, settings in var.containers :
     name => merge(local.container_chamber[name], lookup(local.container_s3, name, {}), settings, )
     if local.enabled
-  }
-  containers_priority_s3 = {
+  } : {}
+  containers_priority_s3 = local.enabled ? {
     for name, settings in var.containers :
     name => merge(settings, local.container_chamber[name], lookup(local.container_s3, name, {}))
     if local.enabled
-  }
+  } : {}
 }
 
 data "aws_ssm_parameters_by_path" "default" {
@@ -302,27 +304,39 @@ module "ecs_alb_service_task" {
 
   container_definition_json = jsonencode(local.container_definition)
 
+  # # When following latest, point service at the latest ACTIVE task definition ARN
+  # task_definition = local.follow_latest_task_definition ? compact([local.latest_task_definition]) : []
   # This is set to true to allow ingress from the ALB sg
   use_alb_security_group = local.use_alb_security_group
   container_port         = local.container_port
   alb_security_group     = local.lb_sg_id
-  security_group_ids     = compact(concat([local.vpc_sg_id, local.rds_sg_id], local.external_security_group))
+  security_group_ids     = compact(concat([local.vpc_sg_id, local.rds_sg_id], local.external_security_group, var.additional_security_groups))
   enable_all_egress_rule = var.enable_all_egress_rule
 
   nlb_cidr_blocks     = local.is_nlb ? [module.vpc.outputs.vpc_cidr] : []
   nlb_container_port  = local.is_nlb ? local.container_port : 80
   use_nlb_cidr_blocks = local.is_nlb
 
   # See https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ecs_service#load_balancer
-  ecs_load_balancers = local.use_lb ? [
-    {
-      container_name   = try(local.service_container["name"], null),
-      container_port   = local.container_port,
-      target_group_arn = local.is_alb ? module.alb_ingress[0].target_group_arn : local.nlb_compat.default_target_group_arn
-      # not required since elb is unused but must be set to null
-      elb_name = null
-    },
-  ] : []
+  ecs_load_balancers = local.use_lb ? concat(
+    [
+      {
+        container_name   = try(local.service_container["name"], null),
+        container_port   = local.container_port,
+        target_group_arn = local.is_alb ? module.alb_ingress[0].target_group_arn : local.nlb_compat.default_target_group_arn
+        # not required since elb is unused but must be set to null
+        elb_name = null
+      },
+    ],
+    [
+      for lb_config in var.additional_lb_target_groups : {
+        container_name   = lb_config.container_name
+        container_port   = lb_config.container_port
+        target_group_arn = lb_config.target_group_arn
+        elb_name         = null
+      }
+    ]
+  ) : []
 
   assign_public_ip                   = local.assign_public_ip
   ignore_changes_task_definition     = try(local.task["ignore_changes_task_definition"], false)
@@ -380,7 +394,7 @@ resource "aws_security_group_rule" "custom_sg_rules" {
   cidr_blocks              = try(each.value.cidr_blocks, null)
   source_security_group_id = try(each.value.source_security_group_id, null)
   prefix_list_ids          = try(each.value.prefix_list_ids, null)
-  security_group_id        = one(module.ecs_alb_service_task[*].service_security_group_id)
+  security_group_id        = try(each.value.security_group_id, null) != null ? each.value.security_group_id : one(module.ecs_alb_service_task[*].service_security_group_id)
 }
 
 module "alb_ingress" {
@@ -618,8 +632,24 @@ data "aws_ecs_task_definition" "created_task" {
 
 locals {
   created_task_definition = local.s3_mirroring_enabled ? data.aws_ecs_task_definition.created_task[0] : null
+
+  # Remove the 'image' field only from the service container to prevent drift when CI/CD updates images
+  # CI/CD will provide the image for the service container in the complete task definition
+  # Sidecar containers (datadog, fluent-bit, etc.) retain their images
+  service_container_name = try(local.service_container["name"], null)
+
+  # Process each container: strip image from service container, keep it for sidecars
+  container_definition_without_image = [
+    for container in local.container_definition : merge(
+      container,
+      container.name == local.service_container_name ? { image = "" } : {}
+    )
+  ]
+
+
+  # Build the task template from the Terraform-created task definition
   task_template = local.s3_mirroring_enabled ? {
-    containerDefinitions = local.container_definition
+    containerDefinitions = local.container_definition_without_image
     family               = lookup(local.created_task_definition, "family", null),
     taskRoleArn          = lookup(local.created_task_definition, "task_role_arn", null),
     executionRoleArn     = lookup(local.created_task_definition, "execution_role_arn", null),
@@ -628,11 +658,11 @@ locals {
     requiresCompatibilities = [lookup(local.task, "launch_type", "FARGATE")]
     cpu                     = tostring(lookup(local.task, "task_cpu", null))
     memory                  = tostring(lookup(local.task, "task_memory", null))
-
   } : null
+
 }
 
-resource "aws_s3_bucket_object" "task_definition_template" {
+resource "aws_s3_object" "task_definition_template" {
   count                  = local.s3_mirroring_enabled ? 1 : 0
   bucket                 = lookup(module.s3[0].outputs, "bucket_id", null)
   key                    = format("%s/%s/task-template.json", module.ecs_cluster.outputs.cluster_name, module.this.id)

src/github-actions-iam-role.mixin.tf

@@ -26,7 +26,7 @@ variable "ssm_key_prefix" {
 locals {
   ssm_enabled = module.this.enabled && var.ssm_enabled
 
-  url_params = { for i, url in local.full_urls : format(var.ssm_key_format, var.ssm_key_prefix, var.name, "url/${i}") => {
+  url_params = { for i, url in local.full_urls : format(var.ssm_key_format, var.ssm_key_prefix, join("-", concat([var.name], var.attributes)), "url/${i}") => {
     description = "ECS Service URL for ${var.name}"
     type        = "String",
     value       = url
@@ -56,6 +56,11 @@ resource "aws_ssm_parameter" "full_urls" {
   overwrite   = true
 
   tags = module.this.tags
+
+  # ignore changes to key_id, currently a workaround for terraform pinned version that allows permadrift.
+  lifecycle {
+    ignore_changes = [key_id]
+  }
 }
 
 

src/main.tf

@@ -135,6 +135,9 @@ variable "task" {
     circuit_breaker_deployment_enabled = optional(bool, true)
     circuit_breaker_rollback_enabled   = optional(bool, true)
 
+    ignore_changes_task_definition = optional(bool, true)
+    ignore_changes_desired_count   = optional(bool, false)
+
     ecs_service_enabled = optional(bool, true)
     bind_mount_volumes = optional(list(object({
       name      = string
@@ -718,6 +721,7 @@ variable "custom_security_group_rules" {
     description              = optional(string)
     source_security_group_id = optional(string)
     prefix_list_ids          = optional(list(string))
+    security_group_id        = optional(string)
   }))
   description = "The list of custom security group rules to add to the service security group"
   default     = []
@@ -756,3 +760,34 @@ variable "vpc_component_name" {
   description = "The name of a VPC component"
   default     = "vpc"
 }
+
+variable "additional_security_groups" {
+  type        = list(string)
+  description = "A list of additional security group IDs to add to the service"
+  default     = []
+}
+
+variable "additional_lb_target_groups" {
+  type = list(object({
+    container_name   = string
+    container_port   = number
+    target_group_arn = string
+  }))
+  description = <<-EOT
+    Additional load balancer target group configurations for registering multiple container ports.
+    This allows you to register sidecar containers to separate target groups.
+    Each entry requires:
+    - container_name: Name of the container to register
+    - container_port: Port on the container to register
+    - target_group_arn: ARN of the target group. Each additional port must specify a unique target group ARN
+  EOT
+  default     = []
+
+  validation {
+    condition = alltrue([
+      for config in var.additional_lb_target_groups :
+      can(regex("^arn:aws[a-z-]*:elasticloadbalancing:[a-z0-9-]+:[0-9]{12}:targetgroup/.+$", config.target_group_arn))
+    ])
+    error_message = "Each additional_lb_target_groups entry must specify a valid target_group_arn in the format: arn:aws:elasticloadbalancing:region:account-id:targetgroup/name/id"
+  }
+}