terraform icon indicating copy to clipboard operation
terraform copied to clipboard

Blocks of nesting mode list/map with a dynamic attribute cause the entire block to be dynamic

Open austinvalle opened this issue 1 year ago • 7 comments

Terraform Version

Terraform v1.7.1
on darwin_arm64

Terraform Configuration Files

See below

Debug Output

N/A

Expected Behavior

The logic in terraform-plugin-go currently works with a nested block for nesting mode Single when a dynamic attribute is present:

Config

resource "corner_dynamic_thing" "foo" {
	block_with_dpt {
		bar = "hello" # Note: this attribute is a `dynamic` type
		foo = 4
	}
}

Schema (terraform-plugin-go - protocol v6)

&tfprotov6.Schema{
  Block: &tfprotov6.SchemaBlock{
    BlockTypes: []*tfprotov6.SchemaNestedBlock{
      {
        TypeName: "block_with_dpt",
        // Nesting Mode: Single
        Nesting: tfprotov6.SchemaNestedBlockNestingModeSingle,
        Block: &tfprotov6.SchemaBlock{
          Attributes: []*tfprotov6.SchemaAttribute{
            {
              Name:     "bar",
              Type:     tftypes.DynamicPseudoType,
              Optional: true,
            },
            {
              Name:     "foo",
              Type:     tftypes.Number,
              Optional: true,
            },
          },
        },
      },
    },
  },
},

Resulting msgpack data

{
  "block_with_dpt": {
    "bar": [
    "\"string\"",
    "hello"
    ],
    "foo": 4
  }
}

Currently our expectations are that nesting mode List and Map should behave in a similar fashion to Single with regards to a dynamic attribute. If that logic isn't possible in Terraform core then we may have to adjust terraform-plugin-go, or prevent the upcoming dynamic type support from working with ListNestedBlock: https://github.com/hashicorp/terraform-plugin-framework/issues/147

Actual Behavior

When a nested block with a nesting mode of List or Map has an attribute that is a DynamicPseudoType, Terraform core sends the entire nested block as a DynamicPseudoType.

This ends up triggering an error in terraform-plugin-go when decoding the msgpack data from Terraform core, raised in https://github.com/hashicorp/terraform-plugin-go/issues/267. terraform-plugin-go uses the schema to decode the msgpack data, which is not expecting the entire block to be dynamic, just bar (in this example).

  | Error: Unable to Convert DynamicValue
  | 
  |   with corner_dynamic_thing.foo,
  |   on terraform_plugin_test.tf line 11, in resource "corner_dynamic_thing" "foo":
  |   11: resource "corner_dynamic_thing" "foo" {
  | 
  | Converting the DynamicValue to Value returned an unexpected error:
  | AttributeName("block_with_dpt").ElementKeyInt(0): error decoding object
  | length: msgpack: unexpected code=c4 decoding map length

Example - Nesting Mode List

Config

resource "corner_dynamic_thing" "foo" {
	block_with_dpt {
		bar = "hello" # Note: this attribute is a `dynamic` type
		foo = 4
	}
}

Schema (terraform-plugin-go - protocol v6)

&tfprotov6.Schema{
  Block: &tfprotov6.SchemaBlock{
    BlockTypes: []*tfprotov6.SchemaNestedBlock{
      {
        TypeName: "block_with_dpt",
        // Nesting Mode: List
        Nesting: tfprotov6.SchemaNestedBlockNestingModeList,
        Block: &tfprotov6.SchemaBlock{
          Attributes: []*tfprotov6.SchemaAttribute{
            {
              Name:     "bar",
              Type:     tftypes.DynamicPseudoType,
              Optional: true,
            },
            {
              Name:     "foo",
              Type:     tftypes.Number,
              Optional: true,
            },
          },
        },
      },
    },
  },
}

Resulting MsgPack config

{
  "block_with_dpt": [
    "[\"tuple\",[[\"object\",{\"bar\":\"string\",\"foo\":\"number\"}]]]",
    [
      {
        "bar": "hello",
        "foo": 4
      }
    ]
  ]
}

Example - Nesting Mode Map

Config

resource "corner_dynamic_thing" "foo" {
	block_with_dpt "test" {
		bar = "hello" # Note: this attribute is a `dynamic` type
		foo = 4
	}
}

Schema (terraform-plugin-go - protocol v6)

&tfprotov6.Schema{
  Block: &tfprotov6.SchemaBlock{
    BlockTypes: []*tfprotov6.SchemaNestedBlock{
      {
        TypeName: "block_with_dpt",
        // Nesting Mode: Map
        Nesting: tfprotov6.SchemaNestedBlockNestingModeMap,
        Block: &tfprotov6.SchemaBlock{
          Attributes: []*tfprotov6.SchemaAttribute{
            {
              Name:     "bar",
              Type:     tftypes.DynamicPseudoType,
              Optional: true,
            },
            {
              Name:     "foo",
              Type:     tftypes.Number,
              Optional: true,
            },
          },
        },
      },
    },
  },
}
{
  "block_with_dpt": [
    "[\"object\",{\"test\":[\"object\",{\"bar\":\"string\",\"foo\":\"number\"}]}]",
    {
    "test": {
      "bar": "hello",
      "foo": 4
    }
    }
  ]
}

Steps to Reproduce

I opened a draft PR in our test provider with an acceptance test for each of these examples (Single, Map, List):

  • Source: https://github.com/hashicorp/terraform-provider-corner/blob/av/dpt-testing/internal/dynamic6provider/dynamic_nested_block_test.go
  • Example test runs: https://github.com/hashicorp/terraform-provider-corner/actions/runs/7659190026/job/20873827028?pr=208

I'm not 100% sure how best to help reproduce this for the Terraform core environment, but can provide any assistance needed there 😄

Additional Context

The Terraform DevEx team is planning on introducing Dynamic Type support into Plugin Framework soon, so we've been writing some initial tests against terraform-plugin-go to ensure everything is working as expected. We're currently not sure if this is a bug in Terraform core or in terraform-plugin-go.

  • Original bug report: https://github.com/hashicorp/terraform-plugin-go/issues/267
  • Dynamic Type Support in Plugin Framework: https://github.com/hashicorp/terraform-plugin-framework/issues/147

References

No response

austinvalle avatar Jan 25 '24 19:01 austinvalle

Hi @austinvalle,

I must confess that I've not delved into all of the context you shared here because I'm about to go to lunch :confounded: but I did just want to address what I think is the main question here:

In Terraform's type system, there are both "collection types" and "structural types". Collection types represent a dynamically-chosen number of elements all of the same type, whereas structural types represent a statically-chosen set of elements/attributes that may have different types.

The matrix of these different type kinds is incomplete:

  • tuple is the structural analog to the collection type kind list
  • object is the structural analog to the collection type kind map
  • there is no structural analog to the collection type kind set, because set elements are not identified by anything other than their content and so a structural analog to a list would have to be a constant value that cannot vary at runtime, which would be pretty useless

Because all elements of a collection type must have the same element type, it isn't really that useful to include any (the Terraform language spelling of "dynamically-inferred type") inside the element type of a collection, although it is possible to do it with the constraint that all of the elements of the collection must make the same choice about which type to use for that attribute.

This means that there isn't really any plausible way to represent a set whose element type constraint contains any: the only possible values that could be of type set(any) are an unknown value with that type constraint or a null value with that type constraint. Making a known and not null value conforming to that type constraint would mean replacing the any with a concrete type, which all elements of the set would then need to share.

(I've used set(any) as a shorthand here because it's the shortest way to spell what I mean, but what I've said is true for any set type whose element constraint contains any, even if it's nested inside other collection/structural type constraints.)

Given all of that, it might be best to consider this an impossible schema, since Terraform's type system cannot represent what I assume the provider author intended it to mean -- a set where each element has a different object type -- and all of the possible interpretations we could make of it have limitations to the point of making them essentially useless, such as only allowing unknown or null values.

I'll try to dig in to this some more later if what I've said above seems to miss the point or doesn't completely answer the question, but hopefully the above is useful as a starting point.

apparentlymart avatar Jan 25 '24 20:01 apparentlymart

Hi @austinvalle,

Martin summed up what I was going write, but I'll reiterate that everything mentioned about set also applies to list and map like you have in the examples.

I'll add how I was going to frame the situation, having a nested dynamic type like so (in TF syntax):

map(object({
  attr = string
  dyn = any
}))

May be technically possible, as the dyn attribute type could be determined later, but from my experience that is almost never what the user intended when they wrote it, expecting that the dyn attribute can be different between objects. We can definitely try to narrow down where the type is losing some specificity, but it's worth thinking about if we want to make it possible as it could be more confusing than helpful.

jbardin avatar Jan 25 '24 20:01 jbardin

Thanks both of you for all of that information, extremely helpful context.

A quick clarifying question, my initial report was framed with nested blocks as an example. Is it correct to assume, since the restrictions/confusions are related to the type system, that everything you've described would apply to nested attributes as well?

From some initial tests it seems that nested attribute lists/maps with dynamics are able to be determined correctly. However, reading this:

We can definitely try to narrow down where the type is losing some specificity, but it's worth thinking about if we want to make it possible as it could be more confusing than helpful.

I agree that just because it's technically possible doesn't mean we'd want to enable usage of dynamics inside of collections if it's too confusing.

Here is an example with nested attributes:

resource "corner_dynamic_thing" "foo" {
	list_with_dpt = [
		{
			"bar" = "hello" # dynamic, will find common type `string`
			"foo" = 4
		},
		{
			"bar" = 1.23 # dynamic, will find common type `string`
			"foo" = 4
		},
		{
			"bar" = 333 # dynamic, will find common type `string`
			"foo" = 4
		}
	]
}
&tfprotov6.Schema{
  Block: &tfprotov6.SchemaBlock{
    Attributes: []*tfprotov6.SchemaAttribute{
      {
        Name: "list_with_dpt",
        NestedType: &tfprotov6.SchemaObject{
          Nesting: tfprotov6.SchemaObjectNestingModeList,
          Attributes: []*tfprotov6.SchemaAttribute{
            {
              Name:     "bar",
              Type:     tftypes.DynamicPseudoType,
              Optional: true,
            },
            {
              Name:     "foo",
              Type:     tftypes.Number,
              Optional: true,
            },
          },
        },
        Optional: true,
      },
    },
  },
}
{
  "list_with_dpt": [
    {
      "bar": [
        "\"string\"",
        "hello"
      ],
      "foo": 4
    },
    {
      "bar": [
        "\"string\"",
        "1.23"
      ],
      "foo": 4
    },
    {
      "bar": [
        "\"string\"",
        "333"
      ],
      "foo": 4
    }
  ]
}

If we end up deciding that map and list collections shouldn't contain dynamic attributes, would we be able to add validation logic similar to set to make that more explicit in Terraform core? https://github.com/hashicorp/terraform/blob/c7a44bfc9adb1b43686fb806ad3f14190ec9aa7d/internal/configs/configschema/internal_validate.go#L73-L81

austinvalle avatar Jan 26 '24 15:01 austinvalle

I wanted to take a few minutes to walk through this and find out where the type declaration comes from. Turns out that within the design of the hcldec.Spec it was decided that a nested DynamicType within list and map blocks doesn't need to confirm to a single type! When we create the decoder spec for the schema, if a nested map encounters any dynamic types, the block is instead defined as a BlockObjectSpec to allow for different block values to contain different dynamic types (nested list does the same, by using a BlockTupleSpec).

This does differ from nested attributes, because those were modeled directly after the equivalent cty.Type. Using mixed types in a nested attribute will result in the familiar cty error Inappropriate value for attribute "nested": cannot find a common base type for all elements., but a nested block, being a DynamicPseudoType will allow any and all values.

So what this basically boils down too is that the list and map blocks as allowed in the HCL/Terraform language can't be fully represented by the cty type system, so the best we can do when encoding is to use a DynamicPseudoType for the entire block. This could theoretically allow invalid object values coming back from the provider to decode correctly, which would probably lead to "unsupported attribute" errors coming up in unexpected places. I'm not sure how to reconcile these differences without breaking changes, so maybe this is enough of a reason to not allow dynamic within nested block at all?

jbardin avatar Feb 07 '24 20:02 jbardin

Ah okay, very interesting....

From the DevEx perspective we're trying to limit confusion wherever possible when introducing this dynamic type support. With that in mind, and given all the information you've provided it sounds like a potential plan could be:

1. Allow DynamicAttribute:

2. Add validation to prevent DynamicAttribute under all collection attributes/blocks

To avoid a breaking change to terraform-plugin-framework, this would need to be done at runtime with a validation rule that prevents the following from having children attributes that are DynamicAttribute:

3. Allow Dynamic in function definitions

  • As a DynamicParameter at the root of a function definition
  • (NEW) As a variadic DynamicParameter
  • As a nested attribute type in an ObjectParameter
  • (NEW) As a nested attribute type in a variadic ObjectParameter
  • As a DynamicReturn at the root of a function definition
  • As a nested attribute type in an ObjectReturn

4. Add validation to prevent DynamicType under all collection parameters/returns for functions

Similar to the schema, to avoid a breaking change to terraform-plugin-framework, at runtime, add validation to prevent the following from containing a DynamicType:

  • ListParameter, ListReturn, MapParameter, MapReturn, SetParameter, SetReturn
  • Inside a collection type in a nested attribute type in an ObjectParameter or ObjectReturn

From a core perspective, does this plan make sense? Are there any additional restrictions we should consider?

~~On the framework side it'll be a little awkward if a provider developer wants to make a variadic DynamicParameter, as we currently wrap it in a ListType 😆, so we should probably consider preventing that as well?~~

(EDIT: We plan on adjusting the internals of plugin framework to handle variadic parameters as a TupleType)

austinvalle avatar Feb 08 '24 21:02 austinvalle

1.

  • Those all are places where the result appears to be consistent, so should be OK

2.

  • Yes, avoiding this in blocks entirely will hide the fact that users could create inconsistent types within the instance. While this isn't technically a "problem", it conflicts with how collections work in other contexts and probably causes more confusion than it's worth.
  • I don't see anything wrong with avoiding these in the attribute schemas in principle, and it is easier to add later if we choose to add it later rather than remove it if it was a mistake. I think they are however even less of a problem here, because the attribute expression must deal with types the same way as any other expression in Terraform, so there isn't the chance for inconsistencies and hence less confusion.

I don't know if limiting the nested types is necessary in the function arguments or returns (sets excluded of course). I would of course prefer if users didn't, because they typically pick a nested dynamic type for the wrong reasons, but like the attributes they will work as consistently as they do elsewhere in the language. Again, it's easier to add later, especially if it's just prevented by validation rather than a structural limitation.

As for combining parameters into a list, it depends on how they are represented in the framework code. Terraform handles then as a native Go slice, so all parameters including variadic are part of a []cty.Vaue. If the variadic portion were bundled into a single cty.List, then any dynamic types within that would be subject to the same constraints on type consistency. I could see this causing some confusion because Terraform could accept the individual arguments, but the framework would fail to decode it (for this type of work within Terraform, the default collections are tuple and object as opposed to list and map to avoid errors with inconsistent nested types).

Overall I think limiting the use of dynamic where it's not clear how the framework could handle it is the safer decision. Preventing its use in blocks also tracks well with the idea that we want to encourage attributes of the instance to actually be attributes rather than blocks for consistency within the configuration too.

jbardin avatar Feb 12 '24 20:02 jbardin

Awesome, it sounds like we're on the same page then 👍🏻

For the variadic parameters, Brian and I chatted and we decided that we're going to switch the Framework representation to a TupleType to avoid any weird decoding errors on our side when we introduce DynamicParameter.

For the restrictions we plan to apply, we're also err-ing on the side of caution and will start by initially preventing any collection types/attributes from containing dynamics in schemas or function definitions. Then, as you mentioned, we can introduce them later if provider developers express a need for it.

(I edited my above comment to reflect the entire plan)


As for the initial bug report this issue was created for, it sounds like this behavior in Terraform core around collection blocks cannot be altered and so we'll likely close the bug on the terraform-plugin-go side. If you're in agreement, I think we can close this issue. Thanks!

austinvalle avatar Feb 15 '24 20:02 austinvalle

Yes, this does appear to be working as designed, so there's not much we can do regarding the existing behavior. Going to close this.

jbardin avatar Mar 04 '24 23:03 jbardin

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.

github-actions[bot] avatar Apr 04 '24 02:04 github-actions[bot]