core: validate on verbose graph to detect some cycles earlier

Most CBD-related cycles include destroy nodes, and destroy nodes were
all being pruned from the graph before staring the Validate walk.

In practice this meant that we had scenarios that would error out with
graph cycles on Apply that _seemed_ fine during Plan.

This introduces a Verbose option to the GraphBuilder that tells it to
generate a "worst-case" graph. Validate sets this to true so that cycle
errors will always trigger at this step if they're going to happen.

(This Verbose option will be exposed as a CLI flag to `terraform graph`
in a second incoming PR.)

refs #1651

command/graph.go

@@ -52,7 +52,10 @@ func (c *GraphCommand) Run(args []string) int {
 		return 1
 	}
 
-	g, err := ctx.Graph()
+	g, err := ctx.Graph(&terraform.ContextGraphOpts{
+		Validate: true,
+		Verbose:  false,
+	})
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Error creating graph: %s", err))
 		return 1

terraform/context.go

@@ -116,14 +116,19 @@ func NewContext(opts *ContextOpts) *Context {
 	}
 }
 
+type ContextGraphOpts struct {
+	Validate bool
+	Verbose  bool
+}
+
 // Graph returns the graph for this config.
-func (c *Context) Graph() (*Graph, error) {
+func (c *Context) Graph(g *ContextGraphOpts) (*Graph, error) {
-	return c.GraphBuilder().Build(RootModulePath)
+	return c.graphBuilder(g).Build(RootModulePath)
 }
 
 // GraphBuilder returns the GraphBuilder that will be used to create
 // the graphs for this context.
-func (c *Context) GraphBuilder() GraphBuilder {
+func (c *Context) graphBuilder(g *ContextGraphOpts) GraphBuilder {
 	// TODO test
 	providers := make([]string, 0, len(c.providers))
 	for k, _ := range c.providers {
@@ -143,6 +148,8 @@ func (c *Context) GraphBuilder() GraphBuilder {
 		State:        c.state,
 		Targets:      c.targets,
 		Destroy:      c.destroy,
+		Validate:     g.Validate,
+		Verbose:      g.Verbose,
 	}
 }
 
@@ -226,8 +233,14 @@ func (c *Context) Input(mode InputMode) error {
 	}
 
 	if mode&InputModeProvider != 0 {
+		// Build the graph
+		graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+		if err != nil {
+			return err
+		}
+
 		// Do the walk
-		if _, err := c.walk(walkInput); err != nil {
+		if _, err := c.walk(graph, walkInput); err != nil {
 			return err
 		}
 	}
@@ -247,8 +260,14 @@ func (c *Context) Apply() (*State, error) {
 	// Copy our own state
 	c.state = c.state.DeepCopy()
 
+	// Build the graph
+	graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	if err != nil {
+		return nil, err
+	}
+
 	// Do the walk
-	_, err := c.walk(walkApply)
+	_, err = c.walk(graph, walkApply)
 
 	// Clean out any unused things
 	c.state.prune()
@@ -300,8 +319,14 @@ func (c *Context) Plan() (*Plan, error) {
 	c.diff.init()
 	c.diffLock.Unlock()
 
+	// Build the graph
+	graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	if err != nil {
+		return nil, err
+	}
+
 	// Do the walk
-	if _, err := c.walk(operation); err != nil {
+	if _, err := c.walk(graph, operation); err != nil {
 		return nil, err
 	}
 	p.Diff = c.diff
@@ -322,8 +347,14 @@ func (c *Context) Refresh() (*State, error) {
 	// Copy our own state
 	c.state = c.state.DeepCopy()
 
+	// Build the graph
+	graph, err := c.Graph(&ContextGraphOpts{Validate: true})
+	if err != nil {
+		return nil, err
+	}
+
 	// Do the walk
-	if _, err := c.walk(walkRefresh); err != nil {
+	if _, err := c.walk(graph, walkRefresh); err != nil {
 		return nil, err
 	}
 
@@ -375,8 +406,18 @@ func (c *Context) Validate() ([]string, []error) {
 		}
 	}
 
+	// Build a Verbose version of the graph so we can catch any potential cycles
+	// in the validate stage
+	graph, err := c.Graph(&ContextGraphOpts{
+		Validate: true,
+		Verbose:  true,
+	})
+	if err != nil {
+		return nil, []error{err}
+	}
+
 	// Walk
-	walker, err := c.walk(walkValidate)
+	walker, err := c.walk(graph, walkValidate)
 	if err != nil {
 		return nil, multierror.Append(errs, err).Errors
 	}
@@ -429,13 +470,8 @@ func (c *Context) releaseRun(ch chan<- struct{}) {
 	c.sh.Reset()
 }
 
-func (c *Context) walk(operation walkOperation) (*ContextGraphWalker, error) {
+func (c *Context) walk(
-	// Build the graph
+	graph *Graph, operation walkOperation) (*ContextGraphWalker, error) {
-	graph, err := c.GraphBuilder().Build(RootModulePath)
-	if err != nil {
-		return nil, err
-	}
-
 	// Walk the graph
 	log.Printf("[INFO] Starting graph walk: %s", operation.String())
 	walker := &ContextGraphWalker{Context: c, Operation: operation}

terraform/context_test.go

@@ -2365,6 +2365,25 @@ func TestContext2Validate_countVariableNoDefault(t *testing.T) {
 	}
 }
 
+func TestContext2Validate_cycle(t *testing.T) {
+	p := testProvider("aws")
+	m := testModule(t, "validate-cycle")
+	c := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+	})
+
+	w, e := c.Validate()
+	if len(w) > 0 {
+		t.Fatalf("expected no warns, got: %#v", w)
+	}
+	if len(e) != 1 {
+		t.Fatalf("expected 1 err, got: %s", e)
+	}
+}
+
 func TestContext2Validate_moduleBadOutput(t *testing.T) {
 	p := testProvider("aws")
 	m := testModule(t, "validate-bad-module-output")

terraform/graph_builder.go

@@ -16,9 +16,11 @@ type GraphBuilder interface {
 }
 
 // BasicGraphBuilder is a GraphBuilder that builds a graph out of a
-// series of transforms and validates the graph is a valid structure.
+// series of transforms and (optionally) validates the graph is a valid
+// structure.
 type BasicGraphBuilder struct {
-	Steps []GraphTransformer
+	Steps    []GraphTransformer
+	Validate bool
 }
 
 func (b *BasicGraphBuilder) Build(path []string) (*Graph, error) {
@@ -34,9 +36,11 @@ func (b *BasicGraphBuilder) Build(path []string) (*Graph, error) {
 	}
 
 	// Validate the graph structure
-	if err := g.Validate(); err != nil {
+	if b.Validate {
-		log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
+		if err := g.Validate(); err != nil {
-		return nil, err
+			log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String())
+			return nil, err
+		}
 	}
 
 	return g, nil
@@ -72,12 +76,23 @@ type BuiltinGraphBuilder struct {
 	// Destroy is set to true when we're in a `terraform destroy` or a
 	// `terraform plan -destroy`
 	Destroy bool
+
+	// Determines whether the GraphBuilder should perform graph validation before
+	// returning the Graph. Generally you want this to be done, except when you'd
+	// like to inspect a problematic graph.
+	Validate bool
+
+	// Verbose is set to true when the graph should be built "worst case",
+	// skipping any prune steps. This is used for early cycle detection during
+	// Validate and for manual inspection via `terraform graph -verbose`.
+	Verbose bool
 }
 
 // Build builds the graph according to the steps returned by Steps.
 func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) {
 	basic := &BasicGraphBuilder{
-		Steps: b.Steps(),
+		Steps:    b.Steps(),
+		Validate: b.Validate,
 	}
 
 	return basic.Build(path)
@@ -86,7 +101,7 @@ func (b *BuiltinGraphBuilder) Build(path []string) (*Graph, error) {
 // Steps returns the ordered list of GraphTransformers that must be executed
 // to build a complete graph.
 func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
-	return []GraphTransformer{
+	steps := []GraphTransformer{
 		// Create all our resources from the configuration and state
 		&ConfigTransformer{Module: b.Root},
 		&OrphanTransformer{
@@ -123,7 +138,10 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
 		// Create the destruction nodes
 		&DestroyTransformer{},
 		&CreateBeforeDestroyTransformer{},
-		&PruneDestroyTransformer{Diff: b.Diff, State: b.State},
+		b.conditional(&conditionalOpts{
+			If:   func() bool { return !b.Verbose },
+			Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State},
+		}),
 
 		// Make sure we create one root
 		&RootTransformer{},
@@ -132,4 +150,25 @@ func (b *BuiltinGraphBuilder) Steps() []GraphTransformer {
 		// more sane if possible (it usually is possible).
 		&TransitiveReductionTransformer{},
 	}
+
+	// Remove nils
+	for i, s := range steps {
+		if s == nil {
+			steps = append(steps[:i], steps[i+1:]...)
+		}
+	}
+
+	return steps
+}
+
+type conditionalOpts struct {
+	If   func() bool
+	Then GraphTransformer
+}
+
+func (b *BuiltinGraphBuilder) conditional(o *conditionalOpts) GraphTransformer {
+	if o.If != nil && o.Then != nil && o.If() {
+		return o.Then
+	}
+	return nil
 }

terraform/graph_builder_test.go

@@ -41,6 +41,7 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 			&testBasicGraphBuilderTransform{1},
 			&testBasicGraphBuilderTransform{2},
 		},
+		Validate: true,
 	}
 
 	_, err := b.Build(RootModulePath)
@@ -49,6 +50,21 @@ func TestBasicGraphBuilder_validate(t *testing.T) {
 	}
 }
 
+func TestBasicGraphBuilder_validateOff(t *testing.T) {
+	b := &BasicGraphBuilder{
+		Steps: []GraphTransformer{
+			&testBasicGraphBuilderTransform{1},
+			&testBasicGraphBuilderTransform{2},
+		},
+		Validate: false,
+	}
+
+	_, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("expected no error, got: %s", err)
+	}
+}
+
 func TestBuiltinGraphBuilder_impl(t *testing.T) {
 	var _ GraphBuilder = new(BuiltinGraphBuilder)
 }
@@ -58,7 +74,8 @@ func TestBuiltinGraphBuilder_impl(t *testing.T) {
 // specific ordering of steps should be added in other tests.
 func TestBuiltinGraphBuilder(t *testing.T) {
 	b := &BuiltinGraphBuilder{
-		Root: testModule(t, "graph-builder-basic"),
+		Root:     testModule(t, "graph-builder-basic"),
+		Validate: true,
 	}
 
 	g, err := b.Build(RootModulePath)
@@ -73,11 +90,31 @@ func TestBuiltinGraphBuilder(t *testing.T) {
 	}
 }
 
+func TestBuiltinGraphBuilder_Verbose(t *testing.T) {
+	b := &BuiltinGraphBuilder{
+		Root:     testModule(t, "graph-builder-basic"),
+		Validate: true,
+		Verbose:  true,
+	}
+
+	g, err := b.Build(RootModulePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testBuiltinGraphBuilderVerboseStr)
+	if actual != expected {
+		t.Fatalf("bad: %s", actual)
+	}
+}
+
 // This tests a cycle we got when a CBD resource depends on a non-CBD
 // resource. This cycle shouldn't happen in the general case anymore.
 func TestBuiltinGraphBuilder_cbdDepNonCbd(t *testing.T) {
 	b := &BuiltinGraphBuilder{
-		Root: testModule(t, "graph-builder-cbd-non-cbd"),
+		Root:     testModule(t, "graph-builder-cbd-non-cbd"),
+		Validate: true,
 	}
 
 	_, err := b.Build(RootModulePath)
@@ -86,6 +123,19 @@ func TestBuiltinGraphBuilder_cbdDepNonCbd(t *testing.T) {
 	}
 }
 
+func TestBuiltinGraphBuilder_cbdDepNonCbd_errorsWhenVerbose(t *testing.T) {
+	b := &BuiltinGraphBuilder{
+		Root:     testModule(t, "graph-builder-cbd-non-cbd"),
+		Validate: true,
+		Verbose:  true,
+	}
+
+	_, err := b.Build(RootModulePath)
+	if err == nil {
+		t.Fatalf("expected err, got none")
+	}
+}
+
 /*
 TODO: This exposes a really bad bug we need to fix after we merge
 the f-ast-branch. This bug still exists in master.
@@ -130,6 +180,23 @@ aws_instance.web
 provider.aws
 `
 
+const testBuiltinGraphBuilderVerboseStr = `
+aws_instance.db
+  aws_instance.db (destroy tainted)
+  aws_instance.db (destroy)
+aws_instance.db (destroy tainted)
+  aws_instance.web (destroy tainted)
+aws_instance.db (destroy)
+  aws_instance.web (destroy)
+aws_instance.web
+  aws_instance.db
+aws_instance.web (destroy tainted)
+  provider.aws
+aws_instance.web (destroy)
+  provider.aws
+provider.aws
+`
+
 const testBuiltinGraphBuilderModuleStr = `
 aws_instance.web
   aws_instance.web (destroy)

terraform/test-fixtures/validate-cycle/main.tf

@@ -0,0 +1,19 @@
+provider "aws" { }
+
+/*
+ * When a CBD resource depends on a non-CBD resource,
+ * a cycle is formed that only shows up when Destroy
+ * nodes are included in the graph.
+ */
+resource "aws_security_group" "firewall" {
+}
+
+resource "aws_instance" "web" {
+    security_groups = [
+        "foo",
+        "${aws_security_group.firewall.foo}"
+    ]
+    lifecycle {
+      create_before_destroy = true
+    }
+}