Validate write-only attributes returned from providers during the Upg…

…radeResourceState RPC (#36305)

* Check for non-null WO attrs when a state upgrade takes place

* wip: testing new validation

* update test and let it pass

* Add change file

* fix failing test

* Add comments about some of the gotchas we encountered

* Delete change file

* Remove comment

---------

Co-authored-by: Radek Simko <[email protected]>

internal/providers/testing/provider_mock.go

@@ -252,6 +252,11 @@ func (p *MockProvider) ValidateEphemeralResourceConfig(r providers.ValidateEphem
 	return resp
 }
 
+// UpgradeResourceState mocks out the response from the provider during an UpgradeResourceState RPC
+// The default logic will return the resource's state unchanged, unless other logic is defined on the mock (e.g. UpgradeResourceStateFn)
+//
+// When using this mock you may need to provide custom logic if the plugin-framework alters values in state,
+// e.g. when handling write-only attributes.
 func (p *MockProvider) UpgradeResourceState(r providers.UpgradeResourceStateRequest) (resp providers.UpgradeResourceStateResponse) {
 	p.Lock()
 	defer p.Unlock()

internal/terraform/context_apply_ephemeral_test.go

@@ -638,6 +638,16 @@ resource "ephem_write_only" "wo" {
 			},
 		},
 	}
+	// Below we force the write_only attribute's returned state to be Null, mimicking what the plugin-framework would
+	// return during an UpgradeResourceState RPC
+	ephem.UpgradeResourceStateFn = func(ursr providers.UpgradeResourceStateRequest) providers.UpgradeResourceStateResponse {
+		return providers.UpgradeResourceStateResponse{
+			UpgradedState: cty.ObjectVal(map[string]cty.Value{
+				"normal":     cty.StringVal("normal"),
+				"write_only": cty.NullVal(cty.String),
+			}),
+		}
+	}
 
 	ctx := testContext2(t, &ContextOpts{
 		Providers: map[addrs.Provider]providers.Factory{

internal/terraform/context_plan2_test.go

@@ -6068,3 +6068,63 @@ data "test_data_source" "foo" {
 	_, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 }
+
+func TestContext2Plan_upgradeState_WriteOnlyAttribute(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "aws_instance" "foo" {
+	wo_attr = "value"
+}
+`,
+	})
+
+	p := testProvider("aws")
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"aws_instance": {
+				Attributes: map[string]*configschema.Attribute{
+					"wo_attr": {
+						Type:      cty.String,
+						Optional:  true,
+						WriteOnly: true,
+					},
+				},
+			},
+		},
+	})
+	p.UpgradeResourceStateFn = func(r providers.UpgradeResourceStateRequest) (resp providers.UpgradeResourceStateResponse) {
+		return providers.UpgradeResourceStateResponse{
+			UpgradedState: cty.ObjectVal(map[string]cty.Value{
+				"wo_attr": cty.StringVal("not-empty"),
+			}),
+		}
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p),
+		},
+	})
+
+	priorState := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(
+			mustResourceInstanceAddr("aws_instance.foo"),
+			&states.ResourceInstanceObjectSrc{
+				// The UpgradeResourceStateFn above does not care about specific prior
+				// state but it must not be empty for the function to be actually called
+				AttrsJSON: []byte(`{"wo_attr":null}`),
+				Status:    states.ObjectReady,
+			}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/aws"]`),
+		)
+	})
+
+	// Plan should invoke state upgrade logic and trigger validation, given the mocks above
+	_, diags := ctx.Plan(m, priorState, DefaultPlanOpts)
+	if !diags.HasErrors() {
+		t.Fatalf("expected errors but got none")
+	}
+
+	if got, want := diags.Err().Error(), "Invalid resource state upgrade"; !strings.Contains(got, want) {
+		t.Errorf("unexpected error message\ngot: %s\nwant substring: %s", got, want)
+	}
+}

internal/terraform/upgrade_resource_state.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs/configschema"
+	"github.com/hashicorp/terraform/internal/lang/ephemeral"
 	"github.com/hashicorp/terraform/internal/providers"
 	"github.com/hashicorp/terraform/internal/states"
 	"github.com/hashicorp/terraform/internal/tfdiags"
@@ -121,6 +122,24 @@ func upgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Int
 		return nil, diags
 	}
 
+	// Check for any write-only attributes that have non-null values
+	writeOnlyDiags := ephemeral.ValidateWriteOnlyAttributes(
+		"Invalid resource state upgrade",
+		func(path cty.Path) string {
+			return fmt.Sprintf(
+				"While attempting to upgrade state of resource %s, the provider %q returned a value for the write-only attribute \"%s%s\". Write-only attributes cannot be read back from the provider. This is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				addr, providerType, addr, tfdiags.FormatCtyPath(path),
+			)
+		},
+		newValue,
+		currentSchema,
+	)
+	diags = diags.Append(writeOnlyDiags)
+
+	if writeOnlyDiags.HasErrors() {
+		return nil, diags
+	}
+
 	new, err := src.CompleteUpgrade(newValue, currentSchema.ImpliedType(), uint64(currentVersion))
 	if err != nil {
 		// We already checked for type conformance above, so getting into this