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

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.

Copy link

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 github-actions bot added the stale label Jan 20, 2025
@wiseelf
Copy link
Contributor Author

wiseelf commented Jan 20, 2025

up

@github-actions github-actions bot removed the stale label Jan 21, 2025
Copy link

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 github-actions bot added the stale label Feb 20, 2025
Copy link

github-actions bot commented Mar 2, 2025

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Mar 2, 2025
Copy link

github-actions bot commented Apr 1, 2025

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants