From 71f4526ae5c0bc6d0813491f703d72c17443ea2a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 15 Nov 2019 15:07:23 -0500 Subject: [PATCH 1/3] failing test for cbd cycle --- terraform/context_apply_test.go | 118 +++++++++++++++++++++ terraform/testdata/apply-cbd-cycle/main.tf | 19 ++++ 2 files changed, 137 insertions(+) create mode 100644 terraform/testdata/apply-cbd-cycle/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 4bc016e9f..c61cd8881 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11099,3 +11099,121 @@ func TestContext2Apply_taintedDestroyFailure(t *testing.T) { t.Fatalf("unexpected attrs for c: %q\n", c.Current.AttrsJSON) } } + +func TestContext2Apply_cbdCycle(t *testing.T) { + m, snap := testModuleWithSnapshot(t, "apply-cbd-cycle") + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "a", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a","require_new":"old","foo":"b"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "b", + }, + Module: addrs.RootModuleInstance, + }, + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "c", + }, + Module: addrs.RootModuleInstance, + }, + }, + }, + addrs.ProviderConfig{ + Type: "test", + }.Absolute(addrs.RootModuleInstance), + ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "b", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b","require_new":"old","foo":"c"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "c", + }, + Module: addrs.RootModuleInstance, + }, + }, + }, + addrs.ProviderConfig{ + Type: "test", + }.Absolute(addrs.RootModuleInstance), + ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "c", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"c","require_new":"old"}`), + }, + addrs.ProviderConfig{ + Type: "test", + }.Absolute(addrs.RootModuleInstance), + ) + + providerResolver := providers.ResolverFixed( + map[string]providers.Factory{ + "test": testProviderFuncFixed(p), + }, + ) + + hook := &testHook{} + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providerResolver, + State: state, + Hooks: []Hook{hook}, + }) + + plan, diags := ctx.Plan() + diags.HasErrors() + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } + + // We'll marshal and unmarshal the plan here, to ensure that we have + // a clean new context as would be created if we separately ran + // terraform plan -out=tfplan && terraform apply tfplan + ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + if err != nil { + t.Fatal(err) + } + ctxOpts.ProviderResolver = providerResolver + ctx, diags = NewContext(ctxOpts) + if diags.HasErrors() { + t.Fatalf("failed to create context for plan: %s", diags.Err()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } +} diff --git a/terraform/testdata/apply-cbd-cycle/main.tf b/terraform/testdata/apply-cbd-cycle/main.tf new file mode 100644 index 000000000..5ac53107e --- /dev/null +++ b/terraform/testdata/apply-cbd-cycle/main.tf @@ -0,0 +1,19 @@ +resource "test_instance" "a" { + foo = test_instance.b.id + require_new = "changed" + + lifecycle { + create_before_destroy = true + } +} + +resource "test_instance" "b" { + foo = test_instance.c.id + require_new = "changed" +} + + +resource "test_instance" "c" { + require_new = "changed" +} + From 682083cdd185abdd7efb72c3d018ef59b570eff4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sun, 17 Nov 2019 09:56:44 -0500 Subject: [PATCH 2/3] Creators cannot depend directly on CBD dest nodes Since a create node cannot both depend on its destroy node AND be CreateBeforeDestroy, the same goes for its dependencies. While we do connect resources with dependency destroy nodes so that updates are ordered correctly, this ordering does not make sense in the CreateBeforeDestroy case. If resource node is CreateBeforeDestroy, we need to remove any direct dependencies from it to destroy nodes to prevent cycles. Since we don't know for certain if a crate node is going to be CreateBeforeDestroy at the time the edge is added in the graph, we add it unconditionally and prune it out later on. The pruning happens during the CBD transformer when the CBD destroy node reverses it's own destroy edge. The reason this works for detecting the original edge, is that dependencies of CBD resources are forced to be CBD themselves. This does have a false positive where the case of the original node is NOT CBD, but this can be taken care of later when we gather enough information in the graph to prevent the connection in the first place. --- terraform/transform_destroy_cbd.go | 40 +++++++++++++++++------------ terraform/transform_destroy_edge.go | 6 +++++ 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 21b75c141..5945eeadd 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -160,23 +160,27 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { continue } - // Find the destroy edge. There should only be one. + // Find the resource edges for _, e := range g.EdgesTo(v) { - // Not a destroy edge, ignore it - de, ok := e.(*DestroyEdge) - if !ok { - continue + switch de := e.(type) { + case *DestroyEdge: + // we need to invert the destroy edge from the create node + log.Printf("[TRACE] CBDEdgeTransformer: inverting edge: %s => %s", + dag.VertexName(de.Source()), dag.VertexName(de.Target())) + + // Found it! Invert. + g.RemoveEdge(de) + applyNode := de.Source() + destroyNode := de.Target() + g.Connect(&DestroyEdge{S: destroyNode, T: applyNode}) + default: + // We cannot have any direct dependencies from creators when + // the node is CBD without inducing a cycle. + if _, ok := e.Source().(GraphNodeCreator); ok { + log.Printf("[TRACE] CBDEdgeTransformer: removing non DestroyEdge to CBD destroy node: %s => %s", dag.VertexName(e.Source()), dag.VertexName(e.Target())) + g.RemoveEdge(e) + } } - - log.Printf("[TRACE] CBDEdgeTransformer: inverting edge: %s => %s", - dag.VertexName(de.Source()), dag.VertexName(de.Target())) - - // Found it! Invert. - g.RemoveEdge(de) - applyNode := de.Source() - destroyNode := de.Target() - g.Connect(&DestroyEdge{S: destroyNode, T: applyNode}) - break } // If the address has an index, we strip that. Our depMap creation @@ -289,7 +293,11 @@ func (t *CBDEdgeTransformer) depMap(g *Graph, destroyMap map[string][]dag.Vertex // Keep track of the destroy nodes that this address // needs to depend on. key := rn.ResourceAddr().String() - depMap[key] = append(depMap[key], dns...) + + // we only need to record the same dns once + if _, ok := depMap[key]; !ok { + depMap[key] = append(depMap[key], dns...) + } } } diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index c25bdce7e..df3872a1d 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -122,6 +122,12 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { for _, resAddr := range ri.StateDependencies() { for _, desDep := range destroyersByResource[resAddr.String()] { + // TODO: don't connect this if c is CreateBeforeDestroy. + // This will require getting the actual change action at + // this point, since the lifecycle may have been forced + // by a dependent. This should prevent needing to prune + // the edge back out in CBDEdgeTransformer, and allow + // non-CBD nodes to depend on CBD destroys directly. log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep)) g.Connect(dag.BasicEdge(c, desDep)) From 33bef7c42e1931bdfa579ba54bd24b9fd3b17346 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 15 Nov 2019 16:00:13 -0500 Subject: [PATCH 3/3] don't append refreshed state dependencies EvalRefreshDependencies is used to update resource dependencies when they don't exist, allow broken or old states to be updated. While appending any newly found dependencies is tempting to have the largest set available, changes to the config could conflict with the prior dependencies causing cycles. --- terraform/context_refresh_test.go | 26 ++++++++++++++------------ terraform/eval_state.go | 9 +++++++-- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index d8479ae6c..0c4fa0302 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1978,6 +1978,17 @@ func TestRefresh_updateDependencies(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"foo"}`), + Dependencies: []addrs.AbsResource{ + // Existing dependencies should not be removed during refresh + { + Module: addrs.RootModuleInstance, + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "baz", + }, + }, + }, }, addrs.ProviderConfig{ Type: "aws", @@ -1992,17 +2003,6 @@ func TestRefresh_updateDependencies(t *testing.T) { &states.ResourceInstanceObjectSrc{ Status: states.ObjectReady, AttrsJSON: []byte(`{"id":"bar","foo":"foo"}`), - Dependencies: []addrs.AbsResource{ - // Existing dependencies should not be removed during refresh - { - Module: addrs.RootModuleInstance, - Resource: addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "aws_instance", - Name: "baz", - }, - }, - }, }, addrs.ProviderConfig{ Type: "aws", @@ -2045,11 +2045,13 @@ aws_instance.bar: foo = foo Dependencies: - aws_instance.baz aws_instance.foo aws_instance.foo: ID = foo provider = provider.aws + + Dependencies: + aws_instance.baz `) checkStateString(t, result, expect) diff --git a/terraform/eval_state.go b/terraform/eval_state.go index b0fcee007..0be877544 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -508,8 +508,13 @@ func (n *EvalRefreshDependencies) Eval(ctx EvalContext) (interface{}, error) { depMap[d.String()] = d } - for _, d := range state.Dependencies { - depMap[d.String()] = d + // We have already dependencies in state, so we need to trust those for + // refresh. We can't write out new dependencies until apply time in case + // the configuration has been changed in a manner the conflicts with the + // stored dependencies. + if len(state.Dependencies) > 0 { + *n.Dependencies = state.Dependencies + return nil, nil } deps := make([]addrs.AbsResource, 0, len(depMap))