Skip to content
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

Closed
mattklein123 opened this issue Feb 25, 2017 · 9 comments
Closed

integrate gRPC C++ directly into Envoy #511

mattklein123 opened this issue Feb 25, 2017 · 9 comments
Labels
enhancement Feature requests. Not bugs or questions.

Comments

@mattklein123
Copy link
Member

Per off site meeting:

  • It should doable to use the gRPC C++ library to make outbound calls with the real stubs without doing any thread jumping. We might need a bit of guidance from @ctiller but it I think it could be done pretty quickly.
  • Once this is done, we can stop using the "generic RPC" legacy code hacks, and actually use real stubs for ratelimit, auth, etc.
  • We will eventually need to use the TSI library directly and have some type of TsiConnectionImpl class for upstream connections. This seems lower priority.
@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Feb 25, 2017
@mattklein123
Copy link
Member Author

@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.

@fengli79
Copy link
Contributor

Yes, that's doable, and it has been discussed several times.
Will find some time next week to start this.
A full stack grpc client will definite improve the stability and compatibility.
I would like to use the async generic grpc C++ client for grpc web.
In general, for all outgoing grpc calls from envoy, full stack grpc client is preferred.
Other http2 stack should also work (like nghttp2), however, in practical, it needs to pay more additional engineering cost to test/debug them once compatibility issue comes.

  1. for envoy local origin grpc calls.
  2. for grpc calls get translated from other incoming protocols. like grpc-web/REST, etc.

However, for non-transcoded proxied calls, it's optional but still preferred.

  1. for proxied grpc calls (incoming and outgoing are both grpc)
    Two solutions here:
  2. http2 proxying
    Assuming, re-multiplexing is required, might need additional care to support flow control well.
    Also get limited to HTTP2, and cannot deal with other underlying protocols which supported by grpc.
    Need relies on the TSI library works, etc.
  3. grpc server + grpc client
    Need to support the envoy CM configure, assumes it's not hard.
    Gets all supported underlying protocols works, without extra engineering cost in the future.
    Gets all grpc runtime features works with assured stability/compatibility.

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.

@fengli79
Copy link
Contributor

One more thing, it's orthogonal to whether use TSI with HTTP traffics, since grpc as a dependency will always be there.

@mattklein123
Copy link
Member Author

@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:

  • @fengli79 could possibly spend a couple of days actually looking at integrating the gRPC C++ library just for outbound calls, but while still maintaining all of the existing router functionality in AsyncClient.
  • Then let's circle back and discuss the findings before we start implementing in earnest.
  • Per above let's ignore the fully proxied/transcoded path currently, as I think that is an order of magnitude more complicated of a problem.

cc @louiscryan

@myidpt
Copy link
Contributor

myidpt commented Feb 28, 2017

@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

@mattklein123
Copy link
Member Author

@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.

@fengli79
Copy link
Contributor

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.

@mattklein123
Copy link
Member Author

@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.

@mattklein123
Copy link
Member Author

@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

mandarjog added a commit to mandarjog/envoy that referenced this issue May 15, 2020
istio-testing pushed a commit to istio/envoy that referenced this issue May 18, 2020
* 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]>
lizan pushed a commit to lizan/envoy that referenced this issue Jun 4, 2020
jpsim pushed a commit that referenced this issue Nov 28, 2022
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]>
jpsim pushed a commit that referenced this issue Nov 29, 2022
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Development

No branches or pull requests

3 participants