From f5a9f5ca3dab3c7bf6a5c3977faa0198d91813de Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 11 Jan 2024 16:55:34 -0600 Subject: [PATCH] chore: handle errors in wsproxy server for cli using buildinfo (#11584) Cli errors are pretty formatted. This handles nested pretty types. Before it found the first error it could understand and return that. Now it will print the full error stack with more information. To prevent information loss, a "[Trace=...]" was added to capture some extra error context for debugging. --- cli/errors.go | 21 +++++++- cli/root.go | 99 ++++++++++++++++++++++++++--------- codersdk/client.go | 17 ++++-- codersdk/deployment.go | 2 +- enterprise/wsproxy/wsproxy.go | 6 ++- 5 files changed, 114 insertions(+), 31 deletions(-) diff --git a/cli/errors.go b/cli/errors.go index 12567e0400..ee12ca036a 100644 --- a/cli/errors.go +++ b/cli/errors.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "net/http" "net/http/httptest" @@ -43,6 +44,10 @@ func (RootCmd) errorExample() *clibase.Cmd { //nolint:errorlint,forcetypeassert apiError.(*codersdk.Error).Helper = "Have you tried turning it off and on again?" + //nolint:errorlint,forcetypeassert + apiErrorNoHelper := apiError.(*codersdk.Error) + apiErrorNoHelper.Helper = "" + // Some flags var magicWord clibase.String @@ -65,6 +70,17 @@ func (RootCmd) errorExample() *clibase.Cmd { // A multi-error { Use: "multi-error", + Handler: func(inv *clibase.Invocation) error { + return xerrors.Errorf("wrapped: %w", errors.Join( + xerrors.Errorf("first error: %w", errorWithStackTrace()), + xerrors.Errorf("second error: %w", errorWithStackTrace()), + xerrors.Errorf("wrapped api error: %w", apiErrorNoHelper), + )) + }, + }, + { + Use: "multi-multi-error", + Short: "This is a multi error inside a multi error", Handler: func(inv *clibase.Invocation) error { // Closing the stdin file descriptor will cause the next close // to fail. This is joined to the returned Command error. @@ -72,7 +88,10 @@ func (RootCmd) errorExample() *clibase.Cmd { _ = f.Close() } - return xerrors.Errorf("some error: %w", errorWithStackTrace()) + return errors.Join( + xerrors.Errorf("first error: %w", errorWithStackTrace()), + xerrors.Errorf("second error: %w", errorWithStackTrace()), + ) }, }, diff --git a/cli/root.go b/cli/root.go index cef7df19b3..8b8784735d 100644 --- a/cli/root.go +++ b/cli/root.go @@ -1016,7 +1016,7 @@ type prettyErrorFormatter struct { // format formats the error to the console. This error should be human // readable. func (p *prettyErrorFormatter) format(err error) { - output := cliHumanFormatError(err, &formatOpts{ + output, _ := cliHumanFormatError("", err, &formatOpts{ Verbose: p.verbose, }) // always trail with a newline @@ -1030,41 +1030,66 @@ type formatOpts struct { const indent = " " // cliHumanFormatError formats an error for the CLI. Newlines and styling are -// included. -func cliHumanFormatError(err error, opts *formatOpts) string { +// included. The second return value is true if the error is special and the error +// chain has custom formatting applied. +// +// If you change this code, you can use the cli "example-errors" tool to +// verify all errors still look ok. +// +// go run main.go exp example-error +// go run main.go exp example-error api +// go run main.go exp example-error cmd +// go run main.go exp example-error multi-error +// go run main.go exp example-error validation +// +//nolint:errorlint +func cliHumanFormatError(from string, err error, opts *formatOpts) (string, bool) { if opts == nil { opts = &formatOpts{} } + if err == nil { + return "", true + } - //nolint:errorlint if multi, ok := err.(interface{ Unwrap() []error }); ok { multiErrors := multi.Unwrap() if len(multiErrors) == 1 { // Format as a single error - return cliHumanFormatError(multiErrors[0], opts) + return cliHumanFormatError(from, multiErrors[0], opts) } - return formatMultiError(multiErrors, opts) + return formatMultiError(from, multiErrors, opts), true } // First check for sentinel errors that we want to handle specially. // Order does matter! We want to check for the most specific errors first. - var sdkError *codersdk.Error - if errors.As(err, &sdkError) { - return formatCoderSDKError(sdkError, opts) + if sdkError, ok := err.(*codersdk.Error); ok { + return formatCoderSDKError(from, sdkError, opts), true } - var cmdErr *clibase.RunCommandError - if errors.As(err, &cmdErr) { - return formatRunCommandError(cmdErr, opts) + if cmdErr, ok := err.(*clibase.RunCommandError); ok { + // no need to pass the "from" context to this since it is always + // top level. We care about what is below this. + return formatRunCommandError(cmdErr, opts), true } + uw, ok := err.(interface{ Unwrap() error }) + if ok { + msg, special := cliHumanFormatError(from+traceError(err), uw.Unwrap(), opts) + if special { + return msg, special + } + } + // If we got here, that means that the wrapped error chain does not have + // any special formatting below it. So we want to return the topmost non-special + // error (which is 'err') + // Default just printing the error. Use +v for verbose to handle stack // traces of xerrors. if opts.Verbose { - return pretty.Sprint(headLineStyle(), fmt.Sprintf("%+v", err)) + return pretty.Sprint(headLineStyle(), fmt.Sprintf("%+v", err)), false } - return pretty.Sprint(headLineStyle(), fmt.Sprintf("%v", err)) + return pretty.Sprint(headLineStyle(), fmt.Sprintf("%v", err)), false } // formatMultiError formats a multi-error. It formats it as a list of errors. @@ -1075,15 +1100,20 @@ func cliHumanFormatError(err error, opts *formatOpts) string { // // 2. // -func formatMultiError(multi []error, opts *formatOpts) string { +func formatMultiError(from string, multi []error, opts *formatOpts) string { var errorStrings []string for _, err := range multi { - errorStrings = append(errorStrings, cliHumanFormatError(err, opts)) + msg, _ := cliHumanFormatError("", err, opts) + errorStrings = append(errorStrings, msg) } // Write errors out var str strings.Builder - _, _ = str.WriteString(pretty.Sprint(headLineStyle(), fmt.Sprintf("%d errors encountered:", len(multi)))) + var traceMsg string + if from != "" { + traceMsg = fmt.Sprintf("Trace=[%s])", from) + } + _, _ = str.WriteString(pretty.Sprint(headLineStyle(), fmt.Sprintf("%d errors encountered: %s", len(multi), traceMsg))) for i, errStr := range errorStrings { // Indent each error errStr = strings.ReplaceAll(errStr, "\n", "\n"+indent) @@ -1112,24 +1142,30 @@ func formatRunCommandError(err *clibase.RunCommandError, opts *formatOpts) strin var str strings.Builder _, _ = str.WriteString(pretty.Sprint(headLineStyle(), fmt.Sprintf("Encountered an error running %q", err.Cmd.FullName()))) - msgString := fmt.Sprintf("%v", err.Err) - if opts.Verbose { - // '%+v' includes stack traces - msgString = fmt.Sprintf("%+v", err.Err) - } + msgString, special := cliHumanFormatError("", err.Err, opts) _, _ = str.WriteString("\n") - _, _ = str.WriteString(pretty.Sprint(tailLineStyle(), msgString)) + if special { + _, _ = str.WriteString(msgString) + } else { + _, _ = str.WriteString(pretty.Sprint(tailLineStyle(), msgString)) + } + return str.String() } // formatCoderSDKError come from API requests. In verbose mode, add the // request debug information. -func formatCoderSDKError(err *codersdk.Error, opts *formatOpts) string { +func formatCoderSDKError(from string, err *codersdk.Error, opts *formatOpts) string { var str strings.Builder if opts.Verbose { _, _ = str.WriteString(pretty.Sprint(headLineStyle(), fmt.Sprintf("API request error to \"%s:%s\". Status code %d", err.Method(), err.URL(), err.StatusCode()))) _, _ = str.WriteString("\n") } + // Always include this trace. Users can ignore this. + if from != "" { + _, _ = str.WriteString(pretty.Sprint(headLineStyle(), fmt.Sprintf("Trace=[%s]", from))) + _, _ = str.WriteString("\n") + } _, _ = str.WriteString(pretty.Sprint(headLineStyle(), err.Message)) if err.Helper != "" { @@ -1144,6 +1180,21 @@ func formatCoderSDKError(err *codersdk.Error, opts *formatOpts) string { return str.String() } +// traceError is a helper function that aides developers debugging failed cli +// commands. When we pretty print errors, we lose the context in which they came. +// This function adds the context back. Unfortunately there is no easy way to get +// the prefix to: "error string: %w", so we do a bit of string manipulation. +// +//nolint:errorlint +func traceError(err error) string { + if uw, ok := err.(interface{ Unwrap() error }); ok { + a, b := err.Error(), uw.Unwrap().Error() + c := strings.TrimSuffix(a, b) + return c + } + return err.Error() +} + // These styles are arbitrary. func headLineStyle() pretty.Style { return cliui.DefaultStyles.Error diff --git a/codersdk/client.go b/codersdk/client.go index 05ca69fba6..d7ca661adf 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -323,6 +323,17 @@ func (c *Client) Request(ctx context.Context, method, path string, body interfac return resp, err } +// ExpectJSONMime is a helper function that will assert the content type +// of the response is application/json. +func ExpectJSONMime(res *http.Response) error { + contentType := res.Header.Get("Content-Type") + mimeType := parseMimeType(contentType) + if mimeType != "application/json" { + return xerrors.Errorf("unexpected non-JSON response %q", contentType) + } + return nil +} + // ReadBodyAsError reads the response as a codersdk.Response, and // wraps it in a codersdk.Error type for easy marshaling. func ReadBodyAsError(res *http.Response) error { @@ -330,7 +341,6 @@ func ReadBodyAsError(res *http.Response) error { return xerrors.Errorf("no body returned") } defer res.Body.Close() - contentType := res.Header.Get("Content-Type") var requestMethod, requestURL string if res.Request != nil { @@ -352,8 +362,7 @@ func ReadBodyAsError(res *http.Response) error { return xerrors.Errorf("read body: %w", err) } - mimeType := parseMimeType(contentType) - if mimeType != "application/json" { + if mimeErr := ExpectJSONMime(res); mimeErr != nil { if len(resp) > 2048 { resp = append(resp[:2048], []byte("...")...) } @@ -365,7 +374,7 @@ func ReadBodyAsError(res *http.Response) error { method: requestMethod, url: requestURL, Response: Response{ - Message: fmt.Sprintf("unexpected non-JSON response %q", contentType), + Message: mimeErr.Error(), Detail: string(resp), }, Helper: helpMessage, diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 5ee0ce478c..d6d3439334 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -2065,7 +2065,7 @@ func (c *Client) BuildInfo(ctx context.Context) (BuildInfoResponse, error) { } defer res.Body.Close() - if res.StatusCode != http.StatusOK { + if res.StatusCode != http.StatusOK || ExpectJSONMime(res) != nil { return BuildInfoResponse{}, ReadBodyAsError(res) } diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index 9ad8f16764..438ed664d2 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "crypto/x509" + "errors" "fmt" "net/http" "net/url" @@ -157,7 +158,10 @@ func New(ctx context.Context, opts *Options) (*Server, error) { // TODO: Probably do some version checking here info, err := client.SDKClient.BuildInfo(ctx) if err != nil { - return nil, xerrors.Errorf("failed to fetch build info from %q: %w", opts.DashboardURL, err) + return nil, fmt.Errorf("buildinfo: %w", errors.Join( + fmt.Errorf("unable to fetch build info from primary coderd. Are you sure %q is a coderd instance?", opts.DashboardURL), + err, + )) } if info.WorkspaceProxy { return nil, xerrors.Errorf("%q is a workspace proxy, not a primary coderd instance", opts.DashboardURL)