improvement local.defaulted_check_containers validations

[v-rosa]

The issue

When a task_definition passed to mesh-task contains the property healthCheck = null, the healt check side-car will always report a failiure.

The why

In https://github.com/hashicorp/terraform-aws-consul-ecs/blob/v0.5.2/modules/mesh- task/main.tf#L71-L74 only the keys existence is tested, but specially for healthCheck the value should also be tested, mainly because the task_def passed in var.container_definitions might contain healthCheck = null due to optional flags in task_def creation.

This will pass the task_def to https://github.com/hashicorp/terraform-aws-consul-ecs/blob/8986f2c418cd389638fc9ea44f0a1857f61b5988/modules/mesh-task/config.tf#L35 even it doesn't have a healt_check command

[cthain]

@v-rosa I am able to reproduce this behaviour. I just want to confirm that you're talking about setting healthCheck = null in the var.container_definitions passed to the mesh-task, not the task_definition as mentioned above.

The workaround for this is to omit the healthCheck field entirely from any container definitions that don't include a health check. I agree that this is not ideal and the current behaviour is unexpected and probably led to some confusion.

Thanks for tracking this down and reporting it!

[v-rosa]

Hey @cthain thanks for the feedback. Yes I meant the var.container_definitions passed to the mesh-task.

Could we do something like this to catch this edge case when the healthCheck is null.

  defaulted_check_containers = length(var.checks) == 0 ? [for def in local.container_defs_with_depends_on : def.name
-  if contains(keys(def), "essential") && contains(keys(def), "healthCheck")] : []
+   if contains(keys(def), "essential") && contains(keys(def), "healthCheck") && try(def.healthCheck, null)] : []

I can open a PR if this approach is acceptable.

I'm proposing this mainly because it might not be feasible to ensure the task_def being passed in var.container_definitions has the healthCheck property omitted, for instance if the task_def is created by another terraform module based on parameters, health check will always need to have a value either null or a valid map.

[cthain]

@v-rosa Yes your proposal is an acceptable solution. I tested it out and it catches this edge case as expected.

I completely understand the rationale/need for handling this and if you are willing to PR this change we would appreciate it greatly!

[v-rosa]

Awesome, here it goes: https://github.com/hashicorp/terraform-aws-consul-ecs/pull/153

[cthain]

Thanks for your contribution @v-rosa. Your PR has been merged to main and will be included in the upcoming v0.6.0 release.