Skip to content

Commit

Permalink
Fix grpc transcoding content type not matching response body (envoypr…
Browse files Browse the repository at this point in the history
…oxy#8312)

* When trailer indicates a gRPC error and there was no HTTP body, ensure that the returned content type is `application/json`. This is required because the response body is an empty JSON body by default.

Risk Level: Low
Testing: Modified integration test to catch this bug.
Docs Changes: None
Release Notes: None

Fixes: envoyproxy#5011
Ref: envoyproxy#8158
Ref: envoyproxy#8009

Signed-off-by: Teju Nareddy <nareddyt@google.com>
  • Loading branch information
nareddyt committed Sep 24, 2019
1 parent 72161df commit 62a6478
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -400,9 +400,17 @@ Http::FilterHeadersStatus JsonTranscoderFilter::encodeHeaders(Http::HeaderMap& h
response_headers_ = &headers;

if (end_stream) {

if (method_->server_streaming()) {
// When there is no body in a streaming response, a empty JSON array is
// returned by default. Set the content type correctly.
headers.insertContentType().value().setReference(Http::Headers::get().ContentTypeValues.Json);
}

// In gRPC wire protocol, headers frame with end_stream is a trailers-only response.
// The return value from encodeTrailers is ignored since it is always continue.
encodeTrailers(headers);

return Http::FilterHeadersStatus::Continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,8 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, BindingAndBody) {

TEST_P(GrpcJsonTranscoderIntegrationTest, ServerStreamingGet) {
HttpIntegrationTest::initialize();

// 1: Normal streaming get
testTranscoding<bookstore::ListBooksRequest, bookstore::Book>(
Http::TestHeaderMapImpl{
{":method", "GET"}, {":path", "/shelves/1/books"}, {":authority", "host"}},
Expand All @@ -439,6 +441,22 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, ServerStreamingGet) {
Status(), Http::TestHeaderMapImpl{{":status", "200"}, {"content-type", "application/json"}},
R"([{"id":"1","author":"Neal Stephenson","title":"Readme"})"
R"(,{"id":"2","author":"George R.R. Martin","title":"A Game of Thrones"}])");

// 2: Empty response (trailers only) from streaming backend.
// Response type is a valid JSON, so content type should be application/json.
// Regression test for github.com/envoyproxy/envoy#5011
testTranscoding<bookstore::ListBooksRequest, bookstore::Book>(
Http::TestHeaderMapImpl{
{":method", "GET"}, {":path", "/shelves/2/books"}, {":authority", "host"}},
"", {"shelf: 2"}, {}, Status(),
Http::TestHeaderMapImpl{{":status", "200"}, {"content-type", "application/json"}}, "[]");

// 3: Empty response (trailers only) from streaming backend, with a gRPC error.
testTranscoding<bookstore::ListBooksRequest, bookstore::Book>(
Http::TestHeaderMapImpl{
{":method", "GET"}, {":path", "/shelves/37/books"}, {":authority", "host"}},
"", {"shelf: 37"}, {}, Status(Code::NOT_FOUND, "Shelf 37 not found"),
Http::TestHeaderMapImpl{{":status", "200"}, {"content-type", "application/json"}}, "[]");
}

TEST_P(GrpcJsonTranscoderIntegrationTest, StreamingPost) {
Expand Down Expand Up @@ -553,6 +571,7 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, DeepStruct) {

// The valid deep struct is parsed successfully.
// Since we didn't set the response, it return 503.
// Response body is empty (not a valid JSON), so content type should be application/grpc.
testTranscoding<bookstore::EchoStructReqResp, bookstore::EchoStructReqResp>(
Http::TestHeaderMapImpl{
{":method", "POST"}, {":path", "/echoStruct"}, {":authority", "host"}},
Expand Down

0 comments on commit 62a6478

Please sign in to comment.