connectrpc / connect-go

The Go implementation of Connect: Protobuf RPC that works.
https://connectrpc.com
Apache License 2.0
2.89k stars 96 forks source link

Compatibility layer to work with existing gRPC code #334

Closed jhump closed 10 months ago

jhump commented 2 years ago

gRPC has been around for quite a while now. Lots of organizations have lots of existing code using the client stubs and server interfaces generated by protoc-gen-go-grpc (or its predecessor, the earlier version protoc-gen-go).

This request is for a compatibility layer, that would allow gRPC client stubs to be wired up to a Connect client and gRPC server implementations to be wired up to a Connect server. This would allow an organization to migrate to Connect without having to re-write much of their codebase to refer to new types and interfaces.

This basically boils down to adapting grpc.ClientConnInterface and grpc.ServiceRegistrar to Connect's APIs. Looking at the split of generated code vs. library code with connect, and how generics are used in the library code, it looks like this might require additional generated code. (Though this could possibly be minimized with some additions to both existing generated and library code -- like introduction of a layer that is more generic and transport-agnostic?)

Such a compatibility layer would likely need to be in its own Go module (maybe separate repo?) so as not to pull the gRPC runtime packages into Connect's dependencies.

jhump commented 1 year ago

I was toying around with this the other day. It can be done without any code generation. But it's much more complicated because you then have to adapt to gRPC's ClientConnInterface and ServiceRegistrar abstractions, which then requires adapting gRPC's stream interfaces, which are generic and ostensibly bidirectional (same interface used for all three kinds of streaming calls). I can push up a branch if you want to see; unclear if that's the direction we'd want to go (tradeoff between generating more code or living with higher implementation complexity).

Aside: I noticed that Connect currently strictly disallows bidirectional calls over HTTP 1.1. That is a bit of a shame because half-duplex bidirectional RPCs can work fine. It's only full-duplex (where both sides can send concurrently, vs. server waiting for client to finish and half-close) that is not possible with HTTP 1.1.

FWIW, in FullStory's httpgrpc, half-duplex bidi is supported, and thus so is server reflection: the reflection client must simply issue one request at a time and half-close after each one (so loses the benefits of streaming, but gains the ability to use reflection over HTTP 1.1).

akshayjshah commented 1 year ago

Aside: I noticed that Connect currently strictly disallows bidirectional calls over HTTP 1.1. That is a bit of a shame because half-duplex bidirectional RPCs can work fine. It's only full-duplex (where both sides can send concurrently, vs. server waiting for client to finish and half-close) that is not possible with HTTP 1.1.

That's definitely true; I disallowed this outright to make the error messages comprehensible. I'd love to relax this restriction a bit, but we'll need to detect this case and provide better errors to clients.

jhump commented 1 year ago

For the curious, here's a working branch that allows use of gRPC-generated client stubs and server interfaces. You can see in the test how it's used (in case the Go docs aren't clear): https://github.com/bufbuild/connect-go/compare/main...jhump:connect-go:jh/grpc-adapter

(The test failures are because the Connect runtime does not support any bidi streaming requests over HTTP 1.1, but the test suite being used only has a way to turn off full-duplex streaming tests; if we ignore the half-duplex streaming tests which are failing, everything else is good.)

I'm not opening a pull request because, looking at the code, it's quite complicated... and there's kind of a lot of it. There are two alternative approaches that might be better ways to proceed:

  1. Use code generation to provide support for the gRPC generated interfaces. This could definitely simplify a lot of this, and it would move some of the logic out of the runtime and into generate code. (Is that the right tradeoff?)
  2. Contribute support for Connect and grpcweb protocols to fullstorydev/grpchan's httpgrpc package. (There's actually an issue already filed about supporting grpcweb.) This would provide compatibility, but without making use of the actual connect-go runtime. But it would be simpler to implement that way since the protocol differences between Connect, grpcweb, and grpchan's gRPC-over-HTTP-1.1 support is not that great. (Though grpchan already has some level of support for application/json content-types, but uses a slightly different protocol in its response format. It also does not currently include any support for compression 🤔).
akshayjshah commented 1 year ago

In the same vein as @jhump's second option, we could also ship a server-side net/http middleware that translates from the gRPC-Web and Connect protocols to standard gRPC. (It'd be similar to improbable's grpcweb package, but with the addition of the Connect protocol.) This works with grpc-go's years-old but still experimental Server.ServeHTTP method, which implements http.Handler.

I like this approach because it's narrowly-scoped, has very little API surface area, doesn't pull in any extra dependencies, and can be shipped and maintained right in connect-go.

emcfarlane commented 1 year ago

Started to look at this here: https://github.com/bufbuild/connect-go/compare/main...edward/transcode-handler

Applying the test matrix it is looking promising, but theres a couple issues that need resolving:

  1. Connect unary requests need to be buffered to encode the gRPC envelope.
  2. Connect unary responses need to be buffered to capture gRPC trailers and encode them as connect Trailer- headers.
  3. Connect Unary/Stream is identified by the request Content-Type, half-duplex requests would fail as the handler doesn't identify the right Content-Type.

1 and 2 could be solved by a configuration option to limit payload sizes. 3 is blocking.

jhump commented 1 year ago

Could you elaborate on 3? I don't see the blocker there. All streaming methods, regardless of type (client-only, server-only, half-duplex bidi, and full-duplex bidi), use the same content-type (protocol reference).

emcfarlane commented 1 year ago

@jhump thanks! Misread the spec picking back up.

emcfarlane commented 10 months ago

https://github.com/connectrpc/vanguard-go has just been released to address adapting connect clients to grpc-go backend servers along with supporting REST and other protocol translations. See the docs for vanguardgrpc!