Node groups submodule

[dpiddockcmp]

PR o'clock

Description

Move node_group resources to a submodule for code isolation. As generally discussed and bounced about in #635.

This also changes the node_groups variable to be a map of maps and associated resources use for_each instead of count. This was more complicated than expected, thanks Terraform. Idea originally came from a comment on a different PR: #644 (comment)

I think this is feature compatible with the current setup. Still some improvements to do.

Checklist

• Change added to CHANGELOG.md. All changes must be added and breaking changes and highlighted
• CI tests are passing
• README.md has been updated after any changes to variables and outputs. See /~https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

WIP

• Add node_groups defaults to make user life easier
• Clean up local.workers_group_defaults_defaults
• Defaults in the submodule? Only relevant if we want it to be easily usable standalone
• Look at issues raised in Multiple node groups break if they don't all specify same keys #633

Related

Fixes: #633

[max-rocket-internet]

Nice! I think we create a new release when this is done 🙂

[dpiddockcmp]

I think it's ready for others to test and review.

Solved the race condition with kubernetes_config_map with a little use of a null_data_source.

For people using node_groups from the master branch it would be a good idea to target apply the node_group submodule first before a full apply that will delete the original node_group resources.

terraform apply -target=module.eks.module.node_groups
terraform apply

[max-rocket-internet]

I tested it in my setup without MNGs, LGTM!

[max-rocket-internet]

Question: A user can still use the submodule completely separately right? Like to allow using mixed versions of the main and MNG module? Like using the basic example but add this:

module "mng1" {
  source                 = "git://github.com/terraform-aws-modules/terraform-aws-eks.git/modules/node_groups?ref=v999"
  create_eks             = true
  cluster_name           = module.eks.outputs.cluster_id
  cluster_version        = module.eks.outputs.cluster_version
  default_iam_role_arn   = module.eks.outputs.worker_iam_role_arn
  node_groups = {
    example = {
      desired_capacity = 1
      max_capacity     = 10
      min_capacity     = 1
      instance_type = "m5.large"
    }
  }
}

Second question: What do you think about forcing this approach? i.e. no direct connection between the core and mng module, i.e. no variables of variable.node_groups*.

[barryib]

Thanks @dpiddockcmp for your awesome work. I just have some question, otherwise, it looks good to me.

[dpiddockcmp]

Question: A user can still use the submodule completely separately right? Like to allow using mixed versions of the main and MNG module? Like using the basic example but add this:

They could but that might cause fun issues if the interface between the two is changed. My primary aim was to have the module as self contained code for readability rather than public use.

All variables need setting to something, including keys that "fall back" to workers_group_defaults. So you'd end up nearer to:

module "mng1" {
  source                 = "git://github.com/terraform-aws-modules/terraform-aws-eks.git/modules/node_groups?ref=v999"
  create_eks             = true
  cluster_name           = module.eks.cluster_id
  cluster_version        = module.eks.cluster_version
  default_iam_role_arn   = module.eks.worker_iam_role_arn
  tags                   = {}
  node_groups_defaults   = {}
  workers_group_defaults = {}

  node_groups = {
    example = {
      subnets          = module.vpc.private_subnets
      desired_capacity = 1
      max_capacity     = 10
      min_capacity     = 1
      instance_type    = "m5.large"
      key_name         = ""
    }
  }
}

All the node_groups keys could be put in node_groups_defaults if creating more than one group.

Second question: What do you think about forcing this approach? i.e. no direct connection between the core and mng module, i.e. no variables of variable.node_groups*.

It's an option. But we'd have to modify the root module for interaction with the aws-auth ConfigMap. Need to get any additional worker roles into it and document how to deal with the race condition when first creating the cluster. Possibly exposing an output variable from the kubernetes_config_map resource for dependency ordering? Or have a null_data_source that depends on aws-auth be the source for the cluster_name output? Potential for dependency cycle if users not careful.

[barryib]

@dpiddockcmp @max-rocket-internet LGTM.

[barryib]

@dpiddockcmp please rebase.

[max-rocket-internet]

OK are we good to merge?

[max-rocket-internet]

I'll merge this now, run some test and then create a new release.

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