Merge pull request #598 from hashicorp/f-mod-deps

Fix dependency handling of modules

terraform/context.go

@@ -929,6 +929,13 @@ func (c *walkContext) planDestroyWalkFn() depgraph.WalkFunc {
 	walkFn = func(n *depgraph.Noun) error {
 		switch m := n.Meta.(type) {
 		case *GraphNodeModule:
+			// Set the destroy bool on the module
+			md := result.Diff.ModuleByPath(m.Path)
+			if md == nil {
+				md = result.Diff.AddModule(m.Path)
+			}
+			md.Destroy = true
+
 			// Build another walkContext for this module and walk it.
 			wc := c.Context.walkContext(c.Operation, m.Path)
 

terraform/diff.go

@@ -114,6 +114,7 @@ func (d *Diff) init() {
 type ModuleDiff struct {
 	Path      []string
 	Resources map[string]*InstanceDiff
+	Destroy   bool // Set only by the destroy plan
 }
 
 func (d *ModuleDiff) init() {
@@ -192,6 +193,10 @@ func (d *ModuleDiff) IsRoot() bool {
 func (d *ModuleDiff) String() string {
 	var buf bytes.Buffer
 
+	if d.Destroy {
+		buf.WriteString("DESTROY MODULE\n")
+	}
+
 	names := make([]string, 0, len(d.Resources))
 	for name, _ := range d.Resources {
 		names = append(names, name)

terraform/graph.go

@@ -80,6 +80,8 @@ type GraphNodeModule struct {
 	Config *config.Module
 	Path   []string
 	Graph  *depgraph.Graph
+	State  *ModuleState
+	Flags  ResourceFlag
 }
 
 // GraphNodeResource is a node type in the graph that represents a resource
@@ -246,6 +248,9 @@ func Graph(opts *GraphOpts) (*depgraph.Graph, error) {
 	// Add the orphan dependencies
 	graphAddOrphanDeps(g, modState)
 
+	// Add the orphan module dependencies
+	graphAddOrphanModuleDeps(g, modState)
+
 	// Add the provider dependencies
 	graphAddResourceProviderDeps(g)
 
@@ -269,7 +274,7 @@ func Graph(opts *GraphOpts) (*depgraph.Graph, error) {
 
 	// If we have a diff, then make sure to add that in
 	if modDiff != nil {
-		if err := graphAddDiff(g, modDiff); err != nil {
+		if err := graphAddDiff(g, opts.Diff, modDiff); err != nil {
 			return nil, err
 		}
 	}
@@ -298,45 +303,70 @@ func Graph(opts *GraphOpts) (*depgraph.Graph, error) {
 // allows orphaned resources to be destroyed in the proper order.
 func graphEncodeDependencies(g *depgraph.Graph) {
 	for _, n := range g.Nouns {
-		// Ignore any non-resource nodes
+		switch rn := n.Meta.(type) {
-		rn, ok := n.Meta.(*GraphNodeResource)
+		case *GraphNodeResource:
-		if !ok {
+			// If we are using create-before-destroy, there
-			continue
+			// are some special depedencies injected on the
-		}
+			// deposed node that would cause a circular depedency
-		r := rn.Resource
+			// chain if persisted. We must only handle the new node,
+			// node the deposed node.
+			r := rn.Resource
+			if r.Flags&FlagDeposed != 0 {
+				continue
+			}
 
-		// If we are using create-before-destroy, there
+			// Update the dependencies
-		// are some special depedencies injected on the
+			var inject []string
-		// deposed node that would cause a circular depedency
+			for _, dep := range n.Deps {
-		// chain if persisted. We must only handle the new node,
+				switch target := dep.Target.Meta.(type) {
-		// node the deposed node.
+				case *GraphNodeModule:
-		if r.Flags&FlagDeposed != 0 {
+					inject = append(inject, dep.Target.Name)
-			continue
-		}
 
-		// Update the dependencies
+				case *GraphNodeResource:
-		var inject []string
+					if target.Resource.Id == r.Id {
-		for _, dep := range n.Deps {
+						continue
-			switch target := dep.Target.Meta.(type) {
+					}
-			case *GraphNodeModule:
+					inject = append(inject, target.Resource.Id)
-				inject = append(inject, dep.Target.Name)
 
-			case *GraphNodeResource:
+				case *GraphNodeResourceProvider:
-				if target.Resource.Id == r.Id {
+					// Do nothing
-					continue
+
+				default:
+					panic(fmt.Sprintf("Unknown graph node: %#v", dep.Target))
 				}
-				inject = append(inject, target.Resource.Id)
+			}
 
-			case *GraphNodeResourceProvider:
+			// Update the dependencies
-				// Do nothing
+			r.Dependencies = inject
 
-			default:
+		case *GraphNodeModule:
-				panic(fmt.Sprintf("Unknown graph node: %#v", dep.Target))
+			// Update the dependencies
+			var inject []string
+			for _, dep := range n.Deps {
+				switch target := dep.Target.Meta.(type) {
+				case *GraphNodeModule:
+					if dep.Target.Name == n.Name {
+						continue
+					}
+					inject = append(inject, dep.Target.Name)
+
+				case *GraphNodeResource:
+					inject = append(inject, target.Resource.Id)
+
+				case *GraphNodeResourceProvider:
+					// Do nothing
+
+				default:
+					panic(fmt.Sprintf("Unknown graph node: %#v", dep.Target))
+				}
+
+			}
+
+			// Update the dependencies
+			if rn.State != nil {
+				rn.State.Dependencies = inject
 			}
 		}
-
-		// Update the dependencies
-		r.Dependencies = inject
 	}
 }
 
@@ -357,6 +387,14 @@ func graphAddConfigModules(
 		if n, err := graphModuleNoun(m.Name, m, g, opts); err != nil {
 			return err
 		} else {
+			// Attach the module state if any
+			if opts.State != nil {
+				module := n.Meta.(*GraphNodeModule)
+				module.State = opts.State.ModuleByPath(module.Path)
+				if module.State == nil {
+					module.State = opts.State.AddModule(module.Path)
+				}
+			}
 			nounsList = append(nounsList, n)
 		}
 	}
@@ -506,10 +544,22 @@ func graphAddConfigResources(
 // destroying the VPC's subnets first, whereas creating a VPC requires
 // doing it before the subnets are created. This function handles inserting
 // these nodes for you.
-func graphAddDiff(g *depgraph.Graph, d *ModuleDiff) error {
+func graphAddDiff(g *depgraph.Graph, gDiff *Diff, d *ModuleDiff) error {
 	var nlist []*depgraph.Noun
+	var modules []*depgraph.Noun
 	injected := make(map[*depgraph.Dependency]struct{})
 	for _, n := range g.Nouns {
+		// A module is being destroyed if all it's resources are being
+		// destroyed (via a destroy plan) or if it is orphaned. Only in
+		// those cases do we need to handle depedency inversion.
+		if mod, ok := n.Meta.(*GraphNodeModule); ok {
+			md := gDiff.ModuleByPath(mod.Path)
+			if mod.Flags&FlagOrphan != 0 || (md != nil && md.Destroy) {
+				modules = append(modules, n)
+			}
+			continue
+		}
+
 		rn, ok := n.Meta.(*GraphNodeResource)
 		if !ok {
 			continue
@@ -655,78 +705,81 @@ func graphAddDiff(g *depgraph.Graph, d *ModuleDiff) error {
 		rn.Resource.Diff = rd
 	}
 
-	// Go through each noun and make sure we calculate all the dependencies
+	// Go through each resource and module and make sure we
-	// properly.
+	// calculate all the dependencies properly.
-	for _, n := range nlist {
+	invertDeps := [][]*depgraph.Noun{nlist, modules}
-		deps := n.Deps
+	for _, list := range invertDeps {
-		num := len(deps)
+		for _, n := range list {
-		for i := 0; i < num; i++ {
+			deps := n.Deps
-			dep := deps[i]
+			num := len(deps)
+			for i := 0; i < num; i++ {
+				dep := deps[i]
 
-			// Check if this dependency was just injected, otherwise
+				// Check if this dependency was just injected, otherwise
-			// we will incorrectly flip the depedency twice.
+				// we will incorrectly flip the depedency twice.
-			if _, ok := injected[dep]; ok {
+				if _, ok := injected[dep]; ok {
-				continue
+					continue
-			}
+				}
 
-			switch target := dep.Target.Meta.(type) {
+				switch target := dep.Target.Meta.(type) {
-			case *GraphNodeResource:
+				case *GraphNodeResource:
-				// If the other node is also being deleted,
+					// If the other node is also being deleted,
-				// we must be deleted first. E.g. if A -> B,
+					// we must be deleted first. E.g. if A -> B,
-				// then when we create, B is created first then A.
+					// then when we create, B is created first then A.
-				// On teardown, A is destroyed first, then B.
+					// On teardown, A is destroyed first, then B.
-				// Thus we must flip our depedency and instead inject
+					// Thus we must flip our depedency and instead inject
-				// it on B.
+					// it on B.
-				for _, n2 := range nlist {
+					for _, n2 := range nlist {
-					rn2 := n2.Meta.(*GraphNodeResource)
+						rn2 := n2.Meta.(*GraphNodeResource)
-					if target.Resource.Id == rn2.Resource.Id {
+						if target.Resource.Id == rn2.Resource.Id {
-						newDep := &depgraph.Dependency{
+							newDep := &depgraph.Dependency{
-							Name:   n.Name,
+								Name:   n.Name,
-							Source: n2,
+								Source: n2,
-							Target: n,
+								Target: n,
+							}
+							injected[newDep] = struct{}{}
+							n2.Deps = append(n2.Deps, newDep)
+							break
 						}
-						injected[newDep] = struct{}{}
-						n2.Deps = append(n2.Deps, newDep)
-						break
 					}
+
+					// Drop the dependency. We may have created
+					// an inverse depedency if the dependent resource
+					// is also being deleted, but this dependence is
+					// no longer required.
+					deps[i], deps[num-1] = deps[num-1], nil
+					num--
+					i--
+
+				case *GraphNodeModule:
+					// We invert any module dependencies so we're destroyed
+					// first, before any modules are applied.
+					newDep := &depgraph.Dependency{
+						Name:   n.Name,
+						Source: dep.Target,
+						Target: n,
+					}
+					dep.Target.Deps = append(dep.Target.Deps, newDep)
+
+					// Drop the dependency. We may have created
+					// an inverse depedency if the dependent resource
+					// is also being deleted, but this dependence is
+					// no longer required.
+					deps[i], deps[num-1] = deps[num-1], nil
+					num--
+					i--
+				case *GraphNodeResourceProvider:
+					// Keep these around, but fix up the source to be ourselves
+					// rather than the old node.
+					newDep := *dep
+					newDep.Source = n
+					deps[i] = &newDep
+				default:
+					panic(fmt.Errorf("Unhandled depedency type: %#v", dep.Target.Meta))
 				}
-
-				// Drop the dependency. We may have created
-				// an inverse depedency if the dependent resource
-				// is also being deleted, but this dependence is
-				// no longer required.
-				deps[i], deps[num-1] = deps[num-1], nil
-				num--
-				i--
-
-			case *GraphNodeModule:
-				// We invert any module dependencies so we're destroyed
-				// first, before any modules are applied.
-				newDep := &depgraph.Dependency{
-					Name:   n.Name,
-					Source: dep.Target,
-					Target: n,
-				}
-				dep.Target.Deps = append(dep.Target.Deps, newDep)
-
-				// Drop the dependency. We may have created
-				// an inverse depedency if the dependent resource
-				// is also being deleted, but this dependence is
-				// no longer required.
-				deps[i], deps[num-1] = deps[num-1], nil
-				num--
-				i--
-			case *GraphNodeResourceProvider:
-				// Keep these around, but fix up the source to be ourselves
-				// rather than the old node.
-				newDep := *dep
-				newDep.Source = n
-				deps[i] = &newDep
-			default:
-				panic(fmt.Errorf("Unhandled depedency type: %#v", dep.Target.Meta))
 			}
+			n.Deps = deps[:num]
 		}
-		n.Deps = deps[:num]
 	}
 
 	// Add the nouns to the graph
@@ -857,6 +910,10 @@ func graphAddModuleOrphans(
 		if n, err := graphModuleNoun(k, nil, g, opts); err != nil {
 			return err
 		} else {
+			// Mark this module as being an orphan
+			module := n.Meta.(*GraphNodeModule)
+			module.Flags |= FlagOrphan
+			module.State = m
 			nounsList = append(nounsList, n)
 		}
 	}
@@ -916,6 +973,56 @@ func graphAddOrphanDeps(g *depgraph.Graph, mod *ModuleState) {
 	}
 }
 
+// graphAddOrphanModuleDeps adds the dependencies to the orphan
+// modules based on their explicit Dependencies state.
+func graphAddOrphanModuleDeps(g *depgraph.Graph, mod *ModuleState) {
+	for _, n := range g.Nouns {
+		module, ok := n.Meta.(*GraphNodeModule)
+		if !ok {
+			continue
+		}
+		if module.Flags&FlagOrphan == 0 {
+			continue
+		}
+
+		// If we have no dependencies, then just continue
+		if len(module.State.Dependencies) == 0 {
+			continue
+		}
+
+		for _, n2 := range g.Nouns {
+			// Don't ever depend on ourselves
+			if n2.Meta == n.Meta {
+				continue
+			}
+
+			var compareName string
+			switch rn2 := n2.Meta.(type) {
+			case *GraphNodeModule:
+				compareName = n2.Name
+			case *GraphNodeResource:
+				compareName = rn2.Resource.Id
+			}
+			if compareName == "" {
+				continue
+			}
+
+			for _, depName := range module.State.Dependencies {
+				if !strings.HasPrefix(depName, compareName) {
+					continue
+				}
+				dep := &depgraph.Dependency{
+					Name:   depName,
+					Source: n,
+					Target: n2,
+				}
+				n.Deps = append(n.Deps, dep)
+				break
+			}
+		}
+	}
+}
+
 // graphAddOrphans adds the orphans to the graph.
 func graphAddOrphans(g *depgraph.Graph, c *config.Config, mod *ModuleState) {
 	meta := g.Meta.(*GraphMeta)

terraform/graph_test.go

@@ -731,6 +731,61 @@ func TestGraphAddDiff_module(t *testing.T) {
 	}
 }
 
+func TestGraphAddDiff_module_depends(t *testing.T) {
+	m := testModule(t, "graph-diff-module-dep")
+	diff := &Diff{
+		Modules: []*ModuleDiff{
+			&ModuleDiff{
+				Path: rootModulePath,
+				Resources: map[string]*InstanceDiff{
+					"aws_instance.foo": &InstanceDiff{
+						Destroy: true,
+					},
+				},
+			},
+			&ModuleDiff{
+				Path:    []string{"root", "child"},
+				Destroy: true,
+				Resources: map[string]*InstanceDiff{
+					"aws_instance.foo": &InstanceDiff{
+						Destroy: true,
+					},
+				},
+			},
+		},
+	}
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: []string{"root", "orphan"},
+				Resources: map[string]*ResourceState{
+					"aws_instance.dead": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "dead",
+						},
+					},
+				},
+				Dependencies: []string{
+					"aws_instance.foo",
+					"module.child",
+				},
+			},
+		},
+	}
+
+	g, err := Graph(&GraphOpts{Module: m, Diff: diff, State: state})
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTerraformGraphDiffModuleDependsStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
 func TestGraphAddDiff_createBeforeDestroy(t *testing.T) {
 	m := testModule(t, "graph-diff-create-before")
 	diff := &Diff{
@@ -909,7 +964,7 @@ func TestGraphEncodeDependencies_count(t *testing.T) {
 func TestGraphEncodeDependencies_module(t *testing.T) {
 	m := testModule(t, "graph-modules")
 
-	g, err := Graph(&GraphOpts{Module: m})
+	g, err := Graph(&GraphOpts{Module: m, State: &State{}})
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -928,6 +983,15 @@ func TestGraphEncodeDependencies_module(t *testing.T) {
 	if web.Dependencies[1] != "module.consul" {
 		t.Fatalf("bad: %#v", web)
 	}
+
+	mod := g.Noun("module.consul").Meta.(*GraphNodeModule)
+	deps := mod.State.Dependencies
+	if len(deps) != 1 {
+		t.Fatalf("Bad: %#v", deps)
+	}
+	if deps[0] != "aws_security_group.firewall" {
+		t.Fatalf("Bad: %#v", deps)
+	}
 }
 
 func TestGraph_orphan_dependencies(t *testing.T) {
@@ -1012,6 +1076,57 @@ func TestGraph_orphanDependenciesModules(t *testing.T) {
 	}
 }
 
+func TestGraph_orphanModules_Dependencies(t *testing.T) {
+	m := testModule(t, "graph-modules")
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "foo",
+						},
+						Dependencies: []string{
+							"module.consul",
+						},
+					},
+				},
+			},
+
+			// Add an orphan module
+			&ModuleState{
+				Path: []string{"root", "orphan"},
+				Resources: map[string]*ResourceState{
+					"aws_instance.bar": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "bar",
+						},
+					},
+				},
+				Dependencies: []string{
+					"aws_instance.foo",
+					"aws_instance.web",
+				},
+			},
+		},
+	}
+
+	g, err := Graph(&GraphOpts{Module: m, State: state})
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTerraformGraphOrphanedModuleDepsStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\nactual:\n%s\n\nexpected:\n%s", actual, expected)
+	}
+}
+
 func TestGraphNodeResourceExpand(t *testing.T) {
 	m := testModule(t, "graph-resource-expand")
 
@@ -1214,6 +1329,22 @@ root
   root -> module.child
 `
 
+const testTerraformGraphDiffModuleDependsStr = `
+root: root
+aws_instance.foo
+  aws_instance.foo -> aws_instance.foo (destroy)
+aws_instance.foo (destroy)
+  aws_instance.foo (destroy) -> module.child
+  aws_instance.foo (destroy) -> module.orphan
+module.child
+  module.child -> module.orphan
+module.orphan
+root
+  root -> aws_instance.foo
+  root -> module.child
+  root -> module.orphan
+`
+
 const testTerraformGraphModulesStr = `
 root: root
 aws_instance.web
@@ -1382,6 +1513,33 @@ root
   root -> module.consul
 `
 
+const testTerraformGraphOrphanedModuleDepsStr = `
+root: root
+aws_instance.foo
+  aws_instance.foo -> module.consul
+  aws_instance.foo -> provider.aws
+aws_instance.web
+  aws_instance.web -> aws_security_group.firewall
+  aws_instance.web -> module.consul
+  aws_instance.web -> provider.aws
+aws_security_group.firewall
+  aws_security_group.firewall -> provider.aws
+module.consul
+  module.consul -> aws_security_group.firewall
+  module.consul -> provider.aws
+module.orphan
+  module.orphan -> aws_instance.foo
+  module.orphan -> aws_instance.web
+  module.orphan -> provider.aws
+provider.aws
+root
+  root -> aws_instance.foo
+  root -> aws_instance.web
+  root -> aws_security_group.firewall
+  root -> module.consul
+  root -> module.orphan
+`
+
 const testTerraformGraphResourceExpandStr = `
 root: root
 aws_instance.web.0

terraform/state.go

@@ -185,6 +185,20 @@ type ModuleState struct {
 	// N instances underneath, although a user only needs to think
 	// about the 1:1 case.
 	Resources map[string]*ResourceState `json:"resources"`
+
+	// Dependencies are a list of things that this module relies on
+	// existing to remain intact. For example: an module may depend
+	// on a VPC ID given by an aws_vpc resource.
+	//
+	// Terraform uses this information to build valid destruction
+	// orders and to warn the user if they're destroying a module that
+	// another resource depends on.
+	//
+	// Things can be put into this list that may not be managed by
+	// Terraform. If Terraform doesn't find a matching ID in the
+	// overall state, then it assumes it isn't managed and doesn't
+	// worry about it.
+	Dependencies []string `json:"depends_on,omitempty"`
 }
 
 // IsRoot says whether or not this module diff is for the root module.

terraform/state_test.go

@@ -180,6 +180,9 @@ func TestReadWriteState(t *testing.T) {
 		Modules: []*ModuleState{
 			&ModuleState{
 				Path: rootModulePath,
+				Dependencies: []string{
+					"aws_instance.bar",
+				},
 				Resources: map[string]*ResourceState{
 					"foo": &ResourceState{
 						Primary: &InstanceState{

terraform/terraform_test.go

@@ -762,6 +762,7 @@ DIFF:
 DESTROY: aws_instance.foo
 
 module.child:
+  DESTROY MODULE
   DESTROY: aws_instance.foo
 
 STATE:

terraform/test-fixtures/graph-diff-module-dep/child/main.tf

@@ -0,0 +1,5 @@
+resource "aws_instance" "foo" {}
+
+output "bar" {
+    value = "baz"
+}

terraform/test-fixtures/graph-diff-module-dep/main.tf

@@ -0,0 +1,8 @@
+resource "aws_instance" "foo" {}
+
+module "child" {
+    source = "./child"
+    in = "${aws_instance.foo.id}"
+}
+
+