Unexpected instance tags after upgrading to v6.0.1

[nwalters512]

Description

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

After upgrading to v6.x of this module, an explicitly-specified tag isn't being applied to the instance.

Versions

• Module version [Required]: 6.0.1
• Terraform version: v1.12.2
• Provider version(s): registry.terraform.io/hashicorp/aws v6.0.0

Reproduction Code [Required]

module "main_ec2_instance" {
  source  = "terraform-aws-modules/ec2-instance/aws"
  version = "~> 6.0.0"

  ami = "ami-0c00c714c7f84b49d"
  instance_type = "t3.medium"

  tags = {
    Name = "my.custom.name"
  }
}

Steps to reproduce the behavior:

• Apply the above with v5.x of the provider.
• Apply the above with v6.x of the provider.

Expected behavior

I'd expect my instance to end up with a Name tag with value my.custom.name

Actual behavior

Terraform tries to remove the Name tag entirely; the diff shows null as the value.

Terminal Output

I manually stripped out information specific to my project from the below.

  # REDACTED.aws_instance.this[0] will be updated in-place
  ~ resource "aws_instance" "this" {
        id                                   = "REDACTED"
      ~ tags                                 = {
          ~ "Name" = "my.custom.name" -> null
        }
      ~ tags_all                             = {
          - "Name"        = "my.custom.name"
        } -> (known after apply)
        # (37 unchanged attributes hidden)

        # (9 unchanged blocks hidden)
    }

Additional context

The changelog for v6.0.0 (https://github.com/terraform-aws-modules/terraform-aws-ec2-instance/releases/tag/v6.0.0) shows that this might have been intentional:

Correct tag precedence ordering (least specific to most specific)

However, I can't understand why this module is trying to forcefully override a tag that I have explicitly provided.

Here's the relevant code AFAICT:

terraform-aws-ec2-instance/main.tf

Lines 10 to 14 in a299529

	instance_tags = merge(
	var.tags,
	var.instance_tags,
	{ "Name" = var.name },
	)

I think it would be much more sensible for the Name tag to be listed earlier so that explicit tags that I provide take precedence.

[bryantbiggs]

you want the instance to be named something other than var.name?

[nwalters512]

It's moreso that if I specify an explicit tag, I don't want it to be erased by this module. I haven't actually specified a name attribute at all. While it's a nice shorthand, I prefer an explicit list of tags. IMO it better expresses intent; I had no idea name would make its way into a tag until I read the source for the module!

[bryantbiggs]

this is common throughout all of our modules - if var.name is present, or some form similar (service_name, etc.), that is usually propagated as a { Name = var.name } tag.

The precedence ordering is typically done this way as well, unless there is a higher specificity (i.e. - usually found within a for_each loop where name is no longer unique)

But as it stands, it is correct. If you pass name = foo to your EC2, you would, and should, expect to see it named foo

[nwalters512]

I think it's entirely reasonable to use name if it's present, and I'm even fine with the precedence order. The surprising behavior is what happens when it isn't set. I think what I want is basically this:

instance_tags = merge(
  var.tags,
  var.instance_tags,
  var.name != null ? { "Name" = var.name } : {}
)

Would you accept a change like this?

[bryantbiggs]

absolutely, thats a valid point

However, I might suggest the following instead given the variable default value:

instance_tags = merge(
  var.tags,
  var.instance_tags,
  var.name != "" ? { "Name" = var.name } : {}
)