Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additions to support gRPC #128

Open
dicej opened this issue Sep 3, 2024 · 8 comments
Open

Additions to support gRPC #128

dicej opened this issue Sep 3, 2024 · 8 comments

Comments

@dicej
Copy link
Collaborator

dicej commented Sep 3, 2024

Recently, I've been trying to use a popular .NET library, Grpc.Net, to make outbound gRPC connections using a port of System.Net.Http to wasi-http. I was able to get it to work, but had to hack a few things:

  • Patched wasmtime-wasi-http to use HTTP/2 for all outgoing requests
  • Patched System.Net.Http to assume incoming responses use HTTP/2
  • Patched wasmtime-wasi-http's is_forbidden_header to allow all headers

Ideally, none of those hacks would be necessary, so I'd like to propose the following additions to the wasi-http spec:

  • (Optional) Add documentation to clarify that the set of "forbidden" headers may vary among implementations. E.g. browsers might prohibit TE headers, but wasmtime-wasi-http might allow them (and indeed should allow them).
  • Add a method on outgoing-request to specify the HTTP version to use. The implementation should return an error if it can't guarantee that version will be used.
  • Add a method on incoming-response to retrieve the HTTP version of the response, e.g. as an option<http-version> where http-version is an enum type and none means the version is not known. This allows the client to detect if the server downgraded the connection to a lower version.

See also bytecodealliance/wasmtime#7538 for further discussion.

@tschneidereit
Copy link
Member

I agree that we should unblock gRPC over wasi-http. For the specific way of going about it, I wonder if we might be able to get away with something slightly less invasive:

  • Instead of lifting the restriction on all headers, specifically allow TE with a value of trailers. That is in line with http/2 and /3, and is the one usage of TE that is useful on the application, not just the transport layer.
  • Given that trailers support in http/1.1 is pretty spotty and hence usage of trailers over http/1.1 not really reliable, treat the existence of TE: trailers in the headers as requiring http/2 or above for outgoing requests, and as a guarantee that the request was made over http/2+ for incoming ones.

The combination of these two would I think fully satisfy the requirements for making gRPC work, while not requiring any additions to the WIT interfaces of wasi-http. (And technically not even any spec changes, given that the list of forbidden headers isn't currently part of the spec itself.)

At first glance, this suggestion (or the changes Joel suggests above) seem bad in that they aren't supportable in browser environments, because TE is a forbidden header in browsers. This seems particularly bad because it's not statically visible that a browser environment will not be able to run a given component.

However, I'd argue that if anything allowing TE: trailers is an improvement over the status quo: it's already the case that trailers themselves aren't supported in browsers, so trying to send them will inevitably fail. Given that, it seems strictly better to have an error at a time when it might still be actionable: before even sending a request, instead of after the request has been sent in its entirety, except for the trailers.

@dicej
Copy link
Collaborator Author

dicej commented Sep 4, 2024

@tschneidereit In your proposal how would the implementation indicate that, although the client sent an HTTP/2 request, the server downgraded the response to e.g. HTTP/1.1?

@tschneidereit
Copy link
Member

You're right, I didn't address that part. One possible answer would be that we'd expose that as a connection error, because it'd mean that there's no reliable way to support the required semantics.

In practice, I would assume that someone who wants to use trailers (and hence sends TE: trailers) would do so in an environment that will successfully transport the trailers—meaning the host the request is sent to speaks http/2+. So an error would rarely happen in reality, and would probably be the right result if it does.

@lukewagner
Copy link
Member

Yes, also agreed on the goal of allowing gRPC to work over wasi-http. Riffing on Till's idea, could we add a method roughly like:

resource outgoing-request {
  ...
  expects-trailers: func() -> bool;
  set-expects-trailers: func(required: bool);
}

where setting expects-trailers causes the host to set TE: trailers as appropriate and also causes the handle call to fail with a connection error if the host determines that the transport doesn't support trailers (again giving the host some lattitude to judge the HTTP/1.1 situation)?

@tschneidereit
Copy link
Member

That approach would work, and I agree that it'd be conceptually cleaner. It'd also come at significant cost to portability: it'd mean that any code that would "just work" under my proposal would instead have to be changed. That'll either affect code that's easy to change, but needs changing in many places (potentially multiple per application, with the risk of not catching all of them) or in hard-to-change ones, such as deep within frameworks/libraries that are slow to update, and often not under the interested party's control.

Given that, I do wonder if the conceptual cleanliness is really worth it?

@tschneidereit
Copy link
Member

As a quick heads-up, Joel and I talked about this in much more detail and realized that neither of the last two suggestions will really work. We did come up with an alternative that seems much better, which I'll sketch out here later today

@tschneidereit
Copy link
Member

I didn't get around to this before EOD yesterday, but I now opened #129 with proposed API additions to address the requirements here.

I wrote a long explanation of my reasoning behind this design in the PR, so won't repeat that here :)

@pavelsavara
Copy link

The design #129 proposes new interface per HTTP version.
But there are more dimensions to the HTTP versioning and features.
In the discussion on that PR we concluded that for now, we need to go ahead with more dynamic options on the current (generic HTTP) interface.

Joel's hack of wasmtime header validation bytecodealliance/wasmtime@main...dicej:wasmtime:grpc-hacks is just removing the header validation.

Do I understand correctly that next steps here are

  • create another PR on this repo, adding set-minimal-version and set-maximal-version methods with some sensible defaults.
  • implement it in wasmtime, some minimal implementation. Possibly with HTTP2 only and trap when you try to force HTTP1

Am I missing some steps ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants