Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helper/schema: fix validating nested objects #14784

Merged
merged 1 commit into from
May 24, 2017

Conversation

mattrobenolt
Copy link
Contributor

@mattrobenolt mattrobenolt commented May 24, 2017

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.

},
},

Vars: map[string]string{},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mattrobenolt
Copy link
Contributor Author

mattrobenolt commented May 24, 2017

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"

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:

```tf
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.
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

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

@@ -1373,7 +1373,7 @@ func (m schemaMap) validateObject(
k string,
schema map[string]*Schema,
c *terraform.ResourceConfig) ([]string, []error) {
raw, _ := c.GetRaw(k)
raw, _ := c.Get(k)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -1373,7 +1373,7 @@ func (m schemaMap) validateObject(
k string,
schema map[string]*Schema,
c *terraform.ResourceConfig) ([]string, []error) {
raw, _ := c.GetRaw(k)
raw, _ := c.Get(k)
if _, ok := raw.(map[string]interface{}); !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -3947,6 +3947,33 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: false,
},

"Good sub-resource, interpolated value": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
		},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jbardin jbardin merged commit d34dee5 into hashicorp:master May 24, 2017
jbardin added a commit that referenced this pull request May 24, 2017
GH-14784 allowed nested structures to be validate, rather than relying
on the raw value. However this still returns the same validation error
if the structures contain a computed value, since Get will return the
raw string in that case.

This simply skips the validation in the IsComputed case, since there's
nothing that can be checked.
jbardin added a commit that referenced this pull request May 24, 2017
GH-14784 allowed nested structures to be validate, rather than relying
on the raw value. However this still returns the same validation error
if the structures contain a computed value, since Get will return the
raw string in that case.

This simply skips the validation in the IsComputed case, since there's
nothing that can be checked.
@ghost
Copy link

ghost commented Apr 12, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants