Only one flag for two unrelated additional firewall rules

[fstr]

I'm currently using the beta-private-cluster module and I've installed Istio. I need to open the port 15017 from the GCP managed control-plane to my nodes and pods.

The module has the property firewall_inbound_ports with the default values ["8443", "9443", "15017"]. I made use of this property, but nothing changes. After reading the TF code, I figured out that I need to set add_cluster_firewall_rules to true, so my configured inbound ports would be opened.

The add_cluster_firewall_rules not only controls the firewall_inbound_ports, but also adds the resource google_compute_firewall.intra_egress, even if I don't want those firewall rules to be applied. They don't do any harm, but I have a shared networking project with many clusters attached and I'd like to keep the firewall rules to an absolute minimum.

Proposals:

• Quick win: Improve docs and add a note to firewall_inbound_ports that add_cluster_firewall_rules must be set
• Breaking change: Remove default values from firewall_inbound_ports. In my case I only need one of those ports, so I can overwrite the defaults. But I don't see a reason why a default setting should open more ports than necessary.
• Breaking change: Use separate flags to control the creation of google_compute_firewall.master_webhooks and google_compute_firewall.intra_egress
  • a) Separate flag could be introduced to control creation of webhooks
  • b) google_compute_firewall.master_webhooks could only be applied if length(var.firewall_inbound_ports) > 0, making an additional flag obsolete. If there are no ports to be opened, there's no need for the firewall rule.
    • If we keep the default values for firewall_inbound_ports this would always be applied, adding rules for users that haven't had them before.
    • If we also remove the default values for firewall_inbound_ports, the firewall rule would be removed for users that relied on the default values

[morgante]

They don't do any harm, but I have a shared networking project with many clusters attached and I'd like to keep the firewall rules to an absolute minimum.

If that's the case, might you want to manage all the rules outside the cluster anyways? It seems like in either case you'll end up with repeated rules for each cluster.

Quick win: Improve docs and add a note to firewall_inbound_ports that add_cluster_firewall_rules must be set

This makes sense.

Breaking change: Remove default values from firewall_inbound_ports. In my case I only need one of those ports, so I can overwrite the defaults. But I don't see a reason why a default setting should open more ports than necessary.

We try to have sensible defaults and these are the default ingress ports. You can always override but I don't think we will change the defaults (what would we even choose for a new default?).

Breaking change: Use separate flags to control the creation of google_compute_firewall.master_webhooks and google_compute_firewall.intra_egress

This seems reasonable, though I'd like to do it without a breaking change.

In particular, I suggest we add an add_master_webhook_firewall_rules variable which will always add the master_webhooks if set (while leaving the existing add_cluster_firewall_rules rule unchanged).

[fstr]

If that's the case, might you want to manage all the rules outside the cluster anyways? It seems like in either case you'll end up with repeated rules for each cluster.

I have a central place in my networking module to manage firewall rules, but GKE itself also creates firewall rules. I like the idea that all GKE related firewall rules are created inside my GKE terraform module (which simply wraps this module). This helps with consistency, since the rules need a lot of inputs/outputs from GKE.

It's true that I end up with repeated rules for each cluster anyway but that's acceptable. I don't want to have repeated google_compute_firewall.intra_egress rules though without having a need for them.

This makes sense.

I'll add that.

We try to have sensible defaults and these are the default ingress ports. You can always override but I don't think we will change the defaults (what would we even choose for a new default?).

Basically an empty list [] which would then make the additional flag obsolete. Firewall rules would only be applied if the list contains a value. See outlined Option b) above. If you want to provide sensible defaults, the current pattern works better.

This seems reasonable, though I'd like to do it without a breaking change.
In particular, I suggest we add an add_master_webhook_firewall_rules variable which will always add the master_webhooks if set (while leaving the existing add_cluster_firewall_rules rule unchanged).

I agree that a solution without a breaking change is favourable. I'm not sure if I understand your suggestion with the always correctly, but this might work:

variable "add_master_webhook_firewall_rules" {
  type     = bool
  default = false
}

resource "google_compute_firewall" "master_webhooks" {
  count       = var.add_cluster_firewall_rules || var.add_master_webhook_firewall_rules ? 1 : 0

[morgante]

Basically an empty list [] which would then make the additional flag obsolete. Firewall rules would only be applied if the list contains a value. See outlined Option b) above. If you want to provide sensible defaults, the current pattern works better.

Got it, yeah I'd prefer to provide sensible defaults.

I agree that a solution without a breaking change is favourable. I'm not sure if I understand your suggestion with the always correctly, but this might work:

Exactly, that's what I had in mind. A PR would be very welcome.