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

Investigate and Determine Computed-Only Block Behavior #214

Closed
Tracked by #158
bflad opened this issue Oct 27, 2021 · 6 comments
Closed
Tracked by #158

Investigate and Determine Computed-Only Block Behavior #214

bflad opened this issue Oct 27, 2021 · 6 comments
Assignees
Labels
sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Oct 27, 2021

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: hashicorp/terraform-plugin-sdk#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 bflad added the sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it. label Oct 27, 2021
@bflad
Copy link
Contributor Author

bflad commented Oct 27, 2021

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.

@paddycarver
Copy link
Contributor

That last comment seems related to #42.

@bflad bflad added this to the v1.0.0 milestone Mar 16, 2022
bflad added a commit to hashicorp/terraform-docs-common that referenced this issue Jul 25, 2022
bflad added a commit to hashicorp/terraform-docs-common that referenced this issue Jul 25, 2022
Reference: hashicorp/terraform-plugin-framework#214
Reference: hashicorp/terraform-plugin-sdk#819
Reference: hashicorp/terraform-plugin-sdk#201

As part of investigating some differences between sdk/v2 and framework, the individual sensitivity configuration offered by nested attributes in protocol version 6 is important to call out explicitly.
@bflad
Copy link
Contributor Author

bflad commented Jul 25, 2022

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. Preserve sensitivity when set at impossible levels. terraform-plugin-sdk#819). Both paths are likely to introduce confusion either for provider developers and/or framework maintainers.

@bflad bflad self-assigned this Jul 25, 2022
@bflad
Copy link
Contributor Author

bflad commented Jul 25, 2022

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 added a commit to hashicorp/terraform-docs-common that referenced this issue Jul 26, 2022
#76)

Reference: hashicorp/terraform-plugin-framework#214
Reference: hashicorp/terraform-plugin-sdk#819
Reference: hashicorp/terraform-plugin-sdk#201

As part of investigating some differences between sdk/v2 and framework, the individual sensitivity configuration offered by nested attributes in protocol version 6 is important to call out explicitly.
@bflad bflad modified the milestones: v1.0.0, v0.12.0 Aug 8, 2022
@bflad
Copy link
Contributor Author

bflad commented Sep 13, 2022

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 bflad closed this as completed Sep 13, 2022
@github-actions
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Projects
None yet
Development

No branches or pull requests

3 participants