Merge pull request #27450 from hashicorp/jbardin/internal-provisioner-panics

provisioners: ensure context cancel is never nil in builtin provisioners

builtin/provisioners/file/resource_provisioner.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"sync"
 
 	"github.com/hashicorp/terraform/communicator"
 	"github.com/hashicorp/terraform/configs/configschema"
@@ -16,13 +15,17 @@ import (
 )
 
 func New() provisioners.Interface {
-	return &provisioner{}
+	ctx, cancel := context.WithCancel(context.Background())
+	return &provisioner{
+		ctx:    ctx,
+		cancel: cancel,
+	}
 }
 
 type provisioner struct {
-	// this stored from the running context, so that Stop() can
-	// cancel the transfer
-	mu     sync.Mutex
+	// We store a context here tied to the lifetime of the provisioner.
+	// This allows the Stop method to cancel any in-flight requests.
+	ctx    context.Context
 	cancel context.CancelFunc
 }
 
@@ -71,11 +74,6 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi
 }
 
 func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) {
-	p.mu.Lock()
-	ctx, cancel := context.WithCancel(context.Background())
-	p.cancel = cancel
-	p.mu.Unlock()
-
 	comm, err := communicator.New(req.Connection)
 	if err != nil {
 		resp.Diagnostics = resp.Diagnostics.Append(err)
@@ -94,7 +92,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques
 
 	// Begin the file copy
 	dst := req.Config.GetAttr("destination").AsString()
-	if err := copyFiles(ctx, comm, src, dst); err != nil {
+	if err := copyFiles(p.ctx, comm, src, dst); err != nil {
 		resp.Diagnostics = resp.Diagnostics.Append(err)
 		return resp
 	}
@@ -178,12 +176,11 @@ func copyFiles(ctx context.Context, comm communicator.Communicator, src, dst str
 }
 
 func (p *provisioner) Stop() error {
-	p.mu.Lock()
-	defer p.mu.Unlock()
 	p.cancel()
 	return nil
 }
 
 func (p *provisioner) Close() error {
+	p.cancel()
 	return nil
 }

builtin/provisioners/file/resource_provisioner_test.go

@@ -95,3 +95,10 @@ func TestResourceProvider_Validate_bad_to_many_src(t *testing.T) {
 		t.Fatal("Should have errors")
 	}
 }
+
+// Validate that Stop can Close can be called even when not provisioning.
+func TestResourceProvisioner_StopClose(t *testing.T) {
+	p := New()
+	p.Stop()
+	p.Close()
+}

builtin/provisioners/local-exec/resource_provisioner.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"os/exec"
 	"runtime"
-	"sync"
 
 	"github.com/armon/circbuf"
 	"github.com/hashicorp/terraform/configs/configschema"
@@ -24,13 +23,17 @@ const (
 )
 
 func New() provisioners.Interface {
-	return &provisioner{}
+	ctx, cancel := context.WithCancel(context.Background())
+	return &provisioner{
+		ctx:    ctx,
+		cancel: cancel,
+	}
 }
 
 type provisioner struct {
-	// this stored from the running context, so that Stop() can cancel the
-	// command
-	mu     sync.Mutex
+	// We store a context here tied to the lifetime of the provisioner.
+	// This allows the Stop method to cancel any in-flight requests.
+	ctx    context.Context
 	cancel context.CancelFunc
 }
 
@@ -68,11 +71,6 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi
 }
 
 func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) {
-	p.mu.Lock()
-	ctx, cancel := context.WithCancel(context.Background())
-	p.cancel = cancel
-	p.mu.Unlock()
-
 	command := req.Config.GetAttr("command").AsString()
 	if command == "" {
 		resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("local-exec provisioner command must be a non-empty string"))
@@ -127,7 +125,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques
 	cmdEnv = append(cmdEnv, env...)
 
 	// Setup the command
-	cmd := exec.CommandContext(ctx, cmdargs[0], cmdargs[1:]...)
+	cmd := exec.CommandContext(p.ctx, cmdargs[0], cmdargs[1:]...)
 	cmd.Stderr = pw
 	cmd.Stdout = pw
 	// Dir specifies the working directory of the command.
@@ -165,7 +163,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques
 	// copyOutput goroutine will just hang out until exit.
 	select {
 	case <-copyDoneCh:
-	case <-ctx.Done():
+	case <-p.ctx.Done():
 	}
 
 	if err != nil {
@@ -178,13 +176,12 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques
 }
 
 func (p *provisioner) Stop() error {
-	p.mu.Lock()
-	defer p.mu.Unlock()
 	p.cancel()
 	return nil
 }
 
 func (p *provisioner) Close() error {
+	p.cancel()
 	return nil
 }
 

builtin/provisioners/local-exec/resource_provisioner_test.go

@@ -197,3 +197,10 @@ BAR 1 true`
 		t.Errorf("wrong output\ngot:  %s\nwant: %s", got, want)
 	}
 }
+
+// Validate that Stop can Close can be called even when not provisioning.
+func TestResourceProvisioner_StopClose(t *testing.T) {
+	p := New()
+	p.Stop()
+	p.Close()
+}

builtin/provisioners/remote-exec/resource_provisioner.go

@@ -10,7 +10,6 @@ import (
 	"log"
 	"os"
 	"strings"
-	"sync"
 
 	"github.com/hashicorp/terraform/communicator"
 	"github.com/hashicorp/terraform/communicator/remote"
@@ -21,13 +20,17 @@ import (
 )
 
 func New() provisioners.Interface {
-	return &provisioner{}
+	ctx, cancel := context.WithCancel(context.Background())
+	return &provisioner{
+		ctx:    ctx,
+		cancel: cancel,
+	}
 }
 
 type provisioner struct {
-	// this stored from the running context, so that Stop() can cancel the
-	// command
-	mu     sync.Mutex
+	// We store a context here tied to the lifetime of the provisioner.
+	// This allows the Stop method to cancel any in-flight requests.
+	ctx    context.Context
 	cancel context.CancelFunc
 }
 
@@ -82,11 +85,6 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi
 }
 
 func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) {
-	p.mu.Lock()
-	ctx, cancel := context.WithCancel(context.Background())
-	p.cancel = cancel
-	p.mu.Unlock()
-
 	comm, err := communicator.New(req.Connection)
 	if err != nil {
 		resp.Diagnostics = resp.Diagnostics.Append(err)
@@ -104,7 +102,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques
 	}
 
 	// Copy and execute each script
-	if err := runScripts(ctx, req.UIOutput, comm, scripts); err != nil {
+	if err := runScripts(p.ctx, req.UIOutput, comm, scripts); err != nil {
 		resp.Diagnostics = resp.Diagnostics.Append(err)
 		return resp
 	}
@@ -113,13 +111,12 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques
 }
 
 func (p *provisioner) Stop() error {
-	p.mu.Lock()
-	defer p.mu.Unlock()
 	p.cancel()
 	return nil
 }
 
 func (p *provisioner) Close() error {
+	p.cancel()
 	return nil
 }
 

builtin/provisioners/remote-exec/resource_provisioner_test.go

@@ -254,3 +254,10 @@ func TestProvisionerTimeout(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// Validate that Stop can Close can be called even when not provisioning.
+func TestResourceProvisioner_StopClose(t *testing.T) {
+	p := New()
+	p.Stop()
+	p.Close()
+}