Update Security Group rules

[aknysh]

what

• Update Security Group rules

why

• This module can create an additional Security Group for the EKS cluster for backwards compatibility if you are updating this module to the latest version on existing clusters
• If the cluster was created using an older version of the module, EKS did not create a managed cluster Security Group at the time, and the the cluster Security Group was the additional Security Group
• This additional Security Group is returned from the expression one(aws_eks_cluster.default[*].vpc_config[0].cluster_security_group_id)
• When the module tries to create resource "aws_security_group_rule" "managed_ingress_cidr_blocks" to add the allowed ingress CIDR blocks, the following error is thrown

 Error: [WARN] A duplicate Security Group rule was found on (sg-xxxxxxxxx). This may be
│ a side effect of a now-fixed Terraform issue causing two security groups with
│ identical attributes but different source_security_group_ids to overwrite each
│ other in the state. See https://github.com/hashicorp/terraform/pull/2376 for more
│ information and instructions for recovery. Error: InvalidPermission.Duplicate: the specified rule "peer: 10.222.0.0/16, ALL, ALLOW" already exists
│ 	status code: 400, request id: 7065e36d-ffca-4540-8e43-ed75d94d752e
│
│   with module.eks_cluster.aws_security_group_rule.managed_ingress_cidr_blocks[0],
│   on .terraform/modules/eks_cluster/security-group.tf line 17, in resource "aws_security_group_rule" "managed_ingress_cidr_blocks":
│   17: resource "aws_security_group_rule" "managed_ingress_cidr_blocks" {

• This PR adds a variable managed_security_group_rules_enabled. For the very old clusters (which use the custom SG as the main cluster SG), set the variable to false to not add the SG rules to it (since the SG is the custom SG to which the module adds the same rules anyway)

[aknysh]

/terratest

[bridgecrew]

Bridgecrew has found errors in this PR ⬇️

[Nuru]

I think this is wrong and should not be merged.

This attempts to solve a problem for old EKS clusters, but does it in a way that potentially causes problems for new EKS clusters, by not modifying the default EKS cluster Security Group, which risks regressing on #80.

It is a good attempt, but I do not think there is a good way to modify this module to support old EKS clusters. I think, instead, people with old clusters, where the only Security Group for the cluster is the one this module created, should migrate by removing the security group from their Terraform state, and letting it behave as if it were created by EKS. Yes, it will leave the SG dangling when the cluster is deleted, but if deleting the cluster were an option then that is the better path forward anyway, so we can expect that the dangling Security Group will not be a problem, and can be manually deleted when the time comes.

[aknysh]

I think this is wrong and should not be merged.

This attempts to solve a problem for old EKS clusters, but does it in a way that potentially causes problems for new EKS clusters, by not modifying the default EKS cluster Security Group, which risks regressing on #80.

It is a good attempt, but I do not think there is a good way to modify this module to support old EKS clusters. I think, instead, people with old clusters, where the only Security Group for the cluster is the one this module created, should migrate by removing the security group from their Terraform state, and letting it behave as if it were created by EKS. Yes, it will leave the SG dangling when the cluster is deleted, but if deleting the cluster were an option then that is the better path forward anyway, so we can expect that the dangling Security Group will not be a problem, and can be manually deleted when the time comes.

I've updated the PR and added this variable

variable "managed_security_group_rules_enabled" {
  type        = bool
  description = "Flag to enable/disable the ingress and egress rules for the EKS managed Security Group"
  default     = true
}

with the default values set to true so it does not affect anything at all for both new clusters and old cluster.
But for the very old clusters (created when EKS did not create manages SG), setting managed_security_group_rules_enabled to false will allow to move forward and use the latest module version with the very old clusters.

[aknysh]

/terratest

[aknysh]

/terratest