From a5df3973a471b675c757b46b9540c2f0f9f1635a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 4 Nov 2016 18:58:03 -0700 Subject: [PATCH] terraform: module variables should be pruned if nothing depends on them --- terraform/graph_builder_apply.go | 6 +-- terraform/transform_module_variable.go | 38 +++++++++---- terraform/transform_module_variable_test.go | 4 +- terraform/transform_reference.go | 60 +++++++++++++++++++-- terraform/transform_reference_test.go | 51 ++++++++++++++++-- 5 files changed, 137 insertions(+), 22 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 1d88b706d..601792721 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -105,12 +105,12 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Add root variables &RootVariableTransformer{Module: b.Module}, - // Add module variables - &ModuleVariableTransformer{Module: b.Module}, - // Add the outputs &OutputTransformer{Module: b.Module}, + // Add module variables + &ModuleVariableTransformer{Module: b.Module}, + // Connect references so ordering is correct &ReferenceTransformer{}, diff --git a/terraform/transform_module_variable.go b/terraform/transform_module_variable.go index e685b918e..50f00bff1 100644 --- a/terraform/transform_module_variable.go +++ b/terraform/transform_module_variable.go @@ -5,16 +5,18 @@ import ( "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" + "github.com/hashicorp/terraform/dag" ) // ModuleVariableTransformer is a GraphTransformer that adds all the variables // in the configuration to the graph. // -// This only adds variables that either have no dependencies (and therefore -// always succeed) or has dependencies that are 100% represented in the -// graph. +// This only adds variables that are referenced by other thigns in the graph. +// If a module variable is not referenced, it won't be added to the graph. type ModuleVariableTransformer struct { Module *module.Tree + + DisablePrune bool // True if pruning unreferenced should be disabled } func (t *ModuleVariableTransformer) Transform(g *Graph) error { @@ -27,18 +29,18 @@ func (t *ModuleVariableTransformer) transform(g *Graph, parent, m *module.Tree) return nil } - // If we have a parent, we can determine if a module variable is being - // used, so we transform this. - if parent != nil { - if err := t.transformSingle(g, parent, m); err != nil { + // Transform all the children. This must be done BEFORE the transform + // above since child module variables can reference parent module variables. + for _, c := range m.Children() { + if err := t.transform(g, m, c); err != nil { return err } } - // Transform all the children. This must be done AFTER the transform - // above since child module variables can reference parent module variables. - for _, c := range m.Children() { - if err := t.transform(g, m, c); err != nil { + // If we have a parent, we can determine if a module variable is being + // used, so we transform this. + if parent != nil { + if err := t.transformSingle(g, parent, m); err != nil { return err } } @@ -67,6 +69,9 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module. return nil } + // Build the reference map so we can determine if we're referencing things. + refMap := NewReferenceMap(g.Vertices()) + // Add all variables here for _, v := range vars { // Determine the value of the variable. If it isn't in the @@ -96,6 +101,17 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, m *module. Module: t.Module, } + if !t.DisablePrune { + // If the node is not referenced by anything, then we don't need + // to include it since it won't be used. + if matches := refMap.ReferencedBy(node); len(matches) == 0 { + log.Printf( + "[INFO] Not including %q in graph, nothing depends on it", + dag.VertexName(node)) + continue + } + } + // Add it! g.Add(node) } diff --git a/terraform/transform_module_variable_test.go b/terraform/transform_module_variable_test.go index 6842b325c..bd1f9525b 100644 --- a/terraform/transform_module_variable_test.go +++ b/terraform/transform_module_variable_test.go @@ -17,7 +17,7 @@ func TestModuleVariableTransformer(t *testing.T) { } { - tf := &ModuleVariableTransformer{Module: module} + tf := &ModuleVariableTransformer{Module: module, DisablePrune: true} if err := tf.Transform(&g); err != nil { t.Fatalf("err: %s", err) } @@ -42,7 +42,7 @@ func TestModuleVariableTransformer_nested(t *testing.T) { } { - tf := &ModuleVariableTransformer{Module: module} + tf := &ModuleVariableTransformer{Module: module, DisablePrune: true} if err := tf.Transform(&g); err != nil { t.Fatalf("err: %s", err) } diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index f848274aa..613f1484c 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -66,7 +66,8 @@ func (t *ReferenceTransformer) Transform(g *Graph) error { type ReferenceMap struct { // m is the mapping of referenceable name to list of verticies that // implement that name. This is built on initialization. - m map[string][]dag.Vertex + references map[string][]dag.Vertex + referencedBy map[string][]dag.Vertex } // References returns the list of vertices that this vertex @@ -82,7 +83,7 @@ func (m *ReferenceMap) References(v dag.Vertex) ([]dag.Vertex, []string) { prefix := m.prefix(v) for _, n := range rn.References() { n = prefix + n - parents, ok := m.m[n] + parents, ok := m.references[n] if !ok { missing = append(missing, n) continue @@ -106,6 +107,41 @@ func (m *ReferenceMap) References(v dag.Vertex) ([]dag.Vertex, []string) { return matches, missing } +// ReferencedBy returns the list of vertices that reference the +// vertex passed in. +func (m *ReferenceMap) ReferencedBy(v dag.Vertex) []dag.Vertex { + rn, ok := v.(GraphNodeReferenceable) + if !ok { + return nil + } + + var matches []dag.Vertex + prefix := m.prefix(v) + for _, n := range rn.ReferenceableName() { + n = prefix + n + children, ok := m.referencedBy[n] + if !ok { + continue + } + + // Make sure this isn't a self reference, which isn't included + selfRef := false + for _, p := range children { + if p == v { + selfRef = true + break + } + } + if selfRef { + continue + } + + matches = append(matches, children...) + } + + return matches +} + func (m *ReferenceMap) prefix(v dag.Vertex) string { // If the node is stating it is already fully qualified then // we don't have to create the prefix! @@ -146,7 +182,25 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { } } - m.m = refMap + // Build the lookup table for referenced by + refByMap := make(map[string][]dag.Vertex) + for _, v := range vs { + // We're only looking for referenceable nodes + rn, ok := v.(GraphNodeReferencer) + if !ok { + continue + } + + // Go through and cache them + prefix := m.prefix(v) + for _, n := range rn.References() { + n = prefix + n + refByMap[n] = append(refByMap[n], v) + } + } + + m.references = refMap + m.referencedBy = refByMap return &m } diff --git a/terraform/transform_reference_test.go b/terraform/transform_reference_test.go index ae3b7ce79..31cf4664d 100644 --- a/terraform/transform_reference_test.go +++ b/terraform/transform_reference_test.go @@ -112,11 +112,56 @@ func TestReferenceMapReferences(t *testing.T) { for tn, tc := range cases { t.Run(tn, func(t *testing.T) { rm := NewReferenceMap(tc.Nodes) - result, err := rm.References(tc.Check) - if err != nil { - t.Fatalf("err: %s", err) + result, _ := rm.References(tc.Check) + + var resultStr []string + for _, v := range result { + resultStr = append(resultStr, dag.VertexName(v)) } + sort.Strings(resultStr) + sort.Strings(tc.Result) + if !reflect.DeepEqual(resultStr, tc.Result) { + t.Fatalf("bad: %#v", resultStr) + } + }) + } +} + +func TestReferenceMapReferencedBy(t *testing.T) { + cases := map[string]struct { + Nodes []dag.Vertex + Check dag.Vertex + Result []string + }{ + "simple": { + Nodes: []dag.Vertex{ + &graphNodeRefChildTest{ + NameValue: "A", + Refs: []string{"A"}, + }, + &graphNodeRefChildTest{ + NameValue: "B", + Refs: []string{"A"}, + }, + &graphNodeRefChildTest{ + NameValue: "C", + Refs: []string{"B"}, + }, + }, + Check: &graphNodeRefParentTest{ + NameValue: "foo", + Names: []string{"A"}, + }, + Result: []string{"A", "B"}, + }, + } + + for tn, tc := range cases { + t.Run(tn, func(t *testing.T) { + rm := NewReferenceMap(tc.Nodes) + result := rm.ReferencedBy(tc.Check) + var resultStr []string for _, v := range result { resultStr = append(resultStr, dag.VertexName(v))