Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 25 commits into from
Jul 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ce46d96
grpc-json: handle google.api.HttpBody
dio Jul 5, 2018
aa8e4cf
Wrap in exception
dio Jul 5, 2018
ac4455b
Log method name instead
dio Jul 5, 2018
fcd37be
Add api_httpbody_protos
dio Jul 5, 2018
da90483
Fix on mac api build
dio Jul 5, 2018
59cf968
Merge remote-tracking branch 'upstream/master'
dio Jul 6, 2018
6dcd9a6
Merge remote-tracking branch 'upstream/master'
dio Jul 6, 2018
5aa346a
review: remove comment related to bazel
dio Jul 9, 2018
9f4739b
review: use equal operator instead
dio Jul 9, 2018
95778e8
review: clarify on supporting response only
dio Jul 9, 2018
e76880e
Remove base64
dio Jul 9, 2018
eff3c80
Merge remote-tracking branch 'upstream/master'
dio Jul 10, 2018
3aa68fb
Process only one frame for now
dio Jul 10, 2018
4123018
review: move comment, set when !server_streaming
dio Jul 11, 2018
085e967
Remove duplicated comments
dio Jul 11, 2018
d5c4ba9
Only handle HttpBody when !server_streaming
dio Jul 11, 2018
784d789
Merge remote-tracking branch 'upstream/master'
dio Jul 11, 2018
c2dc9e8
review: add todo skylark macro
dio Jul 12, 2018
e9d9b1e
review: clarify the use of HttpBody fields
dio Jul 12, 2018
c0d7c87
Add UnaryGetHttpBody integration test
dio Jul 12, 2018
b45854a
Fix format
dio Jul 12, 2018
d93f57e
Add test for splitting buffer into two encode data
dio Jul 12, 2018
b9a5e43
Merge remote-tracking branch 'upstream/master'
dio Jul 13, 2018
40d9b3f
review: simplify splitting the buffer
dio Jul 13, 2018
9403723
Reuse response object as expected values
dio Jul 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/bazel/api_build_system.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,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",
Expand Down Expand Up @@ -106,6 +107,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",
Expand Down
52 changes: 51 additions & 1 deletion api/bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,67 @@ 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")

filegroup(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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",
],
visibility = ["//visibility:public"],
)
)

go_proto_library(
name = "descriptor_go_proto",
Expand Down Expand Up @@ -93,6 +142,7 @@ proto_library(
deps = ["@com_google_protobuf//:any_proto"],
visibility = ["//visibility:public"],
)

cc_proto_library(
name = "rpc_status_protos",
srcs = ["google/rpc/status.proto"],
Expand Down
8 changes: 8 additions & 0 deletions bazel/envoy_build_system.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,10 @@ def envoy_proto_library(
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")
Expand Down Expand Up @@ -420,6 +424,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")
Expand Down
4 changes: 4 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,16 @@ 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 HTTP response `Content-Type` header to `application/json`. To send abritrary content,
a gRPC service method can use
`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>`_
(which sets the value of the HTTP response `Content-Type` header) and
`data <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto#L71>`_
(which sets the HTTP response body) accordingly.
3 changes: 3 additions & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ 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 building HTTP response from
`google.api.HttpBody <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto>`_.
* config: v1 disabled by default. v1 support remains available until October via flipping --v2-config-only=false.
* config: v1 disabled by default. v1 support remains available until October via setting :option:`--allow-deprecated-v1-api`.
* health check: added support for :ref:`custom health check <envoy_api_field_core.HealthCheck.custom_health_check>`.
* health check: added support for :ref:`specifying jitter as a percentage <envoy_api_field_core.HealthCheck.interval_jitter_percent>`.
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/grpc_json_transcoder/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ envoy_cc_library(
"path_matcher",
"grpc_transcoding",
"http_api_protos",
"api_httpbody_protos",
],
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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#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"
Expand Down Expand Up @@ -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_ = !method_->server_streaming() && hasHttpBodyAsOutputType();

headers.removeContentLength();
headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc);
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dio dio Jul 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@lizan got it, makes sense, thanks.

Copy link
Member Author

@dio dio Jul 12, 2018

Choose a reason for hiding this comment

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

@lizan added d93f57e.

return Http::FilterDataStatus::StopIterationAndBuffer;
}

response_in_.move(data);

if (end_stream) {
Expand Down Expand Up @@ -415,6 +423,32 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea
return false;
}

void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers,
Buffer::Instance& data) {
std::vector<Grpc::Frame> frames;
decoder_.decode(data, frames);
if (frames.empty()) {
return;
}

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;
}
}
}

bool JsonTranscoderFilter::hasHttpBodyAsOutputType() {
return method_->output_type()->full_name() == google::api::HttpBody::descriptor()->full_name();
}

} // namespace GrpcJsonTranscoder
} // namespace HttpFilters
} // namespace Extensions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -117,6 +118,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<google::grpc::transcoding::Transcoder> transcoder_;
Expand All @@ -126,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,17 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGet) {
R"({"shelves":[{"id":"20","theme":"Children"},{"id":"1","theme":"Foo"}]})");
}

TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetHttpBody) {
testTranscoding<Empty, google::api::HttpBody>(
Http::TestHeaderMapImpl{{":method", "GET"}, {":path", "/index"}, {":authority", "host"}}, "",
{""}, {R"(content_type: "text/html" data: "<h1>Hello!</h1>" )"}, Status(),
Http::TestHeaderMapImpl{{":status", "200"},
{"content-type", "text/html"},
{"content-length", "15"},
{"grpc-status", "0"}},
R"(<h1>Hello!</h1>)");
}

TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetError) {
testTranscoding<bookstore::GetShelfRequest, bookstore::Shelf>(
Http::TestHeaderMapImpl{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,84 @@ 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 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("<h1>Hello, world!</h1>");

auto response_data = Grpc::Common::serializeBody(response);

EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer,
filter_.encodeData(*response_data, false));

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));
}

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("<h1>Hello, world!</h1>");

auto response_data = Grpc::Common::serializeBody(response);

// 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);

// 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));

// 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));

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));
}

struct GrpcJsonTranscoderFilterPrintTestParam {
std::string config_json_;
std::string expected_response_;
Expand Down
4 changes: 3 additions & 1 deletion test/proto/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)

Expand All @@ -32,6 +33,7 @@ envoy_proto_descriptor(
],
out = "bookstore.descriptor",
external_deps = [
"api_httpbody_protos",
"http_api_protos",
"well_known_protos",
],
Expand Down
6 changes: 6 additions & 0 deletions test/proto/bookstore.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down