diff --git a/go.mod b/go.mod index b4a05c6ce1..4e0fbeff80 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 6c8abe6f0a..4341124257 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/parse_test.go b/provisioner/terraform/parse_test.go index 3f7680aced..360ce6c627 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.go b/provisioner/terraform/provision.go index ec42f4b28c..08d632f6b8 100644 --- a/provisioner/terraform/provision.go +++ b/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") - _ = reader.Close() - terraform.SetStdout(io.Discard) + terraform.SetStderr(io.Discard) env := os.Environ() env = append(env, diff --git a/provisioner/terraform/provision_test.go b/provisioner/terraform/provision_test.go index 9e381c63da..11926a5098 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" {}`, + }, + Response: &proto.Provision_Response{ + Type: &proto.Provision_Response_Complete{ + Complete: &proto.Provision_Complete{ + Resources: []*proto.Resource{{ + Name: "A", + Type: "null_resource", + }}, + }, }, }, }, - }, { - Name: "invalid-sourcecode", - Files: map[string]string{ - "main.tf": `a`, + { + Name: "bad-syntax-1", + Files: map[string]string{ + "main.tf": `a`, + }, + ErrorContains: "configuration is invalid", + ExpectLogContains: "Argument or block definition required", }, - Error: true, - }} { + { + 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 { - 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 } + + 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 - } - }) } diff --git a/provisionerd/provisionerd.go b/provisionerd/provisionerd.go index 4717dd7905..b17a730681 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{{