Using terraform <1.6.0, `aws_ec2_tag` with dynamic tag *values* results in for_each error about unknown *keys*

lorengordon commented 1 month ago

Description

When using Terraform <1.6.0, a tag value that includes a dynamically interpolated function, like timestamp(), will result in an error of the form:

│ Error: Invalid for_each argument
│ 
│   on .terraform/modules/eks.cluster/main.tf line 111, in resource "aws_ec2_tag" "cluster_primary_security_group":
│  111:   for_each = { for k, v in merge(var.tags, var.cluster_tags) :
│  112:     k => v if local.create && k != "Name" && var.create_cluster_primary_security_group_tags && v != null
│  113:   }
│     ├────────────────
│     │ local.create is true
│     │ var.cluster_tags is empty map of string
│     │ var.create_cluster_primary_security_group_tags is true
│     │ var.tags is map of string with 7 elements
│ 
│ The "for_each" map includes keys derived from resource attributes that
│ cannot be determined until apply, and so Terraform cannot determine the
│ full set of keys that will identify the instances of this resource.
│ 
│ When working with unknown values in for_each, it's better to define the map
│ keys statically in your configuration and place apply-time results only in
│ the map values.
│ 
│ Alternatively, you could use the -target planning option to first apply
│ only the resources that the for_each value depends on, and then apply a
│ second time to fully converge.

Example setup for tags. We add an expiration to all resources, sorta like the below, but a little different to avoid a persistent diff. This does reproduce though:

  tags = {
    expiration = formatdate("YYYY-MM-DD", timeadd(timestamp(), "24h"))
  }

[x] ✋ I have searched the open/closed issues and my issue is not listed.

Actually, if you search for aws_ec2_tag, there are lots of issues all saying the same thing. Difference here is that I know why it is occurring! :D

Valid forms of this error pertain only to keys. But this is a value. Dynamic values should be fine in any for_each expression, so obviously the error is bogus, and represents a bug in Terraform. And this is actually fixed in Terraform 1.6.0 and newer, but we're still pinned to 1.5.7 for now due to the license change.

Still, I was curious, why was this happening at all, and can we modify the module to avoid the bug? Looking at the module code:

https://github.com/terraform-aws-modules/terraform-aws-eks/blob/5fe865e860c4cc8506c639f2e63bc25e21a31b37/main.tf#L126-L128

I couldn't understand why the check was there for v != null. And since this is happening for values, it made me suspicious. So I removed the check. And the error went away. Hurrah!

I wanted to understand why the check was there, so I looked into the blame for the file, and saw the check was introduced in PR 2250. I see @antonbabenko commented in the review, suggesting the check, but no particular explanation. Just maybe a sense that null would be invalid anyway, so might as well remove it? I can't think of any valid reason a tag value would be null. So to me it just seems like user error. Since the check causes problems (for many folks, going off the number of issues in the search), I'm hoping a PR would be accepted to remove the check.

antonbabenko commented 1 month ago

This issue has been resolved in version 20.22.1 :tada:

github-actions[bot] commented 2 weeks ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

terraform-aws-modules / terraform-aws-eks