From d2e9c350070bd999e3fa90af934a1ede6c682c9a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 3 Nov 2016 10:25:11 -0700 Subject: [PATCH] terraform: new apply graph creates provisioners in modules Fixes #9840 The new apply graph wasn't properly nesting provisioners. This resulted in reading the provisioners being nil on apply in the shadow graph which caused the crash in the above issue. The actual cause of this is that the new graphs we're moving towards do not have any "flattening" (they are flat to begin with): all modules are in the root graph from the beginning of construction versus building a number of different graphs and flattening them. The transform that adds the provisioners wasn't modified to handle already-flat graphs and so was only adding provisioners to the root module, not children. The change modifies the `MissingProvisionerTransformer` (primarily) to support already-flat graphs and add provisioners for all module levels. Tests are there to cover this as well. **NOTE:** This PR focuses on fixing that specific issue. I'm going to follow up this PR with another PR that is more focused on being robust against crashing (more nil checks, recover() for shadow graph, etc.). In the interest of focus and keeping a PR reviewable this focuses only on the issue itself. --- terraform/context_apply_test.go | 37 +++++++++ terraform/graph_builder_apply_test.go | 6 +- terraform/terraform_test.go | 7 ++ .../apply-provisioner-module/child/main.tf | 5 ++ .../apply-provisioner-module/main.tf | 1 + .../child/main.tf | 3 + .../transform-provisioner-module/main.tf | 7 ++ terraform/transform_provisioner.go | 51 +++++++++++- terraform/transform_provisioner_test.go | 77 +++++++++++++++++++ 9 files changed, 187 insertions(+), 7 deletions(-) create mode 100644 terraform/test-fixtures/apply-provisioner-module/child/main.tf create mode 100644 terraform/test-fixtures/apply-provisioner-module/main.tf create mode 100644 terraform/test-fixtures/transform-provisioner-module/child/main.tf create mode 100644 terraform/test-fixtures/transform-provisioner-module/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 8ece50adb..337261616 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -2246,6 +2246,43 @@ func TestContext2Apply_providerConfigureDisabled(t *testing.T) { } } +func TestContext2Apply_provisionerModule(t *testing.T) { + m := testModule(t, "apply-provisioner-module") + p := testProvider("aws") + pr := testProvisioner() + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + Provisioners: map[string]ResourceProvisionerFactory{ + "shell": testProvisionerFuncFixed(pr), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + actual := strings.TrimSpace(state.String()) + expected := strings.TrimSpace(testTerraformApplyProvisionerModuleStr) + if actual != expected { + t.Fatalf("bad: \n%s", actual) + } + + // Verify apply was invoked + if !pr.ApplyCalled { + t.Fatalf("provisioner not invoked") + } +} + func TestContext2Apply_Provisioner_compute(t *testing.T) { m := testModule(t, "apply-provisioner-compute") p := testProvider("aws") diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index f224e53f1..28599602e 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -100,16 +100,16 @@ meta.count-boundary (count boundary fixup) module.child.aws_instance.create module.child.aws_instance.other module.child.provider.aws + module.child.provisioner.exec provider.aws - provisioner.exec module.child.aws_instance.create module.child.provider.aws - provisioner.exec + module.child.provisioner.exec module.child.aws_instance.other module.child.aws_instance.create module.child.provider.aws module.child.provider.aws provider.aws +module.child.provisioner.exec provider.aws -provisioner.exec ` diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 56386641f..23fe5aeeb 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -522,6 +522,13 @@ aws_instance.foo: type = aws_instance ` +const testTerraformApplyProvisionerModuleStr = ` + +module.child: + aws_instance.bar: + ID = foo +` + const testTerraformApplyProvisionerFailStr = ` aws_instance.bar: (tainted) ID = foo diff --git a/terraform/test-fixtures/apply-provisioner-module/child/main.tf b/terraform/test-fixtures/apply-provisioner-module/child/main.tf new file mode 100644 index 000000000..85b58ff94 --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-module/child/main.tf @@ -0,0 +1,5 @@ +resource "aws_instance" "bar" { + provisioner "shell" { + foo = "bar" + } +} diff --git a/terraform/test-fixtures/apply-provisioner-module/main.tf b/terraform/test-fixtures/apply-provisioner-module/main.tf new file mode 100644 index 000000000..38c53ca90 --- /dev/null +++ b/terraform/test-fixtures/apply-provisioner-module/main.tf @@ -0,0 +1 @@ +module "child" { source = "./child" } diff --git a/terraform/test-fixtures/transform-provisioner-module/child/main.tf b/terraform/test-fixtures/transform-provisioner-module/child/main.tf new file mode 100644 index 000000000..51b29c72a --- /dev/null +++ b/terraform/test-fixtures/transform-provisioner-module/child/main.tf @@ -0,0 +1,3 @@ +resource "aws_instance" "foo" { + provisioner "shell" {} +} diff --git a/terraform/test-fixtures/transform-provisioner-module/main.tf b/terraform/test-fixtures/transform-provisioner-module/main.tf new file mode 100644 index 000000000..a825a449e --- /dev/null +++ b/terraform/test-fixtures/transform-provisioner-module/main.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "foo" { + provisioner "shell" {} +} + +module "child" { + source = "./child" +} diff --git a/terraform/transform_provisioner.go b/terraform/transform_provisioner.go index 2d86275dc..5bd3f65a1 100644 --- a/terraform/transform_provisioner.go +++ b/terraform/transform_provisioner.go @@ -40,14 +40,15 @@ func (t *ProvisionerTransformer) Transform(g *Graph) error { for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProvisionerConsumer); ok { for _, p := range pv.ProvisionedBy() { - if m[p] == nil { + key := provisionerMapKey(p, pv) + if m[key] == nil { err = multierror.Append(err, fmt.Errorf( "%s: provisioner %s couldn't be found", dag.VertexName(v), p)) continue } - g.Connect(dag.BasicEdge(v, m[p])) + g.Connect(dag.BasicEdge(v, m[key])) } } } @@ -80,8 +81,21 @@ func (t *MissingProvisionerTransformer) Transform(g *Graph) error { continue } + // If this node has a subpath, then we use that as a prefix + // into our map to check for an existing provider. + var path []string + if sp, ok := pv.(GraphNodeSubPath); ok { + raw := normalizeModulePath(sp.Path()) + if len(raw) > len(rootModulePath) { + path = raw + } + } + for _, p := range pv.ProvisionedBy() { - if _, ok := m[p]; ok { + // Build the key for storing in the map + key := provisionerMapKey(p, pv) + + if _, ok := m[key]; ok { // This provisioner already exists as a configure node continue } @@ -92,8 +106,23 @@ func (t *MissingProvisionerTransformer) Transform(g *Graph) error { continue } + // Build the vertex + var newV dag.Vertex = &graphNodeProvisioner{ProvisionerNameValue: p} + if len(path) > 0 { + // If we have a path, we do the flattening immediately. This + // is to support new-style graph nodes that are already + // flattened. + if fn, ok := newV.(GraphNodeFlattenable); ok { + var err error + newV, err = fn.Flatten(path) + if err != nil { + return err + } + } + } + // Add the missing provisioner node to the graph - m[p] = g.Add(&graphNodeProvisioner{ProvisionerNameValue: p}) + m[key] = g.Add(newV) } } @@ -131,6 +160,20 @@ func (t *CloseProvisionerTransformer) Transform(g *Graph) error { return nil } +// provisionerMapKey is a helper that gives us the key to use for the +// maps returned by things such as provisionerVertexMap. +func provisionerMapKey(k string, v dag.Vertex) string { + pathPrefix := "" + if sp, ok := v.(GraphNodeSubPath); ok { + raw := normalizeModulePath(sp.Path()) + if len(raw) > len(rootModulePath) { + pathPrefix = modulePrefixStr(raw) + "." + } + } + + return pathPrefix + k +} + func provisionerVertexMap(g *Graph) map[string]dag.Vertex { m := make(map[string]dag.Vertex) for _, v := range g.Vertices() { diff --git a/terraform/transform_provisioner_test.go b/terraform/transform_provisioner_test.go index 38874bec8..3f37c5a69 100644 --- a/terraform/transform_provisioner_test.go +++ b/terraform/transform_provisioner_test.go @@ -39,6 +39,74 @@ func TestMissingProvisionerTransformer(t *testing.T) { } } +func TestMissingProvisionerTransformer_module(t *testing.T) { + mod := testModule(t, "transform-provisioner-module") + + g := Graph{Path: RootModulePath} + { + concreteResource := func(a *NodeAbstractResource) dag.Vertex { + return a + } + + var state State + state.init() + state.Modules = []*ModuleState{ + &ModuleState{ + Path: []string{"root"}, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Primary: &InstanceState{ID: "foo"}, + }, + }, + }, + + &ModuleState{ + Path: []string{"root", "child"}, + Resources: map[string]*ResourceState{ + "aws_instance.foo": &ResourceState{ + Primary: &InstanceState{ID: "foo"}, + }, + }, + }, + } + + tf := &StateTransformer{ + Concrete: concreteResource, + State: &state, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &AttachResourceConfigTransformer{Module: mod} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &MissingProvisionerTransformer{Provisioners: []string{"shell"}} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &ProvisionerTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformMissingProvisionerModuleStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + func TestCloseProvisionerTransformer(t *testing.T) { mod := testModule(t, "transform-provisioner-basic") @@ -96,6 +164,15 @@ aws_instance.web provisioner.shell ` +const testTransformMissingProvisionerModuleStr = ` +aws_instance.foo + provisioner.shell +module.child.aws_instance.foo + module.child.provisioner.shell +module.child.provisioner.shell +provisioner.shell +` + const testTransformCloseProvisionerBasicStr = ` aws_instance.web provisioner.shell