Provider default tags an tags for self_managed_node_group triggering changes every time.

[ab-wave-rider]

We have default provider tags which we need to propagate_at_launch for EC2 instances.
But code has no block to set additional tags manually on ASG with the propagate_at_launch flag, it only works with var.tags
https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/node_groups.tf#L506
It would be manageable if other resources wouldn't depend on this as well, but:

module.self_managed_node_group["workers"].aws_iam_instance_profile.this[0]
module.self_managed_node_group["workers"].aws_iam_role.this[0]
module.self_managed_node_group["workers"].aws_launch_template.this[0]

get their tags from the same variable and causing duplicate tags always show up as pending changes on these resources

# module.orch1.module.eks_cluster.module.self_managed_node_group["workers"].aws_iam_instance_profile.this[0] will be updated in-place
  ~ resource "aws_iam_instance_profile" "this" {
...
      ~ tags        = {
          + "management"        = "terraform"
        }
    }

# module.orch1.module.eks_cluster.module.self_managed_node_group["workers"].aws_iam_role.this[0] will be updated in-place
  ~ resource "aws_iam_role" "this" {
...
      ~ tags                  = {
          + "management"        = "terraform"
        }
    }

# module.orch1.module.eks_cluster.module.self_managed_node_group["workers"].aws_launch_template.this[0] will be updated in-place
  ~ resource "aws_launch_template" "this" {
...
      ~ tags                    = {
          + "management"        = "terraform"`
        }
    }

Removing tags from provider causing serious crash, which is a separate issue:

Error: Invalid for_each argument

  on .terraform/modules/eks_cluster/modules/self-managed-node-group/main.tf line 750, in resource "aws_iam_role_policy_attachment" "this":
 750:   for_each = { for k, v in toset(compact([
 751:     "${local.iam_role_policy_prefix}/AmazonEKSWorkerNodePolicy",
 752:     "${local.iam_role_policy_prefix}/AmazonEC2ContainerRegistryReadOnly",
 753:     var.iam_role_attach_cni_policy ? local.cni_policy : "",
 754:   ])) : k => v if var.create && var.create_iam_instance_profile }
    ├────────────────
    │ local.cni_policy is a string, known only after apply
    │ local.iam_role_policy_prefix is a string, known only after apply
    │ var.create is true
    │ var.create_iam_instance_profile is true
    │ var.iam_role_attach_cni_policy is true

The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target
argument to first apply only the resources that the for_each depends on.

So essentially the way code is written in v19 makes it impossible to work around this w/o removing tags from the provider, but removing tags from the provider is unacceptable and makes tag management a daymare.

[bryantbiggs]

provider default tags should "just work" - we are not doing anything different in this module to accommodate provider tags; I would follow the changes coming in the v5 AWS provider for this hashicorp/terraform-provider-aws#29842

[ab-wave-rider]

@bryantbiggs so what is the resolution in your opinion for the propagate_at_launch tags that must match provider default tags?
In v18 you had use_default_tags flag which allowed us to work around it, but in v19 you removed it and now it is causing infinite loop of pending changes.
It is clearly an oversight, you closed it w/o full understanding of a problem it seems. Yes default tags "just work" there is no real issue with the provider tags, it is inability to have same tags as default provider tags on the EC2 instances launched by ASG.
Here is your commit which broke it:
https://github.com/terraform-aws-modules/terraform-aws-eks/pull/2250/files#diff-63131896f5123c9ea9363547a14c46df7eb2c6d1173ab128f2230a0957abb06fL393
var.use_default_tags ? merge(data.aws_default_tags.current.tags, var.tags) : var.tags

Just reverting this small change would be an acceptable solution.

[bryantbiggs]

I would suggest looking back through issues and PRs - adding hacks to patch the short comings of the current provider default tags is not tenable and this is why I say that provider tags should "just work" but they currently do not. We won't be making any changes for provider tags in this module at this time - I suggest you follow the AWS provider v5 issue which is aimed largely at correcting the behavior of the provider default tags

[ab-wave-rider]

@bryantbiggs what is proposed solution?
I suggest to fix it with a simplemerge(data.aws_default_tags.current.tags, var.tags) I can even submit a PR for you if you'd like.
After your commit folks who want to propagate default provider tags to ASG launched instances are stuck with perpetual changes because you put var.tags into places that take default tags and ones that don't.
It is too easy to blame provider developers for this. FWIW this is a BREAKING CHANGE.

[bryantbiggs]

I suggest to fix it with a simplemerge(data.aws_default_tags.current.tags, var.tags) I can even submit a PR for you if you'd like.

We had this, and it raised a bunch of issues

The proposed solution is this hashicorp/terraform-provider-aws#29842

[ab-wave-rider]

We already migrated to v19 and discovered this issue with tagging only after that fact because you did not point out that it was breaking things.
For folks like me who already on v19, your solution to wait for aws provider fix is not a solution at all, not even a workaround.

1. There was no release note that would point out that tagging change you introduces is a BREAKING change.
2. No proposed solution or workaround other than a potential FUTURE fix for this in the provider (see if someone else can fix this in the future)
3. Not only timing for this change was wrong (because there is no fix on the provider yet), the provider issue itself has no guarantee that it might work and a BREAKING change also. While most of this work is resolving issues with the design of the feature, it does represent a significant change in behavior that we can’t be sure will not be disruptive for users with existing default tag configurations. As a result, it is considered a breaking change.

Very professional.

So I'll give it a try and ask again. What do you suppose we do in our case to work around it?

[bryantbiggs]

1a. https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/UPGRADE-19.0.md#removed
1b. #2250
2. As stated in the links above, it is not tenable for this module to try to fix something like the provider level tags here. We accepted PRs that attempted to do this and no matter what changes we tried, none completely resolved the issue and therefore we removed the hacks. This is why we state that the resolution is for the provider to fix this issue - I'm not sure what else I can provide here
3. I don't follow - the change in the provider is slated for v5.0 which is a breaking change for the provider and why they are making it there and not the latest v4.x version

I don't see how I am being un-professional here - your window of the world (module) is very narrow, mine is not

[ab-wave-rider]

1a. https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/UPGRADE-19.0.md#removed
1b. #2250

Removed is not the best place for breaking change. You have specific section for that. It is like removing some component from a system, notifying people about it, but failing to mention that some things will stop working. Unprofessional? Maybe, more of an oversight or mistake.

2. As stated in the links above, it is not tenable for this module to try to fix something like the provider level tags here. We accepted PRs that attempted to do this and no matter what changes we tried, none completely resolved the issue and therefore we removed the hacks. This is why we state that the resolution is for the provider to fix this issue - I'm not sure what else I can provide here

Your removal of hacks was very badly timed. For the ASG propagate_at_launch tags specifically, It should have been removed AFTER fix for this would actually exist, not BEFORE. Unprofessional? Yes

4. I don't follow - the change in the provider is slated for v5.0 which is a breaking change for the provider and why they are making it there and not the latest v4.x version

Not fully understanding this part speaks for itself. Unprofessional? Yes

I don't see how I am being un-professional here - your window of the world (module) is very narrow, mine is not

Could something like this be written by a professional? Not in my narrow world.

So I stand by my statement.

[bryantbiggs]

I'm sorry but I'm not going to tolerate this type of interaction - I have provided as much information and detail as to why the changes were made, where those changes are documented, and why we are unable to accept any further changes in this area