diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index 6642560694ea..fb1ec8e71afd 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -162,6 +162,6 @@ message ResponseFlagFilter { // This field is optional. If it is not specified, then any response flag will pass // the filter check. repeated string flags = 1 [(validate.rules).repeated .items.string = { - in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"] + in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX", "RLSE"] }]; } 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 220c1302041b..d79764745ab7 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 @@ -35,7 +35,6 @@ message RateLimit { // set, this defaults to 20ms. google.protobuf.Duration timeout = 4 [(gogoproto.stdduration) = true]; - // [#not-implemented-hide:] Hide from docs. // The filter's behaviour in case the rate limiting service does // not respond back. When it is set to true, Envoy will not allow traffic in case of // communication failure between rate limiting service and the proxy. diff --git a/api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto b/api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto index 386822507cab..fe579f01e444 100644 --- a/api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto +++ b/api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto @@ -27,7 +27,6 @@ message RateLimit { // set, this defaults to 20ms. google.protobuf.Duration timeout = 4 [(gogoproto.stdduration) = true]; - // [#not-implemented-hide:] Hide from docs. // The filter's behaviour in case the rate limiting service does // not respond back. When it is set to true, Envoy will not allow traffic in case of // communication failure between rate limiting service and the proxy. diff --git a/docs/root/configuration/http_filters/rate_limit_filter.rst b/docs/root/configuration/http_filters/rate_limit_filter.rst index dcac97e337cb..52754a018674 100644 --- a/docs/root/configuration/http_filters/rate_limit_filter.rst +++ b/docs/root/configuration/http_filters/rate_limit_filter.rst @@ -16,6 +16,9 @@ apply to a request. Each configuration results in a descriptor being sent to the If the rate limit service is called, and the response for any of the descriptors is over limit, a 429 response is returned. +If there is an error in calling rate limit service or rate limit service returns an error and :ref:`failure_mode_deny ` is +set to true, a 500 response is returned. + .. _config_http_filters_rate_limit_composing_actions: Composing Actions @@ -108,6 +111,8 @@ The buffer filter outputs statistics in the *cluster..rate ok, Counter, Total under limit responses from the rate limit service error, Counter, Total errors contacting the rate limit service over_limit, Counter, total over limit responses from the rate limit service + failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because + of :ref:`failure_mode_deny ` set to false." Runtime ------- diff --git a/docs/root/configuration/network_filters/rate_limit_filter.rst b/docs/root/configuration/network_filters/rate_limit_filter.rst index 4cefd6813699..ff9771bba806 100644 --- a/docs/root/configuration/network_filters/rate_limit_filter.rst +++ b/docs/root/configuration/network_filters/rate_limit_filter.rst @@ -25,6 +25,8 @@ following statistics: ok, Counter, Total under limit responses from the rate limit service cx_closed, Counter, Total connections closed due to an over limit response from the rate limit service active, Gauge, Total active requests to the rate limit service + failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because + of :ref:`failure_mode_deny ` set to false." Runtime ------- diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index 09345b3565cf..154a46b25488 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -48,8 +48,10 @@ enum ResponseFlag { RateLimited = 0x800, // Request was unauthorized by external authorization service. UnauthorizedExternalService = 0x1000, + // Unable to call Ratelimit service. + RateLimitServiceError = 0x2000, // ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG. - LastFlag = UnauthorizedExternalService + LastFlag = RateLimitServiceError }; /** diff --git a/source/common/request_info/utility.cc b/source/common/request_info/utility.cc index 27a01a6b2445..6d71182fff22 100644 --- a/source/common/request_info/utility.cc +++ b/source/common/request_info/utility.cc @@ -19,6 +19,7 @@ const std::string ResponseFlagUtils::DELAY_INJECTED = "DI"; const std::string ResponseFlagUtils::FAULT_INJECTED = "FI"; const std::string ResponseFlagUtils::RATE_LIMITED = "RL"; const std::string ResponseFlagUtils::UNAUTHORIZED_EXTERNAL_SERVICE = "UAEX"; +const std::string ResponseFlagUtils::RATELIMIT_SERVICE_ERROR = "RLSE"; void ResponseFlagUtils::appendString(std::string& result, const std::string& append) { if (result.empty()) { @@ -31,7 +32,7 @@ void ResponseFlagUtils::appendString(std::string& result, const std::string& app const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_info) { std::string result; - static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code."); if (request_info.hasResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); @@ -85,6 +86,10 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in appendString(result, UNAUTHORIZED_EXTERNAL_SERVICE); } + if (request_info.hasResponseFlag(ResponseFlag::RateLimitServiceError)) { + appendString(result, RATELIMIT_SERVICE_ERROR); + } + return result.empty() ? NONE : result; } @@ -104,6 +109,7 @@ absl::optional ResponseFlagUtils::toResponseFlag(const std::string {ResponseFlagUtils::FAULT_INJECTED, ResponseFlag::FaultInjected}, {ResponseFlagUtils::RATE_LIMITED, ResponseFlag::RateLimited}, {ResponseFlagUtils::UNAUTHORIZED_EXTERNAL_SERVICE, ResponseFlag::UnauthorizedExternalService}, + {ResponseFlagUtils::RATELIMIT_SERVICE_ERROR, ResponseFlag::RateLimitServiceError}, }; const auto& it = map.find(flag); if (it != map.end()) { diff --git a/source/common/request_info/utility.h b/source/common/request_info/utility.h index 879bccd196cf..209a8a963dde 100644 --- a/source/common/request_info/utility.h +++ b/source/common/request_info/utility.h @@ -34,6 +34,7 @@ class ResponseFlagUtils { const static std::string FAULT_INJECTED; const static std::string RATE_LIMITED; const static std::string UNAUTHORIZED_EXTERNAL_SERVICE; + const static std::string RATELIMIT_SERVICE_ERROR; }; /** diff --git a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc index 6804cdd9bd61..2453280c3580 100644 --- a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc @@ -85,7 +85,7 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( envoy::data::accesslog::v2::AccessLogCommon& common_access_log, const RequestInfo::RequestInfo& request_info) { - static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000, + static_assert(RequestInfo::ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code."); if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) { @@ -141,6 +141,10 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( envoy::data::accesslog::v2::ResponseFlags_Unauthorized_Reason:: ResponseFlags_Unauthorized_Reason_EXTERNAL_SERVICE); } + + if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError)) { + common_access_log.mutable_response_flags()->set_rate_limit_service_error(true); + } } void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 956bc56d210a..d1b31f67e726 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -147,6 +147,17 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header callbacks_->sendLocalReply(Http::Code::TooManyRequests, "", [this](Http::HeaderMap& headers) { addHeaders(headers); }); callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimited); + } else if (status == RateLimit::LimitStatus::Error) { + if (config_->failureModeAllow()) { + cluster_->statsScope().counter("ratelimit.failure_mode_allowed").inc(); + if (!initiating_call_) { + callbacks_->continueDecoding(); + } + } else { + state_ = State::Responded; + callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr); + callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError); + } } else if (!initiating_call_) { callbacks_->continueDecoding(); } diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h index 5d46592fd92f..4f411a797b0a 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.h +++ b/source/extensions/filters/http/ratelimit/ratelimit.h @@ -37,8 +37,8 @@ class FilterConfig { : domain_(config.domain()), stage_(static_cast(config.stage())), request_type_(config.request_type().empty() ? stringToType("both") : stringToType(config.request_type())), - local_info_(local_info), scope_(scope), runtime_(runtime), cm_(cm) {} - + local_info_(local_info), scope_(scope), runtime_(runtime), cm_(cm), + failure_mode_deny_(config.failure_mode_deny()) {} const std::string& domain() const { return domain_; } const LocalInfo::LocalInfo& localInfo() const { return local_info_; } uint64_t stage() const { return stage_; } @@ -47,6 +47,8 @@ class FilterConfig { Upstream::ClusterManager& cm() { return cm_; } FilterRequestType requestType() const { return request_type_; } + bool failureModeAllow() const { return !failure_mode_deny_; } + private: static FilterRequestType stringToType(const std::string& request_type) { if (request_type == "internal") { @@ -66,6 +68,7 @@ class FilterConfig { Stats::Scope& scope_; Runtime::Loader& runtime_; Upstream::ClusterManager& cm_; + const bool failure_mode_deny_; }; typedef std::shared_ptr FilterConfigSharedPtr; diff --git a/source/extensions/filters/network/ratelimit/ratelimit.cc b/source/extensions/filters/network/ratelimit/ratelimit.cc index dc33acdff773..6543292797d5 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.cc +++ b/source/extensions/filters/network/ratelimit/ratelimit.cc @@ -16,8 +16,7 @@ namespace RateLimitFilter { Config::Config(const envoy::config::filter::network::rate_limit::v2::RateLimit& config, Stats::Scope& scope, Runtime::Loader& runtime) : domain_(config.domain()), stats_(generateStats(config.stat_prefix(), scope)), - runtime_(runtime) { - + runtime_(runtime), failure_mode_deny_(config.failure_mode_deny()) { for (const auto& descriptor : config.descriptors()) { RateLimit::Descriptor new_descriptor; for (const auto& entry : descriptor.entries()) { @@ -85,11 +84,20 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&&) { break; } - // We fail open if there is an error contacting the service. if (status == RateLimit::LimitStatus::OverLimit && config_->runtime().snapshot().featureEnabled("ratelimit.tcp_filter_enforcing", 100)) { config_->stats().cx_closed_.inc(); filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); + } else if (status == RateLimit::LimitStatus::Error) { + if (config_->failureModeAllow()) { + config_->stats().failure_mode_allowed_.inc(); + if (!calling_limit_) { + filter_callbacks_->continueReading(); + } + } else { + config_->stats().cx_closed_.inc(); + filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); + } } else { // We can get completion inline, so only call continue if that isn't happening. if (!calling_limit_) { diff --git a/source/extensions/filters/network/ratelimit/ratelimit.h b/source/extensions/filters/network/ratelimit/ratelimit.h index 9ad7718a8185..a19de07b8b41 100644 --- a/source/extensions/filters/network/ratelimit/ratelimit.h +++ b/source/extensions/filters/network/ratelimit/ratelimit.h @@ -27,6 +27,7 @@ namespace RateLimitFilter { COUNTER(error) \ COUNTER(over_limit) \ COUNTER(ok) \ + COUNTER(failure_mode_allowed) \ COUNTER(cx_closed) \ GAUGE (active) // clang-format on @@ -49,6 +50,7 @@ class Config { const std::vector& descriptors() { return descriptors_; } Runtime::Loader& runtime() { return runtime_; } const InstanceStats& stats() { return stats_; } + bool failureModeAllow() const { return !failure_mode_deny_; }; private: static InstanceStats generateStats(const std::string& name, Stats::Scope& scope); @@ -57,6 +59,7 @@ class Config { std::vector descriptors_; const InstanceStats stats_; Runtime::Loader& runtime_; + const bool failure_mode_deny_; }; typedef std::shared_ptr ConfigSharedPtr; diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index f7d0ded0074d..7753db8004a9 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -813,11 +813,12 @@ name: envoy.file_access_log - FI - RL - UAEX + - RLSE config: path: /dev/null )EOF"; - static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000, + static_assert(RequestInfo::ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code."); std::vector all_response_flags = { @@ -834,6 +835,7 @@ name: envoy.file_access_log RequestInfo::ResponseFlag::FaultInjected, RequestInfo::ResponseFlag::RateLimited, RequestInfo::ResponseFlag::UnauthorizedExternalService, + RequestInfo::ResponseFlag::RateLimitServiceError, }; InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_); @@ -863,7 +865,7 @@ name: envoy.file_access_log "Proto constraint validation failed (AccessLogFilterValidationError.ResponseFlagFilter: " "[\"embedded message failed validation\"] | caused by " "ResponseFlagFilterValidationError.Flags[i]: [\"value must be in list \" [\"LH\" \"UH\" " - "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\"]]): " + "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\" \"RLSE\"]]): " "response_flag_filter {\n flags: \"UnsupportedFlag\"\n}\n"); } diff --git a/test/common/request_info/utility_test.cc b/test/common/request_info/utility_test.cc index d2e677d2d324..cecf3dd4aefe 100644 --- a/test/common/request_info/utility_test.cc +++ b/test/common/request_info/utility_test.cc @@ -14,7 +14,7 @@ namespace Envoy { namespace RequestInfo { TEST(ResponseFlagUtilsTest, toShortStringConversion) { - static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code."); std::vector> expected = { std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"), @@ -30,6 +30,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { std::make_pair(ResponseFlag::FaultInjected, "FI"), std::make_pair(ResponseFlag::RateLimited, "RL"), std::make_pair(ResponseFlag::UnauthorizedExternalService, "UAEX"), + std::make_pair(ResponseFlag::RateLimitServiceError, "RLSE"), }; for (const auto& test_case : expected) { @@ -58,7 +59,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { } TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) { - static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code."); std::vector> expected = { std::make_pair("LH", ResponseFlag::FailedLocalHealthCheck), @@ -74,6 +75,7 @@ TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) { std::make_pair("FI", ResponseFlag::FaultInjected), std::make_pair("RL", ResponseFlag::RateLimited), std::make_pair("UAEX", ResponseFlag::UnauthorizedExternalService), + std::make_pair("RLSE", ResponseFlag::RateLimitServiceError), }; EXPECT_FALSE(ResponseFlagUtils::toResponseFlag("NonExistentFlag").has_value()); diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc index 95cffa3e29a0..b3f7a0da39a3 100644 --- a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc @@ -434,6 +434,7 @@ TEST(responseFlagsToAccessLogResponseFlagsTest, All) { common_access_log_expected.mutable_response_flags()->mutable_unauthorized_details()->set_reason( envoy::data::accesslog::v2::ResponseFlags_Unauthorized_Reason:: ResponseFlags_Unauthorized_Reason_EXTERNAL_SERVICE); + common_access_log_expected.mutable_response_flags()->set_rate_limit_service_error(true); EXPECT_EQ(common_access_log_expected.DebugString(), common_access_log.DebugString()); } diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index 8d49e933ac4f..ba5837d96704 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -46,10 +46,10 @@ class HttpRateLimitFilterTest : public testing::Test { .WillByDefault(Return(true)); } - void SetUpTest(const std::string json) { - Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json); + void SetUpTest(const std::string& yaml) { envoy::config::filter::http::rate_limit::v2::RateLimit proto_config{}; - Config::FilterJson::translateHttpRateLimitFilter(*json_config, proto_config); + MessageUtil::loadFromYaml(yaml, proto_config); + config_.reset(new FilterConfig(proto_config, local_info_, stats_store_, runtime_, cm_)); client_ = new RateLimit::MockClient(); @@ -64,10 +64,13 @@ class HttpRateLimitFilterTest : public testing::Test { .emplace_back(vh_rate_limit_); } + const std::string fail_close_config_ = R"EOF( + domain: foo + failure_mode_deny: true + )EOF"; + const std::string filter_config_ = R"EOF( - { - "domain": "foo" - } + domain: foo )EOF"; FilterConfigSharedPtr config_; @@ -302,6 +305,39 @@ TEST_F(HttpRateLimitFilterTest, ImmediateOkResponse) { cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.ok").value()); } +TEST_F(HttpRateLimitFilterTest, ImmediateErrorResponse) { + SetUpTest(filter_config_); + InSequence s; + + EXPECT_CALL(vh_rate_limit_, populateDescriptors(_, _, _, _, _)) + .WillOnce(SetArgReferee<1>(descriptor_)); + + EXPECT_CALL(*client_, limit(_, "foo", + testing::ContainerEq(std::vector{ + {{{"descriptor_key", "descriptor_value"}}}}), + _)) + .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { + callbacks.complete(RateLimit::LimitStatus::Error, nullptr); + }))); + + EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_headers_)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->encode100ContinueHeaders(response_headers_)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers_, false)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data_, false)); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_headers_)); + + EXPECT_EQ( + 1U, + cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.error").value()); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("ratelimit.failure_mode_allowed") + .value()); +} + TEST_F(HttpRateLimitFilterTest, ErrorResponse) { SetUpTest(filter_config_); InSequence s; @@ -328,6 +364,37 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponse) { EXPECT_EQ( 1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.error").value()); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("ratelimit.failure_mode_allowed") + .value()); +} + +TEST_F(HttpRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { + SetUpTest(fail_close_config_); + InSequence s; + + EXPECT_CALL(route_rate_limit_, populateDescriptors(_, _, _, _, _)) + .WillOnce(SetArgReferee<1>(descriptor_)); + EXPECT_CALL(*client_, limit(_, _, _, _)) + .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { + request_callbacks_ = &callbacks; + }))); + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers_, false)); + + request_callbacks_->complete(RateLimit::LimitStatus::Error, nullptr); + + EXPECT_CALL(filter_callbacks_.request_info_, + setResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError)) + .Times(0); + + EXPECT_EQ( + 1U, + cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.error").value()); + EXPECT_EQ(0U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("ratelimit.failure_mode_allowed") + .value()); } TEST_F(HttpRateLimitFilterTest, LimitResponse) { diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc index ec8aa55408e4..703d56d4c8d6 100644 --- a/test/extensions/filters/network/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/ratelimit/ratelimit_test.cc @@ -33,26 +33,16 @@ namespace RateLimitFilter { class RateLimitFilterTest : public testing::Test { public: - RateLimitFilterTest() { - std::string json = R"EOF( - { - "domain": "foo", - "descriptors": [ - [{"key": "hello", "value": "world"}, {"key": "foo", "value": "bar"}], - [{"key": "foo2", "value": "bar2"}] - ], - "stat_prefix": "name" - } - )EOF"; + RateLimitFilterTest() {} + void SetUpTest(const std::string& yaml) { ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.tcp_filter_enabled", 100)) .WillByDefault(Return(true)); ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.tcp_filter_enforcing", 100)) .WillByDefault(Return(true)); - Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json); envoy::config::filter::network::rate_limit::v2::RateLimit proto_config{}; - Envoy::Config::FilterJson::translateTcpRateLimitFilter(*json_config, proto_config); + MessageUtil::loadFromYaml(yaml, proto_config); config_.reset(new Config(proto_config, stats_store_, runtime_)); client_ = new RateLimit::MockClient(); filter_.reset(new Filter(config_, RateLimit::ClientPtr{client_})); @@ -69,6 +59,35 @@ class RateLimitFilterTest : public testing::Test { } } + const std::string filter_config_ = R"EOF( +domain: foo +descriptors: +- entries: + - key: hello + value: world + - key: foo + value: bar +- entries: + - key: foo2 + value: bar2 +stat_prefix: name +)EOF"; + + const std::string fail_close_config_ = R"EOF( +domain: foo +descriptors: +- entries: + - key: hello + value: world + - key: foo + value: bar +- entries: + - key: foo2 + value: bar2 +stat_prefix: name +failure_mode_deny: true +)EOF"; + Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; ConfigSharedPtr config_; @@ -97,6 +116,7 @@ TEST_F(RateLimitFilterTest, BadRatelimitConfig) { TEST_F(RateLimitFilterTest, OK) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(*client_, limit(_, "foo", testing::ContainerEq(std::vector{ @@ -125,6 +145,7 @@ TEST_F(RateLimitFilterTest, OK) { TEST_F(RateLimitFilterTest, OverLimit) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(*client_, limit(_, "foo", _, _)) .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { @@ -148,6 +169,7 @@ TEST_F(RateLimitFilterTest, OverLimit) { TEST_F(RateLimitFilterTest, OverLimitNotEnforcing) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(*client_, limit(_, "foo", _, _)) .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { @@ -174,6 +196,7 @@ TEST_F(RateLimitFilterTest, OverLimitNotEnforcing) { TEST_F(RateLimitFilterTest, Error) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(*client_, limit(_, "foo", _, _)) .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { @@ -194,10 +217,12 @@ TEST_F(RateLimitFilterTest, Error) { EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.total").value()); EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.error").value()); + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.failure_mode_allowed").value()); } TEST_F(RateLimitFilterTest, Disconnect) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(*client_, limit(_, "foo", _, _)) .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { @@ -216,6 +241,7 @@ TEST_F(RateLimitFilterTest, Disconnect) { TEST_F(RateLimitFilterTest, ImmediateOK) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); EXPECT_CALL(*client_, limit(_, "foo", _, _)) @@ -235,8 +261,32 @@ TEST_F(RateLimitFilterTest, ImmediateOK) { EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.ok").value()); } +TEST_F(RateLimitFilterTest, ImmediateError) { + InSequence s; + SetUpTest(filter_config_); + + EXPECT_CALL(filter_callbacks_, continueReading()).Times(0); + EXPECT_CALL(*client_, limit(_, "foo", _, _)) + .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { + callbacks.complete(RateLimit::LimitStatus::Error, nullptr); + }))); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.error").value()); + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.failure_mode_allowed").value()); +} + TEST_F(RateLimitFilterTest, RuntimeDisable) { InSequence s; + SetUpTest(filter_config_); EXPECT_CALL(runtime_.snapshot_, featureEnabled("ratelimit.tcp_filter_enabled", 100)) .WillOnce(Return(false)); @@ -247,6 +297,30 @@ TEST_F(RateLimitFilterTest, RuntimeDisable) { EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); } +TEST_F(RateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) { + InSequence s; + SetUpTest(fail_close_config_); + + EXPECT_CALL(*client_, limit(_, "foo", _, _)) + .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void { + request_callbacks_ = &callbacks; + }))); + + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection()); + Buffer::OwnedImpl data("hello"); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data, false)); + request_callbacks_->complete(RateLimit::LimitStatus::Error, nullptr); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(data, false)); + + EXPECT_CALL(*client_, cancel()).Times(0); + filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.total").value()); + EXPECT_EQ(1U, stats_store_.counter("ratelimit.name.error").value()); + EXPECT_EQ(0U, stats_store_.counter("ratelimit.name.failure_mode_allowed").value()); +} + } // namespace RateLimitFilter } // namespace NetworkFilters } // namespace Extensions diff --git a/test/integration/ratelimit_integration_test.cc b/test/integration/ratelimit_integration_test.cc index aa6517b0e0fe..02cbbaae183e 100644 --- a/test/integration/ratelimit_integration_test.cc +++ b/test/integration/ratelimit_integration_test.cc @@ -44,8 +44,14 @@ class RatelimitIntegrationTest : public HttpIntegrationTest, } void initialize() override { - config_helper_.addFilter( - "{ name: envoy.rate_limit, config: { domain: some_domain, timeout: 0.5s } }"); + if (failure_mode_deny_) { + config_helper_.addFilter("{ name: envoy.rate_limit, config: { domain: some_domain, " + "failure_mode_deny: true, timeout: 0.5s } }"); + + } else { + config_helper_.addFilter( + "{ name: envoy.rate_limit, config: { domain: some_domain, timeout: 0.5s } }"); + } config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { auto* ratelimit_cluster = bootstrap.mutable_static_resources()->add_clusters(); ratelimit_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); @@ -176,10 +182,19 @@ class RatelimitIntegrationTest : public HttpIntegrationTest, const uint64_t request_size_ = 1024; const uint64_t response_size_ = 512; + bool failure_mode_deny_ = false; +}; + +// Test that verifies failure mode cases. +class RatelimitFailureModeIntegrationTest : public RatelimitIntegrationTest { +public: + RatelimitFailureModeIntegrationTest() { failure_mode_deny_ = true; } }; INSTANTIATE_TEST_CASE_P(IpVersionsClientType, RatelimitIntegrationTest, RATELIMIT_GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_CASE_P(IpVersionsClientType, RatelimitFailureModeIntegrationTest, + RATELIMIT_GRPC_CLIENT_INTEGRATION_PARAMS); TEST_P(RatelimitIntegrationTest, Ok) { initiateClientConnection(); @@ -268,6 +283,7 @@ TEST_P(RatelimitIntegrationTest, Error) { EXPECT_EQ(nullptr, test_server_->counter("cluster.cluster_0.ratelimit.ok")); EXPECT_EQ(nullptr, test_server_->counter("cluster.cluster_0.ratelimit.over_limit")); EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.ratelimit.error")->value()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.ratelimit.failure_mode_allowed")->value()); } TEST_P(RatelimitIntegrationTest, Timeout) { @@ -316,5 +332,19 @@ TEST_P(RatelimitIntegrationTest, FailedConnect) { cleanup(); } +TEST_P(RatelimitFailureModeIntegrationTest, ErrorWithFailureModeOff) { + initiateClientConnection(); + waitForRatelimitRequest(); + ratelimit_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "503"}}, true); + // Rate limiter fail closed + waitForFailedUpstreamResponse(500); + cleanup(); + + EXPECT_EQ(nullptr, test_server_->counter("cluster.cluster_0.ratelimit.ok")); + EXPECT_EQ(nullptr, test_server_->counter("cluster.cluster_0.ratelimit.over_limit")); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.ratelimit.error")->value()); + EXPECT_EQ(nullptr, test_server_->counter("cluster.cluster_0.ratelimit.failure_mode_allowed")); +} + } // namespace } // namespace Envoy