Fill in the 'error' variant #52

lukewagner · 2023-09-12T22:37:10Z

As discussed in #49, this PR replaces the current placeholder error variant cases with the error types enumerated by RFC 9209 which are maintained in an IANA table. Note that these specs also specify payloads for some of these error cases which I also reflected in the error case payloads.

The one interesting bit of minutiae, where I'm going with Adam's suggestion, but I have no strong opinions about so happy to hear arguments to the contrary, is that there is no error case corresponding to http_request_error, as this seems like it would turn into an ok result with a 4xx status-code.

Mossaka

lgtm!

wit/types.wit

pchickey · 2023-10-31T22:07:38Z

This PR should also add an http-error: func(stream-error) -> option<error> function for elaborating stream.error into one of these HTTP-specific error variants, see for example WebAssembly/wasi-filesystem#137

wit/types.wit

pchickey · 2023-11-02T18:26:06Z

wit/types.wit

+    HTTP-response-content-coding(string),
+    HTTP-response-timeout,
+    HTTP-upgrade-failed,
+    HTTP-protocol-error,


This error should only be used when a more specific one is not defined. from https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types - this can represent whatwg fetch's network error concept https://fetch.spec.whatwg.org/#concept-network-error. Maybe this should have an (option<string>) payload so that an implementation can provide extra debugging information if it is available? and the docs should make it clear this is the catch-all case for when no other error could be determined.

Maybe this should have an (option<string>) payload so that an implementation can provide extra debugging information if it is available?

I worry that if we expose this extra debugging information to clients, this will become load-bearing like the situation we have with 503s and custom HTTP/1 reason phrases. Perhaps we could instead encourage embedders to treat this as a cause for internal logging.

OK, we will not add this information now. We may find some way to provide more debugging or error context in the medium term, but not now.

No, I think we really need to provide some option<string> here or else we will regret it. For the catch-all error case, we need to have some sort of way for the user to debug why an unexpected error occurred. Like the stream error to-debug-string, we will document that this information is unstable. If programmers ignore that and it breaks their code when it changes, I would prefer that situation to programmers whose code breaks when they move it to a different embedding, but they have no reasonable means to determine why, especially given that the purpose of this standard is to work on many different embedding which will have a variety of constraints which we can't yet imagine. Sending a user off to dig an error out of internal logging is always a much worse solution than just providing a string.

@pchickey I think it comes down to who the "user" is that we want to empower to debug. If the user is a person who moves their code from one hosting provider to another and suddenly they start seeing protocol errors for requests to the same origin, that is very unlikely to be actionable by that user. It almost certainly indicates a problem with the embedder or the network between the embedder and the origin. In this situation, adding a free-form string at best gives that user something to paste into the support request with their new provider.

If you have other user profiles in mind that would benefit from the debugging info directly, I don't want to hold up progress on the spec. I would just recommend that any embedder who wants to avoid ossification not populate this field for production traffic.

@acfoltzer that is too narrow a definition of user. This spec already has implementations in progress that will be deployed beyond vendor-managed FaaS. For example the wasmtime-wasi-http implementation is intended to be run via runwasi in kubernetes environments, the jco implementation that could run in multiple different versions of node and node-like engines (deno, bun, etc), and there is interest in implementations that run on top of ServiceWorker, an implementation for nginx unit, and so on. In all of those cases, the user may also be managing major details of the embedding themselves, and the embeddings may encounter errors in ways that are difficult for us to anticipate here.

We're designating internal-error as the catch-all error case

lukewagner · 2023-11-07T22:42:03Z

This all looks great to me; thanks for all the work @elliottt! (I'd approve, but I can't as the PR author :P)

acfoltzer · 2023-11-08T18:12:35Z

@lukewagner can we please make sure this is addressed before committing? #52 (comment)

Resolves #49

wit/types.wit

PiotrSikora · 2023-11-09T19:10:24Z

wit/types.wit

+    TLS-alert-received(TLS-alert-received-payload),
+    HTTP-request-denied,
+    HTTP-request-length-required,
+    HTTP-request-content-too-large,


The corresponding error for the response is HTTP-response-body-size(option<u32>), so this introduces inconsistency within the spec.

PiotrSikora · 2023-11-09T19:10:59Z

wit/types.wit

+    HTTP-request-header-section-size(option<u32>),
+    HTTP-request-header-size(option<field-size-payload>),


We should also add:

HTTP-request-trailer-section-size(option<u32>), HTTP-request-trailer-size(field-size-payload),

PiotrSikora · 2023-11-09T19:15:22Z

wit/types.wit

+    TLS-certificate-error,
+    TLS-alert-received(TLS-alert-received-payload),
+    HTTP-request-denied,
+    HTTP-request-length-required,


The generic HTTP-request-error would be still useful, since those listed here don't even cover the existing 4xx error space. Either that, or we should add HTTP-request-* errors covering existing 4xx error codes.

My rationale laid out here is that the remaining 4xx space would best be covered by returning an ok result with the next hop's 4xx response directly conveyed. Unlike the consumer of a proxy-status header, the perspective of the wasi-http client code makes it clear via the result whether the error arises from within the embedder, or from the next hop, so I don't think we need this case which primarily exists to disambiguate whether it was a proxy-generated status code or the next hop's.

Following the addition of the HTTP-request-* variants, are there further 4xx codes that you see being ambiguous? That's probably an indication that we're missing variants still.

The two cases from wasmtime-wasi-http that would be nice to cover are 400 and 405. We don't have anything that maps nicely to 400 right now, which is what was useful about the original HTTP-request-error. We were raising 405 for parse errors from hyper when trying to process the Other case for the Method variant.

@elliottt what errors are you wanting to map to 400?

When we receive an error from hyper that returns true only for is_parse, it's not entirely clear what we should map that to without having HTTP-request-error available. 400 seemed like a good use for that, but without HTTP-request-error I've switched back to HTTP-protocol-error.

I think that's the right choice. The is_parse() predicate is pretty broad within hyper, but in general indicates that a successful message was not received. But HTTP-request-error(400) would imply that the next hop successfully returned a 400, at least going by its definition in RFC 9209

This is the specific case I'm thinking of, where the outgoing-handler needs to reject a Scheme::Other value during request validation. The request hasn't been sent upstream yet, but we still need to indicate that it was malformed. After the change to remove HTTP-request-error there's no longer a good place for that:

bytecodealliance/wasmtime@7af92d0#diff-5dc7fbb5c0f1d7e9bc2d07ee4d374589e46e4c2f054fd6d28b239da1aff426bdL55-L65

Gotcha, I don't think HTTP-request-error would be appropriate for that case regardless, I think we need a HTTP-request-URI-invalid instead

My rationale laid out here is that the remaining 4xx space would best be covered by returning an ok result with the next hop's 4xx response directly conveyed

But returning ok and 4xx status code would imply that the error came from the peer and not the host environment, wouldn't it?

Right, the remaining 4xx codes are those that really can't be answered by the environment. If you notice a case we've missed, let's add it as new variants in a follow-up PR

PiotrSikora · 2023-11-09T19:16:42Z

wit/types.wit

+    HTTP-response-timeout,
+    HTTP-upgrade-failed,
+    HTTP-protocol-error,
+    proxy-internal-response,


Note that this is used to indicate that the intermediate generated the response and didn't forward the request to the next hop. It's not an error condition.

Yeah, I think this variant probably doesn't belong, especially because returning an error means we don't get a response for this tag to apply to.

Suggested change

proxy-internal-response,

PiotrSikora · 2023-11-09T19:17:07Z

wit/types.wit

+    proxy-internal-error,
+    proxy-configuration-error,


I'm not sure if those are applicable here?

I think it's useful to have a catch-all "internal error" when implementing an embedder, and setting apart whether that error might have arisen from a bad configuration is helpful too. But we should probably not label them as being specific to a proxy; they could equally apply to incoming- or outgoing-only handler programs.

Suggested change

proxy-internal-error,

proxy-configuration-error,

internal-error,

configuration-error,

PiotrSikora · 2023-11-09T19:17:25Z

wit/types.wit

-    protocol-error(string),
-    unexpected-error(string)
+  /// The cases of this variant correspond to the IANA HTTP Proxy Error Types:
+  ///   https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types


The list below already diverged from the IANA table.

pchickey · 2023-11-09T22:56:09Z

Thanks everyone for all of the discussion here - I've mentioned this in some other contexts that we are scheduled to cut a release candidate for WASI Preview 2 at the end of this week. I would like to get this PR resolved and merged by the end of Friday Nov 10, even if it means cutting off some discussion and deferring it to a later date. @elliottt and I are going to pair his afternoon to try to resolve these outstanding threads of discussion and get this PR ready to land, and bring the wasmtime implementation up-to-date with it. cc @lukewagner @acfoltzer @PiotrSikora

I failed to notice this was missing in WebAssembly#52. Status codes outside the range of 100-599 are [invalid](https://www.rfc-editor.org/rfc/rfc9110#section-15-6), though there is a nod to implementation-defined behavior outside those valid ranges. In practice we have found it very useful to return error codes specific to status problems to customers since our platform does not support codes outside this range, and the next-best option in the RFC 9209 equivalents is the generic `http-protocol-error` which doesn't help a user much with debugging.

lukewagner requested a review from PiotrSikora September 12, 2023 22:37

lukewagner force-pushed the error-codes branch 2 times, most recently from 5683e33 to bc91404 Compare September 12, 2023 22:53

Mossaka approved these changes Sep 18, 2023

View reviewed changes

PiotrSikora reviewed Sep 19, 2023

View reviewed changes

wit/types.wit Outdated Show resolved Hide resolved

pchickey mentioned this pull request Oct 31, 2023

Fill out the error variant in wasi-http bytecodealliance/wasmtime#7428

Closed

elliottt mentioned this pull request Nov 1, 2023

wasi-http: Migrate to more descriptive error variant bytecodealliance/wasmtime#7434

Merged

acfoltzer reviewed Nov 1, 2023

View reviewed changes

wit/types.wit Outdated Show resolved Hide resolved

pchickey reviewed Nov 2, 2023

View reviewed changes

elliottt force-pushed the error-codes branch from d25c0ab to 99b5155 Compare November 7, 2023 22:11

elliottt requested a review from pchickey November 7, 2023 22:54

lukewagner and others added 3 commits November 8, 2023 16:44

Fill in the 'error' variant

8e473e7

Resolves #49

Switch to doc comments, make all variant fields optional

cd5cbf8

Rename error to error-code, and add the http-error-code downcast

c83597b

elliottt force-pushed the error-codes branch from 99b5155 to 5dd45c8 Compare November 9, 2023 00:50

acfoltzer reviewed Nov 9, 2023

View reviewed changes

wit/types.wit Outdated Show resolved Hide resolved

Remove http-client-error, and add specializations

4850f66

elliottt force-pushed the error-codes branch 2 times, most recently from 8d40487 to 0ba79a3 Compare November 9, 2023 00:59

PiotrSikora reviewed Nov 9, 2023

View reviewed changes

elliottt added 2 commits November 9, 2023 16:11

Address remaining comments

cbdcde3

Regenerate proxy.md

5af2ac9

elliottt force-pushed the error-codes branch from 0ba79a3 to 5af2ac9 Compare November 10, 2023 00:12

elliottt merged commit 8db6763 into main Nov 10, 2023
1 check passed

elliottt deleted the error-codes branch December 5, 2023 19:04

acfoltzer mentioned this pull request Feb 21, 2024

Add http-response-status-invalid error code #104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fill in the 'error' variant #52

Fill in the 'error' variant #52

lukewagner commented Sep 12, 2023

Mossaka left a comment

pchickey commented Oct 31, 2023 •

edited

Loading

pchickey Nov 2, 2023

acfoltzer Nov 2, 2023

pchickey Nov 8, 2023

pchickey Nov 9, 2023 •

edited

Loading

acfoltzer Nov 9, 2023

pchickey Nov 9, 2023

acfoltzer Nov 9, 2023

lukewagner commented Nov 7, 2023

acfoltzer commented Nov 8, 2023

PiotrSikora Nov 9, 2023

PiotrSikora Nov 9, 2023

PiotrSikora Nov 9, 2023

acfoltzer Nov 9, 2023

elliottt Nov 9, 2023

acfoltzer Nov 9, 2023

elliottt Nov 9, 2023

acfoltzer Nov 9, 2023

elliottt Nov 9, 2023

acfoltzer Nov 9, 2023

PiotrSikora Nov 10, 2023

acfoltzer Nov 10, 2023

PiotrSikora Nov 9, 2023

acfoltzer Nov 9, 2023

PiotrSikora Nov 9, 2023

acfoltzer Nov 9, 2023

PiotrSikora Nov 9, 2023

pchickey commented Nov 9, 2023

		HTTP-request-header-section-size(option<u32>),
		HTTP-request-header-size(option<field-size-payload>),

Fill in the 'error' variant #52

Fill in the 'error' variant #52

Conversation

lukewagner commented Sep 12, 2023

Mossaka left a comment

Choose a reason for hiding this comment

pchickey commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchickey Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukewagner commented Nov 7, 2023

acfoltzer commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchickey commented Nov 9, 2023

pchickey commented Oct 31, 2023 •

edited

Loading

pchickey Nov 9, 2023 •

edited

Loading