api: Google gRPC client library configuration. by htuch · Pull Request #398 · envoyproxy/data-plane-api

htuch · 2018-01-05T19:07:37Z

In support of envoyproxy/envoy#2200 and some
Google internal needs, we are planning on adding support to Envoy to
allow a configuration (or possibly build) driven decision on whether to
using the existing Envoy in-built Grpc::AsyncClient or
the Google C++ gRPC client library (https://grpc.io/grpc/cpp/index.html).

To move in this direction, the idea is we have the xDS ApiConfigSources,
rate limit service config and other filter configurations point at a
GrpcService object. This can be configured to use an Envoy cluster,
where Grpc::AsyncClient will orchestrate communication, or to contain
the config needed to establish a channel in Google C++ gRPC client
library.

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

In support of envoyproxy/envoy#2200 and some Google internal needs, we are planning on adding support to Envoy to allow a configuration (or possibly build) driven decision on whether to using the existing Envoy in-built Grpc::AsyncClient or the Google C++ gRPC client library (https://grpc.io/grpc/cpp/index.html). To move in this direction, the idea is we have the xDS ApiConfigSources, rate limit service config and other filter configurations point at a GrpcService object. This can be configured to use an Envoy cluster, where Grpc::AsyncClient will orchestrate communication, or to contain the config needed to establish a channel in Google C++ gRPC client library. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-01-05T19:11:24Z

@mattklein123 @wora @ramaraochavali @lizan

I'm not exactly thrilled by this PR, but it's the Least Bad Option IMHO. The fundamental issue is that if we want to support both Envoy's internal notion of specifying backends via clusters for Grpc::AsyncClient and the Google C++ gRPC library, we need to have two overlapping but semantically distinct ways of providing configuration. In particular, it's clear that some bits of the Google C++ gRPC library correspond to Envoy concepts such as UpstreamTlsContext (the SslCredentials), but it seemed clearer to me to use the Google C++ gRPC definitions, as they are much more restricted.

I'm open to ideas on how to better bridge the two worlds, consider this PR a strawman.

@ctiller @markdroth for gRPC-side visibility and comments.

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

mattklein123 · 2018-01-05T21:33:03Z

api/grpc_service.proto

+// <envoy_api_msg_ApiConfigSource>` and filter configurations.
+message GrpcService {
+  oneof target_specifier {
+    // The name of the upstream gRPC cluster when using Envoy's gRPC client. SSL


nit: s/SSL/TLS everywhere.

We should keep it as SslCredentials to match the terminology used in the Google gRPC library for that specific message, but elsewhere +1.

Sure sounds good.

mattklein123 · 2018-01-05T21:52:51Z

api/grpc_service.proto

+  }
+  // A set of credentials that will be composed to form the `channel credentials
+  // <https://grpc.io/docs/guides/auth.html#credential-types>`_.
+  repeated Credentials credentials = 4;


WDYT about splitting GrpcService into basically:

Common elements.

A one_of between "envoy gRPC" and "google gRPC." (Which are discrete messages).

IMO ^ is a bit better because then it becomes more clear on what the split is around creds and possibly other things in the future.

None of the code that uses GrpcService is locked yet, but will be soon at Lyft, so now is the time to sort this out for sure.

Do you plan on plumbing Credentials into the Envoy gRPC client? If not, should it go inside Google gRPC client?

I think we want to leave this option open at the API level. For now, there is no immediate plan, as the recommendation will be to "just use the Google gRPC client if you want auth". However, there might emerge later use cases where folks will want to stick with standard Envoy cluster management for their gRPC service backends but want auth, where someone is willing to spend the cycles doing the lifting here. So, let's keep it open. I will document the current limitations.

Why is this form of auth common between the types, but SslCredentials are specific to google-grpc?

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

wora · 2018-01-06T23:32:18Z

I recommend we use a single PR just to finalize the GrpcService design, then we do the refactoring. I don't think GrpcService exists today, so adding it would have no risk and it is much easier to reason about it. Another issue is there will be many gRPC clients talking to gRPC servers, we want the GrpcService can be used by them as well. Everyone will have the same issue as we just discovery ourselves. We should take a bit time to do it right.

mattklein123 · 2018-01-07T00:57:14Z

We are going to be using this in production at Lyft via the access log service very shortly. I would like to work on Monday to get this into decent shape. I'm not suggesting we split up GrpcService, just have sub-types which include a common part, Envoy gRPC, and Google gRPC. As much as I would like to drop Envoy gRPC and use Google gRPC, until Google gRPC has complete parity with all Envoy router features that is not possible.

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

htuch · 2018-01-07T12:37:06Z

Updated. I'm hoping this is self-contained enough we can just agree on what GrpcService should look like, but I can split up if that needs to happen first.

mattklein123

Thanks, I like where this is heading. Two comments.

mattklein123 · 2018-01-08T04:59:30Z

api/grpc_service.proto

+  }
+  // A set of credentials that will be composed to form the `channel credentials
+  // <https://grpc.io/docs/guides/auth.html#credential-types>`_.
+  repeated Credentials credentials = 4;


Do you plan on plumbing Credentials into the Envoy gRPC client? If not, should it go inside Google gRPC client?

mattklein123 · 2018-01-08T05:00:20Z

api/grpc_service.proto

+  oneof target_specifier {
+    option (validate.required) = true;
+
+    // Envoy's in-built gRPC client.


We will need more documentation/guidance on which to choose, potentially here and in the architecture overview. Do you want to do that as part of this PR?

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

mattklein123

LGTM, thanks for the doc additions. I would recommending having @ggreenway and @wora take a look also.

ggreenway · 2018-01-08T18:31:57Z

api/bootstrap.proto

+
+    // Specifies the cluster manager cluster name that hosts the rate limit
+    // service. The client will connect to this cluster when it needs to make rate
+    // limit service requests.


Is it worth documenting the difference between the two choices in the one-of? Maybe not, because this one is deprecated. But which gRPC client will get used in the deprecated case? Or is it even the same protocol between these two?

ggreenway · 2018-01-08T18:38:08Z

api/grpc_service.proto

+  }
+  // A set of credentials that will be composed to form the `channel credentials
+  // <https://grpc.io/docs/guides/auth.html#credential-types>`_.
+  repeated Credentials credentials = 4;


Why is this form of auth common between the types, but SslCredentials are specific to google-grpc?

htuch · 2018-01-08T19:43:48Z

@ggreenway The SslCredentials are specific to Google gRPC because they are already there implicitly in the cluster upstream TLS context for Envoy gRPC.

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

As part of the switch to envoy::api::v2::GrpcService and preparing the code base for the Google C++ gRPC client, this PR introduces a singleton gRPC async client manager and factory pattern. This allows for details of TLS and shared state across clients to be hidden from consumers and for consumers to be agnostic about the type of gRPC client. This PR also introduces the deprecation of the specification of gRPC services by cluster name, envoy::api::v2::GrpcService should be used going forward. Risk Level: Medium (some changes to initialization, API cluster validation) Testing: New unit tests, validated full coverage. Docs Changes: envoyproxy/data-plane-api#398 Deprecated: Specifying rate limit and API config sources via cluster names, use the grpc_service(s) fields instead. Signed-off-by: Harvey Tuch <htuch@google.com>

Some new lines.

4b35549

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

htuch mentioned this pull request Jan 5, 2018

Add DirectResponseAction, a new route action type that returns a hard… #393

Merged

mattklein123 reviewed Jan 5, 2018

View reviewed changes

Review feedback.

7ebff6f

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

htuch added 2 commits January 7, 2018 07:29

wip

93e9278

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

Merge remote-tracking branch 'upstream/master' into grpc-api

e2c9726

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

mattklein123 reviewed Jan 8, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into grpc-api

838c11c

htuch mentioned this pull request Jan 8, 2018

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

Merged

Review feedback.

ae82e81

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

mattklein123 previously approved these changes Jan 8, 2018

View reviewed changes

ggreenway reviewed Jan 8, 2018

View reviewed changes

Review feedback.

531ed78

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

htuch dismissed mattklein123’s stale review via 531ed78 January 8, 2018 19:48

ggreenway approved these changes Jan 8, 2018

View reviewed changes

mattklein123 approved these changes Jan 8, 2018

View reviewed changes

htuch merged commit b796da4 into envoyproxy:master Jan 9, 2018

htuch deleted the grpc-api branch January 9, 2018 15:03

davecheney mentioned this pull request Jan 11, 2018

Issue/136 projectcontour/contour#139

Merged

htuch mentioned this pull request Jan 17, 2018

grpc: singleton manager/factory pattern for gRPC async clients. envoyproxy/envoy#2393

Merged

Conversation

htuch commented Jan 5, 2018

Uh oh!

htuch commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wora commented Jan 6, 2018 via email

Uh oh!

mattklein123 commented Jan 7, 2018

Uh oh!

htuch commented Jan 7, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jan 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htuch commented Jan 5, 2018 •

edited

Loading

htuch Jan 8, 2018 •

edited

Loading