Add gRPC Web support.

[sergiocampama]

Initial implementation for supporting gRPC Web directly from
Swift gRPC services.

[sergiocampama]

This is an initial draft for adding support for gRPC Web. I have a local server that I'm testing with, and I'm able to make gRPC connections using cURL with HTTP1.1.

This implementation relies on having apple/swift-nio-http2#36 merged and released, as HTTP2Parser does not support being added dynamically into the pipeline.

[sergiocampama]

The error in Travis is because of apple/swift-nio-http2#36.

[sergiocampama]

Added CORS management, with the latest version I can get the gRPC demo app to work without the Envoy proxy.

[MrMage]

This is great, thank you so much! I was hoping that using NIO'S HTTP1-based primitives would make it fairly easy to add support for gRPC-Web/pPRC, and it looks like that is the case.

I also really appreciate the sample code for serving via NIO; this had been on my Todo list for a while.

I'll give this a thorough review once I return from the holidays. Until then, two questions:

1. Could you give me two sentences of instructions to test this myself with the demo app to make the review easier for me? I have no experience with the gRPC-Web codebase.
2. Given that gRPC-Web aims to support gRPC via vanilla HTTP1 requests (if I'm not mistaken), do you think it would be possible to add some simple test cases (using e.g. NSURLSession or other Foundation HTTP code to send the requests in the tests; if you encounter problems getting those tests to pass on Linux simply run them on macOS only) for all the request methods supported by our gRPC-Web implementation?

Hope this makes sense; let me know what you think. Again, thanks a lot and will review this properly soon!

[sergiocampama]

Thanks! I wasn't expecting an expedite review around the holidays :)

I added a new commit that adds the EchoWeb example. You should just need to install npm and then just run make.

Even with this PR, you'll still need to patch apple/swift-nio-http2#36 locally. What I did was to do a swift build --product Echo to populate dependencies, and then modify the .build/checkouts/swift-nio-http2.git-8449925081683998237/Sources/NIOHTTP2/HTTP2Parser.swift file manually to add the patch. With this, you should be able to run swift run Echo serve_nio and then open the EchoWeb/index.html file in a browser.

RE: Tests, that should be fairly straight forward, I'll look at adding them over the next few days.

Happy holidays!

[sergiocampama]

Added tests, only tested them in macOS, I don't have a linux machine with easy access for dev/tests.

[MrMage]

Excellent work, thank you very much! I am very happy with the entire PR; the comments are almost exclusively nitpicks, formalities and clarification questions.

[MrMage]

Could you add a TODO to add tests and/or investigate this more?

[sergiocampama]

apple/swift-nio-http2#36 was merged, we'll need to wait for a release now

[MrMage]

IIRC we are currently pinning to a specific NIO-HTTP2 commit; feel free to check Package.swift and update the pinned commit. Then we could merge earlier.

[MrMage]

Very happy with how things are progressing. Just a few remarks, and will wait for the Package.swift update before merging.

[MrMage]

I understand the necessity to cache all responses to ensure a well-formed base64 stream, but doesn't buffering all of them defeat the purpose of streaming? How about instead trying to send all but the last length - (length % 3) bytes, as those should always result in four well-formed base64 bytes? Please at least consider that when refactoring the base64 handling out of this class in a separate PR.

[sergiocampama]

Isn't that the issue with HTTP1.1, that there is no proper server streaming? I may be wrong though. if we did do that, we'd still have an issue with some streams, as if there are 2 bytes missing for a full base64 frame, we'd have to wait until the trailers or the next messages to arrive before sending it, otherwise we'd be sending incomplete messages. Perhaps that's ok? not sure

[MrMage]

HTTP1.1 already has chunking, which is essentially server streaming. Also, I think most gRPC clients should be capable of waiting for the next chunk mid-message, as the gRPC spec says that clients should be chunking-agnostic. I'd assume the same applies for HTTP1.1 — the client just receives a byte stream and waits for a full message to arrive before trying to decode it.

Would you mind adding a FIXME for this?

[sergiocampama]

Set the commit with the fix in Package.swift as well

[wenbozhu]

I have been working on grpc/grpc-web and the grpc-web spec. @timburks asked me to have a look at this - and thanks for doing it!

I don't really know swift ... but I'd like to propose a few general requirements here from the point view of grpc-web.

1. Use the "standard" HTTP or web framework in Swift to handle the grpc-web traffic. I.e. handle the HTTP traffic using the high-level server API provided by the language platform, which may support only HTTP/1.1 or both HTTP/1.1 and HTTP/2.
2. Proxy the grpc-web traffic using something like in-process channels (if supported) or domain sockets. I.e. make this an in-process proxy, v.s. some kind of alternative "transport" to grpc runtime.
3. Implement only the core spec and a minimum set of browser-specific features. The latter will evolve, e.g. security features, JSON-compatible text encoding for protobuf, keep-alive support etc.

Could you comment on how you would support the above requirements in grpc-swift, or if you have any concerns or questions on any of the requirements?

Thanks.

[MrMage]

Thanks for the insights, @wenbozhu! The point of the way we implemented the gRPC protocol is actually that it is based on Swift‘s recommended way of handling HTTP. This also lets us handle HTTP1 and HTTP2 almost identically, avoiding wasted effort due to duplicated code. So I guess that would have us conform to point 1.

Could you please elaborate more on points 2 and 3, however? I don’t quite understand yet what they mean, and what they are opposed to, i.e. how things should not be done.

[MrMage]

Also, could you elaborate how points 1 and 2 are related? Point 1 sounds to me like „use the language-specific HTTP serving functionality“, while point 2 sounds like „use the gRPC-Core runtime“ which would be the opposite. I’m probably misunderstanding one of the two points.

[sergiocampama]

Re: point 3, this PR already supports CORS. Missing is the compression section. Could you elaborate on what HTTP status code mapping means?

[MrMage]

LGTM from my side, but I'd like to wait for further comments from @wenbozhu and/or @timburks before merging. @timburks, please merge at your convenience — I'm going to be OOO for a while, so simply merge when you feel ready to.

[sergiocampama]

Thanks!

[Lukasa]

Incidentally, the fix has landed in swift-nio-http2 0.2.1.

[sergiocampama]

even better, thanks @Lukasa!

[wenbozhu]

So point 1 shouldn't be a concern for grpc-swift.

Point 2 is about the request path using a grpcs-swift client to call into the grpc server end-point. I.e. some HTTP handler terminates the browser request, parses the data, and then uses a generic grpc client to call into the server. This might introduce some overhead but will make the whole request path more compatible (to the server). I am open to hear about objections, esp. for a gRPC implementation which uses the language-platform provided HTTP stack (for HTTP/2).

Point 3 is about not implementing advanced features and leaving those features to the "official" grpc-web proxy. The thinking is that most web-tier deployment involves a reverse proxy, and we are expecting Envoy will dominate, e.g. with k8s etc. I.e. in-process grpc-web support (per language) only implements a minimum set of mandatory features to enable grpc-web but leave to Envoy for the production support of grpc-web clients. If this make sense, we can start to identify those features on the spec doc.

[MrMage]

Point 2 is about the request path using a grpcs-swift client to call into the grpc server end-point. I.e. some HTTP handler terminates the browser request, parses the data, and then uses a generic grpc client to call into the server. This might introduce some overhead but will make the whole request path more compatible (to the server). I am open to hear about objections, esp. for a gRPC implementation which uses the language-platform provided HTTP stack (for HTTP/2).

Sorry, I still don't understand. I don't think we have plans to support gRPC-Web on the client-side in SwiftGRPC, only on the server-side. How would this affect us, then?

FYI, on the server-side, we use a (SwiftNIO-)framework-provided HTTP2-to-HTTP1 mapper such that our gRPC implementation only needs to deal with HTTP1 primitives, which are easier to reason about and give us gRPC-Web support for (almost) free.

[sergiocampama]

1: OK

2: Isn't that an implementation detail on how to process the request? How does that relate to the protocol? If grpc-swift adheres to the spec, why does it matter how it is implemented? Swift NIO allows us to package each piece of the logic in very clear blocks that can be added/removed on the fly, so IMO the extra complexity is not required as long as the spec is respected.

3: While that may be true for most deployments, I don't think it should be translated to "The only supported way to use gRPC-Web is through a reverse proxy", which means that each language should be free to decide to implement any features above the minimum required set of features to support gRPC-Web, including not needing (but supporting) a reverse proxy frontend.

In general, I think I'm confused when you join both points 2 and 3, since one is saying how to implement the server to support gRPC-Web, and the other is saying that Envoy should be used as the frontend, which means that there is no need to support gRPC-Web in server libraries at all.

[MrMage]

2: Isn't that an implementation detail on how to process the request? How does that relate to the protocol? If grpc-swift adheres to the spec, why does it matter how it is implemented? Swift NIO allows us to package each piece of the logic in very clear blocks that can be added/removed on the fly, so IMO the extra complexity is not required as long as the spec is respected.

💯

3: While that may be true for most deployments, I don't think it should be translated to "The only supported way to use gRPC-Web is through a reverse proxy", which means that each language should be free to decide to implement any features above the minimum required set of features to support gRPC-Web, including not needing (but supporting) a reverse proxy frontend.

FWIW, as a user I would be fine with using a reverse-proxy in front of the actual service. However, I agree with @sergiocampama that if the gRPC-Web implementation built into SwiftGRPCNIO is 'good enough' for a particular user, they should be free to just use that.

[wenbozhu]

@sergiocampama

2. All fair points, as I mentioned, esp. if the grpc implementation shares the same HTTP stack as grpc-web. I agree this is mostly a language-specific implementation choice, and we are yet to identify cases when using grpc clients to proxy requests is more beneficial, e.g. "advanced flow-control" ... (just making up one).

I'd be OK to suggest the swift approach is desired only when the grpc and grpc-web (http/1) share the same stack provided by the language platform directly.

3. This is mostly a maintenance concern. A lot of grpc-web use cases are for local development, debugging, for which the in-process server will work just fine, with a minimum set of features. But for more advanced features e.g. a different protobuf encoding, we want to limit the implementation to ideally just Envoy. Some features could be under-specified too so there will be compatibility concerns too. E.g. all the gRFCs.

Again, I am just throwing out some thoughts here, and your opinions are very valuable.

[sergiocampama]

Thanks for the explanations! The last one is a valid point. It'd be great if grpc-web specs were expanded to describe 3 different sets of functionalities:

• required spec to interop with grpc-web clients at a minimum (e.g. grpc-web/grpc-web-text decoding/encoding, CORS)
• optional grpc-web spec that grpc servers can implement if they want (e.g. grpc-web-text+json)
• list of features recommended to not implement in grpc servers that proxies like Envoy should provide instead (e.g. compression, other features that are not as well defined)

What I'm thinking is that there could be a minimum set of features that can be used when developing/debugging, and for production apps, client code can be built in an optimized mode that uses the advanced features that Envoy should provide.

I'm also spitballing here, so please let me know if these make sense. In the mean time I don't believe this change conflicts with your original concerns.

[wenbozhu]

@sergiocampama All agreed. I will go ahead and put together a "guideline" on language-specific implementations of grpc-web proxy, along with minimum/optional features identified on the spec. Will put you as reviewers of the doc changes. Thanks. @timburks

[wenbozhu]

https://github.com/grpc/grpc-web/blob/master/IN-PROCESS-PROXY.md

[timburks]

I'm not sure what the right response here should be (or if this is a specific gRPC-web problem), but currently I can crash EchoNIO by connecting to the server with curl http://localhost:8080:

$ swift run EchoNIO serve
starting insecure server
Precondition failed: : file /Users/timburks/Desktop/grpcweb/grpc-swift/.build/checkouts/swift-nio.git-3446246794268775558/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift, line 67
Illegal instruction: 

[Lukasa]

That's a NIO precondition, and it gets hit if the HTTPServerRequestParts are being sent out-of-order. Specifically, in this case, we received a sequence of parts where we received a .end before a .head.

[sergiocampama]

That error you're seeing @timburks is because we're not currently handling unsupported requests, in this case, GET requests.

If you try:

curl -H "Content-Type: application/grpc-web-text" -d AAAAAAsKCWNhY2EgY2FjYQ== http://localhost:8080/echo.Echo/Get

You should get data back, which, when base64 decoded, represents the binary proto response.

[timburks]

Merging this today. @sergiocampama thanks for this great work, and @MrMage and @wenbozhu for your thorough reviews!

[MrMage]

Glad to see this merged 🎉🚀

@Lukasa crashing when we see unexpected requests sounds dangerous; is this planned to change in future iterations of NIOHTTP2, or maybe has already changed with your pure-Swift implementation? I haven't gotten around to update our dependency to that implementation yet and verify that our tests still pass when using it.

[Lukasa]

@MrMage That crash is coming from NIO, but I believe it’s not our fault. As I said, it’s when user code writes out HTTP parts in an invalid order, so it’s worth investigating how that error path issues writes.