hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.86k stars 9.2k forks source link

Support dynamic on aws_autoscaling_group_tag #20705

Open daenney opened 3 years ago

daenney commented 3 years ago

Community Note

Description

In 3.56 we gained the ability to set individual tags on ASGs without managing the ASG through #20674. This is helpful in cases where the ASG is implicitly created by AWS (such as EKS managed node groups) and terraform itself is thus unaware of the ASG.

However, aws_autoscaling_group_tag can only set 1 tag at a time. This is problematic for node groups, where we need to create one tag per label and taint that we pass to an EKS managed node group.

To that end, I want to be able to express it like this:

resource "aws_eks_node_group" "workers" {,
  for_each = local.node_pools  # a hypothetical map of { node_pool_name : {labels: <>, taints: <>, other_attr: <>},}
  [..]
  labels = each.value.labels

  dynamic "taint" {
    for_each = each.value.taints
    content {
      key    = taint.value.key
      value  = taint.value.value
      effect = taint.value.effect
    }
  }
}

resource "aws_autoscaling_group_tag" "labels" {
  for_each = local.node_pools

  autoscaling_group_name = aws_eks_node_group.workers[each.key].resources[0].autoscaling_groups[0].name

  dynamic "tag" {
    for_each = each.value.labels

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

Since each node pool might have a different set of label and taints, you can't spell them out ahead of time as one aws_autoscaling_group_tag each.

It's still possible to solve this by making the for_each a key that is the combination of each node pool name and each of its labels or taints, so that we loop enough times. But it results in harder to understand code than a dynamic block, which seems tailor made to solve this situation.

New or Affected Resource(s)

thpham commented 3 years ago

Hello, @daenney ,

You were talking about a temporary solution, could you help us by providing an example of the for_each "that is the combination of each node pool name and each of its labels or taints, so that we loop enough times" ?

Would be much appreciated, thank you.

daenney commented 3 years ago

Sorry for the lack of response, but here goes. We have a custom node_pools object that looks basically like this:

variable "node_pools" {
  description = "EKS managed node pools to create"
  type = map(object({
    disk_size      = number
    ami_type       = string
    instance_types = list(string)
    subnet_ids     = list(string)
    secgroup_ids   = list(string)
    labels         = map(string)
    taints         = list(object({ key = string, value = string, effect = string }))
    min            = number
    max            = number
    desired        = number
    version        = string
    spot           = bool
  }))
}

Then we build two variables, one with taints per pool and one with labels per pool:

locals {
  nodegroup_labels = { for obj in flatten([
    for name, attr in local.node_pools : [
      for label, value in attr.labels : { pool = name, key = label, value = value }
    ]
  ]) : format("%s/%s", obj.pool, obj.key) => obj }

  nodegroup_taints = { for obj in flatten([
    for name, attr in local.node_pools : [
      for taint in attr.taints : { pool = name, key = taint.key, value = taint.value, effect = taint.effect }
    ]
  ]) : format("%s/%s", obj.pool, obj.key) => obj }
}

And then turn that into tags:

resource "aws_autoscaling_group_tag" "labels" {
  for_each = local.nodegroup_labels

  autoscaling_group_name = aws_eks_node_group.workers[each.value.pool].resources[0].autoscaling_groups[0].name

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

resource "aws_autoscaling_group_tag" "taints" {
  for_each = local.nodegroup_taints

  autoscaling_group_name = aws_eks_node_group.workers[each.value.pool].resources[0].autoscaling_groups[0].name

  tag {
    key = "k8s.io/cluster-autoscaler/node-template/taint/${each.value.key}"
    # The cluster autoscaler expects a tag of <taint>:NoSchedule|NoExecute|PreferNoSchedule
    # https://github.com/kubernetes/autoscaler/blob/a49804544346b2e3690769f1482b0e7442ea457d/cluster-autoscaler/cloudprovider/aws/aws_manager.go#L451
    # but on our node pools the effect needs to be NO_EXECUTE, NO_SCHEDULE, PREFER_NO_SCHEDULE
    # so this abomination of replace, title and lower transforms them from the ENUM style to
    # their TitleCase variant
    value               = each.value.value != "" ? "${each.value.value}:${replace(title(replace(lower(each.value.effect), "_", " ")), " ", "")}" : replace(title(replace(lower(each.value.effect), "_", " ")), " ", "")
    propagate_at_launch = false
  }
}

It's not the best thing since sliced bread, but it seems to work well enough in practice. YMMV.

jaimehrubiks commented 2 years ago

Please implement this, not sure why it only accepts one... In my case the combination doesn't work great because it would mean using the resource state on a local variable, which can't always be predicted properly (in certain cases terraform will complain that I'm using a resource output ahead of its creation)

Edit Nevermind, I made it work... not sure why in this scenario it works for me.

And also, thanks for this part: replace(title(replace(lower(each.value.effect), "_", " ")), " ", "")

Very useful :)

github-actions[bot] commented 8 months ago

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

Sarga commented 8 months ago

remove stale