fix: worker security group handling when worker_create_security_group=false

[sunghospark-calm]

PR o'clock

Description

Currently when you enable create_launch_template=true on the nodegroup, it switches from using primary cluster security group to worker security group which can cause various problems.

The first work around attempted was to use cluster_primary_security_group_id as worker_security_group_id, but that caused variable cycle error.

Error: Cycle: module.eks.local.cluster_primary_security_group_id (expand), module.eks.output.cluster_primary_security_group_id (expand), module.eks.var.worker_security_group_id (expand), module.eks.local.worker_security_group_id (expand), module.eks.aws_security_group_rule.cluster_https_worker_ingress, module.eks.aws_eks_cluster.this

The second workaround was to add the primary security group as worker_additional_security_group_ids, however that causes the ALB ingress controller to fail in AWS because the ingress controller cannot work when there are multiple SG attached to the network interface.

The solution was to delete the unused worker security group, however that causes the failure on launch template creation due to the empty security group id.

This PR cleans up the security groups for launch template if theres an empty string in the list.
There was also a cluster_private_access_sg_source rule which was trying to add the worker security group when the worker security group didn't exist, which is fixed in this PR.

Checklist

• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[sunghospark-calm]

Hello @barryib @max-rocket-internet , I'm hoping to get some reviews on this PR. Please let me know if I should be pinging different person or channel. Thank you for maintaining this project.

[yafanasiev]

Just hit this as well as #1447. Had to fork the base repo, and everything now works beautifully. Hope this gets merged soon.

[aidan-mundy]

I am having the same issue and this appears to be a direct fix for the problem, would appreciate a speedy merge on this small PR.

EDIT: Just want to confirm that my problems were solved by using the source fork for this PR as my source for the module.

[yafanasiev]

@antonbabenko @bryantbiggs please take a look at this PR if you can. We tested this in our fork of the module, and fix works just as expected.

[yafanasiev]

This is the only PR preventing us to go back from fork to official module. I can confirm that the fix is actually working and battle-tested. Please, review this 😢

[antonbabenko]

Hi @yafanasiev ! I will review it during this week and merge it. Sorry that it takes so long time.

[antonbabenko]

I couldn't make it to work on an existing cluster created with examples/launch_templates.

Error messages are like this:

╷
│ Error: Error updating Auto Scaling Group: ValidationError: You must use a valid fully-formed launch template. Value () for parameter groupId is invalid. The value cannot be empty
│       status code: 400, request id: 00236532-0ebb-412d-9f64-289d0ff947c2
│ 
│   with module.eks.aws_autoscaling_group.workers_launch_template[2],
│   on ../../workers_launch_template.tf line 3, in resource "aws_autoscaling_group" "workers_launch_template":
│    3: resource "aws_autoscaling_group" "workers_launch_template" {
│ 
╵

Could you please try to create a cluster using examples, then apply this PR (add worker_create_security_group = false and other required inputs), and run terraform plan/apply to confirm that it works as expected.

Please update the example, if necessary.

[sunghospark-calm]

Thanks for the review @antonbabenko .
I could confirm that just adding worker_create_security_group = false alone did not work with example/launch_templates
The required blob was

  worker_create_security_group = false
  worker_create_cluster_primary_security_group_rules = true
  worker_additional_security_group_ids = [module.eks.cluster_primary_security_group_id]

I think it makes sense because there gotta be a security group in place before the cluster to work.

I also tested with example/managed_node_groups. For this one I could just add worker_create_security_group = false and the apply ran successfully.

[antonbabenko]

Thanks, @sunghospark-calm !

v17.13.0 has been just released.

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