portMappings.hostPort is required

[mcaulifn]

Describe the Bug

hostPort is optional when using awsvpc network_mode. However, this module requires it no matter what.

Expected Behavior

hostPort should be optional.

Steps to Reproduce

Steps to reproduce the behavior:

1. Create a container definition with the following:

   port_mappings = [
   {
     "protocol"      = "tcp",
     "containerPort" = 3000
   }
   ]

Screenshots

Error: Invalid value for module argument

  on ecs.tf line 36, in module "container_definition":
  36:   port_mappings = [
  37:     {
  38:       "protocol"      = "tcp",
  39:       "containerPort" = 3000
  40:     }
  41:   ]

Environment (please complete the following information):

Anything that will help us triage the bug will help. Here are some ideas:

• OS: Linux
• Version:
  • Module 0.47.0

[nitrocode]

Setting the map key to null should work

  port_mappings = [
    {
      "protocol"      = "tcp",
      "containerPort" = 3000,
      "hostPort"      = null
    }
  ]

[dekimsey]

FWIW if set to null with awsvpc, TF will show it in the diff output since AWS stores it internally as the same port. So in my implementations I always set it to the same value as my containerPort.

I'm torn as to whether that's a bug in TF or AWS.

[mcaulifn]

To avoid the set/un-set churn, the module should require it. Else, it should default to null, noting that there will always be a diff on plan/apply.

[nitrocode]

We could follow this approach

https://github.com/cloudposse/terraform-aws-ecs-container-definition/blob/b982a63c9f558e63c6cc4c24abf69a0e10dae03d/main.tf#L17-L23

 port_mappings = length(var.port_mappings) > 0 ? [ 
   for port_mapping in var.port_mappings : { 
     protocol      = lookup(port_mapping, "protocol") 
     containerPort = lookup(port_mapping, "containerPort") 
     hostPort      = lookup(port_mapping, "hostPort", null)
   } 
 ] : var.port_mappings 

Terraform 0.15 has a more novel approach using the defaults function but it's still in beta

[mcaulifn]

Instead of null, could you do portmapping.containerPort?

[nitrocode]

There are cases where that field needs to be null and not port_mapping.containerPort. I wouldn't want someone to use this and think the default is null and not do what they expect.

The more intuitive approach would be to default it to null if it isn't specified. Unless there is not a case where containerPort has a value and hostPort should be null ?

If you want to use the same port for containerPort as for hostPort then it can be explicitly set.

  port_mappings = [
    {
      "protocol"      = "tcp",
      "containerPort" = 3000,
      "hostPort"      = 3000
    }
  ]

[nitrocode]

According to https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_PortMapping.html

If you are using containers in a task with the awsvpc or host network mode, exposed ports should be specified using containerPort. The hostPort can be left blank or it must be the same value as the containerPort.

If we were passing in the network mode, we could potentially do some conditional magic but I think it should be explicitly set either to null or to the same value as the containerPort.