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 7 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
7 changes: 7 additions & 0 deletions api/bazel/api_build_system.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Is this similar issue to bazelbuild/bazel#5163 ?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will remove the comment.

# 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",
Expand Down
51 changes: 50 additions & 1 deletion api/bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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(
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 +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"],
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 @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
4 changes: 4 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <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.
Copy link
Member

Choose a reason for hiding this comment

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

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

1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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>`_.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

* health check: added support for :ref:`custom health check <envoy_api_field_core.HealthCheck.custom_health_check>`.
* health_check: added support for :ref:`health check event logging <arch_overview_health_check_logging>`.
* http: response filters not applied to early error paths such as http_parser generated 400s.
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/http/grpc_json_transcoder/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ envoy_cc_library(
"path_matcher",
"grpc_transcoding",
"http_api_protos",
"api_httpbody_protos",
],
deps = [
":transcoder_input_stream_lib",
Expand All @@ -27,6 +28,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",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -349,6 +352,10 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,
readToBuffer(*transcoder_->ResponseOutput(), data);

if (!method_->server_streaming() && !end_stream) {
Copy link
Member

Choose a reason for hiding this comment

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

support or add a TODO for 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.

👍

if (hasHttpBodyAsOutputType()) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

// Set content-type and data from HttpBody.
buildResponseFromHttpBodyOutput(*response_headers_, data);
}
// Buffer until the response is complete.
return Http::FilterDataStatus::StopIterationAndBuffer;
}
Expand Down Expand Up @@ -415,6 +422,32 @@ bool JsonTranscoderFilter::readToBuffer(Protobuf::io::ZeroCopyInputStream& strea
return false;
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

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 have the length comparison previously. And since the comparison needs to be case-sensitive?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will use the equal operator then.

}

} // namespace GrpcJsonTranscoder
} // namespace HttpFilters
} // namespace Extensions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<google::grpc::transcoding::Transcoder> transcoder_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the 100 continue stuff here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in d93f57e.


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

std::string response_html = response_data->toString();

EXPECT_EQ("text/html", response_headers.get_("content-type"));
EXPECT_EQ("<h1>Hello, world!</h1>", 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_;
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