Change default behavior for hello_world_container_ports

[Nava-JoshLong]

Is your feature request related to a problem? Please describe.
For any deploys not using ports 8080 or 8081 in lb_target_groups, and hello_world_container_ports not defined, the module will error saying something along the lines of the cluster has no routing to the ports defined in lb_target_groups. I don't have the exact error offhand, but the error is somewhat vague, and we ended up changing the task definition to include the port we were using to make the apply happy

This is in no way a pressing need, more of a QoL change I think that would help for future users.

Describe the solution you'd like
Maybe have hello_world_container_ports and lb_target_groups.container_port checked in a local where you concat the target group ports to the hello world ports in case they are missing?

If I have some time, I can try and come up with an idea for how to concat the two params, I know it isn't just a simple concat since the target group param is a list of objects. There'd probably need to be an intermediate list where you pull the service and access ports and append them to that list

Describe alternatives you've considered
We found the hello_world_container_ports param after creating a new version of the definition with the ports defined, that's the only other way we could think of to have it work

Additional context
Add any other context or screenshots about the feature request here.

[jsclarridge]

@Nava-JoshLong Thanks for opening an issue about this! I've run into this issue with the module too. I'm reviewing your suggested changes and the current module configuration to try and get some ideas about ways to improve the module.

hello_world_container_ports is set to [ 8080, 8081 ] by default. The README includes a note about the option to customize it when first creating the module, but it looks like the hello world code is currently hard-coded to work with two ports. Are you looking for a way to configure the hello world app to listen on more ports or a way to ensure that the hello world app at least listens on the lb target group port(s)?

[Nava-JoshLong]

Hi @jsclarridge,

I missed the README line where it said that when I was doing the deploy, and the error message was terraform's vague error message, so it took a little back tracking on AWS to see what was failing. Just thinking of something that I hope would be a small change that could reduce time to troubleshoot

And yeah, I was thinking of seeing if there could be a new feature that adds those ports to hello_world_container_ports.

I'm not sure if something like this would work in main.tf? (Have not tested this, so IDK if it'll work)

locals {
  container_ports = concat(var.lb_target_groups[*].container_port, var.hello_world_container_ports)
}
...
# in the default_container_definitions code, instead of defining each portMappings and environment
# Replaces https://github.com/trussworks/terraform-aws-ecs-service/blob/main/main.tf#L41-L52
dynamic "portMappings" {
  for_each = local.container_ports
  iterator = container_port

  content {
      containerPort = container_port.value
      hostPort         = container_port.value
      protocol         = "tcp"
  }
}

# Replaces https://github.com/trussworks/terraform-aws-ecs-service/blob/main/main.tf#L62-L71
dynamic "environment" {
  for_each = local.container_ports
  iterator = container_port

  content {
      name = "PORT${index(local.container_ports, container_port.value) + 1}"
      value = container_port.value
  }
}

The idea is that if there is no extra ports specified, it won't concat, but otherwise, it will add those ports. Not 100% sure what'll happen if the same port is there twice, like if the user specifies 8080 in thelb_target_groups.container_port call