From d2ab4e3148a00128ca82eedbabaf83d60adb94a8 Mon Sep 17 00:00:00 2001
From: Anatoly Scheglov <abyx@ujkl.ru>
Date: Fri, 4 Oct 2019 02:11:54 +0300
Subject: [PATCH] grpc-json: Handle streaming backend error cases correctly
(#8323)
When a streaming backend returns an error in a trailer-only response,
with the 'convert_grpc_status' option enabled,
instead of writing the [] (empty array) in reply-body
transcode the error in the headers and reply with {"code":...}.
Risk Level: Low
Testing: added an integraion test, tested manually.
Documentation: n/a
Release notes: n/a
Fixes #8315
Signed-off-by: Anatoly Scheglov <ascheglov@yandex-team.ru>
---
.../json_transcoder_filter.cc | 46 +++++++++++++------
.../grpc_json_transcoder_integration_test.cc | 24 ++++++++++
2 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
index 65419ec3125d..619c62d1683c 100644
--- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
+++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
@@ -52,6 +52,11 @@ struct RcDetailsValues {
using RcDetails = ConstSingleton<RcDetailsValues>;
namespace {
+
+const Http::LowerCaseString& trailerHeader() {
+ CONSTRUCT_ON_FIRST_USE(Http::LowerCaseString, "trailer");
+}
+
// Transcoder:
// https://github.com/grpc-ecosystem/grpc-httpjson-transcoding/blob/master/src/include/grpc_transcoding/transcoder.h
// implementation based on JsonRequestTranslator & ResponseToJsonTranslator
@@ -459,12 +464,17 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap&
response_in_.finish();
+ const absl::optional<Grpc::Status::GrpcStatus> grpc_status =
+ Grpc::Common::getGrpcStatus(trailers);
+ if (grpc_status && maybeConvertGrpcStatus(*grpc_status, trailers)) {
+ return Http::FilterTrailersStatus::Continue;
+ }
+
Buffer::OwnedImpl data;
readToBuffer(*transcoder_->ResponseOutput(), data);
if (data.length()) {
encoder_callbacks_->addEncodedData(data, true);
- has_body_ = true;
}
if (method_->server_streaming()) {
@@ -476,25 +486,16 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap&
// so there is no need to copy headers from one to the other.
bool is_trailers_only_response = response_headers_ == &trailers;
- const absl::optional<Grpc::Status::GrpcStatus> grpc_status =
- Grpc::Common::getGrpcStatus(trailers);
- bool status_converted_to_json = grpc_status && maybeConvertGrpcStatus(*grpc_status, trailers);
-
if (!grpc_status || grpc_status.value() == Grpc::Status::GrpcStatus::InvalidCode) {
response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable));
} else {
response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status.value()));
- if (!status_converted_to_json && !is_trailers_only_response) {
+ if (!is_trailers_only_response) {
response_headers_->insertGrpcStatus().value(enumToInt(grpc_status.value()));
}
}
- if (status_converted_to_json && is_trailers_only_response) {
- // Drop the gRPC status headers, we already have them in the JSON body.
- response_headers_->removeGrpcStatus();
- response_headers_->removeGrpcMessage();
- response_headers_->remove(Http::Headers::get().GrpcStatusDetailsBin);
- } else if (!status_converted_to_json && !is_trailers_only_response) {
+ if (!is_trailers_only_response) {
// Copy the grpc-message header if it exists.
const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage();
if (grpc_message_header) {
@@ -504,8 +505,7 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap&
// remove Trailer headers if the client connection was http/1
if (encoder_callbacks_->streamInfo().protocol() != Http::Protocol::Http2) {
- static const Http::LowerCaseString trailer_key = Http::LowerCaseString("trailer");
- response_headers_->remove(trailer_key);
+ response_headers_->remove(trailerHeader());
}
response_headers_->insertContentLength().value(
@@ -594,8 +594,26 @@ bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_
return false;
}
+ response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status));
+
+ bool is_trailers_only_response = response_headers_ == &trailers;
+ if (is_trailers_only_response) {
+ // Drop the gRPC status headers, we already have them in the JSON body.
+ response_headers_->removeGrpcStatus();
+ response_headers_->removeGrpcMessage();
+ response_headers_->remove(Http::Headers::get().GrpcStatusDetailsBin);
+ }
+
+ // remove Trailer headers if the client connection was http/1
+ if (encoder_callbacks_->streamInfo().protocol() != Http::Protocol::Http2) {
+ response_headers_->remove(trailerHeader());
+ }
+
response_headers_->insertContentType().value().setReference(
Http::Headers::get().ContentTypeValues.Json);
+
+ response_headers_->insertContentLength().value(json_status.length());
+
Buffer::OwnedImpl status_data(json_status);
encoder_callbacks_->addEncodedData(status_data, false);
return true;
diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc
index d6995eebd2fd..66043362e43d 100644
--- a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc
+++ b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc
@@ -400,6 +400,30 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryErrorInTrailerConvertedToJson) {
R"({"code":5,"message":"Shelf 100 Not Found"})", true, true);
}
+// Streaming backend returns an error in a trailer-only response.
+TEST_P(GrpcJsonTranscoderIntegrationTest, StreamingErrorConvertedToJson) {
+ const std::string filter =
+ R"EOF(
+ name: envoy.grpc_json_transcoder
+ config:
+ proto_descriptor: "{}"
+ services: "bookstore.Bookstore"
+ convert_grpc_status: true
+ )EOF";
+ config_helper_.addFilter(
+ fmt::format(filter, TestEnvironment::runfilesPath("/test/proto/bookstore.descriptor")));
+ HttpIntegrationTest::initialize();
+ testTranscoding<bookstore::ListBooksRequest, bookstore::Shelf>(
+ 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}},
+ R"({"code":5,"message":"Shelf 37 Not Found"})");
+}
+
TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryDelete) {
HttpIntegrationTest::initialize();
testTranscoding<bookstore::DeleteBookRequest, Empty>(