Stop supporting the simultaneous use of managed and self-managed nodegroups

[RothAndrew]

We have no evidence that such a configuration has ever been or will ever be needed, and it adds unnecessary complexity and work to support it.

[ntwkninja]

IMO, We test how we expect it to be used and support that workflow. It allows us to scope what / how we support things and makes it clear how we intend for things to be used (in the code).

We could test both simultaneously but I don't have any use cases of both being used for any real scenarios and, given the amount of time we've spent making both work, vote to decouple them in testing to avoid history repeating itself (i.e. more time spent).

[RothAndrew]

What I'm proposing goes further than testing. Right now we have separate inputs for self managed nodegroups and managed nodegroups. I'm proposing that we merge those into:

• one input called "nodegroups" that accepts configurations for either self managed or managed nodegroups
• a boolean input called "use_self_managed_nodegroups" that helps the module decide whether to apply the provided configuration as managed nodegroups or self-managed ones

I believe doing this will simplify the user experience and make our module more pleasant to use.

[RothAndrew]

Side-topic for later: We should figure out a better way to do the nodegroups configuration. Currently the user has to pass in a whole HCL object with all sorts of configuration in it. Lots of opportunity for making it simpler by opinionating how we believe the nodegroups should be configured and exposing individual inputs for what we want to be configurable. If there is a need for it we can offer a "backdoor" that lets the user fully configure the nodegroups if they need to by using conditionals

[RothAndrew]

cc @blancharda -- you use these modules. Do you believe there are any valid use cases where both managed and self-managed nodegroups would be used?

[RothAndrew]

What if we offered an "override"? That would allow an advanced user to customize things without making it more difficult for the majority of users.

The majority of users may use:

variable "nodegroup_type" {
  description = "The type of nodegroup to use. Valid values are 'self-managed' and 'managed'"
  type        = string
  default     = "managed"
  validation = {
    condition     = can(regex("^(self-managed|managed)$", var.nodegroup_type))
    error_message = "The nodegroup_type must be either 'self-managed' or 'managed'"
  }
}

variable "nodegroup_instance_type" {
  description = "The instance type to use for the nodegroup"
  type        = string
  default     = "m5.xlarge"
}

<Insert other variables for configuring the nodegroup here, like root volume size, AMI ID, etc>

and advanced users that need full control would use

variable "custom_override_self_managed_nodegroups" {
  description = "Self managed nodegroup configuration to pass directly to the upstream EKS module. If this is set the other non-override nodegroup variables will be ignored. This is intended to be used by advanced users only who want full control over their nodegroup configuration."
  type        = any
  default     = null
}

variable "custom_override_managed_nodegroups" {
  description = "Managed nodegroup configuration to pass directly to the upstream EKS module. If this is set the other non-override nodegroup variables will be ignored. This is intended to be used by advanced users only who want full control over their nodegroup configuration."
  type        = any
  default     = null
}

To ease the burden on testing, we would not do automated tests of the override variables, since the intention would be to directly pass them through to the upstream EKS module that we are using.

[blancharda]

I'm definitely on board with consolodating the configuration.
Would this be simplified by something close to your first example, but with a list of objects instead of assuming a set number?

variable "nodegroups" {
  description = "Nodegroups to provision."
  type = list(object({
    name   = string
    type     = string
    instance_types = list(string)
    
    ...
  }))
  
  validation = {
    condition     = can(regex("^(self-managed|managed)$", var.nodegroups[].type))
    error_message = "The nodegroup_type must be either 'self-managed' or 'managed'"
  }
}

(Maybe with defaults being a smallish managed node group)

[blancharda]

To be clear, I'm suggesting we only expose SOME settings (like those that you outlined above), so the user wouldn't be providing the full object like they are now.

[RothAndrew]

works for me. I think as long as we simplify it down, and provide a strongly typed object rather than type = any then it will be a big step in the right direction.

[zack-is-cool]

You'd have to keep it updated to support every type of input that the upstream module has, which is a lot. It also limits the type of logic you can do on certain things. Also, probably the bigger problem - If something is typed as a string vs a map/tuple terraform will yell at you if you try to do certain functions

[RothAndrew]

@zack-is-cool you have worked with this stuff quite a bit recently. Any feedback?

[zack-is-cool]

Remove it if no one is going to use it 🤷‍♂️