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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

FW Policy unordered lists for address groups #18134

Open
lahughes35 opened this issue May 14, 2024 · 3 comments
Open

FW Policy unordered lists for address groups #18134

lahughes35 opened this issue May 14, 2024 · 3 comments

Comments

@lahughes35
Copy link

lahughes35 commented May 14, 2024

Community Note

  • Please vote on this issue by adding a 馃憤 reaction to the original issue to help the community and maintainers prioritize this request.
  • Please do not leave +1 or me too comments, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.
  • If an issue is assigned to a user, that user is claiming responsibility for the issue.
  • Customers working with a Google Technical Account Manager or Customer Engineer can ask them to reach out internally to expedite investigation and resolution of this issue.

Terraform Version & Provider Version(s)

Terraform >=v1.4.6
on Linux x86

  • provider registry.terraform.io/hashicorp/google >=v5.25.0

Affected Resource(s)

google_compute_network_firewall_policy_rule

Terraform Configuration

locals {
  rules_yaml = yamldecode(
    join("\n", [for f in fileset(var.factory_config.rules_dir, "**/*.yaml") : file("${var.factory_config.rules_dir}/${f}")])
  )
  all_rules = merge([
    for k, v in local.rules_yaml : {
      for policy in v.fwpol_list : "${k}-${policy}-${v.priority}" => {
        action                  = lookup(v, "action", "deny")
        name                    = k
        direction               = lookup(v, "direction", "INGRESS")
        priority                = v.priority
        description             = lookup(v, "description", null)
        fwpol_name              = policy
        disabled                = lookup(v, "disabled", false)
        enable_logging          = lookup(v, "enable_logging", true)
        target_service_accounts = lookup(v, "target_service_accounts", null)
        target_secure_tags = (lookup(v, "target_secure_tags", null) == null
          ? null
          : flatten([
            for t in v.target_secure_tags :
            try(local.tagmap[t], t)
          ])
        )
        match = {
          src_address_groups        = lookup(v.match, "src_address_groups", null)
          dest_address_groups       = lookup(v.match, "dest_address_groups", null)
          src_threat_intelligences  = lookup(v.match, "src_threat_intelligences", null)
          dest_threat_intelligences = lookup(v.match, "dest_threat_intelligences", null)
          dest_ip_ranges = (
            lookup(v.match, "dest_ip_ranges", null) == null
            ? null
            : flatten([
              for c in v.match.dest_ip_ranges :
              try(local.cidr_yaml[c], c)
            ])
          )
          src_ip_ranges = (
            lookup(v.match, "src_ip_ranges", null) == null
            ? null
            : flatten([
              for c in v.match.src_ip_ranges :
              try(local.cidr_yaml[c], c)
            ])
          )
          src_secure_tags = (
            lookup(v.match, "src_secure_tags", null) == null
            ? null
            : flatten([
              for t in v.match.src_secure_tags :
              local.tagmap[t]
            ])
          )
          layer4_configs = (
            lookup(v.match, "layer4_configs", null) == null
            ? [{ protocol = "all", ports = null }]
            : [
              for c in v.match.layer4_configs :
              merge({ protocol = "all", ports = null }, c)
            ]
          )
        }
      }
    }
  ]...)
}
ingress address groups https dev:
  priority: 5050
  fwpol_list:
    - "vpc_one_fw_policy"
    - "vpc_two_fw_policy"
  action: "allow"
  direction: "INGRESS"
  description: "Allow ingress on 443/4443 from address groups"
  match:
    src_address_groups:
      - "projects/{project}/locations/global/addressGroups/{address_group_1}"
      - "projects/{project}/locations/global/addressGroups/{address_group_2}"
      - "projects/{project}/locations/global/addressGroups/{address_group_3}"   
    dest_ip_ranges:
      - 192.168.0.2/24
      - 10.0.0.5/24
    layer4_configs:
      - protocol: tcp
        ports:
          - 443

Debug Output

No response

Expected Behavior

After adding a FW policy rule with source (or destination) address groups, the rule would not need to update when it hasn't been changed.

Actual Behavior

I'm sending a list of source and destination address groups (in different rules) and their order is switching after an apply so TF tries to "update" the rules every run.

Steps to reproduce

  1. Create yaml file with a rule config (example above)
  2. 'terraform apply'
  3. 'terraform plan' - should show no-op, will show update because of list reordering

Important Factoids

We don't see this behavior with lists of IPs in src_ip_ranges or dest_ip_ranges, just with the address groups.

References

No response

@lahughes35 lahughes35 added the bug label May 14, 2024
@ggtisc ggtisc self-assigned this May 21, 2024
@ggtisc
Copy link
Collaborator

ggtisc commented May 21, 2024

Hi @lahughes35

We need the google_compute_network_firewall_policy_rule code to replicate this issue and all other resources involved like:

  1. google_network_security_address_group
  2. google_compute_network_firewall_policy
  3. google_compute_network
  4. google_tags_tag_key
  5. google_tags_tag_value

As you can see in this link

Finally when you talk about the source, destination or address groups are you referring to the google_network_security_address_group?

@lahughes35
Copy link
Author

Hello @ggtisc

All of the elements you listed are created before this stack runs, it only adds rules to existing google_compute_network_firewall_policys. I will include the resource for google_compute_network_firewall_policy_rule below, it just iterates over the all_rules var created from reading in one or more yaml files like the example above. I did include the code to add things like threat intelligences and secure tags because those are used for other rules in my configs but the only parts necessary to replicate the issue are the ones that map to the keys in the example yaml file.

Finally when you talk about the source, destination or address groups are you referring to the google_network_security_address_group?

Yes, I am referring to google_network_security_address_group when talking about source or destination address groups above.

resource "google_compute_network_firewall_policy_rule" "default" {
  for_each                = local.all_rules
  project                 = var.project_id
  firewall_policy         = "https://www.googleapis.com/compute/v1/projects/${var.project_id}/global/firewallPolicies/${each.value.fwpol_name}"
  action                  = each.value.action
  priority                = each.value.priority
  description             = each.value.description
  direction               = each.value.direction
  enable_logging          = each.value.enable_logging
  target_service_accounts = each.value.target_service_accounts
  match {
    src_address_groups        = each.value.match.src_address_groups
    dest_address_groups       = each.value.match.dest_address_groups
    src_threat_intelligences  = each.value.match.src_threat_intelligences
    dest_threat_intelligences = each.value.match.dest_threat_intelligences
    src_ip_ranges             = each.value.match.src_ip_ranges
    dest_ip_ranges            = each.value.match.dest_ip_ranges
    dynamic "src_secure_tags" {
      for_each = toset(coalesce(each.value.match.src_secure_tags, []))
      content {
        name = src_secure_tags.value
      }
    }
    dynamic "layer4_configs" {
      for_each = each.value.match.layer4_configs
      content {
        ip_protocol = layer4_configs.value.protocol
        ports       = layer4_configs.value.ports
      }
    }
  }
  dynamic "target_secure_tags" {
    for_each = toset(
      each.value.target_secure_tags == null ? [] : each.value.target_secure_tags
    )
    content {
      name = target_secure_tags.value
    }
  }
}

@ggtisc
Copy link
Collaborator

ggtisc commented May 29, 2024

There are 2 locals in your code that aren't declared that are necessary to replicate this issue

  1. tagmap
  2. cidr_yaml

But as you are saying that the important issue is to replicate the issue are the ones that map to the keys in the example yaml file maybe you can simplify this file to provide only the necessary to replicate this issue instead of share the complete configuration.

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

2 participants