fix: Adding a 'legacy_names' var to support older v17 name schemes

[jarvismishler]

Description

Adding a true/false variable like legacy_names shown here allows users to deploy the v18+ module using the older v17 style naming conventions for resources.

Motivation and Context

One of the breaking changes introduced in v18 is a change to the naming_prefixes used by almost all resources. These new naming prefixes cannot be overridden so previous "<worker_name>" resources will be destroyed and replaced by "<worker_name>-node-group" resources or the like.

Adding a legacy_names var allows small conditionals that can choose the new v18-style prefixes or the older v17-style prefixes so the v17 resources aren't destroyed/replaced just for a non-functional name change.

Breaking Changes

No breaking changes. A legacy_names var should default to false so existing deployments would be unaffected.

How Has This Been Tested?

Tested during the process of updating our own v17 cluster to v18. The variable defaulting false means it will be ignored by any implementation that doesn't actively set it to true in order to use the v17 matching names.

[bryantbiggs]

we have provided support for var.prefix_separator which v18.x defaults to "-", but you can set to "" to match v17.x behavior and this prevents the control plane resources from re-creating. However, we are not offering support for prior node group naming scheme or backwards compatibility. You can remove the prior node groups from Terraform state (and therefore Terraform's control) and provision new node groups. Once the new node groups are provisioned, you an cordon and drain the old node groups and finally remove once the last pods are removed

[jarvismishler]

Thanks @bryantbiggs for taking the time with your comment. I'm already aware of the prefix_separator attribute though and that seems to only solve part of the upgrading problem.

Using the v18 defaults, TF plan shows many changes like this. The hyphen is an added change that forces a replacement, but so is the addition of "-cluster".

"staging2-eks" -> "staging2-eks-cluster-"

Setting an empty prefix_separator does remove the trailing hyphen, but still doesn't address the addition of "-cluster" so the name difference still forces a replacement.

Here are some excerpts from the upgrade plan (v17.24 -> v18.9) using prefix_separator = "", so you can see that the cluster itself is being replaced and not just the node group resources.

Cluster IAM Role name change forces replacement:

# module.eks.aws_iam_role.this[0] must be replaced
...
~ name_prefix           = "staging2-eks" -> "staging2-eks-cluster" # forces replacement

Cluster Security Group name change forces replacement:

# module.eks.aws_security_group.cluster[0] must be replaced
...
~ name_prefix            = "staging2-eks" -> "staging2-eks-cluster" # forces replacement

Replacing either the cluster security group or the cluster IAM role will force a replacement of the EKS cluster itself.

# module.eks.aws_eks_cluster.this[0] must be replaced 
...
~ role_arn                  = "arn:aws:iam::XXXXXXXXXXXX:role/staging2-eks20210422154717366200000004" -> (known after apply) # forces replacement
...
~ vpc_config {
    ~ cluster_security_group_id = "sg-0e46ecc973ce1b40f" -> (known after apply)
    ~ security_group_ids        = [
        - "sg-0ca6cf076f4d195de",
      ] -> (known after apply) # forces replacement

Adding a legacy_names attribute like the one in this PR will allow the older naming patterns ("NAME") to be upgraded without affecting any current deploys that use the new naming patterns ("NAME-cluster-"). It seems like a small consideration to make for those of us hoping to upgrade. Since resource names are a non-functional change, why not allow us to keep our existing names/resources so we aren't forced to rebuild our clusters?

[bryantbiggs]

have you tried setting:

prefix_separator                   = ""
iam_role_name                      = $CLUSTER_NAME
cluster_security_group_name        = $CLUSTER_NAME
cluster_security_group_description = "EKS cluster security group."

Ref: #1744 (comment)

[jarvismishler]

I tried that originally and just set those for another TF plan now. Originally I set those hoping to fix all of the resource renaming, but found node resources still being replaced (as you mentioned earlier). I did miss that these changes will keep the cluster resources (IAM role & security group) though, so thanks for point that out.

So the block above allows us to keep existing v17 clusters and we just need to do the node group swap you mentioned earlier.

Thanks for taking the time to clarify this!

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