From a45d4467f14981be6de91d110117edb71fb202c4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 23 Feb 2024 12:26:54 -0800 Subject: [PATCH] stackeval: Don't fail when data resource is removed Due to an oversight in our handling of resource instance objects that are neither in configuration nor plan -- which is true for data resources that have since been removed from the configuration -- we were generating plan change objects that were lacking a provider configuration address, which made them syntactically invalid and thus not reloadable using the raw plan parser. This is a bit of a strange situation since we don't technically _need_ a provider configuration address for these; all we're going to do is just unceremoniously delete them from the state during apply anyway. However, we always have the provider configuration address available anyway, so adding this in here is overall simpler than changing the parser, the models it populates, and all of the downstream users of those models to treat this field as optional. This commit is more test case than it is fix, since the fix was relatively straightforward once I had a test case to reproduce the problem it's fixing. --- internal/stacks/stackplan/planned_change.go | 1 + .../internal/stackeval/planning_test.go | 207 +++++++++++++++++- .../step1/remove-data-resource-step1.tf | 11 + .../remove-data-resource-step1.tfstack.hcl | 16 ++ .../step2/remove-data-resource-step2.tf | 10 + .../remove-data-resource-step2.tfstack.hcl | 16 ++ 6 files changed, 257 insertions(+), 4 deletions(-) create mode 100644 internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tf create mode 100644 internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tfstack.hcl create mode 100644 internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tf create mode 100644 internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tfstack.hcl diff --git a/internal/stacks/stackplan/planned_change.go b/internal/stacks/stackplan/planned_change.go index 323972f833..50b977ed1f 100644 --- a/internal/stacks/stackplan/planned_change.go +++ b/internal/stacks/stackplan/planned_change.go @@ -274,6 +274,7 @@ func (pc *PlannedChangeResourceInstancePlanned) PlannedChangeProto() (*terraform ComponentInstanceAddr: rioAddr.Component.String(), ResourceInstanceAddr: rioAddr.Item.ResourceInstance.String(), DeposedKey: rioAddr.Item.DeposedKey.String(), + ProviderConfigAddr: pc.ProviderConfigAddr.String(), }, proto.MarshalOptions{}) if err != nil { return nil, err diff --git a/internal/stacks/stackruntime/internal/stackeval/planning_test.go b/internal/stacks/stackruntime/internal/stackeval/planning_test.go index f949e43967..c61d72c876 100644 --- a/internal/stacks/stackruntime/internal/stackeval/planning_test.go +++ b/internal/stacks/stackruntime/internal/stackeval/planning_test.go @@ -13,6 +13,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/zclconf/go-cty/cty" "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/known/anypb" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/collections" @@ -20,7 +21,7 @@ import ( "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/promising" "github.com/hashicorp/terraform/internal/providers" - testing_provider "github.com/hashicorp/terraform/internal/providers/testing" + providerTesting "github.com/hashicorp/terraform/internal/providers/testing" "github.com/hashicorp/terraform/internal/stacks/stackaddrs" "github.com/hashicorp/terraform/internal/stacks/stackplan" "github.com/hashicorp/terraform/internal/stacks/stackstate" @@ -156,7 +157,7 @@ func TestPlanning_DestroyMode(t *testing.T) { PlanningMode: plans.DestroyMode, ProviderFactories: ProviderFactories{ addrs.NewBuiltInProvider("test"): func() (providers.Interface, error) { - return &testing_provider.MockProvider{ + return &providerTesting.MockProvider{ GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ Provider: providers.Schema{ Block: &configschema.Block{}, @@ -343,7 +344,7 @@ func TestPlanning_RequiredComponents(t *testing.T) { PlanningMode: plans.NormalMode, ProviderFactories: ProviderFactories{ addrs.NewBuiltInProvider("foo"): func() (providers.Interface, error) { - return &testing_provider.MockProvider{ + return &providerTesting.MockProvider{ GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ Provider: providers.Schema{ Block: &configschema.Block{ @@ -601,7 +602,7 @@ func TestPlanning_DeferredChangesPropagation(t *testing.T) { }, ProviderFactories: ProviderFactories{ addrs.NewBuiltInProvider("test"): func() (providers.Interface, error) { - return &testing_provider.MockProvider{ + return &providerTesting.MockProvider{ GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ Provider: providers.Schema{ Block: &configschema.Block{}, @@ -681,6 +682,204 @@ func TestPlanning_DeferredChangesPropagation(t *testing.T) { }) } +func TestPlanning_RemoveDataResource(t *testing.T) { + // This test is here because there was a historical bug where we'd generate + // an invalid plan (unparsable) whenever the plan included deletion of + // a previously-declared data resource, where the provider configuration + // address would not be populated correctly. + // + // Therefore this test is narrowly focused on that specific situation. + // Anything else it's exercising as a side-effect is not crucial for + // this test in particular, although of course unrelated regressions might + // still be important in some other way beyond this test's scope. + + providerFactories := map[addrs.Provider]providers.Factory{ + addrs.NewBuiltInProvider("test"): func() (providers.Interface, error) { + return &providerTesting.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + DataSources: map[string]providers.Schema{ + "test": { + Block: &configschema.Block{}, + }, + }, + }, + ReadDataSourceFn: func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + return providers.ReadDataSourceResponse{ + State: cty.EmptyObjectVal, + } + }, + }, nil + }, + } + objAddr := stackaddrs.AbsResourceInstanceObject{ + Component: stackaddrs.AbsComponentInstance{ + Stack: stackaddrs.RootStackInstance, + Item: stackaddrs.ComponentInstance{ + Component: stackaddrs.Component{Name: "main"}, + }, + }, + Item: addrs.AbsResourceInstanceObject{ + ResourceInstance: addrs.AbsResourceInstance{ + Module: addrs.RootModuleInstance, + Resource: addrs.ResourceInstance{ + Resource: addrs.Resource{ + Mode: addrs.DataResourceMode, + Type: "test", + Name: "test", + }, + }, + }, + DeposedKey: addrs.NotDeposed, + }, + } + + var state *stackstate.State + + // Round 1: data.test.test is present inside component.main + { + ctx := context.Background() + cfg := testStackConfig(t, "planning", "remove_data_resource/step1") + + // Plan + rawPlan, err := promising.MainTask(ctx, func(ctx context.Context) ([]*anypb.Any, error) { + main := NewForPlanning(cfg, stackstate.NewState(), PlanOpts{ + PlanningMode: plans.NormalMode, + ProviderFactories: providerFactories, + }) + outp, outpTest := testPlanOutput(t) + main.PlanAll(ctx, outp) + rawPlan := outpTest.RawChanges(t) + _, diags := outpTest.Close(t) + assertNoDiagnostics(t, diags) + return rawPlan, nil + }) + if err != nil { + t.Fatal(err) + } + + // Apply + newState, err := promising.MainTask(ctx, func(ctx context.Context) (*stackstate.State, error) { + outp, outpTest := testApplyOutput(t, nil) + _, err := ApplyPlan(ctx, cfg, rawPlan, ApplyOpts{ + ProviderFactories: providerFactories, + }, outp) + if err != nil { + t.Fatal(err) + } + state, diags := outpTest.Close(t) + assertNoDiagnostics(t, diags) + + // This test is only valid if the data resource instance is actually + // tracked in the state. + obj := state.ResourceInstanceObjectSrc(objAddr) + if obj == nil { + t.Fatalf("data.test.test is not in the final state for round 1") + } + + return state, nil + }) + if err != nil { + t.Fatal(err) + } + + // We'll use the new state as the input for the next round. + state = newState + } + + // Round 2: data.test.test has its remnant left in the prior state, but + // it's no longer present in the configuration. + { + ctx := context.Background() + cfg := testStackConfig(t, "planning", "remove_data_resource/step2") + + // Plan + type Plans struct { + Nice *stackplan.Plan + Raw []*anypb.Any + } + plans, err := promising.MainTask(ctx, func(ctx context.Context) (Plans, error) { + main := NewForPlanning(cfg, state, PlanOpts{ + PlanningMode: plans.NormalMode, + ProviderFactories: providerFactories, + }) + outp, outpTest := testPlanOutput(t) + main.PlanAll(ctx, outp) + rawPlan := outpTest.RawChanges(t) + // The original bug would occur at this point, because + // outpTest.Close attempts to parse the raw plan, which fails if + // any part of that structure is not syntactically valid. + plan, diags := outpTest.Close(t) + assertNoDiagnostics(t, diags) + return Plans{plan, rawPlan}, nil + }) + if err != nil { + t.Fatal(err) + } + plan := plans.Nice + rawPlan := plans.Raw + + // We'll check whether the data resource even appears in the plan, + // because if not then this test is no longer testing what it thinks + // it's testing and should probably be revised. + // + // (That doesn't necessarily mean that any new behavior is wrong: if + // plan at all anymore then we can update this test to agree with that.) + // + // Specifically we expect to have a prior state and a provider config + // address for this data resource, but no planned action because + // dropping a data resource from the state is not an "action" in the + // usual sense (it doesn't cause any calls to the provider). + mainPlan := plan.Components.Get(stackaddrs.AbsComponentInstance{ + Stack: stackaddrs.RootStackInstance, + Item: stackaddrs.ComponentInstance{ + Component: stackaddrs.Component{Name: "main"}, + }, + }) + if mainPlan == nil { + t.Fatalf("main component not appear in the plan at all") + } + riAddr := objAddr.Item + _, ok := mainPlan.ResourceInstancePriorState.GetOk(riAddr) + if !ok { + t.Fatalf("data resource instance does not appear in the prior state at all") + } + providerConfig, ok := mainPlan.ResourceInstanceProviderConfig.GetOk(riAddr) + if !ok { + t.Fatalf("data resource instance does not have a provider config in the plan") + } + if got, want := providerConfig.Provider, addrs.NewBuiltInProvider("test"); got != want { + t.Errorf("wrong provider configuration address\ngot: %s\nwant: %s", got, want) + } + + // For good measure we'll also apply this new plan, to make sure that + // we're left with no remnant of the data resource in the updated state. + newState, err := promising.MainTask(ctx, func(ctx context.Context) (*stackstate.State, error) { + outp, outpTest := testApplyOutput(t, nil) + _, err := ApplyPlan(ctx, cfg, rawPlan, ApplyOpts{ + ProviderFactories: providerFactories, + }, outp) + if err != nil { + t.Fatal(err) + } + state, diags := outpTest.Close(t) + assertNoDiagnostics(t, diags) + + return state, nil + }) + if err != nil { + t.Fatal(err) + } + + state = newState + } + + // Our final state should not include the data resource at all. + objState := state.ResourceInstanceObjectSrc(objAddr) + if objState != nil { + t.Errorf("%s is still in the state after it should've been dropped", objAddr) + } +} + func TestPlanning_NoWorkspaceNameRef(t *testing.T) { // This test verifies that a reference to terraform.workspace is treated // as invalid for modules used in a stacks context, because there's diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tf b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tf new file mode 100644 index 0000000000..430b5bf3d6 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tf @@ -0,0 +1,11 @@ + +terraform { + required_providers { + test = { + source = "terraform.io/builtin/test" + } + } +} + +data "test" "test" { +} diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tfstack.hcl b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tfstack.hcl new file mode 100644 index 0000000000..a7aca6df80 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step1/remove-data-resource-step1.tfstack.hcl @@ -0,0 +1,16 @@ +required_providers { + test = { + source = "terraform.io/builtin/test" + } +} + +provider "test" "main" { +} + +component "main" { + source = "./" + + providers = { + test = provider.test.main + } +} diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tf b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tf new file mode 100644 index 0000000000..8e75ad23f6 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tf @@ -0,0 +1,10 @@ + +terraform { + required_providers { + test = { + source = "terraform.io/builtin/test" + } + } +} + +# data.test.test is now gone! diff --git a/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tfstack.hcl b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tfstack.hcl new file mode 100644 index 0000000000..a7aca6df80 --- /dev/null +++ b/internal/stacks/stackruntime/internal/stackeval/testdata/sourcebundle/planning/remove_data_resource/step2/remove-data-resource-step2.tfstack.hcl @@ -0,0 +1,16 @@ +required_providers { + test = { + source = "terraform.io/builtin/test" + } +} + +provider "test" "main" { +} + +component "main" { + source = "./" + + providers = { + test = provider.test.main + } +}