refactor: always test against default terraform (#533)

Integration tests now always use the configured default version of
terraform, rather than whatever terraform happens to be at the front of
PATH.

fix: race condition between writing downloaded terraform binary and
setting file permissions.

go.mod

@@ -141,3 +141,5 @@ require (
 replace github.com/r3labs/sse/v2 => github.com/leg100/sse/v2 v2.0.0-20220910081853-79ffbd7c2fad
 
 replace github.com/jaschaephraim/lrserver => github.com/sowiner/lrserver v0.0.0-20230123160823-795409868576
+
+replace github.com/natefinch/atomic => github.com/sdassow/atomic v0.0.0-20220219102542-174b5d2a3ea6

go.sum

@@ -538,8 +538,6 @@ github.com/modocache/gover v0.0.0-20171022184752-b58185e213c5/go.mod h1:caMODM3P
 github.com/mrunalp/fileutils v0.5.0/go.mod h1:M1WthSahJixYnrXQl/DFQuteStB1weuxD2QJNHXfbSQ=
 github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
 github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
-github.com/natefinch/atomic v1.0.1 h1:ZPYKxkqQOx3KZ+RsbnP/YsgvxWQPGxjC0oBt2AhwV0A=
-github.com/natefinch/atomic v1.0.1/go.mod h1:N/D/ELrljoqDyT3rZrsUmtsuzvHkeB/wWjHV22AZRbM=
 github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U=
 github.com/opencontainers/go-digest v1.0.0-rc1/go.mod h1:cMLVZDEM3+U2I4VmLI6N8jQYUd2OVphdqWwCJHrFt2s=
 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
@@ -609,6 +607,8 @@ github.com/rs/zerolog v1.15.0/go.mod h1:xYTKnLHcpfU2225ny5qZjxnj9NvkumZYjJHlAThC
 github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
 github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0=
+github.com/sdassow/atomic v0.0.0-20220219102542-174b5d2a3ea6 h1:yUJHXMYPIyd+qLuvZaIidsA424KxywivfFQzYNJbkj0=
+github.com/sdassow/atomic v0.0.0-20220219102542-174b5d2a3ea6/go.mod h1:N/D/ELrljoqDyT3rZrsUmtsuzvHkeB/wWjHV22AZRbM=
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
 github.com/seccomp/libseccomp-golang v0.9.1/go.mod h1:GbW5+tmTXfcxTToHLXlScSlAvWlF4P2Ca7zGrPiEpWo=
 github.com/sergi/go-diff v1.0.0/go.mod h1:0CfEIISq7TuYL3j771MWULgwwjU+GofnZX9QAmXWZgo=

internal/agent/agent.go

@@ -36,10 +36,10 @@ type agent struct {
 	client.Client
 	logr.Logger
 
-	spooler             // spools new run events
-	*terminator         // terminates runs
-	Downloader          // terraform cli downloader
-	terraformPathFinder // determines destination dir for terraform bins
+	spooler              // spools new run events
+	*terminator          // terminates runs
+	Downloader           // terraform cli downloader
+	*TerraformPathFinder // determines destination dir for terraform bins
 
 	envs []string // terraform environment variables
 }
@@ -71,8 +71,8 @@ func NewAgent(logger logr.Logger, app client.Client, cfg Config) (*agent, error)
 		envs:                DefaultEnvs,
 		spooler:             newSpooler(app, logger, cfg),
 		terminator:          newTerminator(),
-		Downloader:          newTerraformDownloader(pathFinder),
-		terraformPathFinder: pathFinder,
+		Downloader:          NewDownloader(pathFinder),
+		TerraformPathFinder: pathFinder,
 	}
 
 	if cfg.PluginCache {

internal/agent/environment.go

@@ -97,7 +97,7 @@ func newEnvironment(
 		runner:     &runner{out: writer},
 		executor: &executor{
 			Config:              agent.Config,
-			terraformPathFinder: agent.terraformPathFinder,
+			TerraformPathFinder: agent.TerraformPathFinder,
 			version:             ws.TerraformVersion,
 			out:                 writer,
 			envs:                envs,

internal/agent/executor.go

@@ -19,7 +19,7 @@ type (
 	// executor executes processes.
 	executor struct {
 		Config
-		terraformPathFinder
+		*TerraformPathFinder
 
 		version string // terraform cli version
 		out     io.Writer

internal/agent/steps.go

@@ -105,7 +105,7 @@ func (r *runner) cancel(force bool) {
 }
 
 func (b *stepsBuilder) downloadTerraform(ctx context.Context) error {
-	_, err := b.download(ctx, b.version, b.out)
+	_, err := b.Download(ctx, b.version, b.out)
 	return err
 }
 

internal/agent/download.go → internal/agent/terraform_download.go

@@ -17,6 +17,7 @@ import (
 type download struct {
 	// for outputting progress updates
 	io.Writer
+
 	version   string
 	src, dest string
 	client    *http.Client
@@ -91,12 +92,9 @@ func (d *download) unzip(zipfile string) error {
 				return err
 			}
 			defer fr.Close()
-			if err := atomic.WriteFile(d.dest, fr); err != nil {
+			if err := atomic.WriteFile(d.dest, fr, atomic.DefaultFileMode(0o755)); err != nil {
 				return fmt.Errorf("writing terraform binary: %w", err)
 			}
-			if err := os.Chmod(d.dest, 0x755); err != nil {
-				return fmt.Errorf("setting permissions on terraform binary: %w", err)
-			}
 			return nil
 		}
 	}

internal/agent/downloader.go → internal/agent/terraform_downloader.go

@@ -17,35 +17,43 @@ const HashicorpReleasesHost = "releases.hashicorp.com"
 type (
 	// terraformDownloader downloads terraform binaries
 	terraformDownloader struct {
-		host                string        // server hosting binaries
-		terraformPathFinder               // used to lookup destination path for saving download
-		client              *http.Client  // client for downloading from server via http
-		mu                  chan struct{} // ensures only one download at a time
+		*TerraformPathFinder // used to lookup destination path for saving download
+
+		host   string        // server hosting binaries
+		client *http.Client  // client for downloading from server via http
+		mu     chan struct{} // ensures only one download at a time
 	}
 
 	// Downloader downloads a specific version of a binary and returns its path
 	Downloader interface {
-		download(ctx context.Context, version string, w io.Writer) (string, error)
+		Download(ctx context.Context, version string, w io.Writer) (string, error)
 	}
 )
 
-func newTerraformDownloader(pathFinder terraformPathFinder) *terraformDownloader {
+// NewDownloader constructs a terraform downloader. Pass a path finder to
+// customise the location to which the bins are persisted, or pass nil to use
+// the default.
+func NewDownloader(pathFinder *TerraformPathFinder) *terraformDownloader {
+	if pathFinder == nil {
+		pathFinder = newTerraformPathFinder(defaultTerraformBinDir)
+	}
+
 	mu := make(chan struct{}, 1)
 	mu <- struct{}{}
 
 	return &terraformDownloader{
 		host:                HashicorpReleasesHost,
-		terraformPathFinder: pathFinder,
+		TerraformPathFinder: pathFinder,
 		client:              &http.Client{},
 		mu:                  mu,
 	}
 }
 
 // Download ensures the given version of terraform is available on the local
-// filesystem and returns its path. Thread-safe: if a download is in-flight and
-// another download is requested then it'll be made to wait until the
+// filesystem and returns its path. Thread-safe: if a Download is in-flight and
+// another Download is requested then it'll be made to wait until the
 // former has finished.
-func (d *terraformDownloader) download(ctx context.Context, version string, w io.Writer) (string, error) {
+func (d *terraformDownloader) Download(ctx context.Context, version string, w io.Writer) (string, error) {
 	if internal.Exists(d.dest(version)) {
 		return d.dest(version), nil
 	}

internal/agent/downloader_test.go → internal/agent/terraform_downloader_test.go

@@ -25,7 +25,7 @@ func TestDownloader(t *testing.T) {
 	require.NoError(t, err)
 
 	pathFinder := newTerraformPathFinder(t.TempDir())
-	dl := newTerraformDownloader(pathFinder)
+	dl := NewDownloader(pathFinder)
 	dl.host = u.Host
 	dl.client = &http.Client{
 		Transport: &http.Transport{
@@ -34,7 +34,7 @@ func TestDownloader(t *testing.T) {
 	}
 
 	buf := new(bytes.Buffer)
-	tfpath, err := dl.download(context.Background(), "1.2.3", buf)
+	tfpath, err := dl.Download(context.Background(), "1.2.3", buf)
 	require.NoError(t, err)
 	require.FileExists(t, tfpath)
 	tfbin, err := os.ReadFile(tfpath)

internal/agent/terraform.go → internal/agent/terraform_path_finder.go

@@ -8,20 +8,20 @@ import (
 var defaultTerraformBinDir = path.Join(os.TempDir(), "otf-terraform-bins")
 
 type (
-	terraformPathFinder struct {
+	TerraformPathFinder struct {
 		dest string
 	}
 )
 
-func newTerraformPathFinder(dest string) terraformPathFinder {
+func newTerraformPathFinder(dest string) *TerraformPathFinder {
 	if dest == "" {
 		dest = defaultTerraformBinDir
 	}
-	return terraformPathFinder{
+	return &TerraformPathFinder{
 		dest: dest,
 	}
 }
 
-func (t terraformPathFinder) TerraformPath(version string) string {
+func (t *TerraformPathFinder) TerraformPath(version string) string {
 	return path.Join(t.dest, version, "terraform")
 }

internal/integration/daemon_helpers_test.go

@@ -439,14 +439,16 @@ func (s *testDaemon) tfcli(t *testing.T, ctx context.Context, command, configPat
 func (s *testDaemon) tfcliWithError(t *testing.T, ctx context.Context, command, configPath string, args ...string) (string, error) {
 	t.Helper()
 
+	tfpath := downloadTerraform(t, ctx, nil)
+
 	// Create user token expressly for the terraform cli
 	user := userFromContext(t, ctx)
 	_, token := s.createToken(t, ctx, user)
 
 	cmdargs := []string{command, "-no-color"}
 	cmdargs = append(cmdargs, args...)
 
-	cmd := exec.Command("terraform", cmdargs...)
+	cmd := exec.Command(tfpath, cmdargs...)
 	cmd.Dir = configPath
 
 	cmd.Env = internal.SafeAppend(sharedEnvs, internal.CredentialEnv(s.Hostname(), token))

internal/integration/helpers_test.go

@@ -3,11 +3,14 @@ package integration
 import (
 	"context"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"testing"
 
+	"github.com/leg100/otf/internal"
 	"github.com/leg100/otf/internal/auth"
+	"github.com/leg100/otf/internal/workspace"
 	"github.com/stretchr/testify/require"
 )
 
@@ -76,3 +79,14 @@ func userFromContext(t *testing.T, ctx context.Context) *auth.User {
 	require.NoError(t, err)
 	return user
 }
+
+func downloadTerraform(t *testing.T, ctx context.Context, version *string) string {
+	t.Helper()
+
+	if version == nil {
+		version = internal.String(workspace.DefaultTerraformVersion)
+	}
+	tfpath, err := tfDownloader.Download(ctx, *version, io.Discard)
+	require.NoError(t, err)
+	return tfpath
+}

internal/integration/main_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/leg100/otf/internal"
+	"github.com/leg100/otf/internal/agent"
 	"github.com/leg100/otf/internal/auth"
 	"github.com/leg100/otf/internal/testbrowser"
 	"github.com/leg100/otf/internal/testcompose"
@@ -28,6 +29,9 @@ var (
 
 	// pool of web browsers
 	browser *testbrowser.Pool
+
+	// downloader for specific versions of terraform for tests to use
+	tfDownloader agent.Downloader
 )
 
 func TestMain(m *testing.M) {
@@ -146,6 +150,10 @@ func doMain(m *testing.M) (int, error) {
 	defer cleanup()
 	browser = pool
 
+	// Setup terraform downloader. The default (nil) saves the terraform bins to
+	// the system temp directory so they can be persisted between tests.
+	tfDownloader = agent.NewDownloader(nil)
+
 	return m.Run(), nil
 }
 

internal/integration/tag_e2e_test.go

@@ -36,10 +36,12 @@ terraform {
 resource "null_resource" "tags_e2e" {}
 `, daemon.Hostname(), org.Name))
 
+	tfpath := downloadTerraform(t, ctx, nil)
+
 	// run terraform init
 	_, token := daemon.createToken(t, ctx, nil)
 	e, tferr, err := expect.SpawnWithArgs(
-		[]string{"terraform", "-chdir=" + root, "init", "-no-color"},
+		[]string{tfpath, "-chdir=" + root, "init", "-no-color"},
 		time.Minute,
 		expect.PartialMatch(true),
 		expect.SetEnv(internal.SafeAppend(sharedEnvs, internal.CredentialEnv(daemon.Hostname(), token))),

internal/integration/terraform_cli_cancel_test.go

@@ -41,10 +41,12 @@ data "http" "wait" {
 `, srv.URL))
 	svc.tfcli(t, ctx, "init", config)
 
+	tfpath := downloadTerraform(t, ctx, nil)
+
 	// Invoke terraform plan
 	_, token := svc.createToken(t, ctx, nil)
 	e, tferr, err := expect.SpawnWithArgs(
-		[]string{"terraform", "-chdir=" + config, "plan", "-no-color"},
+		[]string{tfpath, "-chdir=" + config, "plan", "-no-color"},
 		time.Minute,
 		expect.PartialMatch(true),
 		expect.SetEnv(

internal/integration/terraform_cli_discard_test.go

@@ -27,9 +27,11 @@ func TestIntegration_TerraformCLIDiscard(t *testing.T) {
 	// Create user token expressly for terraform apply
 	_, token := svc.createToken(t, ctx, nil)
 
+	tfpath := downloadTerraform(t, ctx, nil)
+
 	// Invoke terraform apply
 	e, tferr, err := expect.SpawnWithArgs(
-		[]string{"terraform", "-chdir=" + configPath, "apply", "-no-color"},
+		[]string{tfpath, "-chdir=" + configPath, "apply", "-no-color"},
 		time.Minute,
 		expect.PartialMatch(true),
 		expect.SetEnv(internal.SafeAppend(sharedEnvs, internal.CredentialEnv(svc.Hostname(), token))),

internal/integration/terraform_login_test.go

@@ -31,8 +31,10 @@ func TestTerraformLogin(t *testing.T) {
 	require.NoError(t, err)
 	killBrowserPath := path.Join(wd, "./fixtures/kill-browser")
 
+	tfpath := downloadTerraform(t, ctx, nil)
+
 	e, tferr, err := expect.SpawnWithArgs(
-		[]string{"terraform", "login", svc.Hostname()},
+		[]string{tfpath, "login", svc.Hostname()},
 		time.Minute,
 		expect.PartialMatch(true),
 		// expect.Verbose(testing.Verbose()),
@@ -74,7 +76,7 @@ func TestTerraformLogin(t *testing.T) {
 	// has authenticated successfully.
 	org := svc.createOrganization(t, ctx)
 	configPath := newRootModule(t, svc.Hostname(), org.Name, t.Name())
-	cmd := exec.Command("terraform", "init")
+	cmd := exec.Command(tfpath, "init")
 	cmd.Dir = configPath
 	assert.NoError(t, cmd.Run())
 }