From 44daf76624c6445ff82d0634b1e00091b579db68 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Jan 2021 12:11:43 -0500 Subject: [PATCH 1/7] remove InitProvisioner and close all at once InitProvisioner only stored the provisioner in the cache and provided a way to return an error. We can push this back further into the cli init process, but for now we can move the error into the Provisioner call and drop InitProvisioner. This also changes CloseProvisioner to CloseProvisioners, in preparation for the removal of the graph nodes. --- terraform/eval_context.go | 11 +++----- terraform/eval_context_builtin.go | 45 +++++++++++++------------------ terraform/eval_context_mock.go | 24 ++++------------- 3 files changed, 27 insertions(+), 53 deletions(-) diff --git a/terraform/eval_context.go b/terraform/eval_context.go index 6c8a5d9d1..7421d69cb 100644 --- a/terraform/eval_context.go +++ b/terraform/eval_context.go @@ -77,22 +77,17 @@ type EvalContext interface { ProviderInput(addrs.AbsProviderConfig) map[string]cty.Value SetProviderInput(addrs.AbsProviderConfig, map[string]cty.Value) - // InitProvisioner initializes the provisioner with the given name. - // It is an error to initialize the same provisioner more than once. - InitProvisioner(string) error - // Provisioner gets the provisioner instance with the given name (already // initialized) or returns nil if the provisioner isn't initialized. - Provisioner(string) provisioners.Interface + Provisioner(string) (provisioners.Interface, error) // ProvisionerSchema retrieves the main configuration schema for a // particular provisioner, which must have already been initialized with // InitProvisioner. ProvisionerSchema(string) *configschema.Block - // CloseProvisioner closes provisioner connections that aren't needed - // anymore. - CloseProvisioner(string) error + // CloseProvisioner closes all provisioner plugins. + CloseProvisioners() error // EvaluateBlock takes the given raw configuration block and associated // schema and evaluates it to produce a value of an object type that diff --git a/terraform/eval_context_builtin.go b/terraform/eval_context_builtin.go index 97d2ff6ef..d477c00c1 100644 --- a/terraform/eval_context_builtin.go +++ b/terraform/eval_context_builtin.go @@ -228,48 +228,41 @@ func (ctx *BuiltinEvalContext) SetProviderInput(pc addrs.AbsProviderConfig, c ma ctx.ProviderLock.Unlock() } -func (ctx *BuiltinEvalContext) InitProvisioner(n string) error { - // If we already initialized, it is an error - if p := ctx.Provisioner(n); p != nil { - return fmt.Errorf("Provisioner '%s' already initialized", n) - } - - // Warning: make sure to acquire these locks AFTER the call to Provisioner - // above, since it also acquires locks. +func (ctx *BuiltinEvalContext) Provisioner(n string) (provisioners.Interface, error) { ctx.ProvisionerLock.Lock() defer ctx.ProvisionerLock.Unlock() - p, err := ctx.Components.ResourceProvisioner(n) - if err != nil { - return err + p, ok := ctx.ProvisionerCache[n] + if !ok { + var err error + p, err = ctx.Components.ResourceProvisioner(n) + if err != nil { + return nil, err + } + + ctx.ProvisionerCache[n] = p } - ctx.ProvisionerCache[n] = p - - return nil -} - -func (ctx *BuiltinEvalContext) Provisioner(n string) provisioners.Interface { - ctx.ProvisionerLock.Lock() - defer ctx.ProvisionerLock.Unlock() - - return ctx.ProvisionerCache[n] + return p, nil } func (ctx *BuiltinEvalContext) ProvisionerSchema(n string) *configschema.Block { return ctx.Schemas.ProvisionerConfig(n) } -func (ctx *BuiltinEvalContext) CloseProvisioner(n string) error { +func (ctx *BuiltinEvalContext) CloseProvisioners() error { + var diags tfdiags.Diagnostics ctx.ProvisionerLock.Lock() defer ctx.ProvisionerLock.Unlock() - prov := ctx.ProvisionerCache[n] - if prov != nil { - return prov.Close() + for name, prov := range ctx.ProvisionerCache { + err := prov.Close() + if err != nil { + diags = diags.Append(fmt.Errorf("provisioner.Close %s: %s", name, err)) + } } - return nil + return diags.Err() } func (ctx *BuiltinEvalContext) EvaluateBlock(body hcl.Body, schema *configschema.Block, self addrs.Referenceable, keyData InstanceKeyEvalData) (cty.Value, hcl.Body, tfdiags.Diagnostics) { diff --git a/terraform/eval_context_mock.go b/terraform/eval_context_mock.go index ca8b84a6a..aa25f75f9 100644 --- a/terraform/eval_context_mock.go +++ b/terraform/eval_context_mock.go @@ -63,11 +63,6 @@ type MockEvalContext struct { ConfigureProviderConfig cty.Value ConfigureProviderDiags tfdiags.Diagnostics - InitProvisionerCalled bool - InitProvisionerName string - InitProvisionerProvisioner provisioners.Interface - InitProvisionerError error - ProvisionerCalled bool ProvisionerName string ProvisionerProvisioner provisioners.Interface @@ -76,9 +71,7 @@ type MockEvalContext struct { ProvisionerSchemaName string ProvisionerSchemaSchema *configschema.Block - CloseProvisionerCalled bool - CloseProvisionerName string - CloseProvisionerProvisioner provisioners.Interface + CloseProvisionersCalled bool EvaluateBlockCalled bool EvaluateBlockBody hcl.Body @@ -208,16 +201,10 @@ func (c *MockEvalContext) SetProviderInput(addr addrs.AbsProviderConfig, vals ma c.SetProviderInputValues = vals } -func (c *MockEvalContext) InitProvisioner(n string) error { - c.InitProvisionerCalled = true - c.InitProvisionerName = n - return c.InitProvisionerError -} - -func (c *MockEvalContext) Provisioner(n string) provisioners.Interface { +func (c *MockEvalContext) Provisioner(n string) (provisioners.Interface, error) { c.ProvisionerCalled = true c.ProvisionerName = n - return c.ProvisionerProvisioner + return c.ProvisionerProvisioner, nil } func (c *MockEvalContext) ProvisionerSchema(n string) *configschema.Block { @@ -226,9 +213,8 @@ func (c *MockEvalContext) ProvisionerSchema(n string) *configschema.Block { return c.ProvisionerSchemaSchema } -func (c *MockEvalContext) CloseProvisioner(n string) error { - c.CloseProvisionerCalled = true - c.CloseProvisionerName = n +func (c *MockEvalContext) CloseProvisioners() error { + c.CloseProvisionersCalled = true return nil } From a6db207b8772cc874723b4d8100c9e19a51a8a3e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 15 Jan 2021 16:53:45 -0500 Subject: [PATCH 2/7] remove provisioner graph nodes Provisioners are always run via a single instance, and their configuration and references are taken care of via their parent resource, so there is no need to maintain the provisioner graph nodes. The only action that will still need to be completed is calling the Close method for external plugins. This was not consistently done with the current handling of provisioners anyway, and we can now add that to a single point with the new CloseProvisioners context method. --- terraform/graph_builder_apply.go | 5 - terraform/graph_builder_apply_test.go | 69 ------- terraform/graph_builder_plan.go | 4 - terraform/node_provisioner.go | 45 ---- terraform/node_resource_abstract_instance.go | 7 +- terraform/node_resource_validate.go | 7 +- terraform/transform_provisioner.go | 174 ---------------- terraform/transform_provisioner_test.go | 203 ------------------- 8 files changed, 12 insertions(+), 502 deletions(-) delete mode 100644 terraform/node_provisioner.go delete mode 100644 terraform/transform_provisioner_test.go diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 5501319d8..b5e50e772 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -108,10 +108,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Attach the configuration to any resources &AttachResourceConfigTransformer{Config: b.Config}, - // Provisioner-related transformations - &MissingProvisionerTransformer{Provisioners: b.Components.ResourceProvisioners()}, - &ProvisionerTransformer{}, - // add providers TransformProviders(b.Components.ResourceProviders(), concreteProvider, b.Config), @@ -162,7 +158,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Close opened plugin connections &CloseProviderTransformer{}, - &CloseProvisionerTransformer{}, // close the root module &CloseRootModuleTransformer{}, diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 3e1f6e29c..cf7076911 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -423,70 +423,6 @@ func TestApplyGraphBuilder_moduleDestroy(t *testing.T) { ) } -func TestApplyGraphBuilder_provisioner(t *testing.T) { - changes := &plans.Changes{ - Resources: []*plans.ResourceInstanceChangeSrc{ - { - Addr: mustResourceInstanceAddr("test_object.foo"), - ChangeSrc: plans.ChangeSrc{ - Action: plans.Create, - }, - }, - }, - } - - b := &ApplyGraphBuilder{ - Config: testModule(t, "graph-builder-apply-provisioner"), - Changes: changes, - Components: simpleMockComponentFactory(), - Schemas: simpleTestSchemas(), - } - - g, err := b.Build(addrs.RootModuleInstance) - if err != nil { - t.Fatalf("err: %s", err) - } - - testGraphContains(t, g, "provisioner.test") - testGraphHappensBefore( - t, g, - "provisioner.test", - "test_object.foo", - ) -} - -func TestApplyGraphBuilder_provisionerDestroy(t *testing.T) { - changes := &plans.Changes{ - Resources: []*plans.ResourceInstanceChangeSrc{ - { - Addr: mustResourceInstanceAddr("test_object.foo"), - ChangeSrc: plans.ChangeSrc{ - Action: plans.Delete, - }, - }, - }, - } - - b := &ApplyGraphBuilder{ - Config: testModule(t, "graph-builder-apply-provisioner"), - Changes: changes, - Components: simpleMockComponentFactory(), - Schemas: simpleTestSchemas(), - } - - g, err := b.Build(addrs.RootModuleInstance) - if err != nil { - t.Fatalf("err: %s", err) - } - - testGraphContains(t, g, "provisioner.test") - testGraphHappensBefore( - t, g, - "provisioner.test", - "test_object.foo (destroy)", - ) -} - func TestApplyGraphBuilder_targetModule(t *testing.T) { changes := &plans.Changes{ Resources: []*plans.ResourceInstanceChangeSrc{ @@ -784,7 +720,6 @@ module.child.test_object.create module.child.test_object.create (expand) module.child (expand) provider["registry.terraform.io/hashicorp/test"] - provisioner.test module.child.test_object.other module.child.test_object.create module.child.test_object.other (expand) @@ -795,13 +730,9 @@ provider["registry.terraform.io/hashicorp/test"] provider["registry.terraform.io/hashicorp/test"] (close) module.child.test_object.other test_object.other -provisioner.test -provisioner.test (close) - module.child.test_object.create root meta.count-boundary (EachMode fixup) provider["registry.terraform.io/hashicorp/test"] (close) - provisioner.test (close) test_object.create test_object.create (expand) test_object.create (expand) diff --git a/terraform/graph_builder_plan.go b/terraform/graph_builder_plan.go index 49184c2e2..2e3a35ccb 100644 --- a/terraform/graph_builder_plan.go +++ b/terraform/graph_builder_plan.go @@ -115,10 +115,6 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { // Attach the configuration to any resources &AttachResourceConfigTransformer{Config: b.Config}, - // Provisioner-related transformations - &MissingProvisionerTransformer{Provisioners: b.Components.ResourceProvisioners()}, - &ProvisionerTransformer{}, - // add providers TransformProviders(b.Components.ResourceProviders(), b.ConcreteProvider, b.Config), diff --git a/terraform/node_provisioner.go b/terraform/node_provisioner.go deleted file mode 100644 index eaa6fc815..000000000 --- a/terraform/node_provisioner.go +++ /dev/null @@ -1,45 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/tfdiags" -) - -// NodeProvisioner represents a provider that has no associated operations. -// It registers all the common interfaces across operations for providers. -type NodeProvisioner struct { - NameValue string - PathValue addrs.ModuleInstance -} - -var ( - _ GraphNodeModuleInstance = (*NodeProvisioner)(nil) - _ GraphNodeProvisioner = (*NodeProvisioner)(nil) - _ GraphNodeExecutable = (*NodeProvisioner)(nil) -) - -func (n *NodeProvisioner) Name() string { - result := fmt.Sprintf("provisioner.%s", n.NameValue) - if len(n.PathValue) > 0 { - result = fmt.Sprintf("%s.%s", n.PathValue.String(), result) - } - - return result -} - -// GraphNodeModuleInstance -func (n *NodeProvisioner) Path() addrs.ModuleInstance { - return n.PathValue -} - -// GraphNodeProvisioner -func (n *NodeProvisioner) ProvisionerName() string { - return n.NameValue -} - -// GraphNodeExecutable impl. -func (n *NodeProvisioner) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { - return diags.Append(ctx.InitProvisioner(n.NameValue)) -} diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index e02b0b79c..1393d1abe 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -1638,7 +1638,12 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state log.Printf("[TRACE] applyProvisioners: provisioning %s with %q", n.Addr, prov.Type) // Get the provisioner - provisioner := ctx.Provisioner(prov.Type) + provisioner, err := ctx.Provisioner(prov.Type) + if err != nil { + diags = diags.Append(err) + return diags.Err() + } + schema := ctx.ProvisionerSchema(prov.Type) config, configDiags := evalScope(ctx, prov.Config, self, schema) diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index 83327d894..933cb43d6 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -67,7 +67,12 @@ func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (di func (n *NodeValidatableResource) validateProvisioner(ctx EvalContext, p *configs.Provisioner, hasCount, hasForEach bool) tfdiags.Diagnostics { var diags tfdiags.Diagnostics - provisioner := ctx.Provisioner(p.Type) + provisioner, err := ctx.Provisioner(p.Type) + if err != nil { + diags = diags.Append(err) + return diags + } + if provisioner == nil { return diags.Append(fmt.Errorf("provisioner %s not initialized", p.Type)) } diff --git a/terraform/transform_provisioner.go b/terraform/transform_provisioner.go index 80bc25f26..38e3a8ed7 100644 --- a/terraform/transform_provisioner.go +++ b/terraform/transform_provisioner.go @@ -1,182 +1,8 @@ package terraform -import ( - "fmt" - "log" - - "github.com/hashicorp/go-multierror" - "github.com/hashicorp/terraform/dag" - "github.com/hashicorp/terraform/tfdiags" -) - -// GraphNodeProvisioner is an interface that nodes that can be a provisioner -// must implement. The ProvisionerName returned is the name of the provisioner -// they satisfy. -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 names of the // provisioners to use. type GraphNodeProvisionerConsumer interface { ProvisionedBy() []string } - -// ProvisionerTransformer is a GraphTransformer that maps resources to -// provisioners within the graph. This will error if there are any resources -// that don't map to proper resources. -type ProvisionerTransformer struct{} - -func (t *ProvisionerTransformer) Transform(g *Graph) error { - // Go through the other nodes and match them to provisioners they need - var err error - m := provisionerVertexMap(g) - for _, v := range g.Vertices() { - if pv, ok := v.(GraphNodeProvisionerConsumer); ok { - for _, p := range pv.ProvisionedBy() { - if m[p] == nil { - err = multierror.Append(err, fmt.Errorf( - "%s: provisioner %s couldn't be found", - dag.VertexName(v), p)) - continue - } - - log.Printf("[TRACE] ProvisionerTransformer: %s is provisioned by %s (%q)", dag.VertexName(v), p, dag.VertexName(m[p])) - g.Connect(dag.BasicEdge(v, m[p])) - } - } - } - - return err -} - -// MissingProvisionerTransformer is a GraphTransformer that adds nodes -// for missing provisioners into the graph. -type MissingProvisionerTransformer struct { - // Provisioners is the list of provisioners we support. - Provisioners []string -} - -func (t *MissingProvisionerTransformer) Transform(g *Graph) error { - // Create a set of our supported provisioners - supported := make(map[string]struct{}, len(t.Provisioners)) - for _, v := range t.Provisioners { - supported[v] = struct{}{} - } - - // Get the map of provisioners we already have in our graph - m := provisionerVertexMap(g) - - // Go through all the provisioner consumers and make sure we add - // that provisioner if it is missing. - for _, v := range g.Vertices() { - pv, ok := v.(GraphNodeProvisionerConsumer) - if !ok { - continue - } - - for _, p := range pv.ProvisionedBy() { - if _, ok := m[p]; ok { - // This provisioner already exists as a configure node - continue - } - - if _, ok := supported[p]; !ok { - // If we don't support the provisioner type, we skip it. - // Validation later will catch this as an error. - continue - } - - // Build the vertex - var newV dag.Vertex = &NodeProvisioner{ - NameValue: p, - } - - // Add the missing provisioner node to the graph - m[p] = g.Add(newV) - log.Printf("[TRACE] MissingProviderTransformer: added implicit provisioner %s, first implied by %s", p, dag.VertexName(v)) - } - } - - return nil -} - -// 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 -} - -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 -} - -var _ GraphNodeExecutable = (*graphNodeCloseProvisioner)(nil) - -func (n *graphNodeCloseProvisioner) Name() string { - return fmt.Sprintf("provisioner.%s (close)", n.ProvisionerNameValue) -} - -// GraphNodeExecutable impl. -func (n *graphNodeCloseProvisioner) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { - return diags.Append(ctx.CloseProvisioner(n.ProvisionerNameValue)) -} - -func (n *graphNodeCloseProvisioner) CloseProvisionerName() string { - return n.ProvisionerNameValue -} diff --git a/terraform/transform_provisioner_test.go b/terraform/transform_provisioner_test.go deleted file mode 100644 index a6da10afc..000000000 --- a/terraform/transform_provisioner_test.go +++ /dev/null @@ -1,203 +0,0 @@ -package terraform - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/dag" - "github.com/hashicorp/terraform/states" -) - -func TestMissingProvisionerTransformer(t *testing.T) { - mod := testModule(t, "transform-provisioner-basic") - - g := Graph{Path: addrs.RootModuleInstance} - { - tf := &ConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &AttachResourceConfigTransformer{Config: 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(testTransformMissingProvisionerBasicStr) - if actual != expected { - t.Fatalf("bad:\n\n%s", actual) - } -} - -func TestMissingProvisionerTransformer_module(t *testing.T) { - mod := testModule(t, "transform-provisioner-module") - - g := Graph{Path: addrs.RootModuleInstance} - { - concreteResource := func(a *NodeAbstractResourceInstance) dag.Vertex { - return a - } - - state := states.BuildState(func(s *states.SyncState) { - s.SetResourceInstanceCurrent( - addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "aws_instance", - Name: "foo", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), - &states.ResourceInstanceObjectSrc{ - AttrsFlat: map[string]string{ - "id": "foo", - }, - Status: states.ObjectReady, - }, - addrs.AbsProviderConfig{ - Provider: addrs.NewDefaultProvider("aws"), - Module: addrs.RootModule, - }, - ) - s.SetResourceInstanceCurrent( - addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "aws_instance", - Name: "foo", - }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance.Child("child", addrs.NoKey)), - &states.ResourceInstanceObjectSrc{ - AttrsFlat: map[string]string{ - "id": "foo", - }, - Status: states.ObjectReady, - }, - addrs.AbsProviderConfig{ - Provider: addrs.NewDefaultProvider("aws"), - Module: addrs.RootModule, - }, - ) - }) - - tf := &StateTransformer{ - ConcreteCurrent: concreteResource, - State: state, - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - t.Logf("graph after StateTransformer:\n%s", g.StringWithNodeTypes()) - } - - { - transform := &AttachResourceConfigTransformer{Config: 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) - } - t.Logf("graph after MissingProvisionerTransformer:\n%s", g.StringWithNodeTypes()) - } - - { - transform := &ProvisionerTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - t.Logf("graph after ProvisionerTransformer:\n%s", g.StringWithNodeTypes()) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformMissingProvisionerModuleStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - } -} - -func TestCloseProvisionerTransformer(t *testing.T) { - mod := testModule(t, "transform-provisioner-basic") - - g := Graph{Path: addrs.RootModuleInstance} - { - tf := &ConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - transform := &AttachResourceConfigTransformer{Config: 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) - } - } - - { - transform := &CloseProvisionerTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformCloseProvisionerBasicStr) - if actual != expected { - t.Fatalf("bad:\n\n%s", actual) - } -} - -const testTransformMissingProvisionerBasicStr = ` -aws_instance.web - provisioner.shell -provisioner.shell -` - -const testTransformMissingProvisionerModuleStr = ` -aws_instance.foo - provisioner.shell -module.child.aws_instance.foo - provisioner.shell -provisioner.shell -` - -const testTransformCloseProvisionerBasicStr = ` -aws_instance.web - provisioner.shell -provisioner.shell -provisioner.shell (close) - aws_instance.web -` From fe7635f438c15458a88f95fdb80c0874c93f8f36 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Jan 2021 12:58:00 -0500 Subject: [PATCH 3/7] always close all provisioners after each walk This was not done consistently in all cases when the individual provisioner graph nodes were used. Since we removed the graph nodes, the only thing left is to close these at the end of the walk. --- terraform/node_module_expand.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 2fbd77426..92064f6e4 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -140,7 +140,9 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd // wait on this node. // Besides providing a root node for dependency ordering, nodeCloseModule also // cleans up state after all the module nodes have been evaluated, removing -// empty resources and modules from the state. +// empty resources and modules from the state, and closes a remaining +// provisioner plugins which do not have a lifecycle controlled controlled by +// individual graph nodes. type nodeCloseModule struct { Addr addrs.Module } @@ -174,6 +176,12 @@ func (n *nodeCloseModule) Name() string { } func (n *nodeCloseModule) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { + if n.Addr.IsRoot() { + // If this is the root module, we are cleaning up the walk, so close + // any running provisioners + diags = diags.Append(ctx.CloseProvisioners()) + } + switch op { case walkApply, walkDestroy: state := ctx.State().Lock() From 63a9ab4944c4815e1075080c491e3b8369657576 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Jan 2021 16:59:22 -0500 Subject: [PATCH 4/7] ensure that provisioners are not used after Close --- terraform/context_apply_test.go | 23 +++++++++++++++-------- terraform/terraform_test.go | 4 +++- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f1da45c61..537bd289b 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9801,14 +9801,17 @@ func TestContext2Apply_plannedConnectionRefs(t *testing.T) { return resp } - pr := testProvisioner() - pr.ProvisionResourceFn = func(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { - host := req.Connection.GetAttr("host") - if host.IsNull() || !host.IsKnown() { - resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("invalid host value: %#v", host)) - } + provisionerFactory := func() (provisioners.Interface, error) { + pr := testProvisioner() + pr.ProvisionResourceFn = func(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { + host := req.Connection.GetAttr("host") + if host.IsNull() || !host.IsKnown() { + resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("invalid host value: %#v", host)) + } - return resp + return resp + } + return pr, nil } Providers := map[addrs.Provider]providers.Factory{ @@ -9816,7 +9819,7 @@ func TestContext2Apply_plannedConnectionRefs(t *testing.T) { } provisioners := map[string]provisioners.Factory{ - "shell": testProvisionerFuncFixed(pr), + "shell": provisionerFactory, } hook := &testHook{} @@ -12163,6 +12166,7 @@ output "out" { func TestContext2Apply_provisionerSensitive(t *testing.T) { m := testModule(t, "apply-provisioner-sensitive") p := testProvider("aws") + pr := testProvisioner() pr.ProvisionResourceFn = func(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { if req.Config.ContainsMarked() { @@ -12201,6 +12205,9 @@ func TestContext2Apply_provisionerSensitive(t *testing.T) { t.Fatal("plan failed") } + // "restart" provisioner + pr.CloseCalled = false + state, diags := ctx.Apply() if diags.HasErrors() { logDiagnostics(t, diags) diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 018858676..ffca0ff7b 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -170,8 +170,10 @@ func testProviderFuncFixed(rp providers.Interface) providers.Factory { } } -func testProvisionerFuncFixed(rp provisioners.Interface) provisioners.Factory { +func testProvisionerFuncFixed(rp *MockProvisioner) provisioners.Factory { return func() (provisioners.Interface, error) { + // make sure this provisioner has has not been closed + rp.CloseCalled = false return rp, nil } } From ff30030e4f6b447c033167e97fd8529f4ea66140 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Jan 2021 17:29:46 -0500 Subject: [PATCH 5/7] builtin provisioners are re-used and not Closed --- builtin/provisioners/file/resource_provisioner.go | 1 - builtin/provisioners/local-exec/resource_provisioner.go | 1 - builtin/provisioners/remote-exec/resource_provisioner.go | 1 - 3 files changed, 3 deletions(-) diff --git a/builtin/provisioners/file/resource_provisioner.go b/builtin/provisioners/file/resource_provisioner.go index 2625d6643..b96c03121 100644 --- a/builtin/provisioners/file/resource_provisioner.go +++ b/builtin/provisioners/file/resource_provisioner.go @@ -181,6 +181,5 @@ func (p *provisioner) Stop() error { } func (p *provisioner) Close() error { - p.cancel() return nil } diff --git a/builtin/provisioners/local-exec/resource_provisioner.go b/builtin/provisioners/local-exec/resource_provisioner.go index fb58ad254..f3225ff91 100644 --- a/builtin/provisioners/local-exec/resource_provisioner.go +++ b/builtin/provisioners/local-exec/resource_provisioner.go @@ -181,7 +181,6 @@ func (p *provisioner) Stop() error { } func (p *provisioner) Close() error { - p.cancel() return nil } diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go index 088584ce5..4311ba164 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/builtin/provisioners/remote-exec/resource_provisioner.go @@ -116,7 +116,6 @@ func (p *provisioner) Stop() error { } func (p *provisioner) Close() error { - p.cancel() return nil } From 21896f74afe6d7e8fe8f92d9bb9537a5b74736ce Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Jan 2021 17:33:41 -0500 Subject: [PATCH 6/7] builtin provisioner e2e test --- command/e2etest/provisioner_test.go | 44 ++++++++++++++++++++ command/e2etest/testdata/provisioner/main.tf | 5 +++ 2 files changed, 49 insertions(+) create mode 100644 command/e2etest/provisioner_test.go create mode 100644 command/e2etest/testdata/provisioner/main.tf diff --git a/command/e2etest/provisioner_test.go b/command/e2etest/provisioner_test.go new file mode 100644 index 000000000..39bfa3757 --- /dev/null +++ b/command/e2etest/provisioner_test.go @@ -0,0 +1,44 @@ +package e2etest + +import ( + "strings" + "testing" + + "github.com/hashicorp/terraform/e2e" +) + +// TestProviderDevOverrides is a test that terraform can execute a 3rd party +// provisioner plugin. +func TestProvisioner(t *testing.T) { + t.Parallel() + + // This test reaches out to releases.hashicorp.com to download the + // template and null providers, so it can only run if network access is + // allowed. + skipIfCannotAccessNetwork(t) + + tf := e2e.NewBinary(terraformBin, "testdata/provisioner") + defer tf.Close() + + //// INIT + _, stderr, err := tf.Run("init") + if err != nil { + t.Fatalf("unexpected init error: %s\nstderr:\n%s", err, stderr) + } + + //// PLAN + _, stderr, err = tf.Run("plan", "-out=tfplan") + if err != nil { + t.Fatalf("unexpected plan error: %s\nstderr:\n%s", err, stderr) + } + + //// APPLY + stdout, stderr, err := tf.Run("apply", "tfplan") + if err != nil { + t.Fatalf("unexpected apply error: %s\nstderr:\n%s", err, stderr) + } + + if !strings.Contains(stdout, "HelloProvisioner") { + t.Fatalf("missing provisioner output:\n%s", stdout) + } +} diff --git a/command/e2etest/testdata/provisioner/main.tf b/command/e2etest/testdata/provisioner/main.tf new file mode 100644 index 000000000..c37ad380b --- /dev/null +++ b/command/e2etest/testdata/provisioner/main.tf @@ -0,0 +1,5 @@ +resource "null_resource" "a" { + provisioner "local-exec" { + command = "echo HelloProvisioner" + } +} From 439d06b29f4182c3505ca1363ced65040a176fae Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 Jan 2021 10:17:29 -0500 Subject: [PATCH 7/7] comment updates --- terraform/eval_context.go | 3 +-- terraform/node_module_expand.go | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/terraform/eval_context.go b/terraform/eval_context.go index 7421d69cb..49cac746a 100644 --- a/terraform/eval_context.go +++ b/terraform/eval_context.go @@ -77,8 +77,7 @@ type EvalContext interface { ProviderInput(addrs.AbsProviderConfig) map[string]cty.Value SetProviderInput(addrs.AbsProviderConfig, map[string]cty.Value) - // Provisioner gets the provisioner instance with the given name (already - // initialized) or returns nil if the provisioner isn't initialized. + // Provisioner gets the provisioner instance with the given name. Provisioner(string) (provisioners.Interface, error) // ProvisionerSchema retrieves the main configuration schema for a diff --git a/terraform/node_module_expand.go b/terraform/node_module_expand.go index 92064f6e4..07ff4545c 100644 --- a/terraform/node_module_expand.go +++ b/terraform/node_module_expand.go @@ -140,9 +140,9 @@ func (n *nodeExpandModule) Execute(ctx EvalContext, op walkOperation) (diags tfd // wait on this node. // Besides providing a root node for dependency ordering, nodeCloseModule also // cleans up state after all the module nodes have been evaluated, removing -// empty resources and modules from the state, and closes a remaining -// provisioner plugins which do not have a lifecycle controlled controlled by -// individual graph nodes. +// empty resources and modules from the state. +// The root module instance also closes any remaining provisioner plugins which +// do not have a lifecycle controlled by individual graph nodes. type nodeCloseModule struct { Addr addrs.Module }