From 0b87ef82c3c07dd58c6471343f8afeb52ba30d97 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 11 Nov 2016 18:54:37 -0800 Subject: [PATCH 1/6] terraform: depends_on can reference entire modules --- terraform/context_apply_test.go | 136 ++++++++++++++++++ terraform/terraform_test.go | 12 ++ .../child/main.tf | 1 + .../apply-resource-depends-on-module/main.tf | 7 + terraform/transform_reference.go | 21 +++ 5 files changed, 177 insertions(+) create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module/child/main.tf create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index b6ac3b2774d8..53c211807612 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -110,6 +110,142 @@ 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) + } +} + +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_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..cbe59abf1e16 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -653,6 +653,18 @@ aws_instance.foo: num = 2 ` +const testTerraformApplyResourceDependsOnModuleStr = ` +aws_instance.a: + ID = foo + + Dependencies: + module.child + +module.child: + aws_instance.child: + ID = foo +` + const testTerraformApplyTaintStr = ` aws_instance.bar: ID = foo 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..ac67032986a9 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -200,6 +200,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 { + if p := ReferenceModulePath(pn.Path()); p != "" { + refMap[p] = append(refMap[p], v) + } + } } // Build the lookup table for referenced by @@ -224,6 +233,18 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { return &m } +// Returns the reference name for a module path. The path "foo" would return +// "module.foo" +func ReferenceModulePath(p []string) string { + p = normalizeModulePath(p) + if len(p) == 1 { + // Root, no name + return "" + } + + return fmt.Sprintf("module.%s", strings.Join(p[1:], ".")) +} + // ReferencesFromConfig returns the references that a configuration has // based on the interpolated variables in a configuration. func ReferencesFromConfig(c *config.RawConfig) []string { From 576b61a21de387f47e78fc6c83d565b9d4ad215b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 12 Nov 2016 08:21:27 -0800 Subject: [PATCH 2/6] config: validate depends_on with module values --- config/config.go | 63 +++++++++++++------ config/config_test.go | 53 ++++++++++++++-- .../validate-depends-on-bad-module/main.tf | 7 +++ .../validate-depends-on-module/main.tf | 7 +++ 4 files changed, 107 insertions(+), 23 deletions(-) create mode 100644 config/test-fixtures/validate-depends-on-bad-module/main.tf create mode 100644 config/test-fixtures/validate-depends-on-module/main.tf 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"] +} From 22dd4303bcf8e88f1b28d069911027c2765dcf5c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 12 Nov 2016 08:24:09 -0800 Subject: [PATCH 3/6] terraform: tests for ReferenceMap for module paths --- terraform/transform_reference_test.go | 54 +++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) 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 From cfd3be485621b0b00f88b19c308cd20e952cc564 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 12 Nov 2016 08:41:18 -0800 Subject: [PATCH 4/6] website: update website for "module.X" depends_on --- .../docs/configuration/resources.html.md | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) 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] From bcfec4e24eb538e43beab4d30a95c527b172ca82 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 12 Nov 2016 15:22:48 -0800 Subject: [PATCH 5/6] terraform: test that dependencies in the state are enough to maintain order --- terraform/context_apply_test.go | 89 +++++++++++++++++++ .../main.tf | 1 + 2 files changed, 90 insertions(+) create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-empty/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 53c211807612..00be062b8ad9 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -164,6 +164,95 @@ func TestContext2Apply_resourceDependsOnModule(t *testing.T) { } } +// 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") 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! From 538302f143d531922a30561bc00df1d771f7668f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 12 Nov 2016 15:38:28 -0800 Subject: [PATCH 6/6] terraform: resources nested within a module must also be depended on For example: A => B => C (modules). If A depends on module B, then it also must depend on everything in module C. --- terraform/context_apply_test.go | 108 ++++++++++++++++++ terraform/terraform_test.go | 25 ++++ .../child/child/main.tf | 1 + .../child/main.tf | 3 + .../main.tf | 7 ++ .../child/child/main.tf | 2 + .../child/main.tf | 7 ++ .../main.tf | 3 + terraform/transform_reference.go | 22 +++- 9 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-deep/child/child/main.tf create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-deep/child/main.tf create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-deep/main.tf create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/child/main.tf create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-in-module/child/main.tf create mode 100644 terraform/test-fixtures/apply-resource-depends-on-module-in-module/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 00be062b8ad9..50c2012589b5 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -335,6 +335,114 @@ 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 cbe59abf1e16..453e8b79f348 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -665,6 +665,31 @@ module.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-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/transform_reference.go b/terraform/transform_reference.go index ac67032986a9..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: @@ -205,7 +210,7 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { // 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 { - if p := ReferenceModulePath(pn.Path()); p != "" { + for _, p := range ReferenceModulePath(pn.Path()) { refMap[p] = append(refMap[p], v) } } @@ -234,15 +239,22 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap { } // Returns the reference name for a module path. The path "foo" would return -// "module.foo" -func ReferenceModulePath(p []string) string { +// "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 "" + return nil } - return fmt.Sprintf("module.%s", strings.Join(p[1:], ".")) + 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