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; 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 = + 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 = - 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( + 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(