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

Inconsistency between the default and specific ACL parameters #415

Open
acdha opened this issue Mar 18, 2020 · 11 comments
Open

Inconsistency between the default and specific ACL parameters #415

acdha opened this issue Mar 18, 2020 · 11 comments

Comments

@acdha
Copy link
Contributor

acdha commented Mar 18, 2020

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.

@acdha acdha changed the title Inconcistency between the default and specific ACL parameters Inconsistency between the default and specific ACL parameters Mar 18, 2020
@github-actions
Copy link

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 github-actions bot added the stale label Feb 11, 2022
@nathanblair
Copy link

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

@github-actions github-actions bot removed the stale label Feb 12, 2022
@rpadovani
Copy link

Yes, this is still valid, unfortunately :/

@bryantbiggs
Copy link
Member

bryantbiggs commented Mar 9, 2022

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
Copy link
Contributor

lorengordon commented Mar 10, 2022

@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
Copy link

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 github-actions bot added the stale label Apr 10, 2022
@github-actions
Copy link

This issue was automatically closed because of stale in 10 days

@lorengordon
Copy link
Contributor

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

@antonbabenko
Copy link
Member

@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
Copy link
Contributor

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

@ghost
Copy link

ghost commented Sep 26, 2023

@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

@bryantbiggs bryantbiggs added this to the v6.0 milestone Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants