From faf57cc9e30d74955920c5bed1d364990a66a72b Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Thu, 22 Aug 2019 13:55:44 +0300 Subject: [PATCH 01/10] grpc-json: add option to convert gRPC status into JSON body (#3383) When trailer indicates a gRPC error and there was no HTTP body, with the convert_grpc_status option enabled, take google.rpc.Status from the grpc-status-details-bin header and use it as a JSON body. If there was no such header, make google.rpc.Status out of the grpc-status and grpc-message headers. This also adds google.rpc.Status to user-provided protobuf descriptor. Risk Level: Small-medium Testing: Added unit and integration tests tests, tested manually. Docs Changes: Added field description in api/envoy/config/filter/http/transcoder/v2/transcoder.proto Release Notes: Signed-off-by: Anatoly Scheglov --- .../http/transcoder/v2/transcoder.proto | 8 + docs/root/intro/version_history.rst | 1 + include/envoy/http/header_map.h | 1 + source/common/grpc/BUILD | 1 + source/common/grpc/common.cc | 22 ++ source/common/grpc/common.h | 9 + source/common/http/conn_manager_impl.cc | 3 +- source/common/http/headers.h | 1 + source/common/protobuf/protobuf.h | 1 + .../json_transcoder_filter.cc | 95 +++++++- .../json_transcoder_filter.h | 18 ++ test/common/grpc/common_test.cc | 22 ++ .../grpc_json_transcoder_integration_test.cc | 216 ++++++++++++------ .../json_transcoder_filter_test.cc | 189 ++++++++++++++- test/proto/BUILD | 3 + tools/check_format.py | 5 +- 16 files changed, 515 insertions(+), 80 deletions(-) diff --git a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto index 14f54124508d..dc60055edd87 100644 --- a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto +++ b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto @@ -120,4 +120,12 @@ message GrpcJsonTranscoder { // not know them beforehand. Otherwise use ``ignored_query_parameters``. // Defaults to false. bool ignore_unknown_query_parameters = 8; + + // Whether to convert gRPC status headers to JSON. + // When trailer indicates a gRPC error and there was no HTTP body, take ``google.rpc.Status`` + // from the ``grpc-status-details-bin`` header and use it as JSON body. + // If there was no such header, make ``google.rpc.Status`` out of the ``grpc-status`` and + // ``grpc-message`` headers. + // This also adds ``google.rpc.Status`` to user-provided protobuf descriptor. + bool convert_grpc_status = 9; } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 2a049c36c740..9540a73b11bb 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -23,6 +23,7 @@ Version history * fault: added overrides for default runtime keys in :ref:`HTTPFault ` filter. * grpc: added :ref:`AWS IAM grpc credentials extension ` for AWS-managed xDS. * grpc-json: added support for :ref:`ignoring unknown query parameters`. +* grpc-json: added support for :ref:`the grpc-status-details-bin header`. * header to metadata: added :ref:`PROTOBUF_VALUE ` and :ref:`ValueEncode ` to support protobuf Value and Base64 encoding. * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`. * http: added the ability to :ref:`merge adjacent slashes` in the path. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 050b3d0b2342..ef5cbaa9b331 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -315,6 +315,7 @@ class HeaderEntry { HEADER_FUNC(GrpcAcceptEncoding) \ HEADER_FUNC(GrpcMessage) \ HEADER_FUNC(GrpcStatus) \ + HEADER_FUNC(GrpcStatusDetailsBin) \ HEADER_FUNC(GrpcTimeout) \ HEADER_FUNC(Host) \ HEADER_FUNC(KeepAlive) \ diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 603474289767..0993761f3537 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -86,6 +86,7 @@ envoy_cc_library( "//source/common/buffer:buffer_lib", "//source/common/buffer:zero_copy_input_stream_lib", "//source/common/common:assert_lib", + "//source/common/common:base64_lib", "//source/common/common:empty_string", "//source/common/common:enum_to_int", "//source/common/common:hash_lib", diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 2683044b715c..2928fbf13c5d 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -10,6 +10,7 @@ #include "common/buffer/buffer_impl.h" #include "common/buffer/zero_copy_input_stream_impl.h" #include "common/common/assert.h" +#include "common/common/base64.h" #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" #include "common/common/fmt.h" @@ -68,6 +69,27 @@ std::string Common::getGrpcMessage(const Http::HeaderMap& trailers) { return entry ? std::string(entry->value().getStringView()) : EMPTY_STRING; } +absl::optional +Common::getGrpcStatusDetailsBin(const Http::HeaderMap& trailers) { + const Http::HeaderEntry* details_header = trailers.GrpcStatusDetailsBin(); + if (!details_header) { + return {}; + } + + // Some implementations use non-padded base64 encoding for grpc-status-details-bin. + auto decoded_value = Base64::decodeWithoutPadding(details_header->value().getStringView()); + if (decoded_value.empty()) { + return {}; + } + + google::rpc::Status status; + if (!status.ParseFromString(decoded_value)) { + return {}; + } + + return {std::move(status)}; +} + Buffer::InstancePtr Common::serializeToGrpcFrame(const Protobuf::Message& message) { // http://www.grpc.io/docs/guides/wire.html // Reserve enough space for the entire message and the 5 byte header. diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index fa5fe72f7bc7..32f4fd02ee36 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -57,6 +57,15 @@ class Common { */ static std::string getGrpcMessage(const Http::HeaderMap& trailers); + /** + * Returns the decoded google.rpc.Status message from a given set of trailers, if present. + * @param trailers the trailers to parse. + * @return std::unique_ptr the gRPC status message or empty pointer if no + * grpc-status-details-bin trailer found or it was invalid. + */ + static absl::optional + getGrpcStatusDetailsBin(const Http::HeaderMap& trailers); + /** * Parse gRPC header 'grpc-timeout' value to a duration in milliseconds. * @param request_headers the header map from which to extract the value of 'grpc-timeout' header. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 8ee08cf2b988..79d1260b3601 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1501,7 +1501,8 @@ void ConnectionManagerImpl::ActiveStream::addEncodedData(ActiveStreamEncoderFilt Buffer::Instance& data, bool streaming) { if (state_.filter_call_state_ == 0 || (state_.filter_call_state_ & FilterCallState::EncodeHeaders) || - (state_.filter_call_state_ & FilterCallState::EncodeData)) { + (state_.filter_call_state_ & FilterCallState::EncodeData) || + ((state_.filter_call_state_ & FilterCallState::EncodeTrailers) && !filter.canIterate())) { // Make sure if this triggers watermarks, the correct action is taken. state_.encoder_filters_streaming_ = streaming; // If no call is happening or we are in the decode headers/data callback, buffer the data. diff --git a/source/common/http/headers.h b/source/common/http/headers.h index b711fb4b142a..718f19be595b 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -123,6 +123,7 @@ class HeaderValues { const LowerCaseString GrpcStatus{"grpc-status"}; const LowerCaseString GrpcTimeout{"grpc-timeout"}; const LowerCaseString GrpcAcceptEncoding{"grpc-accept-encoding"}; + const LowerCaseString GrpcStatusDetailsBin{"grpc-status-details-bin"}; const LowerCaseString Host{":authority"}; const LowerCaseString HostLegacy{"host"}; const LowerCaseString KeepAlive{"keep-alive"}; diff --git a/source/common/protobuf/protobuf.h b/source/common/protobuf/protobuf.h index ff6da52694f3..620f3b1c0b4d 100644 --- a/source/common/protobuf/protobuf.h +++ b/source/common/protobuf/protobuf.h @@ -7,6 +7,7 @@ #include "google/protobuf/any.pb.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" +#include "google/protobuf/descriptor_database.h" #include "google/protobuf/empty.pb.h" #include "google/protobuf/io/coded_stream.h" #include "google/protobuf/io/zero_copy_stream.h" 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 fb26f1f39c0d..63507b7bfdd5 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 @@ -111,9 +111,13 @@ JsonTranscoderConfig::JsonTranscoderConfig( } for (const auto& file : descriptor_set.file()) { - if (descriptor_pool_.BuildFile(file) == nullptr) { - throw EnvoyException("transcoding_filter: Unable to build proto descriptor pool"); - } + AddFileDescriptor(file); + } + + convert_grpc_status_ = proto_config.convert_grpc_status(); + if (convert_grpc_status_) { + AddBuiltinSymbolDescriptor("google.protobuf.Any"); + AddBuiltinSymbolDescriptor("google.rpc.Status"); } PathMatcherBuilder pmb; @@ -164,10 +168,34 @@ JsonTranscoderConfig::JsonTranscoderConfig( ignore_unknown_query_parameters_ = proto_config.ignore_unknown_query_parameters(); } +void JsonTranscoderConfig::AddFileDescriptor(const Protobuf::FileDescriptorProto& file) { + if (descriptor_pool_.BuildFile(file) == nullptr) { + throw EnvoyException("transcoding_filter: Unable to build proto descriptor pool"); + } +} + +void JsonTranscoderConfig::AddBuiltinSymbolDescriptor(const std::string& symbol_name) { + if (descriptor_pool_.FindFileContainingSymbol(symbol_name) != nullptr) { + return; + } + + auto* builtin_pool = Protobuf::DescriptorPool::generated_pool(); + if (!builtin_pool) { + return; + } + + Protobuf::DescriptorPoolDatabase pool_database(*builtin_pool); + Protobuf::FileDescriptorProto file_proto; + pool_database.FindFileContainingSymbol(symbol_name, &file_proto); + AddFileDescriptor(file_proto); +} + bool JsonTranscoderConfig::matchIncomingRequestInfo() const { return match_incoming_request_route_; } +bool JsonTranscoderConfig::convertGrpcStatus() const { return convert_grpc_status_; } + ProtobufUtil::Status JsonTranscoderConfig::createTranscoder( const Http::HeaderMap& headers, ZeroCopyInputStream& request_input, google::grpc::transcoding::TranscoderInputStream& response_input, @@ -244,6 +272,14 @@ JsonTranscoderConfig::methodToRequestInfo(const Protobuf::MethodDescriptor* meth return ProtobufUtil::Status(); } +ProtobufUtil::Status +JsonTranscoderConfig::translateProtoMessageToJson(const Protobuf::Message& message, + std::string* json_out) { + return ProtobufUtil::BinaryToJsonString( + type_helper_->Resolver(), Grpc::Common::typeUrl(message.GetDescriptor()->full_name()), + message.SerializeAsString(), json_out, print_options_); +} + JsonTranscoderFilter::JsonTranscoderFilter(JsonTranscoderConfig& config) : config_(config) {} Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& headers, @@ -385,6 +421,8 @@ Http::FilterDataStatus JsonTranscoderFilter::encodeData(Buffer::Instance& data, return Http::FilterDataStatus::Continue; } + has_body_ = true; + // TODO(dio): Add support for streaming case. if (has_http_body_output_) { buildResponseFromHttpBodyOutput(*response_headers_, data); @@ -420,6 +458,7 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& if (data.length()) { encoder_callbacks_->addEncodedData(data, true); + has_body_ = true; } if (method_->server_streaming()) { @@ -429,15 +468,19 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& 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())); - response_headers_->insertGrpcStatus().value(enumToInt(grpc_status.value())); + if (!status_converted_to_json) { + response_headers_->insertGrpcStatus().value(enumToInt(grpc_status.value())); + } } const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage(); - if (grpc_message_header) { + if (grpc_message_header && !status_converted_to_json) { response_headers_->insertGrpcMessage().value(*grpc_message_header); } @@ -496,6 +539,48 @@ void JsonTranscoderFilter::buildResponseFromHttpBodyOutput(Http::HeaderMap& resp } } +bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_status, + Http::HeaderMap& trailers) { + if (!config_.convertGrpcStatus()) { + return false; + } + + // Send a serialized status only if there was no body. + if (has_body_) { + return false; + } + + if (grpc_status == Grpc::Status::GrpcStatus::Ok || + grpc_status == Grpc::Status::GrpcStatus::InvalidCode) { + return false; + } + + auto status_details = Grpc::Common::getGrpcStatusDetailsBin(trailers); + if (!status_details) { + // If no rpc.Status object was sent in the grpc-status-details-bin header, + // construct it from the grpc-status and grpc-message headers. + status_details.emplace(); + status_details->set_code(grpc_status); + + auto grpc_message_header = trailers.GrpcMessage(); + if (grpc_message_header) { + auto message = grpc_message_header->value().getStringView(); + status_details->set_message(message.data(), message.size()); + } + } + + std::string json_status; + auto translate_status = config_.translateProtoMessageToJson(*status_details, &json_status); + if (!translate_status.ok()) { + ENVOY_LOG(debug, "Transcoding status error {}", translate_status.ToString()); + return false; + } + + Buffer::OwnedImpl status_data(json_status); + encoder_callbacks_->addEncodedData(status_data, false); + return true; +} + bool JsonTranscoderFilter::hasHttpBodyAsOutputType() { return method_->output_type()->full_name() == google::api::HttpBody::descriptor()->full_name(); } diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h index 21976ef9b760..98508ae3fe17 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h @@ -68,6 +68,12 @@ class JsonTranscoderConfig : public Logger::Loggable { std::unique_ptr& transcoder, const Protobuf::MethodDescriptor*& method_descriptor); + /** + * Converts an arbitrary protobuf message to JSON. + */ + ProtobufUtil::Status translateProtoMessageToJson(const Protobuf::Message& message, + std::string* json_out); + /** * If true, skip clearing the route cache after the incoming request has been modified. * This allows Envoy to select the upstream cluster based on the incoming request @@ -75,6 +81,12 @@ class JsonTranscoderConfig : public Logger::Loggable { */ bool matchIncomingRequestInfo() const; + /** + * If true, when trailer indicates a gRPC error and there was no HTTP body, + * make google.rpc.Status out of gRPC status headers and use it as JSON body. + */ + bool convertGrpcStatus() const; + private: /** * Convert method descriptor to RequestInfo that needed for transcoding library @@ -83,6 +95,9 @@ class JsonTranscoderConfig : public Logger::Loggable { google::grpc::transcoding::RequestInfo* info); private: + void AddFileDescriptor(const Protobuf::FileDescriptorProto& file); + void AddBuiltinSymbolDescriptor(const std::string& symbol_name); + Protobuf::DescriptorPool descriptor_pool_; google::grpc::transcoding::PathMatcherPtr path_matcher_; std::unique_ptr type_helper_; @@ -90,6 +105,7 @@ class JsonTranscoderConfig : public Logger::Loggable { bool match_incoming_request_route_{false}; bool ignore_unknown_query_parameters_{false}; + bool convert_grpc_status_{false}; }; using JsonTranscoderConfigSharedPtr = std::shared_ptr; @@ -125,6 +141,7 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< private: bool readToBuffer(Protobuf::io::ZeroCopyInputStream& stream, Buffer::Instance& data); void buildResponseFromHttpBodyOutput(Http::HeaderMap& response_headers, Buffer::Instance& data); + bool maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_status, Http::HeaderMap& trailers); bool hasHttpBodyAsOutputType(); JsonTranscoderConfig& config_; @@ -139,6 +156,7 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< bool error_{false}; bool has_http_body_output_{false}; + bool has_body_{false}; }; } // namespace GrpcJsonTranscoder diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index b2ab23f919ad..68128c7faf71 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -78,6 +78,28 @@ TEST(GrpcContextTest, GetGrpcTimeout) { // so we don't test for them. } +TEST(GrpcCommonTest, GrpcStatusDetailsBin) { + Http::TestHeaderMapImpl empty_trailers; + EXPECT_FALSE(Common::getGrpcStatusDetailsBin(empty_trailers)); + + Http::TestHeaderMapImpl invalid_value{{"grpc-status-details-bin", "invalid"}}; + EXPECT_FALSE(Common::getGrpcStatusDetailsBin(invalid_value)); + + Http::TestHeaderMapImpl unpadded_value{ + {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; + auto status = Common::getGrpcStatusDetailsBin(unpadded_value); + ASSERT_TRUE(status); + EXPECT_EQ(Status::GrpcStatus::NotFound, status->code()); + EXPECT_EQ("Resource not found", status->message()); + + Http::TestHeaderMapImpl padded_value{ + {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA=="}}; + status = Common::getGrpcStatusDetailsBin(padded_value); + ASSERT_TRUE(status); + EXPECT_EQ(Status::GrpcStatus::NotFound, status->code()); + EXPECT_EQ("Resource not found", status->message()); +} + TEST(GrpcContextTest, ToGrpcTimeout) { Http::HeaderString value; 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 14f491375a8a..c433279a17d1 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 @@ -47,89 +47,91 @@ class GrpcJsonTranscoderIntegrationTest * Global destructor for all integration tests. */ void TearDown() override { + closeConnection(); test_server_.reset(); fake_upstream_connection_.reset(); fake_upstreams_.clear(); } protected: - template - void testTranscoding(Http::HeaderMap&& request_headers, const std::string& request_body, - const std::vector& grpc_request_messages, - const std::vector& grpc_response_messages, - const Status& grpc_status, Http::HeaderMap&& response_headers, - const std::string& response_body, bool full_response = true) { + void sendRequest(Http::HeaderMap&& request_headers, const std::string& request_body) { codec_client_ = makeHttpConnection(lookupPort("http")); - IntegrationStreamDecoderPtr response; if (!request_body.empty()) { auto encoder_decoder = codec_client_->startRequest(request_headers); request_encoder_ = &encoder_decoder.first; - response = std::move(encoder_decoder.second); + response_ = std::move(encoder_decoder.second); Buffer::OwnedImpl body(request_body); codec_client_->sendData(*request_encoder_, body, true); } else { - response = codec_client_->makeHeaderOnlyRequest(request_headers); + response_ = codec_client_->makeHeaderOnlyRequest(request_headers); } ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); - if (!grpc_request_messages.empty()) { - ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); - ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); - - Grpc::Decoder grpc_decoder; - std::vector frames; - EXPECT_TRUE(grpc_decoder.decode(upstream_request_->body(), frames)); - EXPECT_EQ(grpc_request_messages.size(), frames.size()); - - for (size_t i = 0; i < grpc_request_messages.size(); ++i) { - RequestType actual_message; - if (frames[i].length_ > 0) { - EXPECT_TRUE(actual_message.ParseFromString(frames[i].data_->toString())); - } - RequestType expected_message; - EXPECT_TRUE(TextFormat::ParseFromString(grpc_request_messages[i], &expected_message)); - - EXPECT_TRUE(MessageDifferencer::Equivalent(expected_message, actual_message)); + } + + template + void expectGrpcRequest(const std::vector& grpc_request_messages) { + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + Grpc::Decoder grpc_decoder; + std::vector frames; + EXPECT_TRUE(grpc_decoder.decode(upstream_request_->body(), frames)); + EXPECT_EQ(grpc_request_messages.size(), frames.size()); + + for (size_t i = 0; i < grpc_request_messages.size(); ++i) { + RequestType actual_message; + if (frames[i].length_ > 0) { + EXPECT_TRUE(actual_message.ParseFromString(frames[i].data_->toString())); } + RequestType expected_message; + EXPECT_TRUE(TextFormat::ParseFromString(grpc_request_messages[i], &expected_message)); + + EXPECT_TRUE(MessageDifferencer::Equivalent(expected_message, actual_message)); + } + } - Http::TestHeaderMapImpl response_headers; - response_headers.insertStatus().value(200); - response_headers.insertContentType().value(std::string("application/grpc")); - if (grpc_response_messages.empty()) { - response_headers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); - response_headers.insertGrpcMessage().value(absl::string_view( - grpc_status.error_message().data(), grpc_status.error_message().size())); - upstream_request_->encodeHeaders(response_headers, true); - } else { - response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Status"); - response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Message"); - upstream_request_->encodeHeaders(response_headers, false); - for (const auto& response_message_str : grpc_response_messages) { - ResponseType response_message; - EXPECT_TRUE(TextFormat::ParseFromString(response_message_str, &response_message)); - auto buffer = Grpc::Common::serializeToGrpcFrame(response_message); - upstream_request_->encodeData(*buffer, false); - } - Http::TestHeaderMapImpl response_trailers; - response_trailers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); - response_trailers.insertGrpcMessage().value(absl::string_view( - grpc_status.error_message().data(), grpc_status.error_message().size())); - upstream_request_->encodeTrailers(response_trailers); + template + void sendGrpcResponse(const std::vector& grpc_response_messages, + const Status& grpc_status) { + Http::TestHeaderMapImpl response_headers; + response_headers.insertStatus().value(200); + response_headers.insertContentType().value(std::string("application/grpc")); + if (grpc_response_messages.empty()) { + response_headers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); + response_headers.insertGrpcMessage().value(absl::string_view( + grpc_status.error_message().data(), grpc_status.error_message().size())); + upstream_request_->encodeHeaders(response_headers, true); + } else { + response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Status"); + response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Message"); + upstream_request_->encodeHeaders(response_headers, false); + for (const auto& response_message_str : grpc_response_messages) { + ResponseType response_message; + EXPECT_TRUE(TextFormat::ParseFromString(response_message_str, &response_message)); + auto buffer = Grpc::Common::serializeToGrpcFrame(response_message); + upstream_request_->encodeData(*buffer, false); } - EXPECT_TRUE(upstream_request_->complete()); + Http::TestHeaderMapImpl response_trailers; + response_trailers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); + response_trailers.insertGrpcMessage().value(absl::string_view( + grpc_status.error_message().data(), grpc_status.error_message().size())); + upstream_request_->encodeTrailers(response_trailers); } + } - response->waitForEndStream(); - EXPECT_TRUE(response->complete()); + void expectResponseHeaders(Http::HeaderMap&& response_headers) { + response_->waitForEndStream(); + EXPECT_TRUE(response_->complete()); - if (response->headers().get(Http::LowerCaseString("transfer-encoding")) == nullptr || - !absl::StartsWith(response->headers() + if (response_->headers().get(Http::LowerCaseString("transfer-encoding")) == nullptr || + !absl::StartsWith(response_->headers() .get(Http::LowerCaseString("transfer-encoding")) ->value() .getStringView(), "chunked")) { - EXPECT_EQ(response->headers().get(Http::LowerCaseString("trailer")), nullptr); + EXPECT_EQ(response_->headers().get(Http::LowerCaseString("trailer")), nullptr); } response_headers.iterate( @@ -140,19 +142,49 @@ class GrpcJsonTranscoderIntegrationTest response->headers().get(lower_key)->value().getStringView()); return Http::HeaderMap::Iterate::Continue; }, - response.get()); + response_.get()); + } + + void expectResponseBody(const std::string& response_body, bool full_response = true) { + if (full_response) { + EXPECT_EQ(response_body, response_->body()); + } else { + EXPECT_TRUE(absl::StartsWith(response_->body(), response_body)); + } + } + + void closeConnection() { + if (response_) { + codec_client_->close(); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } + response_.reset(); + } + + template + void testTranscoding(Http::HeaderMap&& request_headers, const std::string& request_body, + const std::vector& grpc_request_messages, + const std::vector& grpc_response_messages, + const Status& grpc_status, Http::HeaderMap&& response_headers, + const std::string& response_body, bool full_response = true) { + sendRequest(std::move(request_headers), request_body); + + if (!grpc_request_messages.empty()) { + expectGrpcRequest(grpc_request_messages); + sendGrpcResponse(grpc_response_messages, grpc_status); + EXPECT_TRUE(upstream_request_->complete()); + } + + expectResponseHeaders(std::move(response_headers)); if (!response_body.empty()) { - if (full_response) { - EXPECT_EQ(response_body, response->body()); - } else { - EXPECT_TRUE(absl::StartsWith(response->body(), response_body)); - } + expectResponseBody(response_body, full_response); } - codec_client_->close(); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + closeConnection(); } + + IntegrationStreamDecoderPtr response_; }; INSTANTIATE_TEST_SUITE_P(IpVersions, GrpcJsonTranscoderIntegrationTest, @@ -345,6 +377,60 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetError1) { ""); } +TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorConvertedToJson) { + 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/100"}, {":authority", "host"}}, + "", {"shelf: 100"}, {}, Status(Code::NOT_FOUND, "Shelf 100 Not Found"), + Http::TestHeaderMapImpl{{":status", "404"}}, R"({"code":5,"message":"Shelf 100 Not Found"})"); +} + +// Test an upstream that immediately returns status 200, and then returns an error in trailer. +TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorInTrailerConvertedToJson) { + 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(); + sendRequest(Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/shelves/100"}, + {":authority", "host"}}, + ""); + expectGrpcRequest({"shelf: 100"}); + + upstream_request_->encodeHeaders( + Http::TestHeaderMapImpl{ + {":status", "200"}, + {"content-type", "application/grpc"}, + {"trailer", "rpc-status,grpc-status-details-bin"}, + }, + false); + upstream_request_->encodeTrailers(Http::TestHeaderMapImpl{ + {"grpc-status", "5"}, + {"grpc-status-details-bin", "CAUSE1NoZWxmIDEwMCBOb3QgRm91bmQ"}, + }); + EXPECT_TRUE(upstream_request_->complete()); + expectResponseHeaders(Http::TestHeaderMapImpl{{":status", "404"}}); + expectResponseBody(R"({"code":5,"message":"Shelf 100 Not Found"})"); +} + TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryDelete) { HttpIntegrationTest::initialize(); testTranscoding( diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index 673a13e0a80c..f7729cfa527b 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -312,24 +312,24 @@ TEST_F(GrpcJsonTranscoderConfigTest, InvalidVariableBinding) { class GrpcJsonTranscoderFilterTest : public testing::Test, public GrpcJsonTranscoderFilterTestBase { protected: - GrpcJsonTranscoderFilterTest(const bool match_incoming_request_route = false) - : config_(bookstoreProtoConfig(match_incoming_request_route), *api_), filter_(config_) { + GrpcJsonTranscoderFilterTest(envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder + proto_config = bookstoreProtoConfig()) + : config_(proto_config, *api_), filter_(config_) { filter_.setDecoderFilterCallbacks(decoder_callbacks_); filter_.setEncoderFilterCallbacks(encoder_callbacks_); } - const envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder - bookstoreProtoConfig(const bool match_incoming_request_route) { + static const envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder + bookstoreProtoConfig() { std::string json_string = "{\"proto_descriptor\": \"" + bookstoreDescriptorPath() + "\",\"services\": [\"bookstore.Bookstore\"]}"; auto json_config = Json::Factory::loadFromString(json_string); envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder proto_config{}; Envoy::Config::FilterJson::translateGrpcJsonTranscoder(*json_config, proto_config); - proto_config.set_match_incoming_request_route(match_incoming_request_route); return proto_config; } - const std::string bookstoreDescriptorPath() { + static const std::string bookstoreDescriptorPath() { return TestEnvironment::runfilesPath("test/proto/bookstore.descriptor"); } @@ -578,7 +578,15 @@ TEST_F(GrpcJsonTranscoderFilterTest, ForwardUnaryPostGrpc) { class GrpcJsonTranscoderFilterSkipRecalculatingTest : public GrpcJsonTranscoderFilterTest { public: - GrpcJsonTranscoderFilterSkipRecalculatingTest() : GrpcJsonTranscoderFilterTest(true) {} + GrpcJsonTranscoderFilterSkipRecalculatingTest() + : GrpcJsonTranscoderFilterTest(makeProtoConfig()) {} + +private: + const envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder makeProtoConfig() { + auto proto_config = bookstoreProtoConfig(); + proto_config.set_match_incoming_request_route(true); + return proto_config; + } }; TEST_F(GrpcJsonTranscoderFilterSkipRecalculatingTest, TranscodingUnaryPostSkipRecalculate) { @@ -738,6 +746,173 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryWithHttpBodyAsOutputAndSpli EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); } +class GrpcJsonTranscoderFilterConvertGrpcStatusTest : public GrpcJsonTranscoderFilterTest { +public: + GrpcJsonTranscoderFilterConvertGrpcStatusTest() + : GrpcJsonTranscoderFilterTest(makeProtoConfig()) {} + +private: + const envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder makeProtoConfig() { + auto proto_config = bookstoreProtoConfig(); + proto_config.set_convert_grpc_status(true); + return proto_config; + } +}; + +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusFromTextHeaders) { + Http::TestHeaderMapImpl request_headers{ + {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; + + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, + {":status", "200"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + + std::string status_data("{\"code\":5,\"message\":\"Resource not found\"}"); + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { + EXPECT_EQ(status_data, data.toString()); + })); + + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "5"}, + {"grpc-message", "Resource not found"}}; + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); +} + +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusFromBinaryHeader) { + Http::TestHeaderMapImpl request_headers{ + {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; + + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, + {":status", "200"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + + std::string status_data("{\"code\":5,\"message\":\"Resource not found\"}"); + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { + EXPECT_EQ(status_data, data.toString()); + })); + + Http::TestHeaderMapImpl response_trailers{ + {"grpc-status", "4"}, + {"grpc-message", "other message"}, + {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); +} + +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, + TranscodingStatusFromBinaryHeaderWithDetails) { + Http::TestHeaderMapImpl request_headers{ + {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; + + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, + {":status", "200"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + + // Use the helloworld package because it's not linked into this test binary. + std::string status_data("{\"code\":5,\"message\":\"Error\",\"details\":[{\"@type\":\"type." + "googleapis.com/helloworld.HelloReply\",\"message\":\"details\"}]}"); + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { + EXPECT_EQ(status_data, data.toString()); + })); + + Http::TestHeaderMapImpl response_trailers{ + {"grpc-status", "4"}, + {"grpc-message", "other message"}, + {"grpc-status-details-bin", + "CAUSBUVycm9yGjYKKXR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5IZWxsb1JlcGx5EgkKB2RldGFpbHM"}}; + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); +} + +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, SkipTranscodingStatusIfBodyPresent) { + Http::TestHeaderMapImpl request_headers{ + {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; + + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; + + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + + Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, + {":status", "200"}}; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.encodeHeaders(response_headers, false)); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + + bookstore::Shelf response; + response.set_id(20); + response.set_theme("Children"); + + auto response_data = Grpc::Common::serializeToGrpcFrame(response); + + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, + filter_.encodeData(*response_data, false)); + + std::string response_json = response_data->toString(); + + EXPECT_EQ("{\"id\":\"20\",\"theme\":\"Children\"}", response_json); + + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "2"}, {"grpc-message", "not good"}}; + + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, _)).Times(0); + + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); +} + struct GrpcJsonTranscoderFilterPrintTestParam { std::string config_json_; std::string expected_response_; diff --git a/test/proto/BUILD b/test/proto/BUILD index 08c8384dee44..71f551469cdf 100644 --- a/test/proto/BUILD +++ b/test/proto/BUILD @@ -31,6 +31,9 @@ envoy_proto_descriptor( name = "bookstore_proto_descriptor", srcs = [ "bookstore.proto", + # JSON transcoder doesn't link against ":helloworld_proto_cc", so we can add it to the + # descriptor and test that we can actually transcode types not linked into the test binary. + "helloworld.proto", ], out = "bookstore.descriptor", external_deps = [ diff --git a/tools/check_format.py b/tools/check_format.py index 67292cea223b..20a7c9269e7f 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -57,8 +57,9 @@ "./source/server/overload_manager_impl.cc") # Files in these paths can use MessageLite::SerializeAsString -SERIALIZE_AS_STRING_WHITELIST = ("./test/common/protobuf/utility_test.cc", - "./test/common/grpc/codec_test.cc") +SERIALIZE_AS_STRING_WHITELIST = ( + "./source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc", + "./test/common/protobuf/utility_test.cc", "./test/common/grpc/codec_test.cc") # Files in these paths can use Protobuf::util::JsonStringToMessage JSON_STRING_TO_MESSAGE_WHITELIST = ("./source/common/protobuf/utility.cc") From 948682e14259def2b55c7903ab9a98fd0211dae4 Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Tue, 27 Aug 2019 15:12:08 +0300 Subject: [PATCH 02/10] fix wrong content-type; remove headers; more tests Signed-off-by: Anatoly Scheglov --- .../json_transcoder_filter.cc | 22 +++++++++--- .../grpc_json_transcoder_integration_test.cc | 28 ++++++++++++--- .../json_transcoder_filter_test.cc | 34 +++++++++++++++++++ 3 files changed, 75 insertions(+), 9 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 63507b7bfdd5..7233a5ba305c 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 @@ -466,6 +466,10 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& return Http::FilterTrailersStatus::Continue; } + // If there was no previous headers frame, this |trailers| map is our |response_headers_|, + // 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); @@ -474,14 +478,22 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); } else { response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status.value())); - if (!status_converted_to_json) { + if (!status_converted_to_json && !is_trailers_only_response) { response_headers_->insertGrpcStatus().value(enumToInt(grpc_status.value())); } } - const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage(); - if (grpc_message_header && !status_converted_to_json) { - response_headers_->insertGrpcMessage().value(*grpc_message_header); + 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_->removeGrpcStatusDetailsBin(); + } else if (!status_converted_to_json && !is_trailers_only_response) { + // Copy the grpc-message header if it exists. + const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage(); + if (grpc_message_header) { + response_headers_->insertGrpcMessage().value(*grpc_message_header); + } } // remove Trailer headers if the client connection was http/1 @@ -576,6 +588,8 @@ bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_ return false; } + response_headers_->insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); 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 c433279a17d1..cf226b1c709d 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 @@ -21,6 +21,8 @@ using Envoy::ProtobufWkt::Empty; namespace Envoy { namespace { +constexpr char UnexpectedHeader[] = "Unexpected Header"; + class GrpcJsonTranscoderIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { @@ -138,8 +140,12 @@ class GrpcJsonTranscoderIntegrationTest [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { auto* response = static_cast(context); Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; - EXPECT_EQ(entry.value().getStringView(), - response->headers().get(lower_key)->value().getStringView()); + if (entry.value() == UnexpectedHeader) { + EXPECT_FALSE(response->headers().get(lower_key)); + } else { + EXPECT_EQ(entry.value().getStringView(), + response->headers().get(lower_key)->value().getStringView()); + } return Http::HeaderMap::Iterate::Continue; }, response_.get()); @@ -377,6 +383,7 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetError1) { ""); } +// Test an upstream that returns a trailer-only response. TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorConvertedToJson) { const std::string filter = R"EOF( @@ -393,7 +400,13 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorConvertedToJson) { Http::TestHeaderMapImpl{ {":method", "GET"}, {":path", "/shelves/100"}, {":authority", "host"}}, "", {"shelf: 100"}, {}, Status(Code::NOT_FOUND, "Shelf 100 Not Found"), - Http::TestHeaderMapImpl{{":status", "404"}}, R"({"code":5,"message":"Shelf 100 Not Found"})"); + Http::TestHeaderMapImpl{ + {":status", "404"}, + {"content-type", "application/json"}, + {"grpc-status", UnexpectedHeader}, + {"grpc-message", UnexpectedHeader}, + }, + R"({"code":5,"message":"Shelf 100 Not Found"})"); } // Test an upstream that immediately returns status 200, and then returns an error in trailer. @@ -419,7 +432,7 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorInTrailerConvertedToJson) Http::TestHeaderMapImpl{ {":status", "200"}, {"content-type", "application/grpc"}, - {"trailer", "rpc-status,grpc-status-details-bin"}, + {"trailer", "grpc-status,grpc-status-details-bin"}, }, false); upstream_request_->encodeTrailers(Http::TestHeaderMapImpl{ @@ -427,7 +440,12 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorInTrailerConvertedToJson) {"grpc-status-details-bin", "CAUSE1NoZWxmIDEwMCBOb3QgRm91bmQ"}, }); EXPECT_TRUE(upstream_request_->complete()); - expectResponseHeaders(Http::TestHeaderMapImpl{{":status", "404"}}); + expectResponseHeaders(Http::TestHeaderMapImpl{ + {":status", "404"}, + {"content-type", "application/json"}, + {"grpc-status", UnexpectedHeader}, + {"grpc-status-details-bin", UnexpectedHeader}, + }); expectResponseBody(R"({"code":5,"message":"Shelf 100 Not Found"})"); } diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index f7729cfa527b..726df8d11505 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -913,6 +913,40 @@ TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, SkipTranscodingStatusIfBod EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); } +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusOnEndStream) { + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + + Http::TestHeaderMapImpl request_headers{ + {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Buffer::OwnedImpl request_data{R"({"theme": "Children"})"}; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + + std::string status_data(R"({"code":5,"message":"Resource not found"})"); + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { + EXPECT_EQ(status_data, data.toString()); + })); + + Http::TestHeaderMapImpl response_headers{ + {":status", "200"}, + {"content-type", "application/grpc"}, + {"grpc-status", "5"}, + {"grpc-message", "unused"}, + {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); + EXPECT_EQ("404", response_headers.get_(":status")); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + EXPECT_FALSE(response_headers.has("grpc-status")); + EXPECT_FALSE(response_headers.has("grpc-message")); + EXPECT_FALSE(response_headers.has("grpc-status-details-bin")); +} + struct GrpcJsonTranscoderFilterPrintTestParam { std::string config_json_; std::string expected_response_; From e64ae6a715c8604a14cc509335d8d87ad5c884f9 Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Tue, 27 Aug 2019 22:42:51 +0300 Subject: [PATCH 03/10] renames according to CC; nullopt Signed-off-by: Anatoly Scheglov --- source/common/grpc/common.cc | 8 ++++---- .../grpc_json_transcoder/json_transcoder_filter.cc | 12 ++++++------ .../grpc_json_transcoder/json_transcoder_filter.h | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 2928fbf13c5d..209e682b2a64 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -55,7 +55,7 @@ absl::optional Common::getGrpcStatus(const Http::HeaderMap& uint64_t grpc_status_code; if (!grpc_status_header || grpc_status_header->value().empty()) { - return {}; + return absl::nullopt; } if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) || grpc_status_code > Status::GrpcStatus::MaximumValid) { @@ -73,18 +73,18 @@ absl::optional Common::getGrpcStatusDetailsBin(const Http::HeaderMap& trailers) { const Http::HeaderEntry* details_header = trailers.GrpcStatusDetailsBin(); if (!details_header) { - return {}; + return absl::nullopt; } // Some implementations use non-padded base64 encoding for grpc-status-details-bin. auto decoded_value = Base64::decodeWithoutPadding(details_header->value().getStringView()); if (decoded_value.empty()) { - return {}; + return absl::nullopt; } google::rpc::Status status; if (!status.ParseFromString(decoded_value)) { - return {}; + return absl::nullopt; } return {std::move(status)}; 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 7233a5ba305c..d8ca667039f5 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 @@ -111,13 +111,13 @@ JsonTranscoderConfig::JsonTranscoderConfig( } for (const auto& file : descriptor_set.file()) { - AddFileDescriptor(file); + addFileDescriptor(file); } convert_grpc_status_ = proto_config.convert_grpc_status(); if (convert_grpc_status_) { - AddBuiltinSymbolDescriptor("google.protobuf.Any"); - AddBuiltinSymbolDescriptor("google.rpc.Status"); + addBuiltinSymbolDescriptor("google.protobuf.Any"); + addBuiltinSymbolDescriptor("google.rpc.Status"); } PathMatcherBuilder pmb; @@ -168,13 +168,13 @@ JsonTranscoderConfig::JsonTranscoderConfig( ignore_unknown_query_parameters_ = proto_config.ignore_unknown_query_parameters(); } -void JsonTranscoderConfig::AddFileDescriptor(const Protobuf::FileDescriptorProto& file) { +void JsonTranscoderConfig::addFileDescriptor(const Protobuf::FileDescriptorProto& file) { if (descriptor_pool_.BuildFile(file) == nullptr) { throw EnvoyException("transcoding_filter: Unable to build proto descriptor pool"); } } -void JsonTranscoderConfig::AddBuiltinSymbolDescriptor(const std::string& symbol_name) { +void JsonTranscoderConfig::addBuiltinSymbolDescriptor(const std::string& symbol_name) { if (descriptor_pool_.FindFileContainingSymbol(symbol_name) != nullptr) { return; } @@ -187,7 +187,7 @@ void JsonTranscoderConfig::AddBuiltinSymbolDescriptor(const std::string& symbol_ Protobuf::DescriptorPoolDatabase pool_database(*builtin_pool); Protobuf::FileDescriptorProto file_proto; pool_database.FindFileContainingSymbol(symbol_name, &file_proto); - AddFileDescriptor(file_proto); + addFileDescriptor(file_proto); } bool JsonTranscoderConfig::matchIncomingRequestInfo() const { diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h index 98508ae3fe17..a3bae5e4e027 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h @@ -95,8 +95,8 @@ class JsonTranscoderConfig : public Logger::Loggable { google::grpc::transcoding::RequestInfo* info); private: - void AddFileDescriptor(const Protobuf::FileDescriptorProto& file); - void AddBuiltinSymbolDescriptor(const std::string& symbol_name); + void addFileDescriptor(const Protobuf::FileDescriptorProto& file); + void addBuiltinSymbolDescriptor(const std::string& symbol_name); Protobuf::DescriptorPool descriptor_pool_; google::grpc::transcoding::PathMatcherPtr path_matcher_; From 039e5bf49367d9a47d2575e0cffcf1cf2a6e3086 Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Wed, 28 Aug 2019 00:55:31 +0300 Subject: [PATCH 04/10] refactor tests Signed-off-by: Anatoly Scheglov --- .../grpc_json_transcoder_integration_test.cc | 40 ++-- .../json_transcoder_filter_test.cc | 220 +++++++----------- 2 files changed, 98 insertions(+), 162 deletions(-) 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 cf226b1c709d..12f5f0d1f0da 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 @@ -383,8 +383,8 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetError1) { ""); } -// Test an upstream that returns a trailer-only response. -TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorConvertedToJson) { +// Test an upstream that returns an error in a trailer-only response. +TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryErrorConvertedToJson) { const std::string filter = R"EOF( name: envoy.grpc_json_transcoder @@ -400,17 +400,15 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorConvertedToJson) { Http::TestHeaderMapImpl{ {":method", "GET"}, {":path", "/shelves/100"}, {":authority", "host"}}, "", {"shelf: 100"}, {}, Status(Code::NOT_FOUND, "Shelf 100 Not Found"), - Http::TestHeaderMapImpl{ - {":status", "404"}, - {"content-type", "application/json"}, - {"grpc-status", UnexpectedHeader}, - {"grpc-message", UnexpectedHeader}, - }, + Http::TestHeaderMapImpl{{":status", "404"}, + {"content-type", "application/json"}, + {"grpc-status", UnexpectedHeader}, + {"grpc-message", UnexpectedHeader}}, R"({"code":5,"message":"Shelf 100 Not Found"})"); } -// Test an upstream that immediately returns status 200, and then returns an error in trailer. -TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorInTrailerConvertedToJson) { +// Upstream sends headers (e.g. sends metadata), and then sends trailer with an error. +TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryErrorInTrailerConvertedToJson) { const std::string filter = R"EOF( name: envoy.grpc_json_transcoder @@ -429,23 +427,17 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryGetErrorInTrailerConvertedToJson) expectGrpcRequest({"shelf: 100"}); upstream_request_->encodeHeaders( - Http::TestHeaderMapImpl{ - {":status", "200"}, - {"content-type", "application/grpc"}, - {"trailer", "grpc-status,grpc-status-details-bin"}, - }, + Http::TestHeaderMapImpl{{":status", "200"}, + {"content-type", "application/grpc"}, + {"trailer", "grpc-status,grpc-status-details-bin"}}, false); upstream_request_->encodeTrailers(Http::TestHeaderMapImpl{ - {"grpc-status", "5"}, - {"grpc-status-details-bin", "CAUSE1NoZWxmIDEwMCBOb3QgRm91bmQ"}, - }); + {"grpc-status", "5"}, {"grpc-status-details-bin", "CAUSE1NoZWxmIDEwMCBOb3QgRm91bmQ"}}); EXPECT_TRUE(upstream_request_->complete()); - expectResponseHeaders(Http::TestHeaderMapImpl{ - {":status", "404"}, - {"content-type", "application/json"}, - {"grpc-status", UnexpectedHeader}, - {"grpc-status-details-bin", UnexpectedHeader}, - }); + expectResponseHeaders(Http::TestHeaderMapImpl{{":status", "404"}, + {"content-type", "application/json"}, + {"grpc-status", UnexpectedHeader}, + {"grpc-status-details-bin", UnexpectedHeader}}); expectResponseBody(R"({"code":5,"message":"Shelf 100 Not Found"})"); } diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index 726df8d11505..9e626e5971eb 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -751,6 +751,20 @@ class GrpcJsonTranscoderFilterConvertGrpcStatusTest : public GrpcJsonTranscoderF GrpcJsonTranscoderFilterConvertGrpcStatusTest() : GrpcJsonTranscoderFilterTest(makeProtoConfig()) {} + void SetUp() override { + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + Http::TestHeaderMapImpl request_headers{ + {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); + + Buffer::OwnedImpl request_data{R"({"theme": "Children"})"}; + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + + Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.encode100ContinueHeaders(continue_headers)); + } + private: const envoy::config::filter::http::transcoder::v2::GrpcJsonTranscoder makeProtoConfig() { auto proto_config = bookstoreProtoConfig(); @@ -759,93 +773,73 @@ class GrpcJsonTranscoderFilterConvertGrpcStatusTest : public GrpcJsonTranscoderF } }; -TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusFromTextHeaders) { - Http::TestHeaderMapImpl request_headers{ - {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; - - EXPECT_CALL(decoder_callbacks_, clearRouteCache()); - - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - - Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; - - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); - - Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_.encode100ContinueHeaders(continue_headers)); - - Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, - {":status", "200"}}; - - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_.encodeHeaders(response_headers, false)); - EXPECT_EQ("application/json", response_headers.get_("content-type")); - - std::string status_data("{\"code\":5,\"message\":\"Resource not found\"}"); +// Single headers frame with end_stream flag (trailer), no grpc-status-details-bin header. +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingTextHeadersInTrailerOnlyResponse) { + std::string expected_response(R"({"code":5,"message":"Resource not found"})"); EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) - .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { - EXPECT_EQ(status_data, data.toString()); + .WillOnce(Invoke([&expected_response](Buffer::Instance& data, bool) { + EXPECT_EQ(expected_response, data.toString()); })); - Http::TestHeaderMapImpl response_trailers{{"grpc-status", "5"}, - {"grpc-message", "Resource not found"}}; - - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); -} - -TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusFromBinaryHeader) { - Http::TestHeaderMapImpl request_headers{ - {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; - - EXPECT_CALL(decoder_callbacks_, clearRouteCache()); - - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - - Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); - - Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_.encode100ContinueHeaders(continue_headers)); - - Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, - {":status", "200"}}; - - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_.encodeHeaders(response_headers, false)); + Http::TestHeaderMapImpl response_headers{{":status", "200"}, + {"content-type", "application/grpc"}, + {"grpc-status", "5"}, + {"grpc-message", "Resource not found"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); + EXPECT_EQ("404", response_headers.get_(":status")); EXPECT_EQ("application/json", response_headers.get_("content-type")); + EXPECT_FALSE(response_headers.has("grpc-status")); + EXPECT_FALSE(response_headers.has("grpc-message")); +} - std::string status_data("{\"code\":5,\"message\":\"Resource not found\"}"); +// Trailer-only response with grpc-status-details-bin header. +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, + TranscodingBinaryHeaderInTrailerOnlyResponse) { + std::string expected_response(R"({"code":5,"message":"Resource not found"})"); EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) - .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { - EXPECT_EQ(status_data, data.toString()); + .WillOnce(Invoke([&expected_response](Buffer::Instance& data, bool) { + EXPECT_EQ(expected_response, data.toString()); })); - Http::TestHeaderMapImpl response_trailers{ - {"grpc-status", "4"}, - {"grpc-message", "other message"}, + Http::TestHeaderMapImpl response_headers{ + {":status", "200"}, + {"content-type", "application/grpc"}, + {"grpc-status", "5"}, + {"grpc-message", "unused"}, {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; - - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); + EXPECT_EQ("404", response_headers.get_(":status")); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + EXPECT_FALSE(response_headers.has("grpc-status")); + EXPECT_FALSE(response_headers.has("grpc-message")); + EXPECT_FALSE(response_headers.has("grpc-status-details-bin")); } +// Trailer-only response with grpc-status-details-bin header with details. +// Also tests that a user-defined type from proto descriptor in cofing can be used in details. TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, - TranscodingStatusFromBinaryHeaderWithDetails) { - Http::TestHeaderMapImpl request_headers{ - {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; - - EXPECT_CALL(decoder_callbacks_, clearRouteCache()); - - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - - Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); + TranscodingBinaryHeaderWithDetailsInTrailerOnlyResponse) { + std::string expected_response( + "{\"code\":5,\"message\":\"Error\",\"details\":" + "[{\"@type\":\"type.googleapis.com/helloworld.HelloReply\",\"message\":\"details\"}]}"); + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&expected_response](Buffer::Instance& data, bool) { + EXPECT_EQ(expected_response, data.toString()); + })); - Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_.encode100ContinueHeaders(continue_headers)); + Http::TestHeaderMapImpl response_headers{ + {":status", "200"}, + {"content-type", "application/grpc"}, + {"grpc-status", "5"}, + {"grpc-message", "unused"}, + {"grpc-status-details-bin", + "CAUSBUVycm9yGjYKKXR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5IZWxsb1JlcGx5EgkKB2RldGFpbHM"}}; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); +} +// Response with a header frame and a trailer frame. +// (E.g. a gRPC server sends metadata and then it sends an error.) +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusFromTrailer) { Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, {":status", "200"}}; @@ -853,39 +847,26 @@ TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, filter_.encodeHeaders(response_headers, false)); EXPECT_EQ("application/json", response_headers.get_("content-type")); - // Use the helloworld package because it's not linked into this test binary. - std::string status_data("{\"code\":5,\"message\":\"Error\",\"details\":[{\"@type\":\"type." - "googleapis.com/helloworld.HelloReply\",\"message\":\"details\"}]}"); + std::string expected_response(R"({"code":5,"message":"Resource not found"})"); EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) - .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { - EXPECT_EQ(status_data, data.toString()); + .WillOnce(Invoke([&expected_response](Buffer::Instance& data, bool) { + EXPECT_EQ(expected_response, data.toString()); })); Http::TestHeaderMapImpl response_trailers{ - {"grpc-status", "4"}, - {"grpc-message", "other message"}, - {"grpc-status-details-bin", - "CAUSBUVycm9yGjYKKXR5cGUuZ29vZ2xlYXBpcy5jb20vaGVsbG93b3JsZC5IZWxsb1JlcGx5EgkKB2RldGFpbHM"}}; - + {"grpc-status", "5"}, + {"grpc-message", "unused"}, + {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); + EXPECT_EQ("404", response_headers.get_(":status")); + EXPECT_EQ("application/json", response_headers.get_("content-type")); + EXPECT_FALSE(response_headers.has("grpc-status")); + EXPECT_FALSE(response_headers.has("grpc-message")); + EXPECT_FALSE(response_headers.has("grpc-status-details-bin")); } -TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, SkipTranscodingStatusIfBodyPresent) { - Http::TestHeaderMapImpl request_headers{ - {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; - - EXPECT_CALL(decoder_callbacks_, clearRouteCache()); - - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - - Buffer::OwnedImpl request_data{"{\"theme\": \"Children\"}"}; - - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); - - Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_.encode100ContinueHeaders(continue_headers)); - +// Server sends a response body, don't replace it. +TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, SkipTranscodingStatusIfBodyIsPresent) { Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, {":status", "200"}}; @@ -898,55 +879,18 @@ TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, SkipTranscodingStatusIfBod response.set_theme("Children"); auto response_data = Grpc::Common::serializeToGrpcFrame(response); - EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_.encodeData(*response_data, false)); std::string response_json = response_data->toString(); - - EXPECT_EQ("{\"id\":\"20\",\"theme\":\"Children\"}", response_json); - - Http::TestHeaderMapImpl response_trailers{{"grpc-status", "2"}, {"grpc-message", "not good"}}; + EXPECT_EQ(R"({"id":"20","theme":"Children"})", response_json); EXPECT_CALL(encoder_callbacks_, addEncodedData(_, _)).Times(0); + Http::TestHeaderMapImpl response_trailers{{"grpc-status", "2"}, {"grpc-message", "not good"}}; EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); } -TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusOnEndStream) { - EXPECT_CALL(decoder_callbacks_, clearRouteCache()); - - Http::TestHeaderMapImpl request_headers{ - {"content-type", "application/json"}, {":method", "POST"}, {":path", "/shelf"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false)); - - Buffer::OwnedImpl request_data{R"({"theme": "Children"})"}; - EXPECT_EQ(Http::FilterDataStatus::Continue, filter_.decodeData(request_data, true)); - - Http::TestHeaderMapImpl continue_headers{{":status", "000"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_.encode100ContinueHeaders(continue_headers)); - - std::string status_data(R"({"code":5,"message":"Resource not found"})"); - EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) - .WillOnce(Invoke([&status_data](Buffer::Instance& data, bool) { - EXPECT_EQ(status_data, data.toString()); - })); - - Http::TestHeaderMapImpl response_headers{ - {":status", "200"}, - {"content-type", "application/grpc"}, - {"grpc-status", "5"}, - {"grpc-message", "unused"}, - {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); - EXPECT_EQ("404", response_headers.get_(":status")); - EXPECT_EQ("application/json", response_headers.get_("content-type")); - EXPECT_FALSE(response_headers.has("grpc-status")); - EXPECT_FALSE(response_headers.has("grpc-message")); - EXPECT_FALSE(response_headers.has("grpc-status-details-bin")); -} - struct GrpcJsonTranscoderFilterPrintTestParam { std::string config_json_; std::string expected_response_; From b9ec933c97689a2dd1a476a6231e498fe00141fd Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Wed, 28 Aug 2019 13:03:37 +0300 Subject: [PATCH 05/10] Remove support for errors in separate trailer frames. With this only trailer-only responses are supported. This should be reverted when HCM can buffer data added from |encodeTrailers|. Signed-off-by: Anatoly Scheglov --- source/common/http/conn_manager_impl.cc | 3 +- .../json_transcoder_filter.cc | 6 + .../grpc_json_transcoder_integration_test.cc | 205 ++++++------------ .../json_transcoder_filter_test.cc | 54 ----- 4 files changed, 77 insertions(+), 191 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 79d1260b3601..8ee08cf2b988 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1501,8 +1501,7 @@ void ConnectionManagerImpl::ActiveStream::addEncodedData(ActiveStreamEncoderFilt Buffer::Instance& data, bool streaming) { if (state_.filter_call_state_ == 0 || (state_.filter_call_state_ & FilterCallState::EncodeHeaders) || - (state_.filter_call_state_ & FilterCallState::EncodeData) || - ((state_.filter_call_state_ & FilterCallState::EncodeTrailers) && !filter.canIterate())) { + (state_.filter_call_state_ & FilterCallState::EncodeData)) { // Make sure if this triggers watermarks, the correct action is taken. state_.encoder_filters_streaming_ = streaming; // If no call is happening or we are in the decode headers/data callback, buffer the data. 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 d8ca667039f5..726cc93c5d83 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 @@ -557,6 +557,12 @@ bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_ return false; } + // We do not support responses with a separate trailer frame. + // TODO(ascheglov): remove this if after HCM can buffer data added from |encodeTrailers|. + if (response_headers_ != &trailers) { + return false; + } + // Send a serialized status only if there was no body. if (has_body_) { return false; 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 12f5f0d1f0da..2d4d4c8c2666 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 @@ -21,7 +21,8 @@ using Envoy::ProtobufWkt::Empty; namespace Envoy { namespace { -constexpr char UnexpectedHeader[] = "Unexpected Header"; +// A magic header value which marks header as not expected. +constexpr char UnexpectedHeaderValue[] = "Unexpected header value"; class GrpcJsonTranscoderIntegrationTest : public testing::TestWithParam, @@ -49,98 +50,96 @@ class GrpcJsonTranscoderIntegrationTest * Global destructor for all integration tests. */ void TearDown() override { - closeConnection(); test_server_.reset(); fake_upstream_connection_.reset(); fake_upstreams_.clear(); } protected: - void sendRequest(Http::HeaderMap&& request_headers, const std::string& request_body) { + template + void testTranscoding(Http::HeaderMap&& request_headers, const std::string& request_body, + const std::vector& grpc_request_messages, + const std::vector& grpc_response_messages, + const Status& grpc_status, Http::HeaderMap&& response_headers, + const std::string& response_body, bool full_response = true) { codec_client_ = makeHttpConnection(lookupPort("http")); + IntegrationStreamDecoderPtr response; if (!request_body.empty()) { auto encoder_decoder = codec_client_->startRequest(request_headers); request_encoder_ = &encoder_decoder.first; - response_ = std::move(encoder_decoder.second); + response = std::move(encoder_decoder.second); Buffer::OwnedImpl body(request_body); codec_client_->sendData(*request_encoder_, body, true); } else { - response_ = codec_client_->makeHeaderOnlyRequest(request_headers); + response = codec_client_->makeHeaderOnlyRequest(request_headers); } ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); - } - - template - void expectGrpcRequest(const std::vector& grpc_request_messages) { - ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); - ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); - - Grpc::Decoder grpc_decoder; - std::vector frames; - EXPECT_TRUE(grpc_decoder.decode(upstream_request_->body(), frames)); - EXPECT_EQ(grpc_request_messages.size(), frames.size()); - - for (size_t i = 0; i < grpc_request_messages.size(); ++i) { - RequestType actual_message; - if (frames[i].length_ > 0) { - EXPECT_TRUE(actual_message.ParseFromString(frames[i].data_->toString())); + if (!grpc_request_messages.empty()) { + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + Grpc::Decoder grpc_decoder; + std::vector frames; + EXPECT_TRUE(grpc_decoder.decode(upstream_request_->body(), frames)); + EXPECT_EQ(grpc_request_messages.size(), frames.size()); + + for (size_t i = 0; i < grpc_request_messages.size(); ++i) { + RequestType actual_message; + if (frames[i].length_ > 0) { + EXPECT_TRUE(actual_message.ParseFromString(frames[i].data_->toString())); + } + RequestType expected_message; + EXPECT_TRUE(TextFormat::ParseFromString(grpc_request_messages[i], &expected_message)); + + EXPECT_TRUE(MessageDifferencer::Equivalent(expected_message, actual_message)); } - RequestType expected_message; - EXPECT_TRUE(TextFormat::ParseFromString(grpc_request_messages[i], &expected_message)); - - EXPECT_TRUE(MessageDifferencer::Equivalent(expected_message, actual_message)); - } - } - template - void sendGrpcResponse(const std::vector& grpc_response_messages, - const Status& grpc_status) { - Http::TestHeaderMapImpl response_headers; - response_headers.insertStatus().value(200); - response_headers.insertContentType().value(std::string("application/grpc")); - if (grpc_response_messages.empty()) { - response_headers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); - response_headers.insertGrpcMessage().value(absl::string_view( - grpc_status.error_message().data(), grpc_status.error_message().size())); - upstream_request_->encodeHeaders(response_headers, true); - } else { - response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Status"); - response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Message"); - upstream_request_->encodeHeaders(response_headers, false); - for (const auto& response_message_str : grpc_response_messages) { - ResponseType response_message; - EXPECT_TRUE(TextFormat::ParseFromString(response_message_str, &response_message)); - auto buffer = Grpc::Common::serializeToGrpcFrame(response_message); - upstream_request_->encodeData(*buffer, false); + Http::TestHeaderMapImpl response_headers; + response_headers.insertStatus().value(200); + response_headers.insertContentType().value(std::string("application/grpc")); + if (grpc_response_messages.empty()) { + response_headers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); + response_headers.insertGrpcMessage().value(absl::string_view( + grpc_status.error_message().data(), grpc_status.error_message().size())); + upstream_request_->encodeHeaders(response_headers, true); + } else { + response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Status"); + response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Message"); + upstream_request_->encodeHeaders(response_headers, false); + for (const auto& response_message_str : grpc_response_messages) { + ResponseType response_message; + EXPECT_TRUE(TextFormat::ParseFromString(response_message_str, &response_message)); + auto buffer = Grpc::Common::serializeToGrpcFrame(response_message); + upstream_request_->encodeData(*buffer, false); + } + Http::TestHeaderMapImpl response_trailers; + response_trailers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); + response_trailers.insertGrpcMessage().value(absl::string_view( + grpc_status.error_message().data(), grpc_status.error_message().size())); + upstream_request_->encodeTrailers(response_trailers); } - Http::TestHeaderMapImpl response_trailers; - response_trailers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); - response_trailers.insertGrpcMessage().value(absl::string_view( - grpc_status.error_message().data(), grpc_status.error_message().size())); - upstream_request_->encodeTrailers(response_trailers); + EXPECT_TRUE(upstream_request_->complete()); } - } - void expectResponseHeaders(Http::HeaderMap&& response_headers) { - response_->waitForEndStream(); - EXPECT_TRUE(response_->complete()); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); - if (response_->headers().get(Http::LowerCaseString("transfer-encoding")) == nullptr || - !absl::StartsWith(response_->headers() + if (response->headers().get(Http::LowerCaseString("transfer-encoding")) == nullptr || + !absl::StartsWith(response->headers() .get(Http::LowerCaseString("transfer-encoding")) ->value() .getStringView(), "chunked")) { - EXPECT_EQ(response_->headers().get(Http::LowerCaseString("trailer")), nullptr); + EXPECT_EQ(response->headers().get(Http::LowerCaseString("trailer")), nullptr); } response_headers.iterate( [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { auto* response = static_cast(context); Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; - if (entry.value() == UnexpectedHeader) { + if (entry.value() == UnexpectedHeaderValue) { EXPECT_FALSE(response->headers().get(lower_key)); } else { EXPECT_EQ(entry.value().getStringView(), @@ -148,49 +147,19 @@ class GrpcJsonTranscoderIntegrationTest } return Http::HeaderMap::Iterate::Continue; }, - response_.get()); - } - - void expectResponseBody(const std::string& response_body, bool full_response = true) { - if (full_response) { - EXPECT_EQ(response_body, response_->body()); - } else { - EXPECT_TRUE(absl::StartsWith(response_->body(), response_body)); - } - } - - void closeConnection() { - if (response_) { - codec_client_->close(); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } - response_.reset(); - } - - template - void testTranscoding(Http::HeaderMap&& request_headers, const std::string& request_body, - const std::vector& grpc_request_messages, - const std::vector& grpc_response_messages, - const Status& grpc_status, Http::HeaderMap&& response_headers, - const std::string& response_body, bool full_response = true) { - sendRequest(std::move(request_headers), request_body); - - if (!grpc_request_messages.empty()) { - expectGrpcRequest(grpc_request_messages); - sendGrpcResponse(grpc_response_messages, grpc_status); - EXPECT_TRUE(upstream_request_->complete()); - } - - expectResponseHeaders(std::move(response_headers)); + response.get()); if (!response_body.empty()) { - expectResponseBody(response_body, full_response); + if (full_response) { + EXPECT_EQ(response_body, response->body()); + } else { + EXPECT_TRUE(absl::StartsWith(response->body(), response_body)); + } } - closeConnection(); + codec_client_->close(); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); } - - IntegrationStreamDecoderPtr response_; }; INSTANTIATE_TEST_SUITE_P(IpVersions, GrpcJsonTranscoderIntegrationTest, @@ -402,45 +371,11 @@ TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryErrorConvertedToJson) { "", {"shelf: 100"}, {}, Status(Code::NOT_FOUND, "Shelf 100 Not Found"), Http::TestHeaderMapImpl{{":status", "404"}, {"content-type", "application/json"}, - {"grpc-status", UnexpectedHeader}, - {"grpc-message", UnexpectedHeader}}, + {"grpc-status", UnexpectedHeaderValue}, + {"grpc-message", UnexpectedHeaderValue}}, R"({"code":5,"message":"Shelf 100 Not Found"})"); } -// Upstream sends headers (e.g. sends metadata), and then sends trailer with an error. -TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryErrorInTrailerConvertedToJson) { - 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(); - sendRequest(Http::TestHeaderMapImpl{{":method", "GET"}, - {":path", "/shelves/100"}, - {":authority", "host"}}, - ""); - expectGrpcRequest({"shelf: 100"}); - - upstream_request_->encodeHeaders( - Http::TestHeaderMapImpl{{":status", "200"}, - {"content-type", "application/grpc"}, - {"trailer", "grpc-status,grpc-status-details-bin"}}, - false); - upstream_request_->encodeTrailers(Http::TestHeaderMapImpl{ - {"grpc-status", "5"}, {"grpc-status-details-bin", "CAUSE1NoZWxmIDEwMCBOb3QgRm91bmQ"}}); - EXPECT_TRUE(upstream_request_->complete()); - expectResponseHeaders(Http::TestHeaderMapImpl{{":status", "404"}, - {"content-type", "application/json"}, - {"grpc-status", UnexpectedHeader}, - {"grpc-status-details-bin", UnexpectedHeader}}); - expectResponseBody(R"({"code":5,"message":"Shelf 100 Not Found"})"); -} - TEST_P(GrpcJsonTranscoderIntegrationTest, UnaryDelete) { HttpIntegrationTest::initialize(); testTranscoding( diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index 9e626e5971eb..35b83872a84c 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -837,60 +837,6 @@ TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.encodeHeaders(response_headers, true)); } -// Response with a header frame and a trailer frame. -// (E.g. a gRPC server sends metadata and then it sends an error.) -TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingStatusFromTrailer) { - Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, - {":status", "200"}}; - - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_.encodeHeaders(response_headers, false)); - EXPECT_EQ("application/json", response_headers.get_("content-type")); - - std::string expected_response(R"({"code":5,"message":"Resource not found"})"); - EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) - .WillOnce(Invoke([&expected_response](Buffer::Instance& data, bool) { - EXPECT_EQ(expected_response, data.toString()); - })); - - Http::TestHeaderMapImpl response_trailers{ - {"grpc-status", "5"}, - {"grpc-message", "unused"}, - {"grpc-status-details-bin", "CAUSElJlc291cmNlIG5vdCBmb3VuZA"}}; - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers)); - EXPECT_EQ("404", response_headers.get_(":status")); - EXPECT_EQ("application/json", response_headers.get_("content-type")); - EXPECT_FALSE(response_headers.has("grpc-status")); - EXPECT_FALSE(response_headers.has("grpc-message")); - EXPECT_FALSE(response_headers.has("grpc-status-details-bin")); -} - -// Server sends a response body, don't replace it. -TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, SkipTranscodingStatusIfBodyIsPresent) { - Http::TestHeaderMapImpl response_headers{{"content-type", "application/grpc"}, - {":status", "200"}}; - - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_.encodeHeaders(response_headers, false)); - EXPECT_EQ("application/json", response_headers.get_("content-type")); - - bookstore::Shelf response; - response.set_id(20); - response.set_theme("Children"); - - auto response_data = Grpc::Common::serializeToGrpcFrame(response); - EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, - filter_.encodeData(*response_data, false)); - - std::string response_json = response_data->toString(); - EXPECT_EQ(R"({"id":"20","theme":"Children"})", response_json); - - EXPECT_CALL(encoder_callbacks_, addEncodedData(_, _)).Times(0); - - Http::TestHeaderMapImpl response_trailers{{"grpc-status", "2"}, {"grpc-message", "not good"}}; - EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.decodeTrailers(response_trailers)); -} - struct GrpcJsonTranscoderFilterPrintTestParam { std::string config_json_; std::string expected_response_; From b00edd06daa824190f85cbc8cc1dc6fed3268260 Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Wed, 28 Aug 2019 13:14:18 +0300 Subject: [PATCH 06/10] fix a typo Signed-off-by: Anatoly Scheglov --- .../http/grpc_json_transcoder/json_transcoder_filter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc index 35b83872a84c..6bf9411f318b 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc @@ -816,7 +816,7 @@ TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, } // Trailer-only response with grpc-status-details-bin header with details. -// Also tests that a user-defined type from proto descriptor in cofing can be used in details. +// Also tests that a user-defined type from a proto descriptor in config can be used in details. TEST_F(GrpcJsonTranscoderFilterConvertGrpcStatusTest, TranscodingBinaryHeaderWithDetailsInTrailerOnlyResponse) { std::string expected_response( From db62f473bd1d4b2b5fe3407cbb5f1ebc90be47c5 Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Fri, 30 Aug 2019 14:22:50 +0300 Subject: [PATCH 07/10] docs on details types Signed-off-by: Anatoly Scheglov --- .../filter/http/transcoder/v2/transcoder.proto | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto index dc60055edd87..dc492085cf62 100644 --- a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto +++ b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto @@ -126,6 +126,19 @@ message GrpcJsonTranscoder { // from the ``grpc-status-details-bin`` header and use it as JSON body. // If there was no such header, make ``google.rpc.Status`` out of the ``grpc-status`` and // ``grpc-message`` headers. - // This also adds ``google.rpc.Status`` to user-provided protobuf descriptor. + // The error details types must be present in the ``proto_descriptor``. + // + // Example: + // + // .. code-block:: proto + // + // import "google/rpc/error_details.proto"; + // + // message Dummy { + // google.rpc.RequestInfo request_info = 1; + // } + // + // If a gRPC service uses the types from the ``google/rpc/error_details.proto``, its proto files + // should reference at least one of those types. bool convert_grpc_status = 9; } From c81ae67b05f18c347636a7f5c154d6cbf7b9bee0 Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Wed, 11 Sep 2019 13:51:32 +0300 Subject: [PATCH 08/10] example of transcoded grpc-status-details-bin header Signed-off-by: Anatoly Scheglov --- .../http/transcoder/v2/transcoder.proto | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto index dc492085cf62..28beed928f58 100644 --- a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto +++ b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto @@ -128,17 +128,25 @@ message GrpcJsonTranscoder { // ``grpc-message`` headers. // The error details types must be present in the ``proto_descriptor``. // - // Example: + // For example, if an upstream server replies with headers: // - // .. code-block:: proto + // .. code-block:: none // - // import "google/rpc/error_details.proto"; + // grpc-status: 5 + // grpc-status-details-bin: CAUaMwoqdHlwZS5nb29nbGVhcGlzLmNvbS9nb29nbGUucnBjLlJlcXVlc3RJbmZvEgUKA3ItMQ // - // message Dummy { - // google.rpc.RequestInfo request_info = 1; - // } + // The ``grpc-status-details-bin`` header contains a base64-encoded protobuf message + // ``google.rpc.Status``. It will be transcoded into: + // + // .. code-block:: none + // + // HTTP/1.1 404 Not Found + // content-type: application/json + // + // {"code":5,"details":[{"@type":"type.googleapis.com/google.rpc.RequestInfo","requestId":"r-1"}]} // - // If a gRPC service uses the types from the ``google/rpc/error_details.proto``, its proto files - // should reference at least one of those types. + // In order to transcode the message, the ``google.rpc.RequestInfo`` type from + // the ``google/rpc/error_details.proto`` should be included in the configured + // :ref:`proto descriptor set `. bool convert_grpc_status = 9; } From 76084703d0c6f44356d3b33a540b928acd3bd0bf Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Thu, 12 Sep 2019 13:24:16 +0300 Subject: [PATCH 09/10] remove GrpcStatusDetailsBin from the header_map.h Signed-off-by: Anatoly Scheglov --- include/envoy/http/header_map.h | 1 - source/common/grpc/common.cc | 2 +- .../filters/http/grpc_json_transcoder/json_transcoder_filter.cc | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index ef5cbaa9b331..050b3d0b2342 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -315,7 +315,6 @@ class HeaderEntry { HEADER_FUNC(GrpcAcceptEncoding) \ HEADER_FUNC(GrpcMessage) \ HEADER_FUNC(GrpcStatus) \ - HEADER_FUNC(GrpcStatusDetailsBin) \ HEADER_FUNC(GrpcTimeout) \ HEADER_FUNC(Host) \ HEADER_FUNC(KeepAlive) \ diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 209e682b2a64..4299fddd6f3e 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -71,7 +71,7 @@ std::string Common::getGrpcMessage(const Http::HeaderMap& trailers) { absl::optional Common::getGrpcStatusDetailsBin(const Http::HeaderMap& trailers) { - const Http::HeaderEntry* details_header = trailers.GrpcStatusDetailsBin(); + const Http::HeaderEntry* details_header = trailers.get(Http::Headers::get().GrpcStatusDetailsBin); if (!details_header) { return absl::nullopt; } 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 726cc93c5d83..6bf671133676 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 @@ -487,7 +487,7 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& // Drop the gRPC status headers, we already have them in the JSON body. response_headers_->removeGrpcStatus(); response_headers_->removeGrpcMessage(); - response_headers_->removeGrpcStatusDetailsBin(); + response_headers_->remove(Http::Headers::get().GrpcStatusDetailsBin); } else if (!status_converted_to_json && !is_trailers_only_response) { // Copy the grpc-message header if it exists. const Http::HeaderEntry* grpc_message_header = trailers.GrpcMessage(); From c6d7e31f75dbaef253def40cabff5ddb0af783ca Mon Sep 17 00:00:00 2001 From: Anatoly Scheglov Date: Thu, 12 Sep 2019 16:17:53 +0300 Subject: [PATCH 10/10] fix formatting Signed-off-by: Anatoly Scheglov --- api/envoy/config/filter/http/transcoder/v2/transcoder.proto | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto index 28beed928f58..9583af300ff8 100644 --- a/api/envoy/config/filter/http/transcoder/v2/transcoder.proto +++ b/api/envoy/config/filter/http/transcoder/v2/transcoder.proto @@ -133,7 +133,8 @@ message GrpcJsonTranscoder { // .. code-block:: none // // grpc-status: 5 - // grpc-status-details-bin: CAUaMwoqdHlwZS5nb29nbGVhcGlzLmNvbS9nb29nbGUucnBjLlJlcXVlc3RJbmZvEgUKA3ItMQ + // grpc-status-details-bin: + // CAUaMwoqdHlwZS5nb29nbGVhcGlzLmNvbS9nb29nbGUucnBjLlJlcXVlc3RJbmZvEgUKA3ItMQ // // The ``grpc-status-details-bin`` header contains a base64-encoded protobuf message // ``google.rpc.Status``. It will be transcoded into: