feat: Drop random pets from Managed Node Groups

[barryib]

PR o'clock

Description

• Resolves EKS Node Group fails to recreate when using launch template, on minor template update #1152
• Resolves Remove modules/node_groups/random.tf completely - it only causes issues #1188
• Resolves Version 15.1.0 upgrade forces node group re-creation, causing production outages #1318
• Resolves Update a launch template of a managed node group without recreating that managed node group #1109

BREAKING CHANGES: We now decided to remove random_pet resources in Managed Node Groups (MNG). Those were used to recreate MNG if something changed and also simulate the new argument node_group_name_prefix. But they were causing a lot of issues. To upgrade the module without recreating your MNG, you will need to explicitly reuse their previous name and set them in your MNG name argument. Please see upgrade docs for more details.

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]

cc @ArchiFleKs @daroga0002 @stevehipwell could you review this please ?

[stevehipwell]

/lgtm

[ArchiFleKs]

LGTM, I think this will prevent a lot of issues :)

[daroga0002]

LGTM, the only thing which is missing me is some docs on how to upgrade the module to a newer version.

I think a solution can be removal from terraform state current node groups, creating new groups from new module versions, and legacy node groups from AWS console/cli.

Maybe next week I will find some time to find upgrade path as my workload is using module version 13.2.1

[stevehipwell]

@daroga0002 I think this is only a change if you've not been providing name for the groups? To solve this I expect that you just need to set the name key to the dynamic name.

[daroga0002]

@daroga0002 I think this is only a change if you've not been providing name for the groups? To solve this I expect that you just need to set the name key to the dynamic name.

after introducing this change I believe it will force all group to recreate (because node_group_name_prefix will force replacement), so this should be marked as Breaking change.

[stevehipwell]

@daroga0002 does the launch templates being re-created actually matter? I don't think that the ASGs will be re-created as we're using name there.

[daroga0002]

@stevehipwell we are recreating whole aws_eks_node_group as we moving from node_group_name to node_group_name_prefix.

But let me test it just to ensure

[barryib]

@stevehipwell we are recreating whole aws_eks_node_group as we moving from node_group_name to node_group_name_prefix.

But let me test it just to ensure

Yes, I think the node group will be recreated. Should we let users to define a specific name like before ?

Something like this:

  node_group_name_prefix = lookup(each.value, "name", null) == null ? lookup(each.value, "name_prefix", join("-", [var.cluster_name, each.key])) : null
  node_group_name        = lookup(each.value, "name", null)

[stevehipwell]

I missed the move to prefix over name, how come we're doing this?

Just saw your reply @barryib.

[stevehipwell]

@barryib I'd like to see the ability to specify a name directly.

[daroga0002]

But let me test it just to ensure

Just giving here test result:

Terraform will perform the following actions:

  # module.eks.module.node_groups.aws_eks_node_group.workers["example"] must be replaced
+/- resource "aws_eks_node_group" "workers" {
      ~ arn                    = "arn:aws:eks:us-west-2:*******:nodegroup/test-eks-u8D6IkXR/test-eks-u8D6IkXR-example-grand-kitten/12bcc295-5e10-ba79-5b8a-4d1b61bae030" -> (known after apply)
      ~ id                     = "test-eks-u8D6IkXR:test-eks-u8D6IkXR-example-grand-kitten" -> (known after apply)
      ~ node_group_name        = "test-eks-u8D6IkXR-example-grand-kitten" -> (known after apply)
      + node_group_name_prefix = "test-eks-u8D6IkXR-example-" # forces replacement
      ~ release_version        = "1.17.12-20210512" -> (known after apply)
      ~ resources              = [
          - {
              - autoscaling_groups              = [
                  - {
                      - name = "eks-12bcc295-5e10-ba79-5b8a-4d1b61bae030"
                    },
                ]
              - remote_access_security_group_id = ""
            },
        ] -> (known after apply)
      ~ status                 = "ACTIVE" -> (known after apply)
        tags                   = {
            "Environment" = "test"
            "ExtraTag"    = "example"
            "GithubOrg"   = "terraform-aws-modules"
            "GithubRepo"  = "terraform-aws-eks"
        }
      ~ version                = "1.17" -> (known after apply)
        # (9 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

  # module.eks.module.node_groups.random_pet.node_groups["example"] will be destroyed
  - resource "random_pet" "node_groups" {
      - id        = "grand-kitten" -> null
      - keepers   = {
          - "ami_type"                  = "AL2_x86_64"
          - "capacity_type"             = "SPOT"
          - "disk_size"                 = "50"
          - "iam_role_arn"              = "arn:aws:iam::*******:role/test-eks-u8D6IkXR20210519143621640500000009"
          - "instance_types"            = "m5.large"
          - "key_name"                  = ""
          - "node_group_name"           = "test-eks-u8D6IkXR-example"
          - "source_security_group_ids" = ""
          - "subnet_ids"                = "subnet-0f4e1d832b0b777b8|subnet-024a5e4004c12cf31|subnet-07223e6126567f61c"
        } -> null
      - length    = 2 -> null
      - separator = "-" -> null
    }

Plan: 1 to add, 0 to change, 2 to destroy.

[stevehipwell]

@daroga0002 I assume that's without the suggested changes in #1372 (comment)?

[daroga0002]

yes, this is original PR, those changes proposed by @barryib will solve this issue and give additional possibility to switch to prefixed names (which from my side is desired feature)

[jack1902]

is there a way to extend this so that a launch_template is always created? since #1348 resulted in two node groups not being able to communicate with one another because one used a launch-template defined as part of the module and one didn't (i know EKS created a copy under the hood)

[barryib]

@daroga0002 @ArchiFleKs @stevehipwell just updated my PR according to your reviews.

is there a way to extend this so that a launch_template is always created? since #1348 resulted in two node groups not being able to communicate with one another because one used a launch-template defined as part of the module and one didn't (i know EKS created a copy under the hood)

@jack1902 good point. We can't always create LT, because some users don't need it. But maybe we should create the LT if LT is needed and stop supporting external LT ?

[jack1902]

@barryib maybe, (the worker_sg piece really caught me off guard, since by default, the worker_sg is not attached to nodes created unless a launch template is defined)

Might be worth updating the description on the Security Group to reflect this caveat OR like you say, allow people to extend a pre-defined launch template? [Happy to move this to thread since the change in this PR is a decent one, to swap nodes out without having to go through the process of creating a new managed node group through the petname piece]

[stevehipwell]

/lgtm

[barryib]

@daroga0002 is this docs enough https://github.com/terraform-aws-modules/terraform-aws-eks/pull/1372/files#diff-2b35cda5da63a0dd7e1834e0063af64826711909facdb8a56fafece345fa24fe ?

[ArchiFleKs]

@barryib maybe, (the worker_sg piece really caught me off guard, since by default, the worker_sg is not attached to nodes created unless a launch template is defined)

Might be worth updating the description on the Security Group to reflect this caveat OR like you say, allow people to extend a pre-defined launch template? [Happy to move this to thread since the change in this PR is a decent one, to swap nodes out without having to go through the process of creating a new managed node group through the petname piece]

What about https://github.com/terraform-aws-modules/terraform-aws-eks#input_worker_create_cluster_primary_security_group_rules ? I personally always set it and this allow all the different type to worker to communicate together ?

[fractos]

Heh, turned out I was logged in as my test user for the above comment.

[barryib]

One thing I noted around this issue is that, in the previous version, if you had supplied a name for the node group then the create_before_destroy=true lifecycle rule meant the Terraform apply failed if you did something like changing an EC2 instance type. I just wanted to highlight this as I can see that this is potentially an issue here as well.

I'm don't use MNG, so don't test this intensively. So if changing something like ami_id will re-create the node group, then we should stick with the name_prefix to avoir node group name collision because of create_before_destroy.

Edit:
Just noticed that almost every change in node group will force it's re-creatiom. See https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/resource_aws_eks_node_group.go#L43 :)

[fractos]

Edit: Just noticed that almost every change in node group will force it's re-creatiom. See https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/resource_aws_eks_node_group.go#L43 :)

yep, exactly. I've had to vend this module and drop the lifecycle rule in my current project because of this.

[barryib]

What is the best upgrade path when you're using Managed Node Groups with Terrarform ?

[stevehipwell]

@barryib AMI ID is in the launch template and doesn't force node group re-creation but enough arguments that might be changed regularly do so I'm thinking that name_prefix with create_before_destroy=true would be a better fit. This would also be clearer when names are duplicated which is entirely possible (unlike map keys).

[stevehipwell]

@barryib any progress on this?

[barryib]

@barryib any progress on this?

@stevehipwell still working on it. Just wanted to test it more.

[barryib]

@ArchiFleKs @daroga0002 @stevehipwell a final review please.

[barryib]

Thanks everyone for your help on this.

[philicious]

for what its worth: I can report TF lets me upgrade from <=14.0.0 to 17.0, w/o having to recreate node-groups !
which is why we got stuck below 14 ... and I didnt yet dare to manually patch TF state around...gladly

so thanks for this change !

steps to reproduce, might help someone coming here:

• first pin to version = "<=16.2.0" and run a "plan", according to https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/docs/upgrades.md#upgrade-module-to-v1700-for-managed-node-groups
• as I was using a custom LT and in there set instance_type, I also had to set set_instance_types_on_lt = true on my node_group, so it wouldnt want to recreate, due to it being default set on the node-group instead of LT
• also add name = var.node_group_name and set to the name of existing node-group, as explained in the upgrade doc
• finally set version to 17.0 and plan should NOT want to recreate the node-groups anymore

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