helper/schema: fix validating nested objects

[mattrobenolt]

When interpreting a nested object, we were validating against the "raw"
value, and not the interpolated value, causing incorrect errors.

This affects structures such as:

tags = "${list(map("foo", "bar"))}"

Prior to this, a complaint about "expected object, got string" since the
raw value is obviously a string, when the interpolated value is the
correct shape.

[mattrobenolt]

This was needed to trigger interpolation. Otherwise it's skipped and map() is never resolved since tc.Vars == nil.

[mattrobenolt]

Also, to be explicit, this now fixes behavior and allows doing syntax such as:

provider "google" {
    region = "test"
}

# default image name to use for boot disk
variable "image" {
    default = "test-image"
}

# variable for boot disk overrides
variable "boot_disk" {
    type = "map"
    # override the default boot disk attributes
    default = {
        image = "something-awesome"
        size = 512
    }
}

# Declare additional disk to be attached
variable "disk" {
    type = "list"
    # test simply adding one new disk
    default = [{
        image = "awesome-image"
    }]
}

resource "google_compute_instance" "test" {
    name = "test"
    machine_type = "test"
    zone = "test"

    # Merge a default boot disk option, { image => var.image, size => 100 }
    # with overrides coming in from `boot_disk`, then concat with the additional
    # disks defined in `disk`
    disk = "${concat(
        list(
            merge(
                map(
                    "image", var.image,
                    "size", "100",
                ),
                var.root_disk,
            ),
        ),
        var.disk
    )}"
}

Which spits out the plan:

+ google_compute_instance.test
    can_ip_forward:                    "false"
    create_timeout:                    "4"
    disk.#:                            "2"
    disk.0.auto_delete:                "true"
    disk.0.disk_encryption_key_sha256: "<computed>"
    disk.0.image:                      "something-awesome"
    disk.0.size:                       "512"
    disk.1.auto_delete:                "true"
    disk.1.disk_encryption_key_sha256: "<computed>"
    disk.1.image:                      "awesome-image"
    machine_type:                      "test"
    metadata_fingerprint:              "<computed>"
    name:                              "test"
    self_link:                         "<computed>"
    tags_fingerprint:                  "<computed>"
    zone:                              "test"

[jbardin]

Thanks for the fix!

This section predates first-class maps and lists altogether, regardless of nesting, so I can see how it was overlooked.

[jbardin]

Since Get will return the Raw value if it's computed, this should probably check for that too. I don't think that validation error helps in the unknown variable case either.

[jbardin]

I think simply adding ; !ok && !c.IsComputed(k) would suffice here

[jbardin]

Since we're already in here, I think a test with a computed value (that fails before the IsComputed check) would be useful too.

[mattrobenolt]

It's a bit unclear to me what IsComputed is actually checking for. Can you provide me with an example? Is this something like, foo.0?

[mattrobenolt]

I think I got it, but I'm unable to put together a failing test.

The best case I could come up with was:

		"Bad sub-resource, interpolated value": {
			Schema: map[string]*Schema{
				"ingress": &Schema{
					Type:     TypeList,
					Optional: true,
					Elem: &Resource{
						Schema: map[string]*Schema{
							"from": &Schema{
								Type:     TypeInt,
								Required: true,
							},
						},
					},
				},
			},

			Config: map[string]interface{}{
				"ingress": []interface{}{
					`${map("from", var.port)}`,
				},
			},

			Vars: map[string]string{
				"var.port": config.UnknownVariableValue,
			},

			Err: true,
		},

[jbardin]

IsComputed basically checks if the value is known during validation, i.e. it's in the config and doesn't contain any UnknownValues. If you look at the source to Get, it's actually going to call IsComputed there too, and otherwise return GetRaw.

I think the PR is still good on its own, so I can merge this and I'll send a follow up PR with the additional changes.

[mattrobenolt]

IsComputed basically checks if the value is known during validation, i.e. it's in the config and doesn't contain any UnknownValues.

Yeah, I got that far. :) I was moreso struggling to construct a test case that proves that additional check is required.

I'll send a follow up PR with the additional changes.

Mind cc'ing me on this? I'd like to see what I'm missing here for the future.

Thanks!

[jbardin]

Sorry, started typing before I saw your update...
That looks right, and I swear I had it fail when I tested it. Let me see what's going on.

[jbardin]

I see, it's passing because Err: true,. 😉

You'll just need to change Required to Optional here to pass the test, since there there is no real value here.

[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.