-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
integrate gRPC C++ directly into Envoy #511
Comments
@fengli79 here is the issue to fully integrate gRPC for local origin outbound calls. (Though not for proxied calls most likely, which I think is what you want for gRPC-web). If you feel like working on this that would be great. From talking to @ctiller, it seems like we should be able to build a gRPC channel which consumes an Event::Dispatcher and uses it as the event loop. The easiest thing would probably be to look at the Node implementation which does mostly the same thing according to @ctiller. |
Yes, that's doable, and it has been discussed several times.
However, for non-transcoded proxied calls, it's optional but still preferred.
The #2 choice will get unlocked by this issue, and it sounds like worth to resolve first in spite of the choice we make for grpc to grpc proxying. |
One more thing, it's orthogonal to whether use TSI with HTTP traffics, since grpc as a dependency will always be there. |
@fengli79 @ctiller I was thinking about this more and unfortunately I think I greatly under-estimated how complicated this is going to be (this should have occurred to me during the in person meeting but unfortunately it did not). I agree that it should be relatively simple to integrate the C++ gRPC client for local origin outbound calls into the event loop. However, the current local origin AsyncClient calls run through the entire Envoy router stack which means they support retry/timeouts/circuit breaking/load balancing, etc. We make use of these capabilities at Lyft extensively for auth/ratelimit calls, and I assume the Istio folks want this also for mixer calls, etc. So, either, we have to largely reimplement (or adapt to similar semantics assuming the functionality already exists) a bunch of Envoy router/CM functionality inside of the gRPC stack, or we need to step back and figure out if there is another sensible way to merge the 2 stacks. (The place to merge them at least at an interface level would be I think at the transport layer. We could implement a gRPC Envoy transport that essentially uses the HTTP/2 AsyncClient and a variant of #381). I know @ctiller was very much against integrating at the transport layer interface, but now that I have thought about it more, it seems like the correct place to do it. So, TLDR, even local origin calls I think are complicated. Using the gRPC library in the fully proxied path is going to be a herculean effort that frankly I'm not sure makes sense. So I would rather ignore that for now until we figure out if we can get local origin calls working sensibly. I know there is a desire to use a single HTTP/2 stack for gRPC, but it's not clear to me that it's important enough to warrant the work required to make it happen. Also, I agree that TSI orthogonal. We can make use of that library directly when needed. Here is what I would suggest we do concretely:
cc @louiscryan |
@mattklein123 Just to clarify -- by " I agree that TSI orthogonal. We can make use of that library directly when needed", Envoy is still willing to support look-aside TSI by bundling the gRPC lib, right? cc @wlu2016 |
@myidpt yes, definitely. I would like to fully support gRPC library also, however, this is a really thorny issue and I'm not sure there is a clear solution right now. It might take some real investment to make a full/complete integration work in a sensible way. |
Working on getting gRPC library supports libevent. It will use a given event_base*, thus it can be bound to the same event loop hosted by Envoy. |
@fengli79 what is the end goal here? It's really not clear to me what making gRPC works with the Envoy event loop buys us, since presumably we still want all of the Envoy AsyncClient/Router features per discussion above. In a perfect world we could combine them somehow. |
@fengli79 @ctiller @louiscryan I'm closing this for now. I don't see any near term reasonable path forward on this. We will reimplement the ability to make streaming gRPC calls within Envoy like we do for unary calls. We can revisit if/when we explicitly need functionality that would require substantial code duplication. cc @htuch |
Signed-off-by: Mandar Jog <[email protected]>
* Upgrade to cel-cpp v0.2.0 to pick up ANTLR thread-safety changes. (envoyproxy#514) * Upgrade to cel-cpp v0.2.0 to pick up ANTLR thread-safety changes. Signed-off-by: Tristan Swadell <[email protected]> * emit error if an error value is returned (envoyproxy#511) Signed-off-by: Mandar Jog <[email protected]> Co-authored-by: Tristan Swadell <[email protected]>
Signed-off-by: Mandar Jog <[email protected]>
Description: set stream options to buffer stream body for retries. Risk Level: med - the client now buffers stream body for retries. Testing: local - lyft and example apps. Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: set stream options to buffer stream body for retries. Risk Level: med - the client now buffers stream body for retries. Testing: local - lyft and example apps. Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Per off site meeting:
The text was updated successfully, but these errors were encountered: