fix: Invalid index for google_container_cluster.primary.private_cluster_config[0]

[yuval2313]

Fixes #2353 :)

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[yuval2313]

In order to account for the creation of firewall rules which make use of the local.cluster_endpoint_for_nodes i have modified the logic for determining the value of this local variable according to this pseudo code:

IF var.enable_private_nodes == true:
  IF var.private_endpoint_subnetwork != null: RETURN data.google_compute_subnetwork.private_endpoint_subnetwork[0].ip_cidr_range
  ELSE IF var.master_ipv4_cidr_block != null: RETURN google_container_cluster.primary.private_cluster_config[0].master_ipv4_cidr_block
  ELSE: RETURN local.cluster_subnet_cidr
ELSE: RETURN local.cluster_subnet_cidr

It accounts for the following scenarios:

1. Create a GKE cluster with no private nodes
2. Create a GKE cluster with private nodes and master_ipv4_cidr_block set
3. Create a GKE cluster with private nodes and private_endpoint_subnetwork set
4. Create a GKE cluster with private nodes and no options

This is based on the documentation which states the order of precedence in order to define the control plane network - https://cloud.google.com/kubernetes-engine/docs/how-to/alias-ips#create_a_cluster_and_select_the_control_plane_ip_address_range.

Let me know please if this is overkill or if perhaps a better solution exists.

[apeabody]

cluster_endpoint_for_nodes is only used in firewall.tf.tmpl resources which are already gated var.add_cluster_firewall_rules, etc. So it might be simpler to remove this local (which is always evaluated, hence the issue), and instead evaluate directly in those (gated) resources?

[yuval2313]

Thanks for the feedback :)

Are cluster firewall rules only relevant when enabling private nodes? Because the module allows their creation even without the flag which would also necessitate the same logic.

Before 35.0.0 I noticed that the master_ipv4_cidr_block had a default string value which I guess prevented this since private clusters always had this option specified.

Now it seems that we may need this since there are many possibilities.

[yuval2313]

Firewalls rules can still be created with var.add_cluster_firewall_rules, var.add_master_webhook_firewall_rules, and var.add_shadow_firewall_rules, regardless of the different scenarios which i described in my first comment:

1. Create a GKE cluster with no private nodes
2. Create a GKE cluster with private nodes and master_ipv4_cidr_block set
3. Create a GKE cluster with private nodes and private_endpoint_subnetwork set
4. Create a GKE cluster with private nodes and no options

This is based on the documentation which states the order of precedence in order to define the control plane network - https://cloud.google.com/kubernetes-engine/docs/how-to/alias-ips#create_a_cluster_and_select_the_control_plane_ip_address_range.

Removing local.cluster_endpoint_for_nodes and evaluating directly inside the firewall resources wouldn't simplify the code as it still requires the same logic in order to determine the correct value for the master ip cidr range when firewall rules are created.

[apeabody]

Thanks @yuval2313!

An initial thought which might make this change simpler.

[yuval2313]

@apeabody
Thanks for the code review! Though I believe the current conditional logic is still necessary despite the local.cluster_endpoint_for_nodes being used only in firewall.tf.tmpl gated resources. Left a few replies to your review.

[apeabody]

/gcbrun

[apeabody]

@apeabody Thanks for the code review! Though I believe the current conditional logic is still necessary despite the local.cluster_endpoint_for_nodes being used only in firewall.tf.tmpl gated resources. Left a few replies to your review.

Thanks @yuval2313 - Right now the CI is failing on an un-related issue, once that is resolved I'll get this tested.

[apeabody]

/gcbrun

[apeabody]

/gcbrun

[apeabody]

From the CI test:

        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: projects/ci-gke-4f450dec-327e/regions/us-central1/subnetworks/safer-cluster-subnet not found
        	            	
        	            	  with module.example.module.gke.module.gke.data.google_compute_subnetwork.gke_subnetwork,
        	            	  on ../../../modules/beta-private-cluster/networks.tf line 19, in data "google_compute_subnetwork" "gke_subnetwork":
        	            	  19: data "google_compute_subnetwork" "gke_subnetwork" {
        	            	}
        	Test:       	TestSaferClusterIapBastion