hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io
Other
42.76k stars 9.56k forks source link

Can't use known variables in dynamic block labels #32180

Open wata727 opened 2 years ago

wata727 commented 2 years ago

Terraform Version

Terraform v1.3.4
on linux_amd64
+ provider registry.terraform.io/apparentlymart/testing v0.0.2

Terraform Configuration Files

terraform {
  required_providers {
    testing = {
      source = "apparentlymart/testing"
      version = "0.0.2"
    }
  }
}

variable "foo" {
  type      = string
  default   = "foo"
}

data "testing_assertions" "main" {
  dynamic "equal" {
    for_each = toset(["foo"])
    labels   = [var.foo]
    content {
      got  = equal.value
      want = "foo"
    }
  }
}

Debug Output

https://gist.github.com/wata727/dd14d0bcf7e79af87d6d9b993b40cfb5

Expected Behavior

Since var.foo is a known variable, the plan operation succeeds just like when using the variable in for_each.

Actual Behavior

Got an error that the var.foo is unknown:

╷
│ Error: Invalid dynamic block label
│
│   on main.tf line 18, in data "testing_assertions" "main":
│   18:     labels   = [var.foo]
│     ├────────────────
│     │ var.foo is a string
│
│ This value is not yet known. Dynamic block labels must be immediately-known values.
╵

Steps to Reproduce

  1. Create main.tf with the above config.
  2. Run terraform init
  3. Run terraform plan

Additional Context

To be honest, I'm not sure if this is a bug. This strange behavior is an edge case we stumbled across in supporting dynamic blocks in TFLint, and I think that very few users are likely to be affected by this issue in the real world. Please understand that this issue is not of high priority. Just wanted to let you know that I noticed :)

After a bit of research, I found that this issue occurs when evaluating labels in hcl/ext/dynblock. https://github.com/hashicorp/hcl/blob/v2.14.1/ext/dynblock/expand_spec.go#L189-L199

It seems to occur when calling EvaluateBlock in terraform.NodeValidatableResource. https://github.com/hashicorp/terraform/blob/v1.3.4/internal/terraform/node_resource_validate.go#L442

I'm guessing that perhaps you are missing the possibility of having variables in the labels when validating a resource with a dummy EvalContext during the validation phase. For instance, if for_each contains an unknown variable, it will return an unknown body so it can pass the validation phase. https://github.com/hashicorp/hcl/blob/v2.14.1/ext/dynblock/expand_body.go#L204-L213

Please be aware that if you allow variables, sensitive values may be set. the expandSpec doesn't take into account the marked cty.Value, so it can cause a panic. This is also my motivation to see what happens when I use variables as labels.

References

No response

apparentlymart commented 2 years ago

Hi @wata727! Thanks for reporting this.

I think you're right that this is a bug.

During the validation phase all input variables are treated as unknown values because validation aims to answer if the configuration is valid for any possible plan options, rather than for a specific set of plan options. (Input variables are part of the plan options, because we specify them only when creating a plan using terraform plan.)

This seems to cause a problem with dynamic blocks because they aren't really part of Terraform (they are a generic HCL feature) and so the dynamic block expansion logic doesn't know anything about Terraform's validation phase. For other language features that have similar constraints -- such as count and for_each -- Terraform avoids expanding those during validation and just checks whether the expressions inside are valid for any possible count.index/each.key/each.value.

Thankfully there aren't many providers that actually use labelled blocks as part of their schema, because the Terraform Plugin SDK didn't support declaring such a schema. This apparentlymart/testing provider, which I developed as part of an early prototype of testing in Terraform, was one of the first providers written for Terraform v0.12 in particular and so I implemented it directly against the wire protocol rather than using the SDK, and so I was able to use this feature even though most other providers could not. The modern plugin framework treats nested block types as a legacy capability only for migrating old providers, so I don't think it supports labelled blocks either, and so perhaps this provider and its builtin successor terraform.io/builtin/test are the only providers which make use of labelled blocks like this.

I'm not sure yet if there's a practical way to address this problem, since as mentioned above dynamic blocks are a generic HCL feature and so are not aware of Terraform's special needs during validation. The closest equivalent to what we do for count and for_each would be to skip expanding the dynamic blocks at all during validation, but I think that would mean that we would not be able to validate what's inside the content block, and that would be unfortunate.

We may instead prefer to observe that labelled nested blocks are not a feature we support in any of the official SDKs and to consider this to be a legacy system preserved for backward compatibility only, retaining the limitation you've described here as an unfortunate design flaw. The modern plugin framework instead encourages declaring a map of an object type, because it's typically easier to dynamically construct such a data structure using other features of the language, without using dynamic blocks.

Since it isn't clear yet what makes sense to do here, I'm going to leave this open and label it appropriately but I expect that we won't change anything in response to this for now, until we have some time to confirm whether it's viable to resolve this by documenting labelled nested blocks as not recommended for new providers.

Thanks again!