terraform-aws-ecs-service icon indicating copy to clipboard operation
terraform-aws-ecs-service copied to clipboard

task_definition.memory and task_definition.cpu values need to be quoted

Open kwessel opened this issue 4 years ago • 9 comments

If my terragrunt.hcl task_definition block doesn't contain a task_role_arn parameter and the values for the cpu and memory parameters aren't inside double quotes, I receive the below error. Quoting the values for cpu and memory or adding a task_arn_role (even if it's task_role_arn = ""), this error doesn't occur.

Error: Invalid function argument

on locals.tf line 31, in locals: 28: 29: 30: 31: "containers.json", 32:

Invalid value for "default" parameter: the default value must have the same type as the map elements.

Error: Invalid function argument

on locals.tf line 35, in locals: 35: network_mode = lookup(var.task_definition, "network_mode", "awsvpc")

Invalid value for "default" parameter: the default value must have the same type as the map elements.

Error: Invalid function argument

on locals.tf line 36, in locals: 36: task_role_arn = lookup(var.task_definition, "task_role_arn", "")

Invalid value for "default" parameter: the default value must have the same type as the map elements.

kwessel avatar Oct 07 '20 16:10 kwessel

@ddriddle, this is an interesting and peculiar bug. If you have any ideas, I would be interested in hearing them. This particular use case tickles an issue that I don't think we've seen before.

JonRoma avatar Oct 07 '20 18:10 JonRoma

@JonRoma and @kwessel this is not a bug per se. It is a design limitation or even feature depending on your view point. Terraform is a typed language, and does not allow a map to contain mixed types. All values of a map must be of the same type. The variable task_definition is declared type map(any) so at runtime it can be a map of strings or a map of integers but not both at the same time: "map(any), which accepts any element type as long as every element is the same type" (See Terraform Type Constraints).

When you set task_definition as a map of integers at runtime then the lookups fail on line 35 and 36 due to a type error because both lookups return strings which can not be converted to integers automatically. To fix this issue I recommend you change the type from map(any), which is mostly useless, to map(string). With that change the integers will automatically be converted to a string.

An alternative solution is change the type of task_definition to an object but then you lose default values (See issue https://github.com/hashicorp/terraform/issues/19898). For that reason I do not recommend it.

ddriddle avatar Oct 07 '20 20:10 ddriddle

This was my theory the other day: maps of type any doesn’t mean the map can contain any types. It has to be all one type. That made sense, sort of.

But then why, @ddriddle, is it that if I also include task_role_arn = “” then I don’t need to quote the cpu and memory values? Is it smart enough to cast them?

From: David D. Riddle [email protected] Sent: Wednesday, October 7, 2020 3:14 PM To: techservicesillinois/terraform-aws-ecs-service [email protected] Cc: Wessel, Keith [email protected]; Mention [email protected] Subject: Re: [techservicesillinois/terraform-aws-ecs-service] task_definition.memory and task_definition.cpu values need to be quoted (#45)

@JonRomahttps://github.com/JonRoma and @kwesselhttps://github.com/kwessel this is not a bug per se. It is a design limitation or even feature depending on your view point. Terraform is a typed language, and does not allow a map to contain mixed types. All values of a map must be of the same type. The variable task_definition is declared type map(any) so at runtime it can be a map of strings or a map of integers but not both at the same time: "map(any), which accepts any element type as long as every element is the same type" (See Terraform Type Constraintshttps://www.terraform.io/docs/configuration/types.html).

When you set task_definition as a map of integers at runtime then the lookups fail on line 35 and 36 due to a type error because both lookups return strings which can not be converted to integers automatically. To fix this issue I recommend you change the type from map(any), which is mostly useless, to map(string). With that change the integers will automatically be converted to a string.

An alternative solution is change the type of task_definition to an object but then you lose default values (See issue hashicorp/terraform#19898https://github.com/hashicorp/terraform/issues/19898). For that reason I do not recommend it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/techservicesillinois/terraform-aws-ecs-service/issues/45#issuecomment-705168828, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATJOTEORQD7GPMURKUNI2LSJTDZZANCNFSM4SHUPHOQ.

kwessel avatar Oct 07 '20 20:10 kwessel

I know that during the conversion to Terraform 0.12, we ran into peculiarities dealing with Terraform's new typing system, particularly the parts of that system that aren't fully polished.

The confounding thing about @kwessel and my adventure the other day was the unexpected side effects of a seemingly unrelated change in the configuration.

JonRoma avatar Oct 07 '20 20:10 JonRoma

At the time we thought map(any) was the safest option, I think, but it looks like map(string) is better, and gives the behavior that we would have expected with Terraform 0.11. Of course you will need to verify that, but that is my reading of the situation.

ddriddle avatar Oct 07 '20 20:10 ddriddle

But then why, @ddriddle, is it that if I also include task_role_arn = “” then I don’t need to quote the cpu and memory values? Is it smart enough to cast them?

@kwessel The short answer is yes. Terraform will cast numbers to strings automatically. Since you are setting a string in the map it automatically creates a map of strings and casts everything it can to a string. If there were something else that could not be automatically casted to a string then it would of course fail.

ddriddle avatar Oct 07 '20 20:10 ddriddle

So, if we set the map to a type of string instead of any, it’ll do the casting even if I don’t quote the values or include a task_role_arn? If so, this sounds like the way to go to me.

From: David D. Riddle [email protected] Sent: Wednesday, October 7, 2020 3:36 PM To: techservicesillinois/terraform-aws-ecs-service [email protected] Cc: Wessel, Keith [email protected]; Mention [email protected] Subject: Re: [techservicesillinois/terraform-aws-ecs-service] task_definition.memory and task_definition.cpu values need to be quoted (#45)

But then why, @ddriddlehttps://github.com/ddriddle, is it that if I also include task_role_arn = “” then I don’t need to quote the cpu and memory values? Is it smart enough to cast them?

@kwesselhttps://github.com/kwessel The short answer is yes. Terraform will cast numbers to strings automatically. Since you are setting a string in the map it automatically creates a map of strings and casts everything it can to a string. If there were something else that could not be automatically casted to a string then it would of course fail.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/techservicesillinois/terraform-aws-ecs-service/issues/45#issuecomment-705179385, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AATJOTC27NVTTZDFDU4ESJTSJTGKPANCNFSM4SHUPHOQ.

kwessel avatar Oct 07 '20 20:10 kwessel

@kwessel that is correct. I recommend changing the type to string. Please test that it works as we expect but it should: "The Terraform language will automatically convert number and bool values to string values when needed, and vice-versa as long as the string contains a valid representation of a number or boolean value." (See Terraform Type Constraints).

ddriddle avatar Oct 07 '20 21:10 ddriddle

In theory, an object with a sane default should work in modern Terraform versions.

JonRoma avatar Dec 02 '22 09:12 JonRoma

This is fixed by v3.3.0, which is in beta. A major refactor allows variables to be defined as complex objects, each element of which using the native types like numbers. Legacy versions of Terraform weren't quite so intelligent as they are today.

JonRoma avatar Feb 24 '23 17:02 JonRoma