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

Extract more exact provider name when checking for destroy cycles

internal/terraform/context_plan2_test.go

@@ -3748,3 +3748,82 @@ resource "test_object" "b" {
 	})
 	assertNoErrors(t, diags)
 }
+
+// make sure there are no cycles with changes around a provider configured via
+// managed resources.
+func TestContext2Plan_destroyWithResourceConfiguredProvider(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_object" "a" {
+  in = "a"
+}
+
+provider "test" {
+  alias = "other"
+  in = test_object.a.out
+}
+
+resource "test_object" "b" {
+  provider = test.other
+  in = "a"
+}
+`})
+
+	testProvider := &MockProvider{
+		GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{
+			Provider: providers.Schema{
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"in": {
+							Type:     cty.String,
+							Optional: true,
+						},
+					},
+				},
+			},
+			ResourceTypes: map[string]providers.Schema{
+				"test_object": providers.Schema{
+					Block: &configschema.Block{
+						Attributes: map[string]*configschema.Attribute{
+							"in": {
+								Type:     cty.String,
+								Optional: true,
+							},
+							"out": {
+								Type:     cty.Number,
+								Computed: true,
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(testProvider),
+		},
+	})
+
+	// plan+apply to create the initial state
+	opts := SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))
+	plan, diags := ctx.Plan(m, states.NewState(), opts)
+	assertNoErrors(t, diags)
+	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.
+	//
+	// Try to replace both instances
+	addrA := mustResourceInstanceAddr("test_object.a")
+	addrB := mustResourceInstanceAddr(`test_object.b`)
+	opts.ForceReplace = []addrs.AbsResourceInstance{addrA, addrB}
+
+	_, diags = ctx.Plan(m, state, opts)
+	assertNoErrors(t, diags)
+}

internal/terraform/transform_destroy_edge.go

@@ -72,24 +72,41 @@ func (t *DestroyEdgeTransformer) tryInterProviderDestroyEdge(g *Graph, from, to
 	e := dag.BasicEdge(from, to)
 	g.Connect(e)
 
+	// getComparableProvider inspects the node to try and get the most precise
+	// description of the provider being used to help determine if 2 nodes are
+	// from the same provider instance.
+	getComparableProvider := func(pc GraphNodeProviderConsumer) string {
+		ps := pc.Provider().String()
+
+		// we don't care about `exact` here, since we're only looking for any
+		// clue that the providers may differ.
+		p, _ := pc.ProvidedBy()
+		switch p := p.(type) {
+		case addrs.AbsProviderConfig:
+			ps = p.String()
+		case addrs.LocalProviderConfig:
+			ps = p.String()
+		}
+
+		return ps
+	}
+
 	pc, ok := from.(GraphNodeProviderConsumer)
 	if !ok {
 		return
 	}
-	fromProvider := pc.Provider()
+	fromProvider := getComparableProvider(pc)
 
 	pc, ok = to.(GraphNodeProviderConsumer)
 	if !ok {
 		return
 	}
-	toProvider := pc.Provider()
+	toProvider := getComparableProvider(pc)
-
-	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 {
+	if fromProvider != toProvider && 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)
@@ -138,36 +155,29 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		return nil
 	}
 
-	// Connect destroy dependencies as stored in the state
+	// Go through and connect creators to destroyers. Going along with
-	for _, ds := range destroyers {
+	// our example, this makes: A_d => A
-		for _, des := range ds {
+	for _, v := range g.Vertices() {
-			ri, ok := des.(GraphNodeResourceInstance)
+		cn, ok := v.(GraphNodeCreator)
-			if !ok {
+		if !ok {
-				continue
+			continue
-			}
+		}
 
-			for _, resAddr := range ri.StateDependencies() {
+		addr := cn.CreateAddr()
-				for _, desDep := range destroyersByResource[resAddr.String()] {
+		if addr == nil {
-					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(desDep, des) {
+			continue
-						log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(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))
-					}
-				}
 
-				// We can have some create or update nodes which were
+		for _, d := range destroyers[addr.String()] {
-				// dependents of the destroy node. If they have no destroyer
+			// For illustrating our example
-				// themselves, make the connection directly from the creator.
+			a_d := d.(dag.Vertex)
-				for _, createDep := range creators[resAddr.String()] {
+			a := v
-					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(createDep, des) {
+
-						log.Printf("[DEBUG] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(createDep), dag.VertexName(des))
+			log.Printf(
-						t.tryInterProviderDestroyEdge(g, createDep, des)
+				"[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q",
-					} else {
+				dag.VertexName(a), dag.VertexName(a_d))
-						log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(createDep), dag.VertexName(des))
+
-					}
+			g.Connect(dag.BasicEdge(a, a_d))
-				}
-			}
 		}
 	}
 
@@ -192,29 +202,36 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 		}
 	}
 
-	// Go through and connect creators to destroyers. Going along with
+	// Connect destroy dependencies as stored in the state
-	// our example, this makes: A_d => A
+	for _, ds := range destroyers {
-	for _, v := range g.Vertices() {
+		for _, des := range ds {
-		cn, ok := v.(GraphNodeCreator)
+			ri, ok := des.(GraphNodeResourceInstance)
-		if !ok {
+			if !ok {
-			continue
+				continue
-		}
+			}
 
-		addr := cn.CreateAddr()
+			for _, resAddr := range ri.StateDependencies() {
-		if addr == nil {
+				for _, desDep := range destroyersByResource[resAddr.String()] {
-			continue
+					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(desDep, des) {
-		}
+						log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(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))
+					}
+				}
 
-		for _, d := range destroyers[addr.String()] {
+				// We can have some create or update nodes which were
-			// For illustrating our example
+				// dependents of the destroy node. If they have no destroyer
-			a_d := d.(dag.Vertex)
+				// themselves, make the connection directly from the creator.
-			a := v
+				for _, createDep := range creators[resAddr.String()] {
-
+					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(createDep, des) {
-			log.Printf(
+						log.Printf("[DEBUG] DestroyEdgeTransformer2: %s has stored dependency of %s\n", dag.VertexName(createDep), dag.VertexName(des))
-				"[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q",
+						t.tryInterProviderDestroyEdge(g, createDep, des)
-				dag.VertexName(a), dag.VertexName(a_d))
+					} else {
-
+						log.Printf("[TRACE] DestroyEdgeTransformer2: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(createDep), dag.VertexName(des))
-			g.Connect(dag.BasicEdge(a, a_d))
+					}
+				}
+			}
 		}
 	}