Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Tags to ASG when using node groups #1558

Closed
marianobilli opened this issue Aug 31, 2021 · 37 comments · Fixed by #1680
Closed

Add Tags to ASG when using node groups #1558

marianobilli opened this issue Aug 31, 2021 · 37 comments · Fixed by #1680

Comments

@marianobilli
Copy link
Contributor

Is your request related to a new offering from AWS?

No

Is your request related to a problem? Please describe.

When using nodegroups it is necessary to be able to add tags to the autoscaling groups as described in AWS EKS Cluster Autoscaler Doc

in my case I need to add tags related to taints
Key: k8s.io/cluster-autoscaler/node-template/taint/$TAINT_KEY Value: NoSchedule

Describe the solution you'd like.

Include in the node group module, the positiblity to add asg_tags per each nodegroup and in the module use the terraform resource aws_autoscaling_group_tag

Describe alternatives you've considered.

Manually mantain the ASG tags. t(-_-t)

@daroga0002
Copy link
Contributor

you have map node_groups_defaults where you can specify key additional_tags or specify this directly in node group map as additional_tags and then those tags can be added.

tags = merge(
var.tags,
lookup(var.node_groups_defaults, "additional_tags", {}),
lookup(var.node_groups[each.key], "additional_tags", {}),
)

@marianobilli
Copy link
Contributor Author

you have map node_groups_defaults where you can specify key additional_tags or specify this directly in node group map as additional_tags and then those tags can be added.

tags = merge(
var.tags,
lookup(var.node_groups_defaults, "additional_tags", {}),
lookup(var.node_groups[each.key], "additional_tags", {}),
)

But those tags are going to the node group not to the autoscaling group related to the node group

@daroga0002
Copy link
Contributor

autoscaling group for node groups is done thru EKS api, so you cannot influence it. If you want have own autoscaling group then follow worker nodes approach with custom AMI

@marianobilli
Copy link
Contributor Author

or node groups is done thru EKS api, so you cannot influence it. If you want have own autoscaling group then follow worker nodes approach with custom AMI

I like the nodegroups managed by aws.
Currently I added this code outside the EKS module, but adding it inside the node_group module would be so much easier

resource "aws_autoscaling_group_tag" "tag" {
  for_each = local.asg_tags
  autoscaling_group_name = module.eks_cluster.node_groups[split("_",each.key)[0]].resources[0].autoscaling_groups[0].name

  tag {
    key   = each.value.key
    value = each.value.value
    propagate_at_launch = each.value.propagate_at_launch
  }
}

I can do a proposed PR but only if the maintainers agree to the idea

@daroga0002
Copy link
Contributor

this approach is not supported by AWS, so probably we shouldn't implement it (even if possible).

EKS node group has own limitations which people must accept choosing it, if they want to hack or use unsupported tricks then I think they make it on own responsibility.

@bryantbiggs
Copy link
Member

@marianobilli you can do this with a new resource that will be released this week, but as @daroga0002 stated, we won't be adding that here hashicorp/terraform-provider-aws#20009

@marianobilli
Copy link
Contributor Author

marianobilli commented Aug 31, 2021

@marianobilli you can do this with a new resource that will be released this week, but as @daroga0002 stated, we won't be adding that here hashicorp/terraform-provider-aws#20009

@bryantbiggs the resource you shared is exactly the same I proposed, which I am proposing to add in the terraform-aws-eks module for easier use.

@marianobilli
Copy link
Contributor Author

this approach is not supported by AWS, so probably we shouldn't implement it (even if possible).

EKS node group has own limitations which people must accept choosing it, if they want to hack or use unsupported tricks then I think they make it on own responsibility.

man you need to relax a bit, if you are in krakow I'll buy you a beer.
I dont think it is a bad idea to add that new resource to the module, but if you dont want to Its your decision I guess.

@bryantbiggs
Copy link
Member

bryantbiggs commented Aug 31, 2021

I will admit, I just don't see the benefit of adding it here; adding it externally seems to be a very good approach. The module is already overly complex and we are trying to be thoughtful in what gets added because currently its quite involved in managing it, and tags are an area that create a lot of noise because you can have 100 different users of the module and 199 different ways they want to manage tags

@wgj
Copy link

wgj commented Nov 3, 2021

The module is already overly complex and we are trying to be thoughtful in what gets added because currently its quite involved in managing it[.]

@bryantbiggs I can appreciate being defensive on adding new features; terraform-aws-eks is a beast of a module for users. Hats off to everyone involved. In fact, I think @marianobilli was suggesting adding it to the node_groupssubmodule, which as I understand it should help with maintenance concerns of terraform-aws-eks proper.

[Y]ou can have 100 different users of the module and 199 different ways they want to manage tags

I think the argument to add it isn't quite that complex. I see it more as "do you want your node group tagged by the module that created it?" bringing 199 (at least!) usecases to one "yes or no" question ;), such that 'yes' is passing a map through to provider resource (aws_autoscaling_group_tag), and 'no' is current behavior. And the combinatorial problem of satisfying everyone's tagging preference is encapsulated. "You can have any tagging pattern you like, so long as it's mapped" ;)

@bryantbiggs
Copy link
Member

I guess lets see what a PR looks like then

@bryantbiggs bryantbiggs reopened this Nov 3, 2021
@wgj
Copy link

wgj commented Nov 3, 2021

@marianobilli Let me know if you're still keen to open a PR or want me to run with it.

@marianobilli
Copy link
Contributor Author

@marianobilli Let me know if you're still keen to open a PR or want me to run with it.
go ahead @wgj I am super busy and I would not be able to do it any time soon. thanks!

@daroga0002
Copy link
Contributor

for some reference
aws/containers-roadmap#608
aws/containers-roadmap#1541

@HeshamMeneisi
Copy link

@wgj Are you working on this?

This is how I implemented it on our custom eks module (extending this module):

locals{
...
  asg_tag_list = flatten([
      for node_group_key, node_group in var.node_groups: [
        for tag in lookup(node_group, "asg_tags", []): {
          node_group_key = node_group_key
          key = tag.key
          value = tag.value
          propagate_at_launch = tag.propagate_at_launch
        }
      ]
    ])
...
}
resource "aws_autoscaling_group_tag" "tag" {
  count = length(local.asg_tag_list)

  autoscaling_group_name = module.eks.node_groups[local.asg_tag_list[count.index].node_group_key].resources[0].autoscaling_groups[0].name

  tag {
    key   = local.asg_tag_list[count.index].key
    value = local.asg_tag_list[count.index].value
    propagate_at_launch = local.asg_tag_list[count.index].propagate_at_launch
  }
}

If you are going to do this on the node_groups module though, it'll be a bit different.

@gabops
Copy link

gabops commented Nov 25, 2021

Hello there,

I've implemented the tagging of ASGs in managed node groups and I though someone else might find it useful. This is how I did it:

Logic in modules/node_groups/locals.tf:

locals {
...
  asg_tag_list = flatten([
    for name, info in var.node_groups : [
      [
        for tag in lookup(local.node_groups_expanded[name], "asg_tags", []) : {
          group_name = name
          key        = tag.key
          propagate  = try(tag.propagate_at_launch, false)
          value      = tag.value
        }
      ]
    ]
  ])
...
}

resource call in modules/node_groups/main.tf:

resource "aws_autoscaling_group_tag" "tag" {
  for_each = { for map in local.asg_tag_list : "${map.group_name}_${map.key}" => map }

  autoscaling_group_name = aws_eks_node_group.workers[replace(each.key, "_${each.value.key}", "")].resources[0].autoscaling_groups[0].name

  tag {
    key                 = each.value.key
    value               = each.value.value
    propagate_at_launch = each.value.propagate
  }
}

After that, you just need to declare a group to be passed to the node_groups variable like this:

my_node_groups = {
  foo = {
    max_capacity     = 3
    min_capacity     = 0
    desired_capacity = 0
    instance_types   = ["t2.2xlarge"]
    k8s_labels = {
      "monitoring" = true
    }
    asg_tags = [
      {
        key                 = "k8s.io/cluster-autoscaler/node-template/label/monitoring"
        value               = true
        propagate_at_launch = true // optional, defaults to false.
      }
    ]
}

I have the change ready in a fork I created in case you want to have a look (here).

Regards.

@gabops
Copy link

gabops commented Nov 25, 2021

Related to my previous message, I've created a PR https://github.com/terraform-aws-modules/terraform-aws-eks/pull/1705/files

@rkul
Copy link

rkul commented Dec 2, 2021

@gabops thanks a lot, it was very helpful! Based on your PR I've added that tags outside of EKS module and it works!
For those who want to use this snippet, please, note that I used output of the module.my_eks_cluster to get managed node group list:

locals {
  eks_asg_tags = [
    {
      key                 = "Create_Auto_Alarms"
      value               = "true"
      propagate_at_launch = true
    },
    {
      key                 = "AutoAlarm-AWS/EC2-CPUCreditBalance-LessThanThreshold-5m-Average"
      value               = "30"
      propagate_at_launch = true
    }
  ]
  eks_asg_tag_list = flatten([
    for name, info in module.my_eks_cluster.node_groups : [
      [
        for tag in local.eks_asg_tags : {
          group_name          = name
          key                 = tag.key
          propagate_at_launch = try(tag.propagate_at_launch, false)
          value               = tag.value
        }
      ],
      [
        {
          group_name          = name
          key                 = "Name"
          propagate_at_launch = true
          value               = "eks-${name}"
        }
      ]
    ]
  ])
}

resource "aws_autoscaling_group_tag" "eks" {
  for_each               = { for map in local.eks_asg_tag_list : "${map.group_name}%${map.key}" => map }
  autoscaling_group_name = module.my_eks_cluster.node_groups[split("%", each.key)[0]].resources[0].autoscaling_groups[0].name

  tag {
    key                 = each.value.key
    value               = each.value.value
    propagate_at_launch = each.value.propagate_at_launch
  }
}

@gabops
Copy link

gabops commented Dec 10, 2021

Hi all,

I've updated my PR with some fixes for a few issues I've found when using the module with the changes I've added. I also tested it deeper. I don't have any hopes on having the PR approved and merged given the complaints about the complexity of this module (which I agree with). I suspect the way to go if you want to have this functionality is to add the changes I made to a personal fork of yours or use an approach similar to the great solution proposed by @rkul.

Regards

@timblaktu
Copy link

@marianobilli @bryantbiggs I have used the "external" approach, using the autoscaling_group_tag resource to tag my EKS node_group's ASG with propagate_at_launch set to true. I have confirmed via aws cli that this ASG does indeed have the desired Tags with Key/Value/PropagateAtLaunch, however the EC2 instance launched does NOT have these Tags set. This is all after a complete recreation of my EKS cluster (I'm using the terraform eks module to do this).

Can you think of anything that I might have missed that could be preventing the ASG tags from propagating to the EC2 instance launched in the managed node group?

Below are the commands I used after creating the cluster to verify my EKS's MNG/ASG/EC2 tags:

# get managed node group name
aws eks list-nodegroups --cluster-name <cluster_name>
# get autoscaling group name
aws eks describe-nodegroup --cluster-name <cluster_name> --nodegroup-name <mng_name>
# get names of ec2 instance residing in ASG
aws autoscaling describe-auto-scaling-groups --auto-scaling-group-names <asg_name>
# get tags for each ec2 instance
aws ec2 describe-tags --filters "Name=resource-id,Values=<ec2_id>"

@timblaktu
Copy link

timblaktu commented Dec 18, 2021

@marianobilli @bryantbiggs @gabops @rkul I have a theory why my implementation of @rkul 's external solution is not resulting in the first launched ec2 node having the right tags. I believe what is happening is that because the asg tag is being applied outside the eks module, it is happening AFTER the launching of the first ec2 node in the asg. Because ASG tags are only propagated to ec2 instances upon launch, and any changes to asg tags will NOT be applied to already-launched instances, my first-launched ec2 doesn't exhibit the desired tags. Presumably, if I could find a way to force the ASG to launch another ec2 to validate this, but I do not know how to do this at the moment.

EDIT: I have proven my theory correct (I think). I have gone into the AWS console and manually increased the "min size" of my node group, and witnessed the second ec2 instance created has the correct tags (propagated from ASG tags) while the original ec2 does NOT.

Furthermore, this begs the question, how does this workaround, using the aws_autoscaling_group_tag resource, external-to-the-module solution work for anyone? Or was this 1st instance issue just an oversight?

I mention all this because it seems to be a solid case for why this feature should be implemented in the module

@bryantbiggs
Copy link
Member

The recommendation for AWS EKS managed node groups is to create a custom launch template. When opting in to the custom launch template route, users are able to specify tags that will be propagated to instances launched.

@jaimehrubiks
Copy link
Contributor

Exactly, this PR is not to tag ec2s you can do that from the LT. this is to tag the ASG alone which is useful for the autoscaler or company tagging requirements

@timblaktu
Copy link

Thanks for that, @bryantbiggs, @jaimehrubiks. If I'm specifying a launch_template (with specified tags) with a managed node group, don't I automatically get new ec2 instance with the prescribed tags, even without the ASG having the tags?

@jaimehrubiks
Copy link
Contributor

Exactly, if you put them in the template (either a manually created launch_template, or with the option to create it for you and just specifying tags in this module), yes, you will see them in the ec2 and not in the asg

@antonbabenko
Copy link
Member

This issue has been resolved in version 18.0.0 🎉

@evolart
Copy link

evolart commented Jan 22, 2022

Is this issue the reason default_tags from the aws provider are not applied to the EC2 instances created when using the resource "aws_eks_node_group"?

@tmokmss
Copy link

tmokmss commented Jan 29, 2022

I'm afraid this issue still exists in version ~> 18.0.0. But we can still use the workaround suggested in #1558 (comment) with slight modification, by getting a generated ASG name via module.eks.eks_managed_node_groups[key].node_group_resources[0].autoscaling_groups[0].name or module.eks_managed_node_groups.node_group_resources[0].autoscaling_groups[0].name.

@mckennajones
Copy link

@tmokmss I am also not seeing tags propagated to the ASG(s). The tags are visible on the Launch Template, and instances, but not the ASG.

@antonbabenko Do you know if there is somewhere that shows a working example of this feature?

@bryantbiggs
Copy link
Member

Yes, see the eks-managed-node-group example

@mckennajones
Copy link

mckennajones commented Feb 5, 2022

@bryantbiggs I took a look at the mentioned example. I saw the following comment:

# By default, the module creates a launch template to ensure tags are propagated to instances, etc.,
# so we need to disable it to use the default template provided by the AWS EKS managed node group service

Based on this I left the create_launch_template value to its default true, which it seems should result in propogation of the tags.

Here is my simple TF:

module "eks"  {
  source = "terraform-aws-modules/eks/aws"

  cluster_name                    = "ASG-Test"
  cluster_version                 = "1.21"
  cluster_endpoint_private_access = true
  subnet_ids                      = ["subnet-1", "subnet-2"]
  vpc_id                          = "vpc-1"
  enable_irsa                     = true

  eks_managed_node_groups = {
    default = {
      max_size = 1
      min_size = 1
      desired_size = 1
      instance_types = ["m6i.large"]
      
      tags = {
        "k8s.io/cluster-autoscaler/node-template/label/foo" = "bar"
      }
    }
  }
}

Version details for testing:

  • Terraform: v1.1.4
  • terraform-aws-modules/eks: 18.3.1

Unfortunately with the above, the tag I have specified is not propogated to the ASG:

➜ aws eks list-nodegroups --cluster-name ASG-Test --query 'nodegroups[0]'
"default-2022020504222678110000000d"
➜ aws eks describe-nodegroup --cluster-name ASG-Test --nodegroup-name default-2022020504222678110000000d --query 'nodegroup.resources.autoScalingGroups[0].name'
"eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855"
➜ aws autoscaling describe-auto-scaling-groups --auto-scaling-group-names eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855 --query 'AutoScalingGroups[0].Tags'
[
    {
        "ResourceId": "eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855",
        "ResourceType": "auto-scaling-group",
        "Key": "eks:cluster-name",
        "Value": "ASG-Test",
        "PropagateAtLaunch": true
    },
    {
        "ResourceId": "eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855",
        "ResourceType": "auto-scaling-group",
        "Key": "eks:nodegroup-name",
        "Value": "default-2022020504222678110000000d",
        "PropagateAtLaunch": true
    },
    {
        "ResourceId": "eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855",
        "ResourceType": "auto-scaling-group",
        "Key": "k8s.io/cluster-autoscaler/ASG-Test",
        "Value": "owned",
        "PropagateAtLaunch": true
    },
    {
        "ResourceId": "eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855",
        "ResourceType": "auto-scaling-group",
        "Key": "k8s.io/cluster-autoscaler/enabled",
        "Value": "true",
        "PropagateAtLaunch": true
    },
    {
        "ResourceId": "eks-default-2022020504222678110000000d-f8bf640f-3bf7-0c93-74ce-0340039e0855",
        "ResourceType": "auto-scaling-group",
        "Key": "kubernetes.io/cluster/ASG-Test",
        "Value": "owned",
        "PropagateAtLaunch": true
    }
]

Am I missing anything here?

@tmokmss
Copy link

tmokmss commented Feb 5, 2022

Hi @mckennajones, you should be able to add tags to ASGs by the code below:

resource "aws_autoscaling_group_tag" "this" {
  autoscaling_group_name = module.eks.eks_managed_node_groups["default"].node_group_resources[0].autoscaling_groups[0].name

  tag {
    key                 = "k8s.io/cluster-autoscaler/node-template/label/foo"
    value               = "bar"
    propagate_at_launch = false
  }
}

And I guess it might help if we add this snippet to the examples...

@bryantbiggs
Copy link
Member

bryantbiggs commented Feb 5, 2022

ya, unfortunately this one is not solved. I tried a bunch of different ways but it doesn't look like we can put the aws_autoscaling_group_tag resource into the module either because there is a dependency/lifecycle conflict, and unfortunately this looks like the only way to tag EKS managed node groups

Note: this solution doesn't work

resource "aws_autoscaling_group_tag" "this" {
  # Build map of maps to iterate over = `for_each` won't take a list of maps here
  for_each = { for tag in flatten([
    for asg in flatten([
      # Unpack autoscaling group name from EKS node group
      for resources in try(aws_eks_node_group.this[0].resources, {}) : resources.autoscaling_groups
      ]) : [
      # Map each tag in `var.tags` to each autoscaling group in EKS node group => returns list of maps
      for k, v in merge(var.tags, var.autoscaling_group_tags) : { asg = asg.name, key = k, val = v }
    ]
  ]) : "${tag.asg}-${tag.key}" => { asg = tag.asg, key = tag.key, val = tag.val } if var.create }

  autoscaling_group_name = each.value.asg

  tag {
    key                 = each.value.key
    value               = each.value.val
    propagate_at_launch = true
  }

  depends_on = [
    aws_eks_node_group.this
  ]
}

@mckennajones
Copy link

@tmokmss thanks for the suggestion. I had to slightly modify the autoscaling_group_name property to the following:

autoscaling_group_name = module.eks.eks_managed_node_groups["default"].node_group_resources.autoscaling_groups[0].name

Difference is referencing node_group_resources instead of just resources. Otherwise terraform errored out with: This object does not have an attribute named "resources".

@bryantbiggs or @antonbabenko should this issue be re-opened?

@tmokmss
Copy link

tmokmss commented Feb 7, 2022

@mckennajones thanks, I fixed my comment.

@bryantbiggs
Copy link
Member

@tmokmss thanks for the suggestion. I had to slightly modify the autoscaling_group_name property to the following:


autoscaling_group_name = module.eks.eks_managed_node_groups["default"].node_group_resources.autoscaling_groups[0].name

Difference is referencing node_group_resources instead of just resources. Otherwise terraform errored out with: This object does not have an attribute named "resources".

@bryantbiggs or @antonbabenko should this issue be re-opened?

We could but like I said above, there doesn't appear to be any path to a resolution within this module

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet