feat!: Swap count for for_each on subnet IAM resources

[thatcoleyouknow]

As an engineer that works for an organization that adds new subnets on a regular basis, we find ourselves constantly having to review very lengthy Terraform plans that are full of google_compute_subnetwork_iam_member recreations. This is partly due to Terraform lists being unordered but this can now be solved for by replacing the count argument with for_each in each of those resources. This would be a one-time breaking change, but it would also make it the last time consumers have to deal subnet IAM grants getting recreated due to changes in the list of subnets passed to this module.

If you all have any questions, concerns, or suggestions, I'd be happy to talk through those. I am also an enterprise customer so I can open a feature request with our TAM to try and help prioritize this, if that would help you all.

Thank you!

[thatcoleyouknow]

FWIW this has been tested internally. If any issues arise once someone kicks off the checks, feel free to @ me.

[thatcoleyouknow]

Could someone tell me what failed in the terraform-google-project-factory-int-trigger check? I don't have permissions to view that.

[thatcoleyouknow]

@imrannayer - Forgive the ping but I see you are an active maintainer. Is there a preferable way to get the gcbrun command ran on this PR so I can try and work through any issues preventing this from being reviewed for merge?

[bharathkkb]

/gcbrun

[thatcoleyouknow]

Could someone tell me what failed in the terraform-google-project-factory-int-trigger check? I don't have permissions to view that.

Now that this job actually ran, could I get someone to share why this failed?

[bharathkkb]

Hi @thatcoleyouknow , it failed when trying to apply the example dynamic-shared-vpc-local. I suspect switching to for_each is causing some values to be unknown during plan. Could you try testing that example locally?

STDERR: Error: Invalid for_each argument

  on ../../../modules/core_project_factory/main.tf line 215, in resource "google_compute_subnetwork_iam_member" "service_account_role_to_vpc_subnets":
 215:   for_each = (var.grant_network_role && var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0 && var.create_project_sa) ? toset(var.shared_vpc_subnets) : toset([])
    ├────────────────
    │ var.create_project_sa is true
    │ var.enable_shared_vpc_service_project is true
    │ var.grant_network_role is true
    │ var.shared_vpc_subnets is list of string with 2 elements

The "for_each" set includes values derived from resource attributes that
cannot be determined until apply, and so Terraform cannot determine the full
set of keys that will identify the instances of this resource.

When working with unknown values in for_each, it's better to use a map value
where the keys are defined statically in your configuration and where only
the values contain apply-time results.

Alternatively, you could use the -target planning option to first apply only
the resources that the for_each value depends on, and then apply a second
time to fully converge.

Error: Invalid for_each argument

  on ../../../modules/core_project_factory/main.tf line 265, in resource "google_compute_subnetwork_iam_member" "apis_service_account_role_to_vpc_subnets":
 265:   for_each = (var.grant_network_role && var.enable_shared_vpc_service_project && length(var.shared_vpc_subnets) > 0) ? toset(var.shared_vpc_subnets) : toset([])
    ├────────────────
    │ var.enable_shared_vpc_service_project is true
    │ var.grant_network_role is true
    │ var.shared_vpc_subnets is list of string with 2 elements

The "for_each" set includes values derived from resource attributes that
cannot be determined until apply, and so Terraform cannot determine the full
set of keys that will identify the instances of this resource.

When working with unknown values in for_each, it's better to use a map value
where the keys are defined statically in your configuration and where only
the values contain apply-time results.

Alternatively, you could use the -target planning option to first apply only
the resources that the for_each value depends on, and then apply a second
time to fully converge.

Error: Invalid for_each argument

  on ../../../modules/shared_vpc_access/main.tf line 98, in resource "google_compute_subnetwork_iam_member" "service_shared_vpc_subnet_users":
  98:   for_each = var.grant_network_role ? toset(local.subnetwork_api) : toset([])
    ├────────────────
    │ local.subnetwork_api is list of string with 6 elements
    │ var.grant_network_role is true

The "for_each" set includes values derived from resource attributes that
cannot be determined until apply, and so Terraform cannot determine the full
set of keys that will identify the instances of this resource.

When working with unknown values in for_each, it's better to use a map value
where the keys are defined statically in your configuration and where only
the values contain apply-time results.

Alternatively, you could use the -target planning option to first apply only
the resources that the for_each value depends on, and then apply a second
time to fully converge.

Error: Invalid for_each argument

  on ../../../modules/shared_vpc_access/main.tf line 124, in resource "google_compute_subnetwork_iam_member" "cloudservices_shared_vpc_subnet_users":
 124:   for_each = local.gke_shared_vpc_enabled && var.enable_shared_vpc_service_project && var.grant_network_role ? toset(local.subnetwork_api) : toset([])
    ├────────────────
    │ local.gke_shared_vpc_enabled is true
    │ local.subnetwork_api is list of string with 6 elements
    │ var.enable_shared_vpc_service_project is true
    │ var.grant_network_role is true

The "for_each" set includes values derived from resource attributes that
cannot be determined until apply, and so Terraform cannot determine the full
set of keys that will identify the instances of this resource.

When working with unknown values in for_each, it's better to use a map value
where the keys are defined statically in your configuration and where only
the values contain apply-time results.

Alternatively, you could use the -target planning option to first apply only
the resources that the for_each value depends on, and then apply a second
time to fully converge.
---- End output of terraform apply -auto-approve -lock=true -lock-timeout=0s -input=false -no-color -parallelism=10 -refresh=true 

[thatcoleyouknow]

@bharathkkb - Per Google's recommendation, we use the Google's Terraform example foundation[1] to manage each layer our foundation, which also uses the terraform-google-network module to provision the VPC and subnets. The terraform-google-network module uses lists for all outputs, leaving me with 2 sensible options here:

1. (Preferred) Adjust the shared_vpc example in this repo to pass formatted self_links to the service project modules so the host project, network, subnets, and service projects that use those subnets can all be deployed in a single apply.
2. Submit a PR to the terraform-google-network module to output formatted self_links based on inputs to solve for the Terraform limitation that requires the self_link to be known before the subnet IAM resources can be planned before apply.

While the approach used in the example[2] may make sense for a simple test, it doesn't align with Google's own Terraform best practices[3]. In most real world scenarios, one would not want to manage all layers of a foundation in a single root module.

Would you be open to me also changing how the subnets are passed from module.vpc to the service project modules (module.service-project*)? I'd like to format the subnets passed to the service project modules so the calculated value isn't needed during the plan phase. That would look something like this:

Lines 17-20 and 95-107 of shared_vpc example[4]...

locals {
  subnets = {
    "subnet_01" = {
      name         = "${var.network_name}-subnet-01"
      region       = "us-west1"
      ip_cidr_range = "10.10.10.0/24"
    }
    "subnet_02" = {
      name         = "${var.network_name}-subnet-02"
      region       = "us-west1"
      ip_cidr_range = "10.10.20.0/24"
    }
  }
  subnet_self_links = [
    for _, subnet in local.subnets :
    format(
      "projects/%s/regions/%s/subnetworks/%s",
      module.host-project.project_id,
      subnet.region,
      subnet.name
    )
  ]
}

...

module "service-project" {
  source  = "terraform-google-modules/project-factory/google//modules/svpc_service_project"
  version = "~> 18.0"

  name              = var.service_project_name
  random_project_id = false

  org_id          = var.organization_id
  folder_id       = var.folder_id
  billing_account = var.billing_account

  shared_vpc         = module.host-project.project_id
  shared_vpc_subnets = module.vpc.subnets_self_links
  shared_vpc_subnets = local.subnet_self_links
...

[1] https://github.com/terraform-google-modules/terraform-example-foundation
[2] https://github.com/terraform-google-modules/terraform-google-project-factory/blob/main/examples/shared_vpc/main.tf
[3] https://cloud.google.com/docs/terraform/best-practices
[4] https://github.com/terraform-google-modules/terraform-google-project-factory/blob/main/examples/shared_vpc/main.tf#L95-L107

[bharathkkb]

Thanks for the options @thatcoleyouknow. In both cases when we change to for_each, the module no longer works with a computed attrib as input. I do prefer us to switch to for_each over count so I am leaning towards option 1 even if doesnt work for subset of usecases (which you rightly pointed out is not a commonly used pattern). @imrannayer @apeabody @q2w any thoughts?

[apeabody]

Thanks for the options @thatcoleyouknow. In both cases when we change to for_each, the module no longer works with a computed attrib as input. I do prefer us to switch to for_each over count so I am leaning towards option 1 even if doesnt work for subset of usecases (which you rightly pointed out is not a commonly used pattern). @imrannayer @apeabody @q2w any thoughts?

+1 On the preference for for_each, my current thought would also be option 1.

[apeabody]

/gemini review

[gemini-code-assist]

Code Review

This pull request is a great improvement to prevent the recreation of subnet IAM resources by switching from count to for_each. The changes are well-motivated and mostly correct.

I've found a critical issue in modules/shared_vpc_access/main.tf where the logic to extract the subnet self-link from the loop variable is incorrect, which would cause Terraform plans to fail.

Additionally, I've provided suggestions across all modified resources to simplify the code by leveraging the fact that the subnetwork argument can accept a full self-link. This removes the need for manual parsing of the region and subnetwork name, making the code more readable and robust.

Please review the comments for details and suggestions.

[gemini-code-assist]

There seems to be a bug introduced here. each.value is a string like "${api},${subnet}". The original code correctly did split(",", ...)[1] to extract the subnet self-link before parsing it. This seems to have been lost in the refactoring. You need to split each.value by comma first.

Additionally, you can simplify this logic. The subnetwork argument accepts a self-link. You can pass the subnet self-link directly and remove the region argument, as it will be inferred.

  subnetwork = split(",", each.value)[1]

[gemini-code-assist]

There seems to be a bug introduced here. each.value is a string like "${api},${subnet}". The original code correctly did split(",", ...)[1] to extract the subnet self-link before parsing it. This seems to have been lost in the refactoring. You need to split each.value by comma first.

Additionally, you can simplify this logic. The subnetwork argument accepts a self-link. You can pass the subnet self-link directly and remove the region argument, as it will be inferred.

  subnetwork = split(",", each.value)[1]

[gemini-code-assist]

The subnetwork argument of google_compute_subnetwork_iam_member can accept the subnetwork's self-link directly. Since var.shared_vpc_subnets contains the self-links, you can simplify the code by assigning each.value to subnetwork and removing the explicit region argument, as it can be inferred from the self-link. This makes the code cleaner and less brittle.

  subnetwork = each.value

[gemini-code-assist]

The subnetwork argument of google_compute_subnetwork_iam_member can accept the subnetwork's self-link directly. Since var.shared_vpc_subnets contains the self-links, you can simplify the code by assigning each.value to subnetwork and removing the explicit region argument, as it can be inferred from the self-link. This makes the code cleaner and less brittle.

  subnetwork = each.value

[gemini-code-assist]

The subnetwork argument of google_compute_subnetwork_iam_member can accept the subnetwork's self-link directly. Since var.shared_vpc_subnets contains the self-links, you can simplify the code by assigning each.value to subnetwork and removing the explicit region argument, as it can be inferred from the self-link. This makes the code cleaner and less brittle.

  subnetwork = each.value