From 0b1dbf31a37f2d68000e5f652fbaa5b58626a2a1 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Fri, 19 Jun 2015 21:52:50 +0200 Subject: [PATCH] core: close provider/provisioner connections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently Terraform is leaking goroutines and with that memory. I know strictly speaking this maybe isn’t a real concern for Terraform as it’s mostly used as a short running command line executable. But there are a few of us out there that are using Terraform in some long running processes and then this starts to become a problem. Next to that it’s of course good programming practise to clean up resources when they're not needed anymore. So even for the standard command line use case, this seems an improvement in resource management. Personally I see no downsides as the primary connection to the plugin is kept alive (the plugin is not killed) and only unused connections that will never be used again are closed to free up any related goroutines and memory. --- rpc/mux_broker.go | 10 +- rpc/resource_provider.go | 4 + rpc/resource_provider_test.go | 28 +++++ rpc/resource_provisioner.go | 4 + rpc/resource_provisioner_test.go | 31 +++++ terraform/eval_context.go | 7 ++ terraform/eval_context_builtin.go | 44 +++++++ terraform/eval_context_mock.go | 20 +++ terraform/eval_provider.go | 11 ++ terraform/eval_provider_test.go | 16 +++ terraform/eval_provisioner.go | 11 ++ terraform/eval_provisioner_test.go | 16 +++ terraform/evaltree_provider.go | 6 + terraform/graph_builder.go | 2 + terraform/graph_builder_test.go | 37 +++--- terraform/resource_provider.go | 6 + terraform/resource_provisioner.go | 6 + terraform/transform_provider.go | 159 ++++++++++++++++++++++-- terraform/transform_provider_test.go | 85 ++++++++++++- terraform/transform_provisioner.go | 118 ++++++++++++++++-- terraform/transform_provisioner_test.go | 26 +++- 21 files changed, 601 insertions(+), 46 deletions(-) diff --git a/rpc/mux_broker.go b/rpc/mux_broker.go index 639902a825..dbc98d1c20 100644 --- a/rpc/mux_broker.go +++ b/rpc/mux_broker.go @@ -3,9 +3,9 @@ package rpc import ( "encoding/binary" "fmt" + "math/rand" "net" "sync" - "sync/atomic" "time" "github.com/hashicorp/yamux" @@ -17,7 +17,6 @@ import ( // or accept a connection from, and the broker handles the details of // holding these channels open while they're being negotiated. type muxBroker struct { - nextId uint32 session *yamux.Session streams map[uint32]*muxBrokerPending @@ -95,9 +94,12 @@ func (m *muxBroker) Dial(id uint32) (net.Conn, error) { return stream, nil } -// NextId returns a unique ID to use next. +// NextId returns a unique ID to use next. There is no need for seeding the +// rand package as the returned ID's aren't stored or used anywhere outside +// the current runtime. So it's perfectly fine to get the same pseudo-random +// numbers each time terraform is running. func (m *muxBroker) NextId() uint32 { - return atomic.AddUint32(&m.nextId, 1) + return rand.Uint32() } // Run starts the brokering and should be executed in a goroutine, since it diff --git a/rpc/resource_provider.go b/rpc/resource_provider.go index e3fa4241cc..3fe6927de8 100644 --- a/rpc/resource_provider.go +++ b/rpc/resource_provider.go @@ -174,6 +174,10 @@ func (p *ResourceProvider) Resources() []terraform.ResourceType { return result } +func (p *ResourceProvider) Close() error { + return p.Client.Close() +} + // ResourceProviderServer is a net/rpc compatible structure for serving // a ResourceProvider. This should not be used directly. type ResourceProviderServer struct { diff --git a/rpc/resource_provider_test.go b/rpc/resource_provider_test.go index 91cf38bfe2..3efdbce25f 100644 --- a/rpc/resource_provider_test.go +++ b/rpc/resource_provider_test.go @@ -488,3 +488,31 @@ func TestResourceProvider_validateResource_warns(t *testing.T) { t.Fatalf("bad: %#v", w) } } + +func TestResourceProvider_close(t *testing.T) { + client, _ := testNewClientServer(t) + defer client.Close() + + provider, err := client.ResourceProvider() + if err != nil { + t.Fatalf("err: %s", err) + } + + var p interface{} + p = provider + pCloser, ok := p.(terraform.ResourceProviderCloser) + if !ok { + t.Fatal("should be a ResourceProviderCloser") + } + + if err := pCloser.Close(); err != nil { + t.Fatalf("failed to close provider: %s", err) + } + + // The connection should be closed now, so if we to make a + // new call we should get an error. + err = provider.Configure(&terraform.ResourceConfig{}) + if err == nil { + t.Fatal("should have error") + } +} diff --git a/rpc/resource_provisioner.go b/rpc/resource_provisioner.go index cf8c008120..715704d024 100644 --- a/rpc/resource_provisioner.go +++ b/rpc/resource_provisioner.go @@ -63,6 +63,10 @@ func (p *ResourceProvisioner) Apply( return err } +func (p *ResourceProvisioner) Close() error { + return p.Client.Close() +} + type ResourceProvisionerValidateArgs struct { Config *terraform.ResourceConfig } diff --git a/rpc/resource_provisioner_test.go b/rpc/resource_provisioner_test.go index 32b6c06f38..6fabdb6d41 100644 --- a/rpc/resource_provisioner_test.go +++ b/rpc/resource_provisioner_test.go @@ -132,3 +132,34 @@ func TestResourceProvisioner_validate_warns(t *testing.T) { t.Fatalf("bad: %#v", w) } } + +func TestResourceProvisioner_close(t *testing.T) { + client, _ := testNewClientServer(t) + defer client.Close() + + provisioner, err := client.ResourceProvisioner() + if err != nil { + t.Fatalf("err: %s", err) + } + + var p interface{} + p = provisioner + pCloser, ok := p.(terraform.ResourceProvisionerCloser) + if !ok { + t.Fatal("should be a ResourceProvisionerCloser") + } + + if err := pCloser.Close(); err != nil { + t.Fatalf("failed to close provisioner: %s", err) + } + + // The connection should be closed now, so if we to make a + // new call we should get an error. + o := &terraform.MockUIOutput{} + s := &terraform.InstanceState{} + c := &terraform.ResourceConfig{} + err = provisioner.Apply(o, s, c) + if err == nil { + t.Fatal("should have error") + } +} diff --git a/terraform/eval_context.go b/terraform/eval_context.go index 8a838afb71..f4427939ae 100644 --- a/terraform/eval_context.go +++ b/terraform/eval_context.go @@ -28,6 +28,9 @@ type EvalContext interface { // initialized) or returns nil if the provider isn't initialized. Provider(string) ResourceProvider + // CloseProvider closes provider connections that aren't needed anymore. + CloseProvider(string) error + // ConfigureProvider configures the provider with the given // configuration. This is a separate context call because this call // is used to store the provider configuration for inheritance lookups @@ -51,6 +54,10 @@ type EvalContext interface { // initialized) or returns nil if the provisioner isn't initialized. Provisioner(string) ResourceProvisioner + // CloseProvisioner closes provisioner connections that aren't needed + // anymore. + CloseProvisioner(string) error + // Interpolate takes the given raw configuration and completes // the interpolations, returning the processed ResourceConfig. // diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index efc7d04fe4..f7cdf27cc3 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -114,6 +114,28 @@ func (ctx *BuiltinEvalContext) Provider(n string) ResourceProvider { return ctx.ProviderCache[PathCacheKey(providerPath)] } +func (ctx *BuiltinEvalContext) CloseProvider(n string) error { + ctx.once.Do(ctx.init) + + ctx.ProviderLock.Lock() + defer ctx.ProviderLock.Unlock() + + providerPath := make([]string, len(ctx.Path())+1) + copy(providerPath, ctx.Path()) + providerPath[len(providerPath)-1] = n + + var provider interface{} + provider = ctx.ProviderCache[PathCacheKey(providerPath)] + if provider != nil { + if p, ok := provider.(ResourceProviderCloser); ok { + delete(ctx.ProviderCache, PathCacheKey(providerPath)) + return p.Close() + } + } + + return nil +} + func (ctx *BuiltinEvalContext) ConfigureProvider( n string, cfg *ResourceConfig) error { p := ctx.Provider(n) @@ -222,6 +244,28 @@ func (ctx *BuiltinEvalContext) Provisioner(n string) ResourceProvisioner { return ctx.ProvisionerCache[PathCacheKey(provPath)] } +func (ctx *BuiltinEvalContext) CloseProvisioner(n string) error { + ctx.once.Do(ctx.init) + + ctx.ProvisionerLock.Lock() + defer ctx.ProvisionerLock.Unlock() + + provPath := make([]string, len(ctx.Path())+1) + copy(provPath, ctx.Path()) + provPath[len(provPath)-1] = n + + var prov interface{} + prov = ctx.ProvisionerCache[PathCacheKey(provPath)] + if prov != nil { + if p, ok := prov.(ResourceProvisionerCloser); ok { + delete(ctx.ProvisionerCache, PathCacheKey(provPath)) + return p.Close() + } + } + + return nil +} + func (ctx *BuiltinEvalContext) Interpolate( cfg *config.RawConfig, r *Resource) (*ResourceConfig, error) { if cfg != nil { diff --git a/terraform/eval_context_mock.go b/terraform/eval_context_mock.go index 2a5856829d..60d83c7240 100644 --- a/terraform/eval_context_mock.go +++ b/terraform/eval_context_mock.go @@ -25,6 +25,10 @@ type MockEvalContext struct { ProviderName string ProviderProvider ResourceProvider + CloseProviderCalled bool + CloseProviderName string + CloseProviderProvider ResourceProvider + ProviderInputCalled bool ProviderInputName string ProviderInputConfig map[string]interface{} @@ -55,6 +59,10 @@ type MockEvalContext struct { ProvisionerName string ProvisionerProvisioner ResourceProvisioner + CloseProvisionerCalled bool + CloseProvisionerName string + CloseProvisionerProvisioner ResourceProvisioner + InterpolateCalled bool InterpolateConfig *config.RawConfig InterpolateResource *Resource @@ -105,6 +113,12 @@ func (c *MockEvalContext) Provider(n string) ResourceProvider { return c.ProviderProvider } +func (c *MockEvalContext) CloseProvider(n string) error { + c.CloseProviderCalled = true + c.CloseProviderName = n + return nil +} + func (c *MockEvalContext) ConfigureProvider(n string, cfg *ResourceConfig) error { c.ConfigureProviderCalled = true c.ConfigureProviderName = n @@ -150,6 +164,12 @@ func (c *MockEvalContext) Provisioner(n string) ResourceProvisioner { return c.ProvisionerProvisioner } +func (c *MockEvalContext) CloseProvisioner(n string) error { + c.CloseProvisionerCalled = true + c.CloseProvisionerName = n + return nil +} + func (c *MockEvalContext) Interpolate( config *config.RawConfig, resource *Resource) (*ResourceConfig, error) { c.InterpolateCalled = true diff --git a/terraform/eval_provider.go b/terraform/eval_provider.go index e5205a556d..61efcc2352 100644 --- a/terraform/eval_provider.go +++ b/terraform/eval_provider.go @@ -71,6 +71,17 @@ func (n *EvalInitProvider) Eval(ctx EvalContext) (interface{}, error) { return ctx.InitProvider(n.Name) } +// EvalCloseProvider is an EvalNode implementation that closes provider +// connections that aren't needed anymore. +type EvalCloseProvider struct { + Name string +} + +func (n *EvalCloseProvider) Eval(ctx EvalContext) (interface{}, error) { + ctx.CloseProvider(n.Name) + return nil, nil +} + // EvalGetProvider is an EvalNode implementation that retrieves an already // initialized provider instance for the given name. type EvalGetProvider struct { diff --git a/terraform/eval_provider_test.go b/terraform/eval_provider_test.go index 5d50d746ba..5719b62c59 100644 --- a/terraform/eval_provider_test.go +++ b/terraform/eval_provider_test.go @@ -112,6 +112,22 @@ func TestEvalInitProvider(t *testing.T) { } } +func TestEvalCloseProvider(t *testing.T) { + n := &EvalCloseProvider{Name: "foo"} + provider := &MockResourceProvider{} + ctx := &MockEvalContext{CloseProviderProvider: provider} + if _, err := n.Eval(ctx); err != nil { + t.Fatalf("err: %s", err) + } + + if !ctx.CloseProviderCalled { + t.Fatal("should be called") + } + if ctx.CloseProviderName != "foo" { + t.Fatalf("bad: %#v", ctx.CloseProviderName) + } +} + func TestEvalGetProvider_impl(t *testing.T) { var _ EvalNode = new(EvalGetProvider) } diff --git a/terraform/eval_provisioner.go b/terraform/eval_provisioner.go index 362a26e845..89579c0557 100644 --- a/terraform/eval_provisioner.go +++ b/terraform/eval_provisioner.go @@ -15,6 +15,17 @@ func (n *EvalInitProvisioner) Eval(ctx EvalContext) (interface{}, error) { return ctx.InitProvisioner(n.Name) } +// EvalCloseProvisioner is an EvalNode implementation that closes provisioner +// connections that aren't needed anymore. +type EvalCloseProvisioner struct { + Name string +} + +func (n *EvalCloseProvisioner) Eval(ctx EvalContext) (interface{}, error) { + ctx.CloseProvisioner(n.Name) + return nil, nil +} + // EvalGetProvisioner is an EvalNode implementation that retrieves an already // initialized provisioner instance for the given name. type EvalGetProvisioner struct { diff --git a/terraform/eval_provisioner_test.go b/terraform/eval_provisioner_test.go index 1225c5abb7..390bc364d7 100644 --- a/terraform/eval_provisioner_test.go +++ b/terraform/eval_provisioner_test.go @@ -24,6 +24,22 @@ func TestEvalInitProvisioner(t *testing.T) { } } +func TestEvalCloseProvisioner(t *testing.T) { + n := &EvalCloseProvisioner{Name: "foo"} + provisioner := &MockResourceProvisioner{} + ctx := &MockEvalContext{CloseProvisionerProvisioner: provisioner} + if _, err := n.Eval(ctx); err != nil { + t.Fatalf("err: %s", err) + } + + if !ctx.CloseProvisionerCalled { + t.Fatal("should be called") + } + if ctx.CloseProvisionerName != "foo" { + t.Fatalf("bad: %#v", ctx.CloseProvisionerName) + } +} + func TestEvalGetProvisioner_impl(t *testing.T) { var _ EvalNode = new(EvalGetProvisioner) } diff --git a/terraform/evaltree_provider.go b/terraform/evaltree_provider.go index 59916d9b5e..6b26d43e95 100644 --- a/terraform/evaltree_provider.go +++ b/terraform/evaltree_provider.go @@ -72,3 +72,9 @@ func ProviderEvalTree(n string, config *config.RawConfig) EvalNode { return &EvalSequence{Nodes: seq} } + +// CloseProviderEvalTree returns the evaluation tree for closing +// provider connections that aren't needed anymore. +func CloseProviderEvalTree(n string) EvalNode { + return &EvalCloseProvider{Name: n} +} diff --git a/terraform/graph_builder.go b/terraform/graph_builder.go index 28a76e5138..061edb599e 100644 --- a/terraform/graph_builder.go +++ b/terraform/graph_builder.go @@ -116,12 +116,14 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer { // Provider-related transformations &MissingProviderTransformer{Providers: b.Providers}, &ProviderTransformer{}, + &CloseProviderTransformer{}, &PruneProviderTransformer{}, &DisableProviderTransformer{}, // Provisioner-related transformations &MissingProvisionerTransformer{Provisioners: b.Provisioners}, &ProvisionerTransformer{}, + &CloseProvisionerTransformer{}, &PruneProvisionerTransformer{}, // Run our vertex-level transforms diff --git a/terraform/graph_builder_test.go b/terraform/graph_builder_test.go index 1f7f250558..ee6a5e140c 100644 --- a/terraform/graph_builder_test.go +++ b/terraform/graph_builder_test.go @@ -196,6 +196,8 @@ aws_instance.db aws_instance.web aws_instance.db provider.aws +provider.aws (close) + aws_instance.web ` const testBuiltinGraphBuilderVerboseStr = ` @@ -213,8 +215,28 @@ aws_instance.web (destroy tainted) aws_instance.web (destroy) provider.aws provider.aws +provider.aws (close) + aws_instance.web ` +const testBuiltinGraphBuilderMultiLevelStr = ` +module.foo.module.bar.output.value + module.foo.module.bar.var.bar +module.foo.module.bar.plan-destroy +module.foo.module.bar.var.bar + module.foo.var.foo +module.foo.plan-destroy +module.foo.var.foo +root + module.foo.module.bar.output.value + module.foo.module.bar.plan-destroy + module.foo.plan-destroy +` + +/* +TODO: Commented out this const as it's likely this needs to +be updated when the TestBuiltinGraphBuilder_modules test is +enabled again. const testBuiltinGraphBuilderModuleStr = ` aws_instance.web aws_instance.web (destroy) @@ -231,17 +253,4 @@ module.consul (expanded) provider.aws provider.aws ` - -const testBuiltinGraphBuilderMultiLevelStr = ` -module.foo.module.bar.output.value - module.foo.module.bar.var.bar -module.foo.module.bar.plan-destroy -module.foo.module.bar.var.bar - module.foo.var.foo -module.foo.plan-destroy -module.foo.var.foo -root - module.foo.module.bar.output.value - module.foo.module.bar.plan-destroy - module.foo.plan-destroy -` +*/ diff --git a/terraform/resource_provider.go b/terraform/resource_provider.go index d8705232d0..ea23b031d3 100644 --- a/terraform/resource_provider.go +++ b/terraform/resource_provider.go @@ -71,6 +71,12 @@ type ResourceProvider interface { Refresh(*InstanceInfo, *InstanceState) (*InstanceState, error) } +// ResourceProviderCloser is an interface that providers that can close +// connections that aren't needed anymore must implement. +type ResourceProviderCloser interface { + Close() error +} + // ResourceType is a type of resource that a resource provider can manage. type ResourceType struct { Name string diff --git a/terraform/resource_provisioner.go b/terraform/resource_provisioner.go index acc80d054a..3327e30007 100644 --- a/terraform/resource_provisioner.go +++ b/terraform/resource_provisioner.go @@ -23,6 +23,12 @@ type ResourceProvisioner interface { Apply(UIOutput, *InstanceState, *ResourceConfig) error } +// ResourceProvisionerCloser is an interface that provisioners that can close +// connections that aren't needed anymore must implement. +type ResourceProvisionerCloser interface { + Close() error +} + // ResourceProvisionerFactory is a function type that creates a new instance // of a resource provisioner. type ResourceProvisionerFactory func() (ResourceProvisioner, error) diff --git a/terraform/transform_provider.go b/terraform/transform_provider.go index 4863a4c987..3a017fee24 100644 --- a/terraform/transform_provider.go +++ b/terraform/transform_provider.go @@ -17,6 +17,13 @@ type GraphNodeProvider interface { ProviderConfig() *config.RawConfig } +// GraphNodeCloseProvider is an interface that nodes that can be a close +// provider must implement. The CloseProviderName returned is the name of +// the provider they satisfy. +type GraphNodeCloseProvider interface { + CloseProviderName() string +} + // GraphNodeProviderConsumer is an interface that nodes that require // a provider must implement. ProvidedBy must return the name of the provider // to use. @@ -98,6 +105,37 @@ func (t *ProviderTransformer) Transform(g *Graph) error { return err } +// CloseProviderTransformer is a GraphTransformer that adds nodes to the +// graph that will close open provider connections that aren't needed anymore. +// A provider connection is not needed anymore once all depended resources +// in the graph are evaluated. +type CloseProviderTransformer struct{} + +func (t *CloseProviderTransformer) Transform(g *Graph) error { + m := closeProviderVertexMap(g) + for _, v := range g.Vertices() { + if pv, ok := v.(GraphNodeProviderConsumer); ok { + for _, p := range pv.ProvidedBy() { + source := m[p] + + if source == nil { + // Create a new graphNodeCloseProvider and add it to the graph + source = &graphNodeCloseProvider{ProviderNameValue: p} + g.Add(source) + + // Make sure we also add the new graphNodeCloseProvider to the map + // so we don't create and add any duplicate graphNodeCloseProviders. + m[p] = source + } + + g.Connect(dag.BasicEdge(source, v)) + } + } + } + + return nil +} + // MissingProviderTransformer is a GraphTransformer that adds nodes // for missing providers into the graph. Specifically, it creates provider // configuration nodes for all the providers that we support. These are @@ -143,6 +181,28 @@ func (t *PruneProviderTransformer) Transform(g *Graph) error { return nil } +func providerVertexMap(g *Graph) map[string]dag.Vertex { + m := make(map[string]dag.Vertex) + for _, v := range g.Vertices() { + if pv, ok := v.(GraphNodeProvider); ok { + m[pv.ProviderName()] = v + } + } + + return m +} + +func closeProviderVertexMap(g *Graph) map[string]dag.Vertex { + m := make(map[string]dag.Vertex) + for _, v := range g.Vertices() { + if pv, ok := v.(GraphNodeCloseProvider); ok { + m[pv.CloseProviderName()] = v + } + } + + return m +} + type graphNodeDisabledProvider struct { GraphNodeProvider } @@ -258,6 +318,94 @@ func (n *graphNodeDisabledProviderFlat) DependentOn() []string { return result } +type graphNodeCloseProvider struct { + ProviderNameValue string +} + +func (n *graphNodeCloseProvider) Name() string { + return fmt.Sprintf("provider.%s (close)", n.ProviderNameValue) +} + +// GraphNodeEvalable impl. +func (n *graphNodeCloseProvider) EvalTree() EvalNode { + return CloseProviderEvalTree(n.ProviderNameValue) +} + +// GraphNodeDependable impl. +func (n *graphNodeCloseProvider) DependableName() []string { + return []string{n.Name()} +} + +func (n *graphNodeCloseProvider) CloseProviderName() string { + return n.ProviderNameValue +} + +// GraphNodeDotter impl. +func (n *graphNodeCloseProvider) DotNode(name string, opts *GraphDotOpts) *dot.Node { + return dot.NewNode(name, map[string]string{ + "label": n.Name(), + "shape": "diamond", + }) +} + +// GraphNodeDotterOrigin impl. +func (n *graphNodeCloseProvider) DotOrigin() bool { + return true +} + +// 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 } @@ -305,17 +453,6 @@ func (n *graphNodeMissingProvider) Flatten(p []string) (dag.Vertex, error) { }, nil } -func providerVertexMap(g *Graph) map[string]dag.Vertex { - m := make(map[string]dag.Vertex) - for _, v := range g.Vertices() { - if pv, ok := v.(GraphNodeProvider); ok { - m[pv.ProviderName()] = v - } - } - - return m -} - // Same as graphNodeMissingProvider, but for flattening type graphNodeMissingProviderFlat struct { *graphNodeMissingProvider diff --git a/terraform/transform_provider_test.go b/terraform/transform_provider_test.go index fdf764c3f9..63ddfba0eb 100644 --- a/terraform/transform_provider_test.go +++ b/terraform/transform_provider_test.go @@ -30,6 +30,38 @@ func TestProviderTransformer(t *testing.T) { } } +func TestCloseProviderTransformer(t *testing.T) { + mod := testModule(t, "transform-provider-basic") + + g := Graph{Path: RootModulePath} + { + tf := &ConfigTransformer{Module: mod} + if err := tf.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) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformCloseProviderBasicStr) + if actual != expected { + t.Fatalf("bad:\n\n%s", actual) + } +} + func TestMissingProviderTransformer(t *testing.T) { mod := testModule(t, "transform-provider-basic") @@ -41,9 +73,18 @@ func TestMissingProviderTransformer(t *testing.T) { } } - transform := &MissingProviderTransformer{Providers: []string{"foo"}} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) + { + transform := &MissingProviderTransformer{Providers: []string{"foo"}} + 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) + } } actual := strings.TrimSpace(g.String()) @@ -78,6 +119,13 @@ func TestPruneProviderTransformer(t *testing.T) { } } + { + transform := &CloseProviderTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + { transform := &PruneProviderTransformer{} if err := transform.Transform(&g); err != nil { @@ -117,6 +165,13 @@ func TestDisableProviderTransformer(t *testing.T) { } } + { + transform := &CloseProviderTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + { transform := &PruneProviderTransformer{} if err := transform.Transform(&g); err != nil { @@ -163,6 +218,13 @@ func TestDisableProviderTransformer_keep(t *testing.T) { } } + { + transform := &CloseProviderTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + { transform := &PruneProviderTransformer{} if err := transform.Transform(&g); err != nil { @@ -203,9 +265,19 @@ aws_instance.web provider.aws ` +const testTransformCloseProviderBasicStr = ` +aws_instance.web + provider.aws +provider.aws +provider.aws (close) + aws_instance.web +` + const testTransformMissingProviderBasicStr = ` aws_instance.web provider.aws +provider.aws (close) + aws_instance.web provider.foo ` @@ -213,12 +285,16 @@ const testTransformPruneProviderBasicStr = ` foo_instance.web provider.foo provider.foo +provider.foo (close) + foo_instance.web ` const testTransformDisableProviderBasicStr = ` module.child provider.aws (disabled) var.foo +provider.aws (close) + module.child provider.aws (disabled) var.foo ` @@ -230,5 +306,8 @@ module.child provider.aws var.foo provider.aws +provider.aws (close) + aws_instance.foo + module.child var.foo ` diff --git a/terraform/transform_provisioner.go b/terraform/transform_provisioner.go index bf661f88a4..a1107a1214 100644 --- a/terraform/transform_provisioner.go +++ b/terraform/transform_provisioner.go @@ -14,6 +14,13 @@ type GraphNodeProvisioner interface { ProvisionerName() string } +// GraphNodeCloseProvisioner is an interface that nodes that can be a close +// provisioner must implement. The CloseProvisionerName returned is the name +// of the provisioner they satisfy. +type GraphNodeCloseProvisioner interface { + CloseProvisionerName() string +} + // GraphNodeProvisionerConsumer is an interface that nodes that require // a provisioner must implement. ProvisionedBy must return the name of the // provisioner to use. @@ -49,6 +56,37 @@ func (t *ProvisionerTransformer) Transform(g *Graph) error { return err } +// CloseProvisionerTransformer is a GraphTransformer that adds nodes to the +// graph that will close open provisioner connections that aren't needed +// anymore. A provisioner connection is not needed anymore once all depended +// resources in the graph are evaluated. +type CloseProvisionerTransformer struct{} + +func (t *CloseProvisionerTransformer) Transform(g *Graph) error { + m := closeProvisionerVertexMap(g) + for _, v := range g.Vertices() { + if pv, ok := v.(GraphNodeProvisionerConsumer); ok { + for _, p := range pv.ProvisionedBy() { + source := m[p] + + if source == nil { + // Create a new graphNodeCloseProvisioner and add it to the graph + source = &graphNodeCloseProvisioner{ProvisionerNameValue: p} + g.Add(source) + + // Make sure we also add the new graphNodeCloseProvisioner to the map + // so we don't create and add any duplicate graphNodeCloseProvisioners. + m[p] = source + } + + g.Connect(dag.BasicEdge(source, v)) + } + } + } + + return nil +} + // MissingProvisionerTransformer is a GraphTransformer that adds nodes // for missing provisioners into the graph. Specifically, it creates provisioner // configuration nodes for all the provisioners that we support. These are @@ -94,6 +132,75 @@ func (t *PruneProvisionerTransformer) Transform(g *Graph) error { return nil } +func provisionerVertexMap(g *Graph) map[string]dag.Vertex { + m := make(map[string]dag.Vertex) + for _, v := range g.Vertices() { + if pv, ok := v.(GraphNodeProvisioner); ok { + m[pv.ProvisionerName()] = v + } + } + + return m +} + +func closeProvisionerVertexMap(g *Graph) map[string]dag.Vertex { + m := make(map[string]dag.Vertex) + for _, v := range g.Vertices() { + if pv, ok := v.(GraphNodeCloseProvisioner); ok { + m[pv.CloseProvisionerName()] = v + } + } + + return m +} + +type graphNodeCloseProvisioner struct { + ProvisionerNameValue string +} + +func (n *graphNodeCloseProvisioner) Name() string { + return fmt.Sprintf("provisioner.%s (close)", n.ProvisionerNameValue) +} + +// GraphNodeEvalable impl. +func (n *graphNodeCloseProvisioner) EvalTree() EvalNode { + return &EvalCloseProvisioner{Name: n.ProvisionerNameValue} +} + +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 } @@ -119,17 +226,6 @@ func (n *graphNodeMissingProvisioner) Flatten(p []string) (dag.Vertex, error) { }, nil } -func provisionerVertexMap(g *Graph) map[string]dag.Vertex { - m := make(map[string]dag.Vertex) - for _, v := range g.Vertices() { - if pv, ok := v.(GraphNodeProvisioner); ok { - m[pv.ProvisionerName()] = v - } - } - - return m -} - // Same as graphNodeMissingProvisioner, but for flattening type graphNodeMissingProvisionerFlat struct { *graphNodeMissingProvisioner diff --git a/terraform/transform_provisioner_test.go b/terraform/transform_provisioner_test.go index f38a0a0452..ea5faff770 100644 --- a/terraform/transform_provisioner_test.go +++ b/terraform/transform_provisioner_test.go @@ -18,9 +18,18 @@ func TestMissingProvisionerTransformer(t *testing.T) { } } - transform := &MissingProvisionerTransformer{Provisioners: []string{"foo"}} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) + { + transform := &MissingProvisionerTransformer{Provisioners: []string{"foo"}} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &CloseProvisionerTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } } actual := strings.TrimSpace(g.String()) @@ -56,6 +65,13 @@ func TestPruneProvisionerTransformer(t *testing.T) { } } + { + transform := &CloseProvisionerTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + { transform := &PruneProvisionerTransformer{} if err := transform.Transform(&g); err != nil { @@ -86,10 +102,14 @@ func TestGraphNodeMissingProvisioner_ProvisionerName(t *testing.T) { const testTransformMissingProvisionerBasicStr = ` aws_instance.web provisioner.foo +provisioner.shell (close) + aws_instance.web ` const testTransformPruneProvisionerBasicStr = ` aws_instance.web provisioner.foo provisioner.foo +provisioner.foo (close) + aws_instance.web `