feat: Ability to manage worker groups as maps #858

grzegorzlisowski · 2020-05-03T20:54:14Z

PR o'clock

Description

Resolves #774

The change is intended to improve the ability to manage worker groups using maps. Which should allow to more flexibly add/remove worker groups (improve this: https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/faq.md#how-do-i-safely-remove-old-worker-groups).

The change includes suggested in #774 changes which include:

create separate submodule for worker_groups
the submodule includes only LaunchTemplate support

Change to the worker groups definitions:

to use new worker groups definitions use:
worker_groups = {}

Checklist

CI tests are passing
README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

barryib · 2020-05-04T15:06:32Z

Thanks @grzegorzlisowski for opening this PR. Actually, this is something we want to add into this module and totally drop loops with count in favor of for/for_each.

With that said, it would be nice to split the feature into a new submodule. We started some discussion at #774.

@js-timbirkett is maybe busy actually, so if you can open a PR to address #774 it would be very appreciated.

mbarrien · 2020-05-06T07:00:11Z

aws_auth.tf

@@ -38,11 +37,27 @@ locals {
    }
  ]

+  auth_launch_template_worker_roles_ext = [
+    for k, v in local.worker_group_launch_template_configurations_ext : {
+      worker_role_arn = "arn:aws:iam::${data.aws_caller_identity.current.account_id}:role/${var.manage_worker_iam_resources ? aws_iam_instance_profile.workers_launch_template_ext[k].role : data.aws_iam_instance_profile.custom_worker_group_launch_template_iam_instance_profile_ext[k].role_name}"


Need to replace the :aws: portion of the ARN with :${data.aws_partition.current.partition}: (same in auth_worker_roles_ext below)

mbarrien · 2020-05-06T15:24:49Z

data.tf

+  name = each.value["iam_instance_profile_name"]
+}
+
+data "aws_region" "current" {}


Doesn't seem to be used. Remove?

barryib · 2020-05-08T16:08:11Z

modules/worker_groups/main.tf

+
+  dynamic mixed_instances_policy {
+    iterator = item
+    for_each = ((lookup(var.worker_groups[each.key], "override_instance_types", null) != null) || (lookup(var.worker_groups[each.key], "on_demand_allocation_strategy", null) != null)) ? list(each.value) : []


does var.worker_groups[each.key] different from each.value ? If not, I'll prefer to use that. It'll more consistant for readability.

Unfortunately, it is different according to my analysis. This is only to avoid changing old logic. See here: https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/workers_launch_template.tf#L93

Sorry, responded too early.
This is what came as an argument from user: var.worker_groups_launch_template[count.index] while "each.value" represents what is already a merge with module defaults. Which IMHO could change original behaviour

barryib · 2020-05-08T16:08:11Z

modules/worker_groups/main.tf

+
+  dynamic launch_template {
+    iterator = item
+    for_each = ((lookup(var.worker_groups[each.key], "override_instance_types", null) != null) || (lookup(var.worker_groups[each.key], "on_demand_allocation_strategy", null) != null)) ? [] : list(each.value)


does var.worker_groups[each.key] different from each.value ?

This is the same as the previous case above

barryib · 2020-05-08T16:29:08Z

the old way of defining worker groups still possible for some backward compatibility and easier migration to the new version (after some time old way of defining worker_groups could be dropped entirely)

I was wondering if we shouldn't drop this directly. Because for existing worker group, users will have to move resources in the state. So why not move them directly into a map ?

For locals, shouldn't we move https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/local.tf#L34-L100 and into the submodule ?

grzegorzlisowski · 2020-05-08T17:36:24Z

My assumption was that users could apply the new module and add new worker groups using maps then migrate K8S resources to new nodes and then remove old groups simply by removing the list.

Those locals I have left in the original place as I assumed we still use them in "legacy" worker definitions. If we will just drop old worker_groups definitions then those should be moved.

stevehipwell · 2020-05-21T09:07:10Z

Do we know what is blocking this?

grzegorzlisowski · 2020-05-21T11:15:52Z

I presume review and approval? I'm not sure why this "Semantic Pull Request" check is not executing

venky999 · 2020-06-04T12:32:59Z

we can use worker_groups as a list of maps with unique key name.. instead of map so that it would be more cleaner and we can do ..

worker_groups = { for worker_group in var.worker_groups : worker_group.name => worker_group }

worker_group_configurations = {
    for k, v in local.worker_groups : k => merge(
      local.workers_group_defaults,
      v,
    ) if var.create_workers
  }
}

venky999 · 2020-06-04T13:01:33Z

modules/worker_groups/data.tf

+}
+
+data "aws_ami" "eks_worker" {
+  filter {


can we use for_each and add support for worker_node_version for each workers_group and the default value will be cluster_version if worker_node_version not specified

What do you mean by 'worker_node_version'?

grzegorzlisowski · 2020-06-23T21:43:53Z

we can use worker_groups as a list of maps with unique key name.. instead of map so that it would be more cleaner and we can do ..
worker_groups = { for worker_group in var.worker_groups : worker_group.name => worker_group }

worker_group_configurations = {
    for k, v in local.worker_groups : k => merge(
      local.workers_group_defaults,
      v,
    ) if var.create_workers
  }
}

It could be done but I presume it might make sense to keep the same approach as for node_groups where maps are used as an input.

ZeroDeth · 2020-07-04T11:49:58Z

Error sometimes appear randomly without changing anything in code.

Error: Inconsistent conditional result types

  on .terraform/modules/eks_0/modules/worker_groups/data.tf line 5, in data "aws_iam_instance_profile" "custom_worker_group_iam_instance_profile":
   5:   for_each = var.manage_worker_iam_resources ? {} : local.worker_group_configurations
    |----------------
    | local.worker_group_configurations is object with 3 attributes
    | var.manage_worker_iam_resources is false

The true and false result expressions must have consistent types. The given
expressions are object and object, respectively.

ZeroDeth · 2020-07-04T11:54:12Z

Error sometimes appear randomly without changing anything in code.

Error: Inconsistent conditional result types

  on .terraform/modules/eks_0/modules/worker_groups/data.tf line 5, in data "aws_iam_instance_profile" "custom_worker_group_iam_instance_profile":
   5:   for_each = var.manage_worker_iam_resources ? {} : local.worker_group_configurations
    |----------------
    | local.worker_group_configurations is object with 3 attributes
    | var.manage_worker_iam_resources is false

The true and false result expressions must have consistent types. The given
expressions are object and object, respectively.

Issues happen when adding a second group to worker_grpups

grzegorzlisowski · 2020-07-09T17:14:45Z

@ZeroDeth
Could you share your setup so I will try to recreate the issue?


          - Worker locals/defaults moved to workers submodule

- Create separate defaults for node groups - Workers IAM management left outside of module as both node_group and worker_groups uses them


          Move workers SG to root module due to tf cycle during resources termi…

…nation and improve logic when cluster is disabled

mbarrien reviewed May 6, 2020

View changes

grzegorzlisowski force-pushed the Worldremit:feature/devops-1421 branch 4 times, most recently from 9820bb7 to 695030c May 6, 2020

barryib reviewed May 8, 2020

View changes

barryib added the good first issue label May 11, 2020

grzegorzlisowski force-pushed the Worldremit:feature/devops-1421 branch from 695030c to d5582b6 May 14, 2020

venky999 reviewed Jun 4, 2020

View changes

grzegorzlisowski force-pushed the Worldremit:feature/devops-1421 branch 3 times, most recently from bf35ebe to 8d8bd48 Jun 24, 2020

grzegorzlisowski force-pushed the Worldremit:feature/devops-1421 branch 3 times, most recently from 1cbf970 to 87edcca Aug 3, 2020

grzegorzlisowski added 2 commits Mar 21, 2020

- Worker locals/defaults moved to workers submodule

5aa18ce

- Create separate defaults for node groups - Workers IAM management left outside of module as both node_group and worker_groups uses them

Move workers SG to root module due to tf cycle during resources termi…

Loading status checks…

da785fc

…nation and improve logic when cluster is disabled

grzegorzlisowski force-pushed the Worldremit:feature/devops-1421 branch from 87edcca to da785fc Sep 8, 2020

Sep	OCT	Nov
	02
2019	2020	2021

terraform-aws-modules / terraform-aws-eks

feat: Ability to manage worker groups as maps #858

feat: Ability to manage worker groups as maps #858

grzegorzlisowski commented May 3, 2020 •

edited

barryib commented May 4, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

barryib commented May 8, 2020

grzegorzlisowski commented May 8, 2020

stevehipwell commented May 21, 2020

grzegorzlisowski commented May 21, 2020 •

edited

venky999 commented Jun 4, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

grzegorzlisowski commented Jun 23, 2020

ZeroDeth commented Jul 4, 2020

ZeroDeth commented Jul 4, 2020

grzegorzlisowski commented Jul 9, 2020

terraform-aws-modules / terraform-aws-eks

Sponsor terraform-aws-modules/terraform-aws-eks

Join GitHub today

feat: Ability to manage worker groups as maps #858

feat: Ability to manage worker groups as maps #858

Conversation

grzegorzlisowski commented May 3, 2020 • edited

PR o'clock

Description

Checklist

barryib commented May 4, 2020 • edited

This comment has been minimized.

mbarrien May 6, 2020

This comment has been minimized.

grzegorzlisowski May 6, 2020 Author

This comment has been minimized.

mbarrien May 6, 2020

This comment has been minimized.

grzegorzlisowski May 6, 2020 Author

This comment has been minimized.

barryib May 8, 2020 Member

This comment has been minimized.

grzegorzlisowski May 8, 2020 Author

This comment has been minimized.

grzegorzlisowski May 8, 2020 • edited Author

This comment has been minimized.

barryib May 8, 2020 Member

This comment has been minimized.

grzegorzlisowski May 8, 2020 Author

barryib commented May 8, 2020

grzegorzlisowski commented May 8, 2020

stevehipwell commented May 21, 2020

grzegorzlisowski commented May 21, 2020 • edited

venky999 commented Jun 4, 2020 • edited

This comment has been minimized.

venky999 Jun 4, 2020 • edited

This comment has been minimized.

grzegorzlisowski Jun 23, 2020 Author

grzegorzlisowski commented Jun 23, 2020

ZeroDeth commented Jul 4, 2020

ZeroDeth commented Jul 4, 2020

grzegorzlisowski commented Jul 9, 2020

grzegorzlisowski commented May 3, 2020 •

edited

barryib commented May 4, 2020 •

edited

grzegorzlisowski May 6, 2020
Author

grzegorzlisowski May 6, 2020
Author

barryib May 8, 2020
Member

grzegorzlisowski May 8, 2020
Author

grzegorzlisowski May 8, 2020 •

edited

Author

barryib May 8, 2020
Member

grzegorzlisowski May 8, 2020
Author

grzegorzlisowski commented May 21, 2020 •

edited

venky999 commented Jun 4, 2020 •

edited

venky999 Jun 4, 2020 •

edited

grzegorzlisowski Jun 23, 2020
Author