From 7d2d4dec5e91692baaec076dc499198fb2733dae Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 8 Dec 2023 20:56:51 -0800 Subject: [PATCH] stackeval: When being destroyed, component instance result comes from plan Because we treat dependency edges as reversed when a component instance is being destroyed, the final result (an object representing output values) for a component instance being destroyed must not depend on anything else in the evaluation graph, or else we'd cause a promise self-reference as the downstream component tries to configure itself based on our outputs. As a special case then, for a component instance being destroyed we take the planned output values directly from the plan, relying on the fact that the plan phase sets them to the prior state output values in that case, and therefore the result for such a component is available immediately without blocking on any other expression evaluation during the apply phase. This combines with several previous commits to create a first pass at handling ordering correctly when planning and applying a full destroy. This commit also incorporates some fixes and improvements to stackeval's apply-time testing helpers, which had some quirks and bugs when first added in a recent commit. One of those problems also revealed that the raw state loader was not resilient to a buggy caller setting a state entry to nil instead of removing it altogether, and that mistake seems relatively easy to make (as I did here in the test helper) so we'll tolerate it to make it possible to recover if such a bug does end up occurring in real code too. --- .../internal/stackeval/applying_test.go | 172 +++++++++++++----- .../internal/stackeval/component_instance.go | 43 ++++- .../internal/stackeval/testing_test.go | 6 +- internal/stacks/stackstate/from_proto.go | 20 +- 4 files changed, 192 insertions(+), 49 deletions(-) diff --git a/internal/stacks/stackruntime/internal/stackeval/applying_test.go b/internal/stacks/stackruntime/internal/stackeval/applying_test.go index 1c3909e9b1..2c6b23a3da 100644 --- a/internal/stacks/stackruntime/internal/stackeval/applying_test.go +++ b/internal/stacks/stackruntime/internal/stackeval/applying_test.go @@ -12,9 +12,9 @@ import ( "testing" "github.com/davecgh/go-spew/spew" - "github.com/golang/protobuf/proto" "github.com/google/go-cmp/cmp" "github.com/zclconf/go-cty/cty" + "google.golang.org/protobuf/encoding/prototext" "google.golang.org/protobuf/types/known/anypb" "github.com/hashicorp/terraform/internal/addrs" @@ -154,12 +154,12 @@ func TestApply_componentOrdering(t *testing.T) { // then hopefully at least one of the planning-specific tests is also // failing, and will give some more clues as to what's gone wrong here. if !plan.Applyable { - m := proto.TextMarshaler{ - Compact: false, - ExpandAny: true, + m := prototext.MarshalOptions{ + Multiline: true, + Indent: " ", } for _, raw := range rawPlan { - t.Log(m.Text(raw)) + t.Log(m.Format(raw)) } t.Fatalf("plan is not applyable") } @@ -184,6 +184,7 @@ func TestApply_componentOrdering(t *testing.T) { type applyResultData struct { NewRawState map[string]*anypb.Any + NewState *stackstate.State VisitedMarkers []string } @@ -225,11 +226,19 @@ func TestApply_componentOrdering(t *testing.T) { assertNoDiagnostics(t, outpTester.Diags()) + rawState := outpTester.RawUpdatedState(t) + state, diags := outpTester.Close(t) + assertNoDiagnostics(t, diags) + return applyResultData{ - NewRawState: outpTester.RawUpdatedState(t), + NewRawState: rawState, + NewState: state, VisitedMarkers: visitedMarkers, }, nil }) + if err != nil { + t.Fatal(err) + } { if len(applyResult.VisitedMarkers) != 5 { @@ -264,51 +273,128 @@ func TestApply_componentOrdering(t *testing.T) { // If the initial plan and apply was successful and made its changes in // the correct order, then we'll also test creating and applying a // destroy-mode plan. - // FIXME: We can't actually do this yet, because we don't have the logic - // in place for handling component results correctly when destroying. - // We'll complete this in a future commit. - /* - t.Log("destroy plan") - planOutput, err = promising.MainTask(ctx, func(ctx context.Context) (*planOutputTester, error) { - main := NewForPlanning(cfg, stackstate.NewState(), PlanOpts{ - PlanningMode: plans.DestroyMode, - ProviderFactories: ProviderFactories{ - testProviderAddr: func() (providers.Interface, error) { - return &terraform.MockProvider{ - GetProviderSchemaResponse: &testProviderSchema, - PlanResourceChangeFn: func(prcr providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { - return providers.PlanResourceChangeResponse{ - PlannedState: prcr.ProposedNewState, - } - }, - }, nil - }, + t.Log("destroy plan") + planOutput, err = promising.MainTask(ctx, func(ctx context.Context) (*planOutputTester, error) { + main := NewForPlanning(cfg, applyResult.NewState, PlanOpts{ + PlanningMode: plans.DestroyMode, + ProviderFactories: ProviderFactories{ + testProviderAddr: func() (providers.Interface, error) { + return &terraform.MockProvider{ + GetProviderSchemaResponse: &testProviderSchema, + PlanResourceChangeFn: func(prcr providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + return providers.PlanResourceChangeResponse{ + PlannedState: prcr.ProposedNewState, + } + }, + }, nil }, - }) + }, + }) - outp, outpTester := testPlanOutput(t) - main.PlanAll(ctx, outp) + outp, outpTester := testPlanOutput(t) + main.PlanAll(ctx, outp) - return outpTester, nil - }) + return outpTester, nil + }) + if err != nil { + t.Fatalf("planning failed: %s", err) + } + + rawPlan = planOutput.RawChanges(t) + plan, diags = planOutput.Close(t) + assertNoDiagnostics(t, diags) + if !plan.Applyable { + m := prototext.MarshalOptions{ + Multiline: true, + Indent: " ", + } + for _, raw := range rawPlan { + t.Log(m.Format(raw)) + } + t.Fatalf("plan is not applyable") + } + + // When we apply the destroy plan, the components should be visited in + // reverse dependency order to ensure that dependencies outlive their + // dependents. + t.Log("destroy apply") + applyResult, err = promising.MainTask(ctx, func(ctx context.Context) (applyResultData, error) { + var visitedMarkers []string + var visitedMarkersMu sync.Mutex + + outp, outpTester := testApplyOutput(t, nil) + + main, err := ApplyPlan(ctx, cfg, rawPlan, ApplyOpts{ + ProviderFactories: ProviderFactories{ + testProviderAddr: func() (providers.Interface, error) { + return &terraform.MockProvider{ + GetProviderSchemaResponse: &testProviderSchema, + ApplyResourceChangeFn: func(arcr providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + markerStr := arcr.PriorState.GetAttr("marker").AsString() + log.Printf("[TRACE] TestApply_componentOrdering: visiting %q", markerStr) + visitedMarkersMu.Lock() + visitedMarkers = append(visitedMarkers, markerStr) + visitedMarkersMu.Unlock() + + return providers.ApplyResourceChangeResponse{ + NewState: arcr.PlannedState, + } + }, + }, nil + }, + }, + }, outp) + if main != nil { + defer main.DoCleanup(ctx) + } if err != nil { - t.Fatalf("planning failed: %s", err) + t.Fatal(err) } - rawPlan = planOutput.RawChanges(t) - plan, diags = planOutput.Close(t) + assertNoDiagnostics(t, outpTester.Diags()) + + rawState := outpTester.RawUpdatedState(t) + state, diags := outpTester.Close(t) assertNoDiagnostics(t, diags) - if !plan.Applyable { - m := proto.TextMarshaler{ - Compact: false, - ExpandAny: true, - } - for _, raw := range rawPlan { - t.Log(m.Text(raw)) - } - t.Fatalf("plan is not applyable") + + return applyResultData{ + NewRawState: rawState, + NewState: state, + VisitedMarkers: visitedMarkers, + }, nil + }) + if err != nil { + t.Fatal(err) + } + { + if len(applyResult.VisitedMarkers) != 5 { + t.Fatalf("apply didn't visit all of the resources\n%s", spew.Sdump(applyResult.VisitedMarkers)) } - */ + assertSliceElementsInRelativeOrder( + t, applyResult.VisitedMarkers, + "b.i", "a", + ) + assertSliceElementsInRelativeOrder( + t, applyResult.VisitedMarkers, + "b.ii", "a", + ) + assertSliceElementsInRelativeOrder( + t, applyResult.VisitedMarkers, + "b.iii", "a", + ) + assertSliceElementsInRelativeOrder( + t, applyResult.VisitedMarkers, + "c", "b.i", + ) + assertSliceElementsInRelativeOrder( + t, applyResult.VisitedMarkers, + "c", "b.ii", + ) + assertSliceElementsInRelativeOrder( + t, applyResult.VisitedMarkers, + "c", "b.iii", + ) + } } func sliceElementsInRelativeOrder[S ~[]E, E comparable](s S, v1, v2 E) bool { diff --git a/internal/stacks/stackruntime/internal/stackeval/component_instance.go b/internal/stacks/stackruntime/internal/stackeval/component_instance.go index 10d99c371c..fef7b81ea6 100644 --- a/internal/stacks/stackruntime/internal/stackeval/component_instance.go +++ b/internal/stacks/stackruntime/internal/stackeval/component_instance.go @@ -957,7 +957,6 @@ func (c *ComponentInstance) ResultValue(ctx context.Context, phase EvalPhase) ct } attrs[os.Addr.OutputValue.Name] = v } - return cty.ObjectVal(attrs) } if decl := c.call.Config(ctx).ModuleTree(ctx); decl != nil { // If the plan only ran partially then we might be missing @@ -974,10 +973,52 @@ func (c *ComponentInstance) ResultValue(ctx context.Context, phase EvalPhase) ct attrs[name] = cty.DynamicVal } } + // In the DestroyMode case above we might also find ourselves + // with some remnant additional output values that have since + // been removed from the configuration, but yet remain in the + // state. Destroying with a different configuration than was + // most recently applied is not guaranteed to work, but we + // can make it more likely to work by dropping anything that + // isn't currently declared, since referring directly to these + // would be a static validation error anyway, and including + // them might cause aggregate operations like keys(component.foo) + // to produce broken results. + for name := range attrs { + _, declared := decl.Module.Outputs[name] + if !declared { + // (deleting map elements during iteration is valid in Go, + // unlike some other languages.) + delete(attrs, name) + } + } } return cty.ObjectVal(attrs) case ApplyPhase, InspectPhase: + // As a special case, if we're applying and the planned action is + // to destroy then we'll just return the planned output values + // verbatim without waiting for anything, so that downstreams can + // begin their own destroy phases before we start ours. + if phase == ApplyPhase { + fullPlan := c.main.PlanBeingApplied() + ourPlan := fullPlan.Components.Get(c.Addr()) + if ourPlan == nil { + // Weird, but we'll tolerate it. + return cty.DynamicVal + } + if ourPlan.PlannedAction == plans.Delete { + // In this case our result was already decided during the + // planning phase, because we can't block on anything else + // here to make sure we don't create a self-dependency + // while our downstreams are trying to destroy themselves. + attrs := make(map[string]cty.Value, len(ourPlan.PlannedOutputValues)) + for addr, val := range ourPlan.PlannedOutputValues { + attrs[addr.Name] = val + } + return cty.ObjectVal(attrs) + } + } + var state *states.State switch phase { case ApplyPhase: diff --git a/internal/stacks/stackruntime/internal/stackeval/testing_test.go b/internal/stacks/stackruntime/internal/stackeval/testing_test.go index 43f77c4bb6..093f9e1053 100644 --- a/internal/stacks/stackruntime/internal/stackeval/testing_test.go +++ b/internal/stacks/stackruntime/internal/stackeval/testing_test.go @@ -242,7 +242,11 @@ func (aot *applyOutputTester) RawUpdatedState(t *testing.T) map[string]*anypb.An t.Fatalf("failed to encode %T: %s", change, err) } for _, protoRaw := range protoChange.Raw { - msgs[protoRaw.Key] = protoRaw.Value + if protoRaw.Value != nil { + msgs[protoRaw.Key] = protoRaw.Value + } else { + delete(msgs, protoRaw.Key) + } } } diff --git a/internal/stacks/stackstate/from_proto.go b/internal/stacks/stackstate/from_proto.go index 461705a166..2274e3000b 100644 --- a/internal/stacks/stackstate/from_proto.go +++ b/internal/stacks/stackstate/from_proto.go @@ -5,6 +5,12 @@ package stackstate import ( "fmt" + "log" + + "github.com/zclconf/go-cty/cty" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/known/anypb" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/lang/marks" @@ -13,10 +19,6 @@ import ( "github.com/hashicorp/terraform/internal/stacks/stackstate/statekeys" "github.com/hashicorp/terraform/internal/stacks/tfstackdata1" "github.com/hashicorp/terraform/internal/states" - "github.com/zclconf/go-cty/cty" - "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/reflect/protoreflect" - "google.golang.org/protobuf/types/known/anypb" ) // LoadFromProto produces a [State] object by decoding a raw state map. @@ -44,6 +46,16 @@ func LoadFromProto(msgs map[string]*anypb.Any) (*State, error) { continue } + if rawMsg == nil { + // This suggests a state mutation bug where a deleted object was + // written as a map entry without a value, as opposed to deleting + // the value. We tolerate this here just because otherwise it + // would be harder to recover once a state has been mutated + // incorrectly. + log.Panicf("[WARN] stackstate.LoadFromProto: key %s has no associated object; ignoring", rawKey) + continue + } + msg, err := anypb.UnmarshalNew(rawMsg, proto.UnmarshalOptions{}) if err != nil { return nil, fmt.Errorf("invalid raw value for raw state key %q: %w", rawKey, err)