resilien
/
terraform
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

refactoring: First round of ValidateMoves rules 

This is a first pass at implementing refactoring.ValidateMoves, covering
the main validation rules.

This is not yet complete. A couple situations not yet covered are
represented by commented test cases in TestValidateMoves, although that
isn't necessarily comprehensive. We'll do a further pass of filling this
out with any other subtleties before we ship this feature.
This commit is contained in:
Martin Atkins 2021-07-28 18:27:31 -07:00
parent ae2c93f255
commit aa414f3ab3
6 changed files with 1072 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

85
internal/addrs/move_endpoint_module.go
View File

@ -64,6 +64,91 @@ func (e *MoveEndpointInModule) String() string {
return buf.String()
}
// Module returns the address of the module where the receiving address was
// declared.
func (e *MoveEndpointInModule) Module() Module {
return e.module
}
// InModuleInstance returns an AbsMovable address which concatenates the
// given module instance address with the receiver's relative object selection
// to produce one example of an instance that might be affected by this
// move statement.
//
// The result is meaningful only if the given module instance is an instance
// of the same module returned by the method Module. InModuleInstance doesn't
// fully verify that (aside from some cheap/easy checks), but it will produce
// meaningless garbage if not.
func (e *MoveEndpointInModule) InModuleInstance(modInst ModuleInstance) AbsMoveable {
if len(modInst) != len(e.module) {
// We don't check all of the steps to make sure that their names match,
// because it would be expensive to do that repeatedly for every
// instance of a module, but if the lengths don't match then that's
// _obviously_ wrong.
panic("given instance address does not match module address")
}
switch relSubject := e.relSubject.(type) {
case ModuleInstance:
ret := make(ModuleInstance, 0, len(modInst)+len(relSubject))
ret = append(ret, modInst...)
ret = append(ret, relSubject...)
return ret
case AbsModuleCall:
retModAddr := make(ModuleInstance, 0, len(modInst)+len(relSubject.Module))
retModAddr = append(retModAddr, modInst...)
retModAddr = append(retModAddr, relSubject.Module...)
return relSubject.Call.Absolute(retModAddr)
case AbsResourceInstance:
retModAddr := make(ModuleInstance, 0, len(modInst)+len(relSubject.Module))
retModAddr = append(retModAddr, modInst...)
retModAddr = append(retModAddr, relSubject.Module...)
return relSubject.Resource.Absolute(retModAddr)
case AbsResource:
retModAddr := make(ModuleInstance, 0, len(modInst)+len(relSubject.Module))
retModAddr = append(retModAddr, modInst...)
retModAddr = append(retModAddr, relSubject.Module...)
return relSubject.Resource.Absolute(retModAddr)
default:
panic(fmt.Sprintf("unexpected move subject type %T", relSubject))
}
}
// ModuleCallTraversals returns both the address of the module where the
// receiver was declared and any other module calls it traverses through
// while selecting a particular object to move.
//
// This is a rather special-purpose function here mainly to support our
// validation rule that a module can only traverse down into child modules
// that belong to the same module package.
func (e *MoveEndpointInModule) ModuleCallTraversals() (Module, []ModuleCall) {
// We're returning []ModuleCall rather than Module here to make it clearer
// that this is a relative sequence of calls rather than an absolute
// module path.
var steps []ModuleInstanceStep
switch relSubject := e.relSubject.(type) {
case ModuleInstance:
// We want all of the steps except the last one here, because the
// last one is always selecting something declared in the same module
// even though our address structure doesn't capture that.
steps = []ModuleInstanceStep(relSubject[:len(relSubject)-1])
case AbsModuleCall:
steps = []ModuleInstanceStep(relSubject.Module)
case AbsResourceInstance:
steps = []ModuleInstanceStep(relSubject.Module)
case AbsResource:
steps = []ModuleInstanceStep(relSubject.Module)
default:
panic(fmt.Sprintf("unexpected move subject type %T", relSubject))
}
ret := make([]ModuleCall, len(steps))
for i, step := range steps {
ret[i] = ModuleCall{Name: step.Name}
}
return e.module, ret
}
// SelectsModule returns true if the reciever directly selects either
// the given module or a resource nested directly inside that module.
//

10
internal/instances/set.go
View File

@ -39,3 +39,13 @@ func (s Set) HasResourceInstance(want addrs.AbsResourceInstance) bool {
func (s Set) HasResource(want addrs.AbsResource) bool {
return s.exp.knowsResource(want)
}
// InstancesForModule returns all of the module instances that correspond with
// the given static module path.
//
// If there are multiple module calls in the path that have repetition enabled
// then the result is the full expansion of all combinations of all of their
// declared instance keys.
func (s Set) InstancesForModule(modAddr addrs.Module) []addrs.ModuleInstance {
return s.exp.ExpandModule(modAddr)
}

303
internal/refactoring/move_validate.go
View File

@ -2,7 +2,11 @@ package refactoring
import (
"fmt"
"sort"
"strings"
"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/instances"
"github.com/hashicorp/terraform/internal/tfdiags"
@ -29,12 +33,303 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts
g := buildMoveStatementGraph(stmts)
if len(g.Cycles()) != 0 {
// TODO: proper error messages for this
diags = diags.Append(fmt.Errorf("move statement cycles"))
// We need to track the absolute versions of our endpoint addresses in
// order to detect when there are ambiguous moves.
type AbsMoveEndpoint struct {
Other addrs.AbsMoveable
StmtRange tfdiags.SourceRange
}
stmtFrom := map[addrs.UniqueKey]AbsMoveEndpoint{}
stmtTo := map[addrs.UniqueKey]AbsMoveEndpoint{}
for _, stmt := range stmts {
// Earlier code that constructs MoveStatement values should ensure that
// both stmt.From and stmt.To always belong to the same statement and
// thus to the same module.
stmtMod, fromCallSteps := stmt.From.ModuleCallTraversals()
_, toCallSteps := stmt.To.ModuleCallTraversals()
modCfg := rootCfg.Descendent(stmtMod)
if pkgAddr := callsThroughModulePackage(modCfg, fromCallSteps); pkgAddr != nil {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Cross-package move statement",
Detail: fmt.Sprintf(
"This statement declares a move from an object declared in external module package %q. Move statements can be only within a single module package.",
pkgAddr,
),
Subject: stmt.DeclRange.ToHCL().Ptr(),
})
}
if pkgAddr := callsThroughModulePackage(modCfg, toCallSteps); pkgAddr != nil {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Cross-package move statement",
Detail: fmt.Sprintf(
"This statement declares a move to an object declared in external module package %q. Move statements can be only within a single module package.",
pkgAddr,
),
Subject: stmt.DeclRange.ToHCL().Ptr(),
})
}
for _, modInst := range declaredInsts.InstancesForModule(stmtMod) {
absFrom := stmt.From.InModuleInstance(modInst)
absTo := stmt.To.InModuleInstance(modInst)
fromKey := absFrom.UniqueKey()
toKey := absTo.UniqueKey()
if fromKey == toKey {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Redundant move statement",
Detail: fmt.Sprintf(
"This statement declares a move from %s to the same address, which is the same as not declaring this move at all.",
absFrom,
),
Subject: stmt.DeclRange.ToHCL().Ptr(),
})
continue
}
var noun string
var shortNoun string
switch absFrom.(type) {
case addrs.ModuleInstance:
noun = "module instance"
shortNoun = "instance"
case addrs.AbsModuleCall:
noun = "module call"
shortNoun = "call"
case addrs.AbsResourceInstance:
noun = "resource instance"
shortNoun = "instance"
case addrs.AbsResource:
noun = "resource"
shortNoun = "resource"
default:
// The above cases should cover all of the AbsMoveable types
panic("unsupported AbsMovable address type")
}
// It's invalid to have a move statement whose "from" address
// refers to something that is still declared in the configuration.
if moveableObjectExists(absFrom, declaredInsts) {
conflictRange, hasRange := movableObjectDeclRange(absFrom, rootCfg)
declaredAt := ""
if hasRange {
// NOTE: It'd be pretty weird to _not_ have a range, since
// we're only in this codepath because the plan phase
// thought this object existed in the configuration.
declaredAt = fmt.Sprintf(" at %s", conflictRange.StartString())
}
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Moved object still exists",
Detail: fmt.Sprintf(
"This statement declares a move from %s, but that %s is still declared%s.\n\nChange your configuration so that this %s will be declared as %s instead.",
absFrom, noun, declaredAt, shortNoun, absTo,
),
Subject: stmt.DeclRange.ToHCL().Ptr(),
})
}
// There can only be one destination for each source address.
if existing, exists := stmtFrom[fromKey]; exists {
if existing.Other.UniqueKey() != toKey {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Ambiguous move statements",
Detail: fmt.Sprintf(
"A statement at %s declared that %s moved to %s, but this statement instead declares that it moved to %s.\n\nEach %s can move to only one destination %s.",
existing.StmtRange.StartString(), absFrom, existing.Other, absTo,
noun, shortNoun,
),
Subject: stmt.DeclRange.ToHCL().Ptr(),
})
}
} else {
stmtFrom[fromKey] = AbsMoveEndpoint{
Other: absTo,
StmtRange: stmt.DeclRange,
}
}
// There can only be one source for each destination address.
if existing, exists := stmtTo[toKey]; exists {
if existing.Other.UniqueKey() != fromKey {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Ambiguous move statements",
Detail: fmt.Sprintf(
"A statement at %s declared that %s moved to %s, but this statement instead declares that %s moved there.\n\nEach %s can have moved from only one source %s.",
existing.StmtRange.StartString(), existing.Other, absTo, absFrom,
noun, shortNoun,
),
Subject: stmt.DeclRange.ToHCL().Ptr(),
})
}
} else {
stmtTo[toKey] = AbsMoveEndpoint{
Other: absFrom,
StmtRange: stmt.DeclRange,
}
}
}
}
// TODO: Various other validation rules
// If we're not already returning other errors then we'll also check for
// and report cycles.
//
// Cycles alone are difficult to report in a helpful way because we don't
// have enough context to guess the user's intent. However, some particular
// mistakes that might lead to a cycle can also be caught by other
// validation rules above where we can make better suggestions, and so
// we'll use a cycle report only as a last resort.
if !diags.HasErrors() {
for _, cycle := range g.Cycles() {
// Reporting cycles is awkward because there isn't any definitive
// way to decide which of the objects in the cycle is the cause of
// the problem. Therefore we'll just list them all out and leave
// the user to figure it out. :(
stmtStrs := make([]string, 0, len(cycle))
for _, stmtI := range cycle {
// move statement graph nodes are pointers to move statements
stmt := stmtI.(*MoveStatement)
stmtStrs = append(stmtStrs, fmt.Sprintf(
"\n - %s: %s → %s",
stmt.DeclRange.StartString(),
stmt.From.String(),
stmt.To.String(),
))
}
sort.Strings(stmtStrs) // just to make the order deterministic
diags = diags.Append(tfdiags.Sourceless(
tfdiags.Error,
"Cyclic dependency in move statements",
fmt.Sprintf(
"The following chained move statements form a cycle, and so there is no final location to move objects to:%s\n\nA chain of move statements must end with an address that doesn't appear in any other statements, and which typically also refers to an object still declared in the configuration.",
strings.Join(stmtStrs, ""),
),
))
}
}
return diags
}
func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool {
switch addr := addr.(type) {
case addrs.ModuleInstance:
return in.HasModuleInstance(addr)
case addrs.AbsModuleCall:
return in.HasModuleCall(addr)
case addrs.AbsResourceInstance:
return in.HasResourceInstance(addr)
case addrs.AbsResource:
return in.HasResource(addr)
default:
// The above cases should cover all of the AbsMoveable types
panic("unsupported AbsMovable address type")
}
}
func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiags.SourceRange, bool) {
switch addr := addr.(type) {
case addrs.ModuleInstance:
// For a module instance we're actually looking for the call that
// declared it, which belongs to the parent module.
// (NOTE: This assumes "addr" can never be the root module instance,
// because the root module is never moveable.)
parentAddr, callAddr := addr.Call()
modCfg := cfg.DescendentForInstance(parentAddr)
if modCfg == nil {
return tfdiags.SourceRange{}, false
}
call := modCfg.Module.ModuleCalls[callAddr.Name]
if call == nil {
return tfdiags.SourceRange{}, false
}
// If the call has either count or for_each set then we'll "blame"
// that expression, rather than the block as a whole, because it's
// the expression that decides which instances are available.
switch {
case call.ForEach != nil:
return tfdiags.SourceRangeFromHCL(call.ForEach.Range()), true
case call.Count != nil:
return tfdiags.SourceRangeFromHCL(call.Count.Range()), true
default:
return tfdiags.SourceRangeFromHCL(call.DeclRange), true
}
case addrs.AbsModuleCall:
modCfg := cfg.DescendentForInstance(addr.Module)
if modCfg == nil {
return tfdiags.SourceRange{}, false
}
call := modCfg.Module.ModuleCalls[addr.Call.Name]
if call == nil {
return tfdiags.SourceRange{}, false
}
return tfdiags.SourceRangeFromHCL(call.DeclRange), true
case addrs.AbsResourceInstance:
modCfg := cfg.DescendentForInstance(addr.Module)
if modCfg == nil {
return tfdiags.SourceRange{}, false
}
rc := modCfg.Module.ResourceByAddr(addr.Resource.Resource)
if rc == nil {
return tfdiags.SourceRange{}, false
}
// If the resource has either count or for_each set then we'll "blame"
// that expression, rather than the block as a whole, because it's
// the expression that decides which instances are available.
switch {
case rc.ForEach != nil:
return tfdiags.SourceRangeFromHCL(rc.ForEach.Range()), true
case rc.Count != nil:
return tfdiags.SourceRangeFromHCL(rc.Count.Range()), true
default:
return tfdiags.SourceRangeFromHCL(rc.DeclRange), true
}
case addrs.AbsResource:
modCfg := cfg.DescendentForInstance(addr.Module)
if modCfg == nil {
return tfdiags.SourceRange{}, false
}
rc := modCfg.Module.ResourceByAddr(addr.Resource)
if rc == nil {
return tfdiags.SourceRange{}, false
}
return tfdiags.SourceRangeFromHCL(rc.DeclRange), true
default:
// The above cases should cover all of the AbsMoveable types
panic("unsupported AbsMovable address type")
}
}
func callsThroughModulePackage(modCfg *configs.Config, callSteps []addrs.ModuleCall) addrs.ModuleSource {
var sourceAddr addrs.ModuleSource
current := modCfg
for _, step := range callSteps {
call := current.Module.ModuleCalls[step.Name]
if call == nil {
break
}
if call.EntersNewPackage() {
sourceAddr = call.SourceAddr
}
current = modCfg.Children[step.Name]
if current == nil {
// Weird to have a call but not a config, but we'll tolerate
// it to avoid crashing here.
break
}
}
return sourceAddr
}

607
internal/refactoring/move_validate_test.go Normal file
View File

@ -0,0 +1,607 @@
package refactoring
import (
"strings"
"testing"
"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/hclsyntax"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/configs/configload"
"github.com/hashicorp/terraform/internal/initwd"
"github.com/hashicorp/terraform/internal/instances"
"github.com/hashicorp/terraform/internal/registry"
"github.com/hashicorp/terraform/internal/tfdiags"
"github.com/zclconf/go-cty/cty/gocty"
)
func TestValidateMoves(t *testing.T) {
rootCfg, instances := loadRefactoringFixture(t, "testdata/move-validate-zoo")
tests := map[string]struct {
Statements []MoveStatement
WantError string
}{
"no move statements": {
Statements: nil,
WantError: ``,
},
"some valid statements": {
Statements: []MoveStatement{
// This is just a grab bag of various valid cases that don't
// generate any errors at all.
makeTestMoveStmt(t,
``,
`test.nonexist1`,
`test.target1`,
),
makeTestMoveStmt(t,
`single`,
`test.nonexist1`,
`test.target1`,
),
makeTestMoveStmt(t,
``,
`test.nonexist2`,
`module.nonexist.test.nonexist2`,
),
makeTestMoveStmt(t,
``,
`module.single.test.nonexist3`,
`module.single.test.single`,
),
makeTestMoveStmt(t,
``,
`module.single.test.nonexist4`,
`test.target2`,
),
makeTestMoveStmt(t,
``,
`test.single[0]`, // valid because test.single doesn't have "count" set
`test.target3`,
),
makeTestMoveStmt(t,
``,
`test.zero_count[0]`, // valid because test.zero_count has count = 0
`test.target4`,
),
makeTestMoveStmt(t,
``,
`test.zero_count[1]`, // valid because test.zero_count has count = 0
`test.zero_count[0]`,
),
makeTestMoveStmt(t,
``,
`module.nonexist1`,
`module.target3`,
),
makeTestMoveStmt(t,
``,
`module.nonexist1[0]`,
`module.target4`,
),
makeTestMoveStmt(t,
``,
`module.single[0]`, // valid because module.single doesn't have "count" set
`module.target5`,
),
makeTestMoveStmt(t,
``,
`module.for_each["nonexist1"]`,
`module.for_each["a"]`,
),
makeTestMoveStmt(t,
``,
`module.for_each["nonexist2"]`,
`module.nonexist.module.nonexist`,
),
makeTestMoveStmt(t,
``,
`module.for_each["nonexist3"].test.single`, // valid because module.for_each doesn't currently have a "nonexist3"
`module.for_each["a"].test.single`,
),
},
WantError: ``,
},
"two statements with the same endpoints": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.a`,
`module.b`,
),
makeTestMoveStmt(t,
``,
`module.a`,
`module.b`,
),
},
WantError: ``,
},
"moving nowhere": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.a`,
`module.a`,
),
},
WantError: `Redundant move statement: This statement declares a move from module.a to the same address, which is the same as not declaring this move at all.`,
},
/*
// TODO: This test can't pass until we've implemented
// addrs.MoveEndpointInModule.CanChainFrom, which is what
// detects the chaining condition this is testing for.
"cyclic chain": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.a`,
`module.b`,
),
makeTestMoveStmt(t,
``,
`module.b`,
`module.c`,
),
makeTestMoveStmt(t,
``,
`module.c`,
`module.a`,
),
},
WantError: `bad cycle`,
},
*/
"module.single as a call still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.single`,
`module.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.single, but that module call is still declared at testdata/move-validate-zoo/move-validate-root.tf:6,1.
Change your configuration so that this call will be declared as module.other instead.`,
},
"module.single as an instance still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.single`,
`module.other[0]`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.single, but that module instance is still declared at testdata/move-validate-zoo/move-validate-root.tf:6,1.
Change your configuration so that this instance will be declared as module.other[0] instead.`,
},
"module.count[0] still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.count[0]`,
`module.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.count[0], but that module instance is still declared at testdata/move-validate-zoo/move-validate-root.tf:12,12.
Change your configuration so that this instance will be declared as module.other instead.`,
},
`module.for_each["a"] still exists in configuration`: {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.for_each["a"]`,
`module.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.for_each["a"], but that module instance is still declared at testdata/move-validate-zoo/move-validate-root.tf:22,14.
Change your configuration so that this instance will be declared as module.other instead.`,
},
"test.single as a resource still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.single`,
`test.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from test.single, but that resource is still declared at testdata/move-validate-zoo/move-validate-root.tf:27,1.
Change your configuration so that this resource will be declared as test.other instead.`,
},
"test.single as an instance still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.single`,
`test.other[0]`,
),
},
WantError: `Moved object still exists: This statement declares a move from test.single, but that resource instance is still declared at testdata/move-validate-zoo/move-validate-root.tf:27,1.
Change your configuration so that this instance will be declared as test.other[0] instead.`,
},
"module.single.test.single as a resource still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.single.test.single`,
`test.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.single.test.single, but that resource is still declared at testdata/move-validate-zoo/child/move-validate-child.tf:6,1.
Change your configuration so that this resource will be declared as test.other instead.`,
},
"module.single.test.single as a resource declared in module.single still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
`single`,
`test.single`,
`test.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.single.test.single, but that resource is still declared at testdata/move-validate-zoo/child/move-validate-child.tf:6,1.
Change your configuration so that this resource will be declared as module.single.test.other instead.`,
},
"module.single.test.single as an instance still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.single.test.single`,
`test.other[0]`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.single.test.single, but that resource instance is still declared at testdata/move-validate-zoo/child/move-validate-child.tf:6,1.
Change your configuration so that this instance will be declared as test.other[0] instead.`,
},
"module.count[0].test.single still exists in configuration": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.count[0].test.single`,
`test.other`,
),
},
WantError: `Moved object still exists: This statement declares a move from module.count[0].test.single, but that resource is still declared at testdata/move-validate-zoo/child/move-validate-child.tf:6,1.
Change your configuration so that this resource will be declared as test.other instead.`,
},
"two different moves from test.nonexist": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.nonexist`,
`test.other1`,
),
makeTestMoveStmt(t,
``,
`test.nonexist`,
`test.other2`,
),
},
WantError: `Ambiguous move statements: A statement at test:1,1 declared that test.nonexist moved to test.other1, but this statement instead declares that it moved to test.other2.
Each resource can move to only one destination resource.`,
},
"two different moves to test.single": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.other1`,
`test.single`,
),
makeTestMoveStmt(t,
``,
`test.other2`,
`test.single`,
),
},
WantError: `Ambiguous move statements: A statement at test:1,1 declared that test.other1 moved to test.single, but this statement instead declares that test.other2 moved there.
Each resource can have moved from only one source resource.`,
},
"two different moves to module.count[0].test.single across two modules": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.other1`,
`module.count[0].test.single`,
),
makeTestMoveStmt(t,
`count`,
`test.other2`,
`test.single`,
),
},
WantError: `Ambiguous move statements: A statement at test:1,1 declared that test.other1 moved to module.count[0].test.single, but this statement instead declares that module.count[0].test.other2 moved there.
Each resource can have moved from only one source resource.`,
},
/*
// FIXME: This rule requires a deeper analysis to understand that
// module.single already contains a test.single and thus moving
// it to module.foo implicitly also moves module.single.test.single
// module.foo.test.single.
"two different moves to nested test.single by different paths": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.beep`,
`module.foo.test.single`,
),
makeTestMoveStmt(t,
``,
`module.single`,
`module.foo`,
),
},
WantError: `Ambiguous move statements: A statement at test:1,1 declared that test.beep moved to module.foo.test.single, but this statement instead declares that module.single.test.single moved there.
Each resource can have moved from only one source resource.`,
},
*/
"move from resource in another module package": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.fake_external.test.thing`,
`test.thing`,
),
},
WantError: `Cross-package move statement: This statement declares a move from an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`,
},
"move to resource in another module package": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`test.thing`,
`module.fake_external.test.thing`,
),
},
WantError: `Cross-package move statement: This statement declares a move to an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`,
},
"move from module call in another module package": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.fake_external.module.a`,
`module.b`,
),
},
WantError: `Cross-package move statement: This statement declares a move from an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`,
},
"move to module call in another module package": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.a`,
`module.fake_external.module.b`,
),
},
WantError: `Cross-package move statement: This statement declares a move to an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`,
},
"move to a call that refers to another module package": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.nonexist`,
`module.fake_external`,
),
},
WantError: ``, // This is okay because the call itself is not considered to be inside the package it refers to
},
"move to instance of a call that refers to another module package": {
Statements: []MoveStatement{
makeTestMoveStmt(t,
``,
`module.nonexist`,
`module.fake_external[0]`,
),
},
WantError: ``, // This is okay because the call itself is not considered to be inside the package it refers to
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
gotDiags := ValidateMoves(test.Statements, rootCfg, instances)
switch {
case test.WantError != "":
if !gotDiags.HasErrors() {
t.Fatalf("unexpected success\nwant error: %s", test.WantError)
}
if got, want := gotDiags.Err().Error(), test.WantError; got != want {
t.Fatalf("wrong error\ngot error: %s\nwant error: %s", got, want)
}
default:
if gotDiags.HasErrors() {
t.Fatalf("unexpected error\ngot error: %s", gotDiags.Err().Error())
}
}
})
}
}
// loadRefactoringFixture reads a configuration from the given directory and
// does some naive static processing on any count and for_each expressions
// inside, in order to get a realistic-looking instances.Set for what it
// declares without having to run a full Terraform plan.
func loadRefactoringFixture(t *testing.T, dir string) (*configs.Config, instances.Set) {
t.Helper()
loader, cleanup := configload.NewLoaderForTests(t)
defer cleanup()
inst := initwd.NewModuleInstaller(loader.ModulesDir(), registry.NewClient(nil, nil))
_, instDiags := inst.InstallModules(dir, true, initwd.ModuleInstallHooksImpl{})
if instDiags.HasErrors() {
t.Fatal(instDiags.Err())
}
// Since module installer has modified the module manifest on disk, we need
// to refresh the cache of it in the loader.
if err := loader.RefreshModules(); err != nil {
t.Fatalf("failed to refresh modules after installation: %s", err)
}
rootCfg, diags := loader.LoadConfig(dir)
if diags.HasErrors() {
t.Fatalf("failed to load root module: %s", diags.Error())
}
expander := instances.NewExpander()
staticPopulateExpanderModule(t, rootCfg, addrs.RootModuleInstance, expander)
return rootCfg, expander.AllInstances()
}
func staticPopulateExpanderModule(t *testing.T, rootCfg *configs.Config, moduleAddr addrs.ModuleInstance, expander *instances.Expander) {
t.Helper()
modCfg := rootCfg.DescendentForInstance(moduleAddr)
if modCfg == nil {
t.Fatalf("no configuration for %s", moduleAddr)
}
if len(modCfg.Path) > 0 && modCfg.Path[len(modCfg.Path)-1] == "fake_external" {
// As a funny special case we modify the source address of this
// module to be something that counts as a separate package,
// so we can test rules relating to crossing package boundaries
// even though we really just loaded the module from a local path.
modCfg.SourceAddr = fakeExternalModuleSource
}
for _, call := range modCfg.Module.ModuleCalls {
callAddr := addrs.ModuleCall{Name: call.Name}
if call.Name == "fake_external" {
// As a funny special case we modify the source address of this
// module to be something that counts as a separate package,
// so we can test rules relating to crossing package boundaries
// even though we really just loaded the module from a local path.
call.SourceAddr = fakeExternalModuleSource
}
// In order to get a valid, useful set of instances here we're going
// to just statically evaluate the count and for_each expressions.
// Normally it's valid to use references and functions there, but for
// our unit tests we'll just limit it to literal values to avoid
// bringing all of the core evaluator complexity.
switch {
case call.ForEach != nil:
val, diags := call.ForEach.Value(nil)
if diags.HasErrors() {
t.Fatalf("invalid for_each: %s", diags.Error())
}
expander.SetModuleForEach(moduleAddr, callAddr, val.AsValueMap())
case call.Count != nil:
val, diags := call.Count.Value(nil)
if diags.HasErrors() {
t.Fatalf("invalid count: %s", diags.Error())
}
var count int
err := gocty.FromCtyValue(val, &count)
if err != nil {
t.Fatalf("invalid count at %s: %s", call.Count.Range(), err)
}
expander.SetModuleCount(moduleAddr, callAddr, count)
default:
expander.SetModuleSingle(moduleAddr, callAddr)
}
// We need to recursively analyze the child modules too.
calledMod := modCfg.Path.Child(call.Name)
for _, inst := range expander.ExpandModule(calledMod) {
staticPopulateExpanderModule(t, rootCfg, inst, expander)
}
}
for _, rc := range modCfg.Module.ManagedResources {
staticPopulateExpanderResource(t, moduleAddr, rc, expander)
}
for _, rc := range modCfg.Module.DataResources {
staticPopulateExpanderResource(t, moduleAddr, rc, expander)
}
}
func staticPopulateExpanderResource(t *testing.T, moduleAddr addrs.ModuleInstance, rCfg *configs.Resource, expander *instances.Expander) {
t.Helper()
addr := rCfg.Addr()
switch {
case rCfg.ForEach != nil:
val, diags := rCfg.ForEach.Value(nil)
if diags.HasErrors() {
t.Fatalf("invalid for_each: %s", diags.Error())
}
expander.SetResourceForEach(moduleAddr, addr, val.AsValueMap())
case rCfg.Count != nil:
val, diags := rCfg.Count.Value(nil)
if diags.HasErrors() {
t.Fatalf("invalid count: %s", diags.Error())
}
var count int
err := gocty.FromCtyValue(val, &count)
if err != nil {
t.Fatalf("invalid count at %s: %s", rCfg.Count.Range(), err)
}
expander.SetResourceCount(moduleAddr, addr, count)
default:
expander.SetResourceSingle(moduleAddr, addr)
}
}
func makeTestMoveStmt(t *testing.T, moduleStr, fromStr, toStr string) MoveStatement {
t.Helper()
module := addrs.RootModule
if moduleStr != "" {
module = addrs.Module(strings.Split(moduleStr, "."))
}
traversal, hclDiags := hclsyntax.ParseTraversalAbs([]byte(fromStr), "", hcl.InitialPos)
if hclDiags.HasErrors() {
t.Fatalf("invalid from address: %s", hclDiags.Error())
}
fromEP, diags := addrs.ParseMoveEndpoint(traversal)
if diags.HasErrors() {
t.Fatalf("invalid from address: %s", diags.Err().Error())
}
traversal, hclDiags = hclsyntax.ParseTraversalAbs([]byte(toStr), "", hcl.InitialPos)
if hclDiags.HasErrors() {
t.Fatalf("invalid to address: %s", hclDiags.Error())
}
toEP, diags := addrs.ParseMoveEndpoint(traversal)
if diags.HasErrors() {
t.Fatalf("invalid to address: %s", diags.Err().Error())
}
fromInModule, toInModule := addrs.UnifyMoveEndpoints(module, fromEP, toEP)
if fromInModule == nil || toInModule == nil {
t.Fatalf("incompatible move endpoints")
}
return MoveStatement{
From: fromInModule,
To: toInModule,
DeclRange: tfdiags.SourceRange{
Filename: "test",
Start: tfdiags.SourcePos{Line: 1, Column: 1},
End: tfdiags.SourcePos{Line: 1, Column: 1},
},
}
}
var fakeExternalModuleSource = addrs.ModuleSourceRemote{
PackageAddr: addrs.ModulePackage("fake-external:///"),
}

21
internal/refactoring/testdata/move-validate-zoo/child/move-validate-child.tf vendored Normal file
View File

@ -0,0 +1,21 @@
# NOTE: This fixture is used in a test that doesn't run a full Terraform plan
# operation, so the count and for_each expressions here can only be literal
# values and mustn't include any references or function calls.
resource "test" "single" {
}
resource "test" "count" {
count = 2
}
resource "test" "zero_count" {
count = 0
}
resource "test" "for_each" {
for_each = {
a = "A"
}
}

50
internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf vendored Normal file
View File

@ -0,0 +1,50 @@
# NOTE: This fixture is used in a test that doesn't run a full Terraform plan
# operation, so the count and for_each expressions here can only be literal
# values and mustn't include any references or function calls.
module "single" {
source = "./child"
}
module "count" {
source = "./child"
count = 2
}
module "zero_count" {
source = "./child"
count = 0
}
module "for_each" {
source = "./child"
for_each = {
a = "A"
}
}
resource "test" "single" {
}
resource "test" "count" {
count = 2
}
resource "test" "zero_count" {
count = 0
}
resource "test" "for_each" {
for_each = {
a = "A"
}
}
module "fake_external" {
# Our configuration fixture loader has a special case for a module call
# named "fake_external" where it will mutate the source address after
# loading to instead be an external address, so we can test rules relating
# to crossing module boundaries.
source = "./child"
}