diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index bda422ad4b840..f71b630858013 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -26,6 +26,45 @@ type parameterValue struct { Source parameterValueSource } +type ResolverError struct { + Diagnostics hcl.Diagnostics + Parameter map[string]hcl.Diagnostics +} + +// Error is a pretty bad format for these errors. Try to avoid using this. +func (e *ResolverError) Error() string { + var diags hcl.Diagnostics + diags = diags.Extend(e.Diagnostics) + for _, d := range e.Parameter { + diags = diags.Extend(d) + } + + return diags.Error() +} + +func (e *ResolverError) HasError() bool { + if e.Diagnostics.HasErrors() { + return true + } + + for _, diags := range e.Parameter { + if diags.HasErrors() { + return true + } + } + return false +} + +func (e *ResolverError) Extend(parameterName string, diag hcl.Diagnostics) { + if e.Parameter == nil { + e.Parameter = make(map[string]hcl.Diagnostics) + } + if _, ok := e.Parameter[parameterName]; !ok { + e.Parameter[parameterName] = hcl.Diagnostics{} + } + e.Parameter[parameterName] = e.Parameter[parameterName].Extend(diag) +} + //nolint:revive // firstbuild is a control flag to turn on immutable validation func ResolveParameters( ctx context.Context, @@ -35,7 +74,7 @@ func ResolveParameters( previousValues []database.WorkspaceBuildParameter, buildValues []codersdk.WorkspaceBuildParameter, presetValues []database.TemplateVersionPresetParameter, -) (map[string]string, hcl.Diagnostics) { +) (map[string]string, error) { previousValuesMap := slice.ToMapFunc(previousValues, func(p database.WorkspaceBuildParameter) (string, string) { return p.Name, p.Value }) @@ -73,7 +112,10 @@ func ResolveParameters( // always be valid. If there is a case where this is not true, then this has to // be changed to allow the build to continue with a different set of values. - return nil, diags + return nil, &ResolverError{ + Diagnostics: diags, + Parameter: nil, + } } // The user's input now needs to be validated against the parameters. @@ -113,12 +155,16 @@ func ResolveParameters( // are fatal. Additional validation for immutability has to be done manually. output, diags = renderer.Render(ctx, ownerID, values.ValuesMap()) if diags.HasErrors() { - return nil, diags + return nil, &ResolverError{ + Diagnostics: diags, + Parameter: nil, + } } // parameterNames is going to be used to remove any excess values that were left // around without a parameter. parameterNames := make(map[string]struct{}, len(output.Parameters)) + parameterError := &ResolverError{} for _, parameter := range output.Parameters { parameterNames[parameter.Name] = struct{}{} @@ -132,20 +178,22 @@ func ResolveParameters( } // An immutable parameter was changed, which is not allowed. - // Add the failed diagnostic to the output. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Immutable parameter changed", - Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name), - Subject: src, + // Add a failed diagnostic to the output. + parameterError.Extend(parameter.Name, hcl.Diagnostics{ + &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Immutable parameter changed", + Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name), + Subject: src, + }, }) } } // TODO: Fix the `hcl.Diagnostics(...)` type casting. It should not be needed. if hcl.Diagnostics(parameter.Diagnostics).HasErrors() { - // All validation errors are raised here. - diags = diags.Extend(hcl.Diagnostics(parameter.Diagnostics)) + // All validation errors are raised here for each parameter. + parameterError.Extend(parameter.Name, hcl.Diagnostics(parameter.Diagnostics)) } // If the parameter has a value, but it was not set explicitly by the user at any @@ -174,8 +222,13 @@ func ResolveParameters( } } + if parameterError.HasError() { + // If there are any errors, return them. + return nil, parameterError + } + // Return the values to be saved for the build. - return values.ValuesMap(), diags + return values.ValuesMap(), nil } type parameterValueMap map[string]parameterValue diff --git a/coderd/httpapi/httperror/doc.go b/coderd/httpapi/httperror/doc.go new file mode 100644 index 0000000000000..01a0b3956e3e7 --- /dev/null +++ b/coderd/httpapi/httperror/doc.go @@ -0,0 +1,4 @@ +// Package httperror handles formatting and writing some sentinel errors returned +// within coder to the API. +// This package exists outside httpapi to avoid some cyclic dependencies +package httperror diff --git a/coderd/httpapi/httperror/wsbuild.go b/coderd/httpapi/httperror/wsbuild.go new file mode 100644 index 0000000000000..17436b55d5ae0 --- /dev/null +++ b/coderd/httpapi/httperror/wsbuild.go @@ -0,0 +1,91 @@ +package httperror + +import ( + "context" + "errors" + "fmt" + "net/http" + "sort" + + "github.com/hashicorp/hcl/v2" + + "github.com/coder/coder/v2/coderd/dynamicparameters" + "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/wsbuilder" + "github.com/coder/coder/v2/codersdk" +) + +func WriteWorkspaceBuildError(ctx context.Context, rw http.ResponseWriter, err error) { + var buildErr wsbuilder.BuildError + if errors.As(err, &buildErr) { + if httpapi.IsUnauthorizedError(err) { + buildErr.Status = http.StatusForbidden + } + + httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{ + Message: buildErr.Message, + Detail: buildErr.Error(), + }) + return + } + + var parameterErr *dynamicparameters.ResolverError + if errors.As(err, ¶meterErr) { + resp := codersdk.Response{ + Message: "Unable to validate parameters", + Validations: nil, + } + + // Sort the parameter names so that the order is consistent. + sortedNames := make([]string, 0, len(parameterErr.Parameter)) + for name := range parameterErr.Parameter { + sortedNames = append(sortedNames, name) + } + sort.Strings(sortedNames) + + for _, name := range sortedNames { + diag := parameterErr.Parameter[name] + resp.Validations = append(resp.Validations, codersdk.ValidationError{ + Field: name, + Detail: DiagnosticsErrorString(diag), + }) + } + + if parameterErr.Diagnostics.HasErrors() { + resp.Detail = DiagnosticsErrorString(parameterErr.Diagnostics) + } + + httpapi.Write(ctx, rw, http.StatusBadRequest, resp) + return + } + + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error creating workspace build.", + Detail: err.Error(), + }) +} + +func DiagnosticError(d *hcl.Diagnostic) string { + return fmt.Sprintf("%s; %s", d.Summary, d.Detail) +} + +func DiagnosticsErrorString(d hcl.Diagnostics) string { + count := len(d) + switch { + case count == 0: + return "no diagnostics" + case count == 1: + return DiagnosticError(d[0]) + default: + for _, d := range d { + // Render the first error diag. + // If there are warnings, do not priority them over errors. + if d.Severity == hcl.DiagError { + return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticError(d), count-1) + } + } + + // All warnings? ok... + return fmt.Sprintf("%s, and %d other diagnostic(s)", DiagnosticError(d[0]), count-1) + } +} diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 526be7e4e4f89..97617d02cdbe4 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -27,6 +27,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/provisionerjobs" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/provisionerdserver" @@ -410,28 +411,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { ) return err }, nil) - var buildErr wsbuilder.BuildError - if xerrors.As(err, &buildErr) { - var authErr dbauthz.NotAuthorizedError - if xerrors.As(err, &authErr) { - buildErr.Status = http.StatusForbidden - } - - if buildErr.Status == http.StatusInternalServerError { - api.Logger.Error(ctx, "workspace build error", slog.Error(buildErr.Wrapped)) - } - - httpapi.Write(ctx, rw, buildErr.Status, codersdk.Response{ - Message: buildErr.Message, - Detail: buildErr.Error(), - }) - return - } if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Error posting new build", - Detail: err.Error(), - }) + httperror.WriteWorkspaceBuildError(ctx, rw, err) return } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 58375ae4c4d45..d2280541d6c11 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -27,6 +27,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/provisionerjobs" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/prebuilds" @@ -732,21 +733,11 @@ func createWorkspace( ) return err }, nil) - var bldErr wsbuilder.BuildError - if xerrors.As(err, &bldErr) { - httpapi.Write(ctx, rw, bldErr.Status, codersdk.Response{ - Message: bldErr.Message, - Detail: bldErr.Error(), - }) - return - } if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error creating workspace.", - Detail: err.Error(), - }) + httperror.WriteWorkspaceBuildError(ctx, rw, err) return } + err = provisionerjobs.PostJob(api.Pubsub, *provisionerJob) if err != nil { // Client probably doesn't care about this error, so just log it. diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index aaccd337ee793..d82d238dae233 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -760,20 +760,12 @@ func (b *Builder) getDynamicParameters() (names, values []string, err error) { return nil, nil, BuildError{http.StatusInternalServerError, "failed to check if first build", err} } - buildValues, diagnostics := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild, + buildValues, err := dynamicparameters.ResolveParameters(b.ctx, b.workspace.OwnerID, render, firstBuild, lastBuildParameters, b.richParameterValues, presetParameterValues) - - if diagnostics.HasErrors() { - // TODO: Improve the error response. The response should include the validations for each failed - // parameter. The response should also indicate it's a validation error or a more general form failure. - // For now, any error is sufficient. - return nil, nil, BuildError{ - Status: http.StatusBadRequest, - Message: fmt.Sprintf("%d errors occurred while resolving parameters", len(diagnostics)), - Wrapped: diagnostics, - } + if err != nil { + return nil, nil, xerrors.Errorf("resolve parameters: %w", err) } names = make([]string, 0, len(buildValues))