Remove toset(...) from for_each in dynamic blocks

[wiktorn]

Describe the bug

 find . -type f -name '*tf' | xargs grep -A1 dynamic | grep toset

• ./blueprints/third-party-solutions/openshift/tf/ilb.tf- for_each = toset(local.bootstrapping ? [""] : [])
• ./modules/compute-mig/autoscaler.tf- for_each = toset(
• ./modules/compute-mig/autoscaler.tf- for_each = toset(
• ./modules/compute-mig/autoscaler.tf- for_each = toset(
• ./modules/compute-mig/autoscaler.tf- for_each = toset(
• ./modules/net-lb-app-ext/urlmap.tf- for_each = toset(coalesce(m.value.path_rules, []))
• ./modules/net-lb-app-ext/urlmap.tf- for_each = toset(coalesce(route_rules.value.match_rules, []))
• ./modules/net-lb-app-ext/urlmap.tf- for_each = toset(coalesce(match_rules.value.headers, []))
• ./modules/net-lb-app-ext/urlmap.tf- for_each = toset(coalesce(match_rules.value.metadata_filters, []))
• ./modules/net-lb-app-ext/urlmap.tf- for_each = toset(coalesce(match_rules.value.query_params, []))
• ./modules/net-lb-app-ext/urlmap.tf- for_each = toset(coalesce(var.urlmap_config.test, []))
• ./modules/binauthz/main.tf- for_each = toset(coalesce(var.admission_whitelist_patterns, []))
• ./modules/net-lb-app-ext-regional/urlmap.tf- for_each = toset(coalesce(m.value.path_rules, []))
• ./modules/net-lb-app-ext-regional/urlmap.tf- for_each = toset(coalesce(route_rules.value.match_rules, []))
• ./modules/net-lb-app-ext-regional/urlmap.tf- for_each = toset(coalesce(match_rules.value.headers, []))
• ./modules/net-lb-app-ext-regional/urlmap.tf- for_each = toset(coalesce(match_rules.value.metadata_filters, []))
• ./modules/net-lb-app-ext-regional/urlmap.tf- for_each = toset(coalesce(match_rules.value.query_params, []))
• ./modules/net-lb-app-ext-regional/urlmap.tf- for_each = toset(coalesce(var.urlmap_config.test, []))
• ./modules/net-vlan-attachment/main.tf- for_each = var.router_config.bfd != null ? toset([var.router_config.bfd]) : []
• ./modules/vpc-sc/access-levels.tf- for_each = toset(each.value.conditions)
• ./modules/vpc-sc/access-levels.tf- for_each = toset(
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.to.operations)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.method_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.permission_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.from.access_levels)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.from.resources)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.to.operations)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.method_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.permission_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.to.operations)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.method_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.permission_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.from.access_levels)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.from.resources)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(policy.value.to.operations)
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.method_selectors, []))
• ./modules/vpc-sc/service-perimeters-regular.tf- for_each = toset(coalesce(o.value.permission_selectors, []))
• ./modules/billing-account/budgets.tf- for_each = toset(coalesce(each.value.sensitive_labels, []))
• ./modules/billing-account/budgets.tf- for_each = toset(each.value.threshold_rules)
• ./modules/net-lb-app-int-cross-region/urlmap.tf- for_each = toset(coalesce(m.value.path_rules, []))
• ./modules/net-lb-app-int-cross-region/urlmap.tf- for_each = toset(coalesce(route_rules.value.match_rules, []))
• ./modules/net-lb-app-int-cross-region/urlmap.tf- for_each = toset(coalesce(match_rules.value.headers, []))
• ./modules/net-lb-app-int-cross-region/urlmap.tf- for_each = toset(coalesce(match_rules.value.metadata_filters, []))
• ./modules/net-lb-app-int-cross-region/urlmap.tf- for_each = toset(coalesce(match_rules.value.query_params, []))
• ./modules/net-lb-app-int-cross-region/urlmap.tf- for_each = toset(coalesce(var.urlmap_config.test, []))
• ./modules/net-firewall-policy/net-global.tf- for_each = toset(coalesce(local.rules[each.key].match.source_tags, []))
• ./modules/net-firewall-policy/net-global.tf- for_each = toset(
• ./modules/net-firewall-policy/net-regional.tf- for_each = toset(coalesce(local.rules[each.key].match.source_tags, []))
• ./modules/net-firewall-policy/net-regional.tf- for_each = toset(
• ./modules/net-lb-app-int/urlmap.tf- for_each = toset(coalesce(m.value.path_rules, []))
• ./modules/net-lb-app-int/urlmap.tf- for_each = toset(coalesce(route_rules.value.match_rules, []))
• ./modules/net-lb-app-int/urlmap.tf- for_each = toset(coalesce(match_rules.value.headers, []))
• ./modules/net-lb-app-int/urlmap.tf- for_each = toset(coalesce(match_rules.value.metadata_filters, []))
• ./modules/net-lb-app-int/urlmap.tf- for_each = toset(coalesce(match_rules.value.query_params, []))
• ./modules/net-lb-app-int/urlmap.tf- for_each = toset(coalesce(var.urlmap_config.test, []))

[juliocc]

So I changed my mind. What if we fix these as needed? Or we fix only those where the order is important.

[wiktorn]

Fixing only those where order is important, means that you need to go through all the list above and understand, whether order is important or not. So IMO, you already done most of the effort. I'd say that at this moment, you may be as well as equipped with enough knowledge to remove toset(...) regardless of order requirements.

On the other hand, I hardly see a case, when you really want to reshuffle / remove duplicates in dynamic for_each...

So maybe, just fix as needed then?

[ludoo]

THere are a few weird cases, from memory BGP advertisements need to be ordered as the API reorders them, and a few others. I would leave these unchanged unless you have e2e tests for those.

[wiktorn]

THere are a few weird cases, from memory BGP advertisements need to be ordered as the API reorders them, and a few others. I would leave these unchanged unless you have e2e tests for those.

I think that relying on toset() to warrant any order is very fragile and may break when object structure changes, like you assumed that the order should be based on attribute b:

> toset([{b: 1}, {b: 3}, {b: 2} ])
toset([
  {
    "b" = 1
  },
  {
    "b" = 2
  },
  {
    "b" = 3
  },
])

If you add attribute a, it breaks ordering on b:

> toset([{a:3, b: 1}, {a: 1, b: 3}, {a: 2 , b: 2} ])
toset([
  {
    "a" = 1
    "b" = 3
  },
  {
    "a" = 2
    "b" = 2
  },
  {
    "a" = 3
    "b" = 1
  },
])

So in such cases, I would either of below:

• rely on the caller to provide proper order, and do not reorder by calling toset(...) at all
• do the sorting on explicit key by having a map with keys defining sort order, instead of toset(...)

We did implement first of the above in #2097.

Maybe the way forward for now, is just to put a TODO comment at each call-site, and then hope to fix as needed?

[ludoo]

I don't agree. Yours is a correct but abstract explanation, I have seen lots of customers with permadiffs which are for them incomprehensible, and often very hard to address given data might come from JSON, YAML, etc.

Again, let's please not blanket do this, and do it case by case only where we have e2e tests and we purposeuflly pass non-ordered data, to check that the API are not sending back the resource with ordered fields.

There are often very good reasons for the way modules do things which stem from actual experience, this might not be one of those but let's just tread carefully.

[juliocc]

I think the use of toset() in dynamic blocks is a cargo cult practice. IMO dynamic blocks inside modules should preserve the order specified by the user.

That being said, most of these have never caused any problems so let's fix them as they surface. And moving forward, let's flag them in code reviews as potential issues.

[ludoo]

Yep, agree with the "let's fix them as they surface". I'm sure BGP ranges get reshuffled by the APIs, and I have a vague recollection of a few others.

Closing this, as I guess we reached a consensus.