From 5d98848200148090306c20b19ce3c507144a850b Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 9 May 2018 18:34:50 -0700 Subject: [PATCH 01/19] router: Use gRPC message for local responses when the request is gRPC Some gRPC clients get confused when they get non-gRPC responses to their gRPC requests. While such clients should be fixed, we can play nice and format the local responses returned by Envoy router filter as gRPC responses when the request is gRPC. Fixes: #1934 Signed-off-by: Jarno Rajahalme --- docs/root/intro/version_history.rst | 3 +++ source/common/grpc/common.cc | 13 +++++++++++ source/common/grpc/common.h | 9 ++++++++ source/common/router/router.cc | 25 +++++++++++++++------- test/integration/http2_integration_test.cc | 2 ++ test/integration/http_integration.cc | 15 +++++++++++++ test/integration/http_integration.h | 1 + test/integration/utility.cc | 15 ++++++++----- test/integration/utility.h | 7 ++++-- 9 files changed, 75 insertions(+), 15 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 09ff8f1424c4d..e2d58c36b85b3 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -48,6 +48,9 @@ Version history local configuration. * http: added the ability to pass DNS type Subject Alternative Names of the client certificate in the :ref:`config_http_conn_man_headers_x-forwarded-client-cert` header. +* http: local responses to gRPC requests are now sent as trailers-only gRPC responses instead of plain HTTP responses. + Notably the HTTP response code is always "200" in this case, and the gRPC error code is carried in "grpc-status" + header, optionally accompanied with a text message in "grpc-message" header. * listeners: added :ref:`tcp_fast_open_queue_length ` option. * load balancing: added :ref:`weighted round robin ` support. The round robin diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 6f7714cc51564..6c1aa8d41d170 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -206,6 +206,19 @@ uint64_t Common::grpcToHttpStatus(Status::GrpcStatus grpc_status) { } } +void Common::sendLocalReply( + std::function encode_headers, + Status::GrpcStatus grpc_status, const std::string& message) { + Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl{ + {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::OK))}, + {Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.Grpc}, + {Http::Headers::get().GrpcStatus, std::to_string(enumToInt(grpc_status))}}}; + if (!message.empty()) { + response_headers->insertGrpcMessage().value(message); + } + encode_headers(std::move(response_headers), true); // Trailers only response +} + Buffer::InstancePtr Common::serializeBody(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 910018bcfd535..e718c4135973b 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -71,6 +71,15 @@ class Common { */ static uint64_t grpcToHttpStatus(Status::GrpcStatus grpc_status); + /** + * @param encode_headers supplies the function to encode response headers. + * @param grpc_status gRPC status for grpc-status header. + * @param message gRPC message for grpc-message header. + */ + static void + sendLocalReply(std::function encode_headers, + Status::GrpcStatus grpc_status, const std::string& message); + /** * Charge a success/failure stat to a cluster/service/method. * @param cluster supplies the target cluster. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index d627135668f94..a48fb42e9d65d 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -174,6 +174,19 @@ void Filter::chargeUpstreamCode(Http::Code code, void Filter::sendLocalReply(Http::Code code, const std::string& body, std::function modify_headers) { + // Respond with a gRPC trailers-only response if the request is gRPC + if (grpc_request_) { + Grpc::Common::sendLocalReply( + [this, modify_headers](Http::HeaderMapPtr&& headers, bool end_stream) -> void { + if (headers != nullptr && modify_headers != nullptr) { + modify_headers(*headers); + } + callbacks_->encodeHeaders(std::move(headers), end_stream); + }, + Grpc::Common::httpToGrpcStatus(enumToInt(code)), body); + return; + } + // This is a customized version of send local reply that allows us to set the overloaded // header. Http::Utility::sendLocalReply( @@ -192,6 +205,8 @@ void Filter::sendLocalReply(Http::Code code, const std::string& body, Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { downstream_headers_ = &headers; + grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + // Only increment rq total stat if we actually decode headers here. This does not count requests // that get handled by earlier filters. config_.stats_.rq_total_.inc(); @@ -204,9 +219,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e headers.Path()->value().c_str()); callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::NoRouteFound); - Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::NotFound))}}}; - callbacks_->encodeHeaders(std::move(response_headers), true); + sendLocalReply(Http::Code::NotFound, ""); return Http::FilterHeadersStatus::StopIteration; } @@ -236,10 +249,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e ENVOY_STREAM_LOG(debug, "unknown cluster '{}'", *callbacks_, route_entry_->clusterName()); callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::NoRouteFound); - Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl{ - {Http::Headers::get().Status, - std::to_string(enumToInt(route_entry_->clusterNotFoundResponseCode()))}}}; - callbacks_->encodeHeaders(std::move(response_headers), true); + sendLocalReply(route_entry_->clusterNotFoundResponseCode(), ""); return Http::FilterHeadersStatus::StopIteration; } cluster_ = cluster->info(); @@ -308,7 +318,6 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e ASSERT(headers.Host()); ASSERT(headers.Path()); - grpc_request_ = Grpc::Common::hasGrpcContentType(headers); upstream_request_.reset(new UpstreamRequest(*this, *conn_pool)); upstream_request_->encodeHeaders(end_stream); if (end_stream) { diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 81c2c7321f1f2..4255540847864 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -101,6 +101,8 @@ TEST_P(Http2IntegrationTest, HittingDecoderFilterLimit) { testHittingDecoderFilt TEST_P(Http2IntegrationTest, HittingEncoderFilterLimit) { testHittingEncoderFilterLimit(); } +TEST_P(Http2IntegrationTest, GrpcRouterNotFound) { testGrpcRouterNotFound(); } + TEST_P(Http2IntegrationTest, GrpcRetry) { testGrpcRetry(); } // Send a request with overly large headers, and ensure it results in stream reset. diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 9bc2dc6153d85..b697829131dbe 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -630,6 +630,21 @@ void HttpIntegrationTest::testRetry() { EXPECT_EQ(512U, response->body().size()); } +// Change the default route to be restrictive, and send a request to an alternate route. +void HttpIntegrationTest::testGrpcRouterNotFound() { + config_helper_.setDefaultHostAndRoute("foo.com", "/found"); + initialize(); + + BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( + lookupPort("http"), "POST", "/service/notfound", "", downstream_protocol_, version_, "host", + Http::Headers::get().ContentTypeValues.Grpc); + ASSERT_TRUE(response->complete()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); + EXPECT_EQ(Http::Headers::get().ContentTypeValues.Grpc, + response->headers().ContentType()->value().c_str()); + EXPECT_STREQ("12", response->headers().GrpcStatus()->value().c_str()); +} + void HttpIntegrationTest::testGrpcRetry() { Http::TestHeaderMapImpl response_trailers{{"response1", "trailer1"}, {"grpc-status", "0"}}; initialize(); diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 99ff4a62e2731..41e07e5bebfbf 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -158,6 +158,7 @@ class HttpIntegrationTest : public BaseIntegrationTest { void testDrainClose(); void testRetry(); void testRetryHittingBufferLimit(); + void testGrpcRouterNotFound(); void testGrpcRetry(); void testHittingDecoderFilterLimit(); void testHittingEncoderFilterLimit(); diff --git a/test/integration/utility.cc b/test/integration/utility.cc index fa41c8b2e50c8..4c8e4242c66b8 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -55,7 +55,7 @@ BufferingStreamDecoderPtr IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPtr& addr, const std::string& method, const std::string& url, const std::string& body, Http::CodecClient::Type type, - const std::string& host) { + const std::string& host, const std::string& content_type) { Api::Impl api(std::chrono::milliseconds(9000)); Event::DispatcherPtr dispatcher(api.allocateDispatcher()); std::shared_ptr cluster{new NiceMock()}; @@ -78,6 +78,9 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt headers.insertPath().value(url); headers.insertHost().value(host); headers.insertScheme().value(Http::Headers::get().SchemeValues.Http); + if (!content_type.empty()) { + headers.insertContentType().value(content_type); + } encoder.encodeHeaders(headers, body.empty()); if (!body.empty()) { Buffer::OwnedImpl body_buffer(body); @@ -88,12 +91,14 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt return response; } -BufferingStreamDecoderPtr IntegrationUtil::makeSingleRequest( - uint32_t port, const std::string& method, const std::string& url, const std::string& body, - Http::CodecClient::Type type, Network::Address::IpVersion ip_version, const std::string& host) { +BufferingStreamDecoderPtr +IntegrationUtil::makeSingleRequest(uint32_t port, const std::string& method, const std::string& url, + const std::string& body, Http::CodecClient::Type type, + Network::Address::IpVersion ip_version, const std::string& host, + const std::string& content_type) { auto addr = Network::Utility::resolveUrl( fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(ip_version), port)); - return makeSingleRequest(addr, method, url, body, type, host); + return makeSingleRequest(addr, method, url, body, type, host, content_type); } RawConnectionDriver::RawConnectionDriver(uint32_t port, Buffer::Instance& initial_data, diff --git a/test/integration/utility.h b/test/integration/utility.h index c15b283063281..34c853fb24991 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -96,13 +96,14 @@ class IntegrationUtil { * @param body supplies the optional request body to send. * @param type supplies the codec to use for the request. * @param host supplies the host header to use for the request. + * @param content_type supplies the content-type header to use for the request, if any. * @return BufferingStreamDecoderPtr the complete request or a partial request if there was * remote easly disconnection. */ static BufferingStreamDecoderPtr makeSingleRequest(const Network::Address::InstanceConstSharedPtr& addr, const std::string& method, const std::string& url, const std::string& body, Http::CodecClient::Type type, - const std::string& host = "host"); + const std::string& host = "host", const std::string& content_type = ""); /** * Make a new connection, issues a request, and then disconnect when the request is complete. @@ -113,13 +114,15 @@ class IntegrationUtil { * @param type supplies the codec to use for the request. * @param version the IP addess version of the client and server. * @param host supplies the host header to use for the request. + * @param content_type supplies the content-type header to use for the request, if any. * @return BufferingStreamDecoderPtr the complete request or a partial request if there was * remote easly disconnection. */ static BufferingStreamDecoderPtr makeSingleRequest(uint32_t port, const std::string& method, const std::string& url, const std::string& body, Http::CodecClient::Type type, - Network::Address::IpVersion ip_version, const std::string& host = "host"); + Network::Address::IpVersion ip_version, const std::string& host = "host", + const std::string& content_type = ""); }; // A set of connection callbacks which tracks connection state. From 7896b64d33f4ed317b8cd649c5a049af660efd1b Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 9 May 2018 18:34:50 -0700 Subject: [PATCH 02/19] grpc: Move utility functions to http/utility. Refactor by moving gRPC utility functions that only depend on http to http/utility. This allows making Http::Utility::sendLocalReply() gRPC aware without changing all the callers to be aware of gRPC. Signed-off-by: Jarno Rajahalme --- source/common/grpc/async_client_impl.cc | 2 +- source/common/grpc/common.cc | 119 ------------------ source/common/grpc/common.h | 28 ----- source/common/http/BUILD | 1 + source/common/http/utility.cc | 119 ++++++++++++++++++ source/common/http/utility.h | 29 +++++ source/common/router/router.cc | 8 +- source/common/upstream/health_checker_impl.cc | 4 +- .../json_transcoder_filter.cc | 4 +- test/common/grpc/common_test.cc | 21 ++-- .../grpc/grpc_client_integration_test.cc | 2 +- 11 files changed, 170 insertions(+), 167 deletions(-) diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 79b6d92b82bb1..eebac7d035a1d 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -106,7 +106,7 @@ void AsyncStreamImpl::onHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { } // Technically this should be // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md - // as given by Common::httpToGrpcStatus(), but the Google gRPC client treats + // as given by Http::Utility::httpToGrpcStatus(), but the Google gRPC client treats // this as GrpcStatus::Canceled. streamError(Status::GrpcStatus::Canceled); return; diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 6c1aa8d41d170..901dd13ebedd3 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -21,43 +21,6 @@ namespace Envoy { namespace Grpc { -bool Common::hasGrpcContentType(const Http::HeaderMap& headers) { - const Http::HeaderEntry* content_type = headers.ContentType(); - if (content_type == nullptr) { - return false; - } - // Fail fast if this is not gRPC. - if (!StringUtil::startsWith(content_type->value().c_str(), - Http::Headers::get().ContentTypeValues.Grpc)) { - return false; - } - // Exact match with application/grpc. This and the above case are likely the - // two most common encountered. - if (content_type->value() == Http::Headers::get().ContentTypeValues.Grpc.c_str()) { - return true; - } - // Prefix match with application/grpc+. It's not sufficient to rely on the an - // application/grpc prefix match, since there are related content types such as - // application/grpc-web. - if (content_type->value().size() > Http::Headers::get().ContentTypeValues.Grpc.size() && - content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+') { - return true; - } - // This must be something like application/grpc-web. - return false; -} - -bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream) { - if (end_stream) { - // Trailers-only response, only grpc-status is required. - return headers.GrpcStatus() != nullptr; - } - if (Http::Utility::getResponseStatus(headers) != enumToInt(Http::Code::OK)) { - return false; - } - return hasGrpcContentType(headers); -} - void Common::chargeStat(const Upstream::ClusterInfo& cluster, const std::string& protocol, const std::string& grpc_service, const std::string& grpc_method, const Http::HeaderEntry* grpc_status) { @@ -124,88 +87,6 @@ bool Common::resolveServiceAndMethod(const Http::HeaderEntry* path, std::string* return true; } -Status::GrpcStatus Common::httpToGrpcStatus(uint64_t http_response_status) { - // From - // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. - switch (http_response_status) { - case 400: - return Status::GrpcStatus::Internal; - case 401: - return Status::GrpcStatus::Unauthenticated; - case 403: - return Status::GrpcStatus::PermissionDenied; - case 404: - return Status::GrpcStatus::Unimplemented; - case 429: - case 502: - case 503: - case 504: - return Status::GrpcStatus::Unavailable; - default: - return Status::GrpcStatus::Unknown; - } -} - -uint64_t Common::grpcToHttpStatus(Status::GrpcStatus grpc_status) { - // From https://cloud.google.com/apis/design/errors#handling_errors. - switch (grpc_status) { - case Status::GrpcStatus::Ok: - return 200; - case Status::GrpcStatus::Canceled: - // Client closed request. - return 499; - case Status::GrpcStatus::Unknown: - // Internal server error. - return 500; - case Status::GrpcStatus::InvalidArgument: - // Bad request. - return 400; - case Status::GrpcStatus::DeadlineExceeded: - // Gateway Time-out. - return 504; - case Status::GrpcStatus::NotFound: - // Not found. - return 404; - case Status::GrpcStatus::AlreadyExists: - // Conflict. - return 409; - case Status::GrpcStatus::PermissionDenied: - // Forbidden. - return 403; - case Status::GrpcStatus::ResourceExhausted: - // Too many requests. - return 429; - case Status::GrpcStatus::FailedPrecondition: - // Bad request. - return 400; - case Status::GrpcStatus::Aborted: - // Conflict. - return 409; - case Status::GrpcStatus::OutOfRange: - // Bad request. - return 400; - case Status::GrpcStatus::Unimplemented: - // Not implemented. - return 501; - case Status::GrpcStatus::Internal: - // Internal server error. - return 500; - case Status::GrpcStatus::Unavailable: - // Service unavailable. - return 503; - case Status::GrpcStatus::DataLoss: - // Internal server error. - return 500; - case Status::GrpcStatus::Unauthenticated: - // Unauthorized. - return 401; - case Status::GrpcStatus::InvalidCode: - default: - // Internal server error. - return 500; - } -} - void Common::sendLocalReply( std::function encode_headers, Status::GrpcStatus grpc_status, const std::string& message) { diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index e718c4135973b..a1a3d64cc2391 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -27,19 +27,6 @@ class Exception : public EnvoyException { class Common { public: - /** - * @param headers the headers to parse. - * @return bool indicating whether content-type is gRPC. - */ - static bool hasGrpcContentType(const Http::HeaderMap& headers); - - /** - * @param headers the headers to parse. - * @param bool indicating wether the header is at end_stream. - * @return bool indicating whether the header is a gRPC reseponse header - */ - static bool isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream); - /** * Returns the GrpcStatus code from a given set of trailers, if present. * @param trailers the trailers to parse. @@ -56,21 +43,6 @@ class Common { */ static std::string getGrpcMessage(const Http::HeaderMap& trailers); - /** - * Returns the gRPC status code from a given HTTP response status code. Ordinarily, it is expected - * that a 200 response is provided, but gRPC defines a mapping for intermediaries that are not - * gRPC aware, see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. - * @param http_response_status HTTP status code. - * @return Status::GrpcStatus corresponding gRPC status code. - */ - static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); - - /** - * @param grpc_status gRPC status from grpc-status header. - * @return uint64_t the canonical HTTP status code corresponding to a gRPC status code. - */ - static uint64_t grpcToHttpStatus(Status::GrpcStatus grpc_status); - /** * @param encode_headers supplies the function to encode response headers. * @param grpc_status gRPC status for grpc-status header. diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 09f1a2a4f9598..f320332846833 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -239,6 +239,7 @@ envoy_cc_library( ":exception_lib", ":header_map_lib", ":headers_lib", + "//include/envoy/grpc:status", "//include/envoy/http:codes_interface", "//include/envoy/http:filter_interface", "//include/envoy/http:header_map_interface", diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 3c159aa79dbde..ffa4a36115128 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -186,6 +186,125 @@ bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { Http::Headers::get().UpgradeValues.WebSocket.c_str()))); } +bool Utility::hasGrpcContentType(const HeaderMap& headers) { + const HeaderEntry* content_type = headers.ContentType(); + if (content_type == nullptr) { + return false; + } + // Fail fast if this is not gRPC. + if (!StringUtil::startsWith(content_type->value().c_str(), + Headers::get().ContentTypeValues.Grpc)) { + return false; + } + // Exact match with application/grpc. This and the above case are likely the + // two most common encountered. + if (content_type->value() == Headers::get().ContentTypeValues.Grpc.c_str()) { + return true; + } + // Prefix match with application/grpc+. It's not sufficient to rely on the an + // application/grpc prefix match, since there are related content types such as + // application/grpc-web. + if (content_type->value().size() > Headers::get().ContentTypeValues.Grpc.size() && + content_type->value().c_str()[Headers::get().ContentTypeValues.Grpc.size()] == '+') { + return true; + } + // This must be something like application/grpc-web. + return false; +} + +bool Utility::isGrpcResponseHeader(const HeaderMap& headers, bool end_stream) { + if (end_stream) { + // Trailers-only response, only grpc-status is required. + return headers.GrpcStatus() != nullptr; + } + if (Utility::getResponseStatus(headers) != enumToInt(Code::OK)) { + return false; + } + return hasGrpcContentType(headers); +} + +Grpc::Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) { + // From + // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. + switch (http_response_status) { + case 400: + return Grpc::Status::GrpcStatus::Internal; + case 401: + return Grpc::Status::GrpcStatus::Unauthenticated; + case 403: + return Grpc::Status::GrpcStatus::PermissionDenied; + case 404: + return Grpc::Status::GrpcStatus::Unimplemented; + case 429: + case 502: + case 503: + case 504: + return Grpc::Status::GrpcStatus::Unavailable; + default: + return Grpc::Status::GrpcStatus::Unknown; + } +} + +uint64_t Utility::grpcToHttpStatus(Grpc::Status::GrpcStatus grpc_status) { + // From https://cloud.google.com/apis/design/errors#handling_errors. + switch (grpc_status) { + case Grpc::Status::GrpcStatus::Ok: + return 200; + case Grpc::Status::GrpcStatus::Canceled: + // Client closed request. + return 499; + case Grpc::Status::GrpcStatus::Unknown: + // Internal server error. + return 500; + case Grpc::Status::GrpcStatus::InvalidArgument: + // Bad request. + return 400; + case Grpc::Status::GrpcStatus::DeadlineExceeded: + // Gateway Time-out. + return 504; + case Grpc::Status::GrpcStatus::NotFound: + // Not found. + return 404; + case Grpc::Status::GrpcStatus::AlreadyExists: + // Conflict. + return 409; + case Grpc::Status::GrpcStatus::PermissionDenied: + // Forbidden. + return 403; + case Grpc::Status::GrpcStatus::ResourceExhausted: + // Too many requests. + return 429; + case Grpc::Status::GrpcStatus::FailedPrecondition: + // Bad request. + return 400; + case Grpc::Status::GrpcStatus::Aborted: + // Conflict. + return 409; + case Grpc::Status::GrpcStatus::OutOfRange: + // Bad request. + return 400; + case Grpc::Status::GrpcStatus::Unimplemented: + // Not implemented. + return 501; + case Grpc::Status::GrpcStatus::Internal: + // Internal server error. + return 500; + case Grpc::Status::GrpcStatus::Unavailable: + // Service unavailable. + return 503; + case Grpc::Status::GrpcStatus::DataLoss: + // Internal server error. + return 500; + case Grpc::Status::GrpcStatus::Unauthenticated: + // Unauthorized. + return 401; + case Grpc::Status::GrpcStatus::InvalidCode: + default: + // Internal server error. + return 500; + } +} + Http2Settings Utility::parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOptions& config) { Http2Settings ret; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 1e1e5fc54ed6f..db878c4265b18 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -5,6 +5,7 @@ #include #include "envoy/api/v2/core/protocol.pb.h" +#include "envoy/grpc/status.h" #include "envoy/http/codes.h" #include "envoy/http/filter.h" @@ -90,6 +91,34 @@ class Utility { */ static bool isWebSocketUpgradeRequest(const HeaderMap& headers); + /** + * @param headers the headers to parse. + * @return bool indicating whether content-type is gRPC. + */ + static bool hasGrpcContentType(const HeaderMap& headers); + + /** + * @param headers the headers to parse. + * @param bool indicating wether the header is at end_stream. + * @return bool indicating whether the header is a gRPC reseponse header + */ + static bool isGrpcResponseHeader(const HeaderMap& headers, bool end_stream); + + /** + * Returns the gRPC status code from a given HTTP response status code. Ordinarily, it is expected + * that a 200 response is provided, but gRPC defines a mapping for intermediaries that are not + * gRPC aware, see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. + * @param http_response_status HTTP status code. + * @return Status::GrpcStatus corresponding gRPC status code. + */ + static Grpc::Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); + + /** + * @param grpc_status gRPC status from grpc-status header. + * @return uint64_t the canonical HTTP status code corresponding to a gRPC status code. + */ + static uint64_t grpcToHttpStatus(Grpc::Status::GrpcStatus grpc_status); + /** * @return Http2Settings An Http2Settings populated from the * envoy::api::v2::core::Http2ProtocolOptions config. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index a48fb42e9d65d..2731a034fcc4e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -183,7 +183,7 @@ void Filter::sendLocalReply(Http::Code code, const std::string& body, } callbacks_->encodeHeaders(std::move(headers), end_stream); }, - Grpc::Common::httpToGrpcStatus(enumToInt(code)), body); + Http::Utility::httpToGrpcStatus(enumToInt(code)), body); return; } @@ -205,7 +205,7 @@ void Filter::sendLocalReply(Http::Code code, const std::string& body, Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { downstream_headers_ = &headers; - grpc_request_ = Grpc::Common::hasGrpcContentType(headers); + grpc_request_ = Http::Utility::hasGrpcContentType(headers); // Only increment rq total stat if we actually decode headers here. This does not count requests // that get handled by earlier filters. @@ -568,7 +568,7 @@ void Filter::handleNon5xxResponseHeaders(const Http::HeaderMap& headers, bool en if (end_stream) { absl::optional grpc_status = Grpc::Common::getGrpcStatus(headers); if (grpc_status && - !Http::CodeUtility::is5xx(Grpc::Common::grpcToHttpStatus(grpc_status.value()))) { + !Http::CodeUtility::is5xx(Http::Utility::grpcToHttpStatus(grpc_status.value()))) { upstream_request_->upstream_host_->stats().rq_success_.inc(); } else { upstream_request_->upstream_host_->stats().rq_error_.inc(); @@ -675,7 +675,7 @@ void Filter::onUpstreamTrailers(Http::HeaderMapPtr&& trailers) { if (upstream_request_->grpc_rq_success_deferred_) { absl::optional grpc_status = Grpc::Common::getGrpcStatus(*trailers); if (grpc_status && - !Http::CodeUtility::is5xx(Grpc::Common::grpcToHttpStatus(grpc_status.value()))) { + !Http::CodeUtility::is5xx(Http::Utility::grpcToHttpStatus(grpc_status.value()))) { upstream_request_->upstream_host_->stats().rq_success_.inc(); } else { upstream_request_->upstream_host_->stats().rq_error_.inc(); diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 509fbcd2fb6d4..2d462d97a18c8 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -362,11 +362,11 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( return; } } - onRpcComplete(Grpc::Common::httpToGrpcStatus(http_response_status), "non-200 HTTP response", + onRpcComplete(Http::Utility::httpToGrpcStatus(http_response_status), "non-200 HTTP response", end_stream); return; } - if (!Grpc::Common::hasGrpcContentType(*headers)) { + if (!Http::Utility::hasGrpcContentType(*headers)) { onRpcComplete(Grpc::Status::GrpcStatus::Internal, "invalid gRPC content-type", false); return; } 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 057234108dad3..da6e37590f0dc 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 @@ -306,7 +306,7 @@ void JsonTranscoderFilter::setDecoderFilterCallbacks( Http::FilterHeadersStatus JsonTranscoderFilter::encodeHeaders(Http::HeaderMap& headers, bool end_stream) { - if (!Grpc::Common::isGrpcResponseHeader(headers, end_stream)) { + if (!Http::Utility::isGrpcResponseHeader(headers, end_stream)) { error_ = true; } @@ -377,7 +377,7 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& if (!grpc_status || grpc_status.value() == Grpc::Status::GrpcStatus::InvalidCode) { response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); } else { - response_headers_->Status()->value(Grpc::Common::grpcToHttpStatus(grpc_status.value())); + response_headers_->Status()->value(Http::Utility::grpcToHttpStatus(grpc_status.value())); response_headers_->insertGrpcStatus().value(enumToInt(grpc_status.value())); } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index cd676270743de..662b4b8b5f511 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -1,6 +1,7 @@ #include "common/grpc/common.h" #include "common/http/headers.h" #include "common/http/message_impl.h" +#include "common/http/utility.h" #include "test/mocks/upstream/mocks.h" #include "test/proto/helloworld.pb.h" @@ -121,7 +122,7 @@ TEST(GrpcCommonTest, GrpcToHttpStatus) { {Status::GrpcStatus::InvalidCode, 500}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Common::grpcToHttpStatus(test_case.first)); + EXPECT_EQ(test_case.second, Http::Utility::grpcToHttpStatus(test_case.first)); } } @@ -134,18 +135,18 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { {500, Status::GrpcStatus::Unknown}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Common::httpToGrpcStatus(test_case.first)); + EXPECT_EQ(test_case.second, Http::Utility::httpToGrpcStatus(test_case.first)); } } TEST(GrpcCommonTest, HasGrpcContentType) { { Http::TestHeaderMapImpl headers{}; - EXPECT_FALSE(Common::hasGrpcContentType(headers)); + EXPECT_FALSE(Http::Utility::hasGrpcContentType(headers)); } auto isGrpcContentType = [](const std::string& s) { Http::TestHeaderMapImpl headers{{"content-type", s}}; - return Common::hasGrpcContentType(headers); + return Http::Utility::hasGrpcContentType(headers); }; EXPECT_FALSE(isGrpcContentType("")); EXPECT_FALSE(isGrpcContentType("application/text")); @@ -159,18 +160,18 @@ TEST(GrpcCommonTest, HasGrpcContentType) { TEST(GrpcCommonTest, IsGrpcResponseHeader) { Http::TestHeaderMapImpl grpc_status_only{{":status", "500"}, {"grpc-status", "14"}}; - EXPECT_TRUE(Common::isGrpcResponseHeader(grpc_status_only, true)); - EXPECT_FALSE(Common::isGrpcResponseHeader(grpc_status_only, false)); + EXPECT_TRUE(Http::Utility::isGrpcResponseHeader(grpc_status_only, true)); + EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(grpc_status_only, false)); Http::TestHeaderMapImpl grpc_response_header{{":status", "200"}, {"content-type", "application/grpc"}}; - EXPECT_FALSE(Common::isGrpcResponseHeader(grpc_response_header, true)); - EXPECT_TRUE(Common::isGrpcResponseHeader(grpc_response_header, false)); + EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(grpc_response_header, true)); + EXPECT_TRUE(Http::Utility::isGrpcResponseHeader(grpc_response_header, false)); Http::TestHeaderMapImpl json_response_header{{":status", "200"}, {"content-type", "application/json"}}; - EXPECT_FALSE(Common::isGrpcResponseHeader(json_response_header, true)); - EXPECT_FALSE(Common::isGrpcResponseHeader(json_response_header, false)); + EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(json_response_header, true)); + EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(json_response_header, false)); } TEST(GrpcCommonTest, ValidateResponse) { diff --git a/test/common/grpc/grpc_client_integration_test.cc b/test/common/grpc/grpc_client_integration_test.cc index 72745ceb613d7..b21622b857ebe 100644 --- a/test/common/grpc/grpc_client_integration_test.cc +++ b/test/common/grpc/grpc_client_integration_test.cc @@ -484,7 +484,7 @@ TEST_P(GrpcClientIntegrationTest, HttpNon200Status) { stream->expectTrailingMetadata(empty_metadata_); // Technically this should be // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md - // as given by Common::httpToGrpcStatus(), but the Google gRPC client treats + // as given by Http::Utility::httpToGrpcStatus(), but the Google gRPC client treats // this as GrpcStatus::Canceled. stream->expectGrpcStatus(Status::GrpcStatus::Canceled); stream->fake_stream_->encodeHeaders(reply_headers, true); From 29ba376e7e19c63b59e3e40a8ca2af7e270483ce Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 9 May 2018 18:34:50 -0700 Subject: [PATCH 03/19] http: Make Utility::sendLocalResponse() aware of gRPC. Add either an 'is_grpc' or 'request_headers' parameter to sendLocalResponse() in order to allow the implementation to issue a gRPC response to gRPC requests. Signed-off-by: Jarno Rajahalme --- source/common/grpc/common.cc | 13 ----- source/common/grpc/common.h | 9 ---- source/common/http/conn_manager_impl.cc | 28 +++++----- source/common/http/utility.cc | 51 +++++++++++++++---- source/common/http/utility.h | 42 ++++++++++++++- source/common/router/router.cc | 14 +---- .../filters/http/buffer/buffer_filter.cc | 13 +++-- .../filters/http/buffer/buffer_filter.h | 1 + .../filters/http/fault/fault_filter.cc | 4 +- .../filters/http/fault/fault_filter.h | 1 + .../json_transcoder_filter.cc | 6 +-- .../filters/http/grpc_web/grpc_web_filter.cc | 7 ++- .../filters/http/grpc_web/grpc_web_filter.h | 1 + test/common/http/utility_test.cc | 20 +++++++- test/integration/integration_test.cc | 6 ++- 15 files changed, 143 insertions(+), 73 deletions(-) diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 901dd13ebedd3..bc047340224f7 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -87,19 +87,6 @@ bool Common::resolveServiceAndMethod(const Http::HeaderEntry* path, std::string* return true; } -void Common::sendLocalReply( - std::function encode_headers, - Status::GrpcStatus grpc_status, const std::string& message) { - Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::OK))}, - {Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.Grpc}, - {Http::Headers::get().GrpcStatus, std::to_string(enumToInt(grpc_status))}}}; - if (!message.empty()) { - response_headers->insertGrpcMessage().value(message); - } - encode_headers(std::move(response_headers), true); // Trailers only response -} - Buffer::InstancePtr Common::serializeBody(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 a1a3d64cc2391..66a7e22d7069e 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -43,15 +43,6 @@ class Common { */ static std::string getGrpcMessage(const Http::HeaderMap& trailers); - /** - * @param encode_headers supplies the function to encode response headers. - * @param grpc_status gRPC status for grpc-status header. - * @param message gRPC message for grpc-message header. - */ - static void - sendLocalReply(std::function encode_headers, - Status::GrpcStatus grpc_status, const std::string& message); - /** * Charge a success/failure stat to a cluster/service/method. * @param cluster supplies the target cluster. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 6ee865933e270..96332da05ea24 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1353,7 +1353,8 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { onDecoderFilterAboveWriteBufferHighWatermark(); } else { parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); - Http::Utility::sendLocalReply(*this, parent_.state_.destroyed_, Http::Code::PayloadTooLarge, + Http::Utility::sendLocalReply(*parent_.request_headers_, *this, parent_.state_.destroyed_, + Http::Code::PayloadTooLarge, CodeUtility::toString(Http::Code::PayloadTooLarge)); } } @@ -1426,18 +1427,19 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.encoder_filters_streaming_ = true; stopped_ = false; - Http::Utility::sendLocalReply( - [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { - parent_.response_headers_ = std::move(response_headers); - parent_.response_encoder_->encodeHeaders(*parent_.response_headers_, end_stream); - }, - [&](Buffer::Instance& data, bool end_stream) -> void { - parent_.response_encoder_->encodeData(data, end_stream); - parent_.state_.local_complete_ = end_stream; - parent_.maybeEndEncode(end_stream); - }, - parent_.state_.destroyed_, Http::Code::InternalServerError, - CodeUtility::toString(Http::Code::InternalServerError)); + Http::Utility::sendLocalReply(*parent_.request_headers_, + [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { + parent_.response_headers_ = std::move(response_headers); + parent_.response_encoder_->encodeHeaders( + *parent_.response_headers_, end_stream); + }, + [&](Buffer::Instance& data, bool end_stream) -> void { + parent_.response_encoder_->encodeData(data, end_stream); + parent_.state_.local_complete_ = end_stream; + parent_.maybeEndEncode(end_stream); + }, + parent_.state_.destroyed_, Http::Code::InternalServerError, + CodeUtility::toString(Http::Code::InternalServerError)); } else { resetStream(); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index ffa4a36115128..c242859d62abd 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -329,22 +329,55 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co return ret; } -void Utility::sendLocalReply(StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, +void Utility::sendLocalReply(const HeaderMap& request_headers, + StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text) { - sendLocalReply( - [&](HeaderMapPtr&& headers, bool end_stream) -> void { - callbacks.encodeHeaders(std::move(headers), end_stream); - }, - [&](Buffer::Instance& data, bool end_stream) -> void { - callbacks.encodeData(data, end_stream); - }, - is_reset, response_code, body_text); + sendLocalReply(hasGrpcContentType(request_headers), callbacks, is_reset, response_code, + body_text); +} + +void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, + const bool& is_reset, Code response_code, + const std::string& body_text) { + sendLocalReply(is_grpc, + [&](HeaderMapPtr&& headers, bool end_stream) -> void { + callbacks.encodeHeaders(std::move(headers), end_stream); + }, + [&](Buffer::Instance& data, bool end_stream) -> void { + callbacks.encodeData(data, end_stream); + }, + is_reset, response_code, body_text); } void Utility::sendLocalReply( + const HeaderMap& request_headers, std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, const std::string& body_text) { + // Respond with a gRPC trailers-only response if the request is gRPC + sendLocalReply(hasGrpcContentType(request_headers), encode_headers, encode_data, is_reset, + response_code, body_text); +} + +void Utility::sendLocalReply( + bool is_grpc, std::function encode_headers, + std::function encode_data, const bool& is_reset, + Code response_code, const std::string& body_text) { + // Respond with a gRPC trailers-only response if the request is gRPC + if (is_grpc) { + HeaderMapPtr response_headers{ + new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(Code::OK))}, + {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc}, + {Headers::get().GrpcStatus, + std::to_string(enumToInt(httpToGrpcStatus(enumToInt(response_code))))}}}; + if (!body_text.empty()) { + // TODO: GrpcMessage should be percent-encoded + response_headers->insertGrpcMessage().value(body_text); + } + encode_headers(std::move(response_headers), true); // Trailers only response + return; + } + HeaderMapPtr response_headers{ new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))}}}; if (!body_text.empty()) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index db878c4265b18..5b4f337dfc40c 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -133,6 +133,7 @@ class Utility { /** * Create a locally generated response using filter callbacks. + * @param request_headers supplies the headers of the request this is a response for. * @param callbacks supplies the filter callbacks to use. * @param is_reset boolean reference that indicates whether a stream has been reset. It is the * responsibility of the caller to ensure that this is set to false if onDestroy() @@ -141,10 +142,46 @@ class Utility { * @param body_text supplies the optional body text which is sent using the text/plain content * type. */ - static void sendLocalReply(StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, + static void sendLocalReply(const HeaderMap& request_headers, + StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text); + + /** + * Create a locally generated response using filter callbacks. + * @param is_grpc tells if this is a response to a gRPC request. + * @param callbacks supplies the filter callbacks to use. + * @param is_reset boolean reference that indicates whether a stream has been reset. It is the + * responsibility of the caller to ensure that this is set to false if onDestroy() + * is invoked in the context of sendLocalReply(). + * @param response_code supplies the HTTP response code. + * @param body_text supplies the optional body text which is sent using the text/plain content + * type. + */ + static void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, + const bool& is_reset, Code response_code, + const std::string& body_text); + + /** + * Create a locally generated response using the provided lambdas. + * @param request_headers supplies the headers of the request this is a response for. + * @param encode_headers supplies the function to encode response headers. + * @param encode_data supplies the function to encode the response body. + * @param is_reset boolean reference that indicates whether a stream has been reset. It is the + * responsibility of the caller to ensure that this is set to false if onDestroy() + * is invoked in the context of sendLocalReply(). + * @param response_code supplies the HTTP response code. + * @param body_text supplies the optional body text which is sent using the text/plain content + * type. + */ + static void + sendLocalReply(const HeaderMap& request_headers, + std::function encode_headers, + std::function encode_data, + const bool& is_reset, Code response_code, const std::string& body_text); + /** * Create a locally generated response using the provided lambdas. + * @param is_grpc tells if this is a response to a gRPC request. * @param encode_headers supplies the function to encode response headers. * @param encode_data supplies the function to encode the response body. * @param is_reset boolean reference that indicates whether a stream has been reset. It is the @@ -155,7 +192,8 @@ class Utility { * type. */ static void - sendLocalReply(std::function encode_headers, + sendLocalReply(bool is_grpc, + std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, const std::string& body_text); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 2731a034fcc4e..4a67e0cb19d77 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -174,22 +174,10 @@ void Filter::chargeUpstreamCode(Http::Code code, void Filter::sendLocalReply(Http::Code code, const std::string& body, std::function modify_headers) { - // Respond with a gRPC trailers-only response if the request is gRPC - if (grpc_request_) { - Grpc::Common::sendLocalReply( - [this, modify_headers](Http::HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { - modify_headers(*headers); - } - callbacks_->encodeHeaders(std::move(headers), end_stream); - }, - Http::Utility::httpToGrpcStatus(enumToInt(code)), body); - return; - } - // This is a customized version of send local reply that allows us to set the overloaded // header. Http::Utility::sendLocalReply( + grpc_request_, [this, modify_headers](Http::HeaderMapPtr&& headers, bool end_stream) -> void { if (headers != nullptr && modify_headers != nullptr) { modify_headers(*headers); diff --git a/source/extensions/filters/http/buffer/buffer_filter.cc b/source/extensions/filters/http/buffer/buffer_filter.cc index ce712a026df6f..09009769a4e7b 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.cc +++ b/source/extensions/filters/http/buffer/buffer_filter.cc @@ -68,7 +68,7 @@ void BufferFilter::initConfig() { settings_ = route_local ?: settings_; } -Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { +Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { if (end_stream) { // If this is a header-only request, we don't need to do any buffering. return Http::FilterHeadersStatus::Continue; @@ -83,6 +83,7 @@ Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap&, bool end callbacks_->setDecoderBufferLimit(settings_->maxRequestBytes()); request_timeout_ = callbacks_->dispatcher().createTimer([this]() -> void { onRequestTimeout(); }); request_timeout_->enableTimer(settings_->maxRequestTime()); + downstream_headers_ = &headers; return Http::FilterHeadersStatus::StopIteration; } @@ -113,12 +114,16 @@ void BufferFilter::onDestroy() { } void BufferFilter::onRequestTimeout() { - Http::Utility::sendLocalReply(*callbacks_, stream_destroyed_, Http::Code::RequestTimeout, - "buffer request timeout"); + ASSERT(downstream_headers_); + Http::Utility::sendLocalReply(*downstream_headers_, *callbacks_, stream_destroyed_, + Http::Code::RequestTimeout, "buffer request timeout"); config_->stats().rq_timeout_.inc(); } -void BufferFilter::resetInternalState() { request_timeout_.reset(); } +void BufferFilter::resetInternalState() { + downstream_headers_ = nullptr; + request_timeout_.reset(); +} void BufferFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { callbacks_ = &callbacks; diff --git a/source/extensions/filters/http/buffer/buffer_filter.h b/source/extensions/filters/http/buffer/buffer_filter.h index 277a0f50f9d04..f6360f045c7f1 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.h +++ b/source/extensions/filters/http/buffer/buffer_filter.h @@ -92,6 +92,7 @@ class BufferFilter : public Http::StreamDecoderFilter { const BufferFilterSettings* settings_; Http::StreamDecoderFilterCallbacks* callbacks_{}; Event::TimerPtr request_timeout_; + Http::HeaderMap* downstream_headers_{}; bool stream_destroyed_{}; bool config_initialized_{}; }; diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index cf646324adba1..17dd752f591e7 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -70,6 +70,7 @@ FaultFilter::~FaultFilter() { ASSERT(!delay_timer_); } // if we inject a delay, then we will inject the abort in the delay timer // callback. Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, bool) { + downstream_headers_ = &headers; // Route-level configuration overrides filter-level configuration // NOTE: We should not use runtime when reading from route-level // faults. In other words, runtime is supported only when faults are @@ -250,7 +251,8 @@ void FaultFilter::postDelayInjection() { void FaultFilter::abortWithHTTPStatus() { callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::FaultInjected); - Http::Utility::sendLocalReply(*callbacks_, stream_destroyed_, + ASSERT(downstream_headers_); + Http::Utility::sendLocalReply(*downstream_headers_, *callbacks_, stream_destroyed_, static_cast(abortHttpStatus()), "fault filter abort"); recordAbortsInjectedStats(); } diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 8c4336bf4362c..6c21abd958049 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -125,6 +125,7 @@ class FaultFilter : public Http::StreamDecoderFilter { std::string downstream_cluster_{}; bool stream_destroyed_{}; const FaultSettings* fault_settings_; + Http::HeaderMap* downstream_headers_{}; std::string downstream_cluster_delay_percent_key_{}; std::string downstream_cluster_abort_percent_key_{}; 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 da6e37590f0dc..fcc089dba08e2 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 @@ -237,8 +237,8 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h if (!request_status.ok()) { ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; - Http::Utility::sendLocalReply(*decoder_callbacks_, stream_reset_, Http::Code::BadRequest, - request_status.error_message()); + Http::Utility::sendLocalReply(false, *decoder_callbacks_, stream_reset_, + Http::Code::BadRequest, request_status.error_message()); return Http::FilterHeadersStatus::StopIteration; } @@ -273,7 +273,7 @@ Http::FilterDataStatus JsonTranscoderFilter::decodeData(Buffer::Instance& data, if (!request_status.ok()) { ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; - Http::Utility::sendLocalReply(*decoder_callbacks_, stream_reset_, Http::Code::BadRequest, + Http::Utility::sendLocalReply(false, *decoder_callbacks_, stream_reset_, Http::Code::BadRequest, request_status.error_message()); return Http::FilterDataStatus::StopIterationNoBuffer; diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 72c9a7ce1dfcb..580d061ba6d30 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -41,6 +41,7 @@ bool GrpcWebFilter::isGrpcWebRequest(const Http::HeaderMap& headers) { // TODO(fengli): Implements the subtypes of gRPC-Web content-type other than proto, like +json, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { const Http::HeaderEntry* content_type = headers.ContentType(); + is_grpc_request_ = Http::Utility::hasGrpcContentType(headers); if (!isGrpcWebRequest(headers)) { return Http::FilterHeadersStatus::Continue; } @@ -94,7 +95,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en } if (available % 4 != 0) { // Client end stream with invalid base64. Note, base64 padding is mandatory. - Http::Utility::sendLocalReply(*decoder_callbacks_, stream_destroyed_, Http::Code::BadRequest, + Http::Utility::sendLocalReply(is_grpc_request_, *decoder_callbacks_, stream_destroyed_, + Http::Code::BadRequest, "Bad gRPC-web request, invalid base64 data."); return Http::FilterDataStatus::StopIterationNoBuffer; } @@ -110,7 +112,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en decoding_buffer_.length())); if (decoded.empty()) { // Error happened when decoding base64. - Http::Utility::sendLocalReply(*decoder_callbacks_, stream_destroyed_, Http::Code::BadRequest, + Http::Utility::sendLocalReply(is_grpc_request_, *decoder_callbacks_, stream_destroyed_, + Http::Code::BadRequest, "Bad gRPC-web request, invalid base64 data."); return Http::FilterDataStatus::StopIterationNoBuffer; } diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 022c6ea853caa..8ff707c8f1a1e 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -68,6 +68,7 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { std::string grpc_method_; bool do_stat_tracking_{}; bool stream_destroyed_{}; + bool is_grpc_request_{}; bool is_grpc_web_request_{}; }; diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index e6124eb6d0a31..c19c12e693475 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -14,6 +14,7 @@ #include "gtest/gtest.h" +using testing::Invoke; using testing::InvokeWithoutArgs; using testing::_; @@ -246,7 +247,22 @@ TEST(HttpUtility, SendLocalReply) { EXPECT_CALL(callbacks, encodeHeaders_(_, false)); EXPECT_CALL(callbacks, encodeData(_, true)); - Utility::sendLocalReply(callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); +} + +TEST(HttpUtility, SendLocalGrpcReply) { + MockStreamDecoderFilterCallbacks callbacks; + bool is_reset = false; + + EXPECT_CALL(callbacks, encodeHeaders_(_, true)) + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ(headers.Status()->value().c_str(), "200"); + EXPECT_NE(headers.GrpcStatus(), nullptr); + EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "2"); // Unknown gRPC error. + EXPECT_NE(headers.GrpcMessage(), nullptr); + EXPECT_STREQ(headers.GrpcMessage()->value().c_str(), "large"); + })); + Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); } TEST(HttpUtility, SendLocalReplyDestroyedEarly) { @@ -257,7 +273,7 @@ TEST(HttpUtility, SendLocalReplyDestroyedEarly) { is_reset = true; })); EXPECT_CALL(callbacks, encodeData(_, true)).Times(0); - Utility::sendLocalReply(callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large"); } TEST(HttpUtility, TestAppendHeader) { diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 888ade21a5201..fc0726ae0cf93 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -147,13 +147,15 @@ TEST_P(IntegrationTest, HittingEncoderFilterLimitBufferingHeaders) { waitForNextUpstreamRequest(); // Send the overly large response. Because the grpc_http1_bridge filter buffers and buffer - // limits are sent, this will be translated into a 500 from Envoy. + // limits are exceeded, this will be translated into a 500 from Envoy. upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false); upstream_request_->encodeData(1024 * 65, false); response->waitForEndStream(); EXPECT_TRUE(response->complete()); - EXPECT_STREQ("500", response->headers().Status()->value().c_str()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); + EXPECT_NE(response->headers().GrpcStatus(), nullptr); + EXPECT_STREQ("2", response->headers().GrpcStatus()->value().c_str()); // Unknown gRPC error } TEST_P(IntegrationTest, HittingEncoderFilterLimit) { testHittingEncoderFilterLimit(); } From 2c264bf5fea3040d063060c5d81721f09e42fb16 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Tue, 8 May 2018 17:47:36 -0700 Subject: [PATCH 04/19] http: add StreamDecoderFilter callback sendLocalReply(). Add a StreamDecoderFilter callback sendLocalReply() and convert current users of Utility::sendLocalReply() to use the callback instead. The new callback is implemented using Utility::sendLocalReply(), but making use of locally available state allows simplifying the API, enabling support for gRPC responses without requiring filters to become gRPC aware. Note that the local responses are still implemented via the encodeHeaders() and encodeData() callbacks that call the corresponding member functions of all encoders in the filter chain, not just the ones downstream of the filter issuing the sendLocalReply(). This has the notable effect of the filter issuing the local reply itself receiving a encodeHeaders() call for the local reply it issued. Signed-off-by: Jarno Rajahalme --- include/envoy/http/filter.h | 11 +++++++ source/common/http/async_client_impl.cc | 1 + source/common/http/async_client_impl.h | 14 ++++++++ source/common/http/conn_manager_impl.cc | 9 +++-- source/common/http/conn_manager_impl.h | 19 +++++++++++ source/common/http/utility.cc | 17 ---------- source/common/http/utility.h | 33 ------------------- source/common/router/router.cc | 30 ++++------------- source/common/router/router.h | 10 ------ .../filters/http/buffer/buffer_filter.cc | 12 ++----- .../filters/http/buffer/buffer_filter.h | 1 - .../filters/http/fault/fault_filter.cc | 6 ++-- .../filters/http/fault/fault_filter.h | 1 - .../json_transcoder_filter.cc | 8 ++--- .../filters/http/grpc_web/grpc_web_filter.cc | 11 +++---- .../filters/http/grpc_web/grpc_web_filter.h | 1 - test/mocks/http/mocks.h | 19 +++++++++++ 17 files changed, 87 insertions(+), 116 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 213e5b359b978..0980b16ab62f5 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -190,6 +190,17 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { */ virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; + /** + * Create a locally generated response using the provided lambdas. + * @param response_code supplies the HTTP response code. + * @param body_text supplies the optional body text which is sent using the text/plain content + * type. + * @param modify_headers supplies an optional callback function that can modify the + * response headers. + */ + virtual void sendLocalReply(Code response_code, const std::string& body_text, + std::function modify_headers) PURE; + /** * Called with 100-Continue headers to be encoded. * diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index ba46b5bf18e95..e13dab4ebb593 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -125,6 +125,7 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { + is_grpc_request_ = Utility::hasGrpcContentType(headers); headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); Utility::appendXff(headers, *parent_.config_.local_info_.address()); diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index a01e4061e12f6..079c75cd6e998 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -261,6 +261,19 @@ class AsyncStreamImpl : public AsyncClient::Stream, void continueDecoding() override { NOT_IMPLEMENTED; } void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED; } const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } + void sendLocalReply(Code code, const std::string& body, + std::function modify_headers) override { + Utility::sendLocalReply( + is_grpc_request_, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (headers != nullptr && modify_headers != nullptr) { + modify_headers(*headers); + } + encodeHeaders(std::move(headers), end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, + remote_closed_, code, body); + } // The async client won't pause if sending an Expect: 100-Continue so simply // swallows any incoming encode100Continue. void encode100ContinueHeaders(HeaderMapPtr&&) override {} @@ -284,6 +297,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, bool local_closed_{}; bool remote_closed_{}; Buffer::InstancePtr buffered_body_; + bool is_grpc_request_{}; friend class AsyncClientImpl; }; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 96332da05ea24..519c31ad95616 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -663,7 +663,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte for (; entry != decoder_filters_.end(); entry++) { ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders)); state_.filter_call_state_ |= FilterCallState::DecodeHeaders; - FilterHeadersStatus status = (*entry)->handle_->decodeHeaders( + FilterHeadersStatus status = (*entry)->decodeHeaders( headers, end_stream && continue_data_entry == decoder_filters_.end()); state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, @@ -1353,9 +1353,8 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { onDecoderFilterAboveWriteBufferHighWatermark(); } else { parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); - Http::Utility::sendLocalReply(*parent_.request_headers_, *this, parent_.state_.destroyed_, - Http::Code::PayloadTooLarge, - CodeUtility::toString(Http::Code::PayloadTooLarge)); + sendLocalReply(Http::Code::PayloadTooLarge, CodeUtility::toString(Http::Code::PayloadTooLarge), + nullptr); } } @@ -1427,7 +1426,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.encoder_filters_streaming_ = true; stopped_ = false; - Http::Utility::sendLocalReply(*parent_.request_headers_, + Http::Utility::sendLocalReply(Http::Utility::hasGrpcContentType(*parent_.request_headers_), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { parent_.response_headers_ = std::move(response_headers); parent_.response_encoder_->encodeHeaders( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 3062720905d63..3af3a642286d8 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -164,6 +164,19 @@ class ConnectionManagerImpl : Logger::Loggable, const Buffer::Instance* decodingBuffer() override { return parent_.buffered_request_data_.get(); } + void sendLocalReply(Code code, const std::string& body, + std::function modify_headers) override { + Utility::sendLocalReply( + is_grpc_request_, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (headers != nullptr && modify_headers != nullptr) { + modify_headers(*headers); + } + encodeHeaders(std::move(headers), end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, + parent_.state_.destroyed_, code, body); + } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; void encodeData(Buffer::Instance& data, bool end_stream) override; @@ -177,10 +190,16 @@ class ConnectionManagerImpl : Logger::Loggable, void setDecoderBufferLimit(uint32_t limit) override { parent_.setBufferLimit(limit); } uint32_t decoderBufferLimit() override { return parent_.buffer_limit_; } + FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { + is_grpc_request_ = Http::Utility::hasGrpcContentType(headers); + return handle_->decodeHeaders(headers, end_stream); + } + void requestDataTooLarge(); void requestDataDrained(); StreamDecoderFilterSharedPtr handle_; + bool is_grpc_request_{}; }; typedef std::unique_ptr ActiveStreamDecoderFilterPtr; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index c242859d62abd..5f7bf112001b8 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -329,13 +329,6 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co return ret; } -void Utility::sendLocalReply(const HeaderMap& request_headers, - StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, const std::string& body_text) { - sendLocalReply(hasGrpcContentType(request_headers), callbacks, is_reset, response_code, - body_text); -} - void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text) { @@ -349,16 +342,6 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac is_reset, response_code, body_text); } -void Utility::sendLocalReply( - const HeaderMap& request_headers, - std::function encode_headers, - std::function encode_data, const bool& is_reset, - Code response_code, const std::string& body_text) { - // Respond with a gRPC trailers-only response if the request is gRPC - sendLocalReply(hasGrpcContentType(request_headers), encode_headers, encode_data, is_reset, - response_code, body_text); -} - void Utility::sendLocalReply( bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 5b4f337dfc40c..17e02fc510e75 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -131,21 +131,6 @@ class Utility { */ static Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& config); - /** - * Create a locally generated response using filter callbacks. - * @param request_headers supplies the headers of the request this is a response for. - * @param callbacks supplies the filter callbacks to use. - * @param is_reset boolean reference that indicates whether a stream has been reset. It is the - * responsibility of the caller to ensure that this is set to false if onDestroy() - * is invoked in the context of sendLocalReply(). - * @param response_code supplies the HTTP response code. - * @param body_text supplies the optional body text which is sent using the text/plain content - * type. - */ - static void sendLocalReply(const HeaderMap& request_headers, - StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, const std::string& body_text); - /** * Create a locally generated response using filter callbacks. * @param is_grpc tells if this is a response to a gRPC request. @@ -161,24 +146,6 @@ class Utility { const bool& is_reset, Code response_code, const std::string& body_text); - /** - * Create a locally generated response using the provided lambdas. - * @param request_headers supplies the headers of the request this is a response for. - * @param encode_headers supplies the function to encode response headers. - * @param encode_data supplies the function to encode the response body. - * @param is_reset boolean reference that indicates whether a stream has been reset. It is the - * responsibility of the caller to ensure that this is set to false if onDestroy() - * is invoked in the context of sendLocalReply(). - * @param response_code supplies the HTTP response code. - * @param body_text supplies the optional body text which is sent using the text/plain content - * type. - */ - static void - sendLocalReply(const HeaderMap& request_headers, - std::function encode_headers, - std::function encode_data, - const bool& is_reset, Code response_code, const std::string& body_text); - /** * Create a locally generated response using the provided lambdas. * @param is_grpc tells if this is a response to a gRPC request. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4a67e0cb19d77..197303bc8dae3 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -172,24 +172,6 @@ void Filter::chargeUpstreamCode(Http::Code code, chargeUpstreamCode(response_status_code, fake_response_headers, upstream_host, dropped); } -void Filter::sendLocalReply(Http::Code code, const std::string& body, - std::function modify_headers) { - // This is a customized version of send local reply that allows us to set the overloaded - // header. - Http::Utility::sendLocalReply( - grpc_request_, - [this, modify_headers](Http::HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { - modify_headers(*headers); - } - callbacks_->encodeHeaders(std::move(headers), end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - callbacks_->encodeData(data, end_stream); - }, - stream_destroyed_, code, body); -} - Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { downstream_headers_ = &headers; @@ -207,7 +189,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e headers.Path()->value().c_str()); callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::NoRouteFound); - sendLocalReply(Http::Code::NotFound, ""); + callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr); return Http::FilterHeadersStatus::StopIteration; } @@ -216,7 +198,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e if (direct_response != nullptr) { config_.stats_.rq_direct_response_.inc(); direct_response->rewritePathHeader(headers); - sendLocalReply( + callbacks_->sendLocalReply( direct_response->responseCode(), direct_response->responseBody(), [ this, direct_response, &request_headers = headers ](Http::HeaderMap & response_headers) ->void { @@ -237,7 +219,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e ENVOY_STREAM_LOG(debug, "unknown cluster '{}'", *callbacks_, route_entry_->clusterName()); callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::NoRouteFound); - sendLocalReply(route_entry_->clusterNotFoundResponseCode(), ""); + callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", nullptr); return Http::FilterHeadersStatus::StopIteration; } cluster_ = cluster->info(); @@ -257,7 +239,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e if (cluster_->maintenanceMode()) { callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::UpstreamOverflow); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, true); - sendLocalReply( + callbacks_->sendLocalReply( Http::Code::ServiceUnavailable, "maintenance mode", [](Http::HeaderMap& headers) { headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); }); @@ -334,7 +316,7 @@ Http::ConnectionPool::Instance* Filter::getConnPool() { void Filter::sendNoHealthyUpstreamResponse() { callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::NoHealthyUpstream); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, false); - sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream"); + callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", nullptr); } Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) { @@ -521,7 +503,7 @@ void Filter::onUpstreamReset(UpstreamResetType type, if (upstream_host != nullptr && !Http::CodeUtility::is5xx(enumToInt(code))) { upstream_host->stats().rq_error_.inc(); } - sendLocalReply(code, body, [dropped](Http::HeaderMap& headers) { + callbacks_->sendLocalReply(code, body, [dropped](Http::HeaderMap& headers) { if (dropped) { headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); } diff --git a/source/common/router/router.h b/source/common/router/router.h index 6c8b483fa1ff7..38908681b16dc 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -337,16 +337,6 @@ class Filter : Logger::Loggable, // and handle difference between gRPC and non-gRPC requests. void handleNon5xxResponseHeaders(const Http::HeaderMap& headers, bool end_stream); - /** - * Send a locally generated (non-proxied) HTTP response. - * @param code supplies the HTTP status code. - * @param body supplies the response body (empty string if no body is needed). - * @param modify_headers supplies an optional callback function that can modify the - * response headers. - */ - void sendLocalReply(Http::Code code, const std::string& body, - std::function modify_headers = nullptr); - FilterConfig& config_; Http::StreamDecoderFilterCallbacks* callbacks_{}; RouteConstSharedPtr route_; diff --git a/source/extensions/filters/http/buffer/buffer_filter.cc b/source/extensions/filters/http/buffer/buffer_filter.cc index 09009769a4e7b..1f79bec9abd20 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.cc +++ b/source/extensions/filters/http/buffer/buffer_filter.cc @@ -68,7 +68,7 @@ void BufferFilter::initConfig() { settings_ = route_local ?: settings_; } -Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { +Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap&, bool end_stream) { if (end_stream) { // If this is a header-only request, we don't need to do any buffering. return Http::FilterHeadersStatus::Continue; @@ -83,7 +83,6 @@ Http::FilterHeadersStatus BufferFilter::decodeHeaders(Http::HeaderMap& headers, callbacks_->setDecoderBufferLimit(settings_->maxRequestBytes()); request_timeout_ = callbacks_->dispatcher().createTimer([this]() -> void { onRequestTimeout(); }); request_timeout_->enableTimer(settings_->maxRequestTime()); - downstream_headers_ = &headers; return Http::FilterHeadersStatus::StopIteration; } @@ -114,16 +113,11 @@ void BufferFilter::onDestroy() { } void BufferFilter::onRequestTimeout() { - ASSERT(downstream_headers_); - Http::Utility::sendLocalReply(*downstream_headers_, *callbacks_, stream_destroyed_, - Http::Code::RequestTimeout, "buffer request timeout"); + callbacks_->sendLocalReply(Http::Code::RequestTimeout, "buffer request timeout", nullptr); config_->stats().rq_timeout_.inc(); } -void BufferFilter::resetInternalState() { - downstream_headers_ = nullptr; - request_timeout_.reset(); -} +void BufferFilter::resetInternalState() { request_timeout_.reset(); } void BufferFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { callbacks_ = &callbacks; diff --git a/source/extensions/filters/http/buffer/buffer_filter.h b/source/extensions/filters/http/buffer/buffer_filter.h index f6360f045c7f1..277a0f50f9d04 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.h +++ b/source/extensions/filters/http/buffer/buffer_filter.h @@ -92,7 +92,6 @@ class BufferFilter : public Http::StreamDecoderFilter { const BufferFilterSettings* settings_; Http::StreamDecoderFilterCallbacks* callbacks_{}; Event::TimerPtr request_timeout_; - Http::HeaderMap* downstream_headers_{}; bool stream_destroyed_{}; bool config_initialized_{}; }; diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 17dd752f591e7..bf225706c0982 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -70,7 +70,6 @@ FaultFilter::~FaultFilter() { ASSERT(!delay_timer_); } // if we inject a delay, then we will inject the abort in the delay timer // callback. Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, bool) { - downstream_headers_ = &headers; // Route-level configuration overrides filter-level configuration // NOTE: We should not use runtime when reading from route-level // faults. In other words, runtime is supported only when faults are @@ -251,9 +250,8 @@ void FaultFilter::postDelayInjection() { void FaultFilter::abortWithHTTPStatus() { callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::FaultInjected); - ASSERT(downstream_headers_); - Http::Utility::sendLocalReply(*downstream_headers_, *callbacks_, stream_destroyed_, - static_cast(abortHttpStatus()), "fault filter abort"); + callbacks_->sendLocalReply(static_cast(abortHttpStatus()), "fault filter abort", + nullptr); recordAbortsInjectedStats(); } diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 6c21abd958049..8c4336bf4362c 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -125,7 +125,6 @@ class FaultFilter : public Http::StreamDecoderFilter { std::string downstream_cluster_{}; bool stream_destroyed_{}; const FaultSettings* fault_settings_; - Http::HeaderMap* downstream_headers_{}; std::string downstream_cluster_delay_percent_key_{}; std::string downstream_cluster_abort_percent_key_{}; 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 fcc089dba08e2..aaf61ae382333 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 @@ -237,8 +237,8 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h if (!request_status.ok()) { ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; - Http::Utility::sendLocalReply(false, *decoder_callbacks_, stream_reset_, - Http::Code::BadRequest, request_status.error_message()); + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, request_status.error_message(), + nullptr); return Http::FilterHeadersStatus::StopIteration; } @@ -273,8 +273,8 @@ Http::FilterDataStatus JsonTranscoderFilter::decodeData(Buffer::Instance& data, if (!request_status.ok()) { ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; - Http::Utility::sendLocalReply(false, *decoder_callbacks_, stream_reset_, Http::Code::BadRequest, - request_status.error_message()); + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, request_status.error_message(), + nullptr); return Http::FilterDataStatus::StopIterationNoBuffer; } diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 580d061ba6d30..ffc0c0c20cfa6 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -41,7 +41,6 @@ bool GrpcWebFilter::isGrpcWebRequest(const Http::HeaderMap& headers) { // TODO(fengli): Implements the subtypes of gRPC-Web content-type other than proto, like +json, etc. Http::FilterHeadersStatus GrpcWebFilter::decodeHeaders(Http::HeaderMap& headers, bool) { const Http::HeaderEntry* content_type = headers.ContentType(); - is_grpc_request_ = Http::Utility::hasGrpcContentType(headers); if (!isGrpcWebRequest(headers)) { return Http::FilterHeadersStatus::Continue; } @@ -95,9 +94,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en } if (available % 4 != 0) { // Client end stream with invalid base64. Note, base64 padding is mandatory. - Http::Utility::sendLocalReply(is_grpc_request_, *decoder_callbacks_, stream_destroyed_, - Http::Code::BadRequest, - "Bad gRPC-web request, invalid base64 data."); + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, + "Bad gRPC-web request, invalid base64 data.", nullptr); return Http::FilterDataStatus::StopIterationNoBuffer; } } else if (available < 4) { @@ -112,9 +110,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en decoding_buffer_.length())); if (decoded.empty()) { // Error happened when decoding base64. - Http::Utility::sendLocalReply(is_grpc_request_, *decoder_callbacks_, stream_destroyed_, - Http::Code::BadRequest, - "Bad gRPC-web request, invalid base64 data."); + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, + "Bad gRPC-web request, invalid base64 data.", nullptr); return Http::FilterDataStatus::StopIterationNoBuffer; } diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 8ff707c8f1a1e..022c6ea853caa 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -68,7 +68,6 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { std::string grpc_method_; bool do_stat_tracking_{}; bool stream_destroyed_{}; - bool is_grpc_request_{}; bool is_grpc_web_request_{}; }; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index a0aa273b9f64c..0553a645309db 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -14,6 +14,8 @@ #include "envoy/http/filter.h" #include "envoy/ssl/connection.h" +#include "common/http/utility.h" + #include "test/mocks/common.h" #include "test/mocks/event/mocks.h" #include "test/mocks/request_info/mocks.h" @@ -195,6 +197,19 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(decoderBufferLimit, uint32_t()); // Http::StreamDecoderFilterCallbacks + void sendLocalReply(Code code, const std::string& body, + std::function modify_headers) override { + Utility::sendLocalReply( + is_grpc_request_, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (headers != nullptr && modify_headers != nullptr) { + modify_headers(*headers); + } + encodeHeaders(std::move(headers), end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, + stream_destroyed_, code, body); + } void encode100ContinueHeaders(HeaderMapPtr&& headers) override { encode100ContinueHeaders_(*headers); } @@ -206,6 +221,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(continueDecoding, void()); MOCK_METHOD2(addDecodedData, void(Buffer::Instance& data, bool streaming)); MOCK_METHOD0(decodingBuffer, const Buffer::Instance*()); + MOCK_METHOD3(sendLocalReply_, void(Code code, const std::string& body, + std::function modify_headers)); MOCK_METHOD1(encode100ContinueHeaders_, void(HeaderMap& headers)); MOCK_METHOD2(encodeHeaders_, void(HeaderMap& headers, bool end_stream)); MOCK_METHOD2(encodeData, void(Buffer::Instance& data, bool end_stream)); @@ -215,6 +232,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, std::list callbacks_{}; testing::NiceMock active_span_; testing::NiceMock tracing_config_; + bool is_grpc_request_{}; + bool stream_destroyed_{}; }; class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks, From c21e50764eeb37f79c1bdbab8504dfbe58e68fcf Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Wed, 9 May 2018 18:49:06 -0700 Subject: [PATCH 05/19] http: Add a missing include. Include for http/utility.h went missing in a master rebase, add it back. Signed-off-by: Jarno Rajahalme --- source/common/http/conn_manager_impl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 3af3a642286d8..aeda96c8f4db1 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -27,6 +27,7 @@ #include "common/common/linked_object.h" #include "common/http/conn_manager_config.h" #include "common/http/user_agent.h" +#include "common/http/utility.h" #include "common/request_info/request_info_impl.h" #include "common/tracing/http_tracer_impl.h" From 4c6b212c725c859dfcaee017d80210018ed18b53 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 08:32:39 -0700 Subject: [PATCH 06/19] http: Mark local complete also when sending local gRPC response. gRPC response has no body, so the headers encoder may be ending the stream as well as the body encoder. Signed-off-by: Jarno Rajahalme --- source/common/http/conn_manager_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 519c31ad95616..5f2bc38409677 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1431,14 +1431,15 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.response_headers_ = std::move(response_headers); parent_.response_encoder_->encodeHeaders( *parent_.response_headers_, end_stream); + parent_.state_.local_complete_ = end_stream; }, [&](Buffer::Instance& data, bool end_stream) -> void { parent_.response_encoder_->encodeData(data, end_stream); parent_.state_.local_complete_ = end_stream; - parent_.maybeEndEncode(end_stream); }, parent_.state_.destroyed_, Http::Code::InternalServerError, CodeUtility::toString(Http::Code::InternalServerError)); + parent_.maybeEndEncode(parent_.state_.local_complete_); } else { resetStream(); } From 090333917303dd1c0c3839e66d6f866f7e21f191 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 08:33:27 -0700 Subject: [PATCH 07/19] http: Add comment for clarification (needed) Signed-off-by: Jarno Rajahalme --- source/common/http/utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5f7bf112001b8..8c2fa932b59e1 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -367,7 +367,7 @@ void Utility::sendLocalReply( response_headers->insertContentLength().value(body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } - + // Somehow it's OK to encode headers even if 'is_reset' is true? encode_headers(std::move(response_headers), body_text.empty()); if (!body_text.empty() && !is_reset) { Buffer::OwnedImpl buffer(body_text); From ae9c40647adc381f92957c99bacbaccf63579ceb Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 09:11:19 -0700 Subject: [PATCH 08/19] http: Move gRPC utilities back to grpc/common hasGrpcContentType() and isGrpcResponseHeader() are after all not needed from Http::Utility, so move them back to Grpc::Common. Signed-off-by: Jarno Rajahalme --- source/common/grpc/common.cc | 37 +++++++++++++++++++ source/common/grpc/common.h | 13 +++++++ source/common/http/async_client_impl.cc | 3 +- source/common/http/conn_manager_impl.cc | 2 +- source/common/http/conn_manager_impl.h | 3 +- source/common/http/utility.cc | 37 ------------------- source/common/http/utility.h | 13 ------- source/common/router/router.cc | 2 +- source/common/upstream/health_checker_impl.cc | 2 +- .../json_transcoder_filter.cc | 2 +- test/common/grpc/common_test.cc | 16 ++++---- 11 files changed, 66 insertions(+), 64 deletions(-) diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index bc047340224f7..b3f36d52d0aaf 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -21,6 +21,43 @@ namespace Envoy { namespace Grpc { +bool Common::hasGrpcContentType(const Http::HeaderMap& headers) { + const Http::HeaderEntry* content_type = headers.ContentType(); + if (content_type == nullptr) { + return false; + } + // Fail fast if this is not gRPC. + if (!StringUtil::startsWith(content_type->value().c_str(), + Http::Headers::get().ContentTypeValues.Grpc)) { + return false; + } + // Exact match with application/grpc. This and the above case are likely the + // two most common encountered. + if (content_type->value() == Http::Headers::get().ContentTypeValues.Grpc.c_str()) { + return true; + } + // Prefix match with application/grpc+. It's not sufficient to rely on the an + // application/grpc prefix match, since there are related content types such as + // application/grpc-web. + if (content_type->value().size() > Http::Headers::get().ContentTypeValues.Grpc.size() && + content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+') { + return true; + } + // This must be something like application/grpc-web. + return false; +} + +bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream) { + if (end_stream) { + // Trailers-only response, only grpc-status is required. + return headers.GrpcStatus() != nullptr; + } + if (Http::Utility::getResponseStatus(headers) != enumToInt(Http::Code::OK)) { + return false; + } + return hasGrpcContentType(headers); +} + void Common::chargeStat(const Upstream::ClusterInfo& cluster, const std::string& protocol, const std::string& grpc_service, const std::string& grpc_method, const Http::HeaderEntry* grpc_status) { diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index 66a7e22d7069e..e203d6eb82cf8 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -27,6 +27,19 @@ class Exception : public EnvoyException { class Common { public: + /** + * @param headers the headers to parse. + * @return bool indicating whether content-type is gRPC. + */ + static bool hasGrpcContentType(const Http::HeaderMap& headers); + + /** + * @param headers the headers to parse. + * @param bool indicating wether the header is at end_stream. + * @return bool indicating whether the header is a gRPC reseponse header + */ + static bool isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream); + /** * Returns the GrpcStatus code from a given set of trailers, if present. * @param trailers the trailers to parse. diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index e13dab4ebb593..ca3200127e617 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -6,6 +6,7 @@ #include #include +#include "common/grpc/common.h" #include "common/http/utility.h" namespace Envoy { @@ -125,7 +126,7 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) { } void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) { - is_grpc_request_ = Utility::hasGrpcContentType(headers); + is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); headers.insertEnvoyInternalRequest().value().setReference( Headers::get().EnvoyInternalRequestValues.True); Utility::appendXff(headers, *parent_.config_.local_info_.address()); diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 5f2bc38409677..2fd882e493123 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1426,7 +1426,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.encoder_filters_streaming_ = true; stopped_ = false; - Http::Utility::sendLocalReply(Http::Utility::hasGrpcContentType(*parent_.request_headers_), + Http::Utility::sendLocalReply(Grpc::Common::hasGrpcContentType(*parent_.request_headers_), [&](HeaderMapPtr&& response_headers, bool end_stream) -> void { parent_.response_headers_ = std::move(response_headers); parent_.response_encoder_->encodeHeaders( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index aeda96c8f4db1..148071ce5d00e 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -25,6 +25,7 @@ #include "common/buffer/watermark_buffer.h" #include "common/common/linked_object.h" +#include "common/grpc/common.h" #include "common/http/conn_manager_config.h" #include "common/http/user_agent.h" #include "common/http/utility.h" @@ -192,7 +193,7 @@ class ConnectionManagerImpl : Logger::Loggable, uint32_t decoderBufferLimit() override { return parent_.buffer_limit_; } FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { - is_grpc_request_ = Http::Utility::hasGrpcContentType(headers); + is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); return handle_->decodeHeaders(headers, end_stream); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 8c2fa932b59e1..b20ebbe4ca3a7 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -186,43 +186,6 @@ bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { Http::Headers::get().UpgradeValues.WebSocket.c_str()))); } -bool Utility::hasGrpcContentType(const HeaderMap& headers) { - const HeaderEntry* content_type = headers.ContentType(); - if (content_type == nullptr) { - return false; - } - // Fail fast if this is not gRPC. - if (!StringUtil::startsWith(content_type->value().c_str(), - Headers::get().ContentTypeValues.Grpc)) { - return false; - } - // Exact match with application/grpc. This and the above case are likely the - // two most common encountered. - if (content_type->value() == Headers::get().ContentTypeValues.Grpc.c_str()) { - return true; - } - // Prefix match with application/grpc+. It's not sufficient to rely on the an - // application/grpc prefix match, since there are related content types such as - // application/grpc-web. - if (content_type->value().size() > Headers::get().ContentTypeValues.Grpc.size() && - content_type->value().c_str()[Headers::get().ContentTypeValues.Grpc.size()] == '+') { - return true; - } - // This must be something like application/grpc-web. - return false; -} - -bool Utility::isGrpcResponseHeader(const HeaderMap& headers, bool end_stream) { - if (end_stream) { - // Trailers-only response, only grpc-status is required. - return headers.GrpcStatus() != nullptr; - } - if (Utility::getResponseStatus(headers) != enumToInt(Code::OK)) { - return false; - } - return hasGrpcContentType(headers); -} - Grpc::Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) { // From // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 17e02fc510e75..ed9e44f2a2d1f 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -91,19 +91,6 @@ class Utility { */ static bool isWebSocketUpgradeRequest(const HeaderMap& headers); - /** - * @param headers the headers to parse. - * @return bool indicating whether content-type is gRPC. - */ - static bool hasGrpcContentType(const HeaderMap& headers); - - /** - * @param headers the headers to parse. - * @param bool indicating wether the header is at end_stream. - * @return bool indicating whether the header is a gRPC reseponse header - */ - static bool isGrpcResponseHeader(const HeaderMap& headers, bool end_stream); - /** * Returns the gRPC status code from a given HTTP response status code. Ordinarily, it is expected * that a 200 response is provided, but gRPC defines a mapping for intermediaries that are not diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 197303bc8dae3..98b10e213e2f6 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -175,7 +175,7 @@ void Filter::chargeUpstreamCode(Http::Code code, Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { downstream_headers_ = &headers; - grpc_request_ = Http::Utility::hasGrpcContentType(headers); + grpc_request_ = Grpc::Common::hasGrpcContentType(headers); // Only increment rq total stat if we actually decode headers here. This does not count requests // that get handled by earlier filters. diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 2d462d97a18c8..4ac6f01cff88b 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -366,7 +366,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( end_stream); return; } - if (!Http::Utility::hasGrpcContentType(*headers)) { + if (!Grpc::Common::hasGrpcContentType(*headers)) { onRpcComplete(Grpc::Status::GrpcStatus::Internal, "invalid gRPC content-type", false); return; } 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 aaf61ae382333..c3a517f219110 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 @@ -306,7 +306,7 @@ void JsonTranscoderFilter::setDecoderFilterCallbacks( Http::FilterHeadersStatus JsonTranscoderFilter::encodeHeaders(Http::HeaderMap& headers, bool end_stream) { - if (!Http::Utility::isGrpcResponseHeader(headers, end_stream)) { + if (!Grpc::Common::isGrpcResponseHeader(headers, end_stream)) { error_ = true; } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 662b4b8b5f511..6127c6e2f7740 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -142,11 +142,11 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { TEST(GrpcCommonTest, HasGrpcContentType) { { Http::TestHeaderMapImpl headers{}; - EXPECT_FALSE(Http::Utility::hasGrpcContentType(headers)); + EXPECT_FALSE(Grpc::Common::hasGrpcContentType(headers)); } auto isGrpcContentType = [](const std::string& s) { Http::TestHeaderMapImpl headers{{"content-type", s}}; - return Http::Utility::hasGrpcContentType(headers); + return Grpc::Common::hasGrpcContentType(headers); }; EXPECT_FALSE(isGrpcContentType("")); EXPECT_FALSE(isGrpcContentType("application/text")); @@ -160,18 +160,18 @@ TEST(GrpcCommonTest, HasGrpcContentType) { TEST(GrpcCommonTest, IsGrpcResponseHeader) { Http::TestHeaderMapImpl grpc_status_only{{":status", "500"}, {"grpc-status", "14"}}; - EXPECT_TRUE(Http::Utility::isGrpcResponseHeader(grpc_status_only, true)); - EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(grpc_status_only, false)); + EXPECT_TRUE(Grpc::Common::isGrpcResponseHeader(grpc_status_only, true)); + EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(grpc_status_only, false)); Http::TestHeaderMapImpl grpc_response_header{{":status", "200"}, {"content-type", "application/grpc"}}; - EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(grpc_response_header, true)); - EXPECT_TRUE(Http::Utility::isGrpcResponseHeader(grpc_response_header, false)); + EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(grpc_response_header, true)); + EXPECT_TRUE(Grpc::Common::isGrpcResponseHeader(grpc_response_header, false)); Http::TestHeaderMapImpl json_response_header{{":status", "200"}, {"content-type", "application/json"}}; - EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(json_response_header, true)); - EXPECT_FALSE(Http::Utility::isGrpcResponseHeader(json_response_header, false)); + EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(json_response_header, true)); + EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(json_response_header, false)); } TEST(GrpcCommonTest, ValidateResponse) { From 52f9baf647b003a9c94727d81efc9347f8da6f54 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 09:51:46 -0700 Subject: [PATCH 09/19] test/grpc: Revert unnecessary namespace prefix changes. Cleaning up the PR by reverting unnecessary changes. Signed-off-by: Jarno Rajahalme --- test/common/grpc/common_test.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 6127c6e2f7740..b21c50c4b79c9 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -142,11 +142,11 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { TEST(GrpcCommonTest, HasGrpcContentType) { { Http::TestHeaderMapImpl headers{}; - EXPECT_FALSE(Grpc::Common::hasGrpcContentType(headers)); + EXPECT_FALSE(Common::hasGrpcContentType(headers)); } auto isGrpcContentType = [](const std::string& s) { Http::TestHeaderMapImpl headers{{"content-type", s}}; - return Grpc::Common::hasGrpcContentType(headers); + return Common::hasGrpcContentType(headers); }; EXPECT_FALSE(isGrpcContentType("")); EXPECT_FALSE(isGrpcContentType("application/text")); @@ -160,18 +160,18 @@ TEST(GrpcCommonTest, HasGrpcContentType) { TEST(GrpcCommonTest, IsGrpcResponseHeader) { Http::TestHeaderMapImpl grpc_status_only{{":status", "500"}, {"grpc-status", "14"}}; - EXPECT_TRUE(Grpc::Common::isGrpcResponseHeader(grpc_status_only, true)); - EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(grpc_status_only, false)); + EXPECT_TRUE(Common::isGrpcResponseHeader(grpc_status_only, true)); + EXPECT_FALSE(Common::isGrpcResponseHeader(grpc_status_only, false)); Http::TestHeaderMapImpl grpc_response_header{{":status", "200"}, {"content-type", "application/grpc"}}; - EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(grpc_response_header, true)); - EXPECT_TRUE(Grpc::Common::isGrpcResponseHeader(grpc_response_header, false)); + EXPECT_FALSE(Common::isGrpcResponseHeader(grpc_response_header, true)); + EXPECT_TRUE(Common::isGrpcResponseHeader(grpc_response_header, false)); Http::TestHeaderMapImpl json_response_header{{":status", "200"}, {"content-type", "application/json"}}; - EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(json_response_header, true)); - EXPECT_FALSE(Grpc::Common::isGrpcResponseHeader(json_response_header, false)); + EXPECT_FALSE(Common::isGrpcResponseHeader(json_response_header, true)); + EXPECT_FALSE(Common::isGrpcResponseHeader(json_response_header, false)); } TEST(GrpcCommonTest, ValidateResponse) { From 04f23a6496f6acaf77f398b75c68a189ce952b7f Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 11:42:06 -0700 Subject: [PATCH 10/19] http: Use sendLocalReply() for all local replies in ConnectionManagerImpl. Move sendLocalReply base implementation to ActiveStream, so that it is usable also from ActiveStream. Use sendLocaslReply() instead of encoding headers directly so that we get gRPC responses in all cases when the request was gRPC. Signed-off-by: Jarno Rajahalme --- source/common/http/conn_manager_impl.cc | 44 ++++++++++++++++--------- source/common/http/conn_manager_impl.h | 14 +++----- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index b223b222225c3..5ad6fe2a311a2 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -473,10 +473,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // The protocol may have shifted in the HTTP/1.0 case so reset it. request_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { - // Send "Upgrade Required" if HTTP/1.0 support is not expliictly configured on. - HeaderMapImpl headers{ - {Headers::get().Status, std::to_string(enumToInt(Code::UpgradeRequired))}}; - encodeHeaders(nullptr, headers, true); + // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on. + sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), + Code::UpgradeRequired, "", nullptr); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -499,8 +498,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.config_.http1Settings().default_host_for_http_10_); } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. - HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::BadRequest))}}; - encodeHeaders(nullptr, headers, true); + sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, + "", nullptr); return; } } @@ -513,9 +512,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // header size http_parser and nghttp2 will allow, down to 16k or 8k for // envoy users who do not wish to proxy large headers. if (request_headers_->byteSize() > (60 * 1024)) { - HeaderMapImpl headers{ - {Headers::get().Status, std::to_string(enumToInt(Code::RequestHeaderFieldsTooLarge))}}; - encodeHeaders(nullptr, headers, true); + sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), + Code::RequestHeaderFieldsTooLarge, "", nullptr); return; } @@ -526,8 +524,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // don't support that currently. if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') { connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); - HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::NotFound))}}; - encodeHeaders(nullptr, headers, true); + sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", + nullptr); return; } @@ -570,8 +568,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else if (websocket_requested) { // Do not allow WebSocket upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); - HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::Forbidden))}}; - encodeHeaders(nullptr, headers, true); + sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, + "", nullptr); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -821,6 +819,23 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { cached_route_ = std::move(route); } +void ConnectionManagerImpl::ActiveStream::sendLocalReply(ActiveStreamEncoderFilter* filter, + bool is_grpc_request, Code code, + const std::string& body, + std::function modify_headers) { + Utility::sendLocalReply( + is_grpc_request, + [this, filter, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (headers != nullptr && modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + encodeHeaders(filter, *response_headers_, end_stream); + }, + [this, filter](Buffer::Instance& data, bool end_stream) -> void { encodeData(filter, data, end_stream); }, + state_.destroyed_, code, body); +} + void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( ActiveStreamEncoderFilter* filter, HeaderMap& headers) { ASSERT(connection_manager_.config_.proxy100Continue()); @@ -1316,8 +1331,7 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { onDecoderFilterAboveWriteBufferHighWatermark(); } else { parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); - sendLocalReply(Http::Code::PayloadTooLarge, CodeUtility::toString(Http::Code::PayloadTooLarge), - nullptr); + sendLocalReply(Code::PayloadTooLarge, CodeUtility::toString(Code::PayloadTooLarge), nullptr); } } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 148071ce5d00e..05dd1dea6bbc4 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -168,16 +168,7 @@ class ConnectionManagerImpl : Logger::Loggable, } void sendLocalReply(Code code, const std::string& body, std::function modify_headers) override { - Utility::sendLocalReply( - is_grpc_request_, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { - modify_headers(*headers); - } - encodeHeaders(std::move(headers), end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - parent_.state_.destroyed_, code, body); + parent_.sendLocalReply(nullptr, is_grpc_request_, code, body, modify_headers); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -278,6 +269,9 @@ class ConnectionManagerImpl : Logger::Loggable, void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers); void maybeEndDecode(bool end_stream); void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); + void sendLocalReply(ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, + const std::string& body, + std::function modify_headers); void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); void encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream); void encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instance& data, bool end_stream); From a47b2a16419e21f9827597d0b77bd813f72fe354 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 11:43:01 -0700 Subject: [PATCH 11/19] filters: Use sendLocalReply() decoder callback for local replies. Signed-off-by: Jarno Rajahalme --- .../extensions/filters/http/ext_authz/ext_authz.cc | 13 +------------ .../filters/http/health_check/health_check.cc | 11 +++-------- .../extensions/filters/http/ratelimit/ratelimit.cc | 13 +------------ 3 files changed, 5 insertions(+), 32 deletions(-) diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 70a1156569e5f..b8d4a5c27a85d 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -19,16 +19,6 @@ namespace Extensions { namespace HttpFilters { namespace ExtAuthz { -namespace { - -const Http::HeaderMap* getDeniedHeader() { - static const Http::HeaderMap* header_map = new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::Forbidden))}}; - return header_map; -} - -} // namespace - void Filter::initiateCall(const Http::HeaderMap& headers) { Router::RouteConstSharedPtr route = callbacks_->route(); if (route == nullptr || route->routeEntry() == nullptr) { @@ -112,8 +102,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::CheckStatus status) { // if there is an error contacting the service. if (status == CheckStatus::Denied || (status == CheckStatus::Error && !config_->failureModeAllow())) { - Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl(*getDeniedHeader())}; - callbacks_->encodeHeaders(std::move(response_headers), true); + callbacks_->sendLocalReply(Http::Code::Forbidden, "", nullptr); callbacks_->requestInfo().setResponseFlag( RequestInfo::ResponseFlag::UnauthorizedExternalService); } else { diff --git a/source/extensions/filters/http/health_check/health_check.cc b/source/extensions/filters/http/health_check/health_check.cc index e992280c2727b..88fa02b1f6920 100644 --- a/source/extensions/filters/http/health_check/health_check.cc +++ b/source/extensions/filters/http/health_check/health_check.cc @@ -89,13 +89,11 @@ Http::FilterHeadersStatus HealthCheckFilter::encodeHeaders(Http::HeaderMap& head void HealthCheckFilter::onComplete() { ASSERT(handling_); - Http::HeaderMapPtr headers; + Http::Code final_status = Http::Code::OK; if (context_.healthCheckFailed()) { callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck); - headers.reset(new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::ServiceUnavailable))}}); + final_status = Http::Code::ServiceUnavailable; } else { - Http::Code final_status = Http::Code::OK; if (cache_manager_) { final_status = cache_manager_->getCachedResponseCode(); } else if (cluster_min_healthy_percentages_ != nullptr && @@ -137,12 +135,9 @@ void HealthCheckFilter::onComplete() { if (!Http::CodeUtility::is2xx(enumToInt(final_status))) { callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck); } - - headers.reset(new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(final_status))}}); } - callbacks_->encodeHeaders(std::move(headers), true); + callbacks_->sendLocalReply(final_status, "", nullptr); } } // namespace HealthCheck diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 67ae791167042..d63f0ca6cdd36 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -16,16 +16,6 @@ namespace Extensions { namespace HttpFilters { namespace RateLimitFilter { -namespace { - -static const Http::HeaderMap* getTooManyRequestsHeader() { - static const Http::HeaderMap* header_map = new Http::HeaderMapImpl{ - {Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::TooManyRequests))}}; - return header_map; -} - -} // namespace - void Filter::initiateCall(const Http::HeaderMap& headers) { bool is_internal_request = headers.EnvoyInternalRequest() && (headers.EnvoyInternalRequest()->value() == "true"); @@ -133,8 +123,7 @@ void Filter::complete(RateLimit::LimitStatus status) { if (status == RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.http_filter_enforcing", 100)) { state_ = State::Responded; - Http::HeaderMapPtr response_headers{new Http::HeaderMapImpl(*getTooManyRequestsHeader())}; - callbacks_->encodeHeaders(std::move(response_headers), true); + callbacks_->sendLocalReply(Http::Code::TooManyRequests, "", nullptr); callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimited); } else if (!initiating_call_) { callbacks_->continueDecoding(); From 6bd512ae184d6f8b9a9c778a71ceaf599f534344 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 11 May 2018 13:08:52 -0700 Subject: [PATCH 12/19] conn_manager_impl: Fix format. Signed-off-by: Jarno Rajahalme --- source/common/http/conn_manager_impl.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 5ad6fe2a311a2..8255fd5d8bc82 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -819,20 +819,21 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { cached_route_ = std::move(route); } -void ConnectionManagerImpl::ActiveStream::sendLocalReply(ActiveStreamEncoderFilter* filter, - bool is_grpc_request, Code code, - const std::string& body, - std::function modify_headers) { +void ConnectionManagerImpl::ActiveStream::sendLocalReply( + ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, const std::string& body, + std::function modify_headers) { Utility::sendLocalReply( is_grpc_request, [this, filter, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { if (headers != nullptr && modify_headers != nullptr) { modify_headers(*headers); } - response_headers_ = std::move(headers); + response_headers_ = std::move(headers); encodeHeaders(filter, *response_headers_, end_stream); }, - [this, filter](Buffer::Instance& data, bool end_stream) -> void { encodeData(filter, data, end_stream); }, + [this, filter](Buffer::Instance& data, bool end_stream) -> void { + encodeData(filter, data, end_stream); + }, state_.destroyed_, code, body); } From 26aa3353623637ed828835af536cec341b84d93a Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Mon, 14 May 2018 14:50:11 -0700 Subject: [PATCH 13/19] filters: Remove unnecessary tracking of stream status. Now that sendLocalReply() callback takes care of stream status tracking itself, such tracking is not needed in the filters any more. Suggested-by: Matt Klein Signed-off-by: Jarno Rajahalme --- source/extensions/filters/http/buffer/buffer_filter.cc | 5 +---- source/extensions/filters/http/buffer/buffer_filter.h | 1 - source/extensions/filters/http/fault/fault_filter.cc | 5 +---- source/extensions/filters/http/fault/fault_filter.h | 1 - .../http/grpc_json_transcoder/json_transcoder_filter.h | 3 +-- source/extensions/filters/http/grpc_web/grpc_web_filter.h | 3 +-- 6 files changed, 4 insertions(+), 14 deletions(-) diff --git a/source/extensions/filters/http/buffer/buffer_filter.cc b/source/extensions/filters/http/buffer/buffer_filter.cc index 1f79bec9abd20..36b45f31fd938 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.cc +++ b/source/extensions/filters/http/buffer/buffer_filter.cc @@ -107,10 +107,7 @@ BufferFilterStats BufferFilter::generateStats(const std::string& prefix, Stats:: return {ALL_BUFFER_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; } -void BufferFilter::onDestroy() { - resetInternalState(); - stream_destroyed_ = true; -} +void BufferFilter::onDestroy() { resetInternalState(); } void BufferFilter::onRequestTimeout() { callbacks_->sendLocalReply(Http::Code::RequestTimeout, "buffer request timeout", nullptr); diff --git a/source/extensions/filters/http/buffer/buffer_filter.h b/source/extensions/filters/http/buffer/buffer_filter.h index 277a0f50f9d04..75228fe191914 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.h +++ b/source/extensions/filters/http/buffer/buffer_filter.h @@ -92,7 +92,6 @@ class BufferFilter : public Http::StreamDecoderFilter { const BufferFilterSettings* settings_; Http::StreamDecoderFilterCallbacks* callbacks_{}; Event::TimerPtr request_timeout_; - bool stream_destroyed_{}; bool config_initialized_{}; }; diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index bf225706c0982..9669b8b041d71 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -231,10 +231,7 @@ FaultFilterStats FaultFilterConfig::generateStats(const std::string& prefix, Sta return {ALL_FAULT_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; } -void FaultFilter::onDestroy() { - resetTimerState(); - stream_destroyed_ = true; -} +void FaultFilter::onDestroy() { resetTimerState(); } void FaultFilter::postDelayInjection() { resetTimerState(); diff --git a/source/extensions/filters/http/fault/fault_filter.h b/source/extensions/filters/http/fault/fault_filter.h index 8c4336bf4362c..7559a30038375 100644 --- a/source/extensions/filters/http/fault/fault_filter.h +++ b/source/extensions/filters/http/fault/fault_filter.h @@ -123,7 +123,6 @@ class FaultFilter : public Http::StreamDecoderFilter { Http::StreamDecoderFilterCallbacks* callbacks_{}; Event::TimerPtr delay_timer_; std::string downstream_cluster_{}; - bool stream_destroyed_{}; const FaultSettings* fault_settings_; std::string downstream_cluster_delay_percent_key_{}; 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 1c70843ed304b..e8544ea2600ff 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 @@ -113,7 +113,7 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< void setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callbacks) override; // Http::StreamFilterBase - void onDestroy() override { stream_reset_ = true; } + void onDestroy() override {} private: bool readToBuffer(Protobuf::io::ZeroCopyInputStream& stream, Buffer::Instance& data); @@ -128,7 +128,6 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable< Http::HeaderMap* response_headers_{nullptr}; bool error_{false}; - bool stream_reset_{false}; }; } // namespace GrpcJsonTranscoder diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.h b/source/extensions/filters/http/grpc_web/grpc_web_filter.h index 022c6ea853caa..f33cc8dca412d 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.h +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.h @@ -23,7 +23,7 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { virtual ~GrpcWebFilter(){}; // Http::StreamFilterBase - void onDestroy() override { stream_destroyed_ = true; }; + void onDestroy() override{}; // Implements StreamDecoderFilter. Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override; @@ -67,7 +67,6 @@ class GrpcWebFilter : public Http::StreamFilter, NonCopyable { std::string grpc_service_; std::string grpc_method_; bool do_stat_tracking_{}; - bool stream_destroyed_{}; bool is_grpc_web_request_{}; }; From dea2787119267025dde4e4130759d9629e0c3827 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Mon, 14 May 2018 14:20:56 -0700 Subject: [PATCH 14/19] http: Clarify comments on sendLocalReply() Clarify the comments regarding the referenced 'is_reset' parameter to sendLocalReply(). ASSERT that it is initially false. Suggested-by: Matt Klein Signed-off-by: Jarno Rajahalme --- source/common/http/utility.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index b20ebbe4ca3a7..baab3080a5a5f 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -309,6 +309,8 @@ void Utility::sendLocalReply( bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, const std::string& body_text) { + // encode_headers() may reset the stream, so the stream must not be reset before calling it. + ASSERT(!is_reset); // Respond with a gRPC trailers-only response if the request is gRPC if (is_grpc) { HeaderMapPtr response_headers{ @@ -330,8 +332,8 @@ void Utility::sendLocalReply( response_headers->insertContentLength().value(body_text.size()); response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text); } - // Somehow it's OK to encode headers even if 'is_reset' is true? encode_headers(std::move(response_headers), body_text.empty()); + // encode_headers()) may have changed the referenced is_reset so we need to test it if (!body_text.empty() && !is_reset) { Buffer::OwnedImpl buffer(body_text); encode_data(buffer, true); From 06b22a784f19cdb3a6f1349ecd460a4132b33af7 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Mon, 14 May 2018 14:47:08 -0700 Subject: [PATCH 15/19] conn_manager_impl: Remove nullptr parameter and add TODOs. Remove the first parameter from sendLocalReply() as it currently is always nullptr. Add comments on the endoceHeaders() and decodeHeaders() calls to note about the filter that should start encoding. Signed-off-by: Jarno Rajahalme --- source/common/http/conn_manager_impl.cc | 47 +++++++++++++------------ source/common/http/conn_manager_impl.h | 8 +++-- 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 8255fd5d8bc82..fc833904360af 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -474,8 +474,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, request_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on. - sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), - Code::UpgradeRequired, "", nullptr); + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::UpgradeRequired, "", + nullptr); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -498,8 +498,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, connection_manager_.config_.http1Settings().default_host_for_http_10_); } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. - sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, - "", nullptr); + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", + nullptr); return; } } @@ -512,7 +512,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // header size http_parser and nghttp2 will allow, down to 16k or 8k for // envoy users who do not wish to proxy large headers. if (request_headers_->byteSize() > (60 * 1024)) { - sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::RequestHeaderFieldsTooLarge, "", nullptr); return; } @@ -524,7 +524,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // don't support that currently. if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') { connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); - sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr); return; } @@ -568,8 +568,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else if (websocket_requested) { // Do not allow WebSocket upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); - sendLocalReply(nullptr, Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, - "", nullptr); + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", + nullptr); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -820,21 +820,24 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } void ConnectionManagerImpl::ActiveStream::sendLocalReply( - ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, const std::string& body, + bool is_grpc_request, Code code, const std::string& body, std::function modify_headers) { - Utility::sendLocalReply( - is_grpc_request, - [this, filter, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - encodeHeaders(filter, *response_headers_, end_stream); - }, - [this, filter](Buffer::Instance& data, bool end_stream) -> void { - encodeData(filter, data, end_stream); - }, - state_.destroyed_, code, body); + Utility::sendLocalReply(is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (headers != nullptr && modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream); + }, + state_.destroyed_, code, body); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 05dd1dea6bbc4..9cefbfd254ef4 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -168,7 +168,7 @@ class ConnectionManagerImpl : Logger::Loggable, } void sendLocalReply(Code code, const std::string& body, std::function modify_headers) override { - parent_.sendLocalReply(nullptr, is_grpc_request_, code, body, modify_headers); + parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -183,6 +183,9 @@ class ConnectionManagerImpl : Logger::Loggable, void setDecoderBufferLimit(uint32_t limit) override { parent_.setBufferLimit(limit); } uint32_t decoderBufferLimit() override { return parent_.buffer_limit_; } + // Each decoder filter instance checks if the request passed to the filter is gRPC + // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders() + // called here may change the content type, so we must check it before the call. FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) { is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers); return handle_->decodeHeaders(headers, end_stream); @@ -269,8 +272,7 @@ class ConnectionManagerImpl : Logger::Loggable, void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers); void maybeEndDecode(bool end_stream); void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); - void sendLocalReply(ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, - const std::string& body, + void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, std::function modify_headers); void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); void encodeHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers, bool end_stream); From e125f63dfd867a38912fd2497da40860f6208778 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Mon, 14 May 2018 14:49:40 -0700 Subject: [PATCH 16/19] filter: Add comment about gRPC encoding of local responses. Add comments to sendLocalReply() about gRPC encoding of the sent replies. Signed-off-by: Jarno Rajahalme --- include/envoy/http/filter.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 0980b16ab62f5..b5e7a4e13c44a 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -191,7 +191,11 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; /** - * Create a locally generated response using the provided lambdas. + * Create a locally generated response using the provided response_code and body_text paramewters. + * If the request was a gRPC request the local reply will be encoded as a gRPC response with a 200 + * HTTP response code and grpc-status and grpc-message headers mapped from the provided + * parameters. + * * @param response_code supplies the HTTP response code. * @param body_text supplies the optional body text which is sent using the text/plain content * type. From 2860297416dad4ea4da1b59a1ef38bb5ba286935 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Mon, 14 May 2018 15:26:37 -0700 Subject: [PATCH 17/19] common/grpc: Add status_lib Keep gRPC status code conversion utilities in common/grpc, but put them into a separate bazel lib from common_lib to avoid circular bazel dependency between common/http/utility_lib and common/grpc/common_lib. Signed-off-by: Jarno Rajahalme --- source/common/grpc/BUILD | 11 ++- source/common/grpc/async_client_impl.cc | 2 +- source/common/grpc/common.h | 1 + source/common/grpc/status.cc | 89 ++++++++++++++++++ source/common/grpc/status.h | 32 +++++++ source/common/http/BUILD | 2 +- source/common/http/utility.cc | 93 ++----------------- source/common/http/utility.h | 15 --- source/common/router/router.cc | 4 +- source/common/upstream/health_checker_impl.cc | 2 +- .../json_transcoder_filter.cc | 2 +- test/common/grpc/common_test.cc | 4 +- .../grpc/grpc_client_integration_test.cc | 2 +- 13 files changed, 147 insertions(+), 112 deletions(-) create mode 100644 source/common/grpc/status.cc create mode 100644 source/common/grpc/status.h diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index de6cc5264fd19..956ac2552c252 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -46,13 +46,21 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "status_lib", + srcs = ["status.cc"], + hdrs = ["status.h"], + deps = [ + "//include/envoy/grpc:status", + ], +) + envoy_cc_library( name = "common_lib", srcs = ["common.cc"], hdrs = ["common.h"], external_deps = ["abseil_optional"], deps = [ - "//include/envoy/grpc:status", "//include/envoy/http:header_map_interface", "//include/envoy/http:message_interface", "//include/envoy/stats:stats_interface", @@ -64,6 +72,7 @@ envoy_cc_library( "//source/common/common:enum_to_int", "//source/common/common:macros", "//source/common/common:utility_lib", + "//source/common/grpc:status_lib", "//source/common/http:filter_utility_lib", "//source/common/http:headers_lib", "//source/common/http:message_lib", diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index eebac7d035a1d..7c54c37c3e2bd 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -106,7 +106,7 @@ void AsyncStreamImpl::onHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { } // Technically this should be // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md - // as given by Http::Utility::httpToGrpcStatus(), but the Google gRPC client treats + // as given by Grpc::Utility::httpToGrpcStatus(), but the Google gRPC client treats // this as GrpcStatus::Canceled. streamError(Status::GrpcStatus::Canceled); return; diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index e203d6eb82cf8..5e66dd601d8ab 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -10,6 +10,7 @@ #include "envoy/http/message.h" #include "envoy/stats/stats.h" +#include "common/grpc/status.h" #include "common/protobuf/protobuf.h" #include "absl/types/optional.h" diff --git a/source/common/grpc/status.cc b/source/common/grpc/status.cc new file mode 100644 index 0000000000000..70fdcce027113 --- /dev/null +++ b/source/common/grpc/status.cc @@ -0,0 +1,89 @@ +#include "common/grpc/status.h" + +namespace Envoy { +namespace Grpc { + +Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) { + // From + // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. + switch (http_response_status) { + case 400: + return Status::GrpcStatus::Internal; + case 401: + return Status::GrpcStatus::Unauthenticated; + case 403: + return Status::GrpcStatus::PermissionDenied; + case 404: + return Status::GrpcStatus::Unimplemented; + case 429: + case 502: + case 503: + case 504: + return Status::GrpcStatus::Unavailable; + default: + return Status::GrpcStatus::Unknown; + } +} + +uint64_t Utility::grpcToHttpStatus(Status::GrpcStatus grpc_status) { + // From https://cloud.google.com/apis/design/errors#handling_errors. + switch (grpc_status) { + case Status::GrpcStatus::Ok: + return 200; + case Status::GrpcStatus::Canceled: + // Client closed request. + return 499; + case Status::GrpcStatus::Unknown: + // Internal server error. + return 500; + case Status::GrpcStatus::InvalidArgument: + // Bad request. + return 400; + case Status::GrpcStatus::DeadlineExceeded: + // Gateway Time-out. + return 504; + case Status::GrpcStatus::NotFound: + // Not found. + return 404; + case Status::GrpcStatus::AlreadyExists: + // Conflict. + return 409; + case Status::GrpcStatus::PermissionDenied: + // Forbidden. + return 403; + case Status::GrpcStatus::ResourceExhausted: + // Too many requests. + return 429; + case Status::GrpcStatus::FailedPrecondition: + // Bad request. + return 400; + case Status::GrpcStatus::Aborted: + // Conflict. + return 409; + case Status::GrpcStatus::OutOfRange: + // Bad request. + return 400; + case Status::GrpcStatus::Unimplemented: + // Not implemented. + return 501; + case Status::GrpcStatus::Internal: + // Internal server error. + return 500; + case Status::GrpcStatus::Unavailable: + // Service unavailable. + return 503; + case Status::GrpcStatus::DataLoss: + // Internal server error. + return 500; + case Status::GrpcStatus::Unauthenticated: + // Unauthorized. + return 401; + case Status::GrpcStatus::InvalidCode: + default: + // Internal server error. + return 500; + } +} + +} // namespace Grpc +} // namespace Envoy diff --git a/source/common/grpc/status.h b/source/common/grpc/status.h new file mode 100644 index 0000000000000..6355e002c2a93 --- /dev/null +++ b/source/common/grpc/status.h @@ -0,0 +1,32 @@ +#pragma once + +#include + +#include "envoy/grpc/status.h" + +namespace Envoy { +namespace Grpc { + +/** + * Grpc::Status utilities. + */ +class Utility { +public: + /** + * Returns the gRPC status code from a given HTTP response status code. Ordinarily, it is expected + * that a 200 response is provided, but gRPC defines a mapping for intermediaries that are not + * gRPC aware, see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. + * @param http_response_status HTTP status code. + * @return Status::GrpcStatus corresponding gRPC status code. + */ + static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); + + /** + * @param grpc_status gRPC status from grpc-status header. + * @return uint64_t the canonical HTTP status code corresponding to a gRPC status code. + */ + static uint64_t grpcToHttpStatus(Status::GrpcStatus grpc_status); +}; + +} // namespace Grpc +} // namespace Envoy diff --git a/source/common/http/BUILD b/source/common/http/BUILD index f320332846833..a52b06f93b50f 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -239,7 +239,6 @@ envoy_cc_library( ":exception_lib", ":header_map_lib", ":headers_lib", - "//include/envoy/grpc:status", "//include/envoy/http:codes_interface", "//include/envoy/http:filter_interface", "//include/envoy/http:header_map_interface", @@ -248,6 +247,7 @@ envoy_cc_library( "//source/common/common:empty_string", "//source/common/common:enum_to_int", "//source/common/common:utility_lib", + "//source/common/grpc:status_lib", "//source/common/json:json_loader_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index baab3080a5a5f..c4dae19e254cf 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -12,6 +12,7 @@ #include "common/common/enum_to_int.h" #include "common/common/fmt.h" #include "common/common/utility.h" +#include "common/grpc/status.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" @@ -186,88 +187,6 @@ bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { Http::Headers::get().UpgradeValues.WebSocket.c_str()))); } -Grpc::Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) { - // From - // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. - switch (http_response_status) { - case 400: - return Grpc::Status::GrpcStatus::Internal; - case 401: - return Grpc::Status::GrpcStatus::Unauthenticated; - case 403: - return Grpc::Status::GrpcStatus::PermissionDenied; - case 404: - return Grpc::Status::GrpcStatus::Unimplemented; - case 429: - case 502: - case 503: - case 504: - return Grpc::Status::GrpcStatus::Unavailable; - default: - return Grpc::Status::GrpcStatus::Unknown; - } -} - -uint64_t Utility::grpcToHttpStatus(Grpc::Status::GrpcStatus grpc_status) { - // From https://cloud.google.com/apis/design/errors#handling_errors. - switch (grpc_status) { - case Grpc::Status::GrpcStatus::Ok: - return 200; - case Grpc::Status::GrpcStatus::Canceled: - // Client closed request. - return 499; - case Grpc::Status::GrpcStatus::Unknown: - // Internal server error. - return 500; - case Grpc::Status::GrpcStatus::InvalidArgument: - // Bad request. - return 400; - case Grpc::Status::GrpcStatus::DeadlineExceeded: - // Gateway Time-out. - return 504; - case Grpc::Status::GrpcStatus::NotFound: - // Not found. - return 404; - case Grpc::Status::GrpcStatus::AlreadyExists: - // Conflict. - return 409; - case Grpc::Status::GrpcStatus::PermissionDenied: - // Forbidden. - return 403; - case Grpc::Status::GrpcStatus::ResourceExhausted: - // Too many requests. - return 429; - case Grpc::Status::GrpcStatus::FailedPrecondition: - // Bad request. - return 400; - case Grpc::Status::GrpcStatus::Aborted: - // Conflict. - return 409; - case Grpc::Status::GrpcStatus::OutOfRange: - // Bad request. - return 400; - case Grpc::Status::GrpcStatus::Unimplemented: - // Not implemented. - return 501; - case Grpc::Status::GrpcStatus::Internal: - // Internal server error. - return 500; - case Grpc::Status::GrpcStatus::Unavailable: - // Service unavailable. - return 503; - case Grpc::Status::GrpcStatus::DataLoss: - // Internal server error. - return 500; - case Grpc::Status::GrpcStatus::Unauthenticated: - // Unauthorized. - return 401; - case Grpc::Status::GrpcStatus::InvalidCode: - default: - // Internal server error. - return 500; - } -} - Http2Settings Utility::parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOptions& config) { Http2Settings ret; @@ -313,11 +232,11 @@ void Utility::sendLocalReply( ASSERT(!is_reset); // Respond with a gRPC trailers-only response if the request is gRPC if (is_grpc) { - HeaderMapPtr response_headers{ - new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(Code::OK))}, - {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc}, - {Headers::get().GrpcStatus, - std::to_string(enumToInt(httpToGrpcStatus(enumToInt(response_code))))}}}; + HeaderMapPtr response_headers{new HeaderMapImpl{ + {Headers::get().Status, std::to_string(enumToInt(Code::OK))}, + {Headers::get().ContentType, Headers::get().ContentTypeValues.Grpc}, + {Headers::get().GrpcStatus, + std::to_string(enumToInt(Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))}}}; if (!body_text.empty()) { // TODO: GrpcMessage should be percent-encoded response_headers->insertGrpcMessage().value(body_text); diff --git a/source/common/http/utility.h b/source/common/http/utility.h index ed9e44f2a2d1f..ca6802b9af493 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -91,21 +91,6 @@ class Utility { */ static bool isWebSocketUpgradeRequest(const HeaderMap& headers); - /** - * Returns the gRPC status code from a given HTTP response status code. Ordinarily, it is expected - * that a 200 response is provided, but gRPC defines a mapping for intermediaries that are not - * gRPC aware, see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md. - * @param http_response_status HTTP status code. - * @return Status::GrpcStatus corresponding gRPC status code. - */ - static Grpc::Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); - - /** - * @param grpc_status gRPC status from grpc-status header. - * @return uint64_t the canonical HTTP status code corresponding to a gRPC status code. - */ - static uint64_t grpcToHttpStatus(Grpc::Status::GrpcStatus grpc_status); - /** * @return Http2Settings An Http2Settings populated from the * envoy::api::v2::core::Http2ProtocolOptions config. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 32a0ce9be42c3..44358004d9fe0 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -532,7 +532,7 @@ void Filter::handleNon5xxResponseHeaders(const Http::HeaderMap& headers, bool en if (end_stream) { absl::optional grpc_status = Grpc::Common::getGrpcStatus(headers); if (grpc_status && - !Http::CodeUtility::is5xx(Http::Utility::grpcToHttpStatus(grpc_status.value()))) { + !Http::CodeUtility::is5xx(Grpc::Utility::grpcToHttpStatus(grpc_status.value()))) { upstream_request_->upstream_host_->stats().rq_success_.inc(); } else { upstream_request_->upstream_host_->stats().rq_error_.inc(); @@ -639,7 +639,7 @@ void Filter::onUpstreamTrailers(Http::HeaderMapPtr&& trailers) { if (upstream_request_->grpc_rq_success_deferred_) { absl::optional grpc_status = Grpc::Common::getGrpcStatus(*trailers); if (grpc_status && - !Http::CodeUtility::is5xx(Http::Utility::grpcToHttpStatus(grpc_status.value()))) { + !Http::CodeUtility::is5xx(Grpc::Utility::grpcToHttpStatus(grpc_status.value()))) { upstream_request_->upstream_host_->stats().rq_success_.inc(); } else { upstream_request_->upstream_host_->stats().rq_error_.inc(); diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 4ac6f01cff88b..024ed6cc22b1b 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -362,7 +362,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( return; } } - onRpcComplete(Http::Utility::httpToGrpcStatus(http_response_status), "non-200 HTTP response", + onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status), "non-200 HTTP response", end_stream); return; } 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 c3a517f219110..573d9eecb273d 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 @@ -377,7 +377,7 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& if (!grpc_status || grpc_status.value() == Grpc::Status::GrpcStatus::InvalidCode) { response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); } else { - response_headers_->Status()->value(Http::Utility::grpcToHttpStatus(grpc_status.value())); + response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status.value())); response_headers_->insertGrpcStatus().value(enumToInt(grpc_status.value())); } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index b21c50c4b79c9..c5231187033f2 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -122,7 +122,7 @@ TEST(GrpcCommonTest, GrpcToHttpStatus) { {Status::GrpcStatus::InvalidCode, 500}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Http::Utility::grpcToHttpStatus(test_case.first)); + EXPECT_EQ(test_case.second, Grpc::Utility::grpcToHttpStatus(test_case.first)); } } @@ -135,7 +135,7 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { {500, Status::GrpcStatus::Unknown}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Http::Utility::httpToGrpcStatus(test_case.first)); + EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first)); } } diff --git a/test/common/grpc/grpc_client_integration_test.cc b/test/common/grpc/grpc_client_integration_test.cc index e710ff7245085..bfac4dd7f03e5 100644 --- a/test/common/grpc/grpc_client_integration_test.cc +++ b/test/common/grpc/grpc_client_integration_test.cc @@ -489,7 +489,7 @@ TEST_P(GrpcClientIntegrationTest, HttpNon200Status) { stream->expectTrailingMetadata(empty_metadata_); // Technically this should be // https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md - // as given by Http::Utility::httpToGrpcStatus(), but the Google gRPC client treats + // as given by Grpc::Utility::httpToGrpcStatus(), but the Google gRPC client treats // this as GrpcStatus::Canceled. stream->expectGrpcStatus(Status::GrpcStatus::Canceled); stream->fake_stream_->encodeHeaders(reply_headers, true); From 74724fdf9c693f0823813a0898b62266ebb774a0 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Tue, 15 May 2018 07:58:43 -0700 Subject: [PATCH 18/19] test: http mock cleanup. Signed-off-by: Jarno Rajahalme --- test/mocks/http/mocks.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 0553a645309db..29d3222bd65d8 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -221,8 +221,6 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(continueDecoding, void()); MOCK_METHOD2(addDecodedData, void(Buffer::Instance& data, bool streaming)); MOCK_METHOD0(decodingBuffer, const Buffer::Instance*()); - MOCK_METHOD3(sendLocalReply_, void(Code code, const std::string& body, - std::function modify_headers)); MOCK_METHOD1(encode100ContinueHeaders_, void(HeaderMap& headers)); MOCK_METHOD2(encodeHeaders_, void(HeaderMap& headers, bool end_stream)); MOCK_METHOD2(encodeData, void(Buffer::Instance& data, bool end_stream)); From 2b57fc4f9cd240344330b0347ba0f3e7eee60fbe Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Tue, 15 May 2018 14:10:45 -0700 Subject: [PATCH 19/19] http: Final review comment fixes. Signed-off-by: Jarno Rajahalme --- include/envoy/http/filter.h | 4 ++-- source/common/http/async_client_impl.h | 2 +- source/common/http/conn_manager_impl.cc | 5 ++--- source/common/router/router.cc | 1 + test/mocks/http/mocks.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index b5e7a4e13c44a..2c3825c0b1c79 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -191,14 +191,14 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; /** - * Create a locally generated response using the provided response_code and body_text paramewters. + * Create a locally generated response using the provided response_code and body_text parameters. * If the request was a gRPC request the local reply will be encoded as a gRPC response with a 200 * HTTP response code and grpc-status and grpc-message headers mapped from the provided * parameters. * * @param response_code supplies the HTTP response code. * @param body_text supplies the optional body text which is sent using the text/plain content - * type. + * type, or encoded in the grpc-message header. * @param modify_headers supplies an optional callback function that can modify the * response headers. */ diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 079c75cd6e998..1b7967adf7eab 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -266,7 +266,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { + if (modify_headers != nullptr) { modify_headers(*headers); } encodeHeaders(std::move(headers), end_stream); diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fc833904360af..c58dd891a7893 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -474,8 +474,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, request_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on. - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::UpgradeRequired, "", - nullptr); + sendLocalReply(false, Code::UpgradeRequired, "", nullptr); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -824,7 +823,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( std::function modify_headers) { Utility::sendLocalReply(is_grpc_request, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { + if (modify_headers != nullptr) { modify_headers(*headers); } response_headers_ = std::move(headers); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 44358004d9fe0..9e4857c092920 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -181,6 +181,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e downstream_headers_ = &headers; + // TODO: Maybe add a filter API for this. grpc_request_ = Grpc::Common::hasGrpcContentType(headers); // Only increment rq total stat if we actually decode headers here. This does not count requests diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 29d3222bd65d8..015709cc02c0b 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -202,7 +202,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (headers != nullptr && modify_headers != nullptr) { + if (modify_headers != nullptr) { modify_headers(*headers); } encodeHeaders(std::move(headers), end_stream);