feat: Allow EKS cluster security group to be included in node group launch template

[davidaah]

Description

Allow including EKS cluster security group in module created launch templates

In order to effectively use this feature, additional cluster security groups may not be specified at cluster creation
which does not work right now. Fix is added for that (Fixes #1892) Rebased on top of #1934

Motivation and Context

The behavior of EKS's launch template support today is that if any node security groups are
specified in the launch template configuration, EKS will not automatically add the cluster security group.
If no security groups are specified, the cluster security group is added by default.
(ref: https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html)

For users that want to preserve similar behavior, when including additional node security groups,
optionally allow the cluster security group to be added to the launch template generated
by the EKS Cluster terraform module

Breaking Changes

• existing behavior preserved with default value (include_cluster_security_group = false)

How Has This Been Tested?

• I have tested and validated these changes using one or more of the provided examples/* projects

[davidaah]

note: open to suggestions on naming here. was trying to come up with something that wasn't overly verbose

[bryantbiggs]

I've opened #1934 which seems to be necessary and I've validated that it is required in some instances.

However, I don't follow the "node group" security groups. Is this for EKS managed node groups, self managed node groups, or both? Are you creating a launch template or no?

[bryantbiggs]

I just tried this in the examples and the node gets the EKS supplied security group EKS created security group applied to ENI that is attached to EKS Control Plane master nodes, as well as any managed workloads.

module "eks" {
  source = "../.."

  cluster_name    = local.name
  cluster_version = local.cluster_version

  vpc_id     = module.vpc.vpc_id
  subnet_ids = module.vpc.private_subnets

  enable_irsa = false

  # https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html#launch-template-security-groups
  create_cluster_security_group = false
  create_node_security_group    = false

  eks_managed_node_groups = {
    # Default node group - as provided by AWS EKS
    default_node_group = {
      # By default, the module creates a launch template to ensure tags are propagated to instances, etc.,
      # so we need to disable it to use the default template provided by the AWS EKS managed node group service
      create_launch_template = false
      launch_template_name   = ""

      create_security_group = false
    }
  }

  tags = local.tags
}

[davidaah]

@bryantbiggs thanks for the feedback... there are more or less three scenarios in EKS today:

• Case 1. create a nodegroup without any launch templates (not present in this module), and the cluster security group is automatically added to the nodes.
• Case 2. create a launch template and add a security group to it. The node that was subsequently created does not have the cluster security group, but it does have the security group.
• Case 3. create a launch template without a security group. The node that is subsequently created has the cluster security group attached to it.

I think the example you described is Case 1. The case i am trying to solve for is to add the ability to add the cluster security group to managed nodes when exercising Case 2. Basically EKS leaves this to the user managing the cluster to determine if they want to leverage the cluster security group if they explicitly specify their own SG (https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html#launch-template-security-groups)

I personally intend to use this with Managed Node Groups but it seemed to make sense to extend this optionality to Self-Managed Node groups as well for consistency.

Let me know if this makes sense/if you have any other questions

(re: bug fix, yes this is required. if #1934 gets merged before this, i will rebase on top of that)

[davidaah]

@bryantbiggs just wanted to follow up to see if you could take another look at this when you have some time? currently i'm blocked from using this module with a custom node SG and cluster SG

[bryantbiggs]

I'll be honest, I don't quite follow what you are trying to achieve here. either way, we'll have to update one of the examples to demonstrate and verify the changes so if you could start there then maybe that will shed some light on what you are trying to do

[davidaah]

Makes sense-- Let me take a pass at updating/adding an example to this PR to clarify the intended use case

[davidaah]

@bryantbiggs hopefully this helps. i think i found the right place to put in the examples... but let me also explain pre/post behavior a little more:

Pre my change (or with change, variable set to false):

• Example will vend managed node group with just aws_security_group.additional.id.
• In this scenario in order for the nodes to properly join the cluster this security group must contain all the rules needed to access the control plane along with a custom SG associated with the cluster OR you need to use some combination of cluster_security_group_additional_rules and node_security_group_additional_rules

Post change:

• Example will vend nodes with BOTH aws_security_group.additional.id AND the AWS created cluster SG attached.
• The reason we need this (as opposed to using the module to manipulate the SGs) is we currently manage these custom SGs with separate automation in our organization which necessitates that we do not modifying them in-line with Terraform. Alternatively, if EKS supported changing the extra cluster SGs after creation, we would just use our custom SG and attach it to the control plane.

let me know if that helps clarify/if you have any follow up questions

[bryantbiggs]

I've opened #1952 which should satisfy this - there are a few differences I've made so that we continue the patterns established in terms of the flexibility users have in how they managed their node groups

[davidaah]

thanks @bryantbiggs that change looks perfectly reasonable to me. much appreciated for helping to bring this over the line!

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