terraform-plugin-framework icon indicating copy to clipboard operation
terraform-plugin-framework copied to clipboard

Investigate and Determine Computed-Only Block Behavior

Open bflad opened this issue 4 years ago • 4 comments

Module version

v0.4.2

Description

If/when block support is implemented (see also: #85), there may need to be an investigation into how Computed only blocks (or more technically accurate, where all block attributes are Computed only) are documented or otherwise supported by the framework.

In Terraform Plugin SDK version 2, there is special logic that automatically applied to Computed-only blocks that converts them into attributes:

https://github.com/hashicorp/terraform-plugin-sdk/blob/9dad6ed9dc7845584ce20f1116c02a10863b1900/helper/schema/core_schema.go#L96-L101

While this has its own issues (see also: https://github.com/hashicorp/terraform-plugin-sdk/issues/201) and likely would not be done in this framework since its design tends to avoid such unexpected abstraction/behavior changes, something may need to be done to prevent Terraform CLI "inconsistent results after apply" errors, if they occur, whether that be additional documentation, helpers, or other handling by the framework.

Definition of Done

terraform-provider-corner may be the most appropriate place to capture end-to-end testing scenarios, since any potential error would occur in Terraform CLI.

  • Create a resource schema that contains a list block with Computed only attributes (replicate for set blocks as well)
    • Verify not setting state does not return an error
    • Verify setting 1 list element with attributes does not return an error
    • Verify setting 2+ list elements with attributes does not return an error
    • Create a resource schema that contains a list block with a list block (nested block) with Computed only attributes (replicate for nested set blocks as well)
    • Verify not setting state does not return an error
    • Verify setting 1 list element with attributes does not return an error
    • Verify setting 2+ list elements with attributes does not return an error
  • If an error is returned in any of these cases, framework maintainers determine next course of action and create further issues as appropriate for documentation, helpers, other handling in the framework, etc.

bflad avatar Oct 27 '21 19:10 bflad

Another potential item to consider is if there is any state issues when migrating between frameworks as the schema will be represented slightly differently to Terraform CLI. This may require manually verifying an applied SDKv2 resource state against a plan/apply with updated values from the new framework.

bflad avatar Oct 27 '21 19:10 bflad

That last comment seems related to #42.

paddycarver avatar Oct 27 '21 21:10 paddycarver

It looks like providers migrating from sdk/v2 to framework, but remaining on protocol version 5, do actually need something to be done here otherwise Terraform will report back empty list of object errors for any configuration references to read-only block attributes. Given the blocks are read-only, there is no configuration for them so therefore Terraform in certain situations will set the value for the block be an empty list or set.

One additional challenge here is that this situation can be non-trivial to catch for data sources via the acceptance testing framework today, because there are behavior differences between a data source having all known values in its configuration versus any unknowns, and because triggering this type of error requires a configuration reference to a nested attribute of the block.

We have a few options (not necessarily mutually exclusive) with tradeoffs:

  • For providers migrating to the framework that can bump to protocol version 6, always recommending they use nested attributes instead of blocks in this situation. The nested attributes definitions will clearly define the capabilities of the attributes.
  • For providers migrating to the framework that are constrained to protocol version 5, recommending that they use an Attribute that is defined as Type: types.ListType{Elems: types.ObjectType{AttrTypes: /* ... */}} or Type: types.SetType{Elems: types.ObjectType{AttrTypes: /* ... */}}. This has the benefit of exactly showing the provider developer that there is no special configurability with the nested attributes, such as Sensitive, but does introduce a migration burden of changing the definition in a non-trivial manner.
  • Adding some field (let's call it Computed bool for concept simplicity) to the Block type, which recreates the terraform-plugin-sdk/v2 behavior of automatically switching a read-only block into its "equivalent" attribute. Equivalent is in quotes because not all schema definitions that go through this conversion can be properly represented. This may make provider migrations simpler on the surface, however it represents an incorrect abstraction choice, which this framework design has exclusively tried to avoid so far. For one example, it would still allow the incorrect schema definition handling present in sdk/v2 where a provider developer can incorrectly declare nested attributes as Sensitive: true. The framework would either need to go down the additional path of giving much more explicit error feedback about schemas that cannot be converted like this or come up with some features proposed in sdk/v2, like trying to convert schema intents to their actual definition (e.g. https://github.com/hashicorp/terraform-plugin-sdk/pull/819). Both paths are likely to introduce confusion either for provider developers and/or framework maintainers.

bflad avatar Jul 25 '22 21:07 bflad

For what it is worth, I initially lean towards documenting this in the migration documentation (items one and two above). For most providers, I think the preference will be to go to protocol version 6 and nested attributes, which is fairly similar in implementation to read-only blocks in sdk/v2. I just wish there was a better way to try to catch this for provider developers migrating -- maybe the lack of the Computed field will trigger a "what should I do now?" question which we can document, but certainly its possible to just remove that and not know about this potential issue in general.

bflad avatar Jul 25 '22 21:07 bflad

Initial sdk/v2 to framework migration guide for this topic has been published: https://www.terraform.io/plugin/framework/migrating/attributes-blocks/blocks-computed

bflad avatar Sep 13 '22 19:09 bflad

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 Oct 14 '22 02:10 github-actions[bot]