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

Merge pull request #29659 from hashicorp/jbardin/really-nested-within 

refactoring: exhaustive NestedWithin checks
This commit is contained in:
James Bardin 2021-09-27 12:55:04 -04:00 committed by GitHub
parent a742d7ee88 cac1f5c264
commit 372814e49a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 139 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
internal/refactoring/move_execute.go
View File

@ -200,10 +200,13 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph {
for dependerI := range stmts {
depender := &stmts[dependerI]
for dependeeI := range stmts {
if dependerI == dependeeI {
// skip comparing the statement to itself
continue
}
dependee := &stmts[dependeeI]
dependeeTo := dependee.To
dependerFrom := depender.From
if dependerFrom.CanChainFrom(dependeeTo) || dependerFrom.NestedWithin(dependeeTo) {
if statementDependsOn(depender, dependee) {
g.Connect(dag.BasicEdge(depender, dependee))
}
}
@ -212,6 +215,36 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph {
return g
}
// statementDependsOn returns true if statement a depends on statement b;
// i.e. statement b must be executed before statement a.
func statementDependsOn(a, b *MoveStatement) bool {
// chain-able moves are simple, as on the destination of one move could be
// equal to the source of another.
if a.From.CanChainFrom(b.To) {
return true
}
// Statement nesting in more complex, as we have 8 possible combinations to
// assess. Here we list all combinations, along with the statement which
// must be executed first when one address is nested within another.
// A.From IsNestedWithin B.From => A
// A.From IsNestedWithin B.To => B
// A.To IsNestedWithin B.From => A
// A.To IsNestedWithin B.To => B
// B.From IsNestedWithin A.From => B
// B.From IsNestedWithin A.To => A
// B.To IsNestedWithin A.From => B
// B.To IsNestedWithin A.To => A
//
// Since we are only interested in checking if A depends on B, we only need
// to check the 4 possibilities above which result in B being executed
// first.
return a.From.NestedWithin(b.To) ||
a.To.NestedWithin(b.To) ||
b.From.NestedWithin(a.From) ||
b.To.NestedWithin(a.From)
}
// MoveResults describes the outcome of an ApplyMoves call.
type MoveResults struct {
// Changes is a map from the unique keys of the final new resource

204
internal/refactoring/move_execute_test.go
View File

@ -491,118 +491,120 @@ func TestApplyMoves(t *testing.T) {
`foo.to[0]`,
},
},
// FIXME: This test seems to flap between the result the test case
// currently records and the "more intuitive" results included inline,
// which suggests we have a missing edge in our move dependency graph.
// (The MoveResults commented out below predates some changes to that
// struct, so will need updating once we uncomment this test.)
/*
"move module and then move resource into it": {
[]MoveStatement{
testMoveStatement(t, "", "module.bar[0]", "module.boo"),
testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
"move resource and containing module": {
[]MoveStatement{
testMoveStatement(t, "", "module.boo", "module.bar[0]"),
testMoveStatement(t, "boo", "foo.from", "foo.to"),
},
states.BuildState(func(s *states.SyncState) {
s.SetResourceInstanceCurrent(
instAddrs["module.boo.foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
}),
MoveResults{
Changes: map[addrs.UniqueKey]MoveSuccess{
instAddrs["module.bar[0].foo.to"].UniqueKey(): {
From: instAddrs["module.boo.foo.from"],
To: instAddrs["module.bar[0].foo.to"],
},
},
states.BuildState(func(s *states.SyncState) {
s.SetResourceInstanceCurrent(
Blocked: map[addrs.UniqueKey]MoveBlocked{},
},
[]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"),
},
states.BuildState(func(s *states.SyncState) {
s.SetResourceInstanceCurrent(
instAddrs["module.bar[0].foo.to"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
s.SetResourceInstanceCurrent(
instAddrs["foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
}),
MoveResults{
Changes: map[addrs.UniqueKey]MoveSuccess{
instAddrs["module.boo.foo.from"].UniqueKey(): {
instAddrs["foo.from"],
instAddrs["module.boo.foo.from"],
},
instAddrs["module.boo.foo.to"].UniqueKey(): {
instAddrs["module.bar[0].foo.to"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
s.SetResourceInstanceCurrent(
instAddrs["foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
}),
MoveResults{
// FIXME: This result is counter-intuitive, because ApplyMoves
// handled the resource move first and then the module move
// collided with it. It would be arguably more intuitive to
// complete the module move first and then let the "smaller"
// resource move merge into it.
// (The arguably-more-intuitive results are commented out
// in the maps below.)
Changes: map[addrs.UniqueKey]MoveSuccess{
//instAddrs["module.boo.foo.to"].UniqueKey(): instAddrs["module.bar[0].foo.to"],
//instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"],
instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"],
},
Blocked: map[addrs.UniqueKey]MoveBlocked{
// intuitive result: nothing blocked
instAddrs["module.bar[0].foo.to"].Module.UniqueKey(): instAddrs["module.boo.foo.from"].Module,
instAddrs["module.boo.foo.to"],
},
},
[]string{
//`foo.from`,
//`module.boo.foo.from`,
`module.bar[0].foo.to`,
`module.boo.foo.from`,
},
Blocked: map[addrs.UniqueKey]MoveBlocked{},
},
*/
[]string{
`module.boo.foo.from`,
`module.boo.foo.to`,
},
},
// FIXME: This test seems to flap between the result the test case
// currently records and the "more intuitive" results included inline,
// which suggests we have a missing edge in our move dependency graph.
// (The MoveResults commented out below predates some changes to that
// struct, so will need updating once we uncomment this test.)
/*
"module move collides with resource move": {
[]MoveStatement{
testMoveStatement(t, "", "module.bar[0]", "module.boo"),
testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
},
states.BuildState(func(s *states.SyncState) {
s.SetResourceInstanceCurrent(
"module move collides with resource move": {
[]MoveStatement{
testMoveStatement(t, "", "module.bar[0]", "module.boo"),
testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
},
states.BuildState(func(s *states.SyncState) {
s.SetResourceInstanceCurrent(
instAddrs["module.bar[0].foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
s.SetResourceInstanceCurrent(
instAddrs["foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
}),
MoveResults{
Changes: map[addrs.UniqueKey]MoveSuccess{
instAddrs["module.boo.foo.from"].UniqueKey(): {
instAddrs["module.bar[0].foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
s.SetResourceInstanceCurrent(
instAddrs["foo.from"],
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{}`),
},
providerAddr,
)
}),
MoveResults{
// FIXME: This result is counter-intuitive, because ApplyMoves
// handled the resource move first and then it was the
// module move that collided, whereas it would arguably have
// been better to let the module take priority and have only
// the one resource move be ignored due to the collision.
// (The arguably-more-intuitive results are commented out
// in the maps below.)
Changes: map[addrs.UniqueKey]MoveSuccess{
//instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["module.bar[0].foo.from"],
instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"],
},
Blocked: map[addrs.UniqueKey]MoveBlocked{
//instAddrs["foo.from"].UniqueKey(): instAddrs["module.bar[0].foo.from"],
instAddrs["module.bar[0].foo.from"].Module.UniqueKey(): instAddrs["module.boo.foo.from"].Module,
instAddrs["module.boo.foo.from"],
},
},
[]string{
//`foo.from`,
//`module.boo.foo.from`,
`module.bar[0].foo.from`,
`module.boo.foo.from`,
Blocked: map[addrs.UniqueKey]MoveBlocked{
instAddrs["foo.from"].ContainingResource().UniqueKey(): {
Actual: instAddrs["foo.from"].ContainingResource(),
Wanted: instAddrs["module.boo.foo.from"].ContainingResource(),
},
},
},
*/
[]string{
`foo.from`,
`module.boo.foo.from`,
},
},
}
for name, test := range tests {