chore: handle errors in wsproxy server for cli using buildinfo (#11584)

Emyrk · web-flow · commit f5a9f5ca3dab · 2024-01-11T16:55:34.000-06:00
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.
diff --git 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,14 +70,28 @@ 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.
 					if f, ok := inv.Stdin.(*os.File); ok {
 						_ = 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
@@ -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 <type>
+//	       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 "<nil>", 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 {
 //		   <verbose error message>
 //		2. <heading error message>
 //		   <verbose error message>
-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
@@ -323,14 +323,24 @@ 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 {
 	if res == nil {
 		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
@@ -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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -2065,7 +2065,7 @@ func (c *Client) BuildInfo(ctx context.Context) (BuildInfoResponse, error) {`
`2065`	`2065`	`}`
`2066`	`2066`	`defer res.Body.Close()`
`2067`	`2067`
`2068`		`- if res.StatusCode != http.StatusOK {`
	`2068`	`+ if res.StatusCode != http.StatusOK \|\| ExpectJSONMime(res) != nil {`
`2069`	`2069`	`return BuildInfoResponse{}, ReadBodyAsError(res)`
`2070`	`2070`	`}`
`2071`	`2071`