Add a filter to support HTTP/JSON<->gRPC transcoding by lizan · Pull Request #1079 · envoyproxy/envoy

lizan · 2017-06-09T20:04:19Z

Closes #501

The class implements TranscoderInputStream from transcoding library, take input from Envoy::Buffer, this also allows Envoy::Buffer to be used as protobuf::io::ZeroCopyInputStream. For envoyproxy#501

htuch · 2017-06-12T21:19:50Z

source/common/grpc/transcoder_input_stream_impl.h

This is a strange include. I was looking for the interface and documentation, where can I find this?

The source files needed for descriptor generation, at https://github.com/lizan/envoy/blob/aa3aada1fa9e2557e34c1144d91bb2241da84ebf/test/proto/BUILD#L24 in envoyproxy/envoy#1079

…nput_stream

fengli79 · 2017-06-16T03:18:43Z

The name looks weird. Do you have plan to support protocols other than REST in this filter?
Prefer to name it as http1_rest_bridge_filter

…nput_stream

…ding

lizan · 2017-06-21T20:11:12Z

Let @mattklein123 or @htuch decide the name, candidates are:

grpc_transcoding
grpc_rest_bridge
grpc_json_bridge

I'm fine with either, or a new name.

…ding

mattklein123 · 2017-06-27T22:57:01Z

docs/configuration/http_filters/grpc_json_transcoder_filter.rst

+    "type": "both",
+    "name": "grpc_json_transcoder",
+    "config": {
+      "proto_descriptors": "proto.pb",


(Not a full review, will do full code review tomorrow)

Can we get a description of each of these config parameters? If I understand correctly, it's required that all descriptors get merged into a single pb file, right? What if there are multiple services from multiple protos? How do those get merged? Can you maybe provide an example?

For "services" what happens if the .pb file contains more services than you specify? Those aren't transcoded? Can you clarify?

Also, in general, I would love to talk about reflection possibilities. Obviously this is a follow up, but from Lyft perspective (and I think others), this filter is vastly less useful and more difficult to use if Envoy has to know ahead of time the .pb files and we need to keep them up to date. If we could do this with reflection I think that is the true absolutely killer feature. cc @twoism @rodaine @louiscryan

Can we get a description of each of these config parameters? If I understand correctly, it's required that all descriptors get merged into a single pb file, right? What if there are multiple services from multiple protos? How do those get merged? Can you maybe provide an example?

Will write it in docs, in short, protoc accepts multiple input files, so just do them together.

For "services" what happens if the .pb file contains more services than you specify? Those aren't transcoded? Can you clarify?

Right, those aren't transcoded.

Also, in general, I would love to talk about reflection possibilities. Obviously this is a follow up, but from Lyft perspective (and I think others), this filter is vastly less useful and more difficult to use if Envoy has to know ahead of time the .pb files and we need to keep them up to date. If we could do this with reflection I think that is the true absolutely killer feature.

I totally agree with you that would provide better usability, I'm just saying that won't included in this PR as this is big enough :) Feel free to open an issue and assign to me once this get merged.

mattklein123 · 2017-06-28T15:07:40Z

@lizan please ping me once the doc comment is fixed and this is passing tests and I will take another pass.

lizan · 2017-06-28T23:51:57Z

@mattklein123 Docs are updated and travis is almost passed. PTAL, thanks!

htuch

A few more comments, it's generally looking good. One meta comment is that this PR is now quite huge, as a reviewer I would appreciate it if we could try and break it up a little further in the future. Thanks.

htuch · 2017-06-29T02:16:33Z

docs/configuration/http_filters/grpc_json_transcoder_filter.rst

+
+proto_descriptors
+  *(required, string)* Supplies binary protobuf descriptor set for the gRPC services.
+  The descriptor set have to include all types that are used in the services. Make sure use


Nit: "Make sure to use the"

htuch · 2017-06-29T02:16:56Z

docs/configuration/http_filters/grpc_json_transcoder_filter.rst

+  The descriptor set have to include all types that are used in the services. Make sure use
+  ``--include_import`` option for ``protoc``.
+
+  To generate protobuf descriptor set for the gRPC service, You'll also need to clone the


Nit: s/You'll/you'll/

htuch · 2017-06-29T02:18:08Z

docs/configuration/http_filters/grpc_json_transcoder_filter.rst

+
+  .. code-block:: bash
+
+    protoc -I$(GOOGLEAPIS_DIR) -I. --include_imports --include_source_info \


I think we can make this more seamless via some bazel run command in the Envoy repository, that uses our googleapis proto imports. Not for this PR, but maybe raise an issue.

I don't think bazel run is easier in this case.
The Envoy customer (not developer) might not know how to build Envoy, and just using docker images, or binaries. Let them checkout envoy and install bazel sounds too much.
protoc is something that they are already using when they try Envoy with gRPC.

Agreed I don't think we should assume that the user of envoy has access to the envoy code or bazel.

htuch · 2017-06-29T02:23:05Z

source/common/grpc/BUILD

+        ":transcoder_input_stream_lib",
+        "//include/envoy/http:filter_interface",
+        "//source/common/common:base64_lib",
+        "//source/common/http:headers_lib",


We can't reference external deps like googleapis with "@googleapis", they need to appear in external_deps instead. We won't be able to perform the remapping during the Google import otherwise.

htuch · 2017-06-29T02:23:58Z

test/proto/BUILD

+    srcs = [":bookstore.proto"],
+    deps = [
+        "@googleapis//:http_api_protos",
+        "@protobuf_bzl//:cc_wkt_protos",


Same point as earlier here and everywhere, we can't use "@foo" deps, they need to be bound to something that gets referenced in external_deps.

Done, added a function in envoy_build_system.bzl and invoke it here.

I still see a reference to @protobuf_bzl here, maybe I'm looking at a stale patch..

htuch · 2017-06-29T02:25:01Z

source/common/grpc/json_transcoder_filter.cc

+
+namespace {
+
+const std::string TypeUrlPrefix{"type.googleapis.com"};


Nit: typeUrlPrefix.

htuch · 2017-06-29T02:26:17Z

source/common/grpc/json_transcoder_filter.cc

+  const std::string proto_descriptor_file = config.getString("proto_descriptor");
+  FileDescriptorSet descriptor_set;
+  if (!descriptor_set.ParseFromString(Filesystem::fileReadToEnd(proto_descriptor_file))) {
+    throw EnvoyException("transcoding_filter: Unable to parse proto descriptor");


I don't see any EXPECT_THROW or EXPECT_THROW_WITH_MESSAGE statements below. Are we testing and obtaining coverage of this error handling (and below)?

Added more tests.

htuch · 2017-06-29T02:31:58Z

source/common/grpc/json_transcoder_filter.cc

+
+  Status status = methodToRequestInfo(method_descriptor, &request_info);
+  if (!status.ok()) {
+    return status;


Can you confirm that you've looked at coverage for this file and obtained 100%? The reason I ask is that there are a lot of error handling cases, and a coverage check is the easiest way to validate this.

Is 100% required to merge the PR? I already added some test to cover config error cases. The coverage is currently 92.7%.

To obtain 100% coverage, it would need adding mocks for config and transcoder. I would like to do it in a follow up PR.

100% coverage is expected when reasonable. In this case this PR is big enough already. Let's do a follow up but please open an issue and link it here so that we can track.

…ding

mattklein123 · 2017-06-30T14:34:34Z

bazel/repositories.bzl

        name = "envoy_eds",
        actual = "@envoy_api//api:eds",
    )
+    native.bind(


This seems like the wrong if statement to have this under? I will defer to @htuch for review on all of the bazel changes.

Since googleapis is loaded from envoy-api, so I added here, let me know if this should go somewhere else. @htuch

Yeah, this is OK, but in theory you could source http_api_protos independently, let's keep it like this for now.

mattklein123 · 2017-06-30T14:43:14Z

source/common/grpc/json_transcoder_filter.cc

+                     method);
+        break;
+      default:
+        break;


Can this happen? Should there be some kind of warning here? If it can't happen can we delete default case?

The only possibility is ::google::api::HttpRule::PATTEN_NOT_SET (added a comment here), and we do nothing here.

mattklein123 · 2017-06-30T14:46:39Z

source/common/grpc/json_transcoder_filter.cc

+
+Http::FilterDataStatus JsonTranscoderFilter::decodeData(Buffer::Instance& data, bool end_stream) {
+  if (error_) {
+    return Http::FilterDataStatus::StopIterationNoBuffer;


I don't think this can happen if you send a local complete reply. I would ASSERT(!error_)

mattklein123 · 2017-06-30T14:48:10Z

source/common/grpc/json_transcoder_filter.cc

+
+Http::FilterHeadersStatus JsonTranscoderFilter::encodeHeaders(Http::HeaderMap& headers,
+                                                              bool end_stream) {
+  if (error_) {


error_ || !transcoder_ and delete dup return below.

mattklein123 · 2017-06-30T14:51:12Z

@lizan P.S. I did some light copy editing on the docs and pushed a commit to your branch.

htuch

LGTM modulo remaining comments.

htuch · 2017-06-30T21:50:00Z

bazel/repositories.bzl

        name = "envoy_eds",
        actual = "@envoy_api//api:eds",
    )
+    native.bind(


Yeah, this is OK, but in theory you could source http_api_protos independently, let's keep it like this for now.

htuch · 2017-06-30T21:55:43Z

source/common/grpc/json_transcoder_filter.cc

+      config_.createTranscoder(headers, request_in_, response_in_, transcoder_, method_);
+
+  if (!status.ok()) {
+    return Http::FilterHeadersStatus::Continue;


Why continue on failure here? Please add a comment if this is intentional.

Add a comment, we shouldn't fail the request because the transcoder creation failed. The request might be a normal gRPC request that it should be passed to upstream.

htuch · 2017-06-30T21:57:04Z

source/common/grpc/json_transcoder_filter.cc

+    Buffer::OwnedImpl data;
+    readToBuffer(*transcoder_->RequestOutput(), data);
+
+    if (data.length()) {


Nit: if (data.length() > 0).

htuch · 2017-06-30T22:07:48Z

source/common/grpc/json_transcoder_filter.cc

+  const Http::HeaderEntry* grpc_status_header = trailers.GrpcStatus();
+  if (grpc_status_header) {
+    uint64_t grpc_status_code;
+    if (!StringUtil::atoul(grpc_status_header->value().c_str(), grpc_status_code)) {


You might want to use Common::getGrpcStatus here as it has additional error checking.

Hmm, since we're just forward grpc-status to client, so I don't need the additional error checking here. There is a plan to have HTTP code mapping, will do that in follow up.

htuch · 2017-06-30T22:11:49Z

source/common/grpc/json_transcoder_filter.h

+  std::unique_ptr<google::grpc::transcoding::TypeHelper> type_helper_;
+};
+
+typedef std::shared_ptr<JsonTranscoderConfig> TranscodingConfigSharedPtr;


Is there a reason this is shared ptr?

It has to be shared_ptr to be captured into lambda here:
https://github.com/lizan/envoy/blob/grpc_json_transcoding/source/server/config/http/grpc_json_transcoder.cc#L17

htuch · 2017-06-30T22:13:36Z

test/proto/BUILD

+    srcs = [":bookstore.proto"],
+    deps = [
+        "@googleapis//:http_api_protos",
+        "@protobuf_bzl//:cc_wkt_protos",


I still see a reference to @protobuf_bzl here, maybe I'm looking at a stale patch..

htuch · 2017-06-30T22:14:23Z

test/proto/bookstore.proto

@@ -0,0 +1,159 @@
+syntax = "proto3";
+
+package bookstore;


This seems a rather complicated proto for doing tests, can't we express everything we need to test in a much smaller proto?

This is already a simplified version compared to what we have in Cloud Endpoints, but feel free to add an item to #1192 to see what can be done here.

quintonparker · 2017-07-01T07:20:14Z

Random software engineer thanks you all. Looking forward to using this filter!

+1 grpc_json_bridge

Description: The was only needed prior to dynamic registration of platform filters. It's benign, but thoroughly unnecessary now. Risk Level: Low Testing: CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

The source files needed for descriptor generation, at https://github.com/lizan/envoy/blob/aa3aada1fa9e2557e34c1144d91bb2241da84ebf/test/proto/BUILD#L24 in envoyproxy/envoy#1079

Add TranscoderInputStreamImpl

6572487

The class implements TranscoderInputStream from transcoding library, take input from Envoy::Buffer, this also allows Envoy::Buffer to be used as protobuf::io::ZeroCopyInputStream. For envoyproxy#501

This was referenced Jun 9, 2017

Add TranscoderInputStreamImpl #1078

Merged

Export protobuf sources as filegroup envoyproxy/data-plane-api#77

Merged

htuch reviewed Jun 12, 2017

View reviewed changes

lizan added 8 commits June 14, 2017 14:51

Merge remote-tracking branch 'upstream/master' into grpc_transcoder_i…

a8f8f33

…nput_stream

Refactor ZeroCopyInputStreamImpl out

579d342

Merge remote-tracking branch 'upstream/master' into grpc_transcoder_i…

118d1e7

…nput_stream

fix

ca46f94

Use ZeroCopyInputStreamImpl in AsyncClientImpl and RpcChannelImpl

8034159

fix

77f9c00

fix format

dd68af1

fix finish test

8d235bd

lizan added 14 commits June 19, 2017 16:00

Address review comments

98c5ccf

Merge remote-tracking branch 'upstream/master' into grpc_transcoder_i…

547b92f

…nput_stream

fix format

8f1f307

fix test include

b88b0d5

fix format

9850c9b

fix grammer in comments

53fcf47

fix format

6be5d0d

Add transcoding docs

ae9094f

Add transcoding filter from istio/proxy

13b66db

Test WIP

5eabe6f

align with master

5446914

Add unittest

db48843

Add integration test

f92d527

Merge remote-tracking branch 'upstream/master' into grpc_json_transco…

b68400e

…ding

lizan force-pushed the grpc_json_transcoding branch from aa3aada to b68400e Compare June 21, 2017 20:11

lizan added 4 commits June 26, 2017 18:47

fix code comment, rename to grpc_json_transcoder

79791ab

docs

af5ac95

fix format

b2265b0

Merge remote-tracking branch 'upstream/master' into grpc_json_transco…

ce354b4

…ding

mattklein123 reviewed Jun 27, 2017

View reviewed changes

mattklein123 mentioned this pull request Jun 28, 2017

gRPC/JSON transcoding using reflection #1182

Open

Update docs

8b154c7

htuch reviewed Jun 29, 2017

View reviewed changes

lizan and others added 6 commits June 29, 2017 13:15

Merge remote-tracking branch 'upstream/master' into grpc_json_transco…

6894025

…ding

increase test coverage

efba771

move descriptor generation into bzl rule

80de522

fix format

2a35000

fix format

d41f9d9

Update grpc_json_transcoder_filter.rst

38d08fa

mattklein123 reviewed Jun 30, 2017

View reviewed changes

assert on error_

c82fd4a

htuch reviewed Jun 30, 2017

View reviewed changes

fix BUILD, add more comments

cdeb87d

mattklein123 approved these changes Jul 1, 2017

View reviewed changes

mattklein123 merged commit e2de6aa into envoyproxy:master Jul 1, 2017

mathetake pushed a commit that referenced this pull request Mar 3, 2026

build(deps): bump helm.sh/helm/v3 from 3.18.4 to 3.18.5 (#1079)

70655f6


		.. code-block:: bash

		protoc -I$(GOOGLEAPIS_DIR) -I. --include_imports --include_source_info \


		namespace {

		const std::string TypeUrlPrefix{"type.googleapis.com"};

Conversation

lizan commented Jun 9, 2017 • 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!

fengli79 commented Jun 16, 2017

Uh oh!

lizan commented Jun 21, 2017

Uh oh!

mattklein123 Jun 27, 2017 • 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!

mattklein123 commented Jun 28, 2017

Uh oh!

lizan commented Jun 28, 2017

Uh oh!

htuch 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!

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!

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!

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!

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!

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!

mattklein123 commented Jun 30, 2017

lizan commented Jun 9, 2017 •

edited

Loading

mattklein123 Jun 27, 2017 •

edited

Loading