From 6f9df8f5eb388b6e24b14a97082d965ed41e44ef Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Sat, 7 Sep 2024 14:54:32 +0200 Subject: [PATCH] stacks: ensure that all components in state are referenced in configuration (#35677) * stacks: ensure that all components in state are referenced by a component or resource block * fix compile error --- internal/stacks/stackruntime/apply_test.go | 2 - internal/stacks/stackruntime/helper_test.go | 8 +- .../stackruntime/internal/stackeval/stack.go | 80 +++++- internal/stacks/stackruntime/plan_test.go | 239 ++++++++++++++++++ 4 files changed, 321 insertions(+), 8 deletions(-) diff --git a/internal/stacks/stackruntime/apply_test.go b/internal/stacks/stackruntime/apply_test.go index 2d914c41f2..6d3687fd75 100644 --- a/internal/stacks/stackruntime/apply_test.go +++ b/internal/stacks/stackruntime/apply_test.go @@ -3149,8 +3149,6 @@ func TestApply_RemovedBlocks(t *testing.T) { // TODO: Add tests for and implement the following cases: // - Removed and component blocks that target the same instance. // - Edge cases around missing providers and type mismatches. - // - Make sure all instances in state are targeted by a component or a - // removed block. // - Validate what happens when a removed block foreach evaluates to // unknown. // - Add a test for a removed block in an embedded stack. diff --git a/internal/stacks/stackruntime/helper_test.go b/internal/stacks/stackruntime/helper_test.go index 0506c30897..92847611d8 100644 --- a/internal/stacks/stackruntime/helper_test.go +++ b/internal/stacks/stackruntime/helper_test.go @@ -113,12 +113,12 @@ func expectDiagnosticsForTest(t *testing.T, actual tfdiags.Diagnostics, expected t.Errorf("diagnostic [%d] has wrong severity: %s (expected %s)", ix, actual[ix].Severity(), expected[ix].severity) } - if actual[ix].Description().Summary != expected[ix].summary { - t.Errorf("diagnostic [%d] has wrong summary: %s (expected %s)", ix, actual[ix].Description().Summary, expected[ix].summary) + if diff := cmp.Diff(actual[ix].Description().Summary, expected[ix].summary); len(diff) > 0 { + t.Errorf("diagnostic [%d] has wrong summary: %s", ix, diff) } - if actual[ix].Description().Detail != expected[ix].detail { - t.Errorf("diagnostic [%d] has wrong detail: %s (expected %s)", ix, actual[ix].Description().Detail, expected[ix].detail) + if diff := cmp.Diff(actual[ix].Description().Detail, expected[ix].detail); len(diff) > 0 { + t.Errorf("diagnostic [%d] has wrong detail: %s", ix, diff) } } } diff --git a/internal/stacks/stackruntime/internal/stackeval/stack.go b/internal/stacks/stackruntime/internal/stackeval/stack.go index 6639659a7d..098722b309 100644 --- a/internal/stacks/stackruntime/internal/stackeval/stack.go +++ b/internal/stacks/stackruntime/internal/stackeval/stack.go @@ -654,6 +654,8 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd return nil, nil } + var diags tfdiags.Diagnostics + // For a root stack we'll return a PlannedChange for each of the output // values, so the caller can see how these would change if this plan is // applied. @@ -664,6 +666,81 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd panic(fmt.Sprintf("invalid result from Stack.ResultValue: %#v", resultVal)) } + // We want to check that all of the components we have in state are + // targeted by something (either a component or a removed block) in + // the configuration. + // + // The root stack analysis is the best place to do this. We must do this + // during the plan (and not during the analysis) because we may have + // for-each attributes that need to be expanded before we can determine + // if a component is targeted. + + for _, inst := range s.main.PlanPrevState().AllComponentInstances().Elems() { + + if s.main.PlanPrevState().ComponentInstanceResourceInstanceObjects(inst).Len() == 0 { + // Then this component instance doesn't have any resource instances + // associated with it, so it doesn't matter if it is or isn't + // targeted by anything in the configuration. + // + // Perhaps we should modify the applied state to remove empty + // components instead of keeping them around? + continue + } + + stack := s.main.Stack(ctx, inst.Stack, PlanPhase) + if stack == nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Unclaimed component instance", + fmt.Sprintf("The component instance %s is not claimed by any component or removed block in the configuration. Make sure it is instantiated by a component block, or targeted for removal by a removed block.", inst.String()), + )) + continue + } + + component, removed := stack.ApplyableComponents(ctx, inst.Item.Component) + if component != nil { + insts, unknown := component.Instances(ctx, PlanPhase) + if unknown { + // We can't determine if the component is targeted or not. This + // is okay, as any changes to this component will be deferred + // anyway and a follow up plan will then detect the missing + // component if it exists. + continue + } + + if _, exists := insts[inst.Item.Key]; exists { + // This component is targeted by a component block, so we won't + // add an error. + continue + } + } + + if removed != nil { + insts, unknown, _ := removed.Instances(ctx, PlanPhase) + if unknown { + // We can't determine if the component is targeted or not. This + // is okay, as any changes to this component will be deferred + // anyway and a follow up plan will then detect the missing + // component if it exists. + continue + } + + if _, exists := insts[inst.Item.Key]; exists { + // This component is targeted by a removed block, so we won't + // add an error. + continue + } + } + + // Otherwise, we have a component that is not targeted by anything in + // the configuration. We should add an error. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Unclaimed component instance", + fmt.Sprintf("The component instance %s is not claimed by any component or removed block in the configuration. Make sure it is instantiated by a component block, or targeted for removal by a removed block.", inst.String()), + )) + } + var changes []stackplan.PlannedChange for it := resultVal.ElementIterator(); it.Next(); { k, v := it.Element() @@ -681,7 +758,6 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd // Any other marks should've been dealt with by our caller before // getting here, since we only know how to preserve the sensitive // marking. - var diags tfdiags.Diagnostics diags = diags.Append(fmt.Errorf( "%s%s: unhandled value marks %#v (this is a bug in Terraform)", outputAddr, @@ -714,7 +790,7 @@ func (s *Stack) PlanChanges(ctx context.Context) ([]stackplan.PlannedChange, tfd NewValueSensitivePaths: sensitivePaths, }) } - return changes, nil + return changes, diags } // CheckApply implements Applyable. diff --git a/internal/stacks/stackruntime/plan_test.go b/internal/stacks/stackruntime/plan_test.go index 0d45c54352..66ead05a0e 100644 --- a/internal/stacks/stackruntime/plan_test.go +++ b/internal/stacks/stackruntime/plan_test.go @@ -8,12 +8,14 @@ import ( "encoding/json" "fmt" "path" + "path/filepath" "sort" "strings" "testing" "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty-debug/ctydebug" "github.com/zclconf/go-cty/cty" @@ -4248,6 +4250,243 @@ func TestPlan_DependsOnUpdatesRequirements(t *testing.T) { } } +func TestPlan_RemovedBlocks(t *testing.T) { + fakePlanTimestamp, err := time.Parse(time.RFC3339, "1991-08-25T20:57:08Z") + if err != nil { + t.Fatal(err) + } + + lock := depsfile.NewLocks() + lock.SetProvider( + addrs.NewDefaultProvider("testing"), + providerreqs.MustParseVersion("0.0.0"), + providerreqs.MustParseVersionConstraints("=0.0.0"), + providerreqs.PreferredHashes([]providerreqs.Hash{}), + ) + + tcs := map[string]struct { + source string + initialState *stackstate.State + store *stacks_testing_provider.ResourceStore + inputs map[string]cty.Value + wantPlanChanges []stackplan.PlannedChange + wantPlanDiags []expectedDiagnostic + }{ + "orphaned component": { + source: filepath.Join("with-single-input", "removed-component-instance"), + initialState: stackstate.NewStateBuilder(). + AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.self[\"removed\"]")). + AddInputVariable("id", cty.StringVal("removed")). + AddInputVariable("input", cty.StringVal("removed"))). + AddResourceInstance(stackstate.NewResourceInstanceBuilder(). + SetAddr(mustAbsResourceInstanceObject("component.self[\"removed\"].testing_resource.data")). + SetProviderAddr(mustDefaultRootProvider("testing")). + SetResourceInstanceObjectSrc(states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: mustMarshalJSONAttrs(map[string]any{ + "id": "removed", + "value": "removed", + }), + })). + AddComponentInstance(stackstate.NewComponentInstanceBuilder(mustAbsComponentInstance("component.self[\"orphaned\"]")). + AddInputVariable("id", cty.StringVal("orphaned")). + AddInputVariable("input", cty.StringVal("orphaned"))). + AddResourceInstance(stackstate.NewResourceInstanceBuilder(). + SetAddr(mustAbsResourceInstanceObject("component.self[\"orphaned\"].testing_resource.data")). + SetProviderAddr(mustDefaultRootProvider("testing")). + SetResourceInstanceObjectSrc(states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: mustMarshalJSONAttrs(map[string]any{ + "id": "orphaned", + "value": "orphaned", + }), + })). + Build(), + store: stacks_testing_provider.NewResourceStoreBuilder(). + AddResource("removed", cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("removed"), + "value": cty.StringVal("removed"), + })). + AddResource("orphaned", cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("orphaned"), + "value": cty.StringVal("orphaned"), + })). + Build(), + inputs: map[string]cty.Value{ + "input": cty.SetVal([]cty.Value{ + cty.StringVal("added"), + }), + "removed": cty.SetVal([]cty.Value{ + cty.StringVal("removed"), + }), + }, + wantPlanChanges: []stackplan.PlannedChange{ + &stackplan.PlannedChangeApplyable{ + Applyable: false, // No! We have an unclaimed instance! + }, + // we're expecting the new component to be created + &stackplan.PlannedChangeComponentInstance{ + Addr: mustAbsComponentInstance("component.self[\"added\"]"), + PlanComplete: true, + PlanApplyable: true, + Action: plans.Create, + PlannedInputValues: map[string]plans.DynamicValue{ + "id": mustPlanDynamicValueDynamicType(cty.StringVal("added")), + "input": mustPlanDynamicValueDynamicType(cty.StringVal("added")), + }, + PlannedInputValueMarks: map[string][]cty.PathValueMarks{ + "input": nil, + "id": nil, + }, + PlannedOutputValues: make(map[string]cty.Value), + PlannedCheckResults: &states.CheckResults{}, + PlanTimestamp: fakePlanTimestamp, + }, + &stackplan.PlannedChangeResourceInstancePlanned{ + ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.self[\"added\"].testing_resource.data"), + ChangeSrc: &plans.ResourceInstanceChangeSrc{ + Addr: mustAbsResourceInstance("testing_resource.data"), + PrevRunAddr: mustAbsResourceInstance("testing_resource.data"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Create, + Before: mustPlanDynamicValue(cty.NullVal(cty.Object(map[string]cty.Type{ + "id": cty.String, + "value": cty.String, + }))), + After: mustPlanDynamicValue(cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("added"), + "value": cty.StringVal("added"), + })), + }, + ProviderAddr: mustDefaultRootProvider("testing"), + }, + ProviderConfigAddr: mustDefaultRootProvider("testing"), + Schema: stacks_testing_provider.TestingResourceSchema, + }, + &stackplan.PlannedChangeComponentInstance{ + Addr: mustAbsComponentInstance("component.self[\"removed\"]"), + PlanComplete: true, + PlanApplyable: true, + Mode: plans.DestroyMode, + Action: plans.Delete, + PlannedInputValues: map[string]plans.DynamicValue{ + "id": mustPlanDynamicValueDynamicType(cty.StringVal("removed")), + "input": mustPlanDynamicValueDynamicType(cty.StringVal("removed")), + }, + PlannedInputValueMarks: map[string][]cty.PathValueMarks{ + "input": nil, + "id": nil, + }, + PlannedOutputValues: make(map[string]cty.Value), + PlannedCheckResults: &states.CheckResults{}, + PlanTimestamp: fakePlanTimestamp, + }, + &stackplan.PlannedChangeResourceInstancePlanned{ + ResourceInstanceObjectAddr: mustAbsResourceInstanceObject("component.self[\"removed\"].testing_resource.data"), + ChangeSrc: &plans.ResourceInstanceChangeSrc{ + Addr: mustAbsResourceInstance("testing_resource.data"), + PrevRunAddr: mustAbsResourceInstance("testing_resource.data"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Delete, + Before: mustPlanDynamicValue(cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("removed"), + "value": cty.StringVal("removed"), + })), + After: mustPlanDynamicValue(cty.NullVal(cty.Object(map[string]cty.Type{ + "id": cty.String, + "value": cty.String, + }))), + }, + ProviderAddr: mustDefaultRootProvider("testing"), + }, + PriorStateSrc: &states.ResourceInstanceObjectSrc{ + AttrsJSON: mustMarshalJSONAttrs(map[string]any{ + "id": "removed", + "value": "removed", + }), + Dependencies: make([]addrs.ConfigResource, 0), + Status: states.ObjectReady, + }, + ProviderConfigAddr: mustDefaultRootProvider("testing"), + Schema: stacks_testing_provider.TestingResourceSchema, + }, + &stackplan.PlannedChangeHeader{ + TerraformVersion: version.SemVer, + }, + &stackplan.PlannedChangePlannedTimestamp{ + PlannedTimestamp: fakePlanTimestamp, + }, + &stackplan.PlannedChangeRootInputValue{ + Addr: stackaddrs.InputVariable{Name: "input"}, + Value: cty.SetVal([]cty.Value{ + cty.StringVal("added"), + }), + }, + &stackplan.PlannedChangeRootInputValue{ + Addr: stackaddrs.InputVariable{Name: "removed"}, + Value: cty.SetVal([]cty.Value{ + cty.StringVal("removed"), + }), + }, + }, + wantPlanDiags: []expectedDiagnostic{ + { + severity: tfdiags.Error, + summary: "Unclaimed component instance", + detail: "The component instance component.self[\"orphaned\"] is not claimed by any component or removed block in the configuration. Make sure it is instantiated by a component block, or targeted for removal by a removed block.", + }, + }, + }, + } + + for name, tc := range tcs { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + cfg := loadMainBundleConfigForTest(t, tc.source) + + inputs := make(map[stackaddrs.InputVariable]ExternalInputValue, len(tc.inputs)) + for name, input := range tc.inputs { + inputs[stackaddrs.InputVariable{Name: name}] = ExternalInputValue{ + Value: input, + } + } + + providers := map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("testing"): func() (providers.Interface, error) { + return stacks_testing_provider.NewProviderWithData(t, tc.store), nil + }, + } + + planChangesCh := make(chan stackplan.PlannedChange) + planDiagsCh := make(chan tfdiags.Diagnostic) + planReq := PlanRequest{ + Config: cfg, + ProviderFactories: providers, + InputValues: inputs, + ForcePlanTimestamp: &fakePlanTimestamp, + PrevState: tc.initialState, + DependencyLocks: *lock, + } + planResp := PlanResponse{ + PlannedChanges: planChangesCh, + Diagnostics: planDiagsCh, + } + go Plan(ctx, &planReq, &planResp) + gotPlanChanges, gotPlanDiags := collectPlanOutput(planChangesCh, planDiagsCh) + + sort.SliceStable(gotPlanChanges, func(i, j int) bool { + return plannedChangeSortKey(gotPlanChanges[i]) < plannedChangeSortKey(gotPlanChanges[j]) + }) + sort.SliceStable(gotPlanDiags, diagnosticSortFunc(gotPlanDiags)) + + expectDiagnosticsForTest(t, gotPlanDiags, tc.wantPlanDiags...) + if diff := cmp.Diff(tc.wantPlanChanges, gotPlanChanges, ctydebug.CmpOptions, cmpCollectionsSet, cmpopts.IgnoreUnexported(states.ResourceInstanceObjectSrc{})); diff != "" { + t.Errorf("wrong changes\n%s", diff) + } + }) + } +} + // collectPlanOutput consumes the two output channels emitting results from // a call to [Plan], and collects all of the data written to them before // returning once changesCh has been closed by the sender to indicate that