Skip to content

build: Google gRPC C++ library as an external dependency.#2318

Merged
htuch merged 4 commits intoenvoyproxy:masterfrom
htuch:grpc-bazel
Jan 9, 2018
Merged

build: Google gRPC C++ library as an external dependency.#2318
htuch merged 4 commits intoenvoyproxy:masterfrom
htuch:grpc-bazel

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented Jan 5, 2018

This is needed for upcoming integration with xDS and other gRPC clients, where the option to use
Google gRPC or the in-built Envoy client will be made available.

Signed-off-by: Harvey Tuch htuch@google.com

This is needed for upcoming integration with xDS and other gRPC clients, where the option to use
Google gRPC or the in-built Envoy client will be made available.

Signed-off-by: Harvey Tuch <htuch@google.com>
@mattklein123
Copy link
Copy Markdown
Member

Q: Do we want to start thinking about compile time options as a more first class concept of what people want to include? I suspect very few people are going to want to actually use this. Obviously we need this for CI (and I worry about CI speed as we add more dependencies when bazel caching from the image seems broken), but just wondering if we want to start thinking about build options.

@ggreenway
Copy link
Copy Markdown
Member

What's the difference between gRPC and the envoy client? Can we eliminate one or the other to keep things simpler?

@mattklein123
Copy link
Copy Markdown
Member

@htuch will be able to provide more history around how we arrived here. It's a long, relatively sad story. :(

I do agree that the existence of the new options in data-plane-api and the new code here are going to be very confusing for users. I think we should have pretty clear documentation and guidance on why we have these two options and what we are working towards.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Jan 7, 2018

@mattklein123 Yeah, let's think about compile time options. We can continue with the current approach of adding a --define google_grpc=disabled to opt out? Or something different?

@ggreenway Yeah, this is a long story that @mattklein123 @ctiller @markdroth and I have discussed over a series of meetings. I'll try and recap below, we'll eventually turn this into the clear documentation/guidance that @mattklein123 suggests.

There are various places we interact with gRPC in Envoy, for example on the data plane (proxying gRPC from client to backend), transcoding filters (e.g. gRPC-Web), sourcing configuration via xDS and in filters that make call-outs (e.g. an authorization or rate limit filter). For the latter two cases, we use the Envoy Grpc::AsyncClient today.

Grpc::AsyncClient is a thin shim on the builtin Http::AsyncClient, i.e. a thin layering of gRPC on HTTP/2. It makes use of Envoy's inbuilt mechanisms for timeouts, retries, endpoint discover/load balancing/failover/load reporting, circuit breaking, health checks, outlier detection. Since the target for a Grpc::AsyncClient is just a cluster name, control plane configuration and filter targets are treated very uniformly, the same as regular data plane clusters and upstreams. Users of this client achieve configuration and monitoring homogeneity.

OTOH, the Google C++ gRPC library (https://github.com/grpc/grpc) is the reference gRPC implementation. It is a very high performance implementation of gRPC which is largely consumed as a black box with a threaded model that is not highly aligned with Envoy's silo model (my later PRs will bridge this somewhat). It has a bunch of features we don't implement in Envoy's Grpc::AsyncClient today. For example, it handles OAuth2 and Google Cloud Platform credentials. It can do gRPC-LB lookaside, and will support alternative transports and security protocols. Some of these will be not even be H2 based. The gRPC client library takes essentially an address, credentials and configuration for stuff like LB and does its own thing in speaking to endpoints. It doesn't follow the cluster/endpoint processing of Envoy.

We would spend a lot of time trying to replicate the Google gRPC library inside Envoy, so we don't strive to reach parity here with Grpc::AsyncClient. At the same time, if we only offered the Google gRPC library, users who don't need all its features but want to specify gRPC targets as regular Envoy clusters would lose out.

So, we plan on offering both, with Grpc::AsyncClient the recommended default approach when only basic features are required in a service mesh, and the Google gRPC client to be used when sophisticated features such as auth, gRPC-LB and other goodies are required.

This isn't set in stone yet, so feedback is welcome, but hopefully this recaps the history accurately and provides the provisional plan-of-record.

@mattklein123
Copy link
Copy Markdown
Member

Yeah, let's think about compile time options. We can continue with the current approach of adding a --define google_grpc=disabled to opt out? Or something different?

That's what I had in mind. If you don't want to do in this PR that's fine, but I think we should start doing this for many of the non-core features. e.g., --define mongo=disabled. I thought we had a ticket tracking this at a high level, but I can't find it from a quick search. I will look again tomorrow.

htuch added 3 commits January 8, 2018 10:12
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@ggreenway
Copy link
Copy Markdown
Member

@htuch Would it be possible, instead of offering both gRPC clients, to provide a simpler way to configure the more sophisticated client? Then people with simpler requirements would still have simple setup/configuration, more complicated requirements could be handled, and we'd only have 1 implementation.

What is the event/threading model of the gRPC library?

@ggreenway
Copy link
Copy Markdown
Member

re: disabling features at compile-time: I think it's worth it to allow disabling features that have external library dependencies. For things that do not, the only benefit is slightly faster builds, and slightly smaller binaries. I personally don't think that's worth the complexity of allowing those things to be compiled out.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Jan 8, 2018

@ggreenway the Google gRPC libs have a threaded concurrency model; part of the complexity in my PR will be around matching the thread/event models, we have some idea of how this works based on Istio Mixer client and a proof-of-concept I have locally.

I don't know if it's really that much simpler to make it build time one client or the other. If you look at the accompanying API PR, envoyproxy/data-plane-api#398, the clients have very different configuration styles, so it's not really a switch that can be applied at build time transparently.

@mattklein123
Copy link
Copy Markdown
Member

Yeah, unfortunately I really don't see any way to offer a single configuration in this case, and it's a non-starter to switch to only Google gRPC client. Google gRPC is not a superset of Envoy gRPC. They basically have very different feature sets with a common base (codec).

@fengli79
Copy link
Copy Markdown
Contributor

fengli79 commented Jan 8, 2018

+1 to #2318 (comment)
AFAIW, Google gRPC enforces the proto message boundary when delivering messages. For data-plane cases, the ability to break the proto message boundary when proxying is important. As you don't want to get envoy waiting for a big proto message be read completely before send it to backend.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Jan 8, 2018

@fengli79 this is an important difference on the data plane, agreed. OTOH, we're only planning on using Google gRPC for the control plane and unary calls in filters (auth checks, rate limit, etc.), so I don't think we have the same big proto issues.

```

## Hot Restart
## Disabling optional features
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup, thanks, was going to ask for this. :)

@htuch htuch merged commit 62a0530 into envoyproxy:master Jan 9, 2018
@htuch htuch deleted the grpc-bazel branch January 9, 2018 10:41
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Rejecting requests which don't have an https scheme if Android disallows cleartext

Risk Level: High
Testing: TODO (envoyproxy/envoy-mobile#2341)
Docs Changes: n/a
Release Notes: inline
Fixes #1572

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Rejecting requests which don't have an https scheme if Android disallows cleartext

Risk Level: High
Testing: TODO (envoyproxy/envoy-mobile#2341)
Docs Changes: n/a
Release Notes: inline
Fixes #1572

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants