Can't declare empty lists of nested blocks in 0.12 SDK

[rileykarson]

Terraform Version

Running a Google provider acceptance test with 
github.com/hashicorp/terraform v0.12.0-alpha4.0.20190226230829-c2f653cf1a35 vendored.

master is in this state.

Terraform Configuration Files

resource "google_container_cluster" "with_private_cluster" {
	name = "cluster-test-%s"
	zone = "us-central1-a"
	initial_node_count = 1

	network = "${google_compute_network.container_network.name}"
	subnetwork = "${google_compute_subnetwork.container_subnetwork.name}"

	private_cluster_config {
		enable_private_endpoint = true
		enable_private_nodes = true
		master_ipv4_cidr_block = "10.42.0.0/28"
	}
	master_authorized_networks_config { cidr_blocks = [] }
	ip_allocation_policy {
		cluster_secondary_range_name  = "${google_compute_subnetwork.container_subnetwork.secondary_ip_range.0.range_name}"
		services_secondary_range_name = "${google_compute_subnetwork.container_subnetwork.secondary_ip_range.1.range_name}"
	}
}

Specifically

	master_authorized_networks_config { cidr_blocks = [] }

Expected Behavior

Terraform should see that an empty list was defined, and I should not see an error.

Actual Behavior

testing.go:568: Step 0 error: config is invalid: Unsupported argument: An argument named "cidr_blocks" is not expected here. Did you mean to define a block of type "cidr_blocks"?

Steps to Reproduce

make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withMasterAuthorizedNetworksConfig'

Additional Context

This is important because of how Optional + Computed interact. If I've defined list entries in my config and then remove them, Terraform will see no difference between my config and state; in effect, there's no way to remove every entry once any entry has been created.

[apparentlymart]

Hi @rileykarson! Thanks for creating an issue for this.

One of the assumptions we made in shimming the old provider schema model to the new config model was that if an attribute has Elem: &schema.Resource{...} then the intent is for it to be thought of by the user as being another object type that is nested within its container, using the block syntax.

We usually use singular names for these because we want users to think of it as a bunch of separate objects, rather than as a single collection. We do still need to gather them into a single collection to pass to the provider, but that was intended as part of the provider's internal model, not as part of the user-facing model.

The idea of using Optional+Computed for a nested block is, to be honest, not something that was intentionally allowed, but happened to work due to some shared code between the Elem: schema.Schema case and the Elem: schema.Resource case. Given how close to release we are, I'm looking for a solution here that allows us to be true to the intent of this particular attribute without making a change that could potentially effect the behavior of other resource types.

I notice that this particular cidr_blocks attribute has a plural name, which makes me suspect that your intent here was for users to think of it as being a single collection value, rather than a number of disconnected objects. It would be confusing to express a pair of cidr blocks using the usual block syntax:

cidr_blocks {
  cidr_block = "10.1.0.0/16"
}
cidr_blocks {
  cidr_block = "10.1.0.0/16"
}

It'd read better if it appeared as a single attribute accepting a list of objects instead, so I assume this is what was intended:

cidr_blocks = [
  {
    cidr_block = "10.1.0.0/16"
  },
  {
    cidr_block = "10.1.0.0/16"
  },
]

...which then has a more obvious reduction down into an empty list as in the example you gave, and makes more sense as syntax for a construct with a plural name.

With all of this said, a solution I'm considering here is a new field in schema.Schema that allows forcing the use of attribute syntax even when Elem is a schema.Resource, which we could then set for this cidr_blocks attribute and have it use the explicit list syntax above rather than the usual nested block syntax.

Does that seem reasonable? Are there other similar examples in the Google Cloud Platform provider that we could evaluate this solution against?

[rileykarson]

We usually use singular names for these because we want users to think of it as a bunch of separate objects, rather than as a single collection.

I'm curious how this differs from cidr_blocks, or other similar fields. Do you mind sharing an example of a typical example? Unless I'm misunderstanding, I'm not familiar with any cases where the values are truly distinct and not implicitly part of a collection.

The cidr_blocks field could have gone either way in terms of being singular or plural. Our API tends to use plural names, so unfortunately we're inconsistent in Terraform-style singular or GCP-style plural. We were mapping the API field into the provider; it's a list of objects so the list-style representation would work well but the same is true for many to nearly all lists/sets of &schema.Resources w/o a MaxSize of 1 (those are a special case of what the API calls a NestedObject- we do our best to make Terraform assume they're always present). The upstream definition includes display_name in addition to cidr_block which I don't think we support yet.

Taking a quick trawl through the provider, we've created an Optional+Computed TypeList or TypeSet in ~3 places. We use this when the API returns a default when nothing is sent; we're unable to represent defaults for complex values.

That includes;

• google_compute_security_policy.rule
• google_bigquery_dataset.access
• And the one in the issue- google_container_cluster.master_authorized_networks_config.cidr_blocks

---

The only difference between these fields and a field like google_compute_subnetwork.secondary_ip_ranges is that they have API-side defaults so we need to set Computed; as stated in the issue, it's impossible [in 0.12] to specify that no values should be present for these fields because we've lost the ability to explicitly specify the field.

[apparentlymart]

The intent of the block syntax is that users should feel like the blocks are distinct from one another. We collect them together into some sort of collection just as a practical matter to pass them to providers in a reasonable way and to make them usable in expressions. Naturally sometimes the implementation leaks into the interface in unintended ways, such as when TypeList is used but users don't actually think of the underlying blocks as having a natural order and are then surprised that adding a new block offsets all of the ones below.

I think most other examples I'm familiar with in the Google Cloud Platform provider do match this user model, even though behind the scenes they are being mapped to collections for sending to the API. Taking network_interface in google_compute_instance as an example:

  network_interface {
    network = "default"
  }

In the user's model, each network_interface is a distinct object that belongs to the instance it is nested within. Adding a new one of these is, conceptually, like writing out a new top-level resource: it is a request to create a new network interface while leaving the other one untouched. The implementation is pretty different than for a new top-level resource block because it's up to the provider to manage the lifecycle of these nested objects, but from the user's perspective these are intended to feel similar. (This is what we tool the SDK to docs mean when they say of an Elem of schema.Resource "possibly with its own lifecycle", though indeed that is worded in a very vague way that is open to other interpretations.)

While this is of course subjective, we think it tends to be confusing if the absense of any blocks of a particular type causes the provider to implicitly generate blocks but yet the first block added makes all of the default ones "vanish"; adding a new object should not lead to the distruction of existing objects. The attribute syntax makes that situation easier to understand because it more clearly distinguishes between unset, set to empty, and set to non-empty, and makes it clear to the user that these objects are collected together into a single value and so may affect one another.

The existing helper/schema model wasn't quite expressive enough to make this intent clear, so when we made Terraform more consistent about it we made some assumptions in the mapping from the existing schemas to the intended model, which is now explicit in Terraform's architecture. In this case, the assumption we made was incorrect.

---

Let's see how those other examples would look if we were to switch them over to using attribute syntax with object elements:

# explicitly disable the creation of the default rule
rule = []

# set one or more rules explicitly, disabling the creation of the default rule
rule = [
  {
    action = "allow"
    priority = 1
    match = [{
      config = [{
        src_ip_ranges = ["10.1.0.0/16"]
      }]
      versioned_expr = "SRC_IPS_V1"
    }]
  }
]

This one looks pretty clumsy as a list-of-objects value rather than as nested blocks 😖 ... the fact that there are more blocks nested inside, combined with the fact that the SDK provides no way to say that we just want the block as a single object rather than as a list/set, leads to the nested blocks looking particularly odd. So I think this one would require some further tweaking if we decided to take this approach with it.

# Explicitly disable the four access rules created by default
access = []

# Disable the four access rules created by default and create these instead...
access = [
  {
    role = "OWNER"
    view = [{
      project_id = "a"
      dataset_id = "b"
      table_id = "c"
    }]
  }
]

Again the nested block doesn't translate very well to this syntax, but otherwise it doesn't seem too bad.

I'm wondering if we could extend this idea by also offering a way in the SDK to explicitly ask for a nested block to be represented as a single object rather than as a list/set of objects. This is a harder one to retrofit because it would also mean that any references to attributes in those objects elsewhere would need to omit the [0] index operation in 0.12, but continue to support it in 0.11 for compatibility.

From some cursory initial investigation though, I think that is possible in principle... we'd leave it set as TypeList for the benefit of 0.11 but have the 0.12 shims detect whatever extra flag we add and unwrap it to be a single object on the way out. We'd need to contend with upgrading any references to these as users move from 0.11 to 0.12, but these all seem like things that would not commonly be referenced elsewhere, so hopefully that impacts a relatively small set of users.

If we were able to get that working, these new examples could look more like this:

rule = [
  {
    action = "allow"
    priority = 1
    match = {
      config = {
        src_ip_ranges = ["10.1.0.0/16"]
      }
      versioned_expr = "SRC_IPS_V1"
    }
  }
]

access = [
  {
    role = "OWNER"
    view = {
      project_id = "a"
      dataset_id = "b"
      table_id = "c"
    }
  }
]

Still quite a big change in appearance from how I assume these tend to look in most 0.11 configs, but at least it doesn't include any confusing additional punctuation.

Let's mull this over a bit more today. So far this feels like the most compelling path in terms of minimizing the risk/impact to other parts of this provider and other providers and ensuring that things continue to work as before for 0.11 users, but perhaps someone else (or me later, with fresher eyes) will find a different angle that seems better.

[rileykarson]

I don't think the expectations for the lifecycle of these blocks is any different than for google_compute_instance.network_interface; looking at the REST docs, the upstream schema is exactly the same as the equivalents; it's a list of objects. I think the only difference is that we require a network interface to be defined (which I would imagine is because the API returns an error otherwise), while we don't for these other fields.

I'm not sure what about the relationship between these sets of blocks would be considered distinguishably different;

cidr_blocks {
  cidr_block = "10.1.0.0/16"
}

cidr_blocks {
  cidr_block = "10.1.0.0/16"
}

network_interface {
  subnetwork = "projects/my-project/region/us-central1/subnetworks/my-subnetwork"
  alias_ip_range {
    ip_cidr_range = "10.1.0.0/16"
  }
}

network_interface {
  subnetwork = "projects/my-project/region/us-central1/subnetworks/my-subnetwork"
  alias_ip_range {
    ip_cidr_range = "10.1.0.0/16"
  }
}

[apparentlymart]

Indeed all of these are naturally modeled as blocks except for the strange requirement of treating unset differently from zero. I was just trying to give some context on how Terraform distinguishes between nested blocks and attributes, and why modelling these ones as attributes is the most straightforward way to support distinguishing unset from [].

(The fact that cidr_blocks is plural is unfortunate if the intent was for it to be a block, but that's a secondary problem to the problem at hand of how to preserve this unset-is-special behavior.)

[apparentlymart]

The Core-side change for this was #20837, along with a few other follow-up improvements. This requires an opt-in on the provider side to consider a particular schema as being an attribute for configuration purposes, and then in turn a fixup step in Terraform Core makes it so the block syntax is still accepted for those.

Work is underway to activate that opt-in mechanism where needed in the various providers, so I'm going to close this issue out for now and we'll track any further work on this mechanism in separate issues.

[lorengordon]

Is there any guidance on what arguments/blocks will or will not be updated to use ConfigMode: schema.SchemaConfigModeAttr?

This change makes it impossible to write modules that work with both tf 0.12 and tf 0.11, where they were previously using the (apparently coincidental) feature where blocks worked as lists of maps.

I was hoping to have a clean upgrade path, that looked something like:

1. Update modules to work with both tf 0.11 and tf 0.12
2. Update executing project to use tf 0.12
3. Update modules to use tf 0.12 syntax

However, now that seems plain impossible, as this attr/block change makes tf 0.12 more of an all-or-nothing upgrade.

[apparentlymart]

Indeed, it's only possible to write cross-compatible modules if they are not relying on undocumented gaps in the Terraform v0.11 language's validation. The best approach if you have a complex module that relies on such features is to create a new major version of the module and keep old configurations on the previous version until they are ready to use Terraform 0.12.

[lorengordon]

I don't think we're talking about "complex" modules in any way. Can be a very simple module that is just exposing a variable to let the user define a block. That was the only way blocks could really be optional in tf 0.11. Worked great. Didn't realize it was a coincidence of implementation, or an "undocumented gap" in validation.

[myroslavmail]

I had absolutely the same error that author referring to and fortunately I was able find and fix it. In my case it was INCORRECT name of "variables.tf" file which was among my modules. Accidentally I misspelled it as "variables.ft". (the "tf" extenstion). So my suggestion is to doublecheck all project files for appropriate naming.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.