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 8ece50adb5c3..337261616c35 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 f224e53f1217..28599602e7fd 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 56386641f913..23fe5aeeba7d 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 000000000000..85b58ff94dc1 --- /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 000000000000..38c53ca901e6 --- /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 000000000000..51b29c72a082 --- /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 000000000000..a825a449eb1b --- /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 2d86275dc84f..5bd3f65a1738 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 38874bec829c..3f37c5a69b7c 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