search

twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.2k stars 326 forks source link

GRPC support #2

Closed jhaynie closed 6 years ago

jhaynie commented 6 years ago

I really like this library and think it would benefit from being able to support both standard RPC as well as GRPC as well. Looking through the code it seems to only generate standard HTTP RPC bindings. Is there interest in also supporting GRPC as well in the same implementation? For certain use cases (at least unary) having support for both is important to us. Seems like it would be rather easy to add support for GRPC as well and you get the best of both worlds without the silly grpc gateway mess.

jhaynie commented 6 years ago

thinking about this a bit more ... i suppose there's no reason I couldn't just call both generators (yours and the standard one) to get both GRPC and RPC support ...

i do like the idea of a unified interceptor / call back model though.

spenczar commented 6 years ago

This sounds interesting. Could you elaborate on what support would look like?

If your service only has unary methods, then I think that twirp's generated interface is pretty much the same as the one that grpc wants. I think you're able to plug a service implementation into both gRPC and twirp today. We don't have a test verifying this regularly though - we should add one if this is a valuable.

jhaynie commented 6 years ago

Yeah, I believe you're absolutely correct and I'm going to try it now. 👍

jhaynie commented 6 years ago

I was able to generate twirp along side my current GRPC and it seems to interop ok. of course, i now have 2 interfaces that are essentially the same and 2 different implementations of the protobuf for the structs for the RPC (request/response). But that's OK given that Go doesn't care about multiple interfaces as long as they are the same.

Looking through the generated code, it would be nice to allow the XYZPathPrefix generated variable to be changed. In mine, it generates a const name AgentPathPrefix and because it's a const I can modify it. That means that path prefixes on the route are forced to be /twirp/v1.Agent/ (in my case) ... I'm a bit worried about exposing this product name into the URL (for both security and style reasons). Ideally this isn't a const since it's public and you can simply override it. Or better yet, we provide an optional WithOption pattern in Go where you can pass in an optional function which can set parameters like this when you create the server...

spenczar commented 6 years ago

Thanks for giving it a quick test, and glad to hear it works!

You can open a separate ticket for mutable path prefixes, but I don't think I'm going to accept that change - using a truly fixed, constant URL scheme means that routing is defined at the protocol level for Twirp. That makes it way easier to write cross-language clients and use stuff like curl - otherwise, you don't know whether the server swizzled things around and is using a custom path on you. There are just too many advantages to having a guaranteed routing scheme at the protocol level.

Regarding the style: yeah, I understand, it's kind of blechy because this isn't how people ever design URLs when they do them by hand. But, hopefully, it's hidden from view if your clients are generated. If not, well, at least it's a very, very simple. I think the simplicity outweighs the smelliness, here.

I would be interested to hear more about any security concern you have in exposing your product name. You can use a codename in your .proto file (or in its package, etc) if you want to be stealthy.

jhaynie commented 6 years ago

If we ended up exposing these services publicly as part of our API, I would have a real problem as a company having a product name like this in the public URL. For a couple of simple reasons: (1) vanity, if we ever changed the generation or product we're stuck this specific url or have to do weird url re-writing and (2) if you have a security issue, hackers immediately could target these public urls. same reason people obfuscate or make generic the "server" tag in HTTP response headers to make it harder to sniff servers.

however, i get the protocol stable nature on the other side for clients.

if you're not going to accept something like this (understandable, your sandbox), i won't bother creating another issue and will just fix it on my side through post processing. totally get it.

spenczar commented 6 years ago

Ah yes, I understand your security concern better now. They're definitely valid concerns, and it's a genuine design tradeoff, but I'm comfortable sticking with constant routing. Thanks for being understanding!