fix: Windows platform worker group does not find an AMI

[brettcave]

Fixes #1410 - windows platform in workers_group_default results in a lookup for a windows node in the linux map. A windows AMI should be refereced to the windows map, and the default map should look for Linux.

PR o'clock

Description

Set a windows platform on the worker groups fails, unable to set an AMI. This is due to a windows lookup from a linux map.

Checklist

• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[barryib]

I think you're changing the default behavior of the module. The default platform is Linux. To deploy windows box, you have set platform=windows in the corresponding worker group right ?

[barryib]

I think I understand your issue now. The PR breaks workloads where uses changed the default plateform value.

Am I correct ?

[brettcave]

I think I understand your issue now. The PR breaks workloads where uses changed the default plateform value.

Am I correct ?

@barryib - yes. If defaults are used, or the worker group platform is set to "linux", it works. however, if this is overridden to "windows", then it fails, because a windows ami lookup never gives a result, due to looking for a windows AMI in a linux AMI map.

I tested locally: a linux deployment works, and overriding to windows results in:

aws_launch_configuration.workers[0]: Creation complete after 5s [id=REDACTED]

[brettcave]

@barryib - an example of where this is breaking is that Windows platform EKS deployments with spotinst are not possible - https://github.com/spotinst/terraform-spotinst-ocean-eks. We reference this module which has some version pinning in it (~> 16.0) and our EKS deployments are blocked because of this until this fix is in.

This is the value that is passed through to the module:

variable "workers_group_defaults" {
  type = map

  default = {
    platform             = "windows"
    root_volume_type     = "gp2"
  }
}

[barryib]

I think if we do this, we'll always lookup for windows and linux AMI if someone doesn’t explicitly set platform on worker nodes. Because both worker_has_linux_ami and worker_has_windows_ami will be true.

I don't know how to solve this without introducing cycle. Maybe it worth to add new variables to explicitly ask for lookup for plateform specific AMI. Something like worker_search_ami_linux and worker_search_ami_windows. Both will be set to true by default.

[brettcave]

@barryib - ah right.

What about changing the default return on the look from lookup(x, "platform", "windows") == "windows" to lookup(x,"platform",local.workers_group_defaults["platform"]) ?

Or instead of using the concat + lookup, maybe the worker_has_windows_ami could be simplified to worker_has_windows_ami = local.workers_group_defaults["platform"] == "windows"? I haven't looked through the module enough to understand the logic yet, i may be overlooking the module's ability to provide worker groups on multiple platforms - can you confirm if that's the case, or if it is mutually exclusive? (i.e. only eks is configured with only windows or linux worker groups)?

[barryib]

I tried that before, but I went into terraform cycle error.

What do you think about #1413 ? Can we close your PR ?

[brettcave]

Looking through how local.workers_group_defaults is used / referenced in other places, I think this approach might work. The update would then look like:

worker_has_linux_ami = local.workers_group_defaults["platform"] == "linux"
worker_has_windows_ami = local.workers_group_defaults["platform"] == "windows"

This is defaulted to "linux" by https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/local.tf#L75 and would only look up 1 or the other depending on default vs a var override being provided.

I'll look to start testing this approach, call out anything i'm overlooking here. If this is the case, it might not even be necessary to have this as a local here, it might be a bit simpler to just use this condition in the count = local.workers_group_defaults["platform"] == "windows" here: https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/data.tf#L34

[brettcave]

@barryib - ok - i see the cycle errors you mentioned. Sure, happy to close this 1 in favour of #1413

[barryib]

ok. I'll close this. Can you please review #1413 ?

[barryib]

Replaced by #1413

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