Ability to add additional security groups to a node group

[nustiueudinastea]

what

• Adds the ability to assign additional security groups to a node group using a module variable.

why

• It's useful to be able to add extra security groups with custom user rules.
• The workers module also supports this feature (https://registry.terraform.io/modules/cloudposse/eks-workers/aws/latest)

[kwarunek]

+1. it would be great to have this.

[mergify]

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

[tirumerla]

Hi @nustiueudinastea thanks for your work on this. There are conflicting files can you please rebase and then run the following commands.

make init
make github/init
make readme

This feature is currently needed for us. It would be great if you can take a look at this.

[nustiueudinastea]

@kwarunek @tirumerla I updated the PR and the docs. Please give it a go in your local environment if you have a chance.

[tirumerla]

@kwarunek @tirumerla I updated the PR and the docs. Please give it a go in your local environment if you have a chance.

Sorry @nustiueudinastea just got back from my paternity. I added a comment and tested removing || length(var.additional_security_group_ids) works fine. Please remove this. Thanks again for your work on this enhancement.

[kwarunek]

@nustiueudinastea @tirumerla we're using this feature over a month without any issues - it just do the job for us. Seems to be ok in the production clusters as well as in the development ones (multiple recreations occur every week) - in summary dozens of node groups.
FYI our usage

module "eks_node_group" {                                                                                                  
  for_each = var.node_groups                                                                                               
                                                                                                                           
  source = "git::https://github.com/DreamLab/terraform-aws-eks-node-group.git"                                             
  # source       = "cloudposse/eks-node-group/aws"                                                                         
  name         = format("%s-%s", var.cluster_name, each.key)                                                               
  namespace    = each.value.namespace                                                                                      
  cluster_name = aws_eks_cluster.aws_eks.name                                                                              
                                                                                                                           
  ami_type       = var.ami_type                                                                                            
  disk_size      = lookup(each.value, "disk_size", var.disk_size)                                                          
  instance_types = lookup(each.value, "instance_types", var.instance_types)                                                
  capacity_type  = lookup(each.value, "capacity_type", var.capacity_type)                                                  
  subnet_ids     = var.subnets                                                                                             
  desired_size   = lookup(each.value, "desired_size", lookup(each.value, "min_size", 1))                                   
  max_size       = lookup(each.value, "max_size", 1)                                                                       
  min_size       = lookup(each.value, "min_size", 1)                                                                       
                                                                                                                           
  source_security_group_ids     = var.remote_access_security_groups                                                        
  additional_security_group_ids = var.additional_security_groups                                                           
  ec2_ssh_key                   = var.ssh_key                                                                              
                                                                                                                           
  # creates label for a autoscaler controller                                                                              
  cluster_autoscaler_enabled = true                                                                                        
  kubernetes_taints          = lookup(each.value, "taints", {})                                                            
  kubernetes_labels          = { "rasp.nodegroup/namespace" : each.value.namespace }                                       
                                                                                                                           
  tags = merge(                                                                                                            
    var.tags,                                                                                                              
    lookup(each.value, "tags", {})                                                                                         
  )                                                                                                                        
  resources_to_tag = ["instance", "volume"]                                                                                                                                                   
                                                                                                                           
  create_before_destroy = true                                                                                             
  depends_on            = [aws_eks_cluster.aws_eks]                                                                        
} 

[nustiueudinastea]

@tirumerla I removed that length operation. Indeed it seemed useless.

[tirumerla]

LGTM

[mergify]

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

[tirumerla]

@nustiueudinastea Could you please rebase it again so that the merge conflicts are fixed. @aknysh Can we run the tests and merge this once the rebase is done.

[Nuru]

Some changes requested, but for now do not rebase this PR, as that will pull in breaking changes that are scheduled to be reverted.

All changes to this module are temporarily on hold. We are working on updates to standardize how we we handle security groups and security group rules. This change will fit right in, but the code base will change significantly and this change might be lost if merged before then. Also, there is a decent chance we will simply incorporate this feature as part of the migration. However, if you want to use this by referencing the Git tag or commit, go ahead and make the requested changes.

[Nuru]

Not needed. get_cluster_date is true only if we need to invoke a data provider to get more information about the cluster.

Suggested change

	get_cluster_data = local.enabled ? (local.need_cluster_kubernetes_version || local.need_bootstrap || local.need_remote_access_sg || length(var.additional_security_group_ids) > 0) : false
	get_cluster_data = local.enabled ? (local.need_cluster_kubernetes_version || local.need_bootstrap || local.need_remote_access_sg) : false

[Nuru]

The ng object is just for data that is needed for the aws_eks_node_group resource. Since additional_security_group_ids only affects the launch template, this should not be here.

[Nuru]

We have a new naming convention. Please rename this associated_security_group_ids

Suggested change

	variable "additional_security_group_ids" {
	variable "associated_security_group_ids" {