-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Changes from 13 commits
ce46d96
aa8e4cf
ac4455b
fcd37be
da90483
59cf968
6dcd9a6
5aa346a
9f4739b
95778e8
e76880e
eff3c80
3aa68fb
4123018
085e967
d5c4ba9
784d789
c2dc9e8
e9d9b1e
c0d7c87
b45854a
d93f57e
b9a5e43
40d9b3f
9403723
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -222,6 +223,7 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h | |
// just pass-through the request to upstream. | ||
return Http::FilterHeadersStatus::Continue; | ||
} | ||
has_http_body_output_ = hasHttpBodyAsOutputType(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the behavior below depends on non-streaming handling, (i.e. buffer response until trailer), lets only set this when server_streaming is false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @lizan sorry, I need to clarify this. Do you want to make sure that we only set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah OK, done. Thanks. |
||
|
||
headers.removeContentLength(); | ||
headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Grpc); | ||
|
@@ -340,6 +342,11 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, | |
return Http::FilterDataStatus::Continue; | ||
} | ||
|
||
if (has_http_body_output_) { | ||
buildResponseFromHttpBodyOutput(*response_headers_, data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @htuch I think @dio already does this correctly, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lizan got it, makes sense, thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return Http::FilterDataStatus::StopIterationAndBuffer; | ||
} | ||
|
||
response_in_.move(data); | ||
|
||
if (end_stream) { | ||
|
@@ -352,6 +359,7 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, | |
// Buffer until the response is complete. | ||
return Http::FilterDataStatus::StopIterationAndBuffer; | ||
} | ||
// TODO(dio): Add support for streaming case. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move this todo to L226 or L345. |
||
// TODO(lizan): Check ResponseStatus | ||
|
||
return Http::FilterDataStatus::Continue; | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the 100 continue stuff here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_; | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.