fix: Remove trailing hyphen from cluster security group and iam role name prefix by imdevin567 · Pull Request #1745 · terraform-aws-modules/terraform-aws-eks

imdevin567 · 2022-01-06T17:55:49Z

Description

A small naming change to prevent downstream recreation of resources.

Motivation and Context

Clusters created with previous versions of this module that relied on the generated cluster security group and/or cluster iam role are forced to be recreated when upgrading to version 18. The previous version used the cluster name as a prefix for both the SG and role. While you can override these now with cluster_security_group_name and iam_role_name, the module appends a hyphen to these values, which makes it impossible to set to the previous value.

Renaming a security group or an IAM role causes a recreation of the resource; changing the security group or IAM role for an EKS cluster forces a recreation of the cluster.

Removing this hyphen still allows the user to append one if they want to, but doesn't force it, making the upgrade smoother.

Breaking Changes

No breaking change

How Has This Been Tested?

I have tested and validated these changes using one or more of the provided examples/* projects

bryantbiggs · 2022-01-06T18:11:28Z

I will let others chime in, but this is the defacto standard that we have adopted across the modules and the breaking nature of the change is why we have introduced it in the v18.x breaking change

antonbabenko · 2022-01-06T18:23:53Z

Well, just a day has passed since we released 18.0. I think we can bump 18.1.0 with this PR merged. We just need to ask people to use the latest version.

Are there any other similar changes we should be doing?

imdevin567 · 2022-01-06T18:42:52Z

@antonbabenko Not a breaking change per se, but the default description of the security group was changed from: EKS cluster security group. to EKS cluster security group (period removed). This causes a recreation of the resource (which causes a downstream recreation of the cluster), but this was easily circumvented by just adding: cluster_security_group_description = "EKS cluster security group.".

In addition, the resource rename of aws_iam_role.cluster to aws_iam_role.this causes a recreation of the resource, but this can be worked around by moving the resource prior to applying:

tf state mv 'module.eks.aws_iam_role.cluster[0]' 'module.eks.aws_iam_role.this[0]'

IMO as long as there is a workaround to prevent cluster recreation it's fine. While I understand this is a breaking change by nature of the v18.x bump, I suspect most people will expect to be able to upgrade this module without a requirement of needing to tear down their existing cluster.

bryantbiggs · 2022-01-06T18:45:08Z

@imdevin567 users can elect to manage/create those roles and security groups outside of the EKS module and then pass them in - therefore, users still have the ability to avoid re-creation disruptions

jseiser · 2022-01-06T18:50:41Z

If you are defaulting to things that cause a cluster re-creation, their probably needs to be really good documentation on what is required to prevent that from happening.

imdevin567 · 2022-01-06T18:53:29Z

@bryantbiggs yes, but roles/security groups created by the module previously have dates appended to the names: i.e. develop-eks-cluster2022010514434579540000000b. The auto-generated names don't lend themselves well to being "migrated" to a managed resource.

As @jseiser said above, if we're going to leave this as the default then we should add more to the upgrade guide to prevent this from happening in the first place.

imdevin567 · 2022-01-06T18:59:36Z

If we want to leave the standard in place, would it be better to just add two variables?

iam_role_name_prefix
cluster_security_group_name_prefix

This would decouple the name from the name prefix, allowing users to override either.

bryantbiggs · 2022-01-06T19:07:15Z

@jseiser we have documentation, if there are pieces missing then PRs are welcome https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/UPGRADE-18.0.md

Again, a lot of the changes made in v18.x are intended to hopefully simplify and stabilize the module. This module has been notoriously complex leading to very difficult maintenance and troubleshooting (#635).

There are always ways around mitigating resource disruption depending on the level of user experience. The external role and security group creation that I am referring to @imdevin567 is not via imports but creating fresh new roles/security groups and then swapping those in to the EKS module. This does not result in any downtime or disruption and can potentially save you any additional headaches in the future

imdevin567 · 2022-01-06T19:12:01Z

@bryantbiggs correct me if I'm wrong, but you can't swap out a cluster role or security group without recreating the cluster. This was the original reason for the PR.

bryantbiggs · 2022-01-06T19:15:14Z

the security group you should be able to since its an additional security group. I can't say for certain on the IAM role swap as I have not attempted it myself

jseiser · 2022-01-06T19:29:09Z

@jseiser we have documentation, if there are pieces missing then PRs are welcome https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/UPGRADE-18.0.md

Considering I have yet to be able to follow your upgrade guide and end up with a working cluster, i do not think ill be much help adding to the documentation. So our enterprise is hard stuck on <18 for the time being.

bryantbiggs · 2022-01-06T19:36:40Z

@jseiser we have documentation, if there are pieces missing then PRs are welcome master/UPGRADE-18.0.md

Considering I have yet to be able to follow your upgrade guide and end up with a working cluster, i do not think ill be much help adding to the documentation. So our enterprise is hard stuck on <18 for the time being.

You or your enterprise can reach out and contribute a sponsorship if you would like, otherwise please feel free to enjoy the free OSS

antonbabenko · 2022-01-06T19:40:02Z

I think we should add a couple of new variables as per this comment by @imdevin567 - #1745 (comment)

So that people in the same situation as @imdevin567 can provide the values to prevent an unnecessary recreation of resources.

@bryantbiggs agree?

imdevin567 · 2022-01-06T19:42:10Z

I can make those changes to this PR pending a thumbs up from @bryantbiggs

I'll submit a separate PR with additions to the upgrade guide for steps I needed to work through to avoid cluster recreation.

jseiser · 2022-01-06T19:45:16Z

You or your enterprise can reach out and contribute a sponsorship if you would like, otherwise please feel free to enjoy the free OSS

You really think that is an appropriate response?

bryantbiggs · 2022-01-06T19:52:12Z

I think we should add a couple of new variables as per this comment by @imdevin567 - #1745 (comment)

So that people in the same situation as @imdevin567 can provide the values to prevent an unnecessary recreation of resources.

@bryantbiggs agree?

are we doing this for all occurrences of both IAM role name and security group name?

eks-managed-node-group sub-module
self-managed_node-group sub-module
fargate-profile sub-module
root module

antonbabenko · 2022-01-06T19:58:41Z

@bryantbiggs I am thinking only about places where it actually changed between v17 and v18.

bryantbiggs · 2022-01-06T20:05:14Z

if we are only looking at the cluster IAM role and security group then we could probably get away with a patch variable like prefix_separator that defaults to "-" which is the desired state but users can use "" which reflects the v17.x state. does this work? shouldn't make things messy and would be quite clear why its used. We'll also have to update:

"${local.cluster_sg_name}-" -> "${local.cluster_sg_name}${var.prefix_separator}"
"${local.iam_role_name}-" -> "${local.iam_role_name}${var.prefix_separator}"

And add a note in the upgrade doc about flipping this value to ""

imdevin567 · 2022-01-06T20:06:47Z

Works for me. I'll add the change.

…ws-eks

bryantbiggs · 2022-01-06T20:19:49Z


 All notable changes to this project will be documented in this file.

+### [18.0.1](https://github.com/terraform-aws-modules/terraform-aws-eks/compare/v18.0.0...v18.0.1) (2022-01-06)


we can leave out any changes here, the release workflow will automatically update this

My bad, needed to pull from upstream.

ah, I see - no worries!

bryantbiggs · 2022-01-06T20:22:01Z

      - The underlying autoscaling group and launch template have been updated to more closely match that of the [`terraform-aws-autoscaling`](https://github.com/terraform-aws-modules/terraform-aws-autoscaling) module and the features it offers
      - The previous iteration used a count over a list of node group definitions which was prone to disruptive updates; this is now replaced with a map/for_each to align with that of the EKS managed node group and Fargate profile behaviors/style
 - The user data configuration supported across the module has been completely revamped. A new `_user_data` internal sub-module has been created to consolidate all user data configuration in one location which provides better support for testability (via the [`examples/user_data`](https://github.com/terraform-aws-modules/terraform-aws-eks/tree/master/examples/user_data) example). The new sub-module supports nearly all possible combinations including the ability to allow users to provide their own user data template which will be rendered by the module. See the `examples/user_data` example project for the full plethora of example configuration possibilities and more details on the logic of the design can be found in the [`modules/_user_data`](https://github.com/terraform-aws-modules/terraform-aws-eks/tree/master/modules/_user_data_) directory.
+- The `aws_eks_cluster` resource has been removed and replaced with the `aws_eks_cluster_role_policy_attachment` resource. This change is to align with the [AWS EKS API changes](https://docs.aws.amazon.com/eks/latest/APIReference/API_CreateCluster.html) and [AWS EKS documentation](https://docs.aws.amazon.com/eks/latest/userguide/create-cluster.html)


is this line 31 change intentional?

hahaha nope, that would be Copilot chiming in. Don't hit tab too quickly.

bryantbiggs

@antonbabenko 👍🏽

thanks for your effort on this fix @imdevin567 🎉

### [18.0.3](v18.0.2...v18.0.3) (2022-01-06) ### Bug Fixes * Remove trailing hyphen from cluster security group and iam role name prefix ([#1745](#1745)) ([7089c71](7089c71))

antonbabenko · 2022-01-06T20:29:53Z

This PR is included in version 18.0.3 🎉

### [18.0.3](terraform-aws-modules/terraform-aws-eks@v18.0.2...v18.0.3) (2022-01-06) ### Bug Fixes * Remove trailing hyphen from cluster security group and iam role name prefix ([#1745](terraform-aws-modules/terraform-aws-eks#1745)) ([6722f30](terraform-aws-modules/terraform-aws-eks@6722f30))

github-actions · 2022-11-11T02:33:44Z

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.

imdevin567 force-pushed the master branch from 3e695db to 04678b9 Compare January 6, 2022 20:02

imdevin567 force-pushed the master branch from 04678b9 to 552795d Compare January 6, 2022 20:05

feat: Add prefix_separator variable to support legacy naming convention

f0c960a

imdevin567 force-pushed the master branch from 552795d to f0c960a Compare January 6, 2022 20:18

Merge branch 'master' of github.com:terraform-aws-modules/terraform-a…

765455a

…ws-eks

bryantbiggs reviewed Jan 6, 2022

View reviewed changes

Removing addition from Copilot.

d2a2161

bryantbiggs approved these changes Jan 6, 2022

View reviewed changes

antonbabenko merged commit 7089c71 into terraform-aws-modules:master Jan 6, 2022

bryantbiggs mentioned this pull request Jan 20, 2022

fix: Sub Module Prefix Separator #1792

Closed

1 task

github-actions Bot locked as resolved and limited conversation to collaborators Nov 11, 2022


		All notable changes to this project will be documented in this file.

		### [18.0.1](https://github.com/terraform-aws-modules/terraform-aws-eks/compare/v18.0.0...v18.0.1) (2022-01-06)

Uh oh!

Conversation

imdevin567 commented Jan 6, 2022

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

Uh oh!

bryantbiggs commented Jan 6, 2022

Uh oh!

antonbabenko commented Jan 6, 2022

Uh oh!

imdevin567 commented Jan 6, 2022

Uh oh!

bryantbiggs commented Jan 6, 2022

Uh oh!

jseiser commented Jan 6, 2022

Uh oh!

imdevin567 commented Jan 6, 2022

Uh oh!

imdevin567 commented Jan 6, 2022

Uh oh!

bryantbiggs commented Jan 6, 2022

Uh oh!

imdevin567 commented Jan 6, 2022

Uh oh!

bryantbiggs commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseiser commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryantbiggs commented Jan 6, 2022

Uh oh!

antonbabenko commented Jan 6, 2022

Uh oh!

imdevin567 commented Jan 6, 2022

Uh oh!

jseiser commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryantbiggs commented Jan 6, 2022

Uh oh!

antonbabenko commented Jan 6, 2022

Uh oh!

bryantbiggs commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imdevin567 commented Jan 6, 2022

Uh oh!

bryantbiggs Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

imdevin567 Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

bryantbiggs Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

bryantbiggs Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

imdevin567 Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

bryantbiggs left a comment

Choose a reason for hiding this comment

Uh oh!

antonbabenko commented Jan 6, 2022

Uh oh!

github-actions Bot commented Nov 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bryantbiggs commented Jan 6, 2022 •

edited

Loading

jseiser commented Jan 6, 2022 •

edited

Loading

jseiser commented Jan 6, 2022 •

edited

Loading

bryantbiggs commented Jan 6, 2022 •

edited

Loading