Fix allow several instance types

[patrickjahns]

what

• Allow to specify more than a single instance_types for eks node groups
• Possible to pass more than a single item in the list since the the introduction of capacity_type see upstream pr

why

• For utilising SPOT instances it is a good practice to specify more than a single instance type

references

• Related to Error when configured multiple instance types #8
• See also instanceTypes in current AWS eks api

[SweetOps]

/test all

[patrickjahns]

I'll take a look later at fixing the tests - seems to be that they expect the validation message to be in a certain format.
Anything else I should consider ?

[SweetOps]

Anything else I should consider ?

fix of validation message should be enough

[patrickjahns]

/test all

🤞 - hope this works

[Nuru]

@osterman @aknysh We need to discuss how to support this feature moving forward.

One problem is that supporting this feature requires AWS Terraform provider >= 3.20 and I do not know if we want to push that requirement on our customers. At a minimum, though, we need to make that change in versions.tf as part of this PR.

A bigger problem is that AWS is currently split on how they support this, which brings us a split in how we can support it. I would prefer to defer support until AWS resolves their split, but I can see where our customers would get upset about that.

According to the AWS documentation:

If you specify launchTemplate, then you can specify zero or one instance type in your launch template or you can specify 0-20 instance types for instanceTypes.

The problem for us with this module is that there are a set of features only available by specifying the launch template: See

terraform-aws-eks-node-group/main.tf

Line 9 in b4c6e02

features_require_launch_template = local.enabled ? length(var.resources_to_tag) > 0 || local.need_userdata || local.features_require_ami : false

This module automatically creates a launch template if the requested configuration requires it, which means that it is not obvious whether or not you can specify more than one instance type, and makes error messages confusing. (I think the test for whether or not we can have more than once instance type is more complex than allowed in a custom validation. If I am wrong about that, then I guess a custom validation with appropriate explanation would be they way to move forward.)

[patrickjahns]

@Nuru

One problem is that supporting this feature requires AWS Terraform provider >= 3.20 and I do not know if we want to push that requirement on our customers. At a minimum, though, we need to make that change in versions.tf as part of this PR.

The support for capacity_type was already added in #45 - so my assumption was, that this requirement for the provider version was already "approved"

Regarding:

If you specify launchTemplate, then you can specify zero or one instance type in your launch template or you can specify 0-20 instance types for instanceTypes.

The next sentences from the AWS documentation clarified it the launch template situation for me

If however, you specify an instance type in your launch template and specify any instanceTypes, the node group deployment will fail

So I understood this, as that you are limited to 0-1 instance-types when you specify it via the launch template - but if you specify the instance types via the instance_types in the aws_eks_node_group resource it should not be a problem.

For users of the module, it is probably good to raise awareness, that when they use a custom launch template they shall rather omit the instance_type in the launch template to be able to specify more than a single type for a node group

[patrickjahns]

Anything I can do here to move forward with this PR?

[mergify]

This pull request is now in conflict. Could you fix it @patrickjahns? 🙏

This Pull Request has been updated, so we're dismissing all reviews.

[Nuru]

The condition argument is an expression that must use the value of the variable to return true if the value is valid, or false if it is invalid.

Suggested change

	length(var.instance_types) > 20
	length(var.instance_types) >= 1 && length(var.instance_types) <= 20

[Nuru]

One remaining issue is that this module creates launch templates implicitly for the user to enable certain features. We now need to update the module so that the launch template does NOT specify instance types and ensure that the aws_eks_node_group ALWAYS specifies instance types.

[Nuru]

/test all