feat!: Removed support for launch configuration and replace `count` with `for_each` #1680

bryantbiggs · 2021-11-07T12:13:08Z

Description

Add support for cluster addons
Add support for cluster identity provider configuration
Create separate, standalone sub-modules for:
- User data
- EKS Managed Node Group
- Self Managed Node Group
- Fargate Profile
Drop support for launch configurations - only launch template will be supported going forward
Drop support for managing aws-auth configmap
- Drop requirement of requiring aws-iam-authenticator
- Drop requirement of Kubernetes terraform provider
Replace list/count usage with maps/for_each
Update user data usage to allow users to provide their own template in addition to the various settings and configurations provided for the module provided user data templates
Update to allow cohesive suite of settings that are uniform across the node pool types supported and continued support of multiple node group default settings under the root module
Update to allow more control over customizations/configurations of (not limited to):
- Security groups
- IAM roles
- Tags
- User data
- Launch template
More details provided under the UPGRADE-18.0.md document

Motivation and Context

Breaking Changes

Yes! Please see the UPGRADE-18.0.md document for details

How Has This Been Tested?

I have tested and validated these changes using one or more of the provided examples/* projects
- All examples under examples/ have been updated and validated

daroga0002 · 2021-11-07T16:37:22Z

@bryantbiggs I think this feature should be moved to wubmodule
similar to node groups

github-actions · 2021-11-07T19:02:36Z

Thank you for your contribution!

The CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts.
Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request.

bryantbiggs · 2021-12-07T00:37:58Z

@daroga0002 / @antonbabenko this is getting close for review if you have time to check it out. I still need to populate the upgrade document and comprehensive list of changes, plus a few other design decisions (i.e. - using the default blocks, security group usage, etc.). I've made some more changes today to align variable usage and descriptions so I need to run through all of the examples yet again tomorrow to re-validate but 🤞🏽 they are working as intended (if not, please let me know).

Let me know if you see any glaring/obvious errors - I'll be working on the upgrade doc and I would like to avoid having to make multiple iterations on it since I know its going to be quite involved.

cc - @barryib / @max-rocket-internet / @dpiddock / @lisfo4ka for any input you may have as well 🙏🏽

antonbabenko

I did the first round of reviews and it looks very good. Minor comments here and there.

examples/irsa_autoscale_refresh/charts.tf

examples/irsa_autoscale_refresh/main.tf

modules/_user_data/variables.tf

antonbabenko · 2021-12-07T14:19:22Z

I think it is a good idea to have an SVG diagram describing bootstrap logic - https://raw.githubusercontent.com/bryantbiggs/terraform-aws-eks/refactor/breaking-changes/.github/images/user_data.svg

bryantbiggs · 2021-12-07T20:12:27Z

I think it is a good idea to have an SVG diagram describing bootstrap logic - bryantbiggs/terraform-aws-eks@refactor/breaking-changes/.github/images/user_data.svg (raw)

yes, agreed! the images will be in the final PR but right now I have both a relative link and a fully qualified link. the relative link allows the image to be rendered in this PR just to make sure everything shows up as expected but it won't render on the remote Terraform docs for the module; this is what the fully qualified URL is for. so I was just putting a TODO reminder to remove the relative links before merging so that we have the fully qualified URLs that will show the images on both GitHub and on Terraform's module docs

bryantbiggs · 2021-12-08T21:09:48Z

ok this is finally ready for final review. I tried to work out some terraform state mv ... steps but in the end the module wants to replace nearly everything that is moved to the new name due to the extensive use of name_prefix (IAM roles, security groups, launch templates, autoscaling group, EKS node group, etc.). Therefore, I have left out any state mv recommendations from the upgrade guide. Let me know if there are any additional changes/feedback/final validation - @antonbabenko / @terraform-aws-modules/triage-supporters

daroga0002 · 2021-12-09T11:04:42Z

@bryantbiggs https://www.terraform.io/docs/language/modules/develop/refactoring.html this can be helpful for migration

Terraform 1.1 introducing this 🎉

antonbabenko · 2021-12-09T12:18:26Z

Are we raising minimum requirements from Terraform 0.13 to 1.1 already? :) It will be a surprise for many users, plus refactoring features released yesterday are not battle-tested yet. I think we can come back to this and start using something like this in half a year but not before.

daroga0002

few questions:

do we remove users/roles feature in aws-auth config? what will be desired way to make it?
aws-auth seems be now required to be done outside module what is causing that in case of self nodes it will always require handling this externally (so module can be threated as not sufficient in terms of creating cluster
EKS silently create in background SG which is assigned by default to node groups, can we also assign this by default to self nodes so then we will have unified approach and will dont require adding those SGs for DNS and etc.
what will happen with legacy module? should it be archived in seperate branch?

Beside some questions and concerns looks pretty well and solving a lot of issues 🎉

.pre-commit-config.yaml

examples/user_data/templates/windows_custom.tpl

examples/user_data/versions.tf

examples/irsa_autoscale_refresh/main.tf

martijnvdp · 2022-01-05T09:22:53Z

@bryantbiggs awesome work on writing all this! 👏 ❤️

I got excited seeing this, so I tried using this new version of the module! Unfortunately, I hit an issue. Chances are I am doing something wrong, but when I try to pass in dynamic resources to this new version of the module, I get an error: The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on. I opened a PR with a minimal example of failing code here: bryantbiggs#1

I think this may be due to Terraform's limitations for for_each? Or I am doing something wrong. Both are possible 😅

workaround is to precreate the iam roles

antonbabenko · 2022-01-05T11:51:20Z

examples/user_data/main.tf

+  bootstrap_extra_args = "-KubeletExtraArgs --node-labels=node.kubernetes.io/lifecycle=spot"
+
+  post_bootstrap_user_data = <<-EOT
+  [string]$Something = 'IStillDoNotKnowAnyPowerShell ¯\_(ツ)_/¯'


antonbabenko · 2022-01-05T11:53:21Z

versions.tf

-      version = ">= 2.0"
-    }
-    http = {
-      source  = "terraform-aws-modules/http"


Do you know if there is still a need for this http provider we forked some time ago?

No I don't believe we need this anymore - but might be best to just archive it in case others are actually using it even if the EKS module is not

antonbabenko · 2022-01-05T12:06:16Z

@bryantbiggs I am not sure why this PR didn't trigger a major release action, do you know?

https://github.com/terraform-aws-modules/terraform-aws-eks/runs/4714285326?check_suite_focus=true

antonbabenko · 2022-01-05T12:14:30Z

@bryantbiggs I renamed the PR title to include ! to make sure that this should be a breaking release but it didn't seem to help. Weird. https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with--to-draw-attention-to-breaking-change

mcanevet · 2022-01-05T13:23:11Z

@antonbabenko it's probably because none of the commits (not the PR title) contains a !.

antonbabenko · 2022-01-05T13:25:35Z

PR title was "feat!: ...", so it was used as a commit ee9f0c6, too.

mcanevet · 2022-01-05T13:43:38Z

oh yes, you're right

bryantbiggs · 2022-01-05T17:45:44Z

well thats really annoying! let me see what I can find out @antonbabenko

…th `for_each` (terraform-aws-modules#1680)

bryantbiggs · 2022-01-05T19:45:47Z

I've opened a PR to fix this here #1736 based on some testing I did today to dive into this issue. tl;dr - default of angular preset does not respect the ! and requires BREAKING CHANGE in the commit footer to trigger a breaking change (feat! is not valid either under this preset which is why nothing was released)

## [18.0.0](v17.24.0...v18.0.0) (2022-01-05) ### ⚠ BREAKING CHANGES * Removed support for launch configuration and replace `count` with `for_each` (#1680) ### Features * Removed support for launch configuration and replace `count` with `for_each` ([#1680](#1680)) ([ee9f0c6](ee9f0c6)) ### Bug Fixes * Update preset rule on semantic-release to use conventional commits ([#1736](#1736)) ([be86c0b](be86c0b))

antonbabenko · 2022-01-05T19:57:47Z

This PR is included in version 18.0.0 🎉

lodotek · 2022-05-02T19:27:32Z

I am trying to find out why docs/iam-permissions.md is missing from master. Because it is now missing from master, there is a broken link here: https://learn.hashicorp.com/tutorials/terraform/eks

bryantbiggs · 2022-05-02T19:38:23Z

its not missing, it was removed. The documentation provided is related to how the module is designed, how to configure the module, etc. If you wish to discover what permissions are required for your configuration, you can use IAM access analyzer, https://github.com/iann0036/iamlive, or other similar tools

## [18.0.0](terraform-aws-modules/terraform-aws-eks@v17.24.0...v18.0.0) (2022-01-05) ### ⚠ BREAKING CHANGES * Removed support for launch configuration and replace `count` with `for_each` (#1680) ### Features * Removed support for launch configuration and replace `count` with `for_each` ([#1680](terraform-aws-modules/terraform-aws-eks#1680)) ([17031bd](terraform-aws-modules/terraform-aws-eks@17031bd)) ### Bug Fixes * Update preset rule on semantic-release to use conventional commits ([#1736](terraform-aws-modules/terraform-aws-eks#1736)) ([e26a7d9](terraform-aws-modules/terraform-aws-eks@e26a7d9))

github-actions · 2022-11-09T02:37:32Z

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.

bryantbiggs mentioned this pull request Nov 7, 2021

Drop support for aws_launch_configuration #1679

Closed

bryantbiggs force-pushed the refactor/breaking-changes branch 11 times, most recently from a24474b to c54c054 Compare November 13, 2021 14:58

antonbabenko approved these changes Dec 7, 2021

View reviewed changes

examples/irsa_autoscale_refresh/charts.tf Outdated Show resolved Hide resolved

examples/irsa_autoscale_refresh/main.tf Outdated Show resolved Hide resolved

modules/_user_data/variables.tf Outdated Show resolved Hide resolved

bryantbiggs force-pushed the refactor/breaking-changes branch from 0179050 to e9d9476 Compare December 7, 2021 19:33

bryantbiggs force-pushed the refactor/breaking-changes branch 4 times, most recently from f6ac71b to 885bb6e Compare December 8, 2021 20:59

bryantbiggs marked this pull request as ready for review December 8, 2021 21:09

bryantbiggs added this to the v18.0.0 milestone Dec 8, 2021

bryantbiggs added the breaking change label Dec 8, 2021

daroga0002 reviewed Dec 9, 2021

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

examples/user_data/templates/windows_custom.tpl Outdated Show resolved Hide resolved

examples/user_data/versions.tf Show resolved Hide resolved

examples/irsa_autoscale_refresh/main.tf Show resolved Hide resolved

antonbabenko changed the title ~~feat: Removed support for launch configuration and replace count with for_each~~ feat!: Removed support for launch configuration and replace count with for_each Jan 5, 2022

antonbabenko reviewed Jan 5, 2022

View reviewed changes

antonbabenko approved these changes Jan 5, 2022

View reviewed changes

antonbabenko merged commit ee9f0c6 into terraform-aws-modules:master Jan 5, 2022

bryantbiggs added a commit to bryantbiggs/terraform-aws-eks that referenced this pull request Jan 5, 2022

feat: Removed support for launch configuration and replace count wi…

5c152bd

…th `for_each` (terraform-aws-modules#1680)

bryantbiggs deleted the refactor/breaking-changes branch January 5, 2022 19:38

stevehipwell mentioned this pull request Jan 6, 2022

Improved CHANGELOG for v18.0.0 #1741

Closed

bryantbiggs mentioned this pull request Jan 10, 2022

Release 18+ Upgrade Guide Breaks Existing Deployments #1744

Closed

antonbabenko mentioned this pull request Jan 19, 2022

feat: Add ability for tagging AWS-generated ASGs when using managed node groups #1705

Closed

TBBle mentioned this pull request Apr 10, 2022

[EKS] [request]: Managed Nodes scale to 0 aws/containers-roadmap#724

Closed

jrouly mentioned this pull request May 2, 2022

Root EBS volume encryption #2051

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Removed support for launch configuration and replace `count` with `for_each` #1680

feat!: Removed support for launch configuration and replace `count` with `for_each` #1680

bryantbiggs commented Nov 7, 2021 •

edited

Loading

daroga0002 commented Nov 7, 2021

github-actions bot commented Nov 7, 2021

bryantbiggs commented Dec 7, 2021

antonbabenko left a comment

antonbabenko commented Dec 7, 2021

bryantbiggs commented Dec 7, 2021

bryantbiggs commented Dec 8, 2021

daroga0002 commented Dec 9, 2021

antonbabenko commented Dec 9, 2021

daroga0002 left a comment

martijnvdp commented Jan 5, 2022

antonbabenko Jan 5, 2022

antonbabenko Jan 5, 2022

bryantbiggs Jan 5, 2022

antonbabenko commented Jan 5, 2022

antonbabenko commented Jan 5, 2022

mcanevet commented Jan 5, 2022 •

edited

Loading

antonbabenko commented Jan 5, 2022

mcanevet commented Jan 5, 2022

bryantbiggs commented Jan 5, 2022

bryantbiggs commented Jan 5, 2022

antonbabenko commented Jan 5, 2022

lodotek commented May 2, 2022 •

edited

Loading

bryantbiggs commented May 2, 2022

github-actions bot commented Nov 9, 2022

feat!: Removed support for launch configuration and replace count with for_each #1680

feat!: Removed support for launch configuration and replace count with for_each #1680

Conversation

bryantbiggs commented Nov 7, 2021 • edited Loading

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

daroga0002 commented Nov 7, 2021

github-actions bot commented Nov 7, 2021

bryantbiggs commented Dec 7, 2021

antonbabenko left a comment

Choose a reason for hiding this comment

antonbabenko commented Dec 7, 2021

bryantbiggs commented Dec 7, 2021

bryantbiggs commented Dec 8, 2021

daroga0002 commented Dec 9, 2021

antonbabenko commented Dec 9, 2021

daroga0002 left a comment

Choose a reason for hiding this comment

martijnvdp commented Jan 5, 2022

antonbabenko Jan 5, 2022

Choose a reason for hiding this comment

antonbabenko Jan 5, 2022

Choose a reason for hiding this comment

bryantbiggs Jan 5, 2022

Choose a reason for hiding this comment

antonbabenko commented Jan 5, 2022

antonbabenko commented Jan 5, 2022

mcanevet commented Jan 5, 2022 • edited Loading

antonbabenko commented Jan 5, 2022

mcanevet commented Jan 5, 2022

bryantbiggs commented Jan 5, 2022

bryantbiggs commented Jan 5, 2022

antonbabenko commented Jan 5, 2022

lodotek commented May 2, 2022 • edited Loading

bryantbiggs commented May 2, 2022

github-actions bot commented Nov 9, 2022

feat!: Removed support for launch configuration and replace `count` with `for_each` #1680

feat!: Removed support for launch configuration and replace `count` with `for_each` #1680

bryantbiggs commented Nov 7, 2021 •

edited

Loading

mcanevet commented Jan 5, 2022 •

edited

Loading

lodotek commented May 2, 2022 •

edited

Loading