-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(tonic-web): implement grpc <-> grpc-web protocol translation #455
Conversation
do you think it would be possible to experiment with this outside of the main crate repo? We could create a tonic-web repo or something? |
Yeah, that works. Where do you want the new repo to live? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, as we discussed, lets move all the interop stuff into docker and make this diff smaller. Once, that is done, ill give the code a much deeper review. Overall, this looks great, looking forward to shipping this!
tonic/src/transport/server/mod.rs
Outdated
@@ -333,7 +333,7 @@ impl Server { | |||
}; | |||
|
|||
let server = hyper::Server::builder(incoming) | |||
.http2_only(true) | |||
.http2_only(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely to cause issues if we turn this off by default. We should think about this impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be on by default. To me, the ideal api to wire up gpr-web is very close to what was proposed in #288, something like:
Server::builder
.add_service(..)
.add_service(..)
.accept_grpc_web(Cors::default())
.serve(addr);
where the builder arranges everything internally and either all services attached to the server are capable of responding to grpc-web requests or none are. This prevents misconfigurations and correct but perplexing protocol or decoding errors that an api like this will cause:
Server::builder
.add_service(GrpcWeb::new(..))
.add_service(..)
.enable_http1(..)
.serve(addr);
where it is possible to:
- Enable http1 unnecessarily
- Wrap services in GrpcWeb and forget to enable http1
- Enabe http1 and forget to wrap some or all services in GrpcWeb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GrpcWeb service, however it ends up being wired-up, would need to handle at least one more case when enabled: http1 requests with application/grpc
content type. In this case it should probably just return bad request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more wrinkle that would probably be nice to handle: Enabling serving gprc-web requests but not http1. The other way around should not be possible though.
I will address your other comments in an upcoming commit and will explore this further once the core translation is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we will need to also ensure alpn still works and maybe also support h2c style upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to confirm but I think alpn works. I don't think h2c is necessary because browsers do not support it anyway.
Would love to work on this effort. Was digging through the improbable-eng/grpc-web repo and their solution for WebSocket transport to support client-side streaming and it is very elegant. Down the rabbit hole and it seems the solution to using WebSocket as a transport and making it seamless to use was building a protoc typescript plugin that both decoupled the transport allow http1.1 & WebSockets and understood javascript WebSockets in the browser, also a go-lang protoc gen for WebSocket upgraded to http2 on the server. The tower-service solution is very elegant as well and was the path I was on prior to finding that this issue has been open for some time. All that to say I believe working on the tonic-build codegen tool to generate Rust code that supports both the grpc-web spec and the improbable extension for WebSocket transport would allow for an end-to-end Rust solution - WASM client -> hyper Rust Server. I am not sure where to start to build that plugin. Some directions on how to contribute to that or this effort would be helpful. Currently, I don't see a way to import a tonic generated grpc client into WASM compile (Is this even necessary?) A can focus on a hardcoded client if that makes sense and copy the protoc generated files and manually change the transport to support the browser. Sorry for the long post. |
Hi @NiQ-B, thank you for your comments. This PR is only focused on enabling tonic servers to handle unary and server streaming grpc-web requests. These are the only two requests defined in the spec and the only two the js and Dart clients support at the moment. Once this PR lands, I would be thrilled to collaborate on future enhancements to tonic's grpc-web support. I haven't looked in any detail into a potential wasm client or a web socket transport but these are my current thoughts: Personally, I see no practical use for a WASM client at the moment. I don't see myself writing web clients in Rust any time soon and compiling a tonic client to WASM just to call it from JavaScript seems unnecessary to me. Having said that, I do think it would be interesting to try and I may explore this at some point in the near future, just out of curiosity. With regards to web sockets, I don't think working on this is worth the trouble at this point. The way forward for client and bidirectional streaming seems to be WHATWG streams, which are not that far off. |
@alce, I appreciate the quick response. There are quite a few practical applications for a WASM client for grpc-web. One problem it would solve would mean you wouldn't have to incur the performance hit in transferring large datasets from WASM on to the js main thread or transmitting & Receiving in a WASM web_worker. WebSockets and the future ReadableStream are attached to the web_sys subsytem meaning that is a native library and not directly a part of javascript it has a javascript interface for both. I firgured once the transport was abstracted within tonic the end user would have the freedom to chose the option that was best for their usecase. ReadableStream not being supported by IE currently makes that a non-starter where Websockets are supported in all the major browsers. I recognize this pull you're submitting is not looking to target these efforts was hoping you could point me in the right direction as to contributing to the tonic-build and protoc area will mention feature request #31 here and comment on that thread for additional suggestions. Thanks again. |
@NiQ-B I see. Here's what we can do, if you agree. Let me finish off a couple of things to have some extra time available and I'll ping you to continue this conversation elsewhere. This is very hand-wavy, but: I don't think we need to mess around with code-gen or wait for the transport to be extracted or think about web sockets to start with. I understand this is ultimately what you want but we could start humble and take it from there. |
Is it still planned to be merged? Any help needed? |
@vicpl It is not clear yet if this will be merged into tonic proper, or in what form (as a separate crate or maybe feature gated). What I do know is that we will eventually polish and publish it, in one form or another. I plan to update this PR once Tonic's new release is ready (or close to), but that won't happen until Tokio 1.0 and hyper 0.14 are ready. When that happens, you could help out reviewing the code or test-driving it. Also, since the initial release will be minimal, you could also suggest or implement missing features. Thanks for the offer. |
@alce Seems like finally we've got tokio 1 and hyper 14. Anything I can do to help out and possibly get this merged/stabilized? In the mean time, we're pinning to this PR so we can start testing the implementation. :) |
This work will not make it into the next release, I plan on getting a simple release out supporting the new tokio then will work on future features like this. |
@KaiserKarel I will update this PR soon so to be compatible with tonic 0.4. Hopefully we can push it through the finish line relatively soon after 0.4 is released. In the mean time, please let me know if you find any issues. |
@alce will do. Thanks for the effort! |
Excited to see grpc-web integrated into Tonic! Had no idea this was on the roadmap so I started work on a proxy this weekend. More than happy to help on this instead if needed. |
Hi, is there any progress on this? I was just about to ask if there's something to do this exact thing:
|
@gregdhill A separate proxy server is a good approach too. I used to run a sketchy one I wrote before working on this implementation. @bragaz I have not done any work on the wasm side of things, and most likely won't. I don't know if someone else has. This PR's only purpose is to allow tonic servers to talk to grpc-web clients directly, without envoy or any other separate proxy. It does not (and will not) include a transport implementation that could be compiled to wasm and used in a browser context. |
@alce what is the status of this? |
@LucioFranco I'll take a look within the next couple of days to see where it stands with the newest changes, but overall it's ready for review. The code has been already updated for tonic 0.4, so I don't expect many tweaks will be needed, if at all. If the general approach seems ok, the last piece is to decide how to integrate it with tonic proper. Should it remain a separate crate or do we want to merge it directly into tonic? Can we piggyback on hyper's http1 feature flag to activate grpc-web support?.. that sort of thing. |
Yeah, I am not opposed to including it within tonic. Just want to ensure that it doesn't cause any issues with pure h2 approaches. |
@LucioFranco I made some small changes and added a few comments to clarify the reasoning behind how requests are handled. I thin this is ready for review. |
|
||
use self::content_types::*; | ||
|
||
pub(crate) mod content_types { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you publish the types and implementations in this file somewhere so that they are accessible from other crates? I'm implementing a wasm grpc-web client and I think this file is completely reusable on the client side. I've copied it for now but would love to be able to just use
it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can move them to a separate crate if they are useful elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just use this crate and we can make these types public right?
@alce I've pushed a WASM grpc-web client implementation: https://github.com/titanous/grpc-web-client The tests are passing against your server implementation here. I built on |
It's not the intention that these changes are merged as is. In particular #[doc(hidden)] should be replaced by docs in each instance.
This PR implements
grpc-web <-> grpc
protocol translation for Tonic services and allows Tonic servers to (optionally) process grpc-web requests directly.It is a continuation of the work performed in #288 and #359. It keeps most of tower-web's
CORS
module but takes a different approach for the actual protocol translation and overall implementation. In particular:tonic-grpc-web
crate.GrpcWeb
type, which is atower::Service
that can be used decoratetonic
services.content-type
andaccepts
request headers, not the http version.application/grpc-web
and textapplication/grpc-web-text
modes.For now, the only change needed in the
tonic
crate is to flip hyper's server http2_only setting to false. This is not strictly necessary though. If the tonic server is configured to accept TLS connections,GrpcWeb
should work with an unmodified tonic transport.The PR includes interop tests. The client is taken directly from
grpc-web's
repo and only slightly modified. The server is a simplified version of the one intonic/interop
.This makes the diff is a bit noisy, particularly because pre-compiled protos for the js client are included. Once it is decided where this code should live and how to best integrate it with tonic's transport (if at all), we could re-use the existing interop server and clean-up all the noise in the client.All this is now hidden in a docker container.Unresolved/Unfished issues
GrpcWeb
type is designed to decorate services but it doesn't have to be. A different design might not even expose the service and wire everything up internally.Expose a CORS builder type. At the moment, cors is configured with some default settings but it should be possible to change the default configuration and even disable it.Done.Add the cors module's tests back and some high level tests of our own.Done.Documentation, once it's decided what should be exposed