fix: Fixed launch template version infinite plan issue and improved rolling updates

[jaimehrubiks]

PR o'clock

Resolves #1446

Description

The recent PR (#1419) added support to set the launch template version on auto-generated launch templates for managed node groups, with the purpose of delaying rolling updates. However, it adds a bug caused by setting "$Latest" as the parameter to the node groups API call, which causes a never settled terraform plan, as the $Latest (or $Default) string is accepted by the API but translated into the corresponding integer afterwards. Read here: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_node_group#version

This PR fixes this issue and sets the corresponding integer directly. When the user specifies either "$Latest" or "$Default" on the "launch_template_version" variable, the module will now resolve the corresponding version number as an integer and use that value instead.

This PR also adds the variable "update_default_version" which will allow delaying the rolling update without knowing a specific version number.

How to delay a Rolling Update?

Using the default values:

launch_template_version: "$Latest"
update_default_version: true

We achieve: 1) Always use the latest changes on the launch template, 2) Push the $Default tag to the latest version

Whenever we need to delay the rolling update, we can simply set:

launch_template_version: "$Default"
update_default_version: false

And when we apply, only the launch template will be changed, but not the node groups, as they will reference to the old "default" which was the last time when update_default_version was set to true

When we revert those changes, the node groups will be updated.

Also, by setting:

    launch_template_version: "$Default"

In all cases, will achieve the same result and we only need to play with update_default_version variable

Note that you cant make it such that "new instances use the new launch template but we don't roll update the old one". You can, however, delay the rolling update as stated above, and try if the new version is working by manually going to the AWS cli and creating a new VM out of the launch template version desired (the latest number for example).

[jaimehrubiks]

Not sure what is the pre-commit check complaining about

why the check is asking me to change

id      = aws_launch_template.workers[each.key].id

with

id = aws_launch_template.workers[each.key].id

when that part is aligned with above resource

[ArchiFleKs]

Not sure what is the pre-commit check complaining about

why the check is asking me to change

id      = aws_launch_template.workers[each.key].id

with

id = aws_launch_template.workers[each.key].id

when that part is aligned with above resource

I'm pretty sure the CI error is due to this becasue teraform-docs file are extracted in the working folder and there is LICENSE file messing up the CI. I had the same issue on some of my repo. I'll make a PR if there is not one already

[jaimehrubiks]

Any way I can help with getting this merged @barryib ?

[llamahunter]

What is blocking this PR merge? The "$Latest" thing is causing unnecessary 'drift' notifications in our terraform plans. Unfortunately, the launch template is not exported from the module as an output, so I can't fetch its latestVersion property. Needs to be fixed in the module.

Or, I could hard code the current value of the launch template version, but worried it might miss updates.

[jaimehrubiks]

Looks like main maintainer is just away this week

Edit: this month*?

[Chili-Man]

Any updates on this? I'm more then happy to help in any way to get this merged

[Chili-Man]

I have tested and validated that this change works as expected. I can post the plan changes of with / without the fix to compare the difference if need be

[toneill818]

Any updates on this PR?

[Chili-Man]

@jaimehrubiks I'm more then happy to take over this change if you don't have time

[jaimehrubiks]

@Chili-Man this PR is fully functional. I am actually using it right now. We are only waiting for a maintainer of this repository to review, accept all the pending PRs, and make a release. @barryib was the one doing it for a very long time, but he is currently missing.

[Chili-Man]

@jaimehrubiks indeed it is functional, I'm using your changes as well; however, this PR is currently failing the Max TF pre-commit check , on the terraform fmt test: https://github.com/terraform-aws-modules/terraform-aws-eks/pull/1447/checks?check_run_id=2843241949#step:6:9 ; I thought it was because of that. I did not realize that @barryib was MIA. maybe one of the other members @bryantbiggs @brandonjbjelland or @antonbabenko can help us out?

[divomen]

I also faced this issue. It would be great if it was merged.

[yafanasiev]

Just hit this as well as #1461. Had to fork the base repo, and everything now works beautifully. Hope this gets merged soon.

[tnimni]

@Chili-Man the fmt fails because of line 51 in that file
after the : there is a line break and than another line break to close the ')', if you change the code to be all in one line i.e. making lines 51-53 a single line the fmt will work as expected. I hope it is clear if not please let me know

[toneill818]

It seems like this repository may need more maintainers, it has not had any new changes accepted for almost two months. @barryib @bryantbiggs @brandonjbjelland @antonbabenko Are you guys open to adding more maintainers to this project?

[bryantbiggs]

hi all, I haven't had time to test this but - does this introduce any breaking changes? does anyone have a plan/diff they could share for this change against master?

[jaimehrubiks]

No, there are no breaking changes. And there are no plan/diffs because the actual result of this PR is to prevent a diff from happening when no changes are made to the code.

[yafanasiev]

@bryantbiggs I've been using the latest version of module with this patch applied and had no issues with it. Infinite update is no longer present on no-changes plans, and managed node groups launch templates version updates are working as expected.

[bryantbiggs]

change seems small enough and seems to jive with what we've seen on ASG module - @antonbabenko / @barryib 👍🏼

[aidan-mundy]

This MR appears to be ready to go and has two approvals. Any particular reason we are waiting on it?

[ArchiFleKs]

I noticed that Pre-Commit / Max TF pre-commit (1.0.0) job fails. Do we have any ideas on how to fix it?

@andreyBar it seems it is the terraform-docs README, but there is also some fmt to do on the code.

I've made a very small PR here #1541 to only extract the binary

[bryantbiggs]

@jaimehrubiks once we merge #1541, could you rebase with master and resolve the conflicts please 🙏🏼

[antonbabenko]

@bryantbiggs I did the fix myself to release a bit quicker.

[antonbabenko]

v17.3.0 has been just released.

[jaimehrubiks]

Thanks @antonbabenko

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