From b25b4b15c81fcface5dbbfd0d21e253e439faf36 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Thu, 26 May 2022 06:14:44 +0000 Subject: [PATCH 01/12] download default terraform version when minor version mismatches --- provisioner/terraform/serve.go | 54 +++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index 60c03ed0dbe34..3de4dbaca6b5e 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -2,7 +2,10 @@ package terraform import ( "context" + "os/exec" "path/filepath" + "regexp" + "strings" "github.com/cli/safeexec" "github.com/hashicorp/go-version" @@ -17,6 +20,7 @@ import ( // This is the exact version of Terraform used internally // when Terraform is missing on the system. const terraformVersion = "1.1.9" +const versionDelimiter = "." var ( // The minimum version of Terraform supported by the provisioner. @@ -45,7 +49,46 @@ type ServeOptions struct { func Serve(ctx context.Context, options *ServeOptions) error { if options.BinaryPath == "" { binaryPath, err := safeexec.LookPath("terraform") + var downloadTerraform bool if err != nil { + downloadTerraform = true + } else { + // If the "coder" binary is in the same directory as + // the "terraform" binary, "terraform" is returned. + // + // We must resolve the absolute path for other processes + // to execute this properly! + absoluteBinary, err := filepath.Abs(binaryPath) + if err != nil { + return xerrors.Errorf("absolute: %w", err) + } + // Checking the installed version of Terraform. + output, err := exec.Command(absoluteBinary, "version").Output() + if err != nil { + return xerrors.Errorf("terraform version: %w", err) + } + // The output for `terraform version` is: + // Terraform v1.2.1 + // on linux_amd64 + versionRegex := regexp.MustCompile("Terraform v(.+)\n?.*") + match := versionRegex.FindStringSubmatch(string(output)) + if match != nil { + // match[0] is the entire string. + // match[1] is the matched substring. + version := match[1] + terraformMinorVersion := strings.Join(strings.Split(terraformVersion, versionDelimiter)[:2], versionDelimiter) + if !strings.HasPrefix(version, terraformMinorVersion) { + downloadTerraform = true + } + } else { + // Download the required Terraform version when unable to determine the existing one. + downloadTerraform = true + } + if !downloadTerraform { + options.BinaryPath = absoluteBinary + } + } + if downloadTerraform { installer := &releases.ExactVersion{ InstallDir: options.CachePath, Product: product.Terraform, @@ -57,17 +100,6 @@ func Serve(ctx context.Context, options *ServeOptions) error { return xerrors.Errorf("install terraform: %w", err) } options.BinaryPath = execPath - } else { - // If the "coder" binary is in the same directory as - // the "terraform" binary, "terraform" is returned. - // - // We must resolve the absolute path for other processes - // to execute this properly! - absoluteBinary, err := filepath.Abs(binaryPath) - if err != nil { - return xerrors.Errorf("absolute: %w", err) - } - options.BinaryPath = absoluteBinary } } return provisionersdk.Serve(ctx, &server{ From 0e1aa3f5720ff9b197531d82984ff00a264fffb2 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 17 Jun 2022 15:45:44 +0000 Subject: [PATCH 02/12] use the new exectuor version method --- provisioner/terraform/executor.go | 15 +++++++++++++++ provisioner/terraform/serve.go | 31 +++++++------------------------ 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go index 0ed000af9bb51..2adbc1de0c7d9 100644 --- a/provisioner/terraform/executor.go +++ b/provisioner/terraform/executor.go @@ -118,6 +118,21 @@ func (e executor) version(ctx context.Context) (*version.Version, error) { return version.NewVersion(vj.Version) } +func versionFromBinaryPath(ctx context.Context, binaryPath string) (*version.Version, error) { + // #nosec + cmd := exec.CommandContext(ctx, binaryPath, "version", "-json") + out, err := cmd.Output() + if err != nil { + return nil, err + } + vj := tfjson.VersionOutput{} + err = json.Unmarshal(out, &vj) + if err != nil { + return nil, err + } + return version.NewVersion(vj.Version) +} + func (e executor) init(ctx context.Context, logr logger) error { outWriter, doneOut := logWriter(logr, proto.LogLevel_DEBUG) errWriter, doneErr := logWriter(logr, proto.LogLevel_ERROR) diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index 3de4dbaca6b5e..ee5bafb97095f 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -2,10 +2,7 @@ package terraform import ( "context" - "os/exec" "path/filepath" - "regexp" - "strings" "github.com/cli/safeexec" "github.com/hashicorp/go-version" @@ -19,8 +16,9 @@ import ( // This is the exact version of Terraform used internally // when Terraform is missing on the system. -const terraformVersion = "1.1.9" -const versionDelimiter = "." +var terraformVersion = version.Must(version.NewVersion("1.1.9")) +var minTerraformVersion = version.Must(version.NewVersion("1.1.0")) +var maxTerraformVersion = version.Must(version.NewVersion("1.2.0")) var ( // The minimum version of Terraform supported by the provisioner. @@ -63,25 +61,10 @@ func Serve(ctx context.Context, options *ServeOptions) error { return xerrors.Errorf("absolute: %w", err) } // Checking the installed version of Terraform. - output, err := exec.Command(absoluteBinary, "version").Output() + version, err := versionFromBinaryPath(ctx, absoluteBinary) if err != nil { - return xerrors.Errorf("terraform version: %w", err) - } - // The output for `terraform version` is: - // Terraform v1.2.1 - // on linux_amd64 - versionRegex := regexp.MustCompile("Terraform v(.+)\n?.*") - match := versionRegex.FindStringSubmatch(string(output)) - if match != nil { - // match[0] is the entire string. - // match[1] is the matched substring. - version := match[1] - terraformMinorVersion := strings.Join(strings.Split(terraformVersion, versionDelimiter)[:2], versionDelimiter) - if !strings.HasPrefix(version, terraformMinorVersion) { - downloadTerraform = true - } - } else { - // Download the required Terraform version when unable to determine the existing one. + downloadTerraform = true + } else if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { downloadTerraform = true } if !downloadTerraform { @@ -92,7 +75,7 @@ func Serve(ctx context.Context, options *ServeOptions) error { installer := &releases.ExactVersion{ InstallDir: options.CachePath, Product: product.Terraform, - Version: version.Must(version.NewVersion(terraformVersion)), + Version: terraformVersion, } execPath, err := installer.Install(ctx) From 544243fda4b9ec8620823212cda137555b15797b Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 17 Jun 2022 17:46:17 +0000 Subject: [PATCH 03/12] refactor into methods --- provisioner/terraform/executor.go | 13 +------- provisioner/terraform/serve.go | 53 ++++++++++++++++--------------- 2 files changed, 29 insertions(+), 37 deletions(-) diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go index 2adbc1de0c7d9..eab070c2d8034 100644 --- a/provisioner/terraform/executor.go +++ b/provisioner/terraform/executor.go @@ -104,18 +104,7 @@ func (e executor) checkMinVersion(ctx context.Context) error { } func (e executor) version(ctx context.Context) (*version.Version, error) { - // #nosec - cmd := exec.CommandContext(ctx, e.binaryPath, "version", "-json") - out, err := cmd.Output() - if err != nil { - return nil, err - } - vj := tfjson.VersionOutput{} - err = json.Unmarshal(out, &vj) - if err != nil { - return nil, err - } - return version.NewVersion(vj.Version) + return versionFromBinaryPath(ctx, e.binaryPath) } func versionFromBinaryPath(ctx context.Context, binaryPath string) (*version.Version, error) { diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index ee5bafb97095f..92d4e3343f61f 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -43,35 +43,38 @@ type ServeOptions struct { Logger slog.Logger } +func getAbsoluteBinaryPath(ctx context.Context) (string, bool) { + binaryPath, err := safeexec.LookPath("terraform") + if err != nil { + return "", false + } + // If the "coder" binary is in the same directory as + // the "terraform" binary, "terraform" is returned. + // + // We must resolve the absolute path for other processes + // to execute this properly! + absoluteBinary, err := filepath.Abs(binaryPath) + if err != nil { + return "", false + } + // Checking the installed version of Terraform. + version, err := versionFromBinaryPath(ctx, absoluteBinary) + if err != nil { + return "", false + } else if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { + return "", false + } + return absoluteBinary, true +} + // Serve starts a dRPC server on the provided transport speaking Terraform provisioner. func Serve(ctx context.Context, options *ServeOptions) error { if options.BinaryPath == "" { - binaryPath, err := safeexec.LookPath("terraform") - var downloadTerraform bool - if err != nil { - downloadTerraform = true + absoluteBinary, ok := getAbsoluteBinaryPath(ctx) + + if ok { + options.BinaryPath = absoluteBinary } else { - // If the "coder" binary is in the same directory as - // the "terraform" binary, "terraform" is returned. - // - // We must resolve the absolute path for other processes - // to execute this properly! - absoluteBinary, err := filepath.Abs(binaryPath) - if err != nil { - return xerrors.Errorf("absolute: %w", err) - } - // Checking the installed version of Terraform. - version, err := versionFromBinaryPath(ctx, absoluteBinary) - if err != nil { - downloadTerraform = true - } else if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { - downloadTerraform = true - } - if !downloadTerraform { - options.BinaryPath = absoluteBinary - } - } - if downloadTerraform { installer := &releases.ExactVersion{ InstallDir: options.CachePath, Product: product.Terraform, From c76885d302940008daa864683ab8a8c4d2e8cf44 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 17 Jun 2022 19:08:17 +0000 Subject: [PATCH 04/12] add unit tests --- provisioner/terraform/serve_internal_test.go | 95 ++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 provisioner/terraform/serve_internal_test.go diff --git a/provisioner/terraform/serve_internal_test.go b/provisioner/terraform/serve_internal_test.go new file mode 100644 index 0000000000000..c04da7cad6346 --- /dev/null +++ b/provisioner/terraform/serve_internal_test.go @@ -0,0 +1,95 @@ +package terraform + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_getAbsoluteBinaryPath(t *testing.T) { + t.Parallel() + type args struct { + ctx context.Context + } + tests := []struct { + name string + args args + terraformVersion string + expectedAbsoluteBinary string + expectedOk bool + }{ + { + name: "TestCorrectVersion", + args: args{ctx: context.Background()}, + terraformVersion: "1.1.9", + expectedAbsoluteBinary: "", + expectedOk: true, + }, + { + name: "TestOldVersion", + args: args{ctx: context.Background()}, + terraformVersion: "1.0.9", + expectedAbsoluteBinary: "", + expectedOk: false, + }, + { + name: "TestNewVersion", + args: args{ctx: context.Background()}, + terraformVersion: "1.2.9", + expectedAbsoluteBinary: "", + expectedOk: false, + }, + { + name: "TestMalformedVersion", + args: args{ctx: context.Background()}, + terraformVersion: "version", + expectedAbsoluteBinary: "", + expectedOk: false, + }, + } + // nolint:paralleltest + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a temp dir with the binary + tempDir := t.TempDir() + terraformBinaryOutput := fmt.Sprintf(`#!/bin/sh + cat <<-EOF + { + "terraform_version": "%s", + "platform": "linux_amd64", + "provider_selections": {}, + "terraform_outdated": false + } + EOF`, tt.terraformVersion) + + // #nosec + err := os.WriteFile( + filepath.Join(tempDir, "terraform"), + []byte(terraformBinaryOutput), + 0770, + ) + require.NoError(t, err) + + // Add the binary to PATH + pathVariable := os.Getenv("PATH") + t.Setenv("PATH", strings.Join([]string{tempDir, pathVariable}, ":")) + + if tt.expectedOk { + tt.expectedAbsoluteBinary = filepath.Join(tempDir, "terraform") + } + + actualAbsoluteBinary, actualOk := getAbsoluteBinaryPath(tt.args.ctx) + if actualAbsoluteBinary != tt.expectedAbsoluteBinary { + t.Errorf("getAbsoluteBinaryPath() absoluteBinaryPath, actual = %v, expected %v", actualAbsoluteBinary, tt.expectedAbsoluteBinary) + } + if actualOk != tt.expectedOk { + t.Errorf("getAbsoluteBinaryPath() ok, actual = %v, expected %v", actualOk, tt.expectedOk) + } + }) + } +} From 6960c33b74ff6ebf1dc185290534c3f6d66479aa Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 17 Jun 2022 19:18:10 +0000 Subject: [PATCH 05/12] unset PATH variable in cleanup --- provisioner/terraform/serve_internal_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/provisioner/terraform/serve_internal_test.go b/provisioner/terraform/serve_internal_test.go index c04da7cad6346..6c38d8ad387cd 100644 --- a/provisioner/terraform/serve_internal_test.go +++ b/provisioner/terraform/serve_internal_test.go @@ -90,6 +90,10 @@ func Test_getAbsoluteBinaryPath(t *testing.T) { if actualOk != tt.expectedOk { t.Errorf("getAbsoluteBinaryPath() ok, actual = %v, expected %v", actualOk, tt.expectedOk) } + + t.Cleanup(func() { + t.Setenv("PATH", pathVariable) + }) }) } } From ebe115bfb68b91db3073f5d659e07d633dece1cc Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 17 Jun 2022 19:37:00 +0000 Subject: [PATCH 06/12] skip tests on Windows --- provisioner/terraform/serve_internal_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/provisioner/terraform/serve_internal_test.go b/provisioner/terraform/serve_internal_test.go index 6c38d8ad387cd..1a4c58923063f 100644 --- a/provisioner/terraform/serve_internal_test.go +++ b/provisioner/terraform/serve_internal_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strings" "testing" @@ -55,6 +56,10 @@ func Test_getAbsoluteBinaryPath(t *testing.T) { // nolint:paralleltest for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Dummy terraform executable on Windows requires sh which isn't very practical.") + } + // Create a temp dir with the binary tempDir := t.TempDir() terraformBinaryOutput := fmt.Sprintf(`#!/bin/sh From b822bf3b1d8e4aad65fb5c91155443b624fe422a Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Fri, 17 Jun 2022 19:51:58 +0000 Subject: [PATCH 07/12] clean up code --- provisioner/terraform/serve.go | 7 ++- provisioner/terraform/serve_internal_test.go | 58 +++++++++----------- 2 files changed, 31 insertions(+), 34 deletions(-) diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index 92d4e3343f61f..6c051005a7695 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -48,6 +48,7 @@ func getAbsoluteBinaryPath(ctx context.Context) (string, bool) { if err != nil { return "", false } + // If the "coder" binary is in the same directory as // the "terraform" binary, "terraform" is returned. // @@ -57,13 +58,17 @@ func getAbsoluteBinaryPath(ctx context.Context) (string, bool) { if err != nil { return "", false } + // Checking the installed version of Terraform. version, err := versionFromBinaryPath(ctx, absoluteBinary) if err != nil { return "", false - } else if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { + } + + if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { return "", false } + return absoluteBinary, true } diff --git a/provisioner/terraform/serve_internal_test.go b/provisioner/terraform/serve_internal_test.go index 1a4c58923063f..9faf1d52262f1 100644 --- a/provisioner/terraform/serve_internal_test.go +++ b/provisioner/terraform/serve_internal_test.go @@ -12,45 +12,40 @@ import ( "github.com/stretchr/testify/require" ) +// nolint:paralleltest func Test_getAbsoluteBinaryPath(t *testing.T) { - t.Parallel() type args struct { ctx context.Context } tests := []struct { - name string - args args - terraformVersion string - expectedAbsoluteBinary string - expectedOk bool + name string + args args + terraformVersion string + expectedOk bool }{ { - name: "TestCorrectVersion", - args: args{ctx: context.Background()}, - terraformVersion: "1.1.9", - expectedAbsoluteBinary: "", - expectedOk: true, + name: "TestCorrectVersion", + args: args{ctx: context.Background()}, + terraformVersion: "1.1.9", + expectedOk: true, }, { - name: "TestOldVersion", - args: args{ctx: context.Background()}, - terraformVersion: "1.0.9", - expectedAbsoluteBinary: "", - expectedOk: false, + name: "TestOldVersion", + args: args{ctx: context.Background()}, + terraformVersion: "1.0.9", + expectedOk: false, }, { - name: "TestNewVersion", - args: args{ctx: context.Background()}, - terraformVersion: "1.2.9", - expectedAbsoluteBinary: "", - expectedOk: false, + name: "TestNewVersion", + args: args{ctx: context.Background()}, + terraformVersion: "1.2.9", + expectedOk: false, }, { - name: "TestMalformedVersion", - args: args{ctx: context.Background()}, - terraformVersion: "version", - expectedAbsoluteBinary: "", - expectedOk: false, + name: "TestMalformedVersion", + args: args{ctx: context.Background()}, + terraformVersion: "version", + expectedOk: false, }, } // nolint:paralleltest @@ -84,21 +79,18 @@ func Test_getAbsoluteBinaryPath(t *testing.T) { pathVariable := os.Getenv("PATH") t.Setenv("PATH", strings.Join([]string{tempDir, pathVariable}, ":")) + var expectedAbsoluteBinary string if tt.expectedOk { - tt.expectedAbsoluteBinary = filepath.Join(tempDir, "terraform") + expectedAbsoluteBinary = filepath.Join(tempDir, "terraform") } actualAbsoluteBinary, actualOk := getAbsoluteBinaryPath(tt.args.ctx) - if actualAbsoluteBinary != tt.expectedAbsoluteBinary { - t.Errorf("getAbsoluteBinaryPath() absoluteBinaryPath, actual = %v, expected %v", actualAbsoluteBinary, tt.expectedAbsoluteBinary) + if actualAbsoluteBinary != expectedAbsoluteBinary { + t.Errorf("getAbsoluteBinaryPath() absoluteBinaryPath, actual = %v, expected %v", actualAbsoluteBinary, expectedAbsoluteBinary) } if actualOk != tt.expectedOk { t.Errorf("getAbsoluteBinaryPath() ok, actual = %v, expected %v", actualOk, tt.expectedOk) } - - t.Cleanup(func() { - t.Setenv("PATH", pathVariable) - }) }) } } From 62b395ea77c409d319c3709eb7629d3c1361026c Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Tue, 21 Jun 2022 15:33:20 +0000 Subject: [PATCH 08/12] Debugging CI failure --- cli/server_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/server_test.go b/cli/server_test.go index 430be799245a7..9ac4a86b14b6b 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -84,6 +84,7 @@ func TestServer(t *testing.T) { }() require.Eventually(t, func() bool { _, err := cfg.URL().Read() + t.Log(err) return err == nil }, time.Minute, 25*time.Millisecond) cancelFunc() From 3f51748d06092e4338e5a284eb168e98fa806a55 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Tue, 21 Jun 2022 15:43:55 +0000 Subject: [PATCH 09/12] Update terraform version to default for ci --- .github/workflows/coder.yaml | 6 +++--- cli/server_test.go | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/coder.yaml b/.github/workflows/coder.yaml index 6d44de19ba1c1..6cf1754a286bd 100644 --- a/.github/workflows/coder.yaml +++ b/.github/workflows/coder.yaml @@ -197,7 +197,7 @@ jobs: - uses: hashicorp/setup-terraform@v2 with: - terraform_version: 1.1.2 + terraform_version: 1.1.9 terraform_wrapper: false - name: Test with Mock Database @@ -264,7 +264,7 @@ jobs: - uses: hashicorp/setup-terraform@v2 with: - terraform_version: 1.1.2 + terraform_version: 1.1.9 terraform_wrapper: false - name: Start PostgreSQL Database @@ -494,7 +494,7 @@ jobs: - uses: hashicorp/setup-terraform@v2 with: - terraform_version: 1.1.2 + terraform_version: 1.1.9 terraform_wrapper: false - uses: actions/setup-node@v3 diff --git a/cli/server_test.go b/cli/server_test.go index 9ac4a86b14b6b..430be799245a7 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -84,7 +84,6 @@ func TestServer(t *testing.T) { }() require.Eventually(t, func() bool { _, err := cfg.URL().Read() - t.Log(err) return err == nil }, time.Minute, 25*time.Millisecond) cancelFunc() From 6b84a730f7fcdb0dcee93f0fefee86d7052ac17d Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Tue, 21 Jun 2022 16:29:58 +0000 Subject: [PATCH 10/12] fix unit tests --- cli/server.go | 3 +-- provisioner/terraform/executor.go | 7 ++++- provisioner/terraform/serve.go | 24 +++++++++-------- provisioner/terraform/serve_internal_test.go | 27 ++++++++++++-------- 4 files changed, 38 insertions(+), 23 deletions(-) diff --git a/cli/server.go b/cli/server.go index 3230031c4f927..7dc27a340c5cf 100644 --- a/cli/server.go +++ b/cli/server.go @@ -376,7 +376,6 @@ func server() *cobra.Command { shutdownConnsCtx, shutdownConns := context.WithCancel(cmd.Context()) defer shutdownConns() go func() { - defer close(errCh) server := http.Server{ // These errors are typically noise like "TLS: EOF". Vault does similar: // https://github.com/hashicorp/vault/blob/e2490059d0711635e529a4efcbaa1b26998d6e1c/command/server.go#L2714 @@ -590,7 +589,7 @@ func newProvisionerDaemon(ctx context.Context, coderAPI *coderd.API, CachePath: cacheDir, Logger: logger, }) - if err != nil { + if err != nil && !xerrors.Is(err, context.Canceled) { errChan <- err } }() diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go index eab070c2d8034..a08bf75149f25 100644 --- a/provisioner/terraform/executor.go +++ b/provisioner/terraform/executor.go @@ -112,7 +112,12 @@ func versionFromBinaryPath(ctx context.Context, binaryPath string) (*version.Ver cmd := exec.CommandContext(ctx, binaryPath, "version", "-json") out, err := cmd.Output() if err != nil { - return nil, err + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + return nil, err + } } vj := tfjson.VersionOutput{} err = json.Unmarshal(out, &vj) diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index 6c051005a7695..8d076516610a2 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -43,10 +43,10 @@ type ServeOptions struct { Logger slog.Logger } -func getAbsoluteBinaryPath(ctx context.Context) (string, bool) { +func absoluteBinaryPath(ctx context.Context) (string, error) { binaryPath, err := safeexec.LookPath("terraform") if err != nil { - return "", false + return "", xerrors.Errorf("Terraform binary not found: %w", err) } // If the "coder" binary is in the same directory as @@ -56,30 +56,32 @@ func getAbsoluteBinaryPath(ctx context.Context) (string, bool) { // to execute this properly! absoluteBinary, err := filepath.Abs(binaryPath) if err != nil { - return "", false + return "", xerrors.Errorf("Terraform binary absolute path not found: %w", err) } // Checking the installed version of Terraform. version, err := versionFromBinaryPath(ctx, absoluteBinary) if err != nil { - return "", false + return "", xerrors.Errorf("Terraform binary get version failed: %w", err) } if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { - return "", false + return "", xerrors.Errorf("Terraform binary minor version mismatch.") } - return absoluteBinary, true + return absoluteBinary, nil } // Serve starts a dRPC server on the provided transport speaking Terraform provisioner. func Serve(ctx context.Context, options *ServeOptions) error { if options.BinaryPath == "" { - absoluteBinary, ok := getAbsoluteBinaryPath(ctx) + absoluteBinary, err := absoluteBinaryPath(ctx) + + if err != nil { + if xerrors.Is(err, context.Canceled) { + return xerrors.Errorf("absolute binary context canceled: %w", err) + } - if ok { - options.BinaryPath = absoluteBinary - } else { installer := &releases.ExactVersion{ InstallDir: options.CachePath, Product: product.Terraform, @@ -91,6 +93,8 @@ func Serve(ctx context.Context, options *ServeOptions) error { return xerrors.Errorf("install terraform: %w", err) } options.BinaryPath = execPath + } else { + options.BinaryPath = absoluteBinary } } return provisionersdk.Serve(ctx, &server{ diff --git a/provisioner/terraform/serve_internal_test.go b/provisioner/terraform/serve_internal_test.go index 9faf1d52262f1..6e7dc203095fd 100644 --- a/provisioner/terraform/serve_internal_test.go +++ b/provisioner/terraform/serve_internal_test.go @@ -10,10 +10,11 @@ import ( "testing" "github.com/stretchr/testify/require" + "golang.org/x/xerrors" ) // nolint:paralleltest -func Test_getAbsoluteBinaryPath(t *testing.T) { +func Test_absoluteBinaryPath(t *testing.T) { type args struct { ctx context.Context } @@ -21,31 +22,31 @@ func Test_getAbsoluteBinaryPath(t *testing.T) { name string args args terraformVersion string - expectedOk bool + expectedErr error }{ { name: "TestCorrectVersion", args: args{ctx: context.Background()}, terraformVersion: "1.1.9", - expectedOk: true, + expectedErr: nil, }, { name: "TestOldVersion", args: args{ctx: context.Background()}, terraformVersion: "1.0.9", - expectedOk: false, + expectedErr: xerrors.Errorf("Terraform binary minor version mismatch."), }, { name: "TestNewVersion", args: args{ctx: context.Background()}, terraformVersion: "1.2.9", - expectedOk: false, + expectedErr: xerrors.Errorf("Terraform binary minor version mismatch."), }, { name: "TestMalformedVersion", args: args{ctx: context.Background()}, terraformVersion: "version", - expectedOk: false, + expectedErr: xerrors.Errorf("Terraform binary get version failed: Malformed version: version"), }, } // nolint:paralleltest @@ -80,16 +81,22 @@ func Test_getAbsoluteBinaryPath(t *testing.T) { t.Setenv("PATH", strings.Join([]string{tempDir, pathVariable}, ":")) var expectedAbsoluteBinary string - if tt.expectedOk { + if tt.expectedErr == nil { expectedAbsoluteBinary = filepath.Join(tempDir, "terraform") } - actualAbsoluteBinary, actualOk := getAbsoluteBinaryPath(tt.args.ctx) + actualAbsoluteBinary, actualErr := absoluteBinaryPath(tt.args.ctx) if actualAbsoluteBinary != expectedAbsoluteBinary { t.Errorf("getAbsoluteBinaryPath() absoluteBinaryPath, actual = %v, expected %v", actualAbsoluteBinary, expectedAbsoluteBinary) } - if actualOk != tt.expectedOk { - t.Errorf("getAbsoluteBinaryPath() ok, actual = %v, expected %v", actualOk, tt.expectedOk) + if tt.expectedErr == nil { + if actualErr != nil { + t.Errorf("getAbsoluteBinaryPath() error, actual = %v, expected %v", actualErr.Error(), tt.expectedErr) + } + } else { + if actualErr.Error() != tt.expectedErr.Error() { + t.Errorf("getAbsoluteBinaryPath() error, actual = %v, expected %v", actualErr.Error(), tt.expectedErr.Error()) + } } }) } From 7f68d1ea7e7de7fd0b328f74045d5c9c6c0455d1 Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Wed, 22 Jun 2022 22:22:08 +0000 Subject: [PATCH 11/12] remove extra select --- provisioner/terraform/executor.go | 7 +------ provisioner/terraform/serve.go | 4 +++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go index a08bf75149f25..eab070c2d8034 100644 --- a/provisioner/terraform/executor.go +++ b/provisioner/terraform/executor.go @@ -112,12 +112,7 @@ func versionFromBinaryPath(ctx context.Context, binaryPath string) (*version.Ver cmd := exec.CommandContext(ctx, binaryPath, "version", "-json") out, err := cmd.Output() if err != nil { - select { - case <-ctx.Done(): - return nil, ctx.Err() - default: - return nil, err - } + return nil, err } vj := tfjson.VersionOutput{} err = json.Unmarshal(out, &vj) diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index 8d076516610a2..1130b17dc79e9 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -76,8 +76,10 @@ func absoluteBinaryPath(ctx context.Context) (string, error) { func Serve(ctx context.Context, options *ServeOptions) error { if options.BinaryPath == "" { absoluteBinary, err := absoluteBinaryPath(ctx) - if err != nil { + // This is an early exit to prevent extra execution in case the context is canceled. + // It generally happens in unit tests since this method is asynchronous and + // the unit test kills the app before this is complete. if xerrors.Is(err, context.Canceled) { return xerrors.Errorf("absolute binary context canceled: %w", err) } From 0c58641416444df0e1ee2f74f5b527ff49eb6efa Mon Sep 17 00:00:00 2001 From: Abhineet Jain Date: Wed, 22 Jun 2022 22:51:34 +0000 Subject: [PATCH 12/12] simplify tests --- provisioner/terraform/executor.go | 9 ++++++++- provisioner/terraform/serve.go | 4 +++- provisioner/terraform/serve_internal_test.go | 17 ++++++----------- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go index eab070c2d8034..44b6d4d9e8275 100644 --- a/provisioner/terraform/executor.go +++ b/provisioner/terraform/executor.go @@ -112,7 +112,14 @@ func versionFromBinaryPath(ctx context.Context, binaryPath string) (*version.Ver cmd := exec.CommandContext(ctx, binaryPath, "version", "-json") out, err := cmd.Output() if err != nil { - return nil, err + select { + // `exec` library throws a `signal: killed`` error instead of the canceled context. + // Since we know the cause for the killed signal, we are throwing the relevant error here. + case <-ctx.Done(): + return nil, ctx.Err() + default: + return nil, err + } } vj := tfjson.VersionOutput{} err = json.Unmarshal(out, &vj) diff --git a/provisioner/terraform/serve.go b/provisioner/terraform/serve.go index 1130b17dc79e9..4e0dd87189302 100644 --- a/provisioner/terraform/serve.go +++ b/provisioner/terraform/serve.go @@ -33,6 +33,8 @@ var ( }() ) +var terraformMinorVersionMismatch = xerrors.New("Terraform binary minor version mismatch.") + type ServeOptions struct { *provisionersdk.ServeOptions @@ -66,7 +68,7 @@ func absoluteBinaryPath(ctx context.Context) (string, error) { } if version.LessThan(minTerraformVersion) || version.GreaterThanOrEqual(maxTerraformVersion) { - return "", xerrors.Errorf("Terraform binary minor version mismatch.") + return "", terraformMinorVersionMismatch } return absoluteBinary, nil diff --git a/provisioner/terraform/serve_internal_test.go b/provisioner/terraform/serve_internal_test.go index 6e7dc203095fd..9daebf57225c3 100644 --- a/provisioner/terraform/serve_internal_test.go +++ b/provisioner/terraform/serve_internal_test.go @@ -34,13 +34,13 @@ func Test_absoluteBinaryPath(t *testing.T) { name: "TestOldVersion", args: args{ctx: context.Background()}, terraformVersion: "1.0.9", - expectedErr: xerrors.Errorf("Terraform binary minor version mismatch."), + expectedErr: terraformMinorVersionMismatch, }, { name: "TestNewVersion", args: args{ctx: context.Background()}, terraformVersion: "1.2.9", - expectedErr: xerrors.Errorf("Terraform binary minor version mismatch."), + expectedErr: terraformMinorVersionMismatch, }, { name: "TestMalformedVersion", @@ -86,17 +86,12 @@ func Test_absoluteBinaryPath(t *testing.T) { } actualAbsoluteBinary, actualErr := absoluteBinaryPath(tt.args.ctx) - if actualAbsoluteBinary != expectedAbsoluteBinary { - t.Errorf("getAbsoluteBinaryPath() absoluteBinaryPath, actual = %v, expected %v", actualAbsoluteBinary, expectedAbsoluteBinary) - } + + require.Equal(t, expectedAbsoluteBinary, actualAbsoluteBinary) if tt.expectedErr == nil { - if actualErr != nil { - t.Errorf("getAbsoluteBinaryPath() error, actual = %v, expected %v", actualErr.Error(), tt.expectedErr) - } + require.NoError(t, actualErr) } else { - if actualErr.Error() != tt.expectedErr.Error() { - t.Errorf("getAbsoluteBinaryPath() error, actual = %v, expected %v", actualErr.Error(), tt.expectedErr.Error()) - } + require.EqualError(t, actualErr, tt.expectedErr.Error()) } }) }