xds rate limit: fixing lower bound for xDS rate limiting (envoyproxy#…

…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 <[email protected]>

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}];
 }
 

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;

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 <envoy_v3_api_field_config.core.v3.RateLimitSettings.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
+    <envoy_v3_api_field_extensions.filters.network.redis_proxy.v3.RedisProxy.ConnectionRateLimit.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*

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_) {

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

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<Server::Configuration::MockFactoryContext> 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