From 036fb9c1bfbdb0742f80251c798ebda4c8a19a12 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 30 Sep 2022 09:26:38 -0400 Subject: [PATCH 1/2] check detailed provider for destroy edge cycles When we checked for cycles with destroy edges around providers, it was only for providers of a different type, but one can do the same thing with the same provider under different local aliases. Check to see if the provider also contains an alias, or is defined absolutely in some other way. The absolute accuracy here isn't critical, since in most cases these edges are not required for correct results, but finding a correct and consistent method for determining when these edges are needed is going to take more research. There was also an oversight fixed here where the basic creator->destroyer edges were added _after_ the cycle checks, limiting their utility. The ordering of the additions was swapped to make sure all cycles are noticed. --- internal/terraform/transform_destroy_edge.go | 125 +++++++++++-------- 1 file changed, 71 insertions(+), 54 deletions(-) diff --git a/internal/terraform/transform_destroy_edge.go b/internal/terraform/transform_destroy_edge.go index a1a60cb63146..14b421b7581d 100644 --- a/internal/terraform/transform_destroy_edge.go +++ b/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() - - sameProvider := fromProvider.Equals(toProvider) + toProvider := getComparableProvider(pc) // 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 - for _, ds := range destroyers { - for _, des := range ds { - ri, ok := des.(GraphNodeResourceInstance) - if !ok { - continue - } + // Go through and connect creators to destroyers. Going along with + // our example, this makes: A_d => A + for _, v := range g.Vertices() { + cn, ok := v.(GraphNodeCreator) + if !ok { + continue + } - for _, resAddr := range ri.StateDependencies() { - 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)) - t.tryInterProviderDestroyEdge(g, desDep, des) - } else { - log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(desDep), dag.VertexName(des)) - } - } + addr := cn.CreateAddr() + if addr == nil { + continue + } - // We can have some create or update nodes which were - // dependents of the destroy node. If they have no destroyer - // themselves, make the connection directly from the creator. - 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)) - t.tryInterProviderDestroyEdge(g, createDep, des) - } else { - log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(createDep), dag.VertexName(des)) - } - } - } + for _, d := range destroyers[addr.String()] { + // For illustrating our example + a_d := d.(dag.Vertex) + a := v + + log.Printf( + "[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q", + dag.VertexName(a), dag.VertexName(a_d)) + + 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 - // our example, this makes: A_d => A - for _, v := range g.Vertices() { - cn, ok := v.(GraphNodeCreator) - if !ok { - continue - } - - addr := cn.CreateAddr() - if addr == nil { - continue - } - - for _, d := range destroyers[addr.String()] { - // For illustrating our example - a_d := d.(dag.Vertex) - a := v + // Connect destroy dependencies as stored in the state + for _, ds := range destroyers { + for _, des := range ds { + ri, ok := des.(GraphNodeResourceInstance) + if !ok { + continue + } - log.Printf( - "[TRACE] DestroyEdgeTransformer: connecting creator %q with destroyer %q", - dag.VertexName(a), dag.VertexName(a_d)) + for _, resAddr := range ri.StateDependencies() { + 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)) + t.tryInterProviderDestroyEdge(g, desDep, des) + } else { + log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(desDep), dag.VertexName(des)) + } + } - g.Connect(dag.BasicEdge(a, a_d)) + // We can have some create or update nodes which were + // dependents of the destroy node. If they have no destroyer + // themselves, make the connection directly from the creator. + for _, createDep := range creators[resAddr.String()] { + if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(createDep, des) { + log.Printf("[DEBUG] DestroyEdgeTransformer2: %s has stored dependency of %s\n", dag.VertexName(createDep), dag.VertexName(des)) + t.tryInterProviderDestroyEdge(g, createDep, des) + } else { + log.Printf("[TRACE] DestroyEdgeTransformer2: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(createDep), dag.VertexName(des)) + } + } + } } } From c296172be77122445088373da057edd10fea67f5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 30 Sep 2022 13:26:34 -0400 Subject: [PATCH 2/2] test for cycle around aliased provider --- internal/terraform/context_plan2_test.go | 79 ++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 3c9ddaf2cf4d..70f614fb14e5 100644 --- a/internal/terraform/context_plan2_test.go +++ b/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) +}