diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 800810981d..178a336af0 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -2,9 +2,11 @@ package refactoring import ( "fmt" + "log" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/dag" + "github.com/hashicorp/terraform/internal/logging" "github.com/hashicorp/terraform/internal/states" ) @@ -53,6 +55,13 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] } } + if startNodes.Len() == 0 { + log.Println("[TRACE] refactoring.ApplyMoves: No 'moved' statements to consider in this configuration") + return results + } + + log.Printf("[TRACE] refactoring.ApplyMoves: Processing 'moved' statements in the configuration\n%s", logging.Indent(g.String())) + g.ReverseDepthFirstWalk(startNodes, func(v dag.Vertex, depth int) error { stmt := v.(*MoveStatement) @@ -69,6 +78,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] // For a module endpoint we just try the module address // directly. if newAddr, matches := modAddr.MoveDestination(stmt.From, stmt.To); matches { + log.Printf("[TRACE] refactoring.ApplyMoves: %s has moved to %s", modAddr, newAddr) // We need to visit all of the resource instances in the // module and record them individually as results. for _, rs := range ms.Resources { @@ -94,6 +104,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] for _, rs := range ms.Resources { rAddr := rs.Addr if newAddr, matches := rAddr.MoveDestination(stmt.From, stmt.To); matches { + log.Printf("[TRACE] refactoring.ApplyMoves: resource %s has moved to %s", rAddr, newAddr) for key := range rs.Instances { oldInst := rAddr.Instance(key) newInst := newAddr.Instance(key) @@ -110,6 +121,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) map[addrs.UniqueKey] for key := range rs.Instances { iAddr := rAddr.Instance(key) if newAddr, matches := iAddr.MoveDestination(stmt.From, stmt.To); matches { + log.Printf("[TRACE] refactoring.ApplyMoves: resource instance %s has moved to %s", iAddr, newAddr) result := MoveResult{From: iAddr, To: newAddr} results[iAddr.UniqueKey()] = result results[newAddr.UniqueKey()] = result diff --git a/internal/terraform/context.go b/internal/terraform/context.go index 0a092c33b0..4e96fd25e7 100644 --- a/internal/terraform/context.go +++ b/internal/terraform/context.go @@ -129,6 +129,20 @@ type Context struct { refreshState *states.State prevRunState *states.State + // NOTE: If you're considering adding something new here, consider first + // whether it'd work to add it to type graphWalkOpts instead, possibly by + // adding new arguments to one of the exported operation methods, to scope + // it only to a particular operation rather than having it survive from one + // operation to the next as global mutable state. + // + // Historically we used fields here as a bit of a dumping ground for + // data that needed to ambiently pass between methods of Context, but + // that has tended to cause surprising misbehavior when data from one + // walk inadvertently bleeds into another walk against the same context. + // Perhaps one day we'll move changes, state, refreshState, and prevRunState + // to graphWalkOpts too. Ideally there shouldn't be anything in here which + // changes after NewContext returns. + hooks []Hook components contextComponentFactory schemas *Schemas @@ -491,7 +505,7 @@ func (c *Context) Eval(path addrs.ModuleInstance) (*lang.Scope, tfdiags.Diagnost diags = diags.Append(graphDiags) if !diags.HasErrors() { var walkDiags tfdiags.Diagnostics - walker, walkDiags = c.walk(graph, walkEval) + walker, walkDiags = c.walk(graph, walkEval, &graphWalkOpts{}) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) } @@ -500,7 +514,7 @@ func (c *Context) Eval(path addrs.ModuleInstance) (*lang.Scope, tfdiags.Diagnost // If we skipped walking the graph (due to errors) then we'll just // use a placeholder graph walker here, which'll refer to the // unmodified state. - walker = c.graphWalker(walkEval) + walker = c.graphWalker(walkEval, &graphWalkOpts{}) } // This is a bit weird since we don't normally evaluate outside of @@ -545,7 +559,7 @@ func (c *Context) Apply() (*states.State, tfdiags.Diagnostics) { } // Walk the graph - walker, walkDiags := c.walk(graph, operation) + walker, walkDiags := c.walk(graph, operation, &graphWalkOpts{}) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) @@ -656,7 +670,7 @@ The -target option is not for routine use, and is provided only for exceptional func (c *Context) plan() (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - moveStmts, _ := c.prePlanFindAndApplyMoves() + moveStmts, moveResults := c.prePlanFindAndApplyMoves() graph, graphDiags := c.Graph(GraphTypePlan, nil) diags = diags.Append(graphDiags) @@ -665,7 +679,9 @@ func (c *Context) plan() (*plans.Plan, tfdiags.Diagnostics) { } // Do the walk - walker, walkDiags := c.walk(graph, walkPlan) + walker, walkDiags := c.walk(graph, walkPlan, &graphWalkOpts{ + MoveResults: moveResults, + }) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { @@ -700,7 +716,7 @@ func (c *Context) destroyPlan() (*plans.Plan, tfdiags.Diagnostics) { } c.changes = plans.NewChanges() - moveStmts, _ := c.prePlanFindAndApplyMoves() + moveStmts, moveResults := c.prePlanFindAndApplyMoves() // A destroy plan starts by running Refresh to read any pending data // sources, and remove missing managed resources. This is required because @@ -734,7 +750,9 @@ func (c *Context) destroyPlan() (*plans.Plan, tfdiags.Diagnostics) { } // Do the walk - walker, walkDiags := c.walk(graph, walkPlanDestroy) + walker, walkDiags := c.walk(graph, walkPlanDestroy, &graphWalkOpts{ + MoveResults: moveResults, + }) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { @@ -764,7 +782,7 @@ func (c *Context) destroyPlan() (*plans.Plan, tfdiags.Diagnostics) { func (c *Context) refreshOnlyPlan() (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics - moveStmts, _ := c.prePlanFindAndApplyMoves() + moveStmts, moveResults := c.prePlanFindAndApplyMoves() graph, graphDiags := c.Graph(GraphTypePlanRefreshOnly, nil) diags = diags.Append(graphDiags) @@ -773,7 +791,9 @@ func (c *Context) refreshOnlyPlan() (*plans.Plan, tfdiags.Diagnostics) { } // Do the walk - walker, walkDiags := c.walk(graph, walkPlan) + walker, walkDiags := c.walk(graph, walkPlan, &graphWalkOpts{ + MoveResults: moveResults, + }) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { @@ -918,7 +938,7 @@ func (c *Context) Validate() tfdiags.Diagnostics { } // Walk - walker, walkDiags := c.walk(graph, walkValidate) + walker, walkDiags := c.walk(graph, walkValidate, &graphWalkOpts{}) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { @@ -991,10 +1011,18 @@ func (c *Context) releaseRun() { c.runContext = nil } -func (c *Context) walk(graph *Graph, operation walkOperation) (*ContextGraphWalker, tfdiags.Diagnostics) { +// graphWalkOpts is an assortment of options and inputs we need when +// constructing a graph walker. +type graphWalkOpts struct { + // MoveResults is a table of the results of applying move statements prior + // to a plan walk. Irrelevant and totally ignored for non-plan walks. + MoveResults map[addrs.UniqueKey]refactoring.MoveResult +} + +func (c *Context) walk(graph *Graph, operation walkOperation, opts *graphWalkOpts) (*ContextGraphWalker, tfdiags.Diagnostics) { log.Printf("[DEBUG] Starting graph walk: %s", operation.String()) - walker := c.graphWalker(operation) + walker := c.graphWalker(operation, opts) // Watch for a stop so we can call the provider Stop() API. watchStop, watchWait := c.watchStop(walker) @@ -1009,7 +1037,7 @@ func (c *Context) walk(graph *Graph, operation walkOperation) (*ContextGraphWalk return walker, diags } -func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker { +func (c *Context) graphWalker(operation walkOperation, opts *graphWalkOpts) *ContextGraphWalker { var state *states.SyncState var refreshState *states.SyncState var prevRunState *states.SyncState @@ -1040,6 +1068,7 @@ func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker { PrevRunState: prevRunState, Changes: c.changes.SyncWrapper(), InstanceExpander: instances.NewExpander(), + MoveResults: opts.MoveResults, Operation: operation, StopContext: c.runContext, RootVariableValues: c.variables, diff --git a/internal/terraform/context_import.go b/internal/terraform/context_import.go index 32ac6e0d36..ccee059d77 100644 --- a/internal/terraform/context_import.go +++ b/internal/terraform/context_import.go @@ -60,7 +60,7 @@ func (c *Context) Import(opts *ImportOpts) (*states.State, tfdiags.Diagnostics) } // Walk it - _, walkDiags := c.walk(graph, walkImport) + _, walkDiags := c.walk(graph, walkImport, &graphWalkOpts{}) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { return c.state, diags diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 048495cad6..a5242ad899 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -730,6 +730,80 @@ provider "test" { } } +func TestContext2Plan_movedResourceBasic(t *testing.T) { + t.Skip("Context.Plan doesn't properly propagate moves into the prior state yet") + + addrA := mustResourceInstanceAddr("test_object.a") + addrB := mustResourceInstanceAddr("test_object.b") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + resource "test_object" "b" { + } + + moved { + from = test_object.a + to = test_object.b + } + + terraform { + experiments = [config_driven_move] + } + `, + }) + + state := states.BuildState(func(s *states.SyncState) { + // The prior state tracks test_object.a, which we should treat as + // test_object.b because of the "moved" block in the config. + s.SetResourceInstanceCurrent(addrA, &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + p := simpleMockProvider() + ctx := testContext2(t, &ContextOpts{ + Config: m, + State: state, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + ForceReplace: []addrs.AbsResourceInstance{ + addrA, + }, + }) + + plan, diags := ctx.Plan() + if diags.HasErrors() { + t.Fatalf("unexpected errors\n%s", diags.Err().Error()) + } + + t.Run(addrA.String(), func(t *testing.T) { + instPlan := plan.Changes.ResourceInstance(addrA) + if instPlan != nil { + t.Fatalf("unexpected plan for %s; should've moved to %s", addrA, addrB) + } + }) + t.Run(addrB.String(), func(t *testing.T) { + instPlan := plan.Changes.ResourceInstance(addrB) + if instPlan == nil { + t.Fatalf("no plan for %s at all", addrB) + } + + if got, want := instPlan.Addr, addrB; !got.Equal(want) { + t.Errorf("wrong current address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.PrevRunAddr, addrA; !got.Equal(want) { + t.Errorf("wrong previous run address\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.Action, plans.NoOp; got != want { + t.Errorf("wrong planned action\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.ActionReason, plans.ResourceInstanceChangeNoReason; got != want { + t.Errorf("wrong action reason\ngot: %s\nwant: %s", got, want) + } + }) +} + func TestContext2Plan_refreshOnlyMode(t *testing.T) { addr := mustResourceInstanceAddr("test_object.a") diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index cae189482c..be3acb7c50 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -1305,7 +1305,7 @@ func TestContext2Validate_PlanGraphBuilder(t *testing.T) { t.Fatalf("errors from PlanGraphBuilder: %s", diags.Err()) } defer c.acquireRun("validate-test")() - walker, diags := c.walk(graph, walkValidate) + walker, diags := c.walk(graph, walkValidate, &graphWalkOpts{}) if diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/internal/terraform/eval_context.go b/internal/terraform/eval_context.go index 0a711f8732..2cee1b0711 100644 --- a/internal/terraform/eval_context.go +++ b/internal/terraform/eval_context.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/refactoring" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" @@ -165,6 +166,16 @@ type EvalContext interface { // EvalContext objects for a given configuration. InstanceExpander() *instances.Expander + // MoveResults returns a map describing the results of handling any + // resource instance move statements prior to the graph walk, so that + // the graph walk can then record that information appropriately in other + // artifacts produced by the graph walk. + // + // This data structure is created prior to the graph walk and read-only + // thereafter, so callers must not modify the returned map or any other + // objects accessible through it. + MoveResults() map[addrs.UniqueKey]refactoring.MoveResult + // WithPath returns a copy of the context with the internal path set to the // path argument. WithPath(path addrs.ModuleInstance) EvalContext diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index 4e956b8dee..1b971fd6b1 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/refactoring" "github.com/hashicorp/terraform/version" "github.com/hashicorp/terraform/internal/states" @@ -74,6 +75,7 @@ type BuiltinEvalContext struct { RefreshStateValue *states.SyncState PrevRunStateValue *states.SyncState InstanceExpanderValue *instances.Expander + MoveResultsValue map[addrs.UniqueKey]refactoring.MoveResult } // BuiltinEvalContext implements EvalContext @@ -367,3 +369,7 @@ func (ctx *BuiltinEvalContext) PrevRunState() *states.SyncState { func (ctx *BuiltinEvalContext) InstanceExpander() *instances.Expander { return ctx.InstanceExpanderValue } + +func (ctx *BuiltinEvalContext) MoveResults() map[addrs.UniqueKey]refactoring.MoveResult { + return ctx.MoveResultsValue +} diff --git a/internal/terraform/eval_context_mock.go b/internal/terraform/eval_context_mock.go index 7d2e64c6ac..0e4fd32d69 100644 --- a/internal/terraform/eval_context_mock.go +++ b/internal/terraform/eval_context_mock.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/refactoring" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" @@ -128,6 +129,9 @@ type MockEvalContext struct { PrevRunStateCalled bool PrevRunStateState *states.SyncState + MoveResultsCalled bool + MoveResultsResults map[addrs.UniqueKey]refactoring.MoveResult + InstanceExpanderCalled bool InstanceExpanderExpander *instances.Expander } @@ -347,6 +351,11 @@ func (c *MockEvalContext) PrevRunState() *states.SyncState { return c.PrevRunStateState } +func (c *MockEvalContext) MoveResults() map[addrs.UniqueKey]refactoring.MoveResult { + c.MoveResultsCalled = true + return c.MoveResultsResults +} + func (c *MockEvalContext) InstanceExpander() *instances.Expander { c.InstanceExpanderCalled = true return c.InstanceExpanderExpander diff --git a/internal/terraform/graph_walk_context.go b/internal/terraform/graph_walk_context.go index 4c16d44c71..fcda4fa734 100644 --- a/internal/terraform/graph_walk_context.go +++ b/internal/terraform/graph_walk_context.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/refactoring" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -23,11 +24,12 @@ type ContextGraphWalker struct { // Configurable values Context *Context - State *states.SyncState // Used for safe concurrent access to state - RefreshState *states.SyncState // Used for safe concurrent access to state - PrevRunState *states.SyncState // Used for safe concurrent access to state - Changes *plans.ChangesSync // Used for safe concurrent writes to changes - InstanceExpander *instances.Expander // Tracks our gradual expansion of module and resource instances + State *states.SyncState // Used for safe concurrent access to state + RefreshState *states.SyncState // Used for safe concurrent access to state + PrevRunState *states.SyncState // Used for safe concurrent access to state + Changes *plans.ChangesSync // Used for safe concurrent writes to changes + InstanceExpander *instances.Expander // Tracks our gradual expansion of module and resource instances + MoveResults map[addrs.UniqueKey]refactoring.MoveResult // Read-only record of earlier processing of move statements Operation walkOperation StopContext context.Context RootVariableValues InputValues @@ -88,6 +90,7 @@ func (w *ContextGraphWalker) EvalContext() EvalContext { InstanceExpanderValue: w.InstanceExpander, Components: w.Context.components, Schemas: w.Context.schemas, + MoveResultsValue: w.MoveResults, ProviderCache: w.providerCache, ProviderInputConfig: w.Context.providerInputConfig, ProviderLock: &w.providerLock, diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 7735897ce4..89275809bb 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -393,7 +393,7 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState // vs. that something being entirely excluded e.g. due to -target. noop := &plans.ResourceInstanceChange{ Addr: absAddr, - PrevRunAddr: absAddr, // TODO-PrevRunAddr: If this instance was moved/renamed in this run, record its old address + PrevRunAddr: n.prevRunAddr(ctx), DeposedKey: deposedKey, Change: plans.Change{ Action: plans.NoOp, @@ -421,7 +421,7 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState // help for this one. plan := &plans.ResourceInstanceChange{ Addr: absAddr, - PrevRunAddr: absAddr, // TODO-PrevRunAddr: If this instance was moved/renamed in this run, record its old address + PrevRunAddr: n.prevRunAddr(ctx), DeposedKey: deposedKey, Change: plans.Change{ Action: plans.Delete, @@ -1066,7 +1066,7 @@ func (n *NodeAbstractResourceInstance) plan( // Update our return plan plan = &plans.ResourceInstanceChange{ Addr: n.Addr, - PrevRunAddr: n.Addr, // TODO-PrevRunAddr: If this instance was moved/renamed in this run, record its old address + PrevRunAddr: n.prevRunAddr(ctx), Private: plannedPrivate, ProviderAddr: n.ResolvedProvider, Change: plans.Change{ @@ -1528,7 +1528,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt // value containing unknowns from PlanDataResourceObject. plannedChange := &plans.ResourceInstanceChange{ Addr: n.Addr, - PrevRunAddr: n.Addr, // data resources are not refactorable + PrevRunAddr: n.prevRunAddr(ctx), ProviderAddr: n.ResolvedProvider, Change: plans.Change{ Action: plans.Read, @@ -2267,3 +2267,20 @@ func (n *NodeAbstractResourceInstance) apply( return nil, diags } } + +func (n *NodeAbstractResourceInstance) prevRunAddr(ctx EvalContext) addrs.AbsResourceInstance { + return resourceInstancePrevRunAddr(ctx, n.Addr) +} + +func resourceInstancePrevRunAddr(ctx EvalContext, currentAddr addrs.AbsResourceInstance) addrs.AbsResourceInstance { + table := ctx.MoveResults() + + result, ok := table[currentAddr.UniqueKey()] + if !ok { + // If there's no entry in the table then we'll assume it didn't move + // at all, and so its previous address is the same as the current one. + return currentAddr + } + + return result.From +}