From 2d6a8c1f391563f37ef1ab35b0a623457c5fdc32 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 26 Jun 2015 14:21:03 -0500 Subject: [PATCH 1/5] core: fix provider/close provider race when targeting When targeting prunes out all the resource nodes between a provider and its close node, there was no dependency to ensure the close happened after the configure. Needed to add an explicit dependency from the close to the provider. This tweak highlighted the fact that CloseProviderTransformer needed to happen after DisableProviderTransformer, since DisableProviderTransformer inspects up-edges to decide what to disable, and CloseProviderTransformer adds an up-edge. fixes #2495 --- terraform/graph_builder.go | 2 +- terraform/transform_provider.go | 21 ++++- terraform/transform_provider_test.go | 130 ++++++++++++--------------- 3 files changed, 74 insertions(+), 79 deletions(-) diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index 1fbeb7399..174581b45 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -116,8 +116,8 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { // Provider-related transformations &MissingProviderTransformer{Providers: b.Providers}, &ProviderTransformer{}, - &CloseProviderTransformer{}, &DisableProviderTransformer{}, + &CloseProviderTransformer{}, // Provisioner-related transformations &MissingProvisionerTransformer{Provisioners: b.Provisioners}, diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 4c549ab45..56dddd512 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -113,28 +113,41 @@ func (t *ProviderTransformer) Transform(g *Graph) error { type CloseProviderTransformer struct{} func (t *CloseProviderTransformer) Transform(g *Graph) error { - m := closeProviderVertexMap(g) + pm := providerVertexMap(g) + cpm := closeProviderVertexMap(g) + var err error for _, v := range g.Vertices() { if pv, ok := v.(GraphNodeProviderConsumer); ok { for _, p := range pv.ProvidedBy() { - source := m[p] + source := cpm[p] if source == nil { // Create a new graphNodeCloseProvider and add it to the graph source = &graphNodeCloseProvider{ProviderNameValue: p} g.Add(source) + // Close node needs to depend on provider + provider, ok := pm[p] + if !ok { + err = multierror.Append(err, fmt.Errorf( + "%s: provider %s couldn't be found", + dag.VertexName(v), p)) + continue + } + g.Connect(dag.BasicEdge(source, provider)) + // Make sure we also add the new graphNodeCloseProvider to the map // so we don't create and add any duplicate graphNodeCloseProviders. - m[p] = source + cpm[p] = source } + // Close node depends on all nodes provided by the provider g.Connect(dag.BasicEdge(source, v)) } } } - return nil + return err } // MissingProviderTransformer is a GraphTransformer that adds nodes diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index 880c24637..9c6ff67e7 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -62,6 +62,36 @@ func TestCloseProviderTransformer(t *testing.T) { } } +func TestCloseProviderTransformer_withTargets(t *testing.T) { + mod := testModule(t, "transform-provider-basic") + + g := Graph{Path: RootModulePath} + transforms := []GraphTransformer{ + &ConfigTransformer{Module: mod}, + &ProviderTransformer{}, + &CloseProviderTransformer{}, + &TargetsTransformer{ + Targets: []string{"something.else"}, + }, + } + + for _, tr := range transforms { + if err := tr.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(` +provider.aws +provider.aws (close) + provider.aws + `) + if actual != expected { + t.Fatalf("expected:%s\n\ngot:\n\n%s", expected, actual) + } +} + func TestMissingProviderTransformer(t *testing.T) { mod := testModule(t, "transform-provider-missing") @@ -144,44 +174,17 @@ func TestDisableProviderTransformer(t *testing.T) { mod := testModule(t, "transform-provider-disable") g := Graph{Path: RootModulePath} - { - tf := &ConfigTransformer{Module: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } + transforms := []GraphTransformer{ + &ConfigTransformer{Module: mod}, + &MissingProviderTransformer{Providers: []string{"aws"}}, + &ProviderTransformer{}, + &DisableProviderTransformer{}, + &CloseProviderTransformer{}, + &PruneProviderTransformer{}, } - { - transform := &MissingProviderTransformer{Providers: []string{"aws"}} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &ProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &CloseProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &PruneProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &DisableProviderTransformer{} - if err := transform.Transform(&g); err != nil { + for _, tr := range transforms { + if err := tr.Transform(&g); err != nil { t.Fatalf("err: %s", err) } } @@ -189,7 +192,7 @@ func TestDisableProviderTransformer(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testTransformDisableProviderBasicStr) if actual != expected { - t.Fatalf("bad:\n\n%s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s\n", expected, actual) } } @@ -197,44 +200,17 @@ func TestDisableProviderTransformer_keep(t *testing.T) { mod := testModule(t, "transform-provider-disable-keep") g := Graph{Path: RootModulePath} - { - tf := &ConfigTransformer{Module: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } + transforms := []GraphTransformer{ + &ConfigTransformer{Module: mod}, + &MissingProviderTransformer{Providers: []string{"aws"}}, + &ProviderTransformer{}, + &DisableProviderTransformer{}, + &CloseProviderTransformer{}, + &PruneProviderTransformer{}, } - { - transform := &MissingProviderTransformer{Providers: []string{"aws"}} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &ProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &CloseProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &PruneProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &DisableProviderTransformer{} - if err := transform.Transform(&g); err != nil { + for _, tr := range transforms { + if err := tr.Transform(&g); err != nil { t.Fatalf("err: %s", err) } } @@ -242,7 +218,7 @@ func TestDisableProviderTransformer_keep(t *testing.T) { actual := strings.TrimSpace(g.String()) expected := strings.TrimSpace(testTransformDisableProviderKeepStr) if actual != expected { - t.Fatalf("bad:\n\n%s", actual) + t.Fatalf("expected:\n%s\n\ngot:\n%s\n", expected, actual) } } @@ -271,6 +247,7 @@ aws_instance.web provider.aws provider.aws (close) aws_instance.web + provider.aws ` const testTransformMissingProviderBasicStr = ` @@ -279,9 +256,11 @@ foo_instance.web provider.aws provider.aws (close) aws_instance.web + provider.aws provider.foo provider.foo (close) foo_instance.web + provider.foo ` const testTransformPruneProviderBasicStr = ` @@ -290,6 +269,7 @@ foo_instance.web provider.foo provider.foo (close) foo_instance.web + provider.foo ` const testTransformDisableProviderBasicStr = ` @@ -298,6 +278,7 @@ module.child var.foo provider.aws (close) module.child + provider.aws (disabled) provider.aws (disabled) var.foo ` @@ -312,5 +293,6 @@ provider.aws provider.aws (close) aws_instance.foo module.child + provider.aws var.foo ` From 00a177cd99b702fd11f9dbc5378375c92c451ddd Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Mon, 29 Jun 2015 11:35:02 -0500 Subject: [PATCH 2/5] provider/aws: add test for provider aliases Not sure if this test has value /cc @mitchellh (who requested one be added) to see what I might be missing here. refs #2495 --- terraform/context_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/terraform/context_test.go b/terraform/context_test.go index 0cb256168..60ff159e9 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -4423,6 +4423,37 @@ func TestContext2Apply_moduleProviderAlias(t *testing.T) { } } +func TestContext2Apply_moduleProviderAliasTargets(t *testing.T) { + m := testModule(t, "apply-module-provider-alias") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + Targets: []string{"no.thing"}, + }) + + 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(` + + `) + if actual != expected { + t.Fatalf("bad: \n%s", actual) + } +} + func TestContext2Apply_moduleVarResourceCount(t *testing.T) { m := testModule(t, "apply-module-var-resource-count") p := testProvider("aws") From d6f72611906e876c3a0f226f183367bb93c73f32 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 29 Jun 2015 10:33:37 -0700 Subject: [PATCH 3/5] terraform: provider mock should close itself to find bugs --- terraform/resource_provider_mock.go | 7 +++++++ terraform/resource_provider_mock_test.go | 1 + 2 files changed, 8 insertions(+) diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index fa2bd24c6..3b2b351dc 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -10,6 +10,8 @@ type MockResourceProvider struct { // Anything you want, in case you need to store extra data with the mock. Meta interface{} + CloseCalled bool + CloseError error InputCalled bool InputInput UIInput InputConfig *ResourceConfig @@ -55,6 +57,11 @@ type MockResourceProvider struct { ValidateResourceReturnErrors []error } +func (p *MockResourceProvider) Close() error { + p.CloseCalled = true + return p.CloseError +} + func (p *MockResourceProvider) Input( input UIInput, c *ResourceConfig) (*ResourceConfig, error) { p.InputCalled = true diff --git a/terraform/resource_provider_mock_test.go b/terraform/resource_provider_mock_test.go index 0beaf87d1..ebefadf29 100644 --- a/terraform/resource_provider_mock_test.go +++ b/terraform/resource_provider_mock_test.go @@ -6,4 +6,5 @@ import ( func TestMockResourceProvider_impl(t *testing.T) { var _ ResourceProvider = new(MockResourceProvider) + var _ ResourceProviderCloser = new(MockResourceProvider) } From 557b299c4b2b6d53f1165f2e199f080663b2bbcc Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Mon, 29 Jun 2015 12:40:20 -0500 Subject: [PATCH 4/5] core: move close transforms after flatten and destroy transforms fixes destroy-related failures introduced by the close transforms --- terraform/graph_builder.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index 174581b45..8a697fee8 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -117,12 +117,10 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { &MissingProviderTransformer{Providers: b.Providers}, &ProviderTransformer{}, &DisableProviderTransformer{}, - &CloseProviderTransformer{}, // Provisioner-related transformations &MissingProvisionerTransformer{Provisioners: b.Provisioners}, &ProvisionerTransformer{}, - &CloseProvisionerTransformer{}, // Run our vertex-level transforms &VertexTransformer{ @@ -166,6 +164,10 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { Then: &PruneDestroyTransformer{Diff: b.Diff, State: b.State}, }), + // Insert nodes to close opened plugin connections + &CloseProviderTransformer{}, + &CloseProvisionerTransformer{}, + // Make sure we have a single root after the above changes. // This is the 2nd root transformer. In practice this shouldn't // actually matter as the RootTransformer is idempotent. From 184edbefcd2810c8b530c7b701fe7354f048bf48 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Mon, 29 Jun 2015 12:46:24 -0500 Subject: [PATCH 5/5] core: remove now-unused flatten impls of close nodes /cc @mitchellh --- terraform/transform_provider.go | 53 ------------------------------ terraform/transform_provisioner.go | 30 ----------------- 2 files changed, 83 deletions(-) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 56dddd512..fb8857c96 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -394,59 +394,6 @@ func (n *graphNodeCloseProvider) DotNode(name string, opts *GraphDotOpts) *dot.N }) } -// GraphNodeFlattenable impl. -func (n *graphNodeCloseProvider) Flatten(p []string) (dag.Vertex, error) { - return &graphNodeCloseProviderFlat{ - graphNodeCloseProvider: n, - PathValue: p, - }, nil -} - -// Same as graphNodeCloseProvider, but for flattening -type graphNodeCloseProviderFlat struct { - *graphNodeCloseProvider - - PathValue []string -} - -func (n *graphNodeCloseProviderFlat) Name() string { - return fmt.Sprintf( - "%s.%s", modulePrefixStr(n.PathValue), n.graphNodeCloseProvider.Name()) -} - -func (n *graphNodeCloseProviderFlat) Path() []string { - return n.PathValue -} - -func (n *graphNodeCloseProviderFlat) ProviderName() string { - return fmt.Sprintf( - "%s.%s", modulePrefixStr(n.PathValue), - n.graphNodeCloseProvider.CloseProviderName()) -} - -// GraphNodeDependable impl. -func (n *graphNodeCloseProviderFlat) DependableName() []string { - return []string{n.Name()} -} - -func (n *graphNodeCloseProviderFlat) DependentOn() []string { - var result []string - - // If we're in a module, then depend on our parent's provider - if len(n.PathValue) > 1 { - prefix := modulePrefixStr(n.PathValue[:len(n.PathValue)-1]) - if prefix != "" { - prefix += "." - } - - result = append(result, fmt.Sprintf( - "%s%s", - prefix, n.graphNodeCloseProvider.Name())) - } - - return result -} - type graphNodeMissingProvider struct { ProviderNameValue string } diff --git a/terraform/transform_provisioner.go b/terraform/transform_provisioner.go index a1107a121..a99fe48b0 100644 --- a/terraform/transform_provisioner.go +++ b/terraform/transform_provisioner.go @@ -171,36 +171,6 @@ func (n *graphNodeCloseProvisioner) CloseProvisionerName() string { return n.ProvisionerNameValue } -// GraphNodeFlattenable impl. -func (n *graphNodeCloseProvisioner) Flatten(p []string) (dag.Vertex, error) { - return &graphNodeCloseProvisionerFlat{ - graphNodeCloseProvisioner: n, - PathValue: p, - }, nil -} - -// Same as graphNodeCloseProvisioner, but for flattening -type graphNodeCloseProvisionerFlat struct { - *graphNodeCloseProvisioner - - PathValue []string -} - -func (n *graphNodeCloseProvisionerFlat) Name() string { - return fmt.Sprintf( - "%s.%s", modulePrefixStr(n.PathValue), n.graphNodeCloseProvisioner.Name()) -} - -func (n *graphNodeCloseProvisionerFlat) Path() []string { - return n.PathValue -} - -func (n *graphNodeCloseProvisionerFlat) ProvisionerName() string { - return fmt.Sprintf( - "%s.%s", modulePrefixStr(n.PathValue), - n.graphNodeCloseProvisioner.CloseProvisionerName()) -} - type graphNodeMissingProvisioner struct { ProvisionerNameValue string }