From fb9fca8bc94ebc8be93c5ee6607ea537bf18dc5e Mon Sep 17 00:00:00 2001
From: Mathias Fredriksson <mafredri@gmail.com>
Date: Thu, 4 Aug 2022 20:37:07 +0300
Subject: [PATCH] fix: Ensure terraform tests have a cache path and logger
 (#3161)

* fix: Ensure terraform tests have a cache path and logger

* fix: Protect against concurrent `terraform init`
---
 provisioner/terraform/executor.go       | 14 ++++++++++++++
 provisioner/terraform/parse_test.go     | 25 +++----------------------
 provisioner/terraform/provision_test.go | 10 +++++++---
 provisioner/terraform/serve.go          | 12 +++++++++---
 4 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go
index 44b6d4d9e8..4eb432252a 100644
--- a/provisioner/terraform/executor.go
+++ b/provisioner/terraform/executor.go
@@ -23,6 +23,7 @@ import (
 )
 
 type executor struct {
+	initMu     sync.Locker
 	binaryPath string
 	cachePath  string
 	workdir    string
@@ -142,6 +143,19 @@ func (e executor) init(ctx context.Context, logr logger) error {
 		"-no-color",
 		"-input=false",
 	}
+
+	// When cache path is set, we must protect against multiple calls
+	// to `terraform init`.
+	//
+	// From the Terraform documentation:
+	//     Note: The plugin cache directory is not guaranteed to be
+	//     concurrency safe. The provider installer's behavior in
+	//     environments with multiple terraform init calls is undefined.
+	if e.cachePath != "" {
+		e.initMu.Lock()
+		defer e.initMu.Unlock()
+	}
+
 	return e.execWriteOutput(ctx, args, e.basicEnv(), outWriter, errWriter)
 }
 
diff --git a/provisioner/terraform/parse_test.go b/provisioner/terraform/parse_test.go
index dab994d335..0a88ab21d5 100644
--- a/provisioner/terraform/parse_test.go
+++ b/provisioner/terraform/parse_test.go
@@ -3,40 +3,20 @@
 package terraform_test
 
 import (
-	"context"
 	"encoding/json"
 	"os"
 	"path/filepath"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/coder/coder/provisioner/terraform"
-	"github.com/coder/coder/provisionersdk"
 	"github.com/coder/coder/provisionersdk/proto"
 )
 
 func TestParse(t *testing.T) {
 	t.Parallel()
 
-	// Create an in-memory provisioner to communicate with.
-	client, server := provisionersdk.TransportPipe()
-	ctx, cancelFunc := context.WithCancel(context.Background())
-	t.Cleanup(func() {
-		_ = client.Close()
-		_ = server.Close()
-		cancelFunc()
-	})
-	go func() {
-		err := terraform.Serve(ctx, &terraform.ServeOptions{
-			ServeOptions: &provisionersdk.ServeOptions{
-				Listener: server,
-			},
-		})
-		assert.NoError(t, err)
-	}()
-	api := proto.NewDRPCProvisionerClient(provisionersdk.Conn(client))
+	ctx, api := setupProvisioner(t)
 
 	testCases := []struct {
 		Name     string
@@ -175,7 +155,8 @@ func TestParse(t *testing.T) {
 								DefaultDestination: &proto.ParameterDestination{
 									Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE,
 								},
-							}},
+							},
+						},
 					},
 				},
 			},
diff --git a/provisioner/terraform/provision_test.go b/provisioner/terraform/provision_test.go
index 10c9e0fe92..4b9e203ab4 100644
--- a/provisioner/terraform/provision_test.go
+++ b/provisioner/terraform/provision_test.go
@@ -23,21 +23,25 @@ import (
 )
 
 func setupProvisioner(t *testing.T) (context.Context, proto.DRPCProvisionerClient) {
+	cachePath := t.TempDir()
 	client, server := provisionersdk.TransportPipe()
 	ctx, cancelFunc := context.WithCancel(context.Background())
+	serverErr := make(chan error, 1)
 	t.Cleanup(func() {
 		_ = client.Close()
 		_ = server.Close()
 		cancelFunc()
+		err := <-serverErr
+		assert.NoError(t, err)
 	})
 	go func() {
-		err := terraform.Serve(ctx, &terraform.ServeOptions{
+		serverErr <- terraform.Serve(ctx, &terraform.ServeOptions{
 			ServeOptions: &provisionersdk.ServeOptions{
 				Listener: server,
 			},
-			Logger: slogtest.Make(t, nil).Leveled(slog.LevelDebug),
+			CachePath: cachePath,
+			Logger:    slogtest.Make(t, nil).Leveled(slog.LevelDebug),
 		})
-		assert.NoError(t, err)
 	}()
 	api := proto.NewDRPCProvisionerClient(provisionersdk.Conn(client))
 	return ctx, api
diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go
index 66e819fbdb..1a75483bda 100644
--- a/provisioner/terraform/serve.go
+++ b/provisioner/terraform/serve.go
@@ -3,6 +3,7 @@ package terraform
 import (
 	"context"
 	"path/filepath"
+	"sync"
 
 	"github.com/cli/safeexec"
 	"github.com/hashicorp/go-version"
@@ -109,15 +110,20 @@ func Serve(ctx context.Context, options *ServeOptions) error {
 }
 
 type server struct {
+	// initMu protects against executors running `terraform init`
+	// concurrently when cache path is set.
+	initMu sync.Mutex
+
 	binaryPath string
 	cachePath  string
 	logger     slog.Logger
 }
 
-func (t server) executor(workdir string) executor {
+func (s *server) executor(workdir string) executor {
 	return executor{
-		binaryPath: t.binaryPath,
-		cachePath:  t.cachePath,
+		initMu:     &s.initMu,
+		binaryPath: s.binaryPath,
+		cachePath:  s.cachePath,
 		workdir:    workdir,
 	}
 }