From 44967f0d593fa5b50209ee32e268348cb3fd25d4 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Fri, 26 Oct 2018 20:13:01 -0700 Subject: [PATCH 1/5] rate-limit: make 429 response mapping configurable This commit enables the configuration of the mapping that translates 429 response code to a gRPC status code. By default, the Rate Limit filter in Envoy translates a 429 HTTP response code to UNAVAILABLE as specified in the gRPC mapping document. Google, however, recommends translating a 429 response to RESOURCE_EXHAUSTED. This commit provides a flag named rate_limited_as_resource_exhausted in the RateLimit config which allows users to explicitly specify whether they want 429 responses to be mapped to RESOURCE_EXHAUSTED, while UNAVAILABLE remains the default. References: * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md * https://cloud.google.com/apis/design/errors#generating_errors Signed-off-by: Venil Noronha --- .../http/rate_limit/v2/rate_limit.proto | 4 +++ docs/root/intro/version_history.rst | 3 ++ include/envoy/http/filter.h | 11 ++++++- source/common/grpc/status.cc | 12 ++++++-- source/common/grpc/status.h | 18 ++++++++--- source/common/http/async_client_impl.h | 5 ++-- source/common/http/conn_manager_impl.cc | 8 +++-- source/common/http/conn_manager_impl.h | 9 +++--- source/common/http/utility.cc | 19 +++++++----- source/common/http/utility.h | 5 ++-- source/common/upstream/health_checker_impl.cc | 4 +-- .../filters/http/ratelimit/ratelimit.cc | 6 ++-- .../filters/http/ratelimit/ratelimit.h | 6 ++-- test/common/grpc/common_test.cc | 7 ++++- test/common/http/utility_test.cc | 30 ++++++++++++++++--- test/mocks/http/mocks.h | 5 ++-- 16 files changed, 112 insertions(+), 40 deletions(-) diff --git a/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto b/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto index d79764745ab74..1174cae66d332 100644 --- a/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto +++ b/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto @@ -40,4 +40,8 @@ message RateLimit { // communication failure between rate limiting service and the proxy. // Defaults to false. bool failure_mode_deny = 5; + + // Specifies whether a `RESOURCE_EXHAUSTED` code must be returned instead of + // the default `UNAVAILABLE` code for a rate limited gRPC call. + bool rate_limited_as_resource_exhausted = 6; } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 4e4732dc90d99..0b21280e233e2 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -17,6 +17,9 @@ Version history See `#3611 `_ for details. * network: removed the reference to `FilterState` in `Connection` in favor of `StreamInfo`. * logging: added missing [ in log prefix. +* rate-limit: added :ref:`configuration ` + to specify whether the `GrpcStatus` status returned should be `RESOURCE_EXHAUSTED` or + `UNAVAILABLE` when a gRPC call is rate limited. * rbac: added support for permission matching by :ref:`requested server name `. * router: added ability to configure arbitrary :ref:`retriable status codes. ` * router: added ability to set attempt count in upstream requests, see :ref:`virtual host's include request diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 4eb8ebdc68b9f..663d453c0d578 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -221,9 +221,18 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { * type, or encoded in the grpc-message header. * @param modify_headers supplies an optional callback function that can modify the * response headers. + * @param rate_limited_as_resource_exhausted specifies whether a RESOURCE_EXHAUSTED code + * should be returned instead of the default + * UNAVAILABLE code for rate limited gRPC calls. */ virtual void sendLocalReply(Code response_code, const std::string& body_text, - std::function modify_headers) PURE; + std::function modify_headers, + bool rate_limited_as_resource_exhausted) PURE; + + void sendLocalReply(Code response_code, const std::string& body_text, + std::function modify_headers) { + sendLocalReply(response_code, body_text, modify_headers, false); + } /** * Called with 100-Continue headers to be encoded. diff --git a/source/common/grpc/status.cc b/source/common/grpc/status.cc index 70fdcce027113..2bf03adb1f850 100644 --- a/source/common/grpc/status.cc +++ b/source/common/grpc/status.cc @@ -3,9 +3,11 @@ 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. +Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status, + bool rate_limited_as_resource_exhausted) { + // See: + // * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md + // * https://cloud.google.com/apis/design/errors#generating_errors switch (http_response_status) { case 400: return Status::GrpcStatus::Internal; @@ -16,6 +18,10 @@ Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) { case 404: return Status::GrpcStatus::Unimplemented; case 429: + if (rate_limited_as_resource_exhausted) { + return Status::GrpcStatus::ResourceExhausted; + } + return Status::GrpcStatus::Unavailable; case 502: case 503: case 504: diff --git a/source/common/grpc/status.h b/source/common/grpc/status.h index 6355e002c2a93..444f0eb602e98 100644 --- a/source/common/grpc/status.h +++ b/source/common/grpc/status.h @@ -13,13 +13,23 @@ namespace Grpc { 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. + * 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. + * + * Google defines a mapping where a code of 429 (rate limited) is mapped to + * RESOURCE_EXHAUSTED instead of UNAVAILABLE as defined by gRPC. This function + * allows the user to specify the GrpcStatus that should map to a 429 response. + * See https://cloud.google.com/apis/design/errors#generating_errors. + * * @param http_response_status HTTP status code. + * @param rate_limited_as_resource_exhausted whether a 429 response code + * should be mapped to RESOURCE_EXHAUSTED instead of UNAVAILABLE. * @return Status::GrpcStatus corresponding gRPC status code. */ - static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); + static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status, + bool rate_limited_as_resource_exhausted); /** * @param grpc_status gRPC status from grpc-status header. diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 89272ae76b149..a8ad2207df471 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -286,7 +286,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } void sendLocalReply(Code code, const std::string& body, - std::function modify_headers) override { + std::function modify_headers, + bool rate_limited_as_resource_exhausted = false) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -296,7 +297,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - remote_closed_, code, body, is_head_request_); + remote_closed_, code, body, is_head_request_, rate_limited_as_resource_exhausted); } // The async client won't pause if sending an Expect: 100-Continue so simply // swallows any incoming encode100Continue. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 8b445ce5d2382..57971048801e5 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -929,7 +929,8 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, const std::string& body, - std::function modify_headers, bool is_head_request) { + std::function modify_headers, bool is_head_request, + bool rate_limited_as_resource_exhausted) { Utility::sendLocalReply(is_grpc_request, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { @@ -945,7 +946,8 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( // request instead. encodeData(nullptr, data, end_stream); }, - state_.destroyed_, code, body, is_head_request); + state_.destroyed_, code, body, is_head_request, + rate_limited_as_resource_exhausted); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( @@ -1623,7 +1625,7 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.local_complete_ = end_stream; }, parent_.state_.destroyed_, Http::Code::InternalServerError, - CodeUtility::toString(Http::Code::InternalServerError), parent_.is_head_request_); + CodeUtility::toString(Http::Code::InternalServerError), parent_.is_head_request_, false); parent_.maybeEndEncode(parent_.state_.local_complete_); } else { resetStream(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 14f90c19ecc88..d5c9fe7bb9aa4 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -173,9 +173,10 @@ class ConnectionManagerImpl : Logger::Loggable, return parent_.buffered_request_data_.get(); } void sendLocalReply(Code code, const std::string& body, - std::function modify_headers) override { - parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, - parent_.is_head_request_); + std::function modify_headers, + bool rate_limited_as_resource_exhausted = false) override { + parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_, + rate_limited_as_resource_exhausted); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -283,7 +284,7 @@ class ConnectionManagerImpl : Logger::Loggable, HeaderMap& addEncodedTrailers(); void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, std::function modify_headers, - bool is_head_request); + bool is_head_request, bool rate_limited_as_resource_exhausted = false); 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); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5a46448a03762..20bca9f89c651 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -243,7 +243,7 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text, - bool is_head_request) { + bool is_head_request, bool rate_limited_as_resource_exhausted) { sendLocalReply(is_grpc, [&](HeaderMapPtr&& headers, bool end_stream) -> void { callbacks.encodeHeaders(std::move(headers), end_stream); @@ -251,22 +251,25 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac [&](Buffer::Instance& data, bool end_stream) -> void { callbacks.encodeData(data, end_stream); }, - is_reset, response_code, body_text, is_head_request); + is_reset, response_code, body_text, is_head_request, + rate_limited_as_resource_exhausted); } 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, bool is_head_request) { + Code response_code, const std::string& body_text, bool is_head_request, + bool rate_limited_as_resource_exhausted) { // 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{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))))}}}; + 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), rate_limited_as_resource_exhausted)))}}}; if (!body_text.empty() && !is_head_request) { // 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 f7e3544ae3371..a535120495cd3 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -139,7 +139,8 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption * @param is_head_request tells if this is a response to a HEAD request */ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, const std::string& body_text, bool is_head_request); + Code response_code, const std::string& body_text, bool is_head_request, + bool rate_limited_as_resource_exhausted); /** * Create a locally generated response using the provided lambdas. @@ -157,7 +158,7 @@ void sendLocalReply(bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, const std::string& body_text, - bool is_head_request = false); + bool is_head_request = false, bool rate_limited_as_resource_exhausted = false); struct GetLastAddressFromXffInfo { // Last valid address pulled from the XFF header. diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index d9c83c3998d9a..8908118a7ce1f 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -402,8 +402,8 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( return; } } - onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status), "non-200 HTTP response", - end_stream); + onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status, false), + "non-200 HTTP response", end_stream); return; } if (!Grpc::Common::hasGrpcContentType(*headers)) { diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 60fc89a16b229..fac0692a0b375 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -144,7 +144,8 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header config_->runtime().snapshot().featureEnabled("ratelimit.http_filter_enforcing", 100)) { state_ = State::Responded; callbacks_->sendLocalReply(Http::Code::TooManyRequests, "", - [this](Http::HeaderMap& headers) { addHeaders(headers); }); + [this](Http::HeaderMap& headers) { addHeaders(headers); }, + config_->rateLimitedAsResourceExhausted()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); } else if (status == RateLimit::LimitStatus::Error) { if (config_->failureModeAllow()) { @@ -154,7 +155,8 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header } } else { state_ = State::Responded; - callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr); + callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, + config_->rateLimitedAsResourceExhausted()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError); } } else if (!initiating_call_) { diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 2f69eadc8a211..2031a22af28c2 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -38,15 +38,16 @@ class FilterConfig { request_type_(config.request_type().empty() ? stringToType("both") : stringToType(config.request_type())), local_info_(local_info), scope_(scope), runtime_(runtime), - failure_mode_deny_(config.failure_mode_deny()) {} + failure_mode_deny_(config.failure_mode_deny()), + rate_limited_as_resource_exhausted_(config.rate_limited_as_resource_exhausted()) {} const std::string& domain() const { return domain_; } const LocalInfo::LocalInfo& localInfo() const { return local_info_; } uint64_t stage() const { return stage_; } Runtime::Loader& runtime() { return runtime_; } Stats::Scope& scope() { return scope_; } FilterRequestType requestType() const { return request_type_; } - bool failureModeAllow() const { return !failure_mode_deny_; } + bool rateLimitedAsResourceExhausted() const { return rate_limited_as_resource_exhausted_; } private: static FilterRequestType stringToType(const std::string& request_type) { @@ -67,6 +68,7 @@ class FilterConfig { Stats::Scope& scope_; Runtime::Loader& runtime_; const bool failure_mode_deny_; + const bool rate_limited_as_resource_exhausted_; }; typedef std::shared_ptr FilterConfigSharedPtr; diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 290328a53941a..84a2ea2dfadfa 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -259,10 +259,15 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { {500, Status::GrpcStatus::Unknown}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first)); + EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first, false)); } } +TEST(GrpcCommonTest, HttpToGrpcStatusRateLimited) { + EXPECT_EQ(Status::GrpcStatus::Unavailable, Grpc::Utility::httpToGrpcStatus(429, false)); + EXPECT_EQ(Status::GrpcStatus::ResourceExhausted, Grpc::Utility::httpToGrpcStatus(429, true)); +} + TEST(GrpcCommonTest, HasGrpcContentType) { { Http::TestHeaderMapImpl headers{}; diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index e1cc328177a0f..ef20be440465c 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -456,7 +456,8 @@ TEST(HttpUtility, SendLocalReply) { EXPECT_CALL(callbacks, encodeHeaders_(_, false)); EXPECT_CALL(callbacks, encodeData(_, true)); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false, + false); } TEST(HttpUtility, SendLocalGrpcReply) { @@ -471,7 +472,26 @@ TEST(HttpUtility, SendLocalGrpcReply) { EXPECT_NE(headers.GrpcMessage(), nullptr); EXPECT_STREQ(headers.GrpcMessage()->value().c_str(), "large"); })); - Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false); + Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false, + false); +} + +TEST(HttpUtility, RateLimitedGrpcStatus) { + MockStreamDecoderFilterCallbacks callbacks; + + EXPECT_CALL(callbacks, encodeHeaders_(_, true)) + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { + EXPECT_NE(headers.GrpcStatus(), nullptr); + EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "14"); // Unavailable + })); + Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", false, false); + + EXPECT_CALL(callbacks, encodeHeaders_(_, true)) + .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { + EXPECT_NE(headers.GrpcStatus(), nullptr); + EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "8"); // ResourceExhausted + })); + Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", false, true); } TEST(HttpUtility, SendLocalReplyDestroyedEarly) { @@ -482,7 +502,8 @@ TEST(HttpUtility, SendLocalReplyDestroyedEarly) { is_reset = true; })); EXPECT_CALL(callbacks, encodeData(_, true)).Times(0); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false, + false); } TEST(HttpUtility, SendLocalReplyHeadRequest) { @@ -493,7 +514,8 @@ TEST(HttpUtility, SendLocalReplyHeadRequest) { EXPECT_STREQ(headers.ContentLength()->value().c_str(), fmt::format("{}", strlen("large")).c_str()); })); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", true); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", true, + false); } TEST(HttpUtility, TestExtractHostPathFromUri) { diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index ce95ac6c85d5e..9b441b9c725be 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -207,7 +207,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, // Http::StreamDecoderFilterCallbacks void sendLocalReply(Code code, const std::string& body, - std::function modify_headers) override { + std::function modify_headers, + bool rate_limited_as_resource_exhausted) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -217,7 +218,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - stream_destroyed_, code, body, is_head_request_); + stream_destroyed_, code, body, is_head_request_, rate_limited_as_resource_exhausted); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override { encode100ContinueHeaders_(*headers); From 161ba4d6deddf91c9e26a2b41be8cb53b9de46e0 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Mon, 29 Oct 2018 14:44:03 -0700 Subject: [PATCH 2/5] filters: take optional status map input Signed-off-by: Venil Noronha --- include/envoy/grpc/status.h | 7 ++++ include/envoy/http/BUILD | 2 + include/envoy/http/filter.h | 15 +++----- source/common/grpc/BUILD | 1 + source/common/grpc/status.cc | 15 +++++--- source/common/grpc/status.h | 8 ++-- source/common/http/BUILD | 1 + source/common/http/async_client_impl.h | 4 +- source/common/http/conn_manager_impl.cc | 30 ++++++++------- source/common/http/conn_manager_impl.h | 6 +-- source/common/http/utility.cc | 21 +++++----- source/common/http/utility.h | 12 ++++-- source/common/router/router.cc | 38 +++++++++++-------- source/common/upstream/health_checker_impl.cc | 2 +- .../filters/http/buffer/buffer_filter.cc | 3 +- .../filters/http/ext_authz/ext_authz.cc | 3 +- .../filters/http/fault/fault_filter.cc | 2 +- .../json_transcoder_filter.cc | 4 +- .../filters/http/grpc_web/grpc_web_filter.cc | 6 ++- .../filters/http/health_check/health_check.cc | 2 +- .../filters/http/jwt_authn/filter.cc | 4 +- .../filters/http/ratelimit/ratelimit.cc | 4 +- .../filters/http/ratelimit/ratelimit.h | 17 +++++++-- .../filters/http/rbac/rbac_filter.cc | 3 +- test/common/grpc/common_test.cc | 10 +++-- test/common/http/conn_manager_impl_test.cc | 3 +- test/common/http/utility_test.cc | 24 +++++++----- test/mocks/http/mocks.h | 4 +- 28 files changed, 153 insertions(+), 98 deletions(-) diff --git a/include/envoy/grpc/status.h b/include/envoy/grpc/status.h index 24c200eb438a5..42a21c3b1fcd6 100644 --- a/include/envoy/grpc/status.h +++ b/include/envoy/grpc/status.h @@ -1,5 +1,7 @@ #pragma once +#include + namespace Envoy { namespace Grpc { @@ -50,5 +52,10 @@ class Status { }; }; +/** + * A map of HTTP status codes to corresponding gRPC status codes. + */ +typedef std::map StatusMap; + } // namespace Grpc } // namespace Envoy diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index ad71101f9c85c..203b5542636c1 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -46,11 +46,13 @@ envoy_cc_library( envoy_cc_library( name = "filter_interface", hdrs = ["filter.h"], + external_deps = ["abseil_optional"], deps = [ ":codec_interface", ":header_map_interface", "//include/envoy/access_log:access_log_interface", "//include/envoy/event:dispatcher_interface", + "//include/envoy/grpc:status", "//include/envoy/router:router_interface", "//include/envoy/ssl:connection_interface", "//include/envoy/tracing:http_tracer_interface", diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 663d453c0d578..19c2bab0bd83b 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -7,6 +7,7 @@ #include "envoy/access_log/access_log.h" #include "envoy/event/dispatcher.h" +#include "envoy/grpc/status.h" #include "envoy/http/codec.h" #include "envoy/http/header_map.h" #include "envoy/router/router.h" @@ -14,6 +15,8 @@ #include "envoy/tracing/http_tracer.h" #include "envoy/upstream/upstream.h" +#include "absl/types/optional.h" + namespace Envoy { namespace Http { @@ -221,18 +224,12 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { * type, or encoded in the grpc-message header. * @param modify_headers supplies an optional callback function that can modify the * response headers. - * @param rate_limited_as_resource_exhausted specifies whether a RESOURCE_EXHAUSTED code - * should be returned instead of the default - * UNAVAILABLE code for rate limited gRPC calls. + * @param status_map a map of HTTP status codes to corresponding gRPC status + * codes to override the default code mapping. */ virtual void sendLocalReply(Code response_code, const std::string& body_text, std::function modify_headers, - bool rate_limited_as_resource_exhausted) PURE; - - void sendLocalReply(Code response_code, const std::string& body_text, - std::function modify_headers) { - sendLocalReply(response_code, body_text, modify_headers, false); - } + const absl::optional& status_map) PURE; /** * Called with 100-Continue headers to be encoded. diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 3507c69d1e490..dfe0008970d46 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -50,6 +50,7 @@ envoy_cc_library( name = "status_lib", srcs = ["status.cc"], hdrs = ["status.h"], + external_deps = ["abseil_optional"], deps = [ "//include/envoy/grpc:status", ], diff --git a/source/common/grpc/status.cc b/source/common/grpc/status.cc index 2bf03adb1f850..83c62a6e26020 100644 --- a/source/common/grpc/status.cc +++ b/source/common/grpc/status.cc @@ -4,10 +4,19 @@ namespace Envoy { namespace Grpc { Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status, - bool rate_limited_as_resource_exhausted) { + const absl::optional& status_map) { // See: // * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md // * https://cloud.google.com/apis/design/errors#generating_errors + + // override default code mapping if provided + if (status_map) { + StatusMap::const_iterator iter = status_map.value().find(http_response_status); + if (iter != status_map.value().end()) { + return iter->second; + } + } + switch (http_response_status) { case 400: return Status::GrpcStatus::Internal; @@ -18,10 +27,6 @@ Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status, case 404: return Status::GrpcStatus::Unimplemented; case 429: - if (rate_limited_as_resource_exhausted) { - return Status::GrpcStatus::ResourceExhausted; - } - return Status::GrpcStatus::Unavailable; case 502: case 503: case 504: diff --git a/source/common/grpc/status.h b/source/common/grpc/status.h index 444f0eb602e98..7675f83ebaae5 100644 --- a/source/common/grpc/status.h +++ b/source/common/grpc/status.h @@ -4,6 +4,8 @@ #include "envoy/grpc/status.h" +#include "absl/types/optional.h" + namespace Envoy { namespace Grpc { @@ -24,12 +26,12 @@ class Utility { * See https://cloud.google.com/apis/design/errors#generating_errors. * * @param http_response_status HTTP status code. - * @param rate_limited_as_resource_exhausted whether a 429 response code - * should be mapped to RESOURCE_EXHAUSTED instead of UNAVAILABLE. + * @param status_map a map of HTTP status codes to corresponding gRPC status + * codes to override the default code mapping. * @return Status::GrpcStatus corresponding gRPC status code. */ static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status, - bool rate_limited_as_resource_exhausted); + const absl::optional& status_map); /** * @param grpc_status gRPC status from grpc-status header. diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 563e78ebdd29b..30ff28c00596f 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -238,6 +238,7 @@ envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], hdrs = ["utility.h"], + external_deps = ["abseil_optional"], deps = [ ":exception_lib", ":header_map_lib", diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index a8ad2207df471..bee0afbade9d7 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -287,7 +287,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } void sendLocalReply(Code code, const std::string& body, std::function modify_headers, - bool rate_limited_as_resource_exhausted = false) override { + const absl::optional& status_map) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -297,7 +297,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - remote_closed_, code, body, is_head_request_, rate_limited_as_resource_exhausted); + remote_closed_, code, body, status_map, is_head_request_); } // The async client won't pause if sending an Expect: 100-Continue so simply // swallows any incoming encode100Continue. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 57971048801e5..fcf5f435096ce 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -430,9 +430,9 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() { // or gRPC status code, and/or set H2 RST_STREAM error. connection_manager_.doEndStream(*this); } else { - sendLocalReply(request_headers_ != nullptr && - Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_); + sendLocalReply( + request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_), + Http::Code::RequestTimeout, "stream timeout", nullptr, is_head_request_, absl::nullopt); } } @@ -504,7 +504,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, Server::OverloadActionState::Active) { connection_manager_.stats_.named_.downstream_rq_overload_close_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), - Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, is_head_request_); + Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, is_head_request_, + absl::nullopt); return; } @@ -537,7 +538,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, stream_info_.protocol(protocol); if (!connection_manager_.config_.http1Settings().accept_http_10_) { // Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on. - sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_); + sendLocalReply(false, Code::UpgradeRequired, "", nullptr, is_head_request_, absl::nullopt); return; } else { // HTTP/1.0 defaults to single-use connections. Make sure the connection @@ -561,7 +562,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } else { // Require host header. For HTTP/1.1 Host has already been translated to :authority. sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr, is_head_request_); + nullptr, is_head_request_, absl::nullopt); return; } } @@ -575,7 +576,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // envoy users who do not wish to proxy large headers. if (request_headers_->byteSize() > (60 * 1024)) { sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), - Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_); + Code::RequestHeaderFieldsTooLarge, "", nullptr, is_head_request_, absl::nullopt); return; } @@ -587,7 +588,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') { connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr, - is_head_request_); + is_head_request_, absl::nullopt); return; } @@ -615,7 +616,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Do not allow upgrades if the route does not support it. connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", - nullptr, is_head_request_); + nullptr, is_head_request_, absl::nullopt); return; } // Allow non websocket requests to go through websocket enabled routes. @@ -930,7 +931,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, const std::string& body, std::function modify_headers, bool is_head_request, - bool rate_limited_as_resource_exhausted) { + const absl::optional& status_map) { Utility::sendLocalReply(is_grpc_request, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { @@ -946,8 +947,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( // request instead. encodeData(nullptr, data, end_stream); }, - state_.destroyed_, code, body, is_head_request, - rate_limited_as_resource_exhausted); + state_.destroyed_, code, body, status_map, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( @@ -1537,7 +1537,8 @@ void ConnectionManagerImpl::ActiveStreamDecoderFilter::requestDataTooLarge() { onDecoderFilterAboveWriteBufferHighWatermark(); } else { parent_.connection_manager_.stats_.named_.downstream_rq_too_large_.inc(); - sendLocalReply(Code::PayloadTooLarge, CodeUtility::toString(Code::PayloadTooLarge), nullptr); + sendLocalReply(Code::PayloadTooLarge, CodeUtility::toString(Code::PayloadTooLarge), nullptr, + absl::nullopt); } } @@ -1625,7 +1626,8 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::responseDataTooLarge() { parent_.state_.local_complete_ = end_stream; }, parent_.state_.destroyed_, Http::Code::InternalServerError, - CodeUtility::toString(Http::Code::InternalServerError), parent_.is_head_request_, false); + CodeUtility::toString(Http::Code::InternalServerError), absl::nullopt, + parent_.is_head_request_); parent_.maybeEndEncode(parent_.state_.local_complete_); } else { resetStream(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index d5c9fe7bb9aa4..9b981d54b7860 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -174,9 +174,9 @@ class ConnectionManagerImpl : Logger::Loggable, } void sendLocalReply(Code code, const std::string& body, std::function modify_headers, - bool rate_limited_as_resource_exhausted = false) override { + const absl::optional& status_map) override { parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_, - rate_limited_as_resource_exhausted); + status_map); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -284,7 +284,7 @@ class ConnectionManagerImpl : Logger::Loggable, HeaderMap& addEncodedTrailers(); void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, std::function modify_headers, - bool is_head_request, bool rate_limited_as_resource_exhausted = false); + bool is_head_request, const absl::optional& status_map); 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); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 20bca9f89c651..42fadd1a5113b 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -243,7 +243,8 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text, - bool is_head_request, bool rate_limited_as_resource_exhausted) { + const absl::optional& status_map, + bool is_head_request) { sendLocalReply(is_grpc, [&](HeaderMapPtr&& headers, bool end_stream) -> void { callbacks.encodeHeaders(std::move(headers), end_stream); @@ -251,25 +252,23 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac [&](Buffer::Instance& data, bool end_stream) -> void { callbacks.encodeData(data, end_stream); }, - is_reset, response_code, body_text, is_head_request, - rate_limited_as_resource_exhausted); + is_reset, response_code, body_text, status_map, is_head_request); } 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, bool is_head_request, - bool rate_limited_as_resource_exhausted) { + Code response_code, const std::string& body_text, + const absl::optional& status_map, bool is_head_request) { // 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{ - 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), rate_limited_as_resource_exhausted)))}}}; + 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), status_map)))}}}; if (!body_text.empty() && !is_head_request) { // 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 a535120495cd3..6334a926fc6b6 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -15,6 +15,7 @@ #include "common/json/json_loader.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" namespace Envoy { namespace Http { @@ -136,11 +137,13 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption * @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 status_map a map of HTTP status codes to corresponding gRPC status + * codes to override the default code mapping. * @param is_head_request tells if this is a response to a HEAD request */ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, const std::string& body_text, bool is_head_request, - bool rate_limited_as_resource_exhausted); + Code response_code, const std::string& body_text, + const absl::optional& status_map, bool is_head_request); /** * Create a locally generated response using the provided lambdas. @@ -153,12 +156,15 @@ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const * @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 status_map a map of HTTP status codes to corresponding gRPC status + * codes to override the default code mapping. */ void sendLocalReply(bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, const std::string& body_text, - bool is_head_request = false, bool rate_limited_as_resource_exhausted = false); + const absl::optional& status_map, + bool is_head_request = false); struct GetLastAddressFromXffInfo { // Last valid address pulled from the XFF header. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 9d966e10a67fd..529649fbccf0c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -220,7 +220,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e headers.Path()->value().c_str()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); - callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr); + callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr, absl::nullopt); return Http::FilterHeadersStatus::StopIteration; } @@ -238,7 +238,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e response_headers.addReferenceKey(Http::Headers::get().Location, new_path); } direct_response->finalizeResponseHeaders(response_headers, callbacks_->streamInfo()); - }); + }, + absl::nullopt); return Http::FilterHeadersStatus::StopIteration; } @@ -250,7 +251,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e ENVOY_STREAM_LOG(debug, "unknown cluster '{}'", *callbacks_, route_entry_->clusterName()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); - callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", nullptr); + callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", nullptr, + absl::nullopt); return Http::FilterHeadersStatus::StopIteration; } cluster_ = cluster->info(); @@ -270,12 +272,14 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e if (cluster_->maintenanceMode()) { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::UpstreamOverflow); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, true); - callbacks_->sendLocalReply( - Http::Code::ServiceUnavailable, "maintenance mode", [this](Http::HeaderMap& headers) { - if (!config_.suppress_envoy_headers_) { - headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); - } - }); + callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "maintenance mode", + [this](Http::HeaderMap& headers) { + if (!config_.suppress_envoy_headers_) { + headers.insertEnvoyOverloaded().value( + Http::Headers::get().EnvoyOverloadedValues.True); + } + }, + absl::nullopt); cluster_->stats().upstream_rq_maintenance_mode_.inc(); return Http::FilterHeadersStatus::StopIteration; } @@ -344,7 +348,8 @@ Http::ConnectionPool::Instance* Filter::getConnPool() { void Filter::sendNoHealthyUpstreamResponse() { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoHealthyUpstream); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, false); - callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", nullptr); + callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", nullptr, + absl::nullopt); } Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) { @@ -534,11 +539,14 @@ void Filter::onUpstreamReset(UpstreamResetType type, if (upstream_host != nullptr && !Http::CodeUtility::is5xx(enumToInt(code))) { upstream_host->stats().rq_error_.inc(); } - callbacks_->sendLocalReply(code, body, [dropped, this](Http::HeaderMap& headers) { - if (dropped && !config_.suppress_envoy_headers_) { - headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); - } - }); + callbacks_->sendLocalReply(code, body, + [dropped, this](Http::HeaderMap& headers) { + if (dropped && !config_.suppress_envoy_headers_) { + headers.insertEnvoyOverloaded().value( + Http::Headers::get().EnvoyOverloadedValues.True); + } + }, + absl::nullopt); } } diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 8908118a7ce1f..409f6f736ffa6 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -402,7 +402,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( return; } } - onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status, false), + onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status, absl::nullopt), "non-200 HTTP response", end_stream); return; } diff --git a/source/extensions/filters/http/buffer/buffer_filter.cc b/source/extensions/filters/http/buffer/buffer_filter.cc index 70e636e6de9d6..7c611fdb62fe9 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.cc +++ b/source/extensions/filters/http/buffer/buffer_filter.cc @@ -110,7 +110,8 @@ BufferFilterStats BufferFilter::generateStats(const std::string& prefix, Stats:: void BufferFilter::onDestroy() { resetInternalState(); } void BufferFilter::onRequestTimeout() { - callbacks_->sendLocalReply(Http::Code::RequestTimeout, "buffer request timeout", nullptr); + callbacks_->sendLocalReply(Http::Code::RequestTimeout, "buffer request timeout", nullptr, + absl::nullopt); config_->stats().rq_timeout_.inc(); } diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 078de6ca4b9da..106c2dcd0d3a7 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -109,7 +109,8 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { ENVOY_STREAM_LOG(trace, " '{}':'{}'", callbacks, header.first.get(), header.second); } - }); + }, + absl::nullopt); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::UnauthorizedExternalService); } else { ENVOY_STREAM_LOG(debug, "ext_authz accepted the request", *callbacks_); diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index fced6700f9769..6281c6bda5f9c 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -257,7 +257,7 @@ void FaultFilter::postDelayInjection() { void FaultFilter::abortWithHTTPStatus() { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::FaultInjected); callbacks_->sendLocalReply(static_cast(abortHttpStatus()), "fault filter abort", - nullptr); + nullptr, absl::nullopt); recordAbortsInjectedStats(); } 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 bc765c6f2f5ac..f97a9f743e78a 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 @@ -244,7 +244,7 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, request_status.error_message(), - nullptr); + nullptr, absl::nullopt); return Http::FilterHeadersStatus::StopIteration; } @@ -280,7 +280,7 @@ Http::FilterDataStatus JsonTranscoderFilter::decodeData(Buffer::Instance& data, ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, request_status.error_message(), - nullptr); + nullptr, absl::nullopt); 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 dbde9c8cc9af5..d6dbccefb88a3 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -94,7 +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. decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, - "Bad gRPC-web request, invalid base64 data.", nullptr); + "Bad gRPC-web request, invalid base64 data.", nullptr, + absl::nullopt); return Http::FilterDataStatus::StopIterationNoBuffer; } } else if (available < 4) { @@ -110,7 +111,8 @@ Http::FilterDataStatus GrpcWebFilter::decodeData(Buffer::Instance& data, bool en if (decoded.empty()) { // Error happened when decoding base64. decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, - "Bad gRPC-web request, invalid base64 data.", nullptr); + "Bad gRPC-web request, invalid base64 data.", nullptr, + absl::nullopt); return Http::FilterDataStatus::StopIterationNoBuffer; } diff --git a/source/extensions/filters/http/health_check/health_check.cc b/source/extensions/filters/http/health_check/health_check.cc index 9d0e3f3205e97..ad7435362a2d4 100644 --- a/source/extensions/filters/http/health_check/health_check.cc +++ b/source/extensions/filters/http/health_check/health_check.cc @@ -143,7 +143,7 @@ void HealthCheckFilter::onComplete() { } } - callbacks_->sendLocalReply(final_status, "", nullptr); + callbacks_->sendLocalReply(final_status, "", nullptr, absl::nullopt); } } // namespace HealthCheck diff --git a/source/extensions/filters/http/jwt_authn/filter.cc b/source/extensions/filters/http/jwt_authn/filter.cc index 751c97fc6c742..5f49c826d41e2 100644 --- a/source/extensions/filters/http/jwt_authn/filter.cc +++ b/source/extensions/filters/http/jwt_authn/filter.cc @@ -58,8 +58,8 @@ void Filter::onComplete(const Status& status) { // verification failed Http::Code code = Http::Code::Unauthorized; // return failure reason as message body - decoder_callbacks_->sendLocalReply(code, ::google::jwt_verify::getStatusString(status), - nullptr); + decoder_callbacks_->sendLocalReply(code, ::google::jwt_verify::getStatusString(status), nullptr, + absl::nullopt); return; } stats_.allowed_.inc(); diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index fac0692a0b375..7acf7b6d554f7 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -145,7 +145,7 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header state_ = State::Responded; callbacks_->sendLocalReply(Http::Code::TooManyRequests, "", [this](Http::HeaderMap& headers) { addHeaders(headers); }, - config_->rateLimitedAsResourceExhausted()); + config_->statusMap()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); } else if (status == RateLimit::LimitStatus::Error) { if (config_->failureModeAllow()) { @@ -156,7 +156,7 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header } else { state_ = State::Responded; callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, - config_->rateLimitedAsResourceExhausted()); + config_->statusMap()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError); } } else if (!initiating_call_) { diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 2031a22af28c2..cb10837193a6e 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -14,6 +14,7 @@ #include "envoy/upstream/cluster_manager.h" #include "common/common/assert.h" +#include "common/common/enum_to_int.h" #include "common/http/header_map_impl.h" namespace Envoy { @@ -39,7 +40,7 @@ class FilterConfig { : stringToType(config.request_type())), local_info_(local_info), scope_(scope), runtime_(runtime), failure_mode_deny_(config.failure_mode_deny()), - rate_limited_as_resource_exhausted_(config.rate_limited_as_resource_exhausted()) {} + status_map_(buildStatusMap(config.rate_limited_as_resource_exhausted())) {} const std::string& domain() const { return domain_; } const LocalInfo::LocalInfo& localInfo() const { return local_info_; } uint64_t stage() const { return stage_; } @@ -47,7 +48,7 @@ class FilterConfig { Stats::Scope& scope() { return scope_; } FilterRequestType requestType() const { return request_type_; } bool failureModeAllow() const { return !failure_mode_deny_; } - bool rateLimitedAsResourceExhausted() const { return rate_limited_as_resource_exhausted_; } + const absl::optional statusMap() const { return status_map_; } private: static FilterRequestType stringToType(const std::string& request_type) { @@ -61,6 +62,16 @@ class FilterConfig { } } + static const absl::optional + buildStatusMap(const bool rate_limited_as_resource_exhausted) { + if (rate_limited_as_resource_exhausted) { + Grpc::StatusMap status_map = { + {enumToInt(Http::Code::TooManyRequests), Grpc::Status::GrpcStatus::ResourceExhausted}}; + return absl::make_optional(status_map); + } + return absl::nullopt; + } + const std::string domain_; const uint64_t stage_; const FilterRequestType request_type_; @@ -68,7 +79,7 @@ class FilterConfig { Stats::Scope& scope_; Runtime::Loader& runtime_; const bool failure_mode_deny_; - const bool rate_limited_as_resource_exhausted_; + const absl::optional status_map_; }; typedef std::shared_ptr FilterConfigSharedPtr; diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 2766ef98f5955..06aed6b332a7c 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -104,7 +104,8 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head return Http::FilterHeadersStatus::Continue; } else { ENVOY_LOG(debug, "enforced denied"); - callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr); + callbacks_->sendLocalReply(Http::Code::Forbidden, "RBAC: access denied", nullptr, + absl::nullopt); config_->stats().denied_.inc(); return Http::FilterHeadersStatus::StopIteration; } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 84a2ea2dfadfa..47addb3d342f7 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -259,13 +259,17 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { {500, Status::GrpcStatus::Unknown}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first, false)); + EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first, absl::nullopt)); } } TEST(GrpcCommonTest, HttpToGrpcStatusRateLimited) { - EXPECT_EQ(Status::GrpcStatus::Unavailable, Grpc::Utility::httpToGrpcStatus(429, false)); - EXPECT_EQ(Status::GrpcStatus::ResourceExhausted, Grpc::Utility::httpToGrpcStatus(429, true)); + EXPECT_EQ(Status::GrpcStatus::Unavailable, Grpc::Utility::httpToGrpcStatus(429, absl::nullopt)); + + Grpc::StatusMap status_map = { + {enumToInt(Http::Code::TooManyRequests), Grpc::Status::GrpcStatus::ResourceExhausted}}; + EXPECT_EQ(Status::GrpcStatus::ResourceExhausted, + Grpc::Utility::httpToGrpcStatus(429, absl::make_optional(status_map))); } TEST(GrpcCommonTest, HasGrpcContentType) { diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 7e4c959ab9ba6..c3dd76b1be5ff 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2684,7 +2684,8 @@ TEST_F(HttpConnectionManagerImplTest, FilterHeadReply) { EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus { - decoder_filters_[0]->callbacks_->sendLocalReply(Code::BadRequest, "Bad request", nullptr); + decoder_filters_[0]->callbacks_->sendLocalReply(Code::BadRequest, "Bad request", nullptr, + absl::nullopt); return FilterHeadersStatus::Continue; })); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index ef20be440465c..ea32dbc4d0ac0 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -456,8 +456,8 @@ TEST(HttpUtility, SendLocalReply) { EXPECT_CALL(callbacks, encodeHeaders_(_, false)); EXPECT_CALL(callbacks, encodeData(_, true)); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false, - false); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt, false); } TEST(HttpUtility, SendLocalGrpcReply) { @@ -472,8 +472,8 @@ TEST(HttpUtility, SendLocalGrpcReply) { EXPECT_NE(headers.GrpcMessage(), nullptr); EXPECT_STREQ(headers.GrpcMessage()->value().c_str(), "large"); })); - Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false, - false); + Utility::sendLocalReply(true, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt, false); } TEST(HttpUtility, RateLimitedGrpcStatus) { @@ -484,14 +484,18 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { EXPECT_NE(headers.GrpcStatus(), nullptr); EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "14"); // Unavailable })); - Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", false, false); + Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", absl::nullopt, + false); EXPECT_CALL(callbacks, encodeHeaders_(_, true)) .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { EXPECT_NE(headers.GrpcStatus(), nullptr); EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "8"); // ResourceExhausted })); - Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", false, true); + Grpc::StatusMap status_map = { + {enumToInt(Http::Code::TooManyRequests), Grpc::Status::GrpcStatus::ResourceExhausted}}; + Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", + absl::make_optional(status_map), false); } TEST(HttpUtility, SendLocalReplyDestroyedEarly) { @@ -502,8 +506,8 @@ TEST(HttpUtility, SendLocalReplyDestroyedEarly) { is_reset = true; })); EXPECT_CALL(callbacks, encodeData(_, true)).Times(0); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", false, - false); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt, false); } TEST(HttpUtility, SendLocalReplyHeadRequest) { @@ -514,8 +518,8 @@ TEST(HttpUtility, SendLocalReplyHeadRequest) { EXPECT_STREQ(headers.ContentLength()->value().c_str(), fmt::format("{}", strlen("large")).c_str()); })); - Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", true, - false); + Utility::sendLocalReply(false, callbacks, is_reset, Http::Code::PayloadTooLarge, "large", + absl::nullopt, true); } TEST(HttpUtility, TestExtractHostPathFromUri) { diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 9b441b9c725be..44d5e2cae6e5e 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -208,7 +208,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, // Http::StreamDecoderFilterCallbacks void sendLocalReply(Code code, const std::string& body, std::function modify_headers, - bool rate_limited_as_resource_exhausted) override { + const absl::optional& status_map) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -218,7 +218,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - stream_destroyed_, code, body, is_head_request_, rate_limited_as_resource_exhausted); + stream_destroyed_, code, body, status_map, is_head_request_); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override { encode100ContinueHeaders_(*headers); From bd1201e8e5ad8cb939c330a3db23b360e46b9620 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Mon, 29 Oct 2018 16:20:19 -0700 Subject: [PATCH 3/5] Replace Grpc::StatusMap with Status::GrpcStatus Signed-off-by: Venil Noronha --- include/envoy/grpc/status.h | 5 ----- include/envoy/http/filter.h | 5 ++--- source/common/grpc/status.cc | 10 +++------ source/common/grpc/status.h | 5 ++--- source/common/http/async_client_impl.h | 4 ++-- source/common/http/conn_manager_impl.cc | 4 ++-- source/common/http/conn_manager_impl.h | 7 +++--- source/common/http/utility.cc | 8 +++---- source/common/http/utility.h | 11 +++++----- .../filters/http/ratelimit/ratelimit.cc | 5 ++--- .../filters/http/ratelimit/ratelimit.h | 22 +++++++------------ test/common/grpc/common_test.cc | 6 ++--- test/common/http/utility_test.cc | 8 +++---- test/mocks/http/mocks.h | 4 ++-- 14 files changed, 42 insertions(+), 62 deletions(-) diff --git a/include/envoy/grpc/status.h b/include/envoy/grpc/status.h index 42a21c3b1fcd6..a0267785aa8e6 100644 --- a/include/envoy/grpc/status.h +++ b/include/envoy/grpc/status.h @@ -52,10 +52,5 @@ class Status { }; }; -/** - * A map of HTTP status codes to corresponding gRPC status codes. - */ -typedef std::map StatusMap; - } // namespace Grpc } // namespace Envoy diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 19c2bab0bd83b..3b90655f1c90f 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -224,12 +224,11 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { * type, or encoded in the grpc-message header. * @param modify_headers supplies an optional callback function that can modify the * response headers. - * @param status_map a map of HTTP status codes to corresponding gRPC status - * codes to override the default code mapping. + * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. */ virtual void sendLocalReply(Code response_code, const std::string& body_text, std::function modify_headers, - const absl::optional& status_map) PURE; + const absl::optional grpc_status) PURE; /** * Called with 100-Continue headers to be encoded. diff --git a/source/common/grpc/status.cc b/source/common/grpc/status.cc index 83c62a6e26020..00e4fb418557a 100644 --- a/source/common/grpc/status.cc +++ b/source/common/grpc/status.cc @@ -4,17 +4,13 @@ namespace Envoy { namespace Grpc { Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status, - const absl::optional& status_map) { + const absl::optional grpc_status) { // See: // * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md // * https://cloud.google.com/apis/design/errors#generating_errors - // override default code mapping if provided - if (status_map) { - StatusMap::const_iterator iter = status_map.value().find(http_response_status); - if (iter != status_map.value().end()) { - return iter->second; - } + if (grpc_status) { + return grpc_status.value(); } switch (http_response_status) { diff --git a/source/common/grpc/status.h b/source/common/grpc/status.h index 7675f83ebaae5..7e7b010ced2c7 100644 --- a/source/common/grpc/status.h +++ b/source/common/grpc/status.h @@ -26,12 +26,11 @@ class Utility { * See https://cloud.google.com/apis/design/errors#generating_errors. * * @param http_response_status HTTP status code. - * @param status_map a map of HTTP status codes to corresponding gRPC status - * codes to override the default code mapping. + * @param grpc_status the gRPC status code to override the default mapping with. * @return Status::GrpcStatus corresponding gRPC status code. */ static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status, - const absl::optional& status_map); + const absl::optional grpc_status); /** * @param grpc_status gRPC status from grpc-status header. diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index bee0afbade9d7..d51ac48d2aa85 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -287,7 +287,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } void sendLocalReply(Code code, const std::string& body, std::function modify_headers, - const absl::optional& status_map) override { + const absl::optional grpc_status) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -297,7 +297,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - remote_closed_, code, body, status_map, is_head_request_); + remote_closed_, code, body, grpc_status, is_head_request_); } // The async client won't pause if sending an Expect: 100-Continue so simply // swallows any incoming encode100Continue. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fcf5f435096ce..6f1394eba8b29 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -931,7 +931,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, const std::string& body, std::function modify_headers, bool is_head_request, - const absl::optional& status_map) { + const absl::optional grpc_status) { Utility::sendLocalReply(is_grpc_request, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { if (modify_headers != nullptr) { @@ -947,7 +947,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( // request instead. encodeData(nullptr, data, end_stream); }, - state_.destroyed_, code, body, status_map, is_head_request); + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 9b981d54b7860..50c092e7858d8 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -174,9 +174,9 @@ class ConnectionManagerImpl : Logger::Loggable, } void sendLocalReply(Code code, const std::string& body, std::function modify_headers, - const absl::optional& status_map) override { + const absl::optional grpc_status) override { parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_, - status_map); + grpc_status); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override; void encodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; @@ -284,7 +284,8 @@ class ConnectionManagerImpl : Logger::Loggable, HeaderMap& addEncodedTrailers(); void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, std::function modify_headers, - bool is_head_request, const absl::optional& status_map); + bool is_head_request, + const absl::optional grpc_status); 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); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 42fadd1a5113b..0ff19e6b34fbe 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -243,7 +243,7 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text, - const absl::optional& status_map, + const absl::optional grpc_status, bool is_head_request) { sendLocalReply(is_grpc, [&](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -252,14 +252,14 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac [&](Buffer::Instance& data, bool end_stream) -> void { callbacks.encodeData(data, end_stream); }, - is_reset, response_code, body_text, status_map, is_head_request); + is_reset, response_code, body_text, grpc_status, is_head_request); } 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, - const absl::optional& status_map, bool is_head_request) { + const absl::optional grpc_status, bool is_head_request) { // 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 @@ -268,7 +268,7 @@ void Utility::sendLocalReply( {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), status_map)))}}}; + enumToInt(response_code), grpc_status)))}}}; if (!body_text.empty() && !is_head_request) { // 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 6334a926fc6b6..4b02855ac1b6c 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -137,13 +137,13 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption * @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 status_map a map of HTTP status codes to corresponding gRPC status - * codes to override the default code mapping. + * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. * @param is_head_request tells if this is a response to a HEAD request */ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, Code response_code, const std::string& body_text, - const absl::optional& status_map, bool is_head_request); + const absl::optional grpc_status, + bool is_head_request); /** * Create a locally generated response using the provided lambdas. @@ -156,14 +156,13 @@ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const * @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 status_map a map of HTTP status codes to corresponding gRPC status - * codes to override the default code mapping. + * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. */ void sendLocalReply(bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, Code response_code, const std::string& body_text, - const absl::optional& status_map, + const absl::optional grpc_status, bool is_head_request = false); struct GetLastAddressFromXffInfo { diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 7acf7b6d554f7..102e4c2c325a7 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -145,7 +145,7 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header state_ = State::Responded; callbacks_->sendLocalReply(Http::Code::TooManyRequests, "", [this](Http::HeaderMap& headers) { addHeaders(headers); }, - config_->statusMap()); + config_->rateLimitedGrpcStatus()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); } else if (status == RateLimit::LimitStatus::Error) { if (config_->failureModeAllow()) { @@ -155,8 +155,7 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header } } else { state_ = State::Responded; - callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, - config_->statusMap()); + callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, absl::nullopt); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimitServiceError); } } else if (!initiating_call_) { diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index cb10837193a6e..0eca636712fe4 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -14,7 +14,6 @@ #include "envoy/upstream/cluster_manager.h" #include "common/common/assert.h" -#include "common/common/enum_to_int.h" #include "common/http/header_map_impl.h" namespace Envoy { @@ -40,7 +39,10 @@ class FilterConfig { : stringToType(config.request_type())), local_info_(local_info), scope_(scope), runtime_(runtime), failure_mode_deny_(config.failure_mode_deny()), - status_map_(buildStatusMap(config.rate_limited_as_resource_exhausted())) {} + rate_limited_grpc_status_( + config.rate_limited_as_resource_exhausted() + ? absl::make_optional(Grpc::Status::GrpcStatus::ResourceExhausted) + : absl::nullopt) {} const std::string& domain() const { return domain_; } const LocalInfo::LocalInfo& localInfo() const { return local_info_; } uint64_t stage() const { return stage_; } @@ -48,7 +50,9 @@ class FilterConfig { Stats::Scope& scope() { return scope_; } FilterRequestType requestType() const { return request_type_; } bool failureModeAllow() const { return !failure_mode_deny_; } - const absl::optional statusMap() const { return status_map_; } + const absl::optional rateLimitedGrpcStatus() const { + return rate_limited_grpc_status_; + } private: static FilterRequestType stringToType(const std::string& request_type) { @@ -62,16 +66,6 @@ class FilterConfig { } } - static const absl::optional - buildStatusMap(const bool rate_limited_as_resource_exhausted) { - if (rate_limited_as_resource_exhausted) { - Grpc::StatusMap status_map = { - {enumToInt(Http::Code::TooManyRequests), Grpc::Status::GrpcStatus::ResourceExhausted}}; - return absl::make_optional(status_map); - } - return absl::nullopt; - } - const std::string domain_; const uint64_t stage_; const FilterRequestType request_type_; @@ -79,7 +73,7 @@ class FilterConfig { Stats::Scope& scope_; Runtime::Loader& runtime_; const bool failure_mode_deny_; - const absl::optional status_map_; + const absl::optional rate_limited_grpc_status_; }; typedef std::shared_ptr FilterConfigSharedPtr; diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 47addb3d342f7..f4d231f50fe2d 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -265,11 +265,9 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { TEST(GrpcCommonTest, HttpToGrpcStatusRateLimited) { EXPECT_EQ(Status::GrpcStatus::Unavailable, Grpc::Utility::httpToGrpcStatus(429, absl::nullopt)); - - Grpc::StatusMap status_map = { - {enumToInt(Http::Code::TooManyRequests), Grpc::Status::GrpcStatus::ResourceExhausted}}; EXPECT_EQ(Status::GrpcStatus::ResourceExhausted, - Grpc::Utility::httpToGrpcStatus(429, absl::make_optional(status_map))); + Grpc::Utility::httpToGrpcStatus(429, absl::make_optional( + Grpc::Status::GrpcStatus::ResourceExhausted))); } TEST(GrpcCommonTest, HasGrpcContentType) { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index ea32dbc4d0ac0..02fd37f73d276 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -492,10 +492,10 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { EXPECT_NE(headers.GrpcStatus(), nullptr); EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "8"); // ResourceExhausted })); - Grpc::StatusMap status_map = { - {enumToInt(Http::Code::TooManyRequests), Grpc::Status::GrpcStatus::ResourceExhausted}}; - Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", - absl::make_optional(status_map), false); + Utility::sendLocalReply( + true, callbacks, false, Http::Code::TooManyRequests, "", + absl::make_optional(Grpc::Status::GrpcStatus::ResourceExhausted), + false); } TEST(HttpUtility, SendLocalReplyDestroyedEarly) { diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 44d5e2cae6e5e..3b8e6f2c390b6 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -208,7 +208,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, // Http::StreamDecoderFilterCallbacks void sendLocalReply(Code code, const std::string& body, std::function modify_headers, - const absl::optional& status_map) override { + const absl::optional grpc_status) override { Utility::sendLocalReply( is_grpc_request_, [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { @@ -218,7 +218,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, encodeHeaders(std::move(headers), end_stream); }, [this](Buffer::Instance& data, bool end_stream) -> void { encodeData(data, end_stream); }, - stream_destroyed_, code, body, status_map, is_head_request_); + stream_destroyed_, code, body, grpc_status, is_head_request_); } void encode100ContinueHeaders(HeaderMapPtr&& headers) override { encode100ContinueHeaders_(*headers); From d6a69bac0e5714e06c97ca3831ac6c94b68af689 Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Tue, 30 Oct 2018 10:20:00 -0700 Subject: [PATCH 4/5] Revert httpToGrpcStatus API changes Signed-off-by: Venil Noronha --- include/envoy/grpc/status.h | 2 -- source/common/grpc/status.cc | 13 +++---------- source/common/grpc/status.h | 19 ++++--------------- source/common/http/utility.cc | 6 ++++-- source/common/upstream/health_checker_impl.cc | 4 ++-- test/common/grpc/common_test.cc | 9 +-------- 6 files changed, 14 insertions(+), 39 deletions(-) diff --git a/include/envoy/grpc/status.h b/include/envoy/grpc/status.h index a0267785aa8e6..24c200eb438a5 100644 --- a/include/envoy/grpc/status.h +++ b/include/envoy/grpc/status.h @@ -1,7 +1,5 @@ #pragma once -#include - namespace Envoy { namespace Grpc { diff --git a/source/common/grpc/status.cc b/source/common/grpc/status.cc index 00e4fb418557a..70fdcce027113 100644 --- a/source/common/grpc/status.cc +++ b/source/common/grpc/status.cc @@ -3,16 +3,9 @@ namespace Envoy { namespace Grpc { -Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status, - const absl::optional grpc_status) { - // See: - // * https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md - // * https://cloud.google.com/apis/design/errors#generating_errors - - if (grpc_status) { - return grpc_status.value(); - } - +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; diff --git a/source/common/grpc/status.h b/source/common/grpc/status.h index 7e7b010ced2c7..6355e002c2a93 100644 --- a/source/common/grpc/status.h +++ b/source/common/grpc/status.h @@ -4,8 +4,6 @@ #include "envoy/grpc/status.h" -#include "absl/types/optional.h" - namespace Envoy { namespace Grpc { @@ -15,22 +13,13 @@ namespace Grpc { 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. - * - * Google defines a mapping where a code of 429 (rate limited) is mapped to - * RESOURCE_EXHAUSTED instead of UNAVAILABLE as defined by gRPC. This function - * allows the user to specify the GrpcStatus that should map to a 429 response. - * See https://cloud.google.com/apis/design/errors#generating_errors. - * + * 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. - * @param grpc_status the gRPC status code to override the default mapping with. * @return Status::GrpcStatus corresponding gRPC status code. */ - static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status, - const absl::optional grpc_status); + static Status::GrpcStatus httpToGrpcStatus(uint64_t http_response_status); /** * @param grpc_status gRPC status from grpc-status header. diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 0ff19e6b34fbe..ed742f2621be1 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -267,8 +267,10 @@ void Utility::sendLocalReply( 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), grpc_status)))}}}; + {Headers::get().GrpcStatus, + std::to_string( + enumToInt(grpc_status ? grpc_status.value() + : Grpc::Utility::httpToGrpcStatus(enumToInt(response_code))))}}}; if (!body_text.empty() && !is_head_request) { // TODO: GrpcMessage should be percent-encoded response_headers->insertGrpcMessage().value(body_text); diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 409f6f736ffa6..d9c83c3998d9a 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -402,8 +402,8 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders( return; } } - onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status, absl::nullopt), - "non-200 HTTP response", end_stream); + onRpcComplete(Grpc::Utility::httpToGrpcStatus(http_response_status), "non-200 HTTP response", + end_stream); return; } if (!Grpc::Common::hasGrpcContentType(*headers)) { diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index f4d231f50fe2d..290328a53941a 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -259,17 +259,10 @@ TEST(GrpcCommonTest, HttpToGrpcStatus) { {500, Status::GrpcStatus::Unknown}, }; for (const auto& test_case : test_set) { - EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first, absl::nullopt)); + EXPECT_EQ(test_case.second, Grpc::Utility::httpToGrpcStatus(test_case.first)); } } -TEST(GrpcCommonTest, HttpToGrpcStatusRateLimited) { - EXPECT_EQ(Status::GrpcStatus::Unavailable, Grpc::Utility::httpToGrpcStatus(429, absl::nullopt)); - EXPECT_EQ(Status::GrpcStatus::ResourceExhausted, - Grpc::Utility::httpToGrpcStatus(429, absl::make_optional( - Grpc::Status::GrpcStatus::ResourceExhausted))); -} - TEST(GrpcCommonTest, HasGrpcContentType) { { Http::TestHeaderMapImpl headers{}; From 104bc64daa01e21226e5a65fc0d0da46d18ad60e Mon Sep 17 00:00:00 2001 From: Venil Noronha Date: Tue, 6 Nov 2018 11:54:18 -0800 Subject: [PATCH 5/5] Address review comments * Add release note on HTTP filter API change * Update rate_limit proto comment * Update test to use utility function instead of hard-coded string Signed-off-by: Venil Noronha --- .../config/filter/http/rate_limit/v2/rate_limit.proto | 5 +++-- docs/root/intro/version_history.rst | 2 ++ test/common/http/utility_test.cc | 9 ++++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto b/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto index 1174cae66d332..72635a74497f6 100644 --- a/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto +++ b/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto @@ -41,7 +41,8 @@ message RateLimit { // Defaults to false. bool failure_mode_deny = 5; - // Specifies whether a `RESOURCE_EXHAUSTED` code must be returned instead of - // the default `UNAVAILABLE` code for a rate limited gRPC call. + // Specifies whether a `RESOURCE_EXHAUSTED` gRPC code must be returned instead + // of the default `UNAVAILABLE` gRPC code for a rate limited gRPC call. The + // HTTP code will be 200 for a gRPC response. bool rate_limited_as_resource_exhausted = 6; } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 0b21280e233e2..397180dd8d4ba 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -15,6 +15,8 @@ Version history * http: no longer adding whitespace when appending X-Forwarded-For headers. **Warning**: this is not compatible with 1.7.0 builds prior to `9d3a4eb4ac44be9f0651fcc7f87ad98c538b01ee `_. See `#3611 `_ for details. +* http: augmented the `sendLocalReply` filter API to accept an optional `GrpcStatus` + value to override the default HTTP to gRPC status mapping. * network: removed the reference to `FilterState` in `Connection` in favor of `StreamInfo`. * logging: added missing [ in log prefix. * rate-limit: added :ref:`configuration ` diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 02fd37f73d276..8606859eeeee3 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -468,7 +468,8 @@ TEST(HttpUtility, SendLocalGrpcReply) { .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_EQ(headers.GrpcStatus()->value().c_str(), + std::to_string(enumToInt(Grpc::Status::GrpcStatus::Unknown))); EXPECT_NE(headers.GrpcMessage(), nullptr); EXPECT_STREQ(headers.GrpcMessage()->value().c_str(), "large"); })); @@ -482,7 +483,8 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { EXPECT_CALL(callbacks, encodeHeaders_(_, true)) .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { EXPECT_NE(headers.GrpcStatus(), nullptr); - EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "14"); // Unavailable + EXPECT_EQ(headers.GrpcStatus()->value().c_str(), + std::to_string(enumToInt(Grpc::Status::GrpcStatus::Unavailable))); })); Utility::sendLocalReply(true, callbacks, false, Http::Code::TooManyRequests, "", absl::nullopt, false); @@ -490,7 +492,8 @@ TEST(HttpUtility, RateLimitedGrpcStatus) { EXPECT_CALL(callbacks, encodeHeaders_(_, true)) .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void { EXPECT_NE(headers.GrpcStatus(), nullptr); - EXPECT_STREQ(headers.GrpcStatus()->value().c_str(), "8"); // ResourceExhausted + EXPECT_EQ(headers.GrpcStatus()->value().c_str(), + std::to_string(enumToInt(Grpc::Status::GrpcStatus::ResourceExhausted))); })); Utility::sendLocalReply( true, callbacks, false, Http::Code::TooManyRequests, "",