Worker group tags

[stefansedich]

PR o'clock

Description

Adding ability to configure worker group ASG tags which resolves #89

Checklist

• terraform fmt and terraform validate both work from the root and examples/eks_test_fixture directories (look in CI for an example)
• Tests for the changes have been added and passing (for bug fixes/features)
• Test results are pasted in this PR (in lieu of CI)
• I've added my change to CHANGELOG.md
• Any breaking changes are highlighted above

[stefansedich]

Looks like I broke the build, trying to understand why and will fix it up, looks like it does not like having no elements defined and I greatly misunderstood Terraform.

[stefansedich]

@max-rocket-internet looks like this may have been a fail, closing off until I can work something out, unless there are some TF hacks you know of many of the things I tried don't seem to work in 0.11 and may in 0.12, unless we had the users always defining the tags with empty arrays to match the worker group count which would not be ideal.

[stefansedich]

@max-rocket-internet ok that involved ALOT of hair pulling but the best I could think of was that by default there is always one item in the list, then we use the matching item at the worker group index if we can find it otherwise we fallback to the first item in the list, this is not ideal but I tried a million other things and this was the only way I could get it working, any thoughts if this is an acceptable hack?

[max-rocket-internet]

I get this error when running with defaults (not adding any extra tags):

Error: Error running plan: 1 error(s) occurred:

* module.max_test.aws_autoscaling_group.workers: 1 error(s) occurred:

* module.max_test.aws_autoscaling_group.workers: concat: unexpected type list in list of type map in:

${concat(
    list(
      map("key", "Name", "value", "${aws_eks_cluster.this.name}-${lookup(var.worker_groups[count.index], "name", count.index)}-eks_asg", "propagate_at_launch", true),
      map("key", "kubernetes.io/cluster/${aws_eks_cluster.this.name}", "value", "owned", "propagate_at_launch", true),
      map("key", "k8s.io/cluster-autoscaler/${lookup(var.worker_groups[count.index], "autoscaling_enabled", local.workers_group_defaults["autoscaling_enabled"]) == 1 ? "enabled" : "disabled"  }", "value", "true", "propagate_at_launch", false)
    ),
    local.asg_tags,
    list(var.worker_group_tags[length(var.worker_group_tags) > count.index ? count.index : "0"]))
  }

[stefansedich]

@max-rocket-internet are you just running the examples? I can successfully run a terraform plan for the examples which are using defaults.

Edit: OK I see it now :shakes-fist:, for some reason my examples run fine but spun up a blank TF file and can see the issue.

For some reason and I don't understand why the default value for the variable and an explicitly set one behave differently, if I remove the wrapping list() it works fine for defaults, but breaks when you set tags and vice versa.

[stefansedich]

Ok @max-rocket-internet turns out I ran into hashicorp/terraform#19033, this next idea works but changes things a little, instead of having an list of lists and relying on matching up to the index, I am using a map and using the worker groups name as the key.

Now it is also slightly hacky in that by default there exists an item "default", and if you do not define tags for all your worker groups the user will need to ensure they add the default item back, but it does also allow you to easily set the tags for one group and default the rest like so it is a little more flexible.

worker_group_tags = {
  "group_a" = [],
  "default" = []
}

My apologies with the brokenness, this PR has been a big learning experience for me as TF is relatively new to me and I probably jumped the gun a little bit, lesson learnt.

[max-rocket-internet]

OK great! I think this looks pretty tidy 🙂

Can you fix conflict and update the README using terraform-docs?

[stefansedich]

Thanks @max-rocket-internet should be good to go!

[max-rocket-internet]

Well done @stefansedich! Thanks for the effort 😃

[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.