Merge pull request #2527 from hashicorp/b-close-provider-target

core: fix provider/close provider race when targeting

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(`
+<no state>
+	`)
+	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")

terraform/graph_builder.go

@@ -116,13 +116,11 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
 		// Provider-related transformations
 		&MissingProviderTransformer{Providers: b.Providers},
 		&ProviderTransformer{},
-		&CloseProviderTransformer{},
 		&DisableProviderTransformer{},
 
 		// 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.

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

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)
 }

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
@@ -381,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
 }

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}
-	{
+	transforms := []GraphTransformer{
-		tf := &ConfigTransformer{Module: mod}
+		&ConfigTransformer{Module: mod},
-		if err := tf.Transform(&g); err != nil {
+		&MissingProviderTransformer{Providers: []string{"aws"}},
-			t.Fatalf("err: %s", err)
+		&ProviderTransformer{},
-		}
+		&DisableProviderTransformer{},
+		&CloseProviderTransformer{},
+		&PruneProviderTransformer{},
 	}
 
-	{
+	for _, tr := range transforms {
-		transform := &MissingProviderTransformer{Providers: []string{"aws"}}
+		if err := tr.Transform(&g); err != nil {
-		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 {
 			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}
-	{
+	transforms := []GraphTransformer{
-		tf := &ConfigTransformer{Module: mod}
+		&ConfigTransformer{Module: mod},
-		if err := tf.Transform(&g); err != nil {
+		&MissingProviderTransformer{Providers: []string{"aws"}},
-			t.Fatalf("err: %s", err)
+		&ProviderTransformer{},
-		}
+		&DisableProviderTransformer{},
+		&CloseProviderTransformer{},
+		&PruneProviderTransformer{},
 	}
 
-	{
+	for _, tr := range transforms {
-		transform := &MissingProviderTransformer{Providers: []string{"aws"}}
+		if err := tr.Transform(&g); err != nil {
-		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 {
 			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
 `

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
 }