api: Endpoints can now return a tree of errors #46099

akerouanton · 2023-07-28T09:51:40Z

- What I did

There're cases like input validation where it makes sense to gather multiple errors and return them all to the client.

Prior to this PR, the API would only return a string message field. As a consequence, it requires those errors to be coalesced into a single string using a specific format, whether it's just a newline separator or some more complicated format.

This PR adds a new errors field containing a list of ErrorResponse.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2023-07-28T10:03:50Z

api/types/error_response.go

+	// The list of unformatted error messages.
+	RawMessages []string `json:"raw_messages"`


Do we anticipate we'd need more structured information at some point? Thinking of the structure that the OCI distribution spec chose; https://github.com/opencontainers/distribution-spec/blob/main/spec.md#error-codes

/cc @corhere @tianon

Maybe we don't need it, but "now" is the moment to choose :)

Discussing in the sync; and we can use a struct (currently with just one field); could be message or detail (see OCI spec)

message probably is most appropriate for our use 👍

akerouanton · 2023-07-28T11:54:18Z

api/server/errorhandler.go

+			stringErrs = append(stringErrs, err.Error())
+		}
+	} else if unwrapHTTP && errdefs.IsHTTPError(err) {
+		return unwrapErrors(errors.Unwrap(err), false)


I think unwrapping only once is enough but I might be wrong. /cc @thaJeztah

Also, this makes me wonder whether the fallback format should be handled by our own implementation of joinError. Otherwise, if we start using an Error tree, only the root will be well-formatted and other levels will use the line break format, as can be seen here: https://go.dev/play/p/MCUaNfjd5Xx.

corhere

I'd love to have a structured error format which provides sufficient detail to be able to marshal and unmarshal a tree of rich errors in full fidelity. But I don't think we need to design that now as I can see lots of ways to extend the schema in a backwards-compatible way.

For example:

Taking inspiration from the error interface and github.com/containerd/typeurl, we could encode trees of rich errors by nesting StructuredErrors recursively, and tagging each error with type information. Just like error.Error(), the message at each level is the complete error message, which may or may not be the concatenation of the nested errors' messages.

{
  "message": "2 errors occurred:\n\t* something bad happened: wrapped error: another\n\t* could not pull image 'library/hello-world' from registry: 503 Service Unavailable",
  "errors": [
    {
      "message": "something bad happened: wrapped error: another",
      "_typeurl": "mobyproject.org/errors/foo",
      "foo": "bar",
      "baz": {"quux": 3.14, "xyzzy": 42},
      "errors": [
        {
          "message": "wrapped error: another",
          "_typeurl": "mobyproject.org/errors/you-dun-goofed",
          "errors": [
            {
               "message": "another",
            }
          ],
        },
      ]
    },
    {
      "message": "could not pull image 'library/hello-world:latest' from registry: 503 Service Unavailable",
      "_typeurl": "mobyproject.org/errors/registry/PullError",
      "reference": "library/hello-world:latest",
      "registry": "registry-1.docker.io",
      "errors": [
        {
           "message": "503 Service Unavailable",
           "_typeurl": "mobyproject.org/registry/HTTPError",
           "status": 503,
           "host": "registry-1.docker.io"
        }
      ]
    }
  ]
}

api/swagger.yaml

corhere · 2023-08-02T22:02:22Z

api/server/errorhandler.go

+		case version == "" || versions.GreaterThanOrEqualTo(version, firstAPIVersionWithJSONMultiErrors):
+			response := &types.ErrorResponse{Message: errMsg, Errors: structuredErrs}
+			_ = httputils.WriteJSON(w, statusCode, response)


Is it necessary to gate multi-errors on API version? We're adding a new property to the error response JSON document, not changing the semantics of any existing ones.

Not sure if that's needed, I can drop it we consider it's not needed.

api/server/errorhandler.go

api/server/errorhandler_test.go

api/types/structured_error.go

thaJeztah · 2023-08-07T17:13:38Z

api/server/errorhandler.go

+	case wrappingError:
+		return &types.ErrorResponse{
+			Message: err.Error(),
+			Errors:  []*types.ErrorResponse{marshalErrorResponse(err.Unwrap())},


I'm wondering if we should unconditionally propagate the Errors array; my thinking here is that the top-level Message can be considered a "pre-rendered" presentation of the error (or errors if there's more than one), which can be used by clients to present as-is, and the Errors slice contains the raw, underlying error(s).

Clients that support the new Errors field would look in the Errors slice, and use it (to present to their liking), and otherwise fall back to using the top-level Message "as-is".

That's what this code is doing, except for the limiting case where there are no underlying errors. Note the recursion with Errors: it's the same type all the way down. Any property which could go on an object inside Errors could also go on the root ErrorResponse for a knowledgeable client to use to their liking. Take a look at the details block in my earlier non-inline review comment for a worked example of what that hypothetically might look like.

corhere · 2023-08-07T22:59:56Z

api/server/errorhandler.go

+
+// marshalErrorResponse returns a tree of [*types.ErrorResponse] representing the argument err.
+func marshalErrorResponse(err error) *types.ErrorResponse {
+	if errdefs.IsHTTPOnlyError(err) {


Do we still need this special case now that we marshal recursively? When one HTTPOnlyError wraps another HTTPOnlyError, why are they marshaled differently? The inner one is marshaled as a wrappingError while the outer is discarded.

When one HTTPOnlyError wraps another HTTPOnlyError, why are they marshaled differently? The inner one is marshaled as a wrappingError while the outer is discarded.

Hum, nope they would be both discarded and only the inner non-HTTPOnlyError would be marshaled.

The initial intent was to discard "HTTPOnlyError" as they don't add any context. If I remove this code, here's what it produces:

// Currently err := errdefs.InvalidParameter(errors.New("foobar")) expected := &types.ErrorResponse{Message: "foobar"} err := errdefs.InvalidParameter(errdefs.InvalidParameter(errors.New("foobar"))) expected := &types.ErrorResponse{Message: "foobar"} // With the IsHTTPOnlyError check removed err := errdefs.InvalidParameter(errors.New("foobar")) expected := &types.ErrorResponse{Message: "foobar", Errors: []*types.ErrorResponse{{Message: "foobar"}}} err := errdefs.InvalidParameter(errdefs.InvalidParameter(errors.New("foobar"))) expected := &types.ErrorResponse{Message: "foobar", Errors: []*types.ErrorResponse{{Message: "foobar"}}}

Do we want to keep this child ErrorResponse although it doesn't add value?

Now try marshaling errdefs.InvalidParameter(errdefs.Conflict(errors.New("foobar"))).

moby/errdefs/helpers.go

Lines 38 to 41 in 63d477b

func InvalidParameter(err error) error {

if err == nil || IsInvalidParameter(err) {

return err

}

Do we want to keep this child ErrorResponse although it doesn't add value?

I wouldn't want to keep HTTPOnly errors in the marshaled error tree. I just want them to be marshaled (or discarded) consistently, independent of their position in the error tree.

Ah, indeed. I see your point. So I should probably recursively unwrap HTTP-only errors here.

if errdefs.IsHTTPOnlyError(err) { if wrappedErr := errors.Unwrap(err); wrappedErr != nil { return marshalErrorResponse(wrappedErr) } }

I'm a bit confused by this part; so currently, this PR adds no additional information about the unwrapped errors (other than splitting the nested error messages). but wouldn't the type of error produced be actually useful once we add more details?

I don't think we should consider these to be "HTTP-only" error messages, because they aren't. Mapping them to a HTTP status code is one purpose, but that's a translation of the "kind" of error returned (e.g. an object was not found).

If we are considering this response to be used for handling specific conditions, wouldn't that information be exactly what's useful? (e.g., we returned an invalid request error, because an object was not found).

To some extend I'm wondering; we added these nested errors because Go's original formatting (omitting the "2 errors") was not ideal, so we added the option to unwrap those errors, so that they could be formatted in different ways, which a fallback for clients that don't support that (in the form of the existing pre-formatted Message.

But currently, there's no such implementation on the client side (should this PR include that part?).

If we anticipate this structured format to be used for more in-depth error analysing, I'm wondering if a stack-trace for errors would be more useful 🤔 I know I gave that a very quick attempt in api: add DOCKER_DEBUG_TRACE to return stack-traces in API error responses #43252

The client implementation can be added in a follow-up PR. There's no need to increase the scope of this one. The API change is backwards-compatible, after all.

Stack traces are not either/or. A stack trace is a property of an individual error. This PR adds support for marshaling a list/tree of errors. They are orthogonal concerns.

As far as the API is concerned, mapping to an HTTP status code is their only purpose. The client can losslessly recover that information from the HTTP status code the error is mapped to, combined with the Message of the unwrapped error.

The client implementation can be added in a follow-up PR. There's no need to increase the scope of this one. The API change is backwards-compatible, after all.

So what's currently the use for these structured errors if there's nothing consuming them? I know this work started because the default formatting of Golang's native "multi-errors" was not great (missing the 2 errors occurred part. And that's addressed in this PR by using our own formatting (through the formatErrors() function)

Do we need the other parts already, or should we split the formatErrors() part to a separate PR, and draft a more in-depth plan what we will need the structured API response for?

api/server/errorhandler.go

api/server/errorhandler_test.go

api/swagger.yaml

There're cases like input validation where it makes sense to gather multiple errors and return them all to the client. Prior to this commit, the API would only return a string `message` field. As a consequence, it requires those errors to be coalesced into a single string using a specific format, whether it's just a newline separator or some more complicated format. This commit adds a new `errors` field containing a list of ErrorResponse. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

corhere

LGTM!

thaJeztah · 2023-08-08T21:53:59Z

errdefs/is.go

+// IsHTTPOnlyError checks whether err is one of the un-exported HTTP error type provided by this package. These errors
+// only define what HTTP status code should be returned, but don't add additional context to the actual error they
+// wrap. By contrast, error types implementing the interfaces defined in this package are also adding such additional
+// context.
+func IsHTTPOnlyError(err error) bool {


Also wondering what would make these errors different from, say,

moby/builder/builder-next/builder.go

Lines 42 to 46 in 2b449e0

type errMultipleFilterValues struct{}

func (errMultipleFilterValues) Error() string { return "filters expect only one value" }

func (errMultipleFilterValues) InvalidParameter() {}

Which wouldn't be caught by this function. The non-exported errors in this package are mostly for convenience (and "one" implementation of errdefs types), but there's many others, and the information they add is to put an error in a specific category (see my other comment above).

These errors all have the property that errors.Unwrap(err).Error() == err.Error(), implement one of the errdefs interfaces (which map 1:1 to HTTP status codes, except for HTTP 500), and nothing else. errMultipleFilterValues is not caught by this function by design, because it carries additional context. The information carried by the non-exported convenience types is already encoded in the response, in the HTTP status code. Duplicating that in the error tree is therefore just redundant noise.

The information carried by the non-exported convenience types is already encoded in the response, in the HTTP status code. Duplicating that in the error tree is therefore just redundant noise.

But that only applies to the final error, and not intermediate ones, and currently we're discarding that information; see for example this test-case;

"error wrapped multiple times with a HTTP-only error": { err: errdefs.InvalidParameter(errdefs.Conflict(errors.New("foobar"))), expected: &types.ErrorResponse{Message: "foobar"}, },

laurazard

LGTM

As we have a hard time figuring out what moby#46099 should look like, this drop-in replacement will solve the initial formatting problem we had. It's made internal such that we can remove it whenever we want and unlike moby#46099 doesn't require thoughtful API changes. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

As we have a hard time figuring out what moby#46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike moby#46099 doesn't require thoughtful API changes. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton · 2023-08-10T16:28:57Z

So, the initial formatting problem isn't gone:

Error response from daemon: invalid endpoint config for network testnet: invalid IPv4 address: foobar
invalid IPv6 address: foobar
invalid link-local IP address yolo
user specified IP address is supported only when connecting to networks with user configured subnets

The original intent of nicely formatting the top-level message was to provide a backward-compatible way of supporting errors.Join. If we get that right, then newer versions of the CLI don't even need to consume the error tree; they can just stick with the formatted message field. However, serving error trees might still be of interest for API consumers that build a GUI, or otherwise need something different than what we'd provide in the formatted message field.

As we don't have a clear view of how this error tree should be consumed, there're disagreement about what errors should be skipped or not (see here and here).

Also, the formatting issue resurface at the logging level. We didn't discuss it so far, but we have the exact same problem, although less important. We'd have to create our own formatter to process errors set in logrus.Fields and reformat errors as we desire.

As an alternative, I'm proposing #46188. It's a temporary drop-in replacement for errors.Join; it lives in an internal package, has the same semantics as errors.Join, and only provide better formatting with no API changes required.

As we have a hard time figuring out what moby#46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike moby#46099 doesn't require thoughtful API changes. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

As we have a hard time figuring out what moby#46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike moby#46099 doesn't require thoughtful API changes. Signed-off-by: Albin Kerouanton <albinker@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah reviewed Jul 28, 2023

View reviewed changes

akerouanton force-pushed the http-multi-errors branch from 7521b4d to 8aa1a7f Compare July 28, 2023 11:45

akerouanton commented Jul 28, 2023

View reviewed changes

akerouanton mentioned this pull request Jul 28, 2023

Validate IPAM config before handing it over to libnetwork #45759

Merged

akerouanton added area/api status/2-code-review impact/api labels Jul 28, 2023

akerouanton mentioned this pull request Jul 27, 2023

25.0 Release Tracking #45926

Closed

26 tasks

akerouanton force-pushed the http-multi-errors branch 2 times, most recently from f2528da to c825ae0 Compare August 1, 2023 08:36

akerouanton requested a review from tianon as a code owner August 1, 2023 08:36

akerouanton force-pushed the http-multi-errors branch 3 times, most recently from a7a26c8 to d75b091 Compare August 2, 2023 14:55

corhere reviewed Aug 2, 2023

View reviewed changes

akerouanton force-pushed the http-multi-errors branch 3 times, most recently from 747351d to 2640ee1 Compare August 4, 2023 08:22

akerouanton changed the title ~~api: Endpoints can now return multiple error messages~~ api: Endpoints can now return a tree of errors Aug 4, 2023

corhere requested changes Aug 4, 2023

View reviewed changes

akerouanton force-pushed the http-multi-errors branch from 2640ee1 to 80080f4 Compare August 5, 2023 06:32

thaJeztah reviewed Aug 7, 2023

View reviewed changes

corhere reviewed Aug 7, 2023

View reviewed changes

akerouanton force-pushed the http-multi-errors branch from 80080f4 to deb5d7a Compare August 8, 2023 08:59

akerouanton force-pushed the http-multi-errors branch from deb5d7a to c658c82 Compare August 8, 2023 16:17

corhere approved these changes Aug 8, 2023

View reviewed changes

thaJeztah reviewed Aug 8, 2023

View reviewed changes

laurazard approved these changes Aug 9, 2023

View reviewed changes

akerouanton mentioned this pull request Aug 10, 2023

Add a temporary drop-in replacement for errors.Join #46188

Merged

thaJeztah mentioned this pull request Aug 29, 2023

daemon/containerd: Replace pkg/errors with std errors #46353

Closed

		// The list of unformatted error messages.
		RawMessages []string `json:"raw_messages"`

	func InvalidParameter(err error) error {
	if err == nil \|\| IsInvalidParameter(err) {
	return err
	}

	type errMultipleFilterValues struct{}

	func (errMultipleFilterValues) Error() string { return "filters expect only one value" }

	func (errMultipleFilterValues) InvalidParameter() {}

api: Endpoints can now return a tree of errors #46099

Are you sure you want to change the base?

api: Endpoints can now return a tree of errors #46099

Uh oh!

Conversation

akerouanton commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton commented Aug 10, 2023

Uh oh!

Uh oh!

akerouanton commented Jul 28, 2023 •

edited

Loading