From e5d4a51ee49a347b40ed73dbcc16bad034eeb8b2 Mon Sep 17 00:00:00 2001 From: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Tue, 14 Jan 2025 14:02:59 +0000 Subject: [PATCH] Validate write-only attributes returned from providers during the UpgradeResourceState 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 --- internal/providers/testing/provider_mock.go | 5 ++ .../terraform/context_apply_ephemeral_test.go | 10 ++++ internal/terraform/context_plan2_test.go | 60 +++++++++++++++++++ internal/terraform/upgrade_resource_state.go | 19 ++++++ 4 files changed, 94 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 997343dbcd0a..a18ce94c7bef 100644 --- a/internal/terraform/context_apply_ephemeral_test.go +++ b/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{ diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index e3bf573c72c8..bfff7de9ad34 100644 --- a/internal/terraform/context_plan2_test.go +++ b/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) + } +} 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