From ce46d96a8c87e00d5d0ccf445e36508a32b74ade Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 5 Jul 2018 13:39:28 +0700 Subject: [PATCH 01/20] grpc-json: handle google.api.HttpBody Signed-off-by: Dhi Aurrahman --- api/bazel/repositories.bzl | 3 ++ .../grpc_json_transcoder_filter.rst | 11 +++++ docs/root/intro/version_history.rst | 7 ++-- .../filters/http/grpc_json_transcoder/BUILD | 1 + .../json_transcoder_filter.cc | 29 +++++++++++++ .../json_transcoder_filter.h | 2 + .../json_transcoder_filter_test.cc | 41 +++++++++++++++++++ test/proto/bookstore.proto | 6 +++ 8 files changed, 97 insertions(+), 3 deletions(-) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index b1550bf7851a..af2b42586bdb 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -26,6 +26,7 @@ filegroup( srcs = [ "google/api/annotations.proto", "google/api/http.proto", + "google/api/httpbody.proto", ], visibility = ["//visibility:public"], ) @@ -49,6 +50,7 @@ cc_proto_library( srcs = [ "google/api/annotations.proto", "google/api/http.proto", + "google/api/httpbody.proto", ], default_runtime = "@com_google_protobuf//:protobuf", protoc = "@com_google_protobuf//:protoc", @@ -61,6 +63,7 @@ py_proto_library( srcs = [ "google/api/annotations.proto", "google/api/http.proto", + "google/api/httpbody.proto", ], include = ".", default_runtime = "@com_google_protobuf//:protobuf_python", diff --git a/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst b/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst index 0c77708dbac5..b6cb4f3a4b7c 100644 --- a/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst +++ b/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst @@ -62,3 +62,14 @@ match the incoming request path, set `match_incoming_request_route` to true. }; } } + +Sending arbitrary content +------------------------- + +By default, when transcoding occurs, gRPC-JSON encodes the message output of a gRPC service method into +JSON and sets the response `Content-Type` header to `application/json`. To send abritrary content, a gRPC +service method can use +`google.api.HttpBody `_ +as its output message type. The implementation needs to set +`content_type `_ +and `data `_ accordingly. \ No newline at end of file diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index dbdb78cb2bdb..dfdac57064a1 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -3,10 +3,11 @@ Version history 1.8.0 (Pending) =============== -* access log: added :ref:`response flag filter ` - 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 +* access log: added :ref:`response flag filter ` + 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 `_. +* grpc-json: added support for encoding `google.api.HttpBody `_. * http: response filters not applied to early error paths such as http_parser generated 400s. * lua: added :ref:`requestInfo() ` wrapper and *protocol()* API. * ratelimit: added support for :repo:`api/envoy/service/ratelimit/v2/rls.proto`. diff --git a/source/extensions/filters/http/grpc_json_transcoder/BUILD b/source/extensions/filters/http/grpc_json_transcoder/BUILD index 5bf972b6ac8e..b7237d82ab90 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/BUILD +++ b/source/extensions/filters/http/grpc_json_transcoder/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( "//source/common/grpc:codec_lib", "//source/common/grpc:common_lib", "//source/common/http:headers_lib", + "//source/common/json:json_loader_lib", "//source/common/protobuf", "@envoy_api//envoy/config/filter/http/transcoder/v2:transcoder_cc", ], diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 4328fb2ab760..298edeb965b6 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -4,16 +4,19 @@ #include "envoy/http/filter.h" #include "common/common/assert.h" +#include "common/common/base64.h" #include "common/common/enum_to_int.h" #include "common/common/utility.h" #include "common/filesystem/filesystem_impl.h" #include "common/grpc/common.h" #include "common/http/headers.h" #include "common/http/utility.h" +#include "common/json/json_loader.h" #include "common/protobuf/protobuf.h" #include "google/api/annotations.pb.h" #include "google/api/http.pb.h" +#include "google/api/httpbody.pb.h" #include "grpc_transcoding/json_request_translator.h" #include "grpc_transcoding/path_matcher_utility.h" #include "grpc_transcoding/response_to_json_translator.h" @@ -349,6 +352,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, readToBuffer(*transcoder_->ResponseOutput(), data); if (!method_->server_streaming() && !end_stream) { + if (hasHttpBodyAsOutputType()) { + // Set content-type and data from HttpBody. + buildResponseFromHttpBodyOutput(*response_headers_, data); + } // Buffer until the response is complete. return Http::FilterDataStatus::StopIterationAndBuffer; } @@ -415,6 +422,28 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea return false; } +void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, + Buffer::Instance& data) { + // TODO(dio): Wrap JSON parsing in a try-catch block. + const Json::ObjectSharedPtr http_body = Json::Factory::loadFromString(data.toString()); + const std::string decoded_body = Base64::decode(http_body->getString("data")); + + data.drain(data.length()); + data.add(decoded_body); + + response_headers.insertContentType().value(http_body->getString("contentType")); + response_headers.insertContentLength().value(decoded_body.size()); +} + +bool JsonTranscoderFilter::hasHttpBodyAsOutputType() { + absl::string_view output_type = method_->output_type()->full_name(); + absl::string_view http_body_type = google::api::HttpBody::descriptor()->full_name(); + if (output_type.length() != http_body_type.length()) { + return false; + } + return StringUtil::startsWith(output_type.data(), http_body_type.data(), true); +} + } // namespace GrpcJsonTranscoder } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h index e8544ea2600f..922efaf49034 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h @@ -117,6 +117,8 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< private: bool readToBuffer(Protobuf::io::ZeroCopyInputStream& stream, Buffer::Instance& data); + void buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data); + bool hasHttpBodyAsOutputType(); JsonTranscoderConfig& config_; std::unique_ptr transcoder_; diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index e3c96bb95dd9..61cfb9e4de51 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -550,6 +550,47 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryNotGrpcResponse) { EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(request_data, true)); } +TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutput) { + Http::TestHeaderMapImpl request_headers{{":method", "GET"}, {":path", "/index"}}; + + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + EXPECT_EQ("application/grpc", request_headers.get_("content-type")); + EXPECT_EQ("/index", request_headers.get_("x-envoy-original-path")); + EXPECT_EQ("/bookstore.Bookstore/GetIndex", request_headers.get_(":path")); + EXPECT_EQ("trailers", request_headers.get_("te")); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, + {":status", "200"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + + google::api::HttpBody response; + response.set_content_type("text/html"); + response.set_data("

Hello, world!

"); + + auto response_data = Grpc::Common::serializeBody(response); + + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, + filter_.encodeData(*response_data, false)); + + std::string response_html = response_data->toString(); + + EXPECT_EQ("text/html", response_headers.get_("content-type")); + EXPECT_EQ("

Hello, world!

", response_html); + + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "0"}, {"grpc-message", ""}}; + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); +} + struct GrpcJsonTranscoderFilterPrintTestParam { std::string config_json_; std::string expected_response_; diff --git a/test/proto/bookstore.proto b/test/proto/bookstore.proto index 45ca15bf54c6..6fb65b42d119 100644 --- a/test/proto/bookstore.proto +++ b/test/proto/bookstore.proto @@ -3,6 +3,7 @@ syntax = "proto3"; package bookstore; import "google/api/annotations.proto"; +import "google/api/httpbody.proto"; import "google/protobuf/empty.proto"; // A simple Bookstore API. @@ -84,6 +85,11 @@ service Bookstore { get: "/authors/{author}" }; } + rpc GetIndex(google.protobuf.Empty) returns (google.api.HttpBody) { + option (google.api.http) = { + get: "/index" + }; + } } // A shelf resource. From aa8e4cf2a0ae186931daf1557b9d86d6ba97f1b6 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 5 Jul 2018 16:01:52 +0700 Subject: [PATCH 02/20] Wrap in exception Signed-off-by: Dhi Aurrahman --- .../json_transcoder_filter.cc | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 298edeb965b6..5fce8c489094 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -424,15 +424,19 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data) { - // TODO(dio): Wrap JSON parsing in a try-catch block. - const Json::ObjectSharedPtr http_body = Json::Factory::loadFromString(data.toString()); - const std::string decoded_body = Base64::decode(http_body->getString("data")); + try { + const Json::ObjectSharedPtr http_body = Json::Factory::loadFromString(data.toString()); + const std::string decoded_body = Base64::decode(http_body->getString("data")); - data.drain(data.length()); - data.add(decoded_body); + data.drain(data.length()); + data.add(decoded_body); - response_headers.insertContentType().value(http_body->getString("contentType")); - response_headers.insertContentLength().value(decoded_body.size()); + response_headers.insertContentType().value(http_body->getString("contentType")); + response_headers.insertContentLength().value(decoded_body.size()); + } catch (const Json::Exception& e) { + ENVOY_LOG(debug, "Failed to parse output of '{}'. e.what(): {}", + method_->input_type()->full_name(), e.what()); + } } bool JsonTranscoderFilter::hasHttpBodyAsOutputType() { From ac4455bd1423b4d823ae834d289f07855024633a Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 5 Jul 2018 16:05:15 +0700 Subject: [PATCH 03/20] Log method name instead Signed-off-by: Dhi Aurrahman --- .../http/grpc_json_transcoder/json_transcoder_filter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 5fce8c489094..daf8e0bbf794 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -434,8 +434,8 @@ void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& resp response_headers.insertContentType().value(http_body->getString("contentType")); response_headers.insertContentLength().value(decoded_body.size()); } catch (const Json::Exception& e) { - ENVOY_LOG(debug, "Failed to parse output of '{}'. e.what(): {}", - method_->input_type()->full_name(), e.what()); + ENVOY_LOG(debug, "Failed to parse output of '{}'. e.what(): {}", method_->full_name(), + e.what()); } } From fcd37be3e67641eacac9bde9fa987ae66f661563 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 5 Jul 2018 18:31:50 +0700 Subject: [PATCH 04/20] Add api_httpbody_protos Signed-off-by: Dhi Aurrahman --- api/bazel/api_build_system.bzl | 3 ++ api/bazel/repositories.bzl | 54 +++++++++++++++++-- bazel/envoy_build_system.bzl | 8 +++ bazel/repositories.bzl | 4 ++ .../filters/http/grpc_json_transcoder/BUILD | 1 + test/proto/BUILD | 4 +- 6 files changed, 69 insertions(+), 5 deletions(-) diff --git a/api/bazel/api_build_system.bzl b/api/bazel/api_build_system.bzl index 8099e9f025aa..591e40c9b58e 100644 --- a/api/bazel/api_build_system.bzl +++ b/api/bazel/api_build_system.bzl @@ -32,6 +32,7 @@ def api_py_proto_library(name, srcs = [], deps = [], has_services = 0): protoc = "@com_google_protobuf//:protoc", deps = [_LibrarySuffix(d, _PY_SUFFIX) for d in deps] + [ "@com_lyft_protoc_gen_validate//validate:validate_py", + "@googleapis//:api_httpbody_protos_py", "@googleapis//:http_api_protos_py", "@googleapis//:rpc_status_protos_py", "@com_github_gogo_protobuf//:gogo_proto_py", @@ -108,6 +109,7 @@ def api_proto_library(name, visibility = ["//visibility:private"], srcs = [], de "@com_google_protobuf//:struct_proto", "@com_google_protobuf//:timestamp_proto", "@com_google_protobuf//:wrappers_proto", + "@googleapis//:api_httpbody_protos_proto", "@googleapis//:http_api_protos_proto", "@googleapis//:rpc_status_protos_lib", "@com_github_gogo_protobuf//:gogo_proto", @@ -125,6 +127,7 @@ 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", + "@googleapis//:api_httpbody_protos", "@googleapis//:http_api_protos", "@googleapis//:rpc_status_protos", "@com_github_gogo_protobuf//:gogo_proto_cc", diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index af2b42586bdb..da383bf5a34d 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -21,15 +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( + name = "api_httpbody_protos_src", + srcs = [ + "google/api/httpbody.proto", + ], + visibility = ["//visibility:public"], +) + +proto_library( + name = "api_httpbody_protos_proto", + srcs = [":api_httpbody_protos_src"], + deps = ["@com_google_protobuf//:descriptor_proto"], + visibility = ["//visibility:public"], +) + +cc_proto_library( + name = "api_httpbody_protos", + srcs = [ + "google/api/httpbody.proto", + ], + default_runtime = "@com_google_protobuf//:protobuf", + protoc = "@com_google_protobuf//:protoc", + deps = ["@com_google_protobuf//:cc_wkt_protos"], + visibility = ["//visibility:public"], +) + +py_proto_library( + name = "api_httpbody_protos_py", + srcs = [ + "google/api/httpbody.proto", + ], + include = ".", + default_runtime = "@com_google_protobuf//:protobuf_python", + protoc = "@com_google_protobuf//:protoc", + visibility = ["//visibility:public"], + deps = ["@com_google_protobuf//:protobuf_python"], +) + +go_proto_library( + name = "api_httpbody_go_proto", + importpath = "google.golang.org/genproto/googleapis/api/httpbody", + proto = ":api_httpbody_protos_proto", + visibility = ["//visibility:public"], + deps = [ + ":descriptor_go_proto", + ], +) + filegroup( name = "http_api_protos_src", srcs = [ "google/api/annotations.proto", "google/api/http.proto", - "google/api/httpbody.proto", ], visibility = ["//visibility:public"], - ) +) go_proto_library( name = "descriptor_go_proto", @@ -50,7 +97,6 @@ cc_proto_library( srcs = [ "google/api/annotations.proto", "google/api/http.proto", - "google/api/httpbody.proto", ], default_runtime = "@com_google_protobuf//:protobuf", protoc = "@com_google_protobuf//:protoc", @@ -63,7 +109,6 @@ py_proto_library( srcs = [ "google/api/annotations.proto", "google/api/http.proto", - "google/api/httpbody.proto", ], include = ".", default_runtime = "@com_google_protobuf//:protobuf_python", @@ -96,6 +141,7 @@ proto_library( deps = ["@com_google_protobuf//:any_proto"], visibility = ["//visibility:public"], ) + cc_proto_library( name = "rpc_status_protos", srcs = ["google/rpc/status.proto"], diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl index bf7885cc7d6c..d79663d83739 100644 --- a/bazel/envoy_build_system.bzl +++ b/bazel/envoy_build_system.bzl @@ -366,6 +366,10 @@ def envoy_proto_library(name, srcs = [], deps = [], external_deps = [], cc_proto_deps = [] py_proto_deps = ["@com_google_protobuf//:protobuf_python"] + if "api_httpbody_protos" in external_deps: + cc_proto_deps.append("@googleapis//:api_httpbody_protos") + py_proto_deps.append("@googleapis//:api_httpbody_protos_py") + if "http_api_protos" in external_deps: cc_proto_deps.append("@googleapis//:http_api_protos") py_proto_deps.append("@googleapis//:http_api_protos_py") @@ -403,6 +407,10 @@ def envoy_proto_descriptor(name, out, srcs = [], external_deps = []): input_files = ["$(location " + src + ")" for src in srcs] include_paths = [".", PACKAGE_NAME] + if "api_httpbody_protos" in external_deps: + srcs.append("@googleapis//:api_httpbody_protos_src") + include_paths.append("external/googleapis") + if "http_api_protos" in external_deps: srcs.append("@googleapis//:http_api_protos_src") include_paths.append("external/googleapis") diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 6440a3a55c16..09daa9032afd 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -182,6 +182,10 @@ def _envoy_api_deps(): if "envoy_api" not in native.existing_rules().keys(): _default_envoy_api(name="envoy_api") + native.bind( + name = "api_httpbody_protos", + actual = "@googleapis//:api_httpbody_protos", + ) native.bind( name = "http_api_protos", actual = "@googleapis//:http_api_protos", diff --git a/source/extensions/filters/http/grpc_json_transcoder/BUILD b/source/extensions/filters/http/grpc_json_transcoder/BUILD index b7237d82ab90..885b5fb2d651 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/BUILD +++ b/source/extensions/filters/http/grpc_json_transcoder/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "path_matcher", "grpc_transcoding", "http_api_protos", + "api_httpbody_protos", ], deps = [ ":transcoder_input_stream_lib", diff --git a/test/proto/BUILD b/test/proto/BUILD index 3b21fb2d2ea2..fa1674251719 100644 --- a/test/proto/BUILD +++ b/test/proto/BUILD @@ -20,8 +20,9 @@ envoy_proto_library( name = "bookstore_proto", srcs = [":bookstore.proto"], external_deps = [ - "well_known_protos", + "api_httpbody_protos", "http_api_protos", + "well_known_protos", ], ) @@ -32,6 +33,7 @@ envoy_proto_descriptor( ], out = "bookstore.descriptor", external_deps = [ + "api_httpbody_protos", "http_api_protos", "well_known_protos", ], From da90483bf543072f88f747f55691289d815e6c6f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 5 Jul 2018 19:55:03 +0700 Subject: [PATCH 05/20] Fix on mac api build Signed-off-by: Dhi Aurrahman --- api/bazel/api_build_system.bzl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/bazel/api_build_system.bzl b/api/bazel/api_build_system.bzl index 591e40c9b58e..749d03be0afb 100644 --- a/api/bazel/api_build_system.bzl +++ b/api/bazel/api_build_system.bzl @@ -127,7 +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", - "@googleapis//:api_httpbody_protos", + # TODO(dio): Running `bazel build @envoy_api//envoy/...` on mac + # with the following line uncommented gives: + # 'error=7, Argument list too long' + # + # "@googleapis//:api_httpbody_protos", "@googleapis//:http_api_protos", "@googleapis//:rpc_status_protos", "@com_github_gogo_protobuf//:gogo_proto_cc", From 5aa346a4452c1d8baf2e94601eeb0b875dc1a925 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 9 Jul 2018 14:01:10 +0700 Subject: [PATCH 06/20] review: remove comment related to bazel Signed-off-by: Dhi Aurrahman --- api/bazel/api_build_system.bzl | 5 ----- 1 file changed, 5 deletions(-) diff --git a/api/bazel/api_build_system.bzl b/api/bazel/api_build_system.bzl index 749d03be0afb..dfda7bfea629 100644 --- a/api/bazel/api_build_system.bzl +++ b/api/bazel/api_build_system.bzl @@ -127,11 +127,6 @@ 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 - # with the following line uncommented gives: - # 'error=7, Argument list too long' - # - # "@googleapis//:api_httpbody_protos", "@googleapis//:http_api_protos", "@googleapis//:rpc_status_protos", "@com_github_gogo_protobuf//:gogo_proto_cc", From 9f4739bbe9304d46044d5474e43f1f929adafdaf Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 9 Jul 2018 14:18:02 +0700 Subject: [PATCH 07/20] review: use equal operator instead Signed-off-by: Dhi Aurrahman --- .../http/grpc_json_transcoder/json_transcoder_filter.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index daf8e0bbf794..3b2919776d6b 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -440,12 +440,7 @@ void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& resp } bool JsonTranscoderFilter::hasHttpBodyAsOutputType() { - absl::string_view output_type = method_->output_type()->full_name(); - absl::string_view http_body_type = google::api::HttpBody::descriptor()->full_name(); - if (output_type.length() != http_body_type.length()) { - return false; - } - return StringUtil::startsWith(output_type.data(), http_body_type.data(), true); + return method_->output_type()->full_name() == google::api::HttpBody::descriptor()->full_name(); } } // namespace GrpcJsonTranscoder From 95778e8d1dbe4a2493caa36f24e705b39d2066a2 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 9 Jul 2018 21:35:42 +0700 Subject: [PATCH 08/20] review: clarify on supporting response only Signed-off-by: Dhi Aurrahman --- docs/root/intro/version_history.rst | 3 +- .../filters/http/grpc_json_transcoder/BUILD | 1 - .../json_transcoder_filter.cc | 43 +++++++++++-------- .../json_transcoder_filter.h | 5 ++- 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 3a948d2defa3..f76b71192519 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -7,7 +7,8 @@ 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 `_. -* grpc-json: added support for encoding `google.api.HttpBody `_. +* grpc-json: added support for building HTTP response from + `google.api.HttpBody `_. * health check: added support for :ref:`custom health check `. * health_check: added support for :ref:`health check event logging `. * http: response filters not applied to early error paths such as http_parser generated 400s. diff --git a/source/extensions/filters/http/grpc_json_transcoder/BUILD b/source/extensions/filters/http/grpc_json_transcoder/BUILD index 885b5fb2d651..f908f7e3c942 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/BUILD +++ b/source/extensions/filters/http/grpc_json_transcoder/BUILD @@ -28,7 +28,6 @@ envoy_cc_library( "//source/common/grpc:codec_lib", "//source/common/grpc:common_lib", "//source/common/http:headers_lib", - "//source/common/json:json_loader_lib", "//source/common/protobuf", "@envoy_api//envoy/config/filter/http/transcoder/v2:transcoder_cc", ], diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 3b2919776d6b..993bcbff2448 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -11,7 +11,6 @@ #include "common/grpc/common.h" #include "common/http/headers.h" #include "common/http/utility.h" -#include "common/json/json_loader.h" #include "common/protobuf/protobuf.h" #include "google/api/annotations.pb.h" @@ -225,6 +224,7 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h // just pass-through the request to upstream. return Http::FilterHeadersStatus::Continue; } + has_http_body_output_ = hasHttpBodyAsOutputType(); headers.removeContentLength(); headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc); @@ -343,6 +343,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, return Http::FilterDataStatus::Continue; } + if (has_http_body_output_ && buildResponseFromHttpBodyOutput(*response_headers_, data)) { + return Http::FilterDataStatus::StopIterationAndBuffer; + } + response_in_.move(data); if (end_stream) { @@ -352,13 +356,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, readToBuffer(*transcoder_->ResponseOutput(), data); if (!method_->server_streaming() && !end_stream) { - if (hasHttpBodyAsOutputType()) { - // Set content-type and data from HttpBody. - buildResponseFromHttpBodyOutput(*response_headers_, data); - } // Buffer until the response is complete. return Http::FilterDataStatus::StopIterationAndBuffer; } + // TODO(dio): Add support for streaming case. // TODO(lizan): Check ResponseStatus return Http::FilterDataStatus::Continue; @@ -422,21 +423,27 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea return false; } -void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, +bool JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data) { - try { - const Json::ObjectSharedPtr http_body = Json::Factory::loadFromString(data.toString()); - const std::string decoded_body = Base64::decode(http_body->getString("data")); - - data.drain(data.length()); - data.add(decoded_body); - - response_headers.insertContentType().value(http_body->getString("contentType")); - response_headers.insertContentLength().value(decoded_body.size()); - } catch (const Json::Exception& e) { - ENVOY_LOG(debug, "Failed to parse output of '{}'. e.what(): {}", method_->full_name(), - e.what()); + std::vector frames; + decoder_.decode(data, frames); + if (frames.empty()) { + return true; + } + + google::api::HttpBody http_body; + for (auto& frame : frames) { + if (frame.length_ > 0) { + Buffer::ZeroCopyInputStreamImpl stream(std::move(frame.data_)); + http_body.ParseFromZeroCopyStream(&stream); + const auto& body = http_body.data(); + data.add(body); + response_headers.insertContentType().value(http_body.content_type()); + response_headers.insertContentLength().value(body.length()); + return true; + } } + return false; } bool JsonTranscoderFilter::hasHttpBodyAsOutputType() { diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h index 922efaf49034..f796d620a53c 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h @@ -7,6 +7,7 @@ #include "envoy/json/json_object.h" #include "common/common/logger.h" +#include "common/grpc/codec.h" #include "common/protobuf/protobuf.h" #include "extensions/filters/http/grpc_json_transcoder/transcoder_input_stream_impl.h" @@ -117,7 +118,7 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< private: bool readToBuffer(Protobuf::io::ZeroCopyInputStream& stream, Buffer::Instance& data); - void buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data); + bool buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data); bool hasHttpBodyAsOutputType(); JsonTranscoderConfig& config_; @@ -128,8 +129,10 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< Http::StreamEncoderFilterCallbacks* encoder_callbacks_{nullptr}; const Protobuf::MethodDescriptor* method_{nullptr}; Http::HeaderMap* response_headers_{nullptr}; + Grpc::Decoder decoder_; bool error_{false}; + bool has_http_body_output_{false}; }; } // namespace GrpcJsonTranscoder From e76880e2fd60fdd937b18209a706d5b28ca23d20 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 9 Jul 2018 21:49:15 +0700 Subject: [PATCH 09/20] Remove base64 Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/grpc_json_transcoder/BUILD | 1 - .../filters/http/grpc_json_transcoder/json_transcoder_filter.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/BUILD b/source/extensions/filters/http/grpc_json_transcoder/BUILD index f908f7e3c942..f411adf589ac 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/BUILD +++ b/source/extensions/filters/http/grpc_json_transcoder/BUILD @@ -24,7 +24,6 @@ envoy_cc_library( deps = [ ":transcoder_input_stream_lib", "//include/envoy/http:filter_interface", - "//source/common/common:base64_lib", "//source/common/grpc:codec_lib", "//source/common/grpc:common_lib", "//source/common/http:headers_lib", diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 993bcbff2448..111382f0ddba 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -4,7 +4,6 @@ #include "envoy/http/filter.h" #include "common/common/assert.h" -#include "common/common/base64.h" #include "common/common/enum_to_int.h" #include "common/common/utility.h" #include "common/filesystem/filesystem_impl.h" From 3aa68fb2d5b59865797608742ffd5e6744a50813 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 10 Jul 2018 14:55:11 +0700 Subject: [PATCH 10/20] Process only one frame for now Signed-off-by: Dhi Aurrahman --- .../grpc_json_transcoder/json_transcoder_filter.cc | 10 +++++----- .../http/grpc_json_transcoder/json_transcoder_filter.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 111382f0ddba..6927ff0fec57 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -342,7 +342,8 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, return Http::FilterDataStatus::Continue; } - if (has_http_body_output_ && buildResponseFromHttpBodyOutput(*response_headers_, data)) { + if (has_http_body_output_) { + buildResponseFromHttpBodyOutput(*response_headers_, data); return Http::FilterDataStatus::StopIterationAndBuffer; } @@ -422,12 +423,12 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea return false; } -bool JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, +void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data) { std::vector frames; decoder_.decode(data, frames); if (frames.empty()) { - return true; + return; } google::api::HttpBody http_body; @@ -439,10 +440,9 @@ bool JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& resp data.add(body); response_headers.insertContentType().value(http_body.content_type()); response_headers.insertContentLength().value(body.length()); - return true; + return; } } - return false; } bool JsonTranscoderFilter::hasHttpBodyAsOutputType() { diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h index f796d620a53c..5849bcc96b2f 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h @@ -118,7 +118,7 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< private: bool readToBuffer(Protobuf::io::ZeroCopyInputStream& stream, Buffer::Instance& data); - bool buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data); + void buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data); bool hasHttpBodyAsOutputType(); JsonTranscoderConfig& config_; From 4123018b7f9ba43691c12f4414d89d394fcde0c0 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 11 Jul 2018 09:57:24 +0700 Subject: [PATCH 11/20] review: move comment, set when !server_streaming Signed-off-by: Dhi Aurrahman --- .../json_transcoder_filter.cc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 6927ff0fec57..f3b9b6291b09 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -225,12 +225,14 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h } has_http_body_output_ = hasHttpBodyAsOutputType(); - headers.removeContentLength(); - headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc); - headers.insertEnvoyOriginalPath().value(*headers.Path()); - headers.insertPath().value("/" + method_->service()->full_name() + "/" + method_->name()); - headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); - headers.insertTE().value().setReference(Http::Headers::get().TEValues.Trailers); + if (!method_->server_streaming()) { + headers.removeContentLength(); + headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc); + headers.insertEnvoyOriginalPath().value(*headers.Path()); + headers.insertPath().value("/" + method_->service()->full_name() + "/" + method_->name()); + headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); + headers.insertTE().value().setReference(Http::Headers::get().TEValues.Trailers); + } if (!config_.matchIncomingRequestInfo()) { decoder_callbacks_->clearRouteCache(); @@ -342,6 +344,7 @@ 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); return Http::FilterDataStatus::StopIterationAndBuffer; From 085e967851d6c3c69bc96a742ba008376fce1243 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 11 Jul 2018 10:00:29 +0700 Subject: [PATCH 12/20] Remove duplicated comments Signed-off-by: Dhi Aurrahman --- .../filters/http/grpc_json_transcoder/json_transcoder_filter.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index f3b9b6291b09..47a75d328782 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -362,7 +362,6 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, // Buffer until the response is complete. return Http::FilterDataStatus::StopIterationAndBuffer; } - // TODO(dio): Add support for streaming case. // TODO(lizan): Check ResponseStatus return Http::FilterDataStatus::Continue; From d5c4ba9e88e48b126ebc4dd223a30fefbd5bb86f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 11 Jul 2018 13:45:44 +0700 Subject: [PATCH 13/20] Only handle HttpBody when !server_streaming Signed-off-by: Dhi Aurrahman --- .../json_transcoder_filter.cc | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 47a75d328782..0d02ea3553dd 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -223,16 +223,14 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h // just pass-through the request to upstream. return Http::FilterHeadersStatus::Continue; } - has_http_body_output_ = hasHttpBodyAsOutputType(); - - if (!method_->server_streaming()) { - headers.removeContentLength(); - headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc); - headers.insertEnvoyOriginalPath().value(*headers.Path()); - headers.insertPath().value("/" + method_->service()->full_name() + "/" + method_->name()); - headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); - headers.insertTE().value().setReference(Http::Headers::get().TEValues.Trailers); - } + has_http_body_output_ = !method_->server_streaming() && hasHttpBodyAsOutputType(); + + headers.removeContentLength(); + headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc); + headers.insertEnvoyOriginalPath().value(*headers.Path()); + headers.insertPath().value("/" + method_->service()->full_name() + "/" + method_->name()); + headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Post); + headers.insertTE().value().setReference(Http::Headers::get().TEValues.Trailers); if (!config_.matchIncomingRequestInfo()) { decoder_callbacks_->clearRouteCache(); From c2dc9e82263c54152df58f40b4f53928ed68dd1d Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 12 Jul 2018 23:53:21 +0700 Subject: [PATCH 14/20] review: add todo skylark macro Signed-off-by: Dhi Aurrahman --- api/bazel/repositories.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index da383bf5a34d..64834a1b4c4e 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -17,6 +17,7 @@ def api_dependencies(): name = "googleapis", strip_prefix = "googleapis-" + GOOGLEAPIS_SHA, url = "https://github.com/googleapis/googleapis/archive/" + GOOGLEAPIS_SHA + ".tar.gz", + # TODO(dio): Consider writing a Skylark macro for importing Google API proto. build_file_content = """ load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library") load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") From e9d9b1e0e1771999b1cc189455ff50b0154548cb Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 12 Jul 2018 23:53:58 +0700 Subject: [PATCH 15/20] review: clarify the use of HttpBody fields Signed-off-by: Dhi Aurrahman --- .../http_filters/grpc_json_transcoder_filter.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst b/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst index b6cb4f3a4b7c..471297286a12 100644 --- a/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst +++ b/docs/root/configuration/http_filters/grpc_json_transcoder_filter.rst @@ -67,9 +67,11 @@ Sending arbitrary content ------------------------- By default, when transcoding occurs, gRPC-JSON encodes the message output of a gRPC service method into -JSON and sets the response `Content-Type` header to `application/json`. To send abritrary content, a gRPC -service method can use +JSON and sets the HTTP response `Content-Type` header to `application/json`. To send abritrary content, +a gRPC service method can use `google.api.HttpBody `_ as its output message type. The implementation needs to set `content_type `_ -and `data `_ accordingly. \ No newline at end of file +(which sets the value of the HTTP response `Content-Type` header) and +`data `_ +(which sets the HTTP response body) accordingly. \ No newline at end of file From c0d7c87808f331828dfa2f55377b998656683c06 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 13 Jul 2018 00:27:53 +0700 Subject: [PATCH 16/20] Add UnaryGetHttpBody integration test Signed-off-by: Dhi Aurrahman --- .../grpc_json_transcoder_integration_test.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc index a63ac5120f11..3ee79fc7234a 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc @@ -172,6 +172,17 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGet) { R"({"shelves":[{"id":"20","theme":"Children"},{"id":"1","theme":"Foo"}]})"); } +TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetHttpBody) { + testTranscoding( + Http::TestHeaderMapImpl{{":method", "GET"}, {":path", "/index"}, {":authority", "host"}}, + "", {""}, {R"(content_type: "text/html" data: "

Hello!

" )"}, Status(), + Http::TestHeaderMapImpl{{":status", "200"}, + {"content-type", "text/html"}, + {"content-length", "15"}, + {"grpc-status", "0"}}, + R"(

Hello!

)"); +} + TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetError) { testTranscoding( Http::TestHeaderMapImpl{ From b45854a39a0aef6f33eaea815dc0b2511665750f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 13 Jul 2018 00:30:09 +0700 Subject: [PATCH 17/20] Fix format Signed-off-by: Dhi Aurrahman --- .../grpc_json_transcoder_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc index 3ee79fc7234a..2417a31794de 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc @@ -174,8 +174,8 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGet) { TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetHttpBody) { testTranscoding( - Http::TestHeaderMapImpl{{":method", "GET"}, {":path", "/index"}, {":authority", "host"}}, - "", {""}, {R"(content_type: "text/html" data: "

Hello!

" )"}, Status(), + Http::TestHeaderMapImpl{{":method", "GET"}, {":path", "/index"}, {":authority", "host"}}, "", + {""}, {R"(content_type: "text/html" data: "

Hello!

" )"}, Status(), Http::TestHeaderMapImpl{{":status", "200"}, {"content-type", "text/html"}, {"content-length", "15"}, From d93f57e0a9d365582d1cde1d256c528af59a21c0 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 13 Jul 2018 02:11:24 +0700 Subject: [PATCH 18/20] Add test for splitting buffer into two encode data Signed-off-by: Dhi Aurrahman --- .../json_transcoder_filter_test.cc | 53 +++++++++++++++++-- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index 61cfb9e4de51..eb183b8254e2 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -561,10 +561,6 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutput) { EXPECT_EQ("/bookstore.Bookstore/GetIndex", request_headers.get_(":path")); EXPECT_EQ("trailers", request_headers.get_("te")); - Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_.encode100ContinueHeaders(continue_headers)); - Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, {":status", "200"}}; @@ -581,7 +577,54 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutput) { EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(*response_data, false)); - std::string response_html = response_data->toString(); + const std::string response_html = response_data->toString(); + + EXPECT_EQ("text/html", response_headers.get_("content-type")); + EXPECT_EQ("

Hello, world!

", response_html); + + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "0"}, {"grpc-message", ""}}; + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); +} + +TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutputAndSplitTwoEncodeData) { + Http::TestHeaderMapImpl request_headers{{":method", "GET"}, {":path", "/index"}}; + + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + EXPECT_EQ("application/grpc", request_headers.get_("content-type")); + EXPECT_EQ("/index", request_headers.get_("x-envoy-original-path")); + EXPECT_EQ("/bookstore.Bookstore/GetIndex", request_headers.get_(":path")); + EXPECT_EQ("trailers", request_headers.get_("te")); + + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, + {":status", "200"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + + google::api::HttpBody response; + response.set_content_type("text/html"); + response.set_data("

Hello, world!

"); + + auto response_data = Grpc::Common::serializeBody(response); + EXPECT_EQ(40, response_data->length()); + + // The response data buffer is splitted into two parts. + std::array out; + Buffer::OwnedImpl part1; + response_data->copyOut(0, 20, out.data()); + part1.add(std::string(out.begin(), out.end())); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(part1, false)); + + Buffer::OwnedImpl part2; + response_data->copyOut(20, 20, out.data()); + part2.add(std::string(out.begin(), out.end())); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(part2, false)); + + const std::string response_html = part2.toString(); EXPECT_EQ("text/html", response_headers.get_("content-type")); EXPECT_EQ("

Hello, world!

", response_html); From 40d9b3fe7ea8786327cb9159fc5832359448ea18 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 13 Jul 2018 17:13:37 +0700 Subject: [PATCH 19/20] review: simplify splitting the buffer Thanks @lizan! Signed-off-by: Dhi Aurrahman --- .../json_transcoder_filter_test.cc | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index ac82e078c28f..d3e31a2f5402 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -610,21 +610,21 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutputAndSpli response.set_data("

Hello, world!

"); auto response_data = Grpc::Common::serializeBody(response); - EXPECT_EQ(40, response_data->length()); - // The response data buffer is splitted into two parts. - std::array out; - Buffer::OwnedImpl part1; - response_data->copyOut(0, 20, out.data()); - part1.add(std::string(out.begin(), out.end())); - EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(part1, false)); + // Firstly, the response data buffer is splitted into two parts. + Buffer::OwnedImpl response_data_first_part; + response_data_first_part.move(*response_data, response_data->length() / 2); - Buffer::OwnedImpl part2; - response_data->copyOut(20, 20, out.data()); - part2.add(std::string(out.begin(), out.end())); - EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(part2, false)); + // Secondly, we send the first part of response data to the data encoding step. + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, + filter_.encodeData(response_data_first_part, false)); - const std::string response_html = part2.toString(); + // Finaly, since half of the response data buffer is moved already, here we can send the rest + // of it to the next data encoding step. + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, + filter_.encodeData(*response_data, false)); + + const std::string response_html = response_data->toString(); EXPECT_EQ("text/html", response_headers.get_("content-type")); EXPECT_EQ("

Hello, world!

", response_html); From 94037231eea7150a20b4a07b8c981ae89d1a0a60 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Fri, 13 Jul 2018 17:31:49 +0700 Subject: [PATCH 20/20] Reuse response object as expected values Signed-off-by: Dhi Aurrahman --- .../json_transcoder_filter_test.cc | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index d3e31a2f5402..f388588a085e 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -577,13 +577,10 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutput) { EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(*response_data, false)); - const std::string response_html = response_data->toString(); - - EXPECT_EQ("text/html", response_headers.get_("content-type")); - EXPECT_EQ("

Hello, world!

", response_html); + EXPECT_EQ(response.content_type(), response_headers.get_("content-type")); + EXPECT_EQ(response.data(), response_data->toString()); Http::TestHeaderMapImpl response_trailers{{"grpc-status", "0"}, {"grpc-message", ""}}; - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); } @@ -624,13 +621,10 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutputAndSpli EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(*response_data, false)); - const std::string response_html = response_data->toString(); - - EXPECT_EQ("text/html", response_headers.get_("content-type")); - EXPECT_EQ("

Hello, world!

", response_html); + EXPECT_EQ(response.content_type(), response_headers.get_("content-type")); + EXPECT_EQ(response.data(), response_data->toString()); Http::TestHeaderMapImpl response_trailers{{"grpc-status", "0"}, {"grpc-message", ""}}; - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); }