grpc-json: handle google.api.HttpBody when building HTTP response #3793

dio · 2018-07-05T06:47:16Z

Description:
This patch prepares HTTP response when an upstream gRPC service method
uses google.api.HttpBody proto as its message output type.

Risk Level: Low

Testing: Unit

Docs Changes:

Added notes on sending arbitrary content as HTTP response by using google.api.HttpBody.

Release Notes:

Added support for encoding google.api.HttpBody when building HTTP response.

Fixes #3205

Signed-off-by: Dhi Aurrahman dio@rockybars.com

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

lizan

Overall looks great, thanks for doing this @dio!

lizan · 2018-07-09T06:22:05Z

api/bazel/api_build_system.bzl

@@ -125,6 +127,11 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de
        deps = [_LibrarySuffix(d, _CC_SUFFIX) for d in deps],
        external_deps = [
            "@com_google_protobuf//:cc_wkt_protos",
+            # TODO(dio): Running `bazel build @envoy_api//envoy/...` on mac


Is this similar issue to bazelbuild/bazel#5163 ?

Though I don't think we should add this here to every api_proto_library.

OK, I will remove the comment.

lizan · 2018-07-09T06:25:15Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+  if (output_type.length() != http_body_type.length()) {
+    return false;
+  }
+  return StringUtil::startsWith(output_type.data(), http_body_type.data(), true);


why startsWith but not equal here? You should use absl::StartsWith for string_views.

I have the length comparison previously. And since the comparison needs to be case-sensitive?

Will use the equal operator then.

lizan · 2018-07-09T06:26:07Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -349,6 +352,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,
  readToBuffer(*transcoder_->ResponseOutput(), data);

  if (!method_->server_streaming() && !end_stream) {
+    if (hasHttpBodyAsOutputType()) {


Can we check this earlier (preferred in decodeData right after creating transcoder_) and store in a member variable?

lizan · 2018-07-09T06:29:15Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -415,6 +422,32 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea
  return false;
 }

+void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers,
+                                                           Buffer::Instance& data) {
+  try {


instead of parsing JSON here, it would be better to parse protobuf directly using Grpc::Frame and Grpc::Decoder. It is cleaner and more efficient, hardcoded "contentType" won't work when preserve_field_names is set to true.

Cool, this is new to me. Thanks for pointing it out.

lizan · 2018-07-09T06:33:36Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -349,6 +352,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,
  readToBuffer(*transcoder_->ResponseOutput(), data);

  if (!method_->server_streaming() && !end_stream) {


support or add a TODO for streaming case?

lizan · 2018-07-09T06:34:51Z

docs/root/intro/version_history.rst

@@ -7,6 +7,7 @@ Version history
  to filter based on the presence of Envoy response flags.
 * admin: added :http:get:`/hystrix_event_stream` as an endpoint for monitoring envoy's statistics
  through `Hystrix dashboard <https://github.com/Netflix-Skunkworks/hystrix-dashboard/wiki>`_.
+* grpc-json: added support for encoding `google.api.HttpBody <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto>`_.


Let's clarify this is for response only, and update PR title.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-07-09T14:52:46Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+      data.add(body);
+      response_headers.insertContentType().value(http_body.content_type());
+      response_headers.insertContentLength().value(body.length());
+      return true;


I'm not really sure about this one. Should we handle more than one frame?

Since you left a TODO to support streaming, so it is ok only process one frame here.

lizan · 2018-07-10T07:13:10Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+      data.add(body);
+      response_headers.insertContentType().value(http_body.content_type());
+      response_headers.insertContentLength().value(body.length());
+      return true;


Since you left a TODO to support streaming, so it is ok only process one frame here.

lizan · 2018-07-10T07:15:01Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+      return true;
+    }
+  }
+  return false;


In the case of one frame with length 0, what is the point to let this return false and let the default handler transcode to {}? It is valid that a response HttpBody have empty content type and data.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

lizan

two nits, thank you!

lizan · 2018-07-10T22:45:00Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -222,6 +223,7 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h
    // just pass-through the request to upstream.
    return Http::FilterHeadersStatus::Continue;
  }
+  has_http_body_output_ = hasHttpBodyAsOutputType();


Since the behavior below depends on non-streaming handling, (i.e. buffer response until trailer), lets only set this when server_streaming is false.

Hi @lizan sorry, I need to clarify this. Do you want to make sure that we only set has_http_body_output_ to true when server_streaming is false and hasHttpBodyAsOutputType() is true?

yes, hasHttpBodyAsOutputType() && !server_streaming

Ah OK, done. Thanks.

lizan · 2018-07-10T22:45:25Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -352,6 +359,7 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,
    // Buffer until the response is complete.
    return Http::FilterDataStatus::StopIterationAndBuffer;
  }
+  // TODO(dio): Add support for streaming case.


move this todo to L226 or L345.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-07-11T03:11:17Z

Thanks, @lizan. I think I have addressed your comments. PTAL when you have time. Thanks again!

lizan · 2018-07-11T04:19:51Z

@dio can you fix the tests?

dio · 2018-07-11T04:25:54Z

Sure. Sorry was not aware of it.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch

Looks good, question about the encodeData assumptions.

htuch · 2018-07-12T14:37:44Z

api/bazel/repositories.bzl

@@ -21,14 +21,62 @@ def api_dependencies():
 load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")

+filegroup(


If we keep doing this for every Google API proto we bring in, it will be quite verbose. I think it's outside the scope of this PR, but we should consider writing a Skylark macro to shrink the boilerplate.

I added a TODO for this. I hope that's OK for now.

htuch · 2018-07-12T14:38:39Z

docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst

+`google.api.HttpBody <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto>`_
+as its output message type. The implementation needs to set
+`content_type <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto#L68>`_
+and `data <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto#L71>`_ accordingly.


Maybe point out that content-type is derived from the proto in this case.

htuch · 2018-07-12T14:43:34Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

@@ -340,6 +342,12 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,
    return Http::FilterDataStatus::Continue;
  }

+  // TODO(dio): Add support for streaming case.
+  if (has_http_body_output_) {
+    buildResponseFromHttpBodyOutput(*response_headers_, data);


Are we assuming here that the entire HTTP response proto is available in a single encodeData invocation? I don't think that's true, even for the non-streaming case.

Thanks, @htuch. It makes sense, it seems it needs to buffer the data. Will try to explore it.

I think if we don't have enough data to decode for a single frame, we buffer it in here: decoder_.decode(data, frames);.

Yeah, fair enough. How do we actually continue processing when we're done though? Looks like we're just always returning Http::FilterDataStatus::StopIterationAndBuffer?

@htuch I think @dio already does this correctly, decoder_.decode(data, frames); buffers the data, and it returns Http::FilterDataStatus::StopIterationAndBuffer to stop the iteration, because the filter need content-type from grpc-frame, and potentially grpc-status in trailer. The transcoder will stop filter iteration until it can send full response (i.e. until encodeTrailer) for unary cases.

@dio can you add some test case to cover this? (one grpc frame split in two encodeData call)

@lizan got it, makes sense, thanks.

@lizan added d93f57e.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch · 2018-07-12T19:01:07Z

test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc

+
+  Http::TestHeaderMapImpl continue_headers{{":status", "000"}};
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue,
+            filter_.encode100ContinueHeaders(continue_headers));


Do we need the 100 continue stuff here?

Removed in d93f57e.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch

Great, thanks.

htuch · 2018-07-12T21:30:25Z

@dio can you merge master to fix version_history.rst conflict?

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-07-13T00:50:45Z

@htuch merged with master.

lizan · 2018-07-13T05:39:58Z

test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc

+  auto response_data = Grpc::Common::serializeBody(response);
+  EXPECT_EQ(40, response_data->length());
+
+  // The response data buffer is splitted into two parts.


Buffer::OwnedImpl part1; part1.move(response_data, 20);

will do what L615-L625 does.

Thank you for this. Updated in 40d9b3f.

lizan · 2018-07-13T05:41:47Z

test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc

+  part2.add(std::string(out.begin(), out.end()));
+  EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(part2, false));
+
+  const std::string response_html = part2.toString();


It is not intuitive why part2 will be response_html, I would just set response_html directly from the string "<h1>Hello, world!</h1>" and use it with set_data and expectation below.

My proposed change is in 40d9b3f.

I would just set response_html directly from the string "<h1>Hello, world!</h1>" and use it with set_data and expectation below.

I'm not sure I get this. Would you mind to elaborate? Thanks!

Hopefully, the change in 9403723 is better. 🤞

Ah yep that is better 👍 LGTM

@lizan

Thanks @lizan! Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio added 5 commits July 5, 2018 13:39

grpc-json: handle google.api.HttpBody

ce46d96

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Wrap in exception

aa8e4cf

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Log method name instead

ac4455b

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add api_httpbody_protos

fcd37be

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix on mac api build

da90483

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

mattklein123 assigned lizan Jul 5, 2018

dio added 2 commits July 6, 2018 08:13

Merge remote-tracking branch 'upstream/master'

59cf968

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/master'

6dcd9a6

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

lizan reviewed Jul 9, 2018

View reviewed changes

dio changed the title ~~grpc-json: handle google.api.HttpBody~~ grpc-json: handle google.api.HttpBody when building HTTP response Jul 9, 2018

dio added 4 commits July 9, 2018 14:01

review: remove comment related to bazel

5aa346a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

review: use equal operator instead

9f4739b

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

review: clarify on supporting response only

95778e8

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove base64

e76880e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio commented Jul 9, 2018

View reviewed changes

lizan reviewed Jul 10, 2018

View reviewed changes

dio added 2 commits July 10, 2018 14:30

Merge remote-tracking branch 'upstream/master'

eff3c80

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Process only one frame for now

3aa68fb

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

lizan reviewed Jul 10, 2018

View reviewed changes

dio added 2 commits July 11, 2018 09:57

review: move comment, set when !server_streaming

4123018

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove duplicated comments

085e967

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio added 2 commits July 11, 2018 13:45

Only handle HttpBody when !server_streaming

d5c4ba9

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/master'

784d789

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

lizan previously approved these changes Jul 11, 2018

View reviewed changes

lizan requested a review from htuch July 11, 2018 22:25

htuch suggested changes Jul 12, 2018

View reviewed changes

review: add todo skylark macro

c2dc9e8

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

review: clarify the use of HttpBody fields

e9d9b1e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio dismissed lizan’s stale review via e9d9b1e July 12, 2018 16:55

dio added 2 commits July 13, 2018 00:27

Add UnaryGetHttpBody integration test

c0d7c87

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix format

b45854a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch reviewed Jul 12, 2018

View reviewed changes

Add test for splitting buffer into two encode data

d93f57e

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch previously approved these changes Jul 12, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master'

b9a5e43

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio dismissed htuch’s stale review via b9a5e43 July 13, 2018 00:46

lizan reviewed Jul 13, 2018

View reviewed changes

dio added 2 commits July 13, 2018 17:13

review: simplify splitting the buffer

40d9b3f

Thanks @lizan! Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Reuse response object as expected values

9403723

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch approved these changes Jul 13, 2018

View reviewed changes

htuch merged commit 0cf947f into envoyproxy:master Jul 13, 2018

RichardSieg mentioned this pull request May 20, 2019

Support google.api.HttpBody for requests in gRPC-JSON transcoder #7007

Closed

		@@ -349,6 +352,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,
		readToBuffer(*transcoder_->ResponseOutput(), data);

		if (!method_->server_streaming() && !end_stream) {

grpc-json: handle google.api.HttpBody when building HTTP response #3793

grpc-json: handle google.api.HttpBody when building HTTP response #3793

Conversation

dio commented Jul 5, 2018 • edited Loading

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Jul 11, 2018

lizan commented Jul 11, 2018

dio commented Jul 11, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch commented Jul 12, 2018

dio commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Jul 5, 2018 •

edited

Loading

dio Jul 12, 2018 •

edited

Loading

dio Jul 12, 2018 •

edited

Loading