diff --git a/config/config.go b/config/config.go index 3c8f8826d396..79367b6f9fd8 100644 --- a/config/config.go +++ b/config/config.go @@ -508,25 +508,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 != "" { @@ -801,6 +784,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() } diff --git a/config/config_test.go b/config/config_test.go index 7c7776b94eb6..8018739c753e 100644 --- a/config/config_test.go +++ b/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) { diff --git a/config/test-fixtures/validate-depends-on-bad-module/main.tf b/config/test-fixtures/validate-depends-on-bad-module/main.tf new file mode 100644 index 000000000000..5fbe75ad9931 --- /dev/null +++ b/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"] +} diff --git a/config/test-fixtures/validate-depends-on-module/main.tf b/config/test-fixtures/validate-depends-on-module/main.tf new file mode 100644 index 000000000000..73f50def8465 --- /dev/null +++ b/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"] +} diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index b6ac3b2774d8..50c2012589b5 100644 --- a/terraform/context_apply_test.go +++ b/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, ` + +module.child: + + `) + } +} + +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, ` + +module.child: + + `) + } +} + +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") diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index cf1f889243ba..453e8b79f348 100644 --- a/terraform/terraform_test.go +++ b/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 = ` + +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 diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-deep/child/child/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-deep/child/child/main.tf new file mode 100644 index 000000000000..716e64b1a774 --- /dev/null +++ b/terraform/test-fixtures/apply-resource-depends-on-module-deep/child/child/main.tf @@ -0,0 +1 @@ +resource "aws_instance" "c" {} diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-deep/child/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-deep/child/main.tf new file mode 100644 index 000000000000..6cbe350a7958 --- /dev/null +++ b/terraform/test-fixtures/apply-resource-depends-on-module-deep/child/main.tf @@ -0,0 +1,3 @@ +module "grandchild" { + source = "./child" +} diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-deep/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-deep/main.tf new file mode 100644 index 000000000000..a0859c0fdc4b --- /dev/null +++ b/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"] +} diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-empty/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-empty/main.tf new file mode 100644 index 000000000000..f2316bd73ada --- /dev/null +++ b/terraform/test-fixtures/apply-resource-depends-on-module-empty/main.tf @@ -0,0 +1 @@ +# Empty! diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/child/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/child/main.tf new file mode 100644 index 000000000000..3794f75c078e --- /dev/null +++ b/terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/child/main.tf @@ -0,0 +1,2 @@ +resource "aws_instance" "c" {} + diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/main.tf new file mode 100644 index 000000000000..62fb42cfd7b8 --- /dev/null +++ b/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"] +} diff --git a/terraform/test-fixtures/apply-resource-depends-on-module-in-module/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module-in-module/main.tf new file mode 100644 index 000000000000..0f6991c536ca --- /dev/null +++ b/terraform/test-fixtures/apply-resource-depends-on-module-in-module/main.tf @@ -0,0 +1,3 @@ +module "child" { + source = "./child" +} diff --git a/terraform/test-fixtures/apply-resource-depends-on-module/child/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module/child/main.tf new file mode 100644 index 000000000000..33e4ced51915 --- /dev/null +++ b/terraform/test-fixtures/apply-resource-depends-on-module/child/main.tf @@ -0,0 +1 @@ +resource "aws_instance" "child" {} diff --git a/terraform/test-fixtures/apply-resource-depends-on-module/main.tf b/terraform/test-fixtures/apply-resource-depends-on-module/main.tf new file mode 100644 index 000000000000..a0859c0fdc4b --- /dev/null +++ b/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"] +} diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 1da835fbd1df..7d8a5445dbed 100644 --- a/terraform/transform_reference.go +++ b/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 { diff --git a/terraform/transform_reference_test.go b/terraform/transform_reference_test.go index 544af6b04915..6468bdf7ed4d 100644 --- a/terraform/transform_reference_test.go +++ b/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 diff --git a/website/source/docs/configuration/resources.html.md b/website/source/docs/configuration/resources.html.md index e9ebb3b5dbbf..685f1a2590fd 100644 --- a/website/source/docs/configuration/resources.html.md +++ 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. + + +### 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. @@ -204,7 +248,7 @@ The full syntax is: resource TYPE NAME { CONFIG ... [count = COUNT] - [depends_on = [RESOURCE NAME, ...]] + [depends_on = [NAME, ...]] [provider = PROVIDER] [LIFECYCLE]