diff --git a/command/graph.go b/command/graph.go index 834d6c865..8bd5413fa 100644 --- a/command/graph.go +++ b/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 diff --git a/terraform/context.go b/terraform/context.go index b13f92f63..669d50b26 100644 --- a/terraform/context.go +++ b/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) { - return c.GraphBuilder().Build(RootModulePath) +func (c *Context) Graph(g *ContextGraphOpts) (*Graph, error) { + 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) { - // Build the graph - graph, err := c.GraphBuilder().Build(RootModulePath) - if err != nil { - return nil, err - } - +func (c *Context) walk( + graph *Graph, operation walkOperation) (*ContextGraphWalker, error) { // Walk the graph log.Printf("[INFO] Starting graph walk: %s", operation.String()) walker := &ContextGraphWalker{Context: c, Operation: operation} diff --git a/terraform/context_test.go b/terraform/context_test.go index d4597f11f..a68c17e26 100644 --- a/terraform/context_test.go +++ b/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") diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index f08e20f16..0b4b32e20 100644 --- a/terraform/graph_builder.go +++ b/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 { - log.Printf("[ERROR] Graph validation failed. Graph:\n\n%s", g.String()) - return nil, err + if b.Validate { + if err := g.Validate(); err != nil { + 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 } diff --git a/terraform/graph_builder_test.go b/terraform/graph_builder_test.go index 23d1eb8ba..0f66947fc 100644 --- a/terraform/graph_builder_test.go +++ b/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) diff --git a/terraform/test-fixtures/validate-cycle/main.tf b/terraform/test-fixtures/validate-cycle/main.tf new file mode 100644 index 000000000..3dc503aa7 --- /dev/null +++ b/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 + } +}