From d4b936251882926984ecd59865a1692d93696bcd Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 23 Apr 2015 10:52:31 -0500 Subject: [PATCH] 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 | 5 +- terraform/context.go | 66 +++++++++++++---- terraform/context_test.go | 19 +++++ terraform/graph_builder.go | 55 +++++++++++--- terraform/graph_builder_test.go | 71 ++++++++++++++++++- .../test-fixtures/validate-cycle/main.tf | 19 +++++ 6 files changed, 209 insertions(+), 26 deletions(-) create mode 100644 terraform/test-fixtures/validate-cycle/main.tf 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 + } +}