grpc-json: Handle streaming backend error cases correctly

[nareddyt]

The grpc-json-transcoder filter does not handle errors from streaming backends correctly. I opened this issue to document the current and proposed downstream responses.

Note that #8009 introduced a new convert_grpc_status flag to deduce the HTTP response code and body from the gRPC backend error. This flag introduces another edge case (below).

I think the logic for unary responses is OK. I've only seen these issues with streaming backends.

cc @qiwzhang
cc @ascheglov

Streaming Error with flag set

Configuration: The backend returns a gRPC error via a trailer-only response. The filter is configured with convert_grpc_status=true.

Current Behavior

The filter returns a HTTP 200 OK. The grpc status flag is set in one of the response headers. Assuming that #8312 will be merged in, the response will be [] with content-type application/json.

Proposed Behavior

The filter should return a response that matches the unary case with convert_grpc_status set:

• The HTTP code should match the gRPC status code.
• The response body should be a JSON with the error message.

Streaming Error without flag

Configuration: The backend returns a gRPC error via a trailer-only response. The filter is configured without convert_grpc_status (default behavior).

Current Behavior

Same as above: The filter returns a HTTP 200 OK. The grpc status flag is set in one of the response headers. Assuming that #8312 will be merged in, the response will be [] with content-type application/json.

Proposed Behavior

The filter should return a response that exposes the error:

• The HTTP code should match the gRPC status code.
• The body should be completely empty (""), not an empty JSON array
• Content type should not be set, or it shouldn't matter what it is set to

Other Notes

I would prefer to get #8312 merged in first to fix the content type issue. Please advise on how to handle this, as it may be a breaking change (?)

[nareddyt]

FYI the behavior of the first case (streaming error with flag set) is a bug. The convert_grpc_status flag does not do anything.

This can be verified by adding the following test case to //test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc, under the UnaryErrorConvertedToJson test. It will fail.

// 2: 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", "404"},
                              {"content-type", "application/json"},
                              {"grpc-status", UnexpectedHeaderValue},
                              {"grpc-message", UnexpectedHeaderValue}},
      "[]");

[ascheglov]

Here is what happens in case of a trailer-only streaming error:
The transcoder filter gets encodeHeaders with end_stream=true, then it
asks the transcoder if it has any data to encode, and transcoder returns that [].
Then since there is some response data, the filter sets its has_body_ flag and doesn't transcode the gRPC error headers.

Well, we can fix that. There is no need to ask the transcoder for data if we know that it's a trailers-only response.