Fix net-firewall-policy variable validation

[richard-olson]

Several new options were recently added for firewall policies but the variable validation checks weren't updated to allow these.

[juliocc]

While these validations make sense, I think we should let the provider handle them and not duplicate them here.

@sruffilli @ludoo what do you think?

[richard-olson]

While these validations make sense, I think we should let the provider handle them and not duplicate them here.

I agree, and it would've been less effort 😄 . I wasn't quite sure why it was there in the first place and I found similar validations in the DNS modules which i'd expect the provider to complain about, so wasn't sure of the approach being taken

[ludoo]

My reasoning for adding validation is

• string variables which are really enums, validation is needed to catch typos and document valid values
• mandatory top-level attributes of a complex type, where they define the "shape" of a resource, eg the tcp/http/http2 exc attributes in health checks
• obvious logical errors, or forbidden combinations which are not prevented by the type of the variable itself

All the rest I tend to delegate to the provider

[richard-olson]

My reasoning for adding validation is

If I understand correctly, your first dot point would apply to the allow/deny/goto variable which has validation and makes total sense to have, and I don't think this applies to match. For the second dot point I had a look through health_check_config and I see what you mean regarding "shape" and I don't think that one applies here either.

For the third dot point I've had a tinker and multiple match variables can coexist in a single rule. The provider handles the case where the minimum requirements for match aren't met:

Error creating NetworkFirewallPolicyRule: googleapi: Error 400: Invalid value for field 'resource.direction': 'INGRESS'. Must specify src_address_groups, src_fqdn, src_ip_ranges, src_region_code, src_secure_tags or src_threat_intelligence for ingress direction., invalid"

Error creating NetworkFirewallPolicyRule: googleapi: Error 400: Invalid value for field 'resource.direction': 'EGRESS'. Must specify dest_address_groups, dest_fqdn, dest_ip_ranges, dest_region_code or dest_threat_intelligence for egress direction., invalid"

Based on that @ludoo would you be happy to drop the validation checks for match?

[juliocc]

Based on that @ludoo would you be happy to drop the validation checks for match?

+1. I'm closing this then

@richard-olson thanks for the PR anyway :)

[richard-olson]

HI @juliocc unfortunately there are still the original validation rules in place which at the moment block the use of other matches vars. I was thinking we could still use this PR to do this for ingress and egress :

- validation {
-    condition = alltrue([
-      for k, v in var.egress_rules : v.match.destination_ranges != null
-    ])
-    error_message = "Engress rules need destination ranges."
-  }

[juliocc]

I was thinking we could still use this PR to do this for ingress and egress

Ah sorry about that. I see you already sent another PR, let's keep this one closed.