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.

Sign up for GitHub

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

Jump to bottom

grpc-json: no buffering at end_stream or non-gRPC response #2363

Merged
merged 9 commits into from
Jan 17, 2018
Merged

grpc-json: no buffering at end_stream or non-gRPC response #2363

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f38ba50
grpc-json: no buffering at stream end
lizan Jan 13, 2018
9e529fd
addressed comment
lizan Jan 14, 2018
fe9e518
Merge remote-tracking branch 'upstream/master' into grpc_transcoder_t…
lizan Jan 14, 2018
2e5890f
Merge remote-tracking branch 'upstream/master' into grpc_transcoder_t…
lizan Jan 17, 2018
b6aacf3
Move isGrpcResponseHeader to Common
lizan Jan 17, 2018
d7ee00f
fix format, add comment
lizan Jan 17, 2018
f265b0b
fix comment
lizan Jan 17, 2018
8dad955
remove CLion inserted include
lizan Jan 17, 2018
8698422
fix format
lizan Jan 17, 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
16 changes: 15 additions & 1 deletion source/common/grpc/json_transcoder_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "common/grpc/json_transcoder_filter.h"

#include <common/http/headers.h>

#include "envoy/common/exception.h"
#include "envoy/http/filter.h"

Expand Down Expand Up @@ -70,6 +72,14 @@ class TranscoderImpl : public Transcoder {
std::unique_ptr<TranscoderInputStream> response_stream_;
};

bool isGrpcResponse(const Http::HeaderMap& headers) {
if (Http::Utility::getResponseStatus(headers) != 200) {
return false;
}
return headers.ContentType() &&
Copy link
Member

Choose a reason for hiding this comment

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

Should this use https://github.com/envoyproxy/envoy/blob/master/source/common/grpc/common.cc#L25? Maybe this util belongs in Grpc::Common as well.

headers.ContentType()->value() == Http::Headers::get().ContentTypeValues.Grpc.c_str();
}

} // namespace

JsonTranscoderConfig::JsonTranscoderConfig(
Expand Down Expand Up @@ -295,6 +305,10 @@ void JsonTranscoderFilter::setDecoderFilterCallbacks(

Http::FilterHeadersStatus JsonTranscoderFilter::encodeHeaders(Http::HeaderMap& headers,
bool end_stream) {
if (!isGrpcResponse(headers)) {
error_ = true;
}

if (error_ || !transcoder_) {
return Http::FilterHeadersStatus::Continue;
}
Expand Down Expand Up @@ -329,7 +343,7 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data,

readToBuffer(*transcoder_->ResponseOutput(), data);

if (!method_->server_streaming()) {
if (!method_->server_streaming() && !end_stream) {
// Buffer until the response is complete.
return Http::FilterDataStatus::StopIterationAndBuffer;
}
Expand Down
50 changes: 48 additions & 2 deletions test/common/grpc/json_transcoder_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,16 @@ TEST_F(GrpcJsonTranscoderFilterTest, NoTranscoding) {
Http::TestHeaderMapImpl request_trailers;
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(request_trailers));

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(request_headers, false));
EXPECT_EQ(expected_request_headers, request_headers);
Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"},
{":status", "200"},
{":path", "/grpc.service/UnknownGrpcMethod"}};

Http::TestHeaderMapImpl expected_response_headers{{"content-type", "application/grpc"},
{":status", "200"},
{":path", "/grpc.service/UnknownGrpcMethod"}};

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false));
EXPECT_EQ(expected_response_headers, response_headers);

Buffer::OwnedImpl response_data{"{}"};
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(response_data, false));
Expand Down Expand Up @@ -334,6 +342,44 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryError) {
EXPECT_EQ(0, request_data.length());
}

TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryTimeout) {
Http::TestHeaderMapImpl request_headers{
{"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}};

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
EXPECT_EQ("/bookstore.Bookstore/CreateShelf", request_headers.get_(":path"));
EXPECT_EQ("trailers", request_headers.get_("te"));

Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"};

EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true));

Http::TestHeaderMapImpl response_headers{
{":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false));
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(request_data, true));
}

TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryNotGrpcResponse) {
Http::TestHeaderMapImpl request_headers{
{"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}};

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
EXPECT_EQ("/bookstore.Bookstore/CreateShelf", request_headers.get_(":path"));
EXPECT_EQ("trailers", request_headers.get_("te"));

Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"};

EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true));

Http::TestHeaderMapImpl response_headers{
{":status", "200"}, {"content-length", "24"}, {"content-type", "text/plain"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, false));
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.encodeData(request_data, true));
}

struct GrpcJsonTranscoderFilterPrintTestParam {
std::string config_json_;
std::string expected_response_;
Expand Down