net-firewall-policy module: Cannot mix "portful" and "portless" protocols

[bcorbitt-ps]

Describe the bug
When using factory to create network or hierarchical firewall policies, the YAML rule cannot contain a mix of protocols with and without ports. For example, including both TCP (with specific ports) and ICMP. Within the YAML, all protocols within any given rule must contain a ports specification, or none of them can. Mixing results in a failed merge (e.g. factory.tf line 65). However, specifying "any" for "portless" protocols like ICMP results in an API error.

Environment

Tested in terraform versions v1.6.2 and v1.3.6

output from `git rev-parse --short HEAD`: 
1ba8298b
(however, module reference being used is  "github.com/GoogleCloudPlatform/cloud-foundation-fabric.git?ref=v28.0.0//modules/net-firewall-policy"

To Reproduce
Example YAML (this is a replica of one of the "automatic" firewall rules created when a GKE cluster is created):

  priority: 10020
  action: allow
  description: Allow intra-cluster communication required by k8s networking model
  enable_logging: "true"
  target_service_accounts:
  - [email protected]
  match:
    source_ranges:
    - gke-nodes-range
    layer4_configs:
    - protocol: tcp
      ports:
      - 1-65535
    - protocol: udp
      ports:
      - 1-65535
    - protocol: icmp
      # ports:
      # - any

Expected behavior
Ideally, rules YAML could contain a mix of protocols with and without ports. But at a minimum, the ability to pass ports: -any but have a valid plan for the google_compute_firewall_policy_rule resource(s).

Result
Plan Failure when not including ports: :

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Inconsistent conditional result types
│
│   on .terraform/modules/network_fw_policy/modules/net-firewall-policy/factory.tf line 107, in locals:
│  105:           lookup(v.match, "layer4_configs", null) == null
│  106:           ? [{ protocol = "all", ports = null }]
│  107:           : [
│  108:             for c in v.match.layer4_configs :
│  109:             merge({ protocol = "all", ports = null }, c)
│  110:           ]
│     ├────────────────
│     │ v.match.layer4_configs is tuple with 3 elements
│
│ The false result value has the wrong type: element types must all match for conversion to list.

Apply failure when including ports:
Plan

              + ip_protocol = "tcp"
              + ports       = [
                  + "1-65535",
                ]
            }
          + layer4_configs {
              + ip_protocol = "udp"
              + ports       = [
                  + "1-65535",
                ]
            }
          + layer4_configs {
              + ip_protocol = "icmp"
              + ports       = [
                  + "any",
                ]
            }

Result:

 Error: Error creating NetworkFirewallPolicyRule: googleapi: Error 400: Invalid value for field 'resource.match.layer4Configs[2].ipProtocol': 'icmp'. Dest ports may only be specified on rules whose protocol is one of [TCP, UDP, SCTP]., invalid
│
│   with module.network_fw_policy["global-blakes-vpc"].google_compute_network_firewall_policy_rule.net-global["ingress/ingress-gke-drr-cluster-comms"],
│   on .terraform/modules/network_fw_policy/modules/net-firewall-policy/net-global.tf line 34, in resource "google_compute_network_firewall_policy_rule" "net-global":
│   34: resource "google_compute_network_firewall_policy_rule" "net-global" {

Additional context
My local solution (essentially via fork) was to add the following to main.tf:

locals {
... # existing code
 portless_protocols = ["icmp", "esp", "ah", "ipip"]
}

and change the dynamic "layer4_configs" sections in hierarchical.tf, net-global.tf, and net-regional.tf from:
ports = layer4_configs.value.ports
to
ports = (contains(local.portless_protocols, layer4_configs.value.protocol) ? null : layer4_configs.value.ports)

Not sure if this would meet standards required here, but this does result in a valid plan and functional apply. It still requires ports: be passed for "portless" protocols, but that is true presently.

[ludoo]

A different workaround is in the above PR, and I also added your example to the module's README so it's tested by our framework. Let me know if that fixes your issue, and thanks a lot for the detailed repro which made my life a lot easier.

[bcorbitt-ps]

Confirming that your fix works. Thanks for that very fast turn-around!