From abbfa0c8a3491c4054dd2852a6b5554179d64019 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Sep 2022 11:59:21 -0400 Subject: [PATCH] prevent cycles when connecting destroy nodes When adding destroy edges between resources from different providers, and a provider itself depends on the other provider's resources, we can get cycles in the final dependency graph. The problem is a little deeper than simply not connecting these nodes, since the edges are still needed when doing a full destroy operation. For now we can get by assuming the edges are required, and reverting them only if they result in a cycle. This works because destroy edges are the last edges added to managed resources during graph building. This was rarely a problem before v1.3, because noop nodes were not added to the apply graph, and unused values were aggressively pruned. In v1.3 however all nodes are kept in the graph so that postcondition blocks are always evaluated during apply, increasing the chances of the cycles appearing. --- internal/terraform/context_apply2_test.go | 14 +++++ internal/terraform/transform_destroy_edge.go | 61 +++++++++++++++++++- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 9637a6951e34..b56c678d0453 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -1201,6 +1201,20 @@ output "out" { state, diags := ctx.Apply(plan, m) assertNoErrors(t, diags) + // Add some changes to make the planning more complex. + // Taint the object to make sure a replacement works in the plan. + otherObjAddr := mustResourceInstanceAddr("other_object.other") + otherObj := state.ResourceInstance(otherObjAddr) + otherObj.Current.Status = states.ObjectTainted + // Force a change which needs to be reverted. + testObjAddr := mustResourceInstanceAddr(`module.mod["a"].test_object.a`) + testObjA := state.ResourceInstance(testObjAddr) + testObjA.Current.AttrsJSON = []byte(`{"test_bool":null,"test_list":null,"test_map":null,"test_number":null,"test_string":"changed"}`) + + _, diags = ctx.Plan(m, state, opts) + assertNoErrors(t, diags) + return + otherProvider.ConfigureProviderCalled = false otherProvider.ConfigureProviderFn = func(req providers.ConfigureProviderRequest) (resp providers.ConfigureProviderResponse) { // check that our config is complete, even during a destroy plan diff --git a/internal/terraform/transform_destroy_edge.go b/internal/terraform/transform_destroy_edge.go index 12028ba0d173..bed62009a039 100644 --- a/internal/terraform/transform_destroy_edge.go +++ b/internal/terraform/transform_destroy_edge.go @@ -39,6 +39,63 @@ type GraphNodeCreator interface { // still subnets. type DestroyEdgeTransformer struct{} +// tryInterProviderDestroyEdge checks if we're inserting a destroy edge +// across a provider boundary, and only adds the edge if it results in no cycles. +// +// FIXME: The cycles can arise in valid configurations when a provider depends +// on resources from another provider. In the future we may want to inspect +// the dependencies of the providers themselves, to avoid needing to use the +// blunt hammer of checking for cycles. +// +// A reduced example of this dependency problem looks something like: +/* + +createA <- createB + | \ / | + | providerB <- | + v \ v +destroyA -------------> destroyB + +*/ +// +// The edge from destroyA to destroyB would be skipped in this case, but there +// are still other combinations of changes which could connect the A and B +// groups around providerB in various ways. +// +// The most difficult problem here happens during a full destroy operation. +// That creates a special case where resources on which a provider depends must +// exist for evaluation before they are destroyed. This means that any provider +// dependencies must wait until all that provider's resources have first been +// destroyed. This is where these cross-provider edges are still required to +// ensure the correct order. +func (t *DestroyEdgeTransformer) tryInterProviderDestroyEdge(g *Graph, from, to dag.Vertex) { + e := dag.BasicEdge(from, to) + g.Connect(e) + + pc, ok := from.(GraphNodeProviderConsumer) + if !ok { + return + } + fromProvider := pc.Provider() + + pc, ok = to.(GraphNodeProviderConsumer) + if !ok { + return + } + toProvider := pc.Provider() + + sameProvider := fromProvider.Equals(toProvider) + + // Check for cycles, and back out the edge if there are any. + // The cycles we are looking for only appears between providers, so don't + // waste time checking for cycles if both nodes use the same provider. + if !sameProvider && len(g.Cycles()) > 0 { + log.Printf("[DEBUG] DestroyEdgeTransformer: skipping inter-provider edge %s->%s which creates a cycle", + dag.VertexName(from), dag.VertexName(to)) + g.RemoveEdge(e) + } +} + func (t *DestroyEdgeTransformer) Transform(g *Graph) error { // Build a map of what is being destroyed (by address string) to // the list of destroyers. @@ -93,7 +150,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { for _, desDep := range destroyersByResource[resAddr.String()] { if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(desDep, des) { log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(des)) - g.Connect(dag.BasicEdge(desDep, des)) + t.tryInterProviderDestroyEdge(g, desDep, des) } else { log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(desDep), dag.VertexName(des)) } @@ -105,7 +162,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { for _, createDep := range creators[resAddr.String()] { if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(createDep, des) { log.Printf("[DEBUG] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(createDep), dag.VertexName(des)) - g.Connect(dag.BasicEdge(createDep, des)) + t.tryInterProviderDestroyEdge(g, createDep, des) } else { log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(createDep), dag.VertexName(des)) }