From ad3d0d43c335618563942e8b0125b300052cf86b Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 9 Jan 2025 14:26:12 +0000 Subject: [PATCH 1/8] Check for non-null WO attrs when a state upgrade takes place --- internal/terraform/upgrade_resource_state.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/terraform/upgrade_resource_state.go b/internal/terraform/upgrade_resource_state.go index c133f483b2ad..f8e84a029b9d 100644 --- a/internal/terraform/upgrade_resource_state.go +++ b/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 From c12fb3211dc2beea69e972903b724083de08e600 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 9 Jan 2025 14:29:45 +0000 Subject: [PATCH 2/8] wip: testing new validation --- internal/terraform/context_plan2_test.go | 42 ++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index e3bf573c72c8..4f99ad4c6a67 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -6068,3 +6068,45 @@ data "test_data_source" "foo" { _, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) assertNoErrors(t, diags) } + +func TestContext2Plan_foo(t *testing.T) { + m := testModule(t, "plan-good") + p := testProvider("aws") + + // Simplify the test provider to contain a minimal resource with a write-only attribute + 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{ + // TODO: Need mocked state here that includes an instance of `aws_instance` resource + // with a non-null value for the wo_attr field. + UpgradedState: cty.Value{}, + } + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + // Plan should invoke state upgrade logic and trigger validation, given the mocks above + _, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + + // TODO: Add more assertions here about how we expect errors about non-null values + if !diags.HasErrors() { + t.Fatalf("expected errors but got none") + } +} From 048868ffdbb84c6cfbe17fd2c5bbb60da156421a Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Thu, 9 Jan 2025 14:59:43 +0000 Subject: [PATCH 3/8] update test and let it pass --- internal/terraform/context_plan2_test.go | 40 +++++++++++++++++------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 4f99ad4c6a67..bfff7de9ad34 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -6069,11 +6069,16 @@ data "test_data_source" "foo" { assertNoErrors(t, diags) } -func TestContext2Plan_foo(t *testing.T) { - m := testModule(t, "plan-good") - p := testProvider("aws") +func TestContext2Plan_upgradeState_WriteOnlyAttribute(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "aws_instance" "foo" { + wo_attr = "value" +} +`, + }) - // Simplify the test provider to contain a minimal resource with a write-only attribute + p := testProvider("aws") p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{ ResourceTypes: map[string]*configschema.Block{ "aws_instance": { @@ -6087,12 +6092,11 @@ func TestContext2Plan_foo(t *testing.T) { }, }, }) - p.UpgradeResourceStateFn = func(r providers.UpgradeResourceStateRequest) (resp providers.UpgradeResourceStateResponse) { return providers.UpgradeResourceStateResponse{ - // TODO: Need mocked state here that includes an instance of `aws_instance` resource - // with a non-null value for the wo_attr field. - UpgradedState: cty.Value{}, + UpgradedState: cty.ObjectVal(map[string]cty.Value{ + "wo_attr": cty.StringVal("not-empty"), + }), } } @@ -6102,11 +6106,25 @@ func TestContext2Plan_foo(t *testing.T) { }, }) - // Plan should invoke state upgrade logic and trigger validation, given the mocks above - _, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + 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"]`), + ) + }) - // TODO: Add more assertions here about how we expect errors about non-null values + // 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) + } } From 8a2dbafbe3b17c37b98844d3aa4abe4bb1d96a86 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Fri, 10 Jan 2025 13:48:47 +0000 Subject: [PATCH 4/8] Add change file --- .changes/unreleased/ENHANCEMENTS-20250110-134832.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/unreleased/ENHANCEMENTS-20250110-134832.yaml diff --git a/.changes/unreleased/ENHANCEMENTS-20250110-134832.yaml b/.changes/unreleased/ENHANCEMENTS-20250110-134832.yaml new file mode 100644 index 000000000000..26242e0b6fae --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-20250110-134832.yaml @@ -0,0 +1,5 @@ +kind: ENHANCEMENTS +body: Added validation that prevented providers returning non-null values for write-only attributes when upgrading resource state +time: 2025-01-10T13:48:32.210045Z +custom: + Issue: "36305" From 4e51fa308a0ebe07be89ef72c0307ff68de57fda Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Fri, 10 Jan 2025 15:03:34 +0000 Subject: [PATCH 5/8] fix failing test --- internal/terraform/context_apply_ephemeral_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index 997343dbcd0a..6b92d0cef27d 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -638,6 +638,14 @@ resource "ephem_write_only" "wo" { }, }, } + 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{ From 2d693bfe64fdaceaae100ad49431eedc257c6b3c Mon Sep 17 00:00:00 2001 From: Sarah French Date: Fri, 10 Jan 2025 18:16:04 +0000 Subject: [PATCH 6/8] Add comments about some of the gotchas we encountered --- internal/providers/testing/provider_mock.go | 5 +++++ internal/terraform/context_apply_ephemeral_test.go | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/internal/providers/testing/provider_mock.go b/internal/providers/testing/provider_mock.go index a9d2708651af..56a53cd4dc4a 100644 --- a/internal/providers/testing/provider_mock.go +++ b/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() diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index 6b92d0cef27d..a93cefa125c6 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -638,6 +638,8 @@ 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{ @@ -658,6 +660,8 @@ resource "ephem_write_only" "wo" { SourceType: ValueFromCLIArg, } + // Non-empty prior state is required to trigger UpgradeResourceState logic + // during planning priorState := states.BuildState(func(state *states.SyncState) { state.SetResourceInstanceCurrent( mustResourceInstanceAddr("ephem_write_only.wo"), From 729034500f20e5219da3b04e125b257bdb155078 Mon Sep 17 00:00:00 2001 From: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Mon, 13 Jan 2025 11:11:45 +0000 Subject: [PATCH 7/8] Delete change file --- .changes/unreleased/ENHANCEMENTS-20250110-134832.yaml | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changes/unreleased/ENHANCEMENTS-20250110-134832.yaml diff --git a/.changes/unreleased/ENHANCEMENTS-20250110-134832.yaml b/.changes/unreleased/ENHANCEMENTS-20250110-134832.yaml deleted file mode 100644 index 26242e0b6fae..000000000000 --- a/.changes/unreleased/ENHANCEMENTS-20250110-134832.yaml +++ /dev/null @@ -1,5 +0,0 @@ -kind: ENHANCEMENTS -body: Added validation that prevented providers returning non-null values for write-only attributes when upgrading resource state -time: 2025-01-10T13:48:32.210045Z -custom: - Issue: "36305" From 9ae30c24a16205c2a78707a281c88b50610fe531 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Tue, 14 Jan 2025 12:12:21 +0000 Subject: [PATCH 8/8] Remove comment --- internal/terraform/context_apply_ephemeral_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/terraform/context_apply_ephemeral_test.go b/internal/terraform/context_apply_ephemeral_test.go index a93cefa125c6..a18ce94c7bef 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/internal/terraform/context_apply_ephemeral_test.go @@ -660,8 +660,6 @@ resource "ephem_write_only" "wo" { SourceType: ValueFromCLIArg, } - // Non-empty prior state is required to trigger UpgradeResourceState logic - // during planning priorState := states.BuildState(func(state *states.SyncState) { state.SetResourceInstanceCurrent( mustResourceInstanceAddr("ephem_write_only.wo"),