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)