diff --git a/internal/refactoring/cross_provider_move.go b/internal/refactoring/cross_provider_move.go new file mode 100644 index 0000000000..67a8e07c40 --- /dev/null +++ b/internal/refactoring/cross_provider_move.go @@ -0,0 +1,188 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package refactoring + +import ( + "fmt" + + "github.com/hashicorp/hcl/v2" + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/providers" + "github.com/hashicorp/terraform/internal/states" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +// crossTypeMover is a collection of data that is needed to calculate the +// cross-provider move state changes. +type crossTypeMover struct { + State *states.State + ProviderFactories map[addrs.Provider]providers.Factory + ProviderCache map[addrs.Provider]providers.Interface +} + +// close ensures the cached providers are closed. +func (m *crossTypeMover) close() tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + for _, provider := range m.ProviderCache { + diags = diags.Append(provider.Close()) + } + return diags +} + +func (m *crossTypeMover) getProvider(providers addrs.Provider) (providers.Interface, error) { + if provider, ok := m.ProviderCache[providers]; ok { + return provider, nil + } + + if factory, ok := m.ProviderFactories[providers]; ok { + provider, err := factory() + if err != nil { + return nil, err + } + + m.ProviderCache[providers] = provider + return provider, nil + } + + // Then we don't have a provider in the cache - this represents a bug in + // Terraform since we should have already loaded all the providers in the + // configuration and the state. + return nil, fmt.Errorf("provider %s implementation not found; this is a bug in Terraform - please report it", providers) +} + +// prepareCrossTypeMove checks if the provided MoveStatement is a cross-type +// move and if so, prepares the data needed to perform the move. +func (m *crossTypeMover) prepareCrossTypeMove(stmt *MoveStatement, source, target addrs.AbsResource) (*crossTypeMove, tfdiags.Diagnostics) { + if stmt.Provider == nil { + // This means the resource was not in the configuration at all, so we + // can't process this. It'll be picked up in the validation errors + // later. + return nil, nil + } + + targetProviderAddr := stmt.Provider + sourceProviderAddr := m.State.Resource(source).ProviderConfig + + if targetProviderAddr.Provider.Equals(sourceProviderAddr.Provider) { + if source.Resource.Type == target.Resource.Type { + // Then this is a move within the same provider and type, so we + // don't need to do anything special. + return nil, nil + } + } + + var diags tfdiags.Diagnostics + var err error + + targetProvider, err := m.getProvider(targetProviderAddr.Provider) + if err != nil { + diags = diags.Append(tfdiags.Sourceless(tfdiags.Error, "Failed to initialise provider", err.Error())) + return nil, diags + } + + targetSchema := targetProvider.GetProviderSchema() + diags = diags.Append(targetSchema.Diagnostics) + if targetSchema.Diagnostics.HasErrors() { + return nil, diags + } + + if !targetSchema.ServerCapabilities.MoveResourceState { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Unsupported `moved` across resource types", + Detail: fmt.Sprintf("The provider %q does not support moved operations across resource types and providers.", targetProviderAddr.Provider), + Subject: stmt.DeclRange.ToHCL().Ptr(), + }) + return nil, diags + } + + targetResourceSchema, targetResourceSchemaVersion := targetSchema.SchemaForResourceAddr(target.Resource) + + return &crossTypeMove{ + targetProvider: targetProvider, + targetProviderAddr: *targetProviderAddr, + targetResourceSchema: targetResourceSchema, + targetResourceSchemaVersion: targetResourceSchemaVersion, + sourceProviderAddr: sourceProviderAddr, + }, diags +} + +type crossTypeMove struct { + targetProvider providers.Interface + targetProviderAddr addrs.AbsProviderConfig + targetResourceSchema *configschema.Block + targetResourceSchemaVersion uint64 + + sourceProviderAddr addrs.AbsProviderConfig +} + +// applyCrossTypeMove will update the provider states.SyncState so that value +// at source is the result of the providers move operation. Note, that this +// doesn't actually move the resource in the state file, it just updates the +// value at source ready to be moved. +func (move *crossTypeMove) applyCrossTypeMove(stmt *MoveStatement, source, target addrs.AbsResourceInstance, state *states.SyncState) tfdiags.Diagnostics { + if move == nil { + // Then we don't need to do any data transformation. + return nil + } + + var diags tfdiags.Diagnostics + + // First, build the request. + + src := state.ResourceInstance(source).Current + request := providers.MoveResourceStateRequest{ + SourceProviderAddress: move.sourceProviderAddr.Provider.String(), + SourceTypeName: source.Resource.Resource.Type, + SourceSchemaVersion: int64(src.SchemaVersion), + SourceStateJSON: src.AttrsJSON, + TargetTypeName: target.Resource.Resource.Type, + } + + // Second, ask the provider to transform the value into the type expected by + // the new resource type. + + resp := move.targetProvider.MoveResourceState(request) + diags = diags.Append(resp.Diagnostics) + if resp.Diagnostics.HasErrors() { + return diags + } + + if resp.TargetState == cty.NilVal { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Provider returned invalid value", + Detail: fmt.Sprintf("The provider returned an invalid value during an across type move operation to %s. This is a bug in the relevant provider; Please report it.", target), + Subject: stmt.DeclRange.ToHCL().Ptr(), + }) + return diags + } + + // Finally, we can update the source value with the new value. + + newValue := &states.ResourceInstanceObject{ + Value: resp.TargetState, + Private: src.Private, + Status: src.Status, + Dependencies: src.Dependencies, + CreateBeforeDestroy: src.CreateBeforeDestroy, + } + + data, err := newValue.Encode(move.targetResourceSchema.ImpliedType(), move.targetResourceSchemaVersion) + if err != nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Failed to encode source value", + Detail: fmt.Sprintf("Terraform failed to encode the value in state for %s: %v. This is a bug in Terraform; Please report it.", source.String(), err), + Subject: stmt.DeclRange.ToHCL().Ptr(), + }) + return diags + } + + state.SetResourceInstanceCurrent(source, data, move.targetProviderAddr) + return diags +} diff --git a/internal/refactoring/mock_provider.go b/internal/refactoring/mock_provider.go new file mode 100644 index 0000000000..0b302cfdd0 --- /dev/null +++ b/internal/refactoring/mock_provider.go @@ -0,0 +1,94 @@ +package refactoring + +import ( + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/providers" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +var _ providers.Interface = (*mockProvider)(nil) + +// mockProvider provides a mock implementation of providers.Interface that only +// implements the methods that are used by the refactoring package. +type mockProvider struct { + moveResourceState bool + moveResourceError error +} + +func (provider *mockProvider) GetProviderSchema() providers.GetProviderSchemaResponse { + return providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "foo": {}, + "bar": {}, + }, + ServerCapabilities: providers.ServerCapabilities{ + MoveResourceState: provider.moveResourceState, + }, + } +} + +func (provider *mockProvider) ValidateProviderConfig(providers.ValidateProviderConfigRequest) providers.ValidateProviderConfigResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) ValidateResourceConfig(providers.ValidateResourceConfigRequest) providers.ValidateResourceConfigResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) ValidateDataResourceConfig(providers.ValidateDataResourceConfigRequest) providers.ValidateDataResourceConfigResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) UpgradeResourceState(providers.UpgradeResourceStateRequest) providers.UpgradeResourceStateResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) ConfigureProvider(providers.ConfigureProviderRequest) providers.ConfigureProviderResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) Stop() error { + panic("not implemented in mock") +} + +func (provider *mockProvider) ReadResource(providers.ReadResourceRequest) providers.ReadResourceResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) PlanResourceChange(providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) ApplyResourceChange(providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) ImportResourceState(providers.ImportResourceStateRequest) providers.ImportResourceStateResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) MoveResourceState(providers.MoveResourceStateRequest) providers.MoveResourceStateResponse { + if provider.moveResourceError != nil { + return providers.MoveResourceStateResponse{ + Diagnostics: tfdiags.Diagnostics{ + tfdiags.Sourceless(tfdiags.Error, "expected error", provider.moveResourceError.Error()), + }, + } + } + return providers.MoveResourceStateResponse{ + TargetState: cty.EmptyObjectVal, + } +} + +func (provider *mockProvider) ReadDataSource(providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) CallFunction(providers.CallFunctionRequest) providers.CallFunctionResponse { + panic("not implemented in mock") +} + +func (provider *mockProvider) Close() error { + return nil // do nothing +} diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 9748f849b8..d79f77ffd4 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -10,7 +10,9 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/logging" + "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/states" + "github.com/hashicorp/terraform/internal/tfdiags" ) // ApplyMoves modifies in-place the given state object so that any existing @@ -28,11 +30,11 @@ import ( // // ApplyMoves expects exclusive access to the given state while it's running. // Don't read or write any part of the state structure until ApplyMoves returns. -func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { +func ApplyMoves(stmts []MoveStatement, state *states.State, providerFactory map[addrs.Provider]providers.Factory) (MoveResults, tfdiags.Diagnostics) { ret := makeMoveResults() if len(stmts) == 0 { - return ret + return ret, nil } // The methodology here is to construct a small graph of all of the move @@ -47,7 +49,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { // separate validation step should detect this and return an error. if diags := validateMoveStatementGraph(g); diags.HasErrors() { log.Printf("[ERROR] ApplyMoves: %s", diags.ErrWithWarnings()) - return ret + return ret, nil } // The graph must be reduced in order for ReverseDepthFirstWalk to work @@ -65,7 +67,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { if startNodes.Len() == 0 { log.Println("[TRACE] refactoring.ApplyMoves: No 'moved' statements to consider in this configuration") - return ret + return ret, nil } log.Printf("[TRACE] refactoring.ApplyMoves: Processing 'moved' statements in the configuration\n%s", logging.Indent(g.String())) @@ -90,6 +92,15 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { }) } + crossTypeMover := &crossTypeMover{ + State: state, + ProviderFactories: providerFactory, + ProviderCache: make(map[addrs.Provider]providers.Interface), + } + + var diags tfdiags.Diagnostics + + syncState := state.SyncWrapper() for _, v := range g.ReverseTopologicalOrder() { stmt := v.(*MoveStatement) @@ -125,7 +136,7 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { } } - state.MoveModuleInstance(modAddr, newAddr) + syncState.MoveModuleInstance(modAddr, newAddr) continue } case addrs.MoveEndpointResource: @@ -152,9 +163,12 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { continue } + crossTypeMove, prepareDiags := crossTypeMover.prepareCrossTypeMove(stmt, rAddr, newAddr) + diags = diags.Append(prepareDiags) for key := range rs.Instances { oldInst := rAddr.Instance(key) newInst := newAddr.Instance(key) + diags = diags.Append(crossTypeMove.applyCrossTypeMove(stmt, oldInst, newInst, syncState)) recordOldAddr(oldInst, newInst) } state.MoveAbsResource(rAddr, newAddr) @@ -174,9 +188,11 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { continue } + crossTypeMove, crossTypeMoveDiags := crossTypeMover.prepareCrossTypeMove(stmt, iAddr.ContainingResource(), newAddr.ContainingResource()) + diags = diags.Append(crossTypeMoveDiags) + diags = diags.Append(crossTypeMove.applyCrossTypeMove(stmt, iAddr, newAddr, syncState)) recordOldAddr(iAddr, newAddr) - - state.MoveAbsResourceInstance(iAddr, newAddr) + syncState.MoveResourceInstance(iAddr, newAddr) continue } } @@ -186,8 +202,10 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { } } } + syncState.Close() - return ret + diags = diags.Append(crossTypeMover.close()) + return ret, diags } // buildMoveStatementGraph constructs a dependency graph of the given move diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index 85411300f2..cc2047c849 100644 --- a/internal/refactoring/move_execute_test.go +++ b/internal/refactoring/move_execute_test.go @@ -15,14 +15,19 @@ import ( "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/states" ) func TestApplyMoves(t *testing.T) { - providerAddr := addrs.AbsProviderConfig{ + barProviderAddress := addrs.AbsProviderConfig{ Module: addrs.RootModule, Provider: addrs.MustParseProviderSourceString("example.com/foo/bar"), } + fooProviderAddress := addrs.AbsProviderConfig{ + Module: addrs.RootModule, + Provider: addrs.MustParseProviderSourceString("example.com/bar/foo"), + } mustParseInstAddr := func(s string) addrs.AbsResourceInstance { addr, err := addrs.ParseAbsResourceInstanceStr(s) @@ -38,47 +43,53 @@ func TestApplyMoves(t *testing.T) { Stmts []MoveStatement State *states.State + // We only need providers if we are doing a cross-resource type move + // so most of the test cases don't need this. + Providers map[addrs.Provider]providers.Factory + WantResults MoveResults + WantDiags []string WantInstanceAddrs []string }{ "no moves and empty state": { - []MoveStatement{}, - states.NewState(), - emptyResults, - nil, + Stmts: []MoveStatement{}, + State: states.NewState(), + Providers: nil, + WantResults: emptyResults, + WantInstanceAddrs: nil, }, "no moves": { - []MoveStatement{}, - states.BuildState(func(s *states.SyncState) { + Stmts: []MoveStatement{}, + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - emptyResults, - []string{ + WantResults: emptyResults, + WantInstanceAddrs: []string{ `foo.from`, }, }, "single move of whole singleton resource": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "foo.to"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.to", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("foo.to"), MoveSuccess{ From: mustParseInstAddr("foo.from"), @@ -87,25 +98,25 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `foo.to`, }, }, "single move of whole 'count' resource": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "foo.to"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.to", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("foo.to[0]"), MoveSuccess{ From: mustParseInstAddr("foo.from[0]"), @@ -114,26 +125,26 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `foo.to[0]`, }, }, "chained move of whole singleton resource": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "foo.mid"), - testMoveStatement(t, "", "foo.mid", "foo.to"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.mid", &barProviderAddress), + testMoveStatement(t, "", "foo.mid", "foo.to", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("foo.to"), MoveSuccess{ From: mustParseInstAddr("foo.from"), @@ -142,26 +153,26 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `foo.to`, }, }, "move whole resource into module": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "module.boo.foo.to"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "module.boo.foo.to", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.to[0]"), MoveSuccess{ From: mustParseInstAddr("foo.from[0]"), @@ -170,26 +181,26 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.boo.foo.to[0]`, }, }, "move resource instance between modules": { - []MoveStatement{ - testMoveStatement(t, "", "module.boo.foo.from[0]", "module.bar[0].foo.to[0]"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.boo.foo.from[0]", "module.bar[0].foo.to[0]", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to[0]"), MoveSuccess{ From: mustParseInstAddr("module.boo.foo.from[0]"), @@ -198,23 +209,23 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].foo.to[0]`, }, }, "module move with child module": { - []MoveStatement{ - testMoveStatement(t, "", "module.boo", "module.bar"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.boo", "module.bar", nil), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.module.hoo.foo.from"), @@ -222,10 +233,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar.foo.from"), MoveSuccess{ From: mustParseInstAddr("module.boo.foo.from"), @@ -238,27 +249,27 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar.foo.from`, `module.bar.module.hoo.foo.from`, }, }, "move whole single module to indexed module": { - []MoveStatement{ - testMoveStatement(t, "", "module.boo", "module.bar[0]"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.boo", "module.bar[0]", nil), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.from[0]"), MoveSuccess{ From: mustParseInstAddr("module.boo.foo.from[0]"), @@ -267,27 +278,27 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].foo.from[0]`, }, }, "move whole module to indexed module and move instance chained": { - []MoveStatement{ - testMoveStatement(t, "", "module.boo", "module.bar[0]"), - testMoveStatement(t, "bar", "foo.from[0]", "foo.to[0]"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.boo", "module.bar[0]", nil), + testMoveStatement(t, "bar", "foo.from[0]", "foo.to[0]", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to[0]"), MoveSuccess{ From: mustParseInstAddr("module.boo.foo.from[0]"), @@ -296,27 +307,27 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].foo.to[0]`, }, }, "move instance to indexed module and instance chained": { - []MoveStatement{ - testMoveStatement(t, "", "module.boo.foo.from[0]", "module.bar[0].foo.from[0]"), - testMoveStatement(t, "bar", "foo.from[0]", "foo.to[0]"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.boo.foo.from[0]", "module.bar[0].foo.from[0]", &barProviderAddress), + testMoveStatement(t, "bar", "foo.from[0]", "foo.to[0]", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.from[0]"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to[0]"), MoveSuccess{ From: mustParseInstAddr("module.boo.foo.from[0]"), @@ -325,23 +336,23 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].foo.to[0]`, }, }, "move module instance to already-existing module instance": { - []MoveStatement{ - testMoveStatement(t, "", "module.bar[0]", "module.boo"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.bar[0]", "module.boo", nil), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.bar[0].foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.to[0]"), @@ -349,10 +360,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ // Nothing moved, because the module.b address is already // occupied by another module. Changes: emptyResults.Changes, @@ -366,24 +377,24 @@ func TestApplyMoves(t *testing.T) { ), ), }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].foo.from`, `module.boo.foo.to[0]`, }, }, "move resource to already-existing resource": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "foo.to"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.to", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("foo.to"), @@ -391,10 +402,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ // Nothing moved, because the from.to address is already // occupied by another resource. Changes: emptyResults.Changes, @@ -408,24 +419,24 @@ func TestApplyMoves(t *testing.T) { ), ), }, - []string{ + WantInstanceAddrs: []string{ `foo.from`, `foo.to`, }, }, "move resource instance to already-existing resource instance": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "foo.to[0]"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "foo.to[0]", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("foo.to[0]"), @@ -433,10 +444,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ // Nothing moved, because the from.to[0] address is already // occupied by another resource instance. Changes: emptyResults.Changes, @@ -450,27 +461,27 @@ func TestApplyMoves(t *testing.T) { ), ), }, - []string{ + WantInstanceAddrs: []string{ `foo.from`, `foo.to[0]`, }, }, "move resource and containing module": { - []MoveStatement{ - testMoveStatement(t, "", "module.boo", "module.bar[0]"), - testMoveStatement(t, "boo", "foo.from", "foo.to"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.boo", "module.bar[0]", nil), + testMoveStatement(t, "boo", "foo.from", "foo.to", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.boo.foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to"), MoveSuccess{ From: mustParseInstAddr("module.boo.foo.from"), @@ -479,24 +490,24 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].foo.to`, }, }, "move module and then move resource into it": { - []MoveStatement{ - testMoveStatement(t, "", "module.bar[0]", "module.boo"), - testMoveStatement(t, "", "foo.from", "module.boo.foo.from"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.bar[0]", "module.boo", nil), + testMoveStatement(t, "", "foo.from", "module.boo.foo.from", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.bar[0].foo.to"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), @@ -504,10 +515,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.from"), MoveSuccess{ mustParseInstAddr("foo.from"), @@ -520,26 +531,26 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.boo.foo.from`, `module.boo.foo.to`, }, }, "move resources into module and then move module": { - []MoveStatement{ - testMoveStatement(t, "", "foo.from", "module.boo.foo.to"), - testMoveStatement(t, "", "bar.from", "module.boo.bar.to"), - testMoveStatement(t, "", "module.boo", "module.bar[0]"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "module.boo.foo.to", &barProviderAddress), + testMoveStatement(t, "", "bar.from", "module.boo.bar.to", &barProviderAddress), + testMoveStatement(t, "", "module.boo", "module.bar[0]", nil), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("bar.from"), @@ -547,10 +558,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.bar[0].foo.to"), MoveSuccess{ mustParseInstAddr("foo.from"), @@ -563,25 +574,25 @@ func TestApplyMoves(t *testing.T) { ), Blocked: emptyResults.Blocked, }, - []string{ + WantInstanceAddrs: []string{ `module.bar[0].bar.to`, `module.bar[0].foo.to`, }, }, "module move collides with resource move": { - []MoveStatement{ - testMoveStatement(t, "", "module.bar[0]", "module.boo"), - testMoveStatement(t, "", "foo.from", "module.boo.foo.from"), + Stmts: []MoveStatement{ + testMoveStatement(t, "", "module.bar[0]", "module.boo", nil), + testMoveStatement(t, "", "foo.from", "module.boo.foo.from", &barProviderAddress), }, - states.BuildState(func(s *states.SyncState) { + State: states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( mustParseInstAddr("module.bar[0].foo.from"), &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) s.SetResourceInstanceCurrent( mustParseInstAddr("foo.from"), @@ -589,10 +600,10 @@ func TestApplyMoves(t *testing.T) { Status: states.ObjectReady, AttrsJSON: []byte(`{}`), }, - providerAddr, + barProviderAddress, ) }), - MoveResults{ + WantResults: MoveResults{ Changes: addrs.MakeMap( addrs.MakeMapElem(mustParseInstAddr("module.boo.foo.from"), MoveSuccess{ mustParseInstAddr("module.bar[0].foo.from"), @@ -609,11 +620,161 @@ func TestApplyMoves(t *testing.T) { ), ), }, - []string{ + WantInstanceAddrs: []string{ `foo.from`, `module.boo.foo.from`, }, }, + + "cross resource type move unsupported": { + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "bar.to", &barProviderAddress), + }, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustParseInstAddr("foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + barProviderAddress, + ) + }), + Providers: map[addrs.Provider]providers.Factory{ + barProviderAddress.Provider: func() (providers.Interface, error) { + return &mockProvider{ + moveResourceState: false, + moveResourceError: nil, + }, nil + }, + }, + WantResults: MoveResults{ + Changes: addrs.MakeMap( + addrs.MakeMapElem(mustParseInstAddr("bar.to"), MoveSuccess{ + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("bar.to"), + }), + ), + Blocked: emptyResults.Blocked, + }, + WantDiags: []string{ + "(Error) Unsupported `moved` across resource types:The provider \"example.com/foo/bar\" does not support moved operations across resource types and providers.", + }, + WantInstanceAddrs: []string{ + `bar.to`, + }, + }, + "cross resource type move errors": { + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "bar.to", &barProviderAddress), + }, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustParseInstAddr("foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + barProviderAddress, + ) + }), + Providers: map[addrs.Provider]providers.Factory{ + barProviderAddress.Provider: func() (providers.Interface, error) { + return &mockProvider{ + moveResourceState: true, + moveResourceError: fmt.Errorf("provider can't move between those resource types"), + }, nil + }, + }, + WantResults: MoveResults{ + Changes: addrs.MakeMap( + addrs.MakeMapElem(mustParseInstAddr("bar.to"), MoveSuccess{ + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("bar.to"), + }), + ), + Blocked: emptyResults.Blocked, + }, + WantDiags: []string{ + "(Error) expected error:provider can't move between those resource types", + }, + WantInstanceAddrs: []string{ + `bar.to`, + }, + }, + "cross resource type move": { + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "bar.to", &barProviderAddress), + }, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustParseInstAddr("foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + barProviderAddress, + ) + }), + Providers: map[addrs.Provider]providers.Factory{ + barProviderAddress.Provider: func() (providers.Interface, error) { + return &mockProvider{ + moveResourceState: true, + moveResourceError: nil, + }, nil + }, + }, + WantResults: MoveResults{ + Changes: addrs.MakeMap( + addrs.MakeMapElem(mustParseInstAddr("bar.to"), MoveSuccess{ + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("bar.to"), + }), + ), + Blocked: emptyResults.Blocked, + }, + WantInstanceAddrs: []string{ + `bar.to`, + }, + }, + "cross provider move": { + Stmts: []MoveStatement{ + testMoveStatement(t, "", "foo.from", "bar.to", &barProviderAddress), + }, + State: states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + mustParseInstAddr("foo.from"), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{}`), + }, + fooProviderAddress, + ) + }), + Providers: map[addrs.Provider]providers.Factory{ + barProviderAddress.Provider: func() (providers.Interface, error) { + return &mockProvider{ + moveResourceState: true, + moveResourceError: nil, + }, nil + }, + fooProviderAddress.Provider: func() (providers.Interface, error) { + return &mockProvider{}, nil + }, + }, + WantResults: MoveResults{ + Changes: addrs.MakeMap( + addrs.MakeMapElem(mustParseInstAddr("bar.to"), MoveSuccess{ + From: mustParseInstAddr("foo.from"), + To: mustParseInstAddr("bar.to"), + }), + ), + Blocked: emptyResults.Blocked, + }, + WantInstanceAddrs: []string{ + `bar.to`, + }, + }, } for name, test := range tests { @@ -627,7 +788,15 @@ func TestApplyMoves(t *testing.T) { t.Logf("resource instances in prior state:\n%s", spew.Sdump(allResourceInstanceAddrsInState(test.State))) state := test.State.DeepCopy() // don't modify the test case in-place - gotResults := ApplyMoves(test.Stmts, state) + gotResults, diags := ApplyMoves(test.Stmts, state, test.Providers) + + var actualDiags []string + for _, diag := range diags { + actualDiags = append(actualDiags, fmt.Sprintf("(%s) %s:%s", diag.Severity(), diag.Description().Summary, diag.Description().Detail)) + } + if diff := cmp.Diff(test.WantDiags, actualDiags); diff != "" { + t.Errorf("wrong diagnostics\n%s", diff) + } if diff := cmp.Diff(test.WantResults, gotResults); diff != "" { t.Errorf("wrong results\n%s", diff) @@ -641,7 +810,7 @@ func TestApplyMoves(t *testing.T) { } } -func testMoveStatement(t *testing.T, module string, from string, to string) MoveStatement { +func testMoveStatement(t *testing.T, module string, from string, to string, provider *addrs.AbsProviderConfig) MoveStatement { t.Helper() moduleAddr := addrs.RootModule @@ -671,12 +840,19 @@ func testMoveStatement(t *testing.T, module string, from string, to string) Move t.Fatalf("incompatible endpoints") } - return MoveStatement{ + stmt := MoveStatement{ From: fromInModule, To: toInModule, // DeclRange not populated because it's unimportant for our tests } + + if provider != nil { + // Only set the provider for resource type moves. + stmt.Provider = provider + } + + return stmt } func allResourceInstanceAddrsInState(state *states.State) []string { diff --git a/internal/refactoring/move_statement.go b/internal/refactoring/move_statement.go index 747c4f69d1..113c8ae1dc 100644 --- a/internal/refactoring/move_statement.go +++ b/internal/refactoring/move_statement.go @@ -16,6 +16,14 @@ type MoveStatement struct { From, To *addrs.MoveEndpointInModule DeclRange tfdiags.SourceRange + // Provider is the provider configuration that applies to the "to" address + // of this move. As in, the provider that will manage the resource after + // it has been moved. + // + // This may be null if the "to" address points to a module instead of a + // resource. + Provider *addrs.AbsProviderConfig + // Implied is true for statements produced by ImpliedMoveStatements, and // false for statements produced by FindMoveStatements. // @@ -47,12 +55,41 @@ func findMoveStatements(cfg *configs.Config, into []MoveStatement) []MoveStateme panic(fmt.Sprintf("incompatible move endpoints in %s", mc.DeclRange)) } - into = append(into, MoveStatement{ + stmt := MoveStatement{ From: fromAddr, To: toAddr, DeclRange: tfdiags.SourceRangeFromHCL(mc.DeclRange), Implied: false, - }) + } + + // We have the statement, let's see if we should attach a provider to + // it. + + if toResource, ok := mc.To.ConfigMoveable(addrs.RootModule).(addrs.ConfigResource); ok { + // Only attach providers if we are moving resources, and we attach + // the to resource provider from the config. We can retrieve the + // from resource provider from the state later. + + resourceConfig := cfg.Descendent(toResource.Module).Module.ResourceByAddr(toResource.Resource) + if resourceConfig != nil { + + // Check the target resource config actually exists before we + // try and extract the provider from them. If the resource + // doesn't exist in config, then we'll get a validation error + // later on anyway. + + stmt.Provider = &addrs.AbsProviderConfig{ + Module: modAddr, + Provider: resourceConfig.Provider, + } + + if resourceConfig.ProviderConfigRef != nil { + stmt.Provider.Alias = resourceConfig.ProviderConfigRef.Alias + } + } + } + + into = append(into, stmt) } for _, childCfg := range cfg.Children { @@ -135,14 +172,25 @@ func impliedMoveStatements(cfg *configs.Config, prevRunState *states.State, expl } if fromKey != toKey { - // We mustn't generate an impied statement if the user already + // We mustn't generate an implied statement if the user already // wrote an explicit statement referring to this resource, // because they may wish to select an instance key other than // zero as the one to retain. if !haveMoveStatementForResource(rAddr, explicitStmts) { + + resource := cfg.Descendent(addrs.RootModule).Module.ResourceByAddr(rAddr.Resource) + provider := &addrs.AbsProviderConfig{ + Module: rAddr.Module.Module(), + Provider: resource.Provider, + } + if resource.ProviderConfigRef != nil { + provider.Alias = resource.ProviderConfigRef.Alias + } + into = append(into, MoveStatement{ From: addrs.ImpliedMoveStatementEndpoint(rAddr.Instance(fromKey), approxSrcRange), To: addrs.ImpliedMoveStatementEndpoint(rAddr.Instance(toKey), approxSrcRange), + Provider: provider, DeclRange: approxSrcRange, Implied: true, }) diff --git a/internal/refactoring/move_statement_test.go b/internal/refactoring/move_statement_test.go index c99eb0b23d..9332d06d97 100644 --- a/internal/refactoring/move_statement_test.go +++ b/internal/refactoring/move_statement_test.go @@ -118,8 +118,11 @@ func TestImpliedMoveStatements(t *testing.T) { got := ImpliedMoveStatements(rootCfg, prevRunState, explicitStmts) want := []MoveStatement{ { - From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("formerly_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), - To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("formerly_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("formerly_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("formerly_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + Provider: &addrs.AbsProviderConfig{ + Provider: addrs.NewProvider("registry.terraform.io", "hashicorp", "foo"), + }, Implied: true, DeclRange: tfdiags.SourceRange{ Filename: "testdata/move-statement-implied/move-statement-implied.tf", @@ -130,8 +133,12 @@ func TestImpliedMoveStatements(t *testing.T) { // Found implied moves in a nested module, ignoring the explicit moves { - From: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "formerly_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), - To: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "formerly_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + From: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "formerly_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "formerly_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + Provider: &addrs.AbsProviderConfig{ + Module: addrs.Module{"child"}, + Provider: addrs.NewProvider("registry.terraform.io", "hashicorp", "foo"), + }, Implied: true, DeclRange: tfdiags.SourceRange{ Filename: "testdata/move-statement-implied/child/move-statement-implied.tf", @@ -141,8 +148,11 @@ func TestImpliedMoveStatements(t *testing.T) { }, { - From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("now_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), - To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("now_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("now_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("now_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + Provider: &addrs.AbsProviderConfig{ + Provider: addrs.NewProvider("registry.terraform.io", "hashicorp", "foo"), + }, Implied: true, DeclRange: tfdiags.SourceRange{ Filename: "testdata/move-statement-implied/move-statement-implied.tf", @@ -153,8 +163,12 @@ func TestImpliedMoveStatements(t *testing.T) { // Found implied moves in a nested module, ignoring the explicit moves { - From: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "now_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), - To: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "now_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + From: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "now_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "now_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + Provider: &addrs.AbsProviderConfig{ + Module: addrs.Module{"child"}, + Provider: addrs.NewProvider("registry.terraform.io", "hashicorp", "foo"), + }, Implied: true, DeclRange: tfdiags.SourceRange{ Filename: "testdata/move-statement-implied/child/move-statement-implied.tf", @@ -169,8 +183,11 @@ func TestImpliedMoveStatements(t *testing.T) { // situation where an object wants to move into an address already // occupied by another object. { - From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("ambiguous").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), - To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("ambiguous").Instance(addrs.NoKey), tfdiags.SourceRange{}), + From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("ambiguous").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("ambiguous").Instance(addrs.NoKey), tfdiags.SourceRange{}), + Provider: &addrs.AbsProviderConfig{ + Provider: addrs.NewProvider("registry.terraform.io", "hashicorp", "foo"), + }, Implied: true, DeclRange: tfdiags.SourceRange{ Filename: "testdata/move-statement-implied/move-statement-implied.tf", diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index 2525ee7cf2..a0a2b55497 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -158,18 +158,6 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts StmtRange: stmt.DeclRange, }) } - - // Resource types must match. - if resourceTypesDiffer(absFrom, absTo) { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Resource type mismatch", - Detail: fmt.Sprintf( - "This statement declares a move from %s to %s, which is a %s of a different type.", absFrom, absTo, noun, - ), - }) - } - } } @@ -254,19 +242,6 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool { } } -func resourceTypesDiffer(absFrom, absTo addrs.AbsMoveable) bool { - switch absFrom := absFrom.(type) { - case addrs.AbsMoveableResource: - // addrs.UnifyMoveEndpoints guarantees that both addresses are of the - // same kind, so at this point we can assume that absTo is also an - // addrs.AbsResourceInstance or addrs.AbsResource. - absTo := absTo.(addrs.AbsMoveableResource) - return absFrom.AffectedAbsResource().Resource.Type != absTo.AffectedAbsResource().Resource.Type - default: - return false - } -} - func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiags.SourceRange, bool) { switch addr := addr.(type) { case addrs.ModuleInstance: diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 58f9011ea3..bcf42aced1 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -434,24 +434,6 @@ Each resource can have moved from only one source resource.`, }, WantError: ``, // This is okay because the call itself is not considered to be inside the package it refers to }, - "resource type mismatch": { - Statements: []MoveStatement{ - makeTestMoveStmt(t, ``, - `test.nonexist1`, - `other.single`, - ), - }, - WantError: `Resource type mismatch: This statement declares a move from test.nonexist1 to other.single, which is a resource of a different type.`, - }, - "resource instance type mismatch": { - Statements: []MoveStatement{ - makeTestMoveStmt(t, ``, - `test.nonexist1[0]`, - `other.single`, - ), - }, - WantError: `Resource type mismatch: This statement declares a move from test.nonexist1[0] to other.single, which is a resource instance of a different type.`, - }, "crossing nested statements": { // overlapping nested moves will result in a cycle. Statements: []MoveStatement{ diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index e31eeb95d6..ae52e3563b 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -497,7 +497,7 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State return destroyPlan, evalScope, diags } -func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State, targets []addrs.Targetable) ([]refactoring.MoveStatement, refactoring.MoveResults) { +func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State) ([]refactoring.MoveStatement, refactoring.MoveResults, tfdiags.Diagnostics) { explicitMoveStmts := refactoring.FindMoveStatements(config) implicitMoveStmts := refactoring.ImpliedMoveStatements(config, prevRunState, explicitMoveStmts) var moveStmts []refactoring.MoveStatement @@ -506,8 +506,8 @@ func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState moveStmts = append(moveStmts, explicitMoveStmts...) moveStmts = append(moveStmts, implicitMoveStmts...) } - moveResults := refactoring.ApplyMoves(moveStmts, prevRunState) - return moveStmts, moveResults + moveResults, diags := refactoring.ApplyMoves(moveStmts, prevRunState, c.plugins.providerFactories) + return moveStmts, moveResults, diags } func (c *Context) prePlanVerifyTargetedMoves(moveResults refactoring.MoveResults, targets []addrs.Targetable) tfdiags.Diagnostics { @@ -622,7 +622,11 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, o log.Printf("[DEBUG] Building and walking plan graph for %s", opts.Mode) prevRunState = prevRunState.DeepCopy() // don't modify the caller's object when we process the moves - moveStmts, moveResults := c.prePlanFindAndApplyMoves(config, prevRunState, opts.Targets) + moveStmts, moveResults, moveDiags := c.prePlanFindAndApplyMoves(config, prevRunState) + diags = diags.Append(moveDiags) + if moveDiags.HasErrors() { + return nil, nil, diags + } // If resource targeting is in effect then it might conflict with the // move result. diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 99427d3619..d0d8a73653 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -1712,6 +1712,302 @@ The -target option is not for routine use, and is provided only for exceptional }) } +func TestContext2Plan_crossResourceMoveBasic(t *testing.T) { + addrA := mustResourceInstanceAddr("test_object_one.a") + addrB := mustResourceInstanceAddr("test_object_two.a") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + resource "test_object_two" "a" { + } + + moved { + from = test_object_one.a + to = test_object_two.a + } + `, + }) + + 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(`{"value":"before"}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + p := &MockProvider{} + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_object_one": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + "test_object_two": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + }, + ServerCapabilities: providers.ServerCapabilities{ + MoveResourceState: true, + }, + } + p.MoveResourceStateResponse = &providers.MoveResourceStateResponse{ + TargetState: cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("after"), + }), + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + }) + 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.Update; 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_crossProviderMove(t *testing.T) { + addrA := mustResourceInstanceAddr("one_object.a") + addrB := mustResourceInstanceAddr("two_object.a") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + resource "two_object" "a" { + } + + moved { + from = one_object.a + to = two_object.a + } + `, + }) + + 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(`{"value":"before"}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/one"]`)) + }) + + one := &MockProvider{} + one.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "one_object": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + }, + } + + two := &MockProvider{} + two.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "two_object": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + }, + ServerCapabilities: providers.ServerCapabilities{ + MoveResourceState: true, + }, + } + two.MoveResourceStateResponse = &providers.MoveResourceStateResponse{ + TargetState: cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("after"), + }), + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("one"): testProviderFuncFixed(one), + addrs.NewDefaultProvider("two"): testProviderFuncFixed(two), + }, + }) + + plan, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + }) + 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.Update; 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_crossResourceMoveMissingConfig(t *testing.T) { + addrA := mustResourceInstanceAddr("test_object_one.a") + addrB := mustResourceInstanceAddr("test_object_two.a") + m := testModuleInline(t, map[string]string{ + "main.tf": ` + moved { + from = test_object_one.a + to = test_object_two.a + } + `, + }) + + 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(`{"value":"before"}`), + Status: states.ObjectReady, + }, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`)) + }) + + p := &MockProvider{} + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + Provider: providers.Schema{}, + ResourceTypes: map[string]providers.Schema{ + "test_object_one": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + "test_object_two": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Optional: true, + }, + }, + }, + }, + }, + } + p.MoveResourceStateResponse = &providers.MoveResourceStateResponse{ + TargetState: cty.ObjectVal(map[string]cty.Value{ + "value": cty.StringVal("after"), + }), + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + plan, diags := ctx.Plan(m, state, &PlanOpts{ + Mode: plans.NormalMode, + }) + 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.Delete; got != want { + t.Errorf("wrong planned action\ngot: %s\nwant: %s", got, want) + } + if got, want := instPlan.ActionReason, plans.ResourceInstanceDeleteBecauseNoMoveTarget; got != want { + t.Errorf("wrong action reason\ngot: %s\nwant: %s", got, want) + } + }) +} + func TestContext2Plan_untargetedResourceSchemaChange(t *testing.T) { // an untargeted resource which requires a schema migration should not // block planning due external changes in the plan.