From b45f53eef4c05473f8d630f8b6bd5cb9a4f8dd77 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 14 Jul 2016 09:33:37 -0600 Subject: [PATCH] dag: fix ReverseDepthFirstWalk when nodes remove themselves The report in #7378 led us into a deep rabbit hole that turned out to expose a bug in the graph walk implementation being used by the `NoopTransformer`. The problem ended up being when two nodes in a single dependency chain both reported `Noop() -> true` and needed to be removed. This was breaking the walk and preventing the second node from ever being visited. Fixes #7378 --- dag/dag.go | 12 ++++----- dag/dag_test.go | 27 +++++++++++++++++++ terraform/context_apply_test.go | 2 ++ .../middle/bottom/bottom.tf | 6 ++++- .../middle/middle.tf | 8 +++--- 5 files changed, 45 insertions(+), 10 deletions(-) diff --git a/dag/dag.go b/dag/dag.go index b609ac707..20b3e0400 100644 --- a/dag/dag.go +++ b/dag/dag.go @@ -332,12 +332,7 @@ func (g *AcyclicGraph) ReverseDepthFirstWalk(start []Vertex, f DepthWalkFunc) er } seen[current.Vertex] = struct{}{} - // Visit the current node - if err := f(current.Vertex, current.Depth); err != nil { - return err - } - - // Visit targets of this in a consistent order. + // Add next set of targets in a consistent order. targets := AsVertexList(g.UpEdges(current.Vertex)) sort.Sort(byVertexName(targets)) for _, t := range targets { @@ -346,6 +341,11 @@ func (g *AcyclicGraph) ReverseDepthFirstWalk(start []Vertex, f DepthWalkFunc) er Depth: current.Depth + 1, }) } + + // Visit the current node + if err := f(current.Vertex, current.Depth); err != nil { + return err + } } return nil diff --git a/dag/dag_test.go b/dag/dag_test.go index 93441775a..2d17a8c37 100644 --- a/dag/dag_test.go +++ b/dag/dag_test.go @@ -260,6 +260,33 @@ func TestAcyclicGraphWalk_error(t *testing.T) { t.Fatalf("bad: %#v", visits) } +func TestAcyclicGraph_ReverseDepthFirstWalk_WithRemoval(t *testing.T) { + var g AcyclicGraph + g.Add(1) + g.Add(2) + g.Add(3) + g.Connect(BasicEdge(3, 2)) + g.Connect(BasicEdge(2, 1)) + + var visits []Vertex + var lock sync.Mutex + err := g.ReverseDepthFirstWalk([]Vertex{1}, func(v Vertex, d int) error { + lock.Lock() + defer lock.Unlock() + visits = append(visits, v) + g.Remove(v) + return nil + }) + if err != nil { + t.Fatalf("err: %s", err) + } + + expected := []Vertex{1, 2, 3} + if !reflect.DeepEqual(visits, expected) { + t.Fatalf("expected: %#v, got: %#v", expected, visits) + } +} + const testGraphTransReductionStr = ` 1 2 diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 142f735c8..ab35ca29d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -4873,6 +4873,8 @@ func TestContext2Apply_destroyNestedModuleWithAttrsReferencingResource(t *testin expected := strings.TrimSpace(` module.middle: + +module.middle.bottom: `) if actual != expected { diff --git a/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/bottom/bottom.tf b/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/bottom/bottom.tf index a9ce7fcc8..b5db44ee3 100644 --- a/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/bottom/bottom.tf +++ b/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/bottom/bottom.tf @@ -1 +1,5 @@ -variable "bottom_param" {} +variable bottom_param {} + +resource "null_resource" "bottom" { + value = "${var.bottom_param}" +} diff --git a/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/middle.tf b/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/middle.tf index 0fde5830b..76652ee44 100644 --- a/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/middle.tf +++ b/terraform/test-fixtures/apply-destroy-nested-module-with-attrs/middle/middle.tf @@ -1,8 +1,10 @@ -variable "param" {} - -resource "null_resource" "n" {} +variable param {} module "bottom" { source = "./bottom" bottom_param = "${var.param}" } + +resource "null_resource" "middle" { + value = "${var.param}" +}