Terraform-aws-eks: Newly refactored module

[js-timbirkett]

Hello 👋

Firstly, this module is great and has a lot to deal with as standing up an EKS cluster and nodes / workers is not a trivial task.

There have been issues regarding orchestrating the creation of the control plane and the workers / nodes [#519], issues questioning complexity or coupling within the module [#635], questions with regards to LC / LT worker_groups and whether they should be merged or simplified [#563], and issues around numeric indexes and modifying arrays in general.

I had a go at creating a new, refactored version of this module and my attempt is here:
https://github.com/devopsmakers/terraform-aws-eks

It breaks things out into sub-modules that can be used by both the parent module (to maintain the simple single module gets you a working cluster) and directly by the user of the module for more complex situations (Custom CNI).

It also allows a map of maps for worker groups and the code has been simplified a bit (reduction of the death by lookups in the current worker related code).

I made a few tough decisions that might stimulate some debate and possibly some outrage. These are highlighted in the repos README.

I'm not saying it's perfect (yet), but I think it would make a good base for a fairly major refactor of this module and I'd be glad to assist in any way I can. Feel free to use any ideas or the codebase itself to further improve this module.

Thanks!

[barryib]

Hello @js-timbirkett thanks for opening this discussion and for your work. It's a good start.

Can you please open a PR so we can iterate and give feedbacks ?

For the design decisions:

• To me, we can move worker groups in a separate submodule like we already did for managed node groups.
• As for dropping LC support, I don't have a strong opinion on this. I personally don't use LC at all, but I don't know if there are lot of people out there which still rely on it. See https://github.com/terraform-aws-modules/terraform-aws-eks/issues/635#issuecomment-571076487

@max-rocket-internet @dpiddockcmp @antonbabenko please advice (mostly for the LC removal)

[antonbabenko]

+1 for dropping LC. I think we all agreed in some previous issues that dropping support for LC is the way to go.

I wonder, are there any downsides of using LT at all comparing to LC?

[max-rocket-internet]

I wonder, are there any downsides of using LT at all comparing to LC?

No downsides to LT that I know of, LC is just older and has less features.

[max-rocket-internet]

I had a go at creating a new, refactored version of this module

Sounds great but we need a PRs to review and a path forward for current users.

I look forward to seeing some PRs 😃

[js-timbirkett]

Hey 👋

The refactored module is still very much in a beta state, when I've finished shaving the Terraform yak and double checking outputs, inputs etc... I'll open a big ole PR for further discussion and improvements.

It'll definitely be a major release and not something that's easily migrate-able to without a lot of Terraform state moving or importing (could probably write up a guide on that).

[chancez]

@js-timbirkett I'll be giving your worker group module a try. I'm vendoring it though since I need to change ignore_changes for the ASG to not ignore desired capacity since we're using TF, not cluster autoscaler for managing workers.

[js-timbirkett]

Oh nice @chancez - Thanks for the 🐛 fix!

I'll be doing some final editing of my version of the module early next week when I have some time off work. Then I'll open what will be a rather large PR against this module and see how it goes...

[barryib]

@js-timbirkett any update here ?

[barryib]

@js-timbirkett are you still working on this ?

[ptphuc]

I'm really looking for this changes could happen. Any updates for this @js-timbirkett ?. Many thx for your contribution so far.

[barryib]

I was wondering if we shouldn't wait for Terraform 0.13 with it's loop support for modules.

With that, we can split the submodule to deploy one worker group and let users loop over the submodule to create their worker groups.

@max-rocket-internet @dpiddockcmp @antonbabenko please advise.

[antonbabenko]

My 5 cents. I would probably wait for using 0.13 features in the core of the modules, because very few users will migrate to 0.13 right away, and for us maintainers it means we will need to have the same functionality for 0.12-users at the same time, too.

Submodule with count for 0.13 users seems like a good use case. +1 from me.

Best regards,
Anton.

23 лип. 2020 р. о 21:33 Thierno IB. BARRY [e-mail] пише:


I was wondering if we shouldn't wait for Terraform 0.13 with it's loop support for modules.

With that, we can split the submodule to deploy one worker group and let users loop over the submodule to create their worker groups.

@max-rocket-internet @dpiddockcmp @antonbabenko please advise.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[dpiddockcmp]

0.13 doesn't appear to require any massive breaking changes to upgrade. It looks like most 0.12 code should mostly run fine under it. Although I haven't experimented very heavily. I wonder if that will speed or slow adoption?

Either way, we should wait a good period of time before adding 0.13 only features that probably have bugs and edge cases. 6 months?

There's also the MNG submodule to consider. It already has the pattern of internal for_each. It would be good for submodules to be consistent in behaviour. So it would need migrating with breaking changes for users if we did go down the for_each module path.

My vote, right now, would be to stick with for_each inside the submodules if we want to implement these changes soon.

[max-rocket-internet]

wait for Terraform 0.13 with it's loop support for modules.

Sounds good to me 🚀

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.