From 2d6a8c1f391563f37ef1ab35b0a623457c5fdc32 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 26 Jun 2015 14:21:03 -0500 Subject: [PATCH] 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 1fbeb7399203..174581b45a2c 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 4c549ab45e80..56dddd5122f9 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 880c24637907..9c6ff67e7227 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) - } - } - - { - 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) - } + transforms := []GraphTransformer{ + &ConfigTransformer{Module: mod}, + &MissingProviderTransformer{Providers: []string{"aws"}}, + &ProviderTransformer{}, + &DisableProviderTransformer{}, + &CloseProviderTransformer{}, + &PruneProviderTransformer{}, } - { - 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 `