fix: Add lifecycle to security_group_rule

[rafaljanicki]

Description

I've added a lifecycle rule to create a security group rule before destroying

Motivation and Context

This is related to an error The specified rule does not exist in this security group. which can be fixed by hashicorp/terraform-provider-aws#12420 (comment)

[github-actions]

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

[rafaljanicki]

Don't stale

[aidanmelen]

@rafaljanicki There is a subtle problem with this request. Your change will be successful on the initial apply and adding additional rules. However, you must consider a side effect of creating resources with count:

if an element was removed from the middle of the list, every instance after that element would see its value change, resulting in more remote object changes than intended.

lifecycle.create_before_destroy=true is incompatible with this count churn as it will try to create a duplicate rule as it tries to shift the count index of the resources. You will get this error:

Error: [WARN] A duplicate Security Group rule was found on (sg-xxxxxxxxxxxxxxx).

Please see #245 for more information.

---

I developed the aws-security-group-v2 module with for_each from the ground up to address challenges like these. Yet, this was an edge case that I did not plan for. Implementing create before destroy logic is one way to avoid service interruptions with security groups; however it is not a full proof strategy for a number of reasons. The cloudposse/aws-security-group module has a very verbose description of avoidance strategies and I have attempted to do the same in the 1.4.0 draft PR. I would be happy to make a PR here if @antonbabenko wants to drop support for terraform 0.11 and implement for_each. At a glance, I think we would need a for_each resource for every count resource.

[aidanmelen]

@rafaljanicki A workaround might be to create a second security group with this module and perform a blue/green migration i.e. first apply the blue, second the green security group with the new rule and third destroy blue security group/rules. I recognize that this would only work with resource(s) that support multiple security group attachments.

You may also be able to achieve the same effect with this module by using name_prefix which implements security group level lifecycle.create_before_destroy.

module "web_server_sg" {
  source  = "terraform-aws-modules/security-group/aws"

  name_prefix = join("-", [local.name, 5, "blue"])
  # Force a SG level CBD with a name change
  # name_prefix = join("-", [local.name, 5, "green"])
  description = "Security group for web-server with HTTP ports open within VPC"
  vpc_id      = "vpc-12345678"

  ingress_cidr_blocks = ["10.10.0.0/16"]
  # ingress_ipv6_cidr_blocks = ["2001:db8::/64"]
}

resource "aws_network_interface" "web_server_eni" {
  subnet_id       = data.aws_subnet.default.id
  private_ips     = [cidrhost(data.aws_subnet.default.cidr_block, 10)]
  security_groups = [module.web_server_sg.security_group_id]
}

This still does not work with computed inputs or security groups that need to preserve the ID (e.g. load balancers). For this, a suitable workaround is to perform a blue/green with module rules similar to the rules_only example.

[aidanmelen]

@rafaljanicki hashicorp/terraform-provider-aws#25173 (comment)

[github-actions]

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

[github-actions]

This PR was automatically closed because of stale in 10 days

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