Inconsistency between the default and specific ACL parameters

[acdha]

The default network ACL parameters are named default_network_acl_ingress and default_network_acl_egress and take lists of maps with the keys rule_no and action.

The public/private rules are named using _{in,out}bound_acl_rules and require the keys to be rule_number and rule_action, which complicates sharing common rulesets between them.

[github-actions]

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

[nathanblair]

Should this still be open? Its been a minute since I had tried the VPC module.

[rpadovani]

Yes, this is still valid, unfortunately :/

[bryantbiggs]

hi all, this should be addressed in the next version which is under development here https://github.com/clowdhaus/terraform-aws-vpc-v4

FYI - they are/were different because at the provider level they are different, but we can make changes to align within the module

[lorengordon]

@bryantbiggs Oh nice, I ran into this issue also! Would be very nice to align the parameters.

Side note: I'm also seeing some weirdness when updating NACL rules due to the use of count on the resources. Rules get destroyed and recreated, which sometimes causes race conditions. Would be addressed by using for_each with the rule number as the for_each key, I think.

Edit: Holy smokes, just looked at the v4 implementation, and for_each with the rule number as the key is exactly what you did! Lolololol. Awesome!

[github-actions]

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

[github-actions]

This issue was automatically closed because of stale in 10 days

[lorengordon]

C'mon stalebot, you can keep this open until the next major release, yeah?

[antonbabenko]

@lorengordon Stalebot respects bug, wip, or on-hold (we don't have such a label). Ref: https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/master/.github/workflows/stale-actions.yaml#L24-L25

[lorengordon]

Thanks @antonbabenko! It was meant playfully, I find myself talking to stalebot a lot (in general, not specifically for your projects).

[ghost]

@antonbabenko I wrote a quick lookup to handle both cases:

  dynamic "ingress" {
    for_each = var.default_network_acl_ingress
    content {
      action          = lookup(ingress.value, "action", ingress.value["rule_action"])
      cidr_block      = lookup(ingress.value, "cidr_block", null)
      from_port       = ingress.value.from_port
      icmp_code       = lookup(ingress.value, "icmp_code", null)
      icmp_type       = lookup(ingress.value, "icmp_type", null)
      ipv6_cidr_block = lookup(ingress.value, "ipv6_cidr_block", null)
      protocol        = ingress.value.protocol
      rule_no         = lookup(ingress.value, "rule_no", ingress.value["rule_number"])
      to_port         = ingress.value.to_port
    }
  }
  dynamic "egress" {
    for_each = var.default_network_acl_egress
    content {
      action          = lookup(egress.value, "action", egress.value["rule_action"])
      cidr_block      = lookup(egress.value, "cidr_block", null)
      from_port       = egress.value.from_port
      icmp_code       = lookup(egress.value, "icmp_code", null)
      icmp_type       = lookup(egress.value, "icmp_type", null)
      ipv6_cidr_block = lookup(egress.value, "ipv6_cidr_block", null)
      protocol        = egress.value.protocol
      rule_no         = lookup(egress.value, "rule_no", egress.value["rule_number"])
      to_port         = egress.value.to_port
    }
  }

The change is backwards-compatible and supports both the current (inconsistent) rule_no and action parameters, as well as the rule_number and rule_action parameters, which are consistent with other ACL specifications.

I've attached the patch.diff file with the changes. I'm happy to open a pull/merge request if you'd like?

patch.diff.txt