feat: improve terraform template parsing errors (#2331)

go.mod

@@ -81,6 +81,7 @@ require (
 	github.com/kirsle/configdir v0.0.0-20170128060238-e45d2f54772f
 	github.com/lib/pq v1.10.6
 	github.com/mattn/go-isatty v0.0.14
+	github.com/mitchellh/go-wordwrap v1.0.1
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/moby/moby v20.10.17+incompatible
 	github.com/open-policy-agent/opa v0.41.0
@@ -190,7 +191,6 @@ require (
 	github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
 	github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b // indirect
 	github.com/miekg/dns v1.1.45 // indirect
-	github.com/mitchellh/go-wordwrap v1.0.1 // indirect
 	github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 // indirect
 	github.com/muesli/ansi v0.0.0-20211031195517-c9f0611b6c70 // indirect
 	github.com/muesli/reflow v0.3.0 // indirect

provisioner/terraform/parse.go

@@ -2,9 +2,13 @@ package terraform
 
 import (
 	"encoding/json"
+	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 
 	"github.com/hashicorp/terraform-config-inspect/tfconfig"
+	"github.com/mitchellh/go-wordwrap"
 	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/provisionersdk/proto"
@@ -12,14 +16,12 @@ import (
 
 // Parse extracts Terraform variables from source-code.
 func (*terraform) Parse(request *proto.Parse_Request, stream proto.DRPCProvisioner_ParseStream) error {
-	defer func() {
+	// Load the module and print any parse errors.
-		_ = stream.CloseSend()
-	}()
-
 	module, diags := tfconfig.LoadModule(request.Directory)
 	if diags.HasErrors() {
-		return xerrors.Errorf("load module: %w", diags.Err())
+		return xerrors.Errorf("load module: %s", formatDiagnostics(request.Directory, diags))
 	}
+
 	parameters := make([]*proto.ParameterSchema, 0, len(module.Variables))
 	for _, v := range module.Variables {
 		schema, err := convertVariableToParameter(v)
@@ -83,3 +85,47 @@ func convertVariableToParameter(variable *tfconfig.Variable) (*proto.ParameterSc
 
 	return schema, nil
 }
+
+// formatDiagnostics returns a nicely formatted string containing all of the
+// error details within the tfconfig.Diagnostics. We need to use this because
+// the default format doesn't provide much useful information.
+func formatDiagnostics(baseDir string, diags tfconfig.Diagnostics) string {
+	var msgs strings.Builder
+	for _, d := range diags {
+		// Convert severity.
+		severity := "UNKNOWN SEVERITY"
+		switch {
+		case d.Severity == tfconfig.DiagError:
+			severity = "ERROR"
+		case d.Severity == tfconfig.DiagWarning:
+			severity = "WARN"
+		}
+
+		// Determine filepath and line
+		location := "unknown location"
+		if d.Pos != nil {
+			filename, err := filepath.Rel(baseDir, d.Pos.Filename)
+			if err != nil {
+				filename = d.Pos.Filename
+			}
+			location = fmt.Sprintf("%s:%d", filename, d.Pos.Line)
+		}
+
+		_, _ = msgs.WriteString(fmt.Sprintf("\n%s: %s (%s)\n", severity, d.Summary, location))
+
+		// Wrap the details to 80 characters and indent them.
+		if d.Detail != "" {
+			wrapped := wordwrap.WrapString(d.Detail, 78)
+			for _, line := range strings.Split(wrapped, "\n") {
+				_, _ = msgs.WriteString(fmt.Sprintf("> %s\n", line))
+			}
+		}
+	}
+
+	spacer := " "
+	if len(diags) > 1 {
+		spacer = "\n\n"
+	}
+
+	return spacer + strings.TrimSpace(msgs.String())
+}

provisioner/terraform/parse_test.go

@@ -38,11 +38,15 @@ func TestParse(t *testing.T) {
 	}()
 	api := proto.NewDRPCProvisionerClient(provisionersdk.Conn(client))
 
-	for _, testCase := range []struct {
+	testCases := []struct {
 		Name     string
 		Files    map[string]string
 		Response *proto.Parse_Response
-	}{{
+		// If ErrorContains is not empty, then response.Recv() should return an
+		// error containing this string before a Complete response is returned.
+		ErrorContains string
+	}{
+		{
 			Name: "single-variable",
 			Files: map[string]string{
 				"main.tf": `variable "A" {
@@ -64,7 +68,8 @@ func TestParse(t *testing.T) {
 					},
 				},
 			},
-	}, {
+		},
+		{
 			Name: "default-variable-value",
 			Files: map[string]string{
 				"main.tf": `variable "A" {
@@ -89,7 +94,8 @@ func TestParse(t *testing.T) {
 					},
 				},
 			},
-	}, {
+		},
+		{
 			Name: "variable-validation",
 			Files: map[string]string{
 				"main.tf": `variable "A" {
@@ -114,7 +120,17 @@ func TestParse(t *testing.T) {
 					},
 				},
 			},
-	}} {
+		},
+		{
+			Name: "bad-syntax",
+			Files: map[string]string{
+				"main.tf": "a;sd;ajsd;lajsd;lasjdf;a",
+			},
+			ErrorContains: `The ";" character is not valid.`,
+		},
+	}
+
+	for _, testCase := range testCases {
 		testCase := testCase
 		t.Run(testCase.Name, func(t *testing.T) {
 			t.Parallel()
@@ -133,11 +149,21 @@ func TestParse(t *testing.T) {
 
 			for {
 				msg, err := response.Recv()
+				if err != nil {
+					if testCase.ErrorContains != "" {
+						require.ErrorContains(t, err, testCase.ErrorContains)
+						break
+					}
+
 					require.NoError(t, err)
+				}
 
 				if msg.GetComplete() == nil {
 					continue
 				}
+				if testCase.ErrorContains != "" {
+					t.Fatal("expected error but job completed successfully")
+				}
 
 				// Ensure the want and got are equivalent!
 				want, err := json.Marshal(testCase.Response)

provisioner/terraform/provision.go

@@ -70,30 +70,6 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro
 		return xerrors.Errorf("terraform version %q is too old. required >= %q", version.String(), minimumTerraformVersion.String())
 	}
 
-	statefilePath := filepath.Join(start.Directory, "terraform.tfstate")
-	if len(start.State) > 0 {
-		err := os.WriteFile(statefilePath, start.State, 0600)
-		if err != nil {
-			return xerrors.Errorf("write statefile %q: %w", statefilePath, err)
-		}
-	}
-
-	reader, writer := io.Pipe()
-	defer reader.Close()
-	defer writer.Close()
-	go func() {
-		scanner := bufio.NewScanner(reader)
-		for scanner.Scan() {
-			_ = stream.Send(&proto.Provision_Response{
-				Type: &proto.Provision_Response_Log{
-					Log: &proto.Log{
-						Level:  proto.LogLevel_DEBUG,
-						Output: scanner.Text(),
-					},
-				},
-			})
-		}
-	}()
 	terraformEnv := map[string]string{}
 	// Required for "terraform init" to find "git" to
 	// clone Terraform modules.
@@ -113,15 +89,38 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro
 	if err != nil {
 		return xerrors.Errorf("set terraform env: %w", err)
 	}
-	terraform.SetStdout(writer)
+
-	t.logger.Debug(shutdown, "running initialization")
+	statefilePath := filepath.Join(start.Directory, "terraform.tfstate")
+	if len(start.State) > 0 {
+		err := os.WriteFile(statefilePath, start.State, 0600)
+		if err != nil {
+			return xerrors.Errorf("write statefile %q: %w", statefilePath, err)
+		}
+	}
+
+	reader, writer := io.Pipe()
+	go func(reader *io.PipeReader) {
+		scanner := bufio.NewScanner(reader)
+		for scanner.Scan() {
+			_ = stream.Send(&proto.Provision_Response{
+				Type: &proto.Provision_Response_Log{
+					Log: &proto.Log{
+						Level:  proto.LogLevel_ERROR,
+						Output: scanner.Text(),
+					},
+				},
+			})
+		}
+	}(reader)
+
+	terraform.SetStderr(writer)
 	err = terraform.Init(shutdown)
+	_ = reader.Close()
+	_ = writer.Close()
 	if err != nil {
 		return xerrors.Errorf("initialize terraform: %w", err)
 	}
-	t.logger.Debug(shutdown, "ran initialization")
+	terraform.SetStderr(io.Discard)
-	_ = reader.Close()
-	terraform.SetStdout(io.Discard)
 
 	env := os.Environ()
 	env = append(env,

provisioner/terraform/provision_test.go

@@ -58,13 +58,19 @@ provider "coder" {
 	}()
 	api := proto.NewDRPCProvisionerClient(provisionersdk.Conn(client))
 
-	for _, testCase := range []struct {
+	testCases := []struct {
 		Name    string
 		Files   map[string]string
 		Request *proto.Provision_Request
+		// Response may be nil to not check the response.
 		Response *proto.Provision_Response
-		Error    bool
+		// If ErrorContains is not empty, then response.Recv() should return an
-	}{{
+		// error containing this string before a Complete response is returned.
+		ErrorContains string
+		// If ExpectLogContains is not empty, then the logs should contain it.
+		ExpectLogContains string
+	}{
+		{
 			Name: "single-variable",
 			Files: map[string]string{
 				"main.tf": `variable "A" {
@@ -87,7 +93,8 @@ provider "coder" {
 					Complete: &proto.Provision_Complete{},
 				},
 			},
-	}, {
+		},
+		{
 			Name: "missing-variable",
 			Files: map[string]string{
 				"main.tf": `variable "A" {
@@ -100,7 +107,8 @@ provider "coder" {
 					},
 				},
 			},
-	}, {
+		},
+		{
 			Name: "single-resource",
 			Files: map[string]string{
 				"main.tf": `resource "null_resource" "A" {}`,
@@ -115,13 +123,43 @@ provider "coder" {
 					},
 				},
 			},
-	}, {
+		},
-		Name: "invalid-sourcecode",
+		{
+			Name: "bad-syntax-1",
 			Files: map[string]string{
 				"main.tf": `a`,
 			},
-		Error: true,
+			ErrorContains:     "configuration is invalid",
-	}} {
+			ExpectLogContains: "Argument or block definition required",
+		},
+		{
+			Name: "bad-syntax-2",
+			Files: map[string]string{
+				"main.tf": `;asdf;`,
+			},
+			ErrorContains:     "configuration is invalid",
+			ExpectLogContains: `The ";" character is not valid.`,
+		},
+		{
+			Name: "destroy-no-state",
+			Files: map[string]string{
+				"main.tf": `resource "null_resource" "A" {}`,
+			},
+			Request: &proto.Provision_Request{
+				Type: &proto.Provision_Request_Start{
+					Start: &proto.Provision_Start{
+						State: nil,
+						Metadata: &proto.Provision_Metadata{
+							WorkspaceTransition: proto.WorkspaceTransition_DESTROY,
+						},
+					},
+				},
+			},
+			ExpectLogContains: "nothing to do",
+		},
+	}
+
+	for _, testCase := range testCases {
 		testCase := testCase
 		t.Run(testCase.Name, func(t *testing.T) {
 			t.Parallel()
@@ -148,19 +186,26 @@ provider "coder" {
 			if request.GetStart().Metadata == nil {
 				request.GetStart().Metadata = &proto.Provision_Metadata{}
 			}
+
 			response, err := api.Provision(ctx)
 			require.NoError(t, err)
 			err = response.Send(request)
 			require.NoError(t, err)
+
+			gotExpectedLog := testCase.ExpectLogContains == ""
 			for {
 				msg, err := response.Recv()
 				if msg != nil && msg.GetLog() != nil {
+					if testCase.ExpectLogContains != "" && strings.Contains(msg.GetLog().Output, testCase.ExpectLogContains) {
+						gotExpectedLog = true
+					}
+
 					t.Logf("log: [%s] %s", msg.GetLog().Level, msg.GetLog().Output)
 					continue
 				}
-				if testCase.Error {
+				if testCase.ErrorContains != "" {
-					require.Error(t, err)
+					require.ErrorContains(t, err, testCase.ErrorContains)
-					return
+					break
 				}
 				require.NoError(t, err)
 
@@ -185,6 +230,7 @@ provider "coder" {
 					}
 				}
 
+				if testCase.Response != nil {
 					resourcesGot, err := json.Marshal(msg.GetComplete().Resources)
 					require.NoError(t, err)
 
@@ -194,54 +240,13 @@ provider "coder" {
 					require.Equal(t, testCase.Response.GetComplete().Error, msg.GetComplete().Error)
 
 					require.Equal(t, string(resourcesWant), string(resourcesGot))
+				}
 				break
 			}
+
+			if !gotExpectedLog {
+				t.Fatalf("expected log string %q but never saw it", testCase.ExpectLogContains)
+			}
 		})
 	}
-
-	t.Run("DestroyNoState", func(t *testing.T) {
-		t.Parallel()
-
-		const template = `resource "null_resource" "A" {}`
-
-		directory := t.TempDir()
-		err := os.WriteFile(filepath.Join(directory, "main.tf"), []byte(template), 0600)
-		require.NoError(t, err)
-
-		request := &proto.Provision_Request{
-			Type: &proto.Provision_Request_Start{
-				Start: &proto.Provision_Start{
-					State:     nil,
-					Directory: directory,
-					Metadata: &proto.Provision_Metadata{
-						WorkspaceTransition: proto.WorkspaceTransition_DESTROY,
-					},
-				},
-			},
-		}
-
-		response, err := api.Provision(ctx)
-		require.NoError(t, err)
-		err = response.Send(request)
-		require.NoError(t, err)
-
-		gotLog := false
-		for {
-			msg, err := response.Recv()
-			require.NoError(t, err)
-			require.NotNil(t, msg)
-
-			if msg.GetLog() != nil && strings.Contains(msg.GetLog().Output, "nothing to do") {
-				gotLog = true
-				continue
-			}
-			if msg.GetComplete() == nil {
-				continue
-			}
-
-			require.Empty(t, msg.GetComplete().Error)
-			require.True(t, gotLog, "never received 'nothing to do' log")
-			break
-		}
-	})
 }

provisionerd/provisionerd.go

@@ -510,12 +510,14 @@ func (p *Server) runTemplateImport(ctx, shutdown context.Context, provisioner sd
 		p.failActiveJobf("client disconnected")
 		return
 	}
+
+	// Parse parameters and update the job with the parameter specs
 	_, err := client.UpdateJob(ctx, &proto.UpdateJobRequest{
 		JobId: job.GetJobId(),
 		Logs: []*proto.Log{{
 			Source:    proto.LogSource_PROVISIONER_DAEMON,
 			Level:     sdkproto.LogLevel_INFO,
-			Stage:     "Parse parameters",
+			Stage:     "Parsing template parameters",
 			CreatedAt: time.Now().UTC().UnixMilli(),
 		}},
 	})
@@ -523,13 +525,11 @@ func (p *Server) runTemplateImport(ctx, shutdown context.Context, provisioner sd
 		p.failActiveJobf("write log: %s", err)
 		return
 	}
-
 	parameterSchemas, err := p.runTemplateImportParse(ctx, provisioner, job)
 	if err != nil {
 		p.failActiveJobf("run parse: %s", err)
 		return
 	}
-
 	updateResponse, err := client.UpdateJob(ctx, &proto.UpdateJobRequest{
 		JobId:            job.JobId,
 		ParameterSchemas: parameterSchemas,
@@ -551,6 +551,7 @@ func (p *Server) runTemplateImport(ctx, shutdown context.Context, provisioner sd
 		}
 	}
 
+	// Determine persistent resources
 	_, err = client.UpdateJob(ctx, &proto.UpdateJobRequest{
 		JobId: job.GetJobId(),
 		Logs: []*proto.Log{{
@@ -572,6 +573,8 @@ func (p *Server) runTemplateImport(ctx, shutdown context.Context, provisioner sd
 		p.failActiveJobf("template import provision for start: %s", err)
 		return
 	}
+
+	// Determine ephemeral resources.
 	_, err = client.UpdateJob(ctx, &proto.UpdateJobRequest{
 		JobId: job.GetJobId(),
 		Logs: []*proto.Log{{