Merge pull request #10076 from hashicorp/f-depend-module

terraform: depends_on can reference entire modules

config/config.go

@@ -509,25 +509,8 @@ func (c *Config) Validate() error {
 		}
 		r.RawCount.init()
 
-		// Verify depends on points to resources that all exist
-		for _, d := range r.DependsOn {
-			// Check if we contain interpolations
-			rc, err := NewRawConfig(map[string]interface{}{
-				"value": d,
-			})
-			if err == nil && len(rc.Variables) > 0 {
-				errs = append(errs, fmt.Errorf(
-					"%s: depends on value cannot contain interpolations: %s",
-					n, d))
-				continue
-			}
-
-			if _, ok := resources[d]; !ok {
-				errs = append(errs, fmt.Errorf(
-					"%s: resource depends on non-existent resource '%s'",
-					n, d))
-			}
-		}
+		// Validate DependsOn
+		errs = append(errs, c.validateDependsOn(n, r.DependsOn, resources, modules)...)
 
 		// Verify provider points to a provider that is configured
 		if r.Provider != "" {
@@ -802,6 +785,48 @@ func (c *Config) validateVarContextFn(
 	}
 }
 
+func (c *Config) validateDependsOn(
+	n string,
+	v []string,
+	resources map[string]*Resource,
+	modules map[string]*Module) []error {
+	// Verify depends on points to resources that all exist
+	var errs []error
+	for _, d := range v {
+		// Check if we contain interpolations
+		rc, err := NewRawConfig(map[string]interface{}{
+			"value": d,
+		})
+		if err == nil && len(rc.Variables) > 0 {
+			errs = append(errs, fmt.Errorf(
+				"%s: depends on value cannot contain interpolations: %s",
+				n, d))
+			continue
+		}
+
+		// If it is a module, verify it is a module
+		if strings.HasPrefix(d, "module.") {
+			name := d[len("module."):]
+			if _, ok := modules[name]; !ok {
+				errs = append(errs, fmt.Errorf(
+					"%s: resource depends on non-existent module '%s'",
+					n, name))
+			}
+
+			continue
+		}
+
+		// Check resources
+		if _, ok := resources[d]; !ok {
+			errs = append(errs, fmt.Errorf(
+				"%s: resource depends on non-existent resource '%s'",
+				n, d))
+		}
+	}
+
+	return errs
+}
+
 func (m *Module) mergerName() string {
 	return m.Id()
 }

config/config_test.go

@@ -2,6 +2,7 @@ package config
 
 import (
 	"flag"
+	"fmt"
 	"io/ioutil"
 	"log"
 	"os"
@@ -130,11 +131,55 @@ func TestConfig_emptyCollections(t *testing.T) {
 	}
 }
 
-func TestConfigValidate(t *testing.T) {
-	c := testConfig(t, "validate-good")
-	if err := c.Validate(); err != nil {
-		t.Fatalf("err: %s", err)
+// This table test is the preferred way to test validation of configuration.
+// There are dozens of functions below which do not follow this that are
+// there mostly historically. They should be converted at some point.
+func TestConfigValidate_table(t *testing.T) {
+	cases := []struct {
+		Name      string
+		Fixture   string
+		Err       bool
+		ErrString string
+	}{
+		{
+			"basic good",
+			"validate-good",
+			false,
+			"",
+		},
+
+		{
+			"depends on module",
+			"validate-depends-on-module",
+			false,
+			"",
+		},
+
+		{
+			"depends on non-existent module",
+			"validate-depends-on-bad-module",
+			true,
+			"non-existent module 'foo'",
+		},
 	}
+
+	for i, tc := range cases {
+		t.Run(fmt.Sprintf("%d-%s", i, tc.Name), func(t *testing.T) {
+			c := testConfig(t, tc.Fixture)
+			err := c.Validate()
+			if (err != nil) != tc.Err {
+				t.Fatalf("err: %s", err)
+			}
+			if err != nil {
+				if tc.ErrString != "" && !strings.Contains(err.Error(), tc.ErrString) {
+					t.Fatalf("expected err to contain: %s\n\ngot: %s", tc.ErrString, err)
+				}
+
+				return
+			}
+		})
+	}
+
 }
 
 func TestConfigValidate_badDependsOn(t *testing.T) {

config/test-fixtures/validate-depends-on-bad-module/main.tf

@@ -0,0 +1,7 @@
+module "child" {
+    source = "./child"
+}
+
+resource aws_instance "web" {
+    depends_on = ["module.foo"]
+}

config/test-fixtures/validate-depends-on-module/main.tf

@@ -0,0 +1,7 @@
+module "child" {
+    source = "./child"
+}
+
+resource aws_instance "web" {
+    depends_on = ["module.child"]
+}

terraform/context_apply_test.go

@@ -110,6 +110,339 @@ test = []`)
 	}
 }
 
+func TestContext2Apply_resourceDependsOnModule(t *testing.T) {
+	m := testModule(t, "apply-resource-depends-on-module")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+
+	{
+		// Wait for the dependency, sleep, and verify the graph never
+		// called a child.
+		var called int32
+		var checked bool
+		p.ApplyFn = func(
+			info *InstanceInfo,
+			is *InstanceState,
+			id *InstanceDiff) (*InstanceState, error) {
+			if info.HumanId() == "module.child.aws_instance.child" {
+				checked = true
+
+				// Sleep to allow parallel execution
+				time.Sleep(50 * time.Millisecond)
+
+				// Verify that called is 0 (dep not called)
+				if atomic.LoadInt32(&called) != 0 {
+					return nil, fmt.Errorf("aws_instance.a should not be called")
+				}
+			}
+
+			atomic.AddInt32(&called, 1)
+			return testApplyFn(info, is, id)
+		}
+
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if !checked {
+			t.Fatal("should check")
+		}
+
+		checkStateString(t, state, testTerraformApplyResourceDependsOnModuleStr)
+	}
+}
+
+// Test that without a config, the Dependencies in the state are enough
+// to maintain proper ordering.
+func TestContext2Apply_resourceDependsOnModuleStateOnly(t *testing.T) {
+	m := testModule(t, "apply-resource-depends-on-module-empty")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.a": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "bar",
+						},
+						Dependencies: []string{"module.child"},
+					},
+				},
+			},
+			&ModuleState{
+				Path: []string{"root", "child"},
+				Resources: map[string]*ResourceState{
+					"aws_instance.child": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "bar",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	{
+		// Wait for the dependency, sleep, and verify the graph never
+		// called a child.
+		var called int32
+		var checked bool
+		p.ApplyFn = func(
+			info *InstanceInfo,
+			is *InstanceState,
+			id *InstanceDiff) (*InstanceState, error) {
+			if info.HumanId() == "aws_instance.a" {
+				checked = true
+
+				// Sleep to allow parallel execution
+				time.Sleep(50 * time.Millisecond)
+
+				// Verify that called is 0 (dep not called)
+				if atomic.LoadInt32(&called) != 0 {
+					return nil, fmt.Errorf("module child should not be called")
+				}
+			}
+
+			atomic.AddInt32(&called, 1)
+			return testApplyFn(info, is, id)
+		}
+
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+			State: state,
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if !checked {
+			t.Fatal("should check")
+		}
+
+		checkStateString(t, state, `
+<no state>
+module.child:
+  <no state>
+		`)
+	}
+}
+
+func TestContext2Apply_resourceDependsOnModuleDestroy(t *testing.T) {
+	m := testModule(t, "apply-resource-depends-on-module")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+
+	var globalState *State
+	{
+		p.ApplyFn = testApplyFn
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		globalState = state
+	}
+
+	{
+		// Wait for the dependency, sleep, and verify the graph never
+		// called a child.
+		var called int32
+		var checked bool
+		p.ApplyFn = func(
+			info *InstanceInfo,
+			is *InstanceState,
+			id *InstanceDiff) (*InstanceState, error) {
+			if info.HumanId() == "aws_instance.a" {
+				checked = true
+
+				// Sleep to allow parallel execution
+				time.Sleep(50 * time.Millisecond)
+
+				// Verify that called is 0 (dep not called)
+				if atomic.LoadInt32(&called) != 0 {
+					return nil, fmt.Errorf("module child should not be called")
+				}
+			}
+
+			atomic.AddInt32(&called, 1)
+			return testApplyFn(info, is, id)
+		}
+
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+			State:   globalState,
+			Destroy: true,
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if !checked {
+			t.Fatal("should check")
+		}
+
+		checkStateString(t, state, `
+<no state>
+module.child:
+  <no state>
+		`)
+	}
+}
+
+func TestContext2Apply_resourceDependsOnModuleGrandchild(t *testing.T) {
+	m := testModule(t, "apply-resource-depends-on-module-deep")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+
+	{
+		// Wait for the dependency, sleep, and verify the graph never
+		// called a child.
+		var called int32
+		var checked bool
+		p.ApplyFn = func(
+			info *InstanceInfo,
+			is *InstanceState,
+			id *InstanceDiff) (*InstanceState, error) {
+			if info.HumanId() == "module.child.grandchild.aws_instance.c" {
+				checked = true
+
+				// Sleep to allow parallel execution
+				time.Sleep(50 * time.Millisecond)
+
+				// Verify that called is 0 (dep not called)
+				if atomic.LoadInt32(&called) != 0 {
+					return nil, fmt.Errorf("aws_instance.a should not be called")
+				}
+			}
+
+			atomic.AddInt32(&called, 1)
+			return testApplyFn(info, is, id)
+		}
+
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if !checked {
+			t.Fatal("should check")
+		}
+
+		checkStateString(t, state, testTerraformApplyResourceDependsOnModuleDeepStr)
+	}
+}
+
+func TestContext2Apply_resourceDependsOnModuleInModule(t *testing.T) {
+	m := testModule(t, "apply-resource-depends-on-module-in-module")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+
+	{
+		// Wait for the dependency, sleep, and verify the graph never
+		// called a child.
+		var called int32
+		var checked bool
+		p.ApplyFn = func(
+			info *InstanceInfo,
+			is *InstanceState,
+			id *InstanceDiff) (*InstanceState, error) {
+			if info.HumanId() == "module.child.grandchild.aws_instance.c" {
+				checked = true
+
+				// Sleep to allow parallel execution
+				time.Sleep(50 * time.Millisecond)
+
+				// Verify that called is 0 (dep not called)
+				if atomic.LoadInt32(&called) != 0 {
+					return nil, fmt.Errorf("nothing else should not be called")
+				}
+			}
+
+			atomic.AddInt32(&called, 1)
+			return testApplyFn(info, is, id)
+		}
+
+		ctx := testContext2(t, &ContextOpts{
+			Module: m,
+			Providers: map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		})
+
+		if _, err := ctx.Plan(); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		state, err := ctx.Apply()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		if !checked {
+			t.Fatal("should check")
+		}
+
+		checkStateString(t, state, testTerraformApplyResourceDependsOnModuleInModuleStr)
+	}
+}
+
 func TestContext2Apply_mapVarBetweenModules(t *testing.T) {
 	m := testModule(t, "apply-map-var-through-module")
 	p := testProvider("null")

terraform/terraform_test.go

@@ -653,6 +653,43 @@ aws_instance.foo:
   num = 2
 `
 
+const testTerraformApplyResourceDependsOnModuleStr = `
+aws_instance.a:
+  ID = foo
+
+  Dependencies:
+    module.child
+
+module.child:
+  aws_instance.child:
+    ID = foo
+`
+
+const testTerraformApplyResourceDependsOnModuleDeepStr = `
+aws_instance.a:
+  ID = foo
+
+  Dependencies:
+    module.child
+
+module.child.grandchild:
+  aws_instance.c:
+    ID = foo
+`
+
+const testTerraformApplyResourceDependsOnModuleInModuleStr = `
+<no state>
+module.child:
+  aws_instance.b:
+    ID = foo
+
+    Dependencies:
+      module.grandchild
+module.child.grandchild:
+  aws_instance.c:
+    ID = foo
+`
+
 const testTerraformApplyTaintStr = `
 aws_instance.bar:
   ID = foo

terraform/test-fixtures/apply-resource-depends-on-module-deep/child/child/main.tf

@@ -0,0 +1 @@
+resource "aws_instance" "c" {}

terraform/test-fixtures/apply-resource-depends-on-module-deep/child/main.tf

@@ -0,0 +1,3 @@
+module "grandchild" {
+    source = "./child"
+}

terraform/test-fixtures/apply-resource-depends-on-module-deep/main.tf

@@ -0,0 +1,7 @@
+module "child" {
+    source = "./child"
+}
+
+resource "aws_instance" "a" {
+    depends_on = ["module.child"]
+}

terraform/test-fixtures/apply-resource-depends-on-module-empty/main.tf

@@ -0,0 +1 @@
+# Empty!

terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/child/main.tf

@@ -0,0 +1,2 @@
+resource "aws_instance" "c" {}
+

terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/main.tf

@@ -0,0 +1,7 @@
+module "grandchild" {
+    source = "./child"
+}
+
+resource "aws_instance" "b" {
+    depends_on = ["module.grandchild"]
+}

terraform/test-fixtures/apply-resource-depends-on-module-in-module/main.tf

@@ -0,0 +1,3 @@
+module "child" {
+    source = "./child"
+}

terraform/test-fixtures/apply-resource-depends-on-module/child/main.tf

@@ -0,0 +1 @@
+resource "aws_instance" "child" {}

terraform/test-fixtures/apply-resource-depends-on-module/main.tf

@@ -0,0 +1,7 @@
+module "child" {
+    source = "./child"
+}
+
+resource "aws_instance" "a" {
+    depends_on = ["module.child"]
+}

terraform/transform_reference.go

@@ -11,6 +11,11 @@ import (
 
 // GraphNodeReferenceable must be implemented by any node that represents
 // a Terraform thing that can be referenced (resource, module, etc.).
+//
+// Even if the thing has no name, this should return an empty list. By
+// implementing this and returning a non-nil result, you say that this CAN
+// be referenced and other methods of referencing may still be possible (such
+// as by path!)
 type GraphNodeReferenceable interface {
 	// ReferenceableName is the name by which this can be referenced.
 	// This can be either just the type, or include the field. Example:
@@ -200,6 +205,15 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap {
 			n = prefix + n
 			refMap[n] = append(refMap[n], v)
 		}
+
+		// If there is a path, it is always referenceable by that. For
+		// example, if this is a referenceable thing at path []string{"foo"},
+		// then it can be referenced at "module.foo"
+		if pn, ok := v.(GraphNodeSubPath); ok {
+			for _, p := range ReferenceModulePath(pn.Path()) {
+				refMap[p] = append(refMap[p], v)
+			}
+		}
 	}
 
 	// Build the lookup table for referenced by
@@ -224,6 +238,25 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap {
 	return &m
 }
 
+// Returns the reference name for a module path. The path "foo" would return
+// "module.foo". If this is a deeply nested module, it will be every parent
+// as well. For example: ["foo", "bar"] would return both "module.foo" and
+// "module.foo.module.bar"
+func ReferenceModulePath(p []string) []string {
+	p = normalizeModulePath(p)
+	if len(p) == 1 {
+		// Root, no name
+		return nil
+	}
+
+	result := make([]string, 0, len(p)-1)
+	for i := len(p); i > 1; i-- {
+		result = append(result, modulePrefixStr(p[:i]))
+	}
+
+	return result
+}
+
 // ReferencesFromConfig returns the references that a configuration has
 // based on the interpolated variables in a configuration.
 func ReferencesFromConfig(c *config.RawConfig) []string {

terraform/transform_reference_test.go

@@ -138,6 +138,54 @@ func TestReferenceTransformer_backupPrimary(t *testing.T) {
 	}
 }
 
+func TestReferenceTransformer_modulePath(t *testing.T) {
+	g := Graph{Path: RootModulePath}
+	g.Add(&graphNodeRefParentTest{
+		NameValue: "A",
+		Names:     []string{"A"},
+		PathValue: []string{"foo"},
+	})
+	g.Add(&graphNodeRefChildTest{
+		NameValue: "B",
+		Refs:      []string{"module.foo"},
+	})
+
+	tf := &ReferenceTransformer{}
+	if err := tf.Transform(&g); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformRefModulePathStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
+func TestReferenceTransformer_modulePathNormalized(t *testing.T) {
+	g := Graph{Path: RootModulePath}
+	g.Add(&graphNodeRefParentTest{
+		NameValue: "A",
+		Names:     []string{"A"},
+		PathValue: []string{"root", "foo"},
+	})
+	g.Add(&graphNodeRefChildTest{
+		NameValue: "B",
+		Refs:      []string{"module.foo"},
+	})
+
+	tf := &ReferenceTransformer{}
+	if err := tf.Transform(&g); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformRefModulePathStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
 func TestReferenceMapReferences(t *testing.T) {
 	cases := map[string]struct {
 		Nodes  []dag.Vertex
@@ -265,6 +313,12 @@ B
 C
 `
 
+const testTransformRefModulePathStr = `
+A
+B
+  A
+`
+
 const testTransformRefPathStr = `
 A
 B

website/source/docs/configuration/resources.html.md

@@ -53,8 +53,8 @@ There are **meta-parameters** available to all resources:
 
   * `depends_on` (list of strings) - Explicit dependencies that this
       resource has. These dependencies will be created before this
-      resource. The dependencies are in the format of `TYPE.NAME`,
-      for example `aws_instance.web`.
+      resource. For syntax and other details, see the section below on
+      [explicit dependencies](#explicit-dependencies).
 
   * `provider` (string) - The name of a specific provider to use for
       this resource. The name is in the format of `TYPE.ALIAS`, for example,
@@ -95,6 +95,50 @@ Additionally you can also use a single entry with a wildcard (e.g. `"*"`)
 which will match all attribute names. Using a partial string together with a
 wildcard (e.g. `"rout*"`) is **not** supported.
 
+<a id="explicit-dependencies"></a>
+
+### Explicit Dependencies
+
+Terraform ensures that dependencies are successfully created before a
+resource is created. During a destroy operation, Terraform ensures that
+this resource is destroyed before its dependencies.
+
+A resource automatically depends on anything it references via
+[interpolations](/docs/configuration/interpolation.html). The automatically
+determined dependencies are all that is needed most of the time. You can also
+use the `depends_on` parameter to explicitly define a list of additional
+dependencies.
+
+The primary use case of explicit `depends_on` is to depend on a _side effect_
+of another operation. For example: if a provisioner creates a file, and your
+resource reads that file, then there is no interpolation reference for Terraform
+to automatically connect the two resources. However, there is a causal
+ordering that needs to be represented. This is an ideal case for `depends_on`.
+In most cases, however, `depends_on` should be avoided and Terraform should
+be allowed to determine dependencies automatically.
+
+The syntax of `depends_on` is a list of resources and modules:
+
+  * Resources are `TYPE.NAME`, such as `aws_instance.web`.
+  * Modules are `module.NAME`, such as `module.foo`.
+
+When a resource depends on a module, _everything_ in that module must be
+created before the resource is created.
+
+An example of a resource depending on both a module and resource is shown
+below. Note that `depends_on` can contain any number of dependencies:
+
+```
+resource "aws_instance" "web" {
+  depends_on = ["aws_instance.leader", "module.vpc"]
+}
+```
+
+-> **Use sparingly!** `depends_on` is rarely necessary.
+In almost every case, Terraform's automatic dependency system is the best-case
+scenario by having your resources depend only on what they explicitly use.
+Please think carefully before you use `depends_on` to determine if Terraform
+could automatically do this a better way.
 
 <a id="connection-block"></a>
 
@@ -204,7 +248,7 @@ The full syntax is:
 resource TYPE NAME {
 	CONFIG ...
 	[count = COUNT]
-	[depends_on = [RESOURCE NAME, ...]]
+	[depends_on = [NAME, ...]]
 	[provider = PROVIDER]
 
     [LIFECYCLE]