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

Unexpected Behavior With Plan Modifier (UseStateForUnknown) Prior State Under Lists and Sets #709

Open
rschmied opened this issue Mar 30, 2023 · 4 comments · Fixed by #711
Labels
bug Something isn't working
Milestone

Comments

@rschmied
Copy link

Module version

github.com/hashicorp/terraform-plugin-framework v1.2.0
Terraform v1.4.2 on linux_amd64

Relevant provider source code

func LabGroup() map[string]schema.Attribute {
	return map[string]schema.Attribute{
		"id": schema.StringAttribute{
			Description: "Group ID (UUID).",
			Optional:    true,
			Computed:    true,
			PlanModifiers: []planmodifier.String{
				stringplanmodifier.UseStateForUnknown(),
			},
		},
		"name": schema.StringAttribute{
			Description: "Descriptive group name.",
			Computed:    true,
			PlanModifiers: []planmodifier.String{
				stringplanmodifier.UseStateForUnknown(),
			},
		},
		"permission": schema.StringAttribute{
			MarkdownDescription: "Permission, either `read_only` or `read_write`.",
			Optional:            true,
			Computed:            true,
			PlanModifiers: []planmodifier.String{
				stringplanmodifier.UseStateForUnknown(),
			},
			Validators: []validator.String{
				cmlvalidator.GroupPermission{},
			},
		},
	}
}

func Lab() map[string]schema.Attribute {
	return map[string]schema.Attribute{
		"id": schema.StringAttribute{
			Computed:    true,
			Description: "Lab identifier, a UUID.",
			PlanModifiers: []planmodifier.String{
				stringplanmodifier.UseStateForUnknown(),
			},
		},
		// attributes ommitted here
		"groups": schema.SetNestedAttribute{
			Optional:    true,
			Computed:    true,
			Description: "Groups assigned to the lab.",
			NestedObject: schema.NestedAttributeObject{
				Attributes: LabGroup(),
			},
			PlanModifiers: []planmodifier.Set{
				setplanmodifier.UseStateForUnknown(),
			},
		},
	}
}

Terraform Configuration Files

resource "cml2_group" "group1" {
	name       = "user_acc_lab_test_group1"
}

resource "cml2_group" "group2" {
	name       = "user_acc_lab_test_group2"
}

resource "cml2_lab" "test" {
	title       = "something"
	description = "something more"
	notes   = "somenotes"
	groups = [
		{
			id = cml2_group.group1.id
			permission = "read_only"
		},
		{
			id = cml2_group.group2.id
			permission = "read_only"
		}
	]
}

Debug Output

Expected Behavior

Running with this configuration and removing one of the groups from the set should update the state to reflect the groups set change.

Actual Behavior

The update happens OK on the backend, the API returns all the right elements in the groups set (1 element) but it's a 50/50 chance that the state still has the other value.

My test removes the element with the ID cml2_group.group1.id from the set. When it fails, the resulting error is

=== RUN   TestAccLabResource
    lab_test.go:29: Step 3/3 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to cml2_lab.test, provider
        "provider[\"registry.terraform.io/hashicorp/cml2\"]" produced an unexpected
        new value: .groups: planned set element
        cty.ObjectVal(map[string]cty.Value{"id":cty.StringVal("ee272f5e-2886-43db-9591-1e60a235bf6e"),
        "name":cty.StringVal("user_acc_lab_test_group1"),
        "permission":cty.StringVal("read_write")}) does not correlate with any
        element in actual.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAccLabResource (8.56s)
FAIL
FAIL    github.com/rschmied/terraform-provider-cml2/internal/provider/resource/lab      8.574s
FAIL

Apparently, the remaining element in the state/set is still the one that was removed from the HCL (and which does not appear in the element returned from the API). It's entirely possible that I do something wrong here as I was working with Lists before but had to move to Sets because of element order etc. As a workaround and in an attempt to mitigate / isolate the error, I added a SetUnknown to ModifyPlan like so:

	resp.Diagnostics.Append(
		resp.Plan.SetAttribute(
			ctx, path.Root("groups"),
			types.SetUnknown(
				types.ObjectType{
					AttrTypes: cmlschema.LabGroupAttrType,
				},
			),
		)...,
	)

When I add the above code, Terraform crashes:

=== RUN   TestAccLabResource
    lab_test.go:29: Step 1/3 error: Error running post-apply plan: exit status 11
        
        !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!
        
        Terraform crashed! This is always indicative of a bug within Terraform.
        Please report the crash with Terraform[1] so that we can fix this.
        
        When reporting bugs, please include your terraform version, the stack trace
        shown below, and any additional information which may help replicate the issue.
        
        [1]: https://github.com/hashicorp/terraform/issues
        
        !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!
        
        value is not known
        goroutine 165 [running]:
        runtime/debug.Stack()
                /usr/local/go/src/runtime/debug/stack.go:24 +0x65
        runtime/debug.PrintStack()
                /usr/local/go/src/runtime/debug/stack.go:16 +0x19
        github.com/hashicorp/terraform/internal/logging.PanicHandler()
                /home/circleci/project/project/internal/logging/panic.go:55 +0x153
        panic({0x227e900, 0x2d49290})
                /usr/local/go/src/runtime/panic.go:890 +0x262
        github.com/zclconf/go-cty/cty.Value.LengthInt({{{0x2d74278?, 0xc0005f1ed0?}}, {0x23329a0?, 0x41b67c0?}})
                /home/circleci/go/pkg/mod/github.com/zclconf/go-cty@v1.12.1/cty/value_ops.go:1063 +0x1f8
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedObjectValid(0xc0003f2640, {{{0x2d74278?, 0xc0005f10d0?}}, {0x2669b20?, 0xc000730d20?}}, {{{0x2d74278?, 0xc0005f0d60?}}, {0x2669b20?, 0xc000730c18?}}, {{{0x2d74278, ...}}, ...}, ...)
                /home/circleci/project/project/internal/plans/objchange/plan_valid.go:421 +0x5bf
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedValueValid(0xc000da1440, {{{0x2d74278?, 0xc0005f10d0?}}, {0x2669b20?, 0xc000730d20?}}, {{{0x2d74278?, 0xc0005f0d60?}}, {0x2669b20?, 0xc000730c18?}}, {{{0x2d74278, ...}}, ...}, ...)
                /home/circleci/project/project/internal/plans/objchange/plan_valid.go:308 +0xa1b
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedAttrValid({0xc000d884b8, 0x6}, 0xc000da1440, {{{0x2d74240?, 0xc0005f11a0?}}, {0x2360480?, 0xc000f8a420?}}, {{{0x2d74240, 0xc0005f0db0}}, {0x2360480, ...}}, ...)
                /home/circleci/project/project/internal/plans/objchange/plan_valid.go:264 +0x345
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedAttrsValid(0x0?, {{{0x2d74240?, 0xc0005f11a0?}}, {0x2360480?, 0xc000f8a420?}}, {{{0x2d74240?, 0xc0005f0db0?}}, {0x2360480?, 0xc000f8a000?}}, {{{0x2d74240, ...}}, ...}, ...)
                /home/circleci/project/project/internal/plans/objchange/plan_valid.go:249 +0x24b
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlanValid(0xc000e18600, {{{0x2d74240?, 0xc0005f11a0?}}, {0x2360480?, 0xc000f8a420?}}, {{{0x2d74240?, 0xc0005f0db0?}}, {0x2360480?, 0xc000f8a000?}}, {{{0x2d74240, ...}}, ...}, ...)
                /home/circleci/project/project/internal/plans/objchange/plan_valid.go:57 +0x3b2
        github.com/hashicorp/terraform/internal/plans/objchange.AssertPlanValid(...)
                /home/circleci/project/project/internal/plans/objchange/plan_valid.go:36
        github.com/hashicorp/terraform/internal/terraform.(*NodeAbstractResourceInstance).plan(0xc000357680, {0x2d8bfd8, 0xc0002af5e0}, 0x0, 0xc000980ba0, 0x0, {0x0, 0x0, 0x1?})
                /home/circleci/project/project/internal/terraform/node_resource_abstract_instance.go:830 +0x19fe
        github.com/hashicorp/terraform/internal/terraform.(*NodePlannableResourceInstance).managedResourceExecute(0xc000a01dc0, {0x2d8bfd8, 0xc0002af5e0})
                /home/circleci/project/project/internal/terraform/node_resource_plan_instance.go:223 +0xdae
        github.com/hashicorp/terraform/internal/terraform.(*NodePlannableResourceInstance).Execute(0x0?, {0x2d8bfd8?, 0xc0002af5e0?}, 0x0?)
                /home/circleci/project/project/internal/terraform/node_resource_plan_instance.go:60 +0x90
        github.com/hashicorp/terraform/internal/terraform.(*ContextGraphWalker).Execute(0xc000aba000, {0x2d8bfd8, 0xc0002af5e0}, {0x7f2c72185f68, 0xc000a01dc0})
                /home/circleci/project/project/internal/terraform/graph_walk_context.go:136 +0xc2
        github.com/hashicorp/terraform/internal/terraform.(*Graph).walk.func1({0x26e9960, 0xc000a01dc0})
                /home/circleci/project/project/internal/terraform/graph.go:75 +0x315
        github.com/hashicorp/terraform/internal/dag.(*Walker).walkVertex(0xc000980120, {0x26e9960, 0xc000a01dc0}, 0xc000a01f80)
                /home/circleci/project/project/internal/dag/walk.go:381 +0x2f6
        created by github.com/hashicorp/terraform/internal/dag.(*Walker).Update
                /home/circleci/project/project/internal/dag/walk.go:304 +0xf65
--- FAIL: TestAccLabResource (3.63s)
FAIL
FAIL    github.com/rschmied/terraform-provider-cml2/internal/provider/resource/lab      3.634s
FAIL

Steps to Reproduce

Testing this requires the provider to talk to some on-Prem software -- so, it's kind of difficult to reproduce without access to said software. It's entirely possible that the code does something utterly stupid. However, setting the Set to Unknown should NOT crash Terraform.

References

@rschmied rschmied added the bug Something isn't working label Mar 30, 2023
bflad added a commit to bflad/terraform-provider-framework that referenced this issue Mar 31, 2023
Reference: hashicorp/terraform-plugin-framework#709

```
Running tool: /opt/homebrew/bin/go test -timeout 30s -run ^TestSetUnknownResource_basic$ github.com/bflad/terraform-provider-framework/internal/provider

--- FAIL: TestSetUnknownResource_basic (0.22s)
    /Users/bflad/src/github.com/bflad/terraform-provider-framework/internal/provider/set_unknown_resource_test.go:10: Step 1/2 error: Error running pre-apply refresh: exit status 11

        !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

        Terraform crashed! This is always indicative of a bug within Terraform.
        Please report the crash with Terraform[1] so that we can fix this.

        When reporting bugs, please include your terraform version, the stack trace
        shown below, and any additional information which may help replicate the issue.

        [1]: https://github.com/hashicorp/terraform/issues

        !!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

        value is not known
        goroutine 78 [running]:
        runtime/debug.Stack()
        	/usr/local/go/src/runtime/debug/stack.go:24 +0x64
        runtime/debug.PrintStack()
        	/usr/local/go/src/runtime/debug/stack.go:16 +0x1c
        github.com/hashicorp/terraform/internal/logging.PanicHandler()
        	/Users/distiller/project/project/internal/logging/panic.go:55 +0x170
        panic({0x1065ae5e0, 0x106a6f0a0})
        	/usr/local/go/src/runtime/panic.go:890 +0x258
        github.com/zclconf/go-cty/cty.Value.LengthInt({{{0x106a99e78?, 0x14000c04180?}}, {0x106662b20?, 0x107e932a0?}})
        	/Users/distiller/go/pkg/mod/github.com/zclconf/go-cty@v1.12.1/cty/value_ops.go:1063 +0x218
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedObjectValid(0x1400040de40, {{{0x0?, 0x0?}}, {0x0?, 0x0?}}, {{{0x106a99e78?, 0x14000c042b0?}}, {0x10699b560?, 0x14000655518?}}, {{{0x106a99e78?, ...}}, ...}, ...)
        	/Users/distiller/project/project/internal/plans/objchange/plan_valid.go:421 +0x418
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedValueValid(0x140004150c0, {{{0x0?, 0x0?}}, {0x0?, 0x0?}}, {{{0x106a99e78?, 0x14000c042b0?}}, {0x10699b560?, 0x14000655518?}}, {{{0x106a99e78?, ...}}, ...}, ...)
        	/Users/distiller/project/project/internal/plans/objchange/plan_valid.go:308 +0x758
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedAttrValid({0x14000406030, 0x14}, 0x140004150c0, {{{0x106a99e40?, 0x1400089e970?}}, {0x0?, 0x0?}}, {{{0x106a99e40?, 0x14000c042e0?}}, {0x1066907e0?, ...}}, ...)
        	/Users/distiller/project/project/internal/plans/objchange/plan_valid.go:264 +0x234
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlannedAttrsValid(0x106a99e40?, {{{0x106a99e40?, 0x1400089e970?}}, {0x0?, 0x0?}}, {{{0x106a99e40?, 0x14000c042e0?}}, {0x1066907e0?, 0x140008ed1d0?}}, {{{0x106a99e40?, ...}}, ...}, ...)
        	/Users/distiller/project/project/internal/plans/objchange/plan_valid.go:249 +0x188
        github.com/hashicorp/terraform/internal/plans/objchange.assertPlanValid(0x140004093b0, {{{0x106a99e40?, 0x1400089e970?}}, {0x0?, 0x0?}}, {{{0x106a99e40?, 0x14000c042e0?}}, {0x1066907e0?, 0x140008ed1d0?}}, {{{0x106a99e40?, ...}}, ...}, ...)
        	/Users/distiller/project/project/internal/plans/objchange/plan_valid.go:57 +0x2ac
        github.com/hashicorp/terraform/internal/plans/objchange.AssertPlanValid(...)
        	/Users/distiller/project/project/internal/plans/objchange/plan_valid.go:36
        github.com/hashicorp/terraform/internal/terraform.(*NodeAbstractResourceInstance).plan(0x140001603c0, {0x106aaf6f8, 0x140009e4380}, 0x0, 0x0, 0x0, {0x0, 0x0, 0x0?})
        	/Users/distiller/project/project/internal/terraform/node_resource_abstract_instance.go:830 +0x153c
        github.com/hashicorp/terraform/internal/terraform.(*NodePlannableResourceInstance).managedResourceExecute(0x1400064f940, {0x106aaf6f8, 0x140009e4380})
        	/Users/distiller/project/project/internal/terraform/node_resource_plan_instance.go:223 +0xa6c
        github.com/hashicorp/terraform/internal/terraform.(*NodePlannableResourceInstance).Execute(0x0?, {0x106aaf6f8?, 0x140009e4380?}, 0x80?)
        	/Users/distiller/project/project/internal/terraform/node_resource_plan_instance.go:60 +0x7c
        github.com/hashicorp/terraform/internal/terraform.(*ContextGraphWalker).Execute(0x140009ea0f0, {0x106aaf6f8, 0x140009e4380}, {0x11001f230, 0x1400064f940})
        	/Users/distiller/project/project/internal/terraform/graph_walk_context.go:136 +0xa8
        github.com/hashicorp/terraform/internal/terraform.(*Graph).walk.func1({0x106a1aec0, 0x1400064f940})
        	/Users/distiller/project/project/internal/terraform/graph.go:75 +0x238
        github.com/hashicorp/terraform/internal/dag.(*Walker).walkVertex(0x1400081e600, {0x106a1aec0, 0x1400064f940}, 0x1400064fa40)
        	/Users/distiller/project/project/internal/dag/walk.go:381 +0x2e0
        created by github.com/hashicorp/terraform/internal/dag.(*Walker).Update
        	/Users/distiller/project/project/internal/dag/walk.go:304 +0xbf0
FAIL
FAIL	github.com/bflad/terraform-provider-framework/internal/provider	1.002s
FAIL
```
bflad added a commit to bflad/terraform-provider-framework that referenced this issue Mar 31, 2023
… and In-Place Update

Reference: hashicorp/terraform-plugin-framework#709

```
Running tool: /opt/homebrew/bin/go test -timeout 30s -run ^TestSetUnknownResource_basic$ github.com/bflad/terraform-provider-framework/internal/provider

--- FAIL: TestSetUnknownResource_basic (0.63s)
    /Users/bflad/src/github.com/bflad/terraform-provider-framework/internal/provider/set_unknown_resource_test.go:10: Step 2/2 error: Error running apply: exit status 1

        Error: Provider produced inconsistent result after apply

        When applying changes to framework_set_unknown.test, provider
        "provider[\"registry.terraform.io/hashicorp/framework\"]" produced an
        unexpected new value: .set_nested_attribute: planned set element
        cty.ObjectVal(map[string]cty.Value{"id":cty.StringVal("id-456"),
        "name":cty.StringVal("name-123"), "permission":cty.StringVal("permission2")})
        does not correlate with any element in actual.

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
FAIL
FAIL	github.com/bflad/terraform-provider-framework/internal/provider	1.525s
FAIL
```
bflad added a commit that referenced this issue Mar 31, 2023
…ta misalignment

Reference: #709

The new unit tests reproduce the issue, but are not necessarily indicative of the expected solution. It is more likely that the planning logic either needs to return null values when list/sets have elements removed/updated (added is fine), making `UseStateForUnknown()`-like plan modifiers unable to fetch incorrect values, and/or have certain plan modifiers potentially raise implementation errors when implemented underneath lists/sets.

```
--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-nested-usestateforunknown-elements-rearranged (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-nested-usestateforunknown-elements-removed (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-nested-usestateforunknown-elements-removed (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-nested-usestateforunknown-elements-rearranged (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-nested-usestateforunknown-elements-removed (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-nested-usestateforunknown-elements-removed (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-list-nested-usestateforunknown-elements-rearranged (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-nested-usestateforunknown-elements-rearranged (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
```
@bflad
Copy link
Contributor

bflad commented Mar 31, 2023

Hi @rschmied 👋 Thank you for reporting these issues and sorry you are running into this unexpected behavior.

In an attempt to immediately unblock you, can you try removing the UseStateForUnknown() plan modifiers from the lab group nested attributes? This should at least prevent the framework logic from causing your plans to raise the Provider produced inconsistent result after apply Terraform error. There a few considerations the maintainers will need to work through to determine if a fix can be made for the underlying issue without breaking compatibility or if plan modifiers such as UseStateForUnknown() will need to raise an implementation error diagnostic to prevent their usage when under a list/set.

After some initial investigation, it appears the following setup is problematic:

  • Attribute-based plan modifiers which attempt to read prior state (such as UseStateForUnknown())
  • Implementing those attribute-based plan modifiers on attributes underneath a schema.ListNestedAttribute, schema.ListNestedBlock, schema.SetNestedAttribute, or schema.SetNestedBlock
  • The resource is planning/applying an in-place update which rearranges and/or removes those elements within the list/set

For the Terraform core panic you mention, I have created a small reproduction and submitted a bug report to Terraform core. It is always invalid for a provider to attempt to set a known configuration value to unknown in a plan response. The framework doesn't have guardrails to raise its own error in this situation and Terraform core is currently raising a panic instead of a proper error (which will be the fix over there). Depending on the clarity of the future Terraform core error, the maintainers here can make a determination whether the framework should implement duplicate detection logic to raise a potentially clearer error.

@bflad bflad changed the title SetUnknown in ModifyPlan crashes Terraform Unexpected Behavior With Plan Modifier (UseStateForUnknown) Prior State Under Lists and Sets Apr 3, 2023
@bflad bflad added this to the v1.3.0 milestone Apr 3, 2023
@rschmied
Copy link
Author

rschmied commented Apr 6, 2023

Thanks for looking into this -- I am currently on PTO and will try your proposed workaround when I am back.

bflad added a commit that referenced this issue Apr 6, 2023
…ing implementation error when under a list or set

Reference: #709
Reference: #717

This change will adjust the `UseStateForUnknown` plan modifiers to return an implementation error if defined on an attribute below a list or set. In any cases where the list or set elements were rearranged or removed, the plan modifier result could cause invalid data being proposed in the plan or Terraform data consistency errors. These situations require additional value identifying information from the provider developer to ensure that the prior state for an element is correctly realigned, however the `UseStateForUnknown` plan modifier takes no parameters for such information and adding those parameters would represent a breaking change to the function signature. Future framework proposals will investigate whether a second plan modifier can be introduced to cover this situation.

The new plan modification unit tests reproduced the issue prior to the new implementation error:

```
--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-nested-usestateforunknown-elements-rearranged (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-nested-usestateforunknown-elements-removed (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-nested-usestateforunknown-elements-removed (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-nested-usestateforunknown-elements-rearranged (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-nested-usestateforunknown-elements-removed (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-nested-usestateforunknown-elements-removed (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-list-nested-usestateforunknown-elements-rearranged (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-nested-usestateforunknown-elements-rearranged (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
```
@bflad bflad closed this as completed in #711 Apr 6, 2023
bflad added a commit that referenced this issue Apr 6, 2023
…ing implementation error when under a list or set (#711)

Reference: #709
Reference: #717

This change will adjust the `UseStateForUnknown` plan modifiers to return an implementation error if defined on an attribute below a list or set. In any cases where the list or set elements were rearranged or removed, the plan modifier result could cause invalid data being proposed in the plan or Terraform data consistency errors. These situations require additional value identifying information from the provider developer to ensure that the prior state for an element is correctly realigned, however the `UseStateForUnknown` plan modifier takes no parameters for such information and adding those parameters would represent a breaking change to the function signature. Future framework proposals will investigate whether a second plan modifier can be introduced to cover this situation.

The new plan modification unit tests reproduced the issue prior to the new implementation error:

```
--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-nested-usestateforunknown-elements-rearranged (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-set-nested-nested-usestateforunknown-elements-removed (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-nested-usestateforunknown-elements-removed (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-nested-usestateforunknown-elements-rearranged (0.00s)
        attribute_plan_modification_test.go:1627: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-nested-usestateforunknown-elements-removed (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-nested-usestateforunknown-elements-removed (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-list-nested-usestateforunknown-elements-rearranged (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
    --- FAIL: TestBlockModifyPlan/block-set-nested-usestateforunknown-elements-rearranged (0.00s)
        block_plan_modification_test.go:2858: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   s`[{"nested_computed":"statevalue2","nested_required":"testvalue2"},{"nested_computed":"statevalue1","nested_required":"testvalue1"}]`,
            + 	AttributePlan:   s`[{"nested_computed":"statevalue1","nested_required":"testvalue2"},{"nested_computed":"statevalue2","nested_required":"testvalue1"}]`,
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
```

Co-authored-by: Austin Valle <austinvalle@gmail.com>
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2023
@bflad bflad modified the milestones: v1.3.0, v1.4.0 Jun 6, 2023
@bflad
Copy link
Contributor

bflad commented Jun 6, 2023

I'm reopening this to signify that the maintainers were not able to rectify the second half of the changes needed before releasing v1.3.0 soon, so v1.3.0 will work with the same (potentially confusing) behavior as before. The hope is that this can be fully addressed in a subsequent release, but there is no timeline yet for that effort.

@bflad bflad reopened this Jun 6, 2023
@hashicorp hashicorp unlocked this conversation Jun 6, 2023
bflad added a commit that referenced this issue Jun 6, 2023
…by raising implementation error when under a list or set

Reference: #709
Reference: #711

This unblocks the v1.3.0 release by reverting the `UseStateForUnknown` implementation errors introduced as an interim solution to the originally reported data alignment issue with the plan modifier. Leaving the half state of the `UseStateForUnknown` implementation error would leave provider developers in an unfortunately much worse situation where existing implementations would break without a straightforward path to fixing their schema implementations. The framework needs additional reworking of plan data (potentially reimplementing some of the Terraform core logic in this regard) as a fuller solution for the issue, rather than trying to shoe-horn a solution only for framework-defined plan modifiers.

This changeset reverts a lot of the prior code changes that were part of the interim implementation error logic, however it does notably leave the additional unit testing that highlights the current (sometimes unfortunate) behavior of the `UseStateForUnknown` plan modifier so that future efforts can show the expected behavior changes of updating the framework planning logic.
bflad added a commit that referenced this issue Jun 7, 2023
…by raising implementation error when under a list or set (#750)

Reference: #709
Reference: #711

This unblocks the v1.3.0 release by reverting the `UseStateForUnknown` implementation errors introduced as an interim solution to the originally reported data alignment issue with the plan modifier. Leaving the half state of the `UseStateForUnknown` implementation error would leave provider developers in an unfortunately much worse situation where existing implementations would break without a straightforward path to fixing their schema implementations. The framework needs additional reworking of plan data (potentially reimplementing some of the Terraform core logic in this regard) as a fuller solution for the issue, rather than trying to shoe-horn a solution only for framework-defined plan modifiers.

This changeset reverts a lot of the prior code changes that were part of the interim implementation error logic, however it does notably leave the additional unit testing that highlights the current (sometimes unfortunate) behavior of the `UseStateForUnknown` plan modifier so that future efforts can show the expected behavior changes of updating the framework planning logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants