feat: Add variable to set custom tags on cluster primary SG

[jnonino]

Description

The purpose of this change is to add the possibility to set additional tags to the cluster primary security group.

Motivation and Context

With version 19, aws_default_tags were removed because it may lead to update conflicts and should be managed by the user in the provider configuration. I agree with this and in the projects where I work, we are setting all the default tags in the provider. But when upgrading from version 18 to version 19 this cause the deletion of several tags from the cluster primary security group that are desired to have.

Exploring the code, I found this:

resource "aws_ec2_tag" "cluster_primary_security_group" {
  # This should not affect the name of the cluster primary security group
  # Ref: https://github.com/terraform-aws-modules/terraform-aws-eks/pull/2006
  # Ref: https://github.com/terraform-aws-modules/terraform-aws-eks/pull/2008
  for_each = { for k, v in merge(var.tags, var.cluster_tags) :
    k => v if local.create && k != "Name" && var.create_cluster_primary_security_group_tags && v != null
  }

  resource_id = aws_eks_cluster.this[0].vpc_config[0].cluster_security_group_id
  key         = each.key
  value       = each. Value
}

Considering we are setting default tags in the provider, with the previous configuration, the cluster primary security group was getting the default tags. Now, it is only receiving tags from var.tags and var.cluster_tags so if I want the cluster primary security group to have the default tags, I only can do it using those variables because this SG is not created by the module it does not get the tags from the provider. The problem of using var.tags and var.cluster_tags is that those variables are also applied to the other resources in the module, causing terraform to throw this error.

Error: "tags" are identical to those in the "default_tags" configuration block of the provider: please de-duplicate and try again

With this new variable it would be possible to assign tags to the cluster primary security group only, without impacting other resources.

Breaking Changes

None known.

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects
• I have executed pre-commit run -a on my pull request
• I have tested and validated these changes using one my own cluster

[ivankatliarchuk]

Question to maintainers. Any reason why default tags data.aws_default_tags.current.tags no longer supported for primary secruity group?

[bryantbiggs]

cluster_tags should be used for this - we don't need a separate variable just for the security group that EKS creates

@ivankatliarchuk provider tags have been removed due to the numerous issues they created. Modules shouldn't need to do anything special with provider tags - they should just work and propagate down to the supported resources created by the provider

[ivankatliarchuk]

@bryantbiggs I got your point. But this security group is created by EKS itself, how you expect terraform-aws-provider to propagate provider tags? There is no way to propagate down default tags for this particular resoucrce.

For exact reason there is a line

resource_id = aws_eks_cluster.this[0].vpc_config[0].cluster_security_group_id

[bryantbiggs]

I don't know what you mean by For exact reason there is a line

But you can add any tags you would like via the cluster_tags variable

[ivankatliarchuk]

I mean, that the resource is created outside the terraform-aws-provider, even given the fact that Terraform requested EKS creation. For example resource https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_cluster send an API to request a cluster creation only. EKS in the background creates this primary SG.

[ivankatliarchuk]

I might not explaining correctly, but the reason why the resource exists (in my understanding)

resource "aws_ec2_tag" "cluster_primary_security_group" { 
  for_each = { for k, v in merge(var.tags, var.cluster_tags, data.aws_default_tags.current.tags) :
    k => v if local.create && k != "Name" && var.create_cluster_primary_security_group_tags
  }

  resource_id = aws_eks_cluster.this[0].vpc_config[0].cluster_security_group_id
  key         = each.key
  value       = each.value
}

Is because

• SG is implicilty created outside of Terraform, and the only way to attach any tags to it is to reference a resource and attach all the tags one buy one.
• The resource aws_ec2_tag manages individual resource tags, and is imposible to combine default_tags and aws_ec2_tag

[bryantbiggs]

correct - I added this resource so that tags are propagated to the security group that the EKS service created since Terraform did NOT create it. if you wish to add any additional tags to this security group you can provide them via cluster_tags (or more generically, tags)

[demolitionmode]

A rough example of using the cluster_tags variable would be:

provider "aws" {
  default_tags {
    tags = {
      team = "foobar"
    }
  }
}

module "eks" {
  ...
  cluster_tags = {
    team = "foobar" # This causes a conflict with default_tags
  }
}

The default_tags configuration in the provider is used to tag all resources with team: "foobar", which also applies to the aws_eks_cluster.this resource; it does not apply to the primary security group however as that is not provisioned via the aws provider. Due to the conflicting tags, they cannot be used in conjunction to apply the same tags.

I believe this is where the below error arises

Error: "tags" are identical to those in the "default_tags" configuration block of the provider: please de-duplicate and try again

[bryantbiggs]

again - we are not taking on any additional work to mitigate the current deficiencies of the provider default tags and instead you should defer to the upcoming changes hashicorp/terraform-provider-aws#29747

[demolitionmode]

Specifically the Have to ability to define identical tags that are overwritten on the resource level item, from your perspective is that the expected change that would/should solve this issue?

[ivankatliarchuk]

I'll leave it here if someone is going to have a similar issue.

The case is only partially related. How you expect a resource aws_ec2_tag to support provider level default_tags for resources created outside of a terraform?

Example tagging, in yaml for simplicity

default_tags:
  owner: paas
  managed_by: terraform

tags:
  service: kubernetes

cluster_tags:
  name: cluster-01

With the input above, SG will only get following tags set

sg_tags:
  service: kubernetes
  name: cluster-01

If we decide to merge default and cluster tags, below input is not going to work at least in meantime. There is a plan to support duplicates, but why would we even duplicate tags.

default_tags:
  owner: paas
  managed_by: terraform

tags:
  service: kubernetes

cluster_tags:
  owner: paas
  managed_by: terraform
  name: cluster-01

So the options left

• Attach provider default_tagsto SG tags outside of a module
• Do not use default_tags
• Wait of a terraform-aws-provider issue resolution and duplicate TAG(s)

[bryantbiggs]

Specifically the Have to ability to define identical tags that are overwritten on the resource level item, from your perspective is that the expected change that would/should solve this issue?

Correct

How you expect a resource aws_ec2_tag to support provider level default_tags for resources created outside of a terraform?

I don't expect anything - I am simply working with what exists. The EKS service creates this security group and its outside of Terraform's control - which is why the tag resource was added. If you wish to add tags to this security group, you can use tags or cluster_tags. I am NOT trying to make provider tags feel transparent to all the resources that are not under Terraform's control

[jnonino]

Regarding the last question you answered, currently there is no feature in AWS provider to overwrite tags and avoid error because of duplication.

Error: "tags" are identical to those in the "default_tags" configuration block of the provider: please de-duplicate and try again

You removed all references of aws_default_tags to avoid update conflicts; this is the responsibility of the provider and should be handled at the provider level and we agree with that, that's why we use the provider to set default tags. But given the cluster primary security group is not created by Terraform in your module, it does not get the default tags. Your suggestion to use tags or cluster_tags will add the tags for the cluster primary SG but it will also add the tags to all the other resources, therefore is not suitable when you you have default tags coming from the provider because it lead to the error described above.

So you say you are simply working with what exists, but you are actually working based on a provider feature that may be released sometime in the future and we are not sure it will actually solve the problem. And even when that solution solved the duplication error it will be weird to have default tags in the provider and at the same time set the variable tags or cluster_tags with the same default tags coming from the provider just for the module to assign the tags to the cluster primary SG.

[bryantbiggs]

And yet you say this proposed change in your PR will work ...

• https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/UPGRADE-19.0.md#removed
• Module resources not inheriting default_tags from AWS provider. #1455
• Provider default_tags are missing for eks_managed_node_group/aws_launch_template #2275
• Cannot connect to cluster after adding "default_tags" to aws provider #1651
• Global Tags - Note: Objects have changed outside of Terraform every run #1578
• default_tags bug Error: Provider produced inconsistent final plan #2079
• Support default_tags in aws_autoscaling_group #1916
• Idempotency issue when using Provider tags #2214
• fix: Invalid for_each argument with var.create_cluster_primary_security_group_tags = false #2264

[jnonino]

I actually tested it, it works for the intended purpose, you can set whatever tags you want to the cluster primary group. If you check my changes, I'm not doing anything with the provider tags or anything like that. I just added a variable so you can add tags to the cluster primary SG not using tags or cluster_tags because those are applied to all the other resources.

Does this change needs to be there forever? No, once the provider supports to overwrite tags instead of throwing a duplication error this variable can be removed or refactored or have a discussion again. But now, the situation is, I have a terraform project using your module but also creating other resources and I'd like to have every resource tagged with some default tags, how do we do that? setting default tags in the provider. But resources not created by terraform, like the cluster primary SG are not picking up those tags for obvious reasons. And the solutions you are giving me to add the default tags to those resources too are:

1. Use the variables tags or cluster_tags. But if I used those, I have a conflict with the default tags coming from the provider because of duplication.
2. Wait for a provider feature that would allow overriding tags, but it is not released yet.

In conclusion, your module is not providing any way to apply the default tags that are being applied to the rest of the resources to resources like the cluster primary SG. And I get it, that SG is created by EKS, not by terraform but it is indirectly created by your module because the SG gets created when you create an EKS cluster which is why we your module, to create EKS clusters.

[jnonino]

When you say:

And yet you say this proposed change in your PR will work ...

It sounds like you know some scenario when my change won't work. If that is the case and I'm missing something, please let me know, show me which is that use case and how the change does not work and I can learn something. If it is that you don't think that this change should be in the module, please explain your reasons without suggesting options that we already tried, and we are telling you that generate errors or options that are based on features not available yet.

I know that it is your module, and at end you'll do with it what you want, but if you offer something open source expecting the community to use it, it would be good that you explain your decisions and to be open to community use cases that you might not have thought about. We are presenting here a use case that is clearly not supported by the module and we expect maintainers to work with us in a solution

[github-actions]

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.