Merge pull request #31857 from hashicorp/jbardin/destroy-edge-cycles

prevent cycles when connecting destroy nodes

internal/terraform/context_apply2_test.go

@@ -1201,6 +1201,25 @@ output "out" {
 	state, diags := ctx.Apply(plan, m)
 	assertNoErrors(t, diags)
 
+	// Resource changes which have dependencies across providers which
+	// themselves depend on resources can result in cycles.
+	// Because other_object transitively depends on the module resources
+	// through its provider, we trigger changes on both sides of this boundary
+	// to ensure we can create a valid plan.
+	//
+	// 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

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))
 					}