From 6dd067ea95fdd277af0a7d44add5bb31f732e835 Mon Sep 17 00:00:00 2001 From: deansheather Date: Tue, 14 Jun 2022 22:51:26 +0000 Subject: [PATCH 1/3] feat: improve terraform template parsing errors --- go.mod | 2 +- provisioner/terraform/parse.go | 56 +++++++++++++++++++++++++++--- provisioner/terraform/provision.go | 47 ++++++++++++------------- provisionerd/provisionerd.go | 9 +++-- 4 files changed, 81 insertions(+), 33 deletions(-) diff --git a/go.mod b/go.mod index b4a05c6ce1478..4e0fbeff80c82 100644 --- a/go.mod +++ b/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 diff --git a/provisioner/terraform/parse.go b/provisioner/terraform/parse.go index 6c8abe6f0aee1..4341124257220 100644 --- a/provisioner/terraform/parse.go +++ b/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() { - _ = stream.CloseSend() - }() - + // Load the module and print any parse errors. 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()) +} diff --git a/provisioner/terraform/provision.go b/provisioner/terraform/provision.go index ec42f4b28cead..43081930cea4d 100644 --- a/provisioner/terraform/provision.go +++ b/provisioner/terraform/provision.go @@ -70,6 +70,26 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro return xerrors.Errorf("terraform version %q is too old. required >= %q", version.String(), minimumTerraformVersion.String()) } + terraformEnv := map[string]string{} + // Required for "terraform init" to find "git" to + // clone Terraform modules. + for _, env := range os.Environ() { + parts := strings.SplitN(env, "=", 2) + if len(parts) < 2 { + continue + } + terraformEnv[parts[0]] = parts[1] + } + // Only Linux reliably works with the Terraform plugin + // cache directory. It's unknown why this is. + if t.cachePath != "" && runtime.GOOS == "linux" { + terraformEnv["TF_PLUGIN_CACHE_DIR"] = t.cachePath + } + err = terraform.SetEnv(terraformEnv) + if err != nil { + return xerrors.Errorf("set terraform env: %w", err) + } + statefilePath := filepath.Join(start.Directory, "terraform.tfstate") if len(start.State) > 0 { err := os.WriteFile(statefilePath, start.State, 0600) @@ -87,41 +107,20 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro _ = stream.Send(&proto.Provision_Response{ Type: &proto.Provision_Response_Log{ Log: &proto.Log{ - Level: proto.LogLevel_DEBUG, + Level: proto.LogLevel_ERROR, Output: scanner.Text(), }, }, }) } }() - terraformEnv := map[string]string{} - // Required for "terraform init" to find "git" to - // clone Terraform modules. - for _, env := range os.Environ() { - parts := strings.SplitN(env, "=", 2) - if len(parts) < 2 { - continue - } - terraformEnv[parts[0]] = parts[1] - } - // Only Linux reliably works with the Terraform plugin - // cache directory. It's unknown why this is. - if t.cachePath != "" && runtime.GOOS == "linux" { - terraformEnv["TF_PLUGIN_CACHE_DIR"] = t.cachePath - } - err = terraform.SetEnv(terraformEnv) - if err != nil { - return xerrors.Errorf("set terraform env: %w", err) - } - terraform.SetStdout(writer) - t.logger.Debug(shutdown, "running initialization") + terraform.SetStderr(writer) err = terraform.Init(shutdown) if err != nil { return xerrors.Errorf("initialize terraform: %w", err) } - t.logger.Debug(shutdown, "ran initialization") _ = reader.Close() - terraform.SetStdout(io.Discard) + terraform.SetStderr(io.Discard) env := os.Environ() env = append(env, diff --git a/provisionerd/provisionerd.go b/provisionerd/provisionerd.go index 4717dd7905c3a..b17a730681923 100644 --- a/provisionerd/provisionerd.go +++ b/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{{ From ca01fcccb2e8fe1c84e2d7952293212bd95aa661 Mon Sep 17 00:00:00 2001 From: deansheather Date: Tue, 14 Jun 2022 23:40:03 +0000 Subject: [PATCH 2/3] chore: add tests for parse failures --- provisioner/terraform/parse_test.go | 142 +++++++++------- provisioner/terraform/provision_test.go | 211 ++++++++++++------------ 2 files changed, 192 insertions(+), 161 deletions(-) diff --git a/provisioner/terraform/parse_test.go b/provisioner/terraform/parse_test.go index 3f7680aced636..360ce6c627eea 100644 --- a/provisioner/terraform/parse_test.go +++ b/provisioner/terraform/parse_test.go @@ -38,83 +38,99 @@ 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 - }{{ - Name: "single-variable", - Files: map[string]string{ - "main.tf": `variable "A" { + // 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" { description = "Testing!" }`, - }, - Response: &proto.Parse_Response{ - Type: &proto.Parse_Response_Complete{ - Complete: &proto.Parse_Complete{ - ParameterSchemas: []*proto.ParameterSchema{{ - Name: "A", - RedisplayValue: true, - AllowOverrideSource: true, - Description: "Testing!", - DefaultDestination: &proto.ParameterDestination{ - Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, - }, - }}, + }, + Response: &proto.Parse_Response{ + Type: &proto.Parse_Response_Complete{ + Complete: &proto.Parse_Complete{ + ParameterSchemas: []*proto.ParameterSchema{{ + Name: "A", + RedisplayValue: true, + AllowOverrideSource: true, + Description: "Testing!", + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }}, + }, }, }, }, - }, { - Name: "default-variable-value", - Files: map[string]string{ - "main.tf": `variable "A" { + { + Name: "default-variable-value", + Files: map[string]string{ + "main.tf": `variable "A" { default = "wow" }`, - }, - Response: &proto.Parse_Response{ - Type: &proto.Parse_Response_Complete{ - Complete: &proto.Parse_Complete{ - ParameterSchemas: []*proto.ParameterSchema{{ - Name: "A", - RedisplayValue: true, - AllowOverrideSource: true, - DefaultSource: &proto.ParameterSource{ - Scheme: proto.ParameterSource_DATA, - Value: "wow", - }, - DefaultDestination: &proto.ParameterDestination{ - Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, - }, - }}, + }, + Response: &proto.Parse_Response{ + Type: &proto.Parse_Response_Complete{ + Complete: &proto.Parse_Complete{ + ParameterSchemas: []*proto.ParameterSchema{{ + Name: "A", + RedisplayValue: true, + AllowOverrideSource: true, + DefaultSource: &proto.ParameterSource{ + Scheme: proto.ParameterSource_DATA, + Value: "wow", + }, + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }}, + }, }, }, }, - }, { - Name: "variable-validation", - Files: map[string]string{ - "main.tf": `variable "A" { + { + Name: "variable-validation", + Files: map[string]string{ + "main.tf": `variable "A" { validation { condition = var.A == "value" } }`, - }, - Response: &proto.Parse_Response{ - Type: &proto.Parse_Response_Complete{ - Complete: &proto.Parse_Complete{ - ParameterSchemas: []*proto.ParameterSchema{{ - Name: "A", - RedisplayValue: true, - ValidationCondition: `var.A == "value"`, - ValidationTypeSystem: proto.ParameterSchema_HCL, - AllowOverrideSource: true, - DefaultDestination: &proto.ParameterDestination{ - Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, - }, - }}, + }, + Response: &proto.Parse_Response{ + Type: &proto.Parse_Response_Complete{ + Complete: &proto.Parse_Complete{ + ParameterSchemas: []*proto.ParameterSchema{{ + Name: "A", + RedisplayValue: true, + ValidationCondition: `var.A == "value"`, + ValidationTypeSystem: proto.ParameterSchema_HCL, + AllowOverrideSource: true, + DefaultDestination: &proto.ParameterDestination{ + Scheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + }, + }}, + }, }, }, }, - }} { + { + 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() - require.NoError(t, err) + 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) diff --git a/provisioner/terraform/provision_test.go b/provisioner/terraform/provision_test.go index 9e381c63dab6e..11926a5098bf8 100644 --- a/provisioner/terraform/provision_test.go +++ b/provisioner/terraform/provision_test.go @@ -58,70 +58,108 @@ provider "coder" { }() api := proto.NewDRPCProvisionerClient(provisionersdk.Conn(client)) - for _, testCase := range []struct { - Name string - Files map[string]string - Request *proto.Provision_Request + 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 - }{{ - Name: "single-variable", - Files: map[string]string{ - "main.tf": `variable "A" { + // 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" { description = "Testing!" }`, - }, - Request: &proto.Provision_Request{ - Type: &proto.Provision_Request_Start{ - Start: &proto.Provision_Start{ - ParameterValues: []*proto.ParameterValue{{ - DestinationScheme: proto.ParameterDestination_PROVISIONER_VARIABLE, - Name: "A", - Value: "example", - }}, + }, + Request: &proto.Provision_Request{ + Type: &proto.Provision_Request_Start{ + Start: &proto.Provision_Start{ + ParameterValues: []*proto.ParameterValue{{ + DestinationScheme: proto.ParameterDestination_PROVISIONER_VARIABLE, + Name: "A", + Value: "example", + }}, + }, }, }, - }, - Response: &proto.Provision_Response{ - Type: &proto.Provision_Response_Complete{ - Complete: &proto.Provision_Complete{}, + Response: &proto.Provision_Response{ + Type: &proto.Provision_Response_Complete{ + Complete: &proto.Provision_Complete{}, + }, }, }, - }, { - Name: "missing-variable", - Files: map[string]string{ - "main.tf": `variable "A" { + { + Name: "missing-variable", + Files: map[string]string{ + "main.tf": `variable "A" { }`, + }, + Response: &proto.Provision_Response{ + Type: &proto.Provision_Response_Complete{ + Complete: &proto.Provision_Complete{ + Error: "exit status 1", + }, + }, + }, }, - Response: &proto.Provision_Response{ - Type: &proto.Provision_Response_Complete{ - Complete: &proto.Provision_Complete{ - Error: "exit status 1", + { + Name: "single-resource", + Files: map[string]string{ + "main.tf": `resource "null_resource" "A" {}`, + }, + Response: &proto.Provision_Response{ + Type: &proto.Provision_Response_Complete{ + Complete: &proto.Provision_Complete{ + Resources: []*proto.Resource{{ + Name: "A", + Type: "null_resource", + }}, + }, }, }, }, - }, { - Name: "single-resource", - Files: map[string]string{ - "main.tf": `resource "null_resource" "A" {}`, + { + Name: "bad-syntax-1", + Files: map[string]string{ + "main.tf": `a`, + }, + ErrorContains: "configuration is invalid", + ExpectLogContains: "Argument or block definition required", }, - Response: &proto.Provision_Response{ - Type: &proto.Provision_Response_Complete{ - Complete: &proto.Provision_Complete{ - Resources: []*proto.Resource{{ - Name: "A", - Type: "null_resource", - }}, - }, + { + Name: "bad-syntax-2", + Files: map[string]string{ + "main.tf": `;asdf;`, }, + ErrorContains: "configuration is invalid", + ExpectLogContains: `The ";" character is not valid.`, }, - }, { - Name: "invalid-sourcecode", - Files: map[string]string{ - "main.tf": `a`, + { + 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", }, - Error: true, - }} { + } + + 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 { - require.Error(t, err) - return + if testCase.ErrorContains != "" { + require.ErrorContains(t, err, testCase.ErrorContains) + break } require.NoError(t, err) @@ -185,63 +230,23 @@ provider "coder" { } } - resourcesGot, err := json.Marshal(msg.GetComplete().Resources) - require.NoError(t, err) + if testCase.Response != nil { + resourcesGot, err := json.Marshal(msg.GetComplete().Resources) + require.NoError(t, err) - resourcesWant, err := json.Marshal(testCase.Response.GetComplete().Resources) - require.NoError(t, err) + resourcesWant, err := json.Marshal(testCase.Response.GetComplete().Resources) + require.NoError(t, err) - require.Equal(t, testCase.Response.GetComplete().Error, msg.GetComplete().Error) + require.Equal(t, testCase.Response.GetComplete().Error, msg.GetComplete().Error) - require.Equal(t, string(resourcesWant), string(resourcesGot)) + require.Equal(t, string(resourcesWant), string(resourcesGot)) + } break } - }) - } - - 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 !gotExpectedLog { + t.Fatalf("expected log string %q but never saw it", testCase.ExpectLogContains) } - if msg.GetComplete() == nil { - continue - } - - require.Empty(t, msg.GetComplete().Error) - require.True(t, gotLog, "never received 'nothing to do' log") - break - } - }) + }) + } } From c1673d1c8082b2fc4a44c797129a9e0452178141 Mon Sep 17 00:00:00 2001 From: deansheather Date: Tue, 14 Jun 2022 23:47:16 +0000 Subject: [PATCH 3/3] fixup! chore: add tests for parse failures --- provisioner/terraform/provision.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/provisioner/terraform/provision.go b/provisioner/terraform/provision.go index 43081930cea4d..08d632f6b8a1d 100644 --- a/provisioner/terraform/provision.go +++ b/provisioner/terraform/provision.go @@ -99,9 +99,7 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro } reader, writer := io.Pipe() - defer reader.Close() - defer writer.Close() - go func() { + go func(reader *io.PipeReader) { scanner := bufio.NewScanner(reader) for scanner.Scan() { _ = stream.Send(&proto.Provision_Response{ @@ -113,13 +111,15 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro }, }) } - }() + }(reader) + terraform.SetStderr(writer) err = terraform.Init(shutdown) + _ = reader.Close() + _ = writer.Close() if err != nil { return xerrors.Errorf("initialize terraform: %w", err) } - _ = reader.Close() terraform.SetStderr(io.Discard) env := os.Environ()