From ffddd03ece01d9a542037bbf275e81a714fd6b8c Mon Sep 17 00:00:00 2001 From: "Adi (Suissa) Peleg" Date: Thu, 7 Sep 2023 12:08:14 -0400 Subject: [PATCH] xds rate limit: fixing lower bound for xDS rate limiting (#28778) Current fill_rate must be above 0.0 (PGV constraint). However, a low double value can cause an infinite value when computing 1/fill_rate and its cast to uint64_t fails. This PR changes the minimal fill_rate to be once-per-year, and if a lower value is given, it is overridden and set to once-per-year. Alternatives considered: changing the PGV value to 3.1709792e-8 (once-per-year). Risk Level: Low - minor change in behavior Testing: Added fuzz test case Docs Changes: Updated API comments Release Notes: Added. Platform Specific Features: N/A Fixes fuzz bug 60974 Signed-off-by: Adi Suissa-Peleg --- api/envoy/config/core/v3/config_source.proto | 3 +- .../network/redis_proxy/v3/redis_proxy.proto | 2 +- changelogs/current.yaml | 9 ++++ source/common/common/token_bucket_impl.cc | 9 +++- test/common/common/token_bucket_impl_test.cc | 16 +++++++ .../network/redis_proxy/config_test.cc | 43 +++++++++++++++++++ test/server/server_corpus/low_fill_rate | 38 ++++++++++++++++ 7 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 test/server/server_corpus/low_fill_rate diff --git a/api/envoy/config/core/v3/config_source.proto b/api/envoy/config/core/v3/config_source.proto index c12930135a1b..70204bad9eba 100644 --- a/api/envoy/config/core/v3/config_source.proto +++ b/api/envoy/config/core/v3/config_source.proto @@ -152,7 +152,8 @@ message RateLimitSettings { google.protobuf.UInt32Value max_tokens = 1; // Rate at which tokens will be filled per second. If not set, a default fill rate of 10 tokens - // per second will be used. + // per second will be used. The minimal fill rate is once per year. Lower + // fill rates will be set to once per year. google.protobuf.DoubleValue fill_rate = 2 [(validate.rules).double = {gt: 0.0}]; } diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index 3a64c9cc8665..fba1c786f7c3 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -230,7 +230,7 @@ message RedisProxy { // from client reconnection storm. message ConnectionRateLimit { // Reconnection rate per sec. Rate limiting is implemented with TokenBucket. - uint32 connection_rate_limit_per_sec = 1; + uint32 connection_rate_limit_per_sec = 1 [(validate.rules).uint32 = {gt: 0}]; } reserved 2; diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f457f119e35b..6e6d5752fe92 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -59,6 +59,15 @@ minor_behavior_changes: Enable copying response_code from the upstream stream_info onto the downstream stream_info. This behavior can be reverted by setting runtime guard ``envoy.reloadable_features.copy_response_code_to_downstream_stream_info`` to false. +- area: xds + change: | + Set the lower bound of :ref:`fill_rate ` + to once per year. Values lower than once per year will automatically be set to that value. +- area: redis + change: | + The redis network filter :ref:`connection_rate_limit_per_sec + ` + must be greater than 0. A config that sets this value to 0 will be rejected. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/source/common/common/token_bucket_impl.cc b/source/common/common/token_bucket_impl.cc index eaf6f06d0c9e..f813d426d04f 100644 --- a/source/common/common/token_bucket_impl.cc +++ b/source/common/common/token_bucket_impl.cc @@ -4,9 +4,14 @@ namespace Envoy { +namespace { +// The minimal fill rate will be one second every year. +constexpr double kMinFillRate = 1.0 / (365 * 24 * 60 * 60); +} // namespace + TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, TimeSource& time_source, double fill_rate) - : max_tokens_(max_tokens), fill_rate_(std::abs(fill_rate)), tokens_(max_tokens), - last_fill_(time_source.monotonicTime()), time_source_(time_source) {} + : max_tokens_(max_tokens), fill_rate_(std::max(std::abs(fill_rate), kMinFillRate)), + tokens_(max_tokens), last_fill_(time_source.monotonicTime()), time_source_(time_source) {} uint64_t TokenBucketImpl::consume(uint64_t tokens, bool allow_partial) { if (tokens_ < max_tokens_) { diff --git a/test/common/common/token_bucket_impl_test.cc b/test/common/common/token_bucket_impl_test.cc index c3d25a4c2b27..66308cfded3a 100644 --- a/test/common/common/token_bucket_impl_test.cc +++ b/test/common/common/token_bucket_impl_test.cc @@ -110,4 +110,20 @@ TEST_F(TokenBucketImplTest, ConsumeAndNextToken) { EXPECT_EQ(time_to_next_token, token_bucket.nextTokenAvailable()); } +// Validate that a minimal refresh time is 1 year. +TEST_F(TokenBucketImplTest, YearlyMinRefillRate) { + constexpr uint64_t seconds_per_year = 365 * 24 * 60 * 60; + // Set the fill rate to be 2 years. + TokenBucketImpl token_bucket{1, time_system_, 1.0 / (seconds_per_year * 2)}; + + // Consume first token. + EXPECT_EQ(1, token_bucket.consume(1, false)); + + // Less than a year should still have no tokens. + time_system_.setMonotonicTime(std::chrono::seconds(seconds_per_year - 1)); + EXPECT_EQ(0, token_bucket.consume(1, false)); + time_system_.setMonotonicTime(std::chrono::seconds(seconds_per_year)); + EXPECT_EQ(1, token_bucket.consume(1, false)); +} + } // namespace Envoy diff --git a/test/extensions/filters/network/redis_proxy/config_test.cc b/test/extensions/filters/network/redis_proxy/config_test.cc index 5bfe03cd6985..f0d220af674c 100644 --- a/test/extensions/filters/network/redis_proxy/config_test.cc +++ b/test/extensions/filters/network/redis_proxy/config_test.cc @@ -147,6 +147,49 @@ stat_prefix: foo cb(connection); } +// Validates that a value of connection_rate_limit above 0 isn't rejected. +TEST(RedisProxyFilterConfigFactoryTest, ValidConnectionRateLimit) { + const std::string yaml = R"EOF( +prefix_routes: + catch_all_route: + cluster: fake_cluster +stat_prefix: foo +settings: + op_timeout: 0.02s + connection_rate_limit: + connection_rate_limit_per_sec: 1 + )EOF"; + + envoy::extensions::filters::network::redis_proxy::v3::RedisProxy proto_config; + TestUtility::loadFromYamlAndValidate(yaml, proto_config); + NiceMock context; + RedisProxyFilterConfigFactory factory; + Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); + EXPECT_TRUE(factory.isTerminalFilterByProto(proto_config, context.getServerFactoryContext())); + Network::MockConnection connection; + EXPECT_CALL(connection, addReadFilter(_)); + cb(connection); +} + +// Validates that a value of connection_rate_limit 0 is rejected. +TEST(RedisProxyFilterConfigFactoryTest, InvalidConnectionRateLimit) { + const std::string yaml = R"EOF( +prefix_routes: + catch_all_route: + cluster: fake_cluster +stat_prefix: foo +settings: + op_timeout: 0.02s + connection_rate_limit: + connection_rate_limit_per_sec: 0 + )EOF"; + + envoy::extensions::filters::network::redis_proxy::v3::RedisProxy proto_config; + EXPECT_THROW_WITH_REGEX(TestUtility::loadFromYamlAndValidate(yaml, proto_config), + ProtoValidationException, + "ConnectionRateLimitPerSec: value must be greater than 0"); +} + } // namespace RedisProxy } // namespace NetworkFilters } // namespace Extensions diff --git a/test/server/server_corpus/low_fill_rate b/test/server/server_corpus/low_fill_rate new file mode 100644 index 000000000000..a0bfde7ab283 --- /dev/null +++ b/test/server/server_corpus/low_fill_rate @@ -0,0 +1,38 @@ +node { + id: "\00096\000@" + cluster: "@" +} +dynamic_resources { + ads_config { + api_type: GRPC + grpc_services { + google_grpc { + target_uri: "127_7.0.1" + stat_prefix: "\t`" + } + } + rate_limit_settings { + max_tokens { + } + fill_rate { + value: 2.47032822920623e-323 + } + } + transport_api_version: V3 + } +} +flags_path: "`" +layered_runtime { + layers { + name: "\337h\177\177\177\177\177\177\177~>" + rtds_layer { + name: "\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\0018592863541252882453177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177\177?\177\177\177\177\177\177\177\177Q177\177" + rtds_config { + ads { + } + resource_api_version: V3 + } + } + } +} +header_prefix: "`"