Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use for_each instead count for network ACLs #1144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wiseelf
Copy link
Contributor

@wiseelf wiseelf commented Nov 28, 2024

Description

Use for_each instead of count for aws_network_acl_rule resources.

Motivation and Context

We frequently encounter situations where additional rules need to be added to public network ACLs to block specific IPs. Since blocking rules must precede the allow 0.0.0.0/0 rule, the allow rule gets recreated each time a blocking rule is added. To address this issue, I propose using for_each instead of count for creating ACL rules. This approach will ensure stability and prevent unnecessary rule recreation.
result of running

Terraform will perform the following actions:
  # aws_network_acl_rule.public_inbound[0] will be destroyed
  # (because resource does not use count)
  - resource "aws_network_acl_rule" "public_inbound" {
      - cidr_block      = "0.0.0.0/0" -> null
      - egress          = false -> null
      - from_port       = -1 -> null
      - id              = "nacl-3893824412" -> null
      - network_acl_id  = "acl-00a45f588af373b04" -> null
      - protocol        = "-1" -> null
      - rule_action     = "allow" -> null
      - rule_number     = 900 -> null
      - to_port         = -1 -> null
        # (1 unchanged attribute hidden)
    }
  # aws_network_acl_rule.public_inbound["100"] will be created
  + resource "aws_network_acl_rule" "public_inbound" {
      + cidr_block     = "0.0.0.0/0"
      + egress         = false
      + from_port      = -1
      + id             = (known after apply)
      + network_acl_id = "acl-00a45f588af373b04"
      + protocol       = "-1"
      + rule_action    = "allow"
      + rule_number    = 100
      + to_port        = -1
    }
  # aws_network_acl_rule.public_outbound[0] will be destroyed
  # (because resource does not use count)
  - resource "aws_network_acl_rule" "public_outbound" {
      - cidr_block      = "0.0.0.0/0" -> null
      - egress          = true -> null
      - from_port       = -1 -> null
      - id              = "nacl-2947690488" -> null
      - network_acl_id  = "acl-00a45f588af373b04" -> null
      - protocol        = "-1" -> null
      - rule_action     = "allow" -> null
      - rule_number     = 900 -> null
      - to_port         = -1 -> null
        # (1 unchanged attribute hidden)
    }
  # aws_network_acl_rule.public_outbound["100"] will be created
  + resource "aws_network_acl_rule" "public_outbound" {
      + cidr_block     = "0.0.0.0/0"
      + egress         = true
      + from_port      = -1
      + id             = (known after apply)
      + network_acl_id = "acl-00a45f588af373b04"
      + protocol       = "-1"
      + rule_action    = "allow"
      + rule_number    = 100
      + to_port        = -1
    }
Plan: 2 to add, 0 to change, 2 to destroy.

Breaking Changes

Yes and no. ACL rules will be recreated which can cause temporary networking issues. But it is always possible to move state like this(for default config):

terraform state mv 'aws_network_acl_rule.public_inbound[0]' 'aws_network_acl_rule.public_inbound["100"]'
terraform state mv 'aws_network_acl_rule.public_outbound[0]' 'aws_network_acl_rule.public_outbound["100"]'

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@wiseelf wiseelf changed the title improvement: use for_each instead count for network ACLs fix: use for_each instead count for network ACLs Nov 28, 2024
@wiseelf wiseelf changed the title fix: use for_each instead count for network ACLs fix: Use for_each instead count for network ACLs Nov 28, 2024
@wiseelf
Copy link
Contributor Author

wiseelf commented Dec 18, 2024

@bryantbiggs @antonbabenko please review, thank you.

@bryantbiggs
Copy link
Member

this is a breaking change and not one we can make lightly

@wiseelf
Copy link
Contributor Author

wiseelf commented Dec 20, 2024

@bryantbiggs how it works right now also introduces breaking changes on every ACL change, since it removes and creates all the rules in ACL, which temporary blocks traffic to VPC.
I also provided commands how to migrate current state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants