From 6ff6a4b2be943750b92180f0518d8dd1cee21df5 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Wed, 16 Oct 2024 11:08:13 +0530 Subject: [PATCH 01/41] Http LocalRateLimit: Add dynamic token bucket support Signed-off-by: Vikas Choudhary (vikasc) --- .../common/ratelimit/v3/ratelimit.proto | 4 +- source/common/common/token_bucket_impl.h | 6 + .../local_ratelimit/local_ratelimit_impl.cc | 127 +++++++++++++++++- .../local_ratelimit/local_ratelimit_impl.h | 25 +++- .../http/local_ratelimit/local_ratelimit.cc | 4 +- .../http/local_ratelimit/local_ratelimit.h | 6 +- 6 files changed, 158 insertions(+), 14 deletions(-) diff --git a/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto b/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto index 73d729adc269c..303e00da13888 100644 --- a/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto +++ b/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto @@ -101,8 +101,8 @@ message RateLimitDescriptor { // Descriptor key. string key = 1 [(validate.rules).string = {min_len: 1}]; - // Descriptor value. - string value = 2 [(validate.rules).string = {min_len: 1}]; + // Descriptor value. Blank value is treated as wildcard to create dynamic token buckets for each unique value. + string value = 2 [(validate.rules).string = {min_len: 0}]; } // Override rate limit to apply to this descriptor instead of the limit diff --git a/source/common/common/token_bucket_impl.h b/source/common/common/token_bucket_impl.h index 673f0a74e2b37..c56c24cd1c9a2 100644 --- a/source/common/common/token_bucket_impl.h +++ b/source/common/common/token_bucket_impl.h @@ -78,6 +78,12 @@ class AtomicTokenBucketImpl { return consumed; } + double secsSinceLastConsume() { + double time_old = time_in_seconds_.load(std::memory_order_relaxed); + const double time_now = timeNowInSeconds(); + return time_now - time_old; + } + /** * Consumes one tokens from the bucket. * @return true if the token is consumed, false otherwise. diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 2537febba9713..f431c3e7e6888 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -159,7 +159,8 @@ AtomicTokenBucket::AtomicTokenBucket(uint32_t max_tokens, uint32_t tokens_per_fi TimeSource& time_source) : token_bucket_(max_tokens, time_source, // Calculate the fill rate in tokens per second. - tokens_per_fill / std::chrono::duration(fill_interval).count()) {} + tokens_per_fill / std::chrono::duration(fill_interval).count()), + fill_interval_(fill_interval) {} bool AtomicTokenBucket::consume(double factor) { ASSERT(!(factor <= 0.0 || factor > 1.0)); @@ -167,6 +168,11 @@ bool AtomicTokenBucket::consume(double factor) { return token_bucket_.consume(cb) != 0.0; } +bool AtomicTokenBucket::isIdle() { + return token_bucket_.secsSinceLastConsume() > + 3 * std::chrono::duration(fill_interval_).count(); +} + LocalRateLimiterImpl::LocalRateLimiterImpl( const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher, @@ -176,6 +182,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( : fill_timer_(fill_interval > std::chrono::milliseconds(0) ? dispatcher.createTimer([this] { onFillTimer(); }) : nullptr), + dynamic_desc_idle_timer_(dispatcher.createTimer([this] { onDynDescIdleTimer(); })), time_source_(dispatcher.timeSource()), share_provider_(std::move(shared_provider)), always_consume_default_token_bucket_(always_consume_default_token_bucket), no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( @@ -197,6 +204,10 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( fill_timer_->enableTimer(default_token_bucket_->fillInterval()); } + if (dynamic_desc_idle_timer_) { + dynamic_desc_idle_timer_->enableTimer(std::chrono::milliseconds(5000)); + } + for (const auto& descriptor : descriptors) { RateLimit::LocalDescriptor new_descriptor; new_descriptor.entries_.reserve(descriptor.entries_size()); @@ -241,6 +252,24 @@ LocalRateLimiterImpl::~LocalRateLimiterImpl() { if (fill_timer_ != nullptr) { fill_timer_->disableTimer(); } + if (dynamic_desc_idle_timer_ != nullptr) { + dynamic_desc_idle_timer_->disableTimer(); + } +} + +void LocalRateLimiterImpl::onDynDescIdleTimer() { + absl::WriterMutexLock lock(&dyn_desc_lock_); + for (auto it = dynamic_descriptors_.begin(), end = dynamic_descriptors_.end(); it != end;) { + // `erase()` will invalidate `it`, so advance `it` first. + auto copy_it = it++; + if (copy_it->second->isIdle()) { + dynamic_descriptors_.erase(copy_it); + ENVOY_LOG(trace, "cleaned up idle descriptor: {}._ current size: {}", + copy_it->first.toString(), dynamic_descriptors_.size()); + } + } + + dynamic_desc_idle_timer_->enableTimer(std::chrono::milliseconds(5000)); } void LocalRateLimiterImpl::onFillTimer() { @@ -257,12 +286,18 @@ void LocalRateLimiterImpl::onFillTimer() { for (const auto& descriptor : descriptors_) { descriptor.second->onFillTimer(refill_counter_, share_factor); } + { + absl::ReaderMutexLock lock(&dyn_desc_lock_); + for (const auto& descriptor : dynamic_descriptors_) { + descriptor.second->onFillTimer(refill_counter_, share_factor); + } + } fill_timer_->enableTimer(default_token_bucket_->fillInterval()); } LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( - absl::Span request_descriptors) const { + absl::Span request_descriptors) { // In most cases the request descriptors has only few elements. We use a inlined vector to // avoid heap allocation. @@ -273,6 +308,65 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( auto iter = descriptors_.find(request_descriptor); if (iter != descriptors_.end()) { matched_descriptors.push_back(iter->second.get()); + } else { + { + absl::ReaderMutexLock lock(&dyn_desc_lock_); + // find request descriptor in the existing dynamic descriptors. If it exists, add bucket to + // matched_descriptors. + auto dynamic_iter = dynamic_descriptors_.find(request_descriptor); + if (dynamic_iter != dynamic_descriptors_.end()) { + matched_descriptors.push_back(dynamic_iter->second.get()); + continue; + } + } + + // If it does not exist, It could be first request for a dynamic descriptor. Verify it matches + // any of the user descriptors by key and value as blank. If it does not match, skip to next + // entry. check if it matches any of the user descriptors by key and value as blank. If it + // does not match, skip to next entry. If it matches, this is the dynamic descriptor case. + for (const auto& pair : descriptors_) { + auto user_descriptor = pair.first; + auto user_bucket = pair.second.get(); + if (user_descriptor.entries_.size() != request_descriptor.entries_.size()) { + continue; + } + RateLimit::LocalDescriptor new_descriptor; + bool wildcard_found = false; + new_descriptor.entries_.reserve(user_descriptor.entries_.size()); + wildcard_found = compareDescriptorEntries( + request_descriptor.entries_, user_descriptor.entries_, new_descriptor.entries_); + + if (!wildcard_found) { + continue; + } + RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; + if (no_timer_based_rate_limit_token_bucket_) { + ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); + ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", + user_bucket->maxTokens(), user_bucket->fillRate(), + std::chrono::duration(user_bucket->fillInterval()).count()); + per_descriptor_token_bucket = std::make_shared( + user_bucket->maxTokens(), + uint32_t(user_bucket->fillRate() * + std::chrono::duration(user_bucket->fillInterval()).count()), + /*1000 milliseconds*/ user_bucket->fillInterval(), time_source_); + } else { + per_descriptor_token_bucket = std::make_shared( + user_bucket->maxTokens(), uint32_t(user_bucket->fillRate()), + std::chrono::milliseconds(1000), user_bucket->multiplier(), *this); + } + + { + absl::WriterMutexLock lock(&dyn_desc_lock_); + auto result = dynamic_descriptors_.emplace(std::move(new_descriptor), + std::move(per_descriptor_token_bucket)); + if (!result.second) { + throw EnvoyException(absl::StrCat("duplicate descriptor in the local rate descriptor: ", + result.first->first.toString())); + } + matched_descriptors.push_back(result.first->second.get()); + } + } } } @@ -294,6 +388,10 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( // If the request is forbidden by a descriptor, return the result and the descriptor // token bucket. return {false, makeOptRefFromPtr(descriptor)}; + } else { + ENVOY_LOG(debug, + "request allowed by descriptor with fill rate: {}, maxToken: {}, remainingToken {}", + descriptor->fillRate(), descriptor->maxTokens(), descriptor->remainingTokens()); } } @@ -316,6 +414,31 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( return {true, makeOptRefFromPtr(matched_descriptors[0])}; } +bool LocalRateLimiterImpl::compareDescriptorEntries( + const std::vector& request_entries, + const std::vector& user_entries, + std::vector& new_descriptor_entries) { + // Check for equality of sizes + if (request_entries.size() != user_entries.size()) { + return false; + } + + bool has_empty_value = false; + for (size_t i = 0; i < request_entries.size(); ++i) { + // Check if the keys are equal + if (request_entries[i].key_ != user_entries[i].key_) { + return false; + } + + // Check for empty value in user entries + if (user_entries[i].value_.empty()) { + has_empty_value = true; + } + new_descriptor_entries.push_back({request_entries[i].key_, request_entries[i].value_}); + } + return has_empty_value; +} + } // namespace LocalRateLimit } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 34ecf1af885b6..b4f8537fac7c0 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -70,9 +70,11 @@ class TokenBucketContext { class RateLimitTokenBucket : public TokenBucketContext { public: virtual bool consume(double factor = 1.0) PURE; + virtual bool isIdle() PURE; virtual void onFillTimer(uint64_t refill_counter, double factor = 1.0) PURE; virtual std::chrono::milliseconds fillInterval() const PURE; virtual double fillRate() const PURE; + virtual uint64_t multiplier() const { return 1; } }; using RateLimitTokenBucketSharedPtr = std::shared_ptr; @@ -87,9 +89,11 @@ class TimerTokenBucket : public RateLimitTokenBucket { // RateLimitTokenBucket bool consume(double factor) override; + bool isIdle() override { return false; /* TODO: implement*/ } void onFillTimer(uint64_t refill_counter, double factor) override; std::chrono::milliseconds fillInterval() const override { return fill_interval_; } double fillRate() const override { return fill_rate_; } + uint64_t multiplier() const override { return multiplier_; } uint32_t maxTokens() const override { return max_tokens_; } uint32_t remainingTokens() const override { return tokens_.load(); } absl::optional remainingFillInterval() const override; @@ -109,15 +113,17 @@ class TimerTokenBucket : public RateLimitTokenBucket { const double fill_rate_{}; }; -class AtomicTokenBucket : public RateLimitTokenBucket { +class AtomicTokenBucket : public RateLimitTokenBucket, + public Logger::Loggable { public: AtomicTokenBucket(uint32_t max_tokens, uint32_t tokens_per_fill, std::chrono::milliseconds fill_interval, TimeSource& time_source); // RateLimitTokenBucket bool consume(double factor) override; + bool isIdle() override; void onFillTimer(uint64_t, double) override {} - std::chrono::milliseconds fillInterval() const override { return {}; } + std::chrono::milliseconds fillInterval() const override { return fill_interval_; } double fillRate() const override { return token_bucket_.fillRate(); } uint32_t maxTokens() const override { return static_cast(token_bucket_.maxTokens()); } uint32_t remainingTokens() const override { @@ -127,9 +133,10 @@ class AtomicTokenBucket : public RateLimitTokenBucket { private: AtomicTokenBucketImpl token_bucket_; + const std::chrono::milliseconds fill_interval_; }; -class LocalRateLimiterImpl { +class LocalRateLimiterImpl : public Logger::Loggable { public: struct Result { bool allowed{}; @@ -145,16 +152,26 @@ class LocalRateLimiterImpl { ShareProviderSharedPtr shared_provider = nullptr); ~LocalRateLimiterImpl(); - Result requestAllowed(absl::Span request_descriptors) const; + Result requestAllowed(absl::Span request_descriptors); private: void onFillTimer(); + bool compareDescriptorEntries(const std::vector& request_entries, + const std::vector& user_entries, + std::vector& new_descriptor_entries); + void onDynDescIdleTimer(); RateLimitTokenBucketSharedPtr default_token_bucket_; const Event::TimerPtr fill_timer_; + const Event::TimerPtr dynamic_desc_idle_timer_; TimeSource& time_source_; RateLimit::LocalDescriptor::Map descriptors_; + + mutable absl::Mutex dyn_desc_lock_; + RateLimit::LocalDescriptor::Map + dynamic_descriptors_ ABSL_GUARDED_BY(dyn_desc_lock_); + // Refill counter is incremented per each refill timer hit. uint64_t refill_counter_{0}; diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 0e12da02bfdd5..5442046562bf1 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -220,11 +220,11 @@ Filter::requestAllowed(absl::Span request_desc : used_config_->requestAllowed(request_descriptors); } -const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionRateLimiter() { +Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionRateLimiter() { ASSERT(used_config_->rateLimitPerConnection()); auto typed_state = - decoder_callbacks_->streamInfo().filterState()->getDataReadOnly( + decoder_callbacks_->streamInfo().filterState()->getDataMutable( PerConnectionRateLimiter::key()); if (typed_state == nullptr) { diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index 2b3af4ec7fb80..507b5a0a72332 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -59,9 +59,7 @@ class PerConnectionRateLimiter : public StreamInfo::FilterState::Object { : rate_limiter_(fill_interval, max_tokens, tokens_per_fill, dispatcher, descriptor, always_consume_default_token_bucket) {} static const std::string& key(); - const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& value() const { - return rate_limiter_; - } + Filters::Common::LocalRateLimit::LocalRateLimiterImpl& value() { return rate_limiter_; } private: Filters::Common::LocalRateLimit::LocalRateLimiterImpl rate_limiter_; @@ -193,7 +191,7 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable& descriptors, Http::RequestHeaderMap& headers); VhRateLimitOptions getVirtualHostRateLimitOption(const Router::RouteConstSharedPtr& route); - const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& getPerConnectionRateLimiter(); + Filters::Common::LocalRateLimit::LocalRateLimiterImpl& getPerConnectionRateLimiter(); Filters::Common::LocalRateLimit::LocalRateLimiterImpl::Result requestAllowed(absl::Span request_descriptors); From 71ecf21508d8e2de62553526fa648e6065fec992 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Wed, 16 Oct 2024 14:02:39 +0530 Subject: [PATCH 02/41] comments Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/local_ratelimit_impl.cc | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index f431c3e7e6888..b9072a27bae35 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -309,10 +309,13 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( if (iter != descriptors_.end()) { matched_descriptors.push_back(iter->second.get()); } else { + // If the request descriptor is not found in the user descriptors, it could be a wildcard case + // where value is left blank in the user configured descriptor entry. In this case, we need to + // check if it matches any of the dynamic descriptors. { absl::ReaderMutexLock lock(&dyn_desc_lock_); - // find request descriptor in the existing dynamic descriptors. If it exists, add bucket to - // matched_descriptors. + // find request descriptor in the existing dynamic descriptors. If it exists, add token + // bucket to matched_descriptors. auto dynamic_iter = dynamic_descriptors_.find(request_descriptor); if (dynamic_iter != dynamic_descriptors_.end()) { matched_descriptors.push_back(dynamic_iter->second.get()); @@ -320,10 +323,8 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( } } - // If it does not exist, It could be first request for a dynamic descriptor. Verify it matches - // any of the user descriptors by key and value as blank. If it does not match, skip to next - // entry. check if it matches any of the user descriptors by key and value as blank. If it - // does not match, skip to next entry. If it matches, this is the dynamic descriptor case. + // If it does not exist, then it could be the first request for a dynamic descriptor. Verify + // it matches any of the user configured descriptors by key where value is left blank for (const auto& pair : descriptors_) { auto user_descriptor = pair.first; auto user_bucket = pair.second.get(); @@ -349,7 +350,7 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( user_bucket->maxTokens(), uint32_t(user_bucket->fillRate() * std::chrono::duration(user_bucket->fillInterval()).count()), - /*1000 milliseconds*/ user_bucket->fillInterval(), time_source_); + user_bucket->fillInterval(), time_source_); } else { per_descriptor_token_bucket = std::make_shared( user_bucket->maxTokens(), uint32_t(user_bucket->fillRate()), @@ -414,6 +415,8 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( return {true, makeOptRefFromPtr(matched_descriptors[0])}; } +// Compare the request descriptor entries with the user descriptor entries. If all non-empty user +// descriptor values match the request descriptor values, return true and fill the new descriptor bool LocalRateLimiterImpl::compareDescriptorEntries( const std::vector& request_entries, const std::vector& user_entries, @@ -430,6 +433,11 @@ bool LocalRateLimiterImpl::compareDescriptorEntries( return false; } + // all non-blank user values must match the request values + if (!user_entries[i].value_.empty() && user_entries[i].value_ != request_entries[i].value_) { + return false; + } + // Check for empty value in user entries if (user_entries[i].value_.empty()) { has_empty_value = true; From 365e7c457da1ce79e0a286bd85ed7b4ad737b2cc Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Wed, 16 Oct 2024 14:06:28 +0530 Subject: [PATCH 03/41] updates Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/common/local_ratelimit/local_ratelimit_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index b9072a27bae35..c67dc11b9c795 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -353,8 +353,10 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( user_bucket->fillInterval(), time_source_); } else { per_descriptor_token_bucket = std::make_shared( - user_bucket->maxTokens(), uint32_t(user_bucket->fillRate()), - std::chrono::milliseconds(1000), user_bucket->multiplier(), *this); + user_bucket->maxTokens(), + uint32_t(user_bucket->fillRate() * + std::chrono::duration(user_bucket->fillInterval()).count()), + user_bucket->fillInterval(), user_bucket->multiplier(), *this); } { From cde6760eb2f05f5f0db8a9bb94a1ad2c92bc7a83 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 18 Oct 2024 09:41:30 +0530 Subject: [PATCH 04/41] lru cache with max size, remove locks and basic test Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/v3/local_rate_limit.proto | 4 + .../local_ratelimit/local_ratelimit_impl.cc | 107 ++++++++++-------- .../local_ratelimit/local_ratelimit_impl.h | 36 +++++- .../http/local_ratelimit/local_ratelimit.cc | 10 +- .../http/local_ratelimit/local_ratelimit.h | 6 +- .../listener/local_ratelimit/config.cc | 3 +- .../local_ratelimit/local_ratelimit.cc | 5 +- .../local_ratelimit/local_ratelimit.h | 3 +- .../filters/network/local_ratelimit/config.cc | 3 +- .../local_ratelimit/local_ratelimit.cc | 6 +- .../network/local_ratelimit/local_ratelimit.h | 4 +- .../local_ratelimit_integration_test.cc | 79 +++++++++++++ 12 files changed, 196 insertions(+), 70 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index 82e38ed91d5a6..119d2088a6f2c 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -167,4 +167,8 @@ message LocalRateLimit { // 3. :ref:`disable_key `. // 4. :ref:`override limit `. repeated config.route.v3.RateLimit rate_limits = 17; + + // Specifies limit for the lru cache which is used to store dynamic descriptors. + // Default is 20. + uint32 dynamic_descripters_lru_cache_limit = 18; } diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index c67dc11b9c795..f61a5da1dcb2e 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -2,6 +2,7 @@ #include #include +#include #include "envoy/runtime/runtime.h" @@ -175,18 +176,20 @@ bool AtomicTokenBucket::isIdle() { LocalRateLimiterImpl::LocalRateLimiterImpl( const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, - const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher, + const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors, - bool always_consume_default_token_bucket, ShareProviderSharedPtr shared_provider) + bool always_consume_default_token_bucket, ShareProviderSharedPtr shared_provider, + uint32_t lru_size) : fill_timer_(fill_interval > std::chrono::milliseconds(0) ? dispatcher.createTimer([this] { onFillTimer(); }) : nullptr), - dynamic_desc_idle_timer_(dispatcher.createTimer([this] { onDynDescIdleTimer(); })), - time_source_(dispatcher.timeSource()), share_provider_(std::move(shared_provider)), + time_source_(dispatcher.timeSource()), tls_(tls.allocateSlot()), + share_provider_(std::move(shared_provider)), always_consume_default_token_bucket_(always_consume_default_token_bucket), no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")) { + "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")), + dispatcher_(dispatcher), lru_size_(lru_size == 0 ? 20 : lru_size) { if (fill_timer_ && fill_interval < std::chrono::milliseconds(50)) { throw EnvoyException("local rate limit token bucket fill timer must be >= 50ms"); } @@ -203,10 +206,13 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( !no_timer_based_rate_limit_token_bucket_) { fill_timer_->enableTimer(default_token_bucket_->fillInterval()); } + RateLimit::LocalDescriptor::Map blank_descriptors; - if (dynamic_desc_idle_timer_) { - dynamic_desc_idle_timer_->enableTimer(std::chrono::milliseconds(5000)); - } + ThreadLocalDynamicDescriptorsCacheSharedPtr empty( + new ThreadLocalDynamicDescriptorsCache(blank_descriptors)); + + tls_->set( + [empty](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return empty; }); for (const auto& descriptor : descriptors) { RateLimit::LocalDescriptor new_descriptor; @@ -252,24 +258,6 @@ LocalRateLimiterImpl::~LocalRateLimiterImpl() { if (fill_timer_ != nullptr) { fill_timer_->disableTimer(); } - if (dynamic_desc_idle_timer_ != nullptr) { - dynamic_desc_idle_timer_->disableTimer(); - } -} - -void LocalRateLimiterImpl::onDynDescIdleTimer() { - absl::WriterMutexLock lock(&dyn_desc_lock_); - for (auto it = dynamic_descriptors_.begin(), end = dynamic_descriptors_.end(); it != end;) { - // `erase()` will invalidate `it`, so advance `it` first. - auto copy_it = it++; - if (copy_it->second->isIdle()) { - dynamic_descriptors_.erase(copy_it); - ENVOY_LOG(trace, "cleaned up idle descriptor: {}._ current size: {}", - copy_it->first.toString(), dynamic_descriptors_.size()); - } - } - - dynamic_desc_idle_timer_->enableTimer(std::chrono::milliseconds(5000)); } void LocalRateLimiterImpl::onFillTimer() { @@ -286,11 +274,8 @@ void LocalRateLimiterImpl::onFillTimer() { for (const auto& descriptor : descriptors_) { descriptor.second->onFillTimer(refill_counter_, share_factor); } - { - absl::ReaderMutexLock lock(&dyn_desc_lock_); - for (const auto& descriptor : dynamic_descriptors_) { - descriptor.second->onFillTimer(refill_counter_, share_factor); - } + for (const auto& descriptor : dynamic_descriptors_) { + descriptor.second->onFillTimer(refill_counter_, share_factor); } fill_timer_->enableTimer(default_token_bucket_->fillInterval()); @@ -312,11 +297,13 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( // If the request descriptor is not found in the user descriptors, it could be a wildcard case // where value is left blank in the user configured descriptor entry. In this case, we need to // check if it matches any of the dynamic descriptors. - { - absl::ReaderMutexLock lock(&dyn_desc_lock_); - // find request descriptor in the existing dynamic descriptors. If it exists, add token - // bucket to matched_descriptors. - auto dynamic_iter = dynamic_descriptors_.find(request_descriptor); + // find request descriptor in the existing dynamic descriptors. If it exists, add token + // bucket to matched_descriptors. + auto dynamic_descriptors = threadLocal().cache(); + if (dynamic_descriptors.empty()) { + ENVOY_LOG(debug, "dynamic_descriptors is empty"); + } else { + auto dynamic_iter = dynamic_descriptors.find(request_descriptor); if (dynamic_iter != dynamic_descriptors_.end()) { matched_descriptors.push_back(dynamic_iter->second.get()); continue; @@ -358,17 +345,8 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( std::chrono::duration(user_bucket->fillInterval()).count()), user_bucket->fillInterval(), user_bucket->multiplier(), *this); } - - { - absl::WriterMutexLock lock(&dyn_desc_lock_); - auto result = dynamic_descriptors_.emplace(std::move(new_descriptor), - std::move(per_descriptor_token_bucket)); - if (!result.second) { - throw EnvoyException(absl::StrCat("duplicate descriptor in the local rate descriptor: ", - result.first->first.toString())); - } - matched_descriptors.push_back(result.first->second.get()); - } + matched_descriptors.push_back(per_descriptor_token_bucket.get()); + notifyMainThread(move(per_descriptor_token_bucket), std::move(new_descriptor)); } } } @@ -392,7 +370,7 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( // token bucket. return {false, makeOptRefFromPtr(descriptor)}; } else { - ENVOY_LOG(debug, + ENVOY_LOG(trace, "request allowed by descriptor with fill rate: {}, maxToken: {}, remainingToken {}", descriptor->fillRate(), descriptor->maxTokens(), descriptor->remainingTokens()); } @@ -417,6 +395,39 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( return {true, makeOptRefFromPtr(matched_descriptors[0])}; } +void LocalRateLimiterImpl::notifyMainThread(RateLimitTokenBucketSharedPtr token_bucket, + RateLimit::LocalDescriptor new_descriptor) { + dispatcher_.post([this, token_bucket, new_descriptor]() -> void { + ENVOY_LOG(trace, "notifyMainThread: creating dynamic descriptor: {}", + new_descriptor.toString()); + // add this bucket to cache. + // After updating cache, make a copy of the cache and update the tls with the new cache. + auto result = dynamic_descriptors_.emplace(new_descriptor, std::move(token_bucket)); + if (!result.second) { + ENVOY_LOG(trace, "notifyMainThread: descriptor, {}, already in the cache", + result.first->first.toString()); + return; + } + lru_list_.emplace_front(new_descriptor); + if (lru_list_.size() > lru_size_) { + auto lru_entry = lru_list_.back(); + auto erased = dynamic_descriptors_.erase(lru_entry); + ENVOY_LOG(trace, "max size: {}, current size: {}, lru entry(descriptor): {}, erased: {}", + lru_size_, lru_list_.size(), lru_entry.toString(), erased); + lru_list_.pop_back(); + } + + // copy the cache and update the tls. + auto cache = dynamic_descriptors_; + + tls_->set([cache](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { + auto copy = + std::make_shared>(cache); + return std::make_shared(*copy); + }); + }); +} + // Compare the request descriptor entries with the user descriptor entries. If all non-empty user // descriptor values match the request descriptor values, return true and fill the new descriptor bool LocalRateLimiterImpl::compareDescriptorEntries( diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index b4f8537fac7c0..bb6860e7ba286 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -80,6 +80,22 @@ using RateLimitTokenBucketSharedPtr = std::shared_ptr; class LocalRateLimiterImpl; +class ThreadLocalDynamicDescriptorsCache : public ThreadLocal::ThreadLocalObject { +public: + ThreadLocalDynamicDescriptorsCache( + RateLimit::LocalDescriptor::Map cache) + : dynamic_descriptors_(cache) {} + + const RateLimit::LocalDescriptor::Map cache() const { + return dynamic_descriptors_; + }; + +private: + RateLimit::LocalDescriptor::Map dynamic_descriptors_; +}; +using ThreadLocalDynamicDescriptorsCacheSharedPtr = + std::shared_ptr; + // Token bucket that implements based on the periodic timer. class TimerTokenBucket : public RateLimitTokenBucket { public: @@ -146,10 +162,11 @@ class LocalRateLimiterImpl : public Logger::Loggable& descriptors, bool always_consume_default_token_bucket = true, - ShareProviderSharedPtr shared_provider = nullptr); + ShareProviderSharedPtr shared_provider = nullptr, const uint32_t lru_size = 20); ~LocalRateLimiterImpl(); Result requestAllowed(absl::Span request_descriptors); @@ -159,18 +176,20 @@ class LocalRateLimiterImpl : public Logger::Loggable& request_entries, const std::vector& user_entries, std::vector& new_descriptor_entries); - void onDynDescIdleTimer(); + void notifyMainThread(RateLimitTokenBucketSharedPtr token_bucket, + RateLimit::LocalDescriptor new_descriptor); + const ThreadLocalDynamicDescriptorsCache& threadLocal() const { + return tls_->getTyped(); + } RateLimitTokenBucketSharedPtr default_token_bucket_; const Event::TimerPtr fill_timer_; - const Event::TimerPtr dynamic_desc_idle_timer_; TimeSource& time_source_; RateLimit::LocalDescriptor::Map descriptors_; - mutable absl::Mutex dyn_desc_lock_; - RateLimit::LocalDescriptor::Map - dynamic_descriptors_ ABSL_GUARDED_BY(dyn_desc_lock_); + RateLimit::LocalDescriptor::Map dynamic_descriptors_; + ThreadLocal::SlotPtr tls_; // Refill counter is incremented per each refill timer hit. uint64_t refill_counter_{0}; @@ -180,6 +199,11 @@ class LocalRateLimiterImpl : public Logger::Loggable; + LruList lru_list_; + uint32_t lru_size_; friend class LocalRateLimiterImplTest; friend class TimerTokenBucket; diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 5442046562bf1..3ac0b3260c26b 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -61,7 +61,8 @@ FilterConfig::FilterConfig( rate_limited_grpc_status_( config.rate_limited_as_resource_exhausted() ? absl::make_optional(Grpc::Status::WellKnownGrpcStatus::ResourceExhausted) - : absl::nullopt) { + : absl::nullopt), + tls_(context.threadLocal()) { // Note: no token bucket is fine for the global config, which would be the case for enabling // the filter globally but disabled and then applying limits at the virtual host or // route level. At the virtual or route level, it makes no sense to have an no token @@ -108,8 +109,9 @@ FilterConfig::FilterConfig( } rate_limiter_ = std::make_unique( - fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, descriptors_, - always_consume_default_token_bucket_, std::move(share_provider)); + fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, context.threadLocal(), + descriptors_, always_consume_default_token_bucket_, std::move(share_provider), + config.dynamic_descripters_lru_cache_limit()); } Filters::Common::LocalRateLimit::LocalRateLimiterImpl::Result FilterConfig::requestAllowed( @@ -230,7 +232,7 @@ Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionR if (typed_state == nullptr) { auto limiter = std::make_shared( used_config_->fillInterval(), used_config_->maxTokens(), used_config_->tokensPerFill(), - decoder_callbacks_->dispatcher(), used_config_->descriptors(), + decoder_callbacks_->dispatcher(), config_->tls(), used_config_->descriptors(), used_config_->consumeDefaultTokenBucket()); decoder_callbacks_->streamInfo().filterState()->setData( diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index 507b5a0a72332..a43986cfbb509 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -52,11 +52,11 @@ class PerConnectionRateLimiter : public StreamInfo::FilterState::Object { public: PerConnectionRateLimiter( const std::chrono::milliseconds& fill_interval, uint32_t max_tokens, uint32_t tokens_per_fill, - Envoy::Event::Dispatcher& dispatcher, + Envoy::Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptor, bool always_consume_default_token_bucket) - : rate_limiter_(fill_interval, max_tokens, tokens_per_fill, dispatcher, descriptor, + : rate_limiter_(fill_interval, max_tokens, tokens_per_fill, dispatcher, tls, descriptor, always_consume_default_token_bucket) {} static const std::string& key(); Filters::Common::LocalRateLimit::LocalRateLimiterImpl& value() { return rate_limiter_; } @@ -88,6 +88,7 @@ class FilterConfig : public Router::RouteSpecificFilterConfig, requestAllowed(absl::Span request_descriptors) const; bool enabled() const; bool enforced() const; + ThreadLocal::SlotAllocator& tls() { return tls_; } LocalRateLimitStats& stats() const { return stats_; } const Router::HeaderParser& responseHeadersParser() const { return *response_headers_parser_; } const Router::HeaderParser& requestHeadersParser() const { return *request_headers_parser_; } @@ -162,6 +163,7 @@ class FilterConfig : public Router::RouteSpecificFilterConfig, const envoy::extensions::common::ratelimit::v3::VhRateLimitsOptions vh_rate_limits_; const absl::optional rate_limited_grpc_status_; std::unique_ptr rate_limit_config_; + ThreadLocal::SlotAllocator& tls_; }; using FilterConfigSharedPtr = std::shared_ptr; diff --git a/source/extensions/filters/listener/local_ratelimit/config.cc b/source/extensions/filters/listener/local_ratelimit/config.cc index 5a2dbde8252ce..5b729944bedd6 100644 --- a/source/extensions/filters/listener/local_ratelimit/config.cc +++ b/source/extensions/filters/listener/local_ratelimit/config.cc @@ -29,7 +29,8 @@ class LocalRateLimitConfigFactory : public Server::Configuration::NamedListenerF message, context.messageValidationVisitor()); FilterConfigSharedPtr config = std::make_shared( - proto_config, context.serverFactoryContext().mainThreadDispatcher(), context.scope(), + proto_config, context.serverFactoryContext().mainThreadDispatcher(), + context.serverFactoryContext().threadLocal(), context.scope(), context.serverFactoryContext().runtime()); return [listener_filter_matcher, config](Network::ListenerFilterManager& filter_manager) -> void { diff --git a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc index 85f810ad409ef..f57c9e103464a 100644 --- a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc @@ -9,7 +9,8 @@ namespace LocalRateLimit { FilterConfig::FilterConfig( const envoy::extensions::filters::listener::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime) + Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, + Runtime::Loader& runtime) : enabled_(proto_config.runtime_enabled(), runtime), stats_(generateStats(proto_config.stat_prefix(), scope)), rate_limiter_( @@ -17,7 +18,7 @@ FilterConfig::FilterConfig( PROTOBUF_GET_MS_REQUIRED(proto_config.token_bucket(), fill_interval)), proto_config.token_bucket().max_tokens(), PROTOBUF_GET_WRAPPED_OR_DEFAULT(proto_config.token_bucket(), tokens_per_fill, 1), - dispatcher, + dispatcher, tls, Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>()) {} diff --git a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h index ae9e0bf19154f..ab95802faa11b 100644 --- a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h @@ -36,7 +36,8 @@ class FilterConfig : Logger::Loggable { public: FilterConfig( const envoy::extensions::filters::listener::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime); + Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, + Runtime::Loader& runtime); ~FilterConfig() = default; diff --git a/source/extensions/filters/network/local_ratelimit/config.cc b/source/extensions/filters/network/local_ratelimit/config.cc index f2b935479e9ba..1bf44494d245c 100644 --- a/source/extensions/filters/network/local_ratelimit/config.cc +++ b/source/extensions/filters/network/local_ratelimit/config.cc @@ -14,7 +14,8 @@ Network::FilterFactoryCb LocalRateLimitConfigFactory::createFilterFactoryFromPro const envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit& proto_config, Server::Configuration::FactoryContext& context) { ConfigSharedPtr filter_config(std::make_shared( - proto_config, context.serverFactoryContext().mainThreadDispatcher(), context.scope(), + proto_config, context.serverFactoryContext().mainThreadDispatcher(), + context.serverFactoryContext().threadLocal(), context.scope(), context.serverFactoryContext().runtime(), context.serverFactoryContext().singletonManager())); return [filter_config](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(std::make_shared(filter_config)); diff --git a/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc index 09594692dd13a..3bf4356624537 100644 --- a/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc @@ -61,8 +61,8 @@ void SharedRateLimitSingleton::removeIfUnused(const Key* key) { Config::Config( const envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime, - Singleton::Manager& singleton_manager) + Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, + Runtime::Loader& runtime, Singleton::Manager& singleton_manager) : enabled_(proto_config.runtime_enabled(), runtime), stats_(generateStats(proto_config.stat_prefix(), scope)), shared_bucket_registry_(singleton_manager.getTyped( @@ -75,7 +75,7 @@ Config::Config( PROTOBUF_GET_MS_REQUIRED(proto_config.token_bucket(), fill_interval)), proto_config.token_bucket().max_tokens(), PROTOBUF_GET_WRAPPED_OR_DEFAULT(proto_config.token_bucket(), tokens_per_fill, 1), - dispatcher, + dispatcher, tls, Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>()); }); diff --git a/source/extensions/filters/network/local_ratelimit/local_ratelimit.h b/source/extensions/filters/network/local_ratelimit/local_ratelimit.h index 2ed721e62e3da..1ce182a1515c9 100644 --- a/source/extensions/filters/network/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/network/local_ratelimit/local_ratelimit.h @@ -70,8 +70,8 @@ class Config : Logger::Loggable { public: Config( const envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime, - Singleton::Manager& singleton_manager); + Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, + Runtime::Loader& runtime, Singleton::Manager& singleton_manager); ~Config(); diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index 2171ea59ebf66..575e2e4ed962e 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -168,6 +168,46 @@ name: envoy.filters.http.local_ratelimit local_rate_limit_per_downstream_connection: {} )EOF"; + static constexpr absl::string_view filter_config_with_blank_value_descriptor_ = + R"EOF( +name: envoy.filters.http.local_ratelimit +typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.local_ratelimit.v3.LocalRateLimit + stat_prefix: http_local_rate_limiter + token_bucket: + max_tokens: 2 + tokens_per_fill: 1 + fill_interval: 1000s + filter_enabled: + runtime_key: local_rate_limit_enabled + default_value: + numerator: 100 + denominator: HUNDRED + filter_enforced: + runtime_key: local_rate_limit_enforced + default_value: + numerator: 100 + denominator: HUNDRED + response_headers_to_add: + - append_action: OVERWRITE_IF_EXISTS_OR_ADD + header: + key: x-local-rate-limit + value: 'true' + descriptors: + - entries: + - key: client_cluster + token_bucket: + max_tokens: 1 + tokens_per_fill: 1 + fill_interval: 1000s + rate_limits: + - actions: # any actions in here + - request_headers: + header_name: x-envoy-downstream-service-cluster + descriptor_key: client_cluster + local_rate_limit_per_downstream_connection: {} +)EOF"; + const std::string filter_config_with_local_cluster_rate_limit_ = R"EOF( name: envoy.filters.http.local_ratelimit @@ -301,6 +341,45 @@ INSTANTIATE_TEST_SUITE_P( testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParamsWithoutHTTP3()), HttpProtocolIntegrationTest::protocolTestParamsToString); +TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { + initializeFilter(fmt::format(filter_config_with_blank_value_descriptor_, "false")); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-envoy-downstream-service-cluster", "foo"}}, + 0); + + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, 1); + + ASSERT_TRUE(response->waitForEndStream()); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(0, response->body().size()); + + cleanupUpstreamAndDownstream(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + response = codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-envoy-downstream-service-cluster", "foo"}}, + 0); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("429", response->headers().getStatusValue()); + EXPECT_EQ(18, response->body().size()); +} + TEST_P(LocalRateLimitFilterIntegrationTest, DenyRequestPerProcess) { initializeFilter(fmt::format(filter_config_, "false")); From e1dc1acb251085899b99f4de442457ef763bb215 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 18 Oct 2024 09:50:59 +0530 Subject: [PATCH 05/41] some cleanup Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/common/local_ratelimit/local_ratelimit_impl.cc | 5 ----- .../filters/common/local_ratelimit/local_ratelimit_impl.h | 3 --- 2 files changed, 8 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index f61a5da1dcb2e..f358815b12358 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -169,11 +169,6 @@ bool AtomicTokenBucket::consume(double factor) { return token_bucket_.consume(cb) != 0.0; } -bool AtomicTokenBucket::isIdle() { - return token_bucket_.secsSinceLastConsume() > - 3 * std::chrono::duration(fill_interval_).count(); -} - LocalRateLimiterImpl::LocalRateLimiterImpl( const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index bb6860e7ba286..928109c76ff3a 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -70,7 +70,6 @@ class TokenBucketContext { class RateLimitTokenBucket : public TokenBucketContext { public: virtual bool consume(double factor = 1.0) PURE; - virtual bool isIdle() PURE; virtual void onFillTimer(uint64_t refill_counter, double factor = 1.0) PURE; virtual std::chrono::milliseconds fillInterval() const PURE; virtual double fillRate() const PURE; @@ -105,7 +104,6 @@ class TimerTokenBucket : public RateLimitTokenBucket { // RateLimitTokenBucket bool consume(double factor) override; - bool isIdle() override { return false; /* TODO: implement*/ } void onFillTimer(uint64_t refill_counter, double factor) override; std::chrono::milliseconds fillInterval() const override { return fill_interval_; } double fillRate() const override { return fill_rate_; } @@ -137,7 +135,6 @@ class AtomicTokenBucket : public RateLimitTokenBucket, // RateLimitTokenBucket bool consume(double factor) override; - bool isIdle() override; void onFillTimer(uint64_t, double) override {} std::chrono::milliseconds fillInterval() const override { return fill_interval_; } double fillRate() const override { return token_bucket_.fillRate(); } From f1e47653f4429c3a4aafc993fc5e2954c9fa13a3 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 18 Oct 2024 14:59:15 +0530 Subject: [PATCH 06/41] updates Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit_integration_test.cc | 80 ++++++++++++------- .../filters/network/local_ratelimit/BUILD | 2 + .../local_ratelimit_fuzz_test.cc | 6 +- .../local_ratelimit/local_ratelimit_test.cc | 10 ++- 4 files changed, 64 insertions(+), 34 deletions(-) diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index 575e2e4ed962e..7ce0030dfc820 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -140,6 +140,40 @@ class LocalRateLimitFilterIntegrationTest : public Event::TestUsingSimulatedTime } } + IntegrationStreamDecoderPtr makeRequest(const std::string& cluster, + const std::string& path = "/test/long/url") { + return codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", path}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-envoy-downstream-service-cluster", cluster}}, + 0); + } + + void verifyResponse(IntegrationStreamDecoderPtr response, const std::string& expected_status, + size_t expected_body_size) { + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ(expected_status, response->headers().getStatusValue()); + EXPECT_EQ(expected_body_size, response->body().size()); + } + + void sendAndVerifyRequest(const std::string& cluster, const std::string& expected_status, + size_t expected_body_size) { + auto response = makeRequest(cluster); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, 1); + verifyResponse(move(response), expected_status, expected_body_size); + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + } + void sendRateLimitedRequest(const std::string& cluster) { + auto response = makeRequest(cluster); + verifyResponse(move(response), "429", + 18); // 18 is the expected body size for rate-limited responses. + } + static constexpr absl::string_view filter_config_ = R"EOF( name: envoy.filters.http.local_ratelimit @@ -343,41 +377,31 @@ INSTANTIATE_TEST_SUITE_P( TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { initializeFilter(fmt::format(filter_config_with_blank_value_descriptor_, "false")); + // filter is adding dynamic descriptors based on the request header + // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval + // of 1000s which means only one request is allowed per 1000s for each unique value of + // 'x-envoy-downstream-service-cluster' header. codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeRequestWithBody( - Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}, - {"x-envoy-downstream-service-cluster", "foo"}}, - 0); - - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(default_response_headers_, 1); - - ASSERT_TRUE(response->waitForEndStream()); + sendAndVerifyRequest("foo", "200", 0); + cleanupUpstreamAndDownstream(); - EXPECT_TRUE(upstream_request_->complete()); - EXPECT_EQ(0U, upstream_request_->bodyLength()); - EXPECT_TRUE(response->complete()); - EXPECT_EQ("200", response->headers().getStatusValue()); - EXPECT_EQ(0, response->body().size()); + // 1 token is exhausted for 'foo' cluster, so the next request with the same cluster should be + // rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("foo"); + cleanupUpstreamAndDownstream(); + // The next request with a different cluster, 'bar', should be allowed. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("bar", "200", 0); cleanupUpstreamAndDownstream(); + // 1 token is exhausted for 'bar' cluster as well, so the next request with the same cluster + // should be rate limited. codec_client_ = makeHttpConnection(lookupPort("http")); - response = codec_client_->makeRequestWithBody( - Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}, - {"x-envoy-downstream-service-cluster", "foo"}}, - 0); - ASSERT_TRUE(response->waitForEndStream()); - EXPECT_TRUE(response->complete()); - EXPECT_EQ("429", response->headers().getStatusValue()); - EXPECT_EQ(18, response->body().size()); + sendRateLimitedRequest("bar"); + cleanupUpstreamAndDownstream(); } TEST_P(LocalRateLimitFilterIntegrationTest, DenyRequestPerProcess) { diff --git a/test/extensions/filters/network/local_ratelimit/BUILD b/test/extensions/filters/network/local_ratelimit/BUILD index 25d160adae3a4..2461c8e7e0802 100644 --- a/test/extensions/filters/network/local_ratelimit/BUILD +++ b/test/extensions/filters/network/local_ratelimit/BUILD @@ -24,6 +24,7 @@ envoy_extension_cc_test( "//test/mocks/event:event_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/mocks/thread_local:thread_local_mocks", "@envoy_api//envoy/extensions/filters/network/local_ratelimit/v3:pkg_cc_proto", ], ) @@ -66,6 +67,7 @@ envoy_cc_fuzz_test( "//test/mocks/event:event_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/mocks/thread_local:thread_local_mocks", "@envoy_api//envoy/extensions/filters/network/local_ratelimit/v3:pkg_cc_proto", ], ) diff --git a/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc b/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc index 45bcd52280972..cf7fe71a476f1 100644 --- a/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc +++ b/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc @@ -11,6 +11,7 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/thread_local/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -55,6 +56,7 @@ DEFINE_PROTO_FUZZER( return; } static NiceMock dispatcher; + NiceMock tls; // TODO(zhxie): The GlobalTimeSystem in MockDispatcher will initialize itself into a // TestRealTimeSystem by default which is incompatible with the SimulatedTimeSystem in // MockReadFilterCallbacks. We will not need to change the time system after the switching of @@ -68,8 +70,8 @@ DEFINE_PROTO_FUZZER( input.config(); ConfigSharedPtr config = nullptr; try { - config = std::make_shared(proto_config, dispatcher, *stats_store.rootScope(), runtime, - singleton_manager); + config = std::make_shared(proto_config, dispatcher, tls, *stats_store.rootScope(), + runtime, singleton_manager); } catch (EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException in config's constructor: {}", e.what()); return; diff --git a/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc index 94af0da7d1348..bf5232da8a991 100644 --- a/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc @@ -8,6 +8,7 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" +#include "test/mocks/thread_local/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -28,12 +29,13 @@ class LocalRateLimitTestBase : public testing::Test, public Event::TestUsingSimu uint64_t initialize(const std::string& filter_yaml) { envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYamlAndValidate(filter_yaml, proto_config); - config_ = std::make_shared(proto_config, dispatcher_, *stats_store_.rootScope(), + config_ = std::make_shared(proto_config, dispatcher_, tls_, *stats_store_.rootScope(), runtime_, singleton_manager_); return proto_config.token_bucket().max_tokens(); } NiceMock dispatcher_; + NiceMock tls_; Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; Singleton::ManagerImpl singleton_manager_; @@ -137,7 +139,7 @@ class LocalRateLimitSharedTokenBucketTest : public LocalRateLimitTestBase { envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYamlAndValidate(filter_yaml2, proto_config); const uint64_t config2_tokens = proto_config.token_bucket().max_tokens(); - config2_ = std::make_shared(proto_config, dispatcher_, *stats_store_.rootScope(), + config2_ = std::make_shared(proto_config, dispatcher_, tls_, *stats_store_.rootScope(), runtime_, singleton_manager_); // This test just uses the initial tokens without ever refilling. @@ -229,8 +231,8 @@ share_key: key_{} std::string yaml = fmt::format(yaml_template, i); envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYamlAndValidate(yaml, proto_config); - configs.push_back(std::make_unique(proto_config, dispatcher_, *stats_store_.rootScope(), - runtime_, singleton_manager_)); + configs.push_back(std::make_unique( + proto_config, dispatcher_, tls_, *stats_store_.rootScope(), runtime_, singleton_manager_)); } configs.clear(); From 15f655e7d828a3fed24581af230349c55747afab Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 18 Oct 2024 15:11:46 +0530 Subject: [PATCH 07/41] updates Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/http/local_ratelimit/v3/local_rate_limit.proto | 2 +- source/common/common/token_bucket_impl.h | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index 119d2088a6f2c..a370cf45330d3 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -23,7 +23,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Local Rate limit :ref:`configuration overview `. // [#extension: envoy.filters.http.local_ratelimit] -// [#next-free-field: 18] +// [#next-free-field: 19] message LocalRateLimit { // The human readable prefix to use when emitting stats. string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; diff --git a/source/common/common/token_bucket_impl.h b/source/common/common/token_bucket_impl.h index c56c24cd1c9a2..673f0a74e2b37 100644 --- a/source/common/common/token_bucket_impl.h +++ b/source/common/common/token_bucket_impl.h @@ -78,12 +78,6 @@ class AtomicTokenBucketImpl { return consumed; } - double secsSinceLastConsume() { - double time_old = time_in_seconds_.load(std::memory_order_relaxed); - const double time_now = timeNowInSeconds(); - return time_now - time_old; - } - /** * Consumes one tokens from the bucket. * @return true if the token is consumed, false otherwise. From 9d251f8a2588e0d3e426756a9057b7cf6e1b4fa7 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 18 Oct 2024 15:17:10 +0530 Subject: [PATCH 08/41] updates Signed-off-by: Vikas Choudhary (vikasc) --- test/extensions/filters/listener/local_ratelimit/BUILD | 1 + .../listener/local_ratelimit/local_ratelimit_test.cc | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/listener/local_ratelimit/BUILD b/test/extensions/filters/listener/local_ratelimit/BUILD index 184d5402a6f79..16d644b851e97 100644 --- a/test/extensions/filters/listener/local_ratelimit/BUILD +++ b/test/extensions/filters/listener/local_ratelimit/BUILD @@ -24,6 +24,7 @@ envoy_extension_cc_test( "//source/extensions/filters/listener/local_ratelimit:local_ratelimit_lib", "//test/mocks/network:network_mocks", "//test/mocks/stats:stats_mocks", + "//test/mocks/thread_local:thread_local_mocks", ], ) diff --git a/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc index 251c3c575dd8d..dd9942838401c 100644 --- a/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc @@ -7,6 +7,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/stats/mocks.h" +#include "test/mocks/thread_local/mocks.h" #include "absl/strings/str_format.h" #include "gmock/gmock.h" @@ -59,8 +60,8 @@ class LocalRateLimitTest : public testing::Test, public Event::TestUsingSimulate uint64_t initialize(const std::string& filter_yaml) { envoy::extensions::filters::listener::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYaml(filter_yaml, proto_config); - config_ = std::make_shared(proto_config, dispatcher_, *stats_store_.rootScope(), - runtime_); + config_ = std::make_shared(proto_config, dispatcher_, tls_, + *stats_store_.rootScope(), runtime_); return proto_config.token_bucket().max_tokens(); } @@ -68,6 +69,7 @@ class LocalRateLimitTest : public testing::Test, public Event::TestUsingSimulate Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; FilterConfigSharedPtr config_; + NiceMock tls_; }; // Basic no rate limit case. From ed02fa602c6508621ffdb46090e61052676fc776 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 18 Oct 2024 15:50:52 +0530 Subject: [PATCH 09/41] updates Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/local_ratelimit_test.cc | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 9becf109faa5d..064fcbf4d5755 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -127,17 +127,17 @@ class LocalRateLimiterImplTest : public testing::Test { initializeTimer(); - rate_limiter_ = - std::make_shared(fill_interval, max_tokens, tokens_per_fill, - dispatcher_, descriptors_, true, share_provider); + rate_limiter_ = std::make_shared(fill_interval, max_tokens, + tokens_per_fill, dispatcher_, tls_, + descriptors_, true, share_provider); } void initializeWithAtomicTokenBucket(const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, const uint32_t tokens_per_fill, ShareProviderSharedPtr share_provider = nullptr) { - rate_limiter_ = - std::make_shared(fill_interval, max_tokens, tokens_per_fill, - dispatcher_, descriptors_, true, share_provider); + rate_limiter_ = std::make_shared(fill_interval, max_tokens, + tokens_per_fill, dispatcher_, tls_, + descriptors_, true, share_provider); } Thread::ThreadSynchronizer& synchronizer() { return rate_limiter_->synchronizer_; } @@ -147,6 +147,7 @@ class LocalRateLimiterImplTest : public testing::Test { std::vector route_descriptors_; NiceMock dispatcher_; + NiceMock tls_; Event::MockTimer* fill_timer_{}; std::shared_ptr rate_limiter_; }; @@ -154,7 +155,7 @@ class LocalRateLimiterImplTest : public testing::Test { // Make sure we fail with a fill rate this is too fast. TEST_F(LocalRateLimiterImplTest, TooFastFillRate) { EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(49), 100, 1, dispatcher_, descriptors_), + LocalRateLimiterImpl(std::chrono::milliseconds(49), 100, 1, dispatcher_, tls_, descriptors_), EnvoyException, "local rate limit token bucket fill timer must be >= 50ms"); } @@ -371,14 +372,14 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { initializeTimer(); rate_limiter_ = std::make_shared( - fill_interval, max_tokens, tokens_per_fill, dispatcher_, descriptors_); + fill_interval, max_tokens, tokens_per_fill, dispatcher_, tls_, descriptors_); } void initializeWithAtomicTokenBucketDescriptor(const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, const uint32_t tokens_per_fill) { rate_limiter_ = std::make_shared( - fill_interval, max_tokens, tokens_per_fill, dispatcher_, descriptors_); + fill_interval, max_tokens, tokens_per_fill, dispatcher_, tls_, descriptors_); } static constexpr absl::string_view single_descriptor_config_yaml = R"( @@ -414,7 +415,7 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitDivisibleByTokenFi *descriptors_.Add()); EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_), + LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, tls_, descriptors_), EnvoyException, "local rate descriptor limit is not a multiple of token bucket fill timer"); } @@ -425,14 +426,14 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DuplicateDescriptor) { *descriptors_.Add()); EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(50), 1, 1, dispatcher_, descriptors_), + LocalRateLimiterImpl(std::chrono::milliseconds(50), 1, 1, dispatcher_, tls_, descriptors_), EnvoyException, "duplicate descriptor in the local rate descriptor: foo2=bar2"); } // Verify no exception for per route config without descriptors. TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitNoExceptionWithoutDescriptor) { - VERBOSE_EXPECT_NO_THROW( - LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_)); + VERBOSE_EXPECT_NO_THROW(LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, + tls_, descriptors_)); } // Verify various token bucket CAS edge cases for descriptors. From d305bd595bcb451cb9252ddbd3af201de1d9a480 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sat, 19 Oct 2024 05:44:05 +0530 Subject: [PATCH 10/41] cleanup Signed-off-by: Vikas Choudhary (vikasc) --- .../common/ratelimit/v3/ratelimit.proto | 2 ++ .../local_ratelimit/local_ratelimit_impl.cc | 24 ++++++++++++----- .../local_ratelimit/local_ratelimit_impl.h | 4 +-- .../http/local_ratelimit/local_ratelimit.cc | 15 ++++++----- .../http/local_ratelimit/local_ratelimit.h | 8 +++--- .../listener/local_ratelimit/config.cc | 3 +-- .../local_ratelimit/local_ratelimit.cc | 5 ++-- .../local_ratelimit/local_ratelimit.h | 3 +-- .../filters/network/local_ratelimit/config.cc | 3 +-- .../local_ratelimit/local_ratelimit.cc | 6 ++--- .../network/local_ratelimit/local_ratelimit.h | 4 +-- .../local_ratelimit/local_ratelimit_test.cc | 27 +++++++++---------- .../filters/listener/local_ratelimit/BUILD | 1 - .../local_ratelimit/local_ratelimit_test.cc | 6 ++--- .../filters/network/local_ratelimit/BUILD | 2 -- .../local_ratelimit_fuzz_test.cc | 6 ++--- .../local_ratelimit/local_ratelimit_test.cc | 10 +++---- 17 files changed, 65 insertions(+), 64 deletions(-) diff --git a/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto b/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto index 303e00da13888..8c15e3986d5a3 100644 --- a/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto +++ b/api/envoy/extensions/common/ratelimit/v3/ratelimit.proto @@ -102,6 +102,8 @@ message RateLimitDescriptor { string key = 1 [(validate.rules).string = {min_len: 1}]; // Descriptor value. Blank value is treated as wildcard to create dynamic token buckets for each unique value. + // Blank Values as wild card is currently supported only with envoy server instance level HTTP local rate limiting + // and will not work if HTTP local rate limiting is enabled per connection level. string value = 2 [(validate.rules).string = {min_len: 0}]; } diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index f358815b12358..c27e62162d218 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -171,15 +171,15 @@ bool AtomicTokenBucket::consume(double factor) { LocalRateLimiterImpl::LocalRateLimiterImpl( const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, - const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, + const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher, const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors, bool always_consume_default_token_bucket, ShareProviderSharedPtr shared_provider, - uint32_t lru_size) + uint32_t lru_size, ThreadLocal::SlotAllocator* tls, bool per_connection) : fill_timer_(fill_interval > std::chrono::milliseconds(0) ? dispatcher.createTimer([this] { onFillTimer(); }) : nullptr), - time_source_(dispatcher.timeSource()), tls_(tls.allocateSlot()), + time_source_(dispatcher.timeSource()), tls_(tls != nullptr ? tls->allocateSlot() : nullptr), share_provider_(std::move(shared_provider)), always_consume_default_token_bucket_(always_consume_default_token_bucket), no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( @@ -205,14 +205,23 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( ThreadLocalDynamicDescriptorsCacheSharedPtr empty( new ThreadLocalDynamicDescriptorsCache(blank_descriptors)); - - tls_->set( - [empty](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return empty; }); + // in per connection rate limit mode, the dynamic descriptors are stored in the per connection + // which is per worker thread. So we don't need to store the dynamic descriptors in the main + // thread and share them with worker threads through thread local storage. + // In per connection mode, therefore, tls is nullptr. + if (tls_) { + tls_->set( + [empty](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return empty; }); + } for (const auto& descriptor : descriptors) { RateLimit::LocalDescriptor new_descriptor; new_descriptor.entries_.reserve(descriptor.entries_size()); for (const auto& entry : descriptor.entries()) { + if (entry.value().empty() && per_connection) { + throw EnvoyException( + "local rate descriptor value cannot be empty in per connection rate limit mode"); + } new_descriptor.entries_.push_back({entry.key(), entry.value()}); } @@ -288,7 +297,8 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( auto iter = descriptors_.find(request_descriptor); if (iter != descriptors_.end()) { matched_descriptors.push_back(iter->second.get()); - } else { + } else if (tls_) { // tls_ is not nullptr means rate limiting is per envoy instance and not per + // connection. // If the request descriptor is not found in the user descriptors, it could be a wildcard case // where value is left blank in the user configured descriptor entry. In this case, we need to // check if it matches any of the dynamic descriptors. diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 928109c76ff3a..a935c2b43934a 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -159,11 +159,11 @@ class LocalRateLimiterImpl : public Logger::Loggable& descriptors, bool always_consume_default_token_bucket = true, - ShareProviderSharedPtr shared_provider = nullptr, const uint32_t lru_size = 20); + ShareProviderSharedPtr shared_provider = nullptr, const uint32_t lru_size = 20, + ThreadLocal::SlotAllocator* tls = nullptr, bool per_connection = false); ~LocalRateLimiterImpl(); Result requestAllowed(absl::Span request_descriptors); diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 3ac0b3260c26b..28f1a169fcc27 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -109,9 +109,10 @@ FilterConfig::FilterConfig( } rate_limiter_ = std::make_unique( - fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, context.threadLocal(), - descriptors_, always_consume_default_token_bucket_, std::move(share_provider), - config.dynamic_descripters_lru_cache_limit()); + fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, descriptors_, + always_consume_default_token_bucket_, std::move(share_provider), + config.dynamic_descripters_lru_cache_limit(), &(context.threadLocal()), + config.local_rate_limit_per_downstream_connection()); } Filters::Common::LocalRateLimit::LocalRateLimiterImpl::Result FilterConfig::requestAllowed( @@ -226,22 +227,22 @@ Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionR ASSERT(used_config_->rateLimitPerConnection()); auto typed_state = - decoder_callbacks_->streamInfo().filterState()->getDataMutable( + decoder_callbacks_->streamInfo().filterState()->getDataReadOnly( PerConnectionRateLimiter::key()); if (typed_state == nullptr) { auto limiter = std::make_shared( used_config_->fillInterval(), used_config_->maxTokens(), used_config_->tokensPerFill(), - decoder_callbacks_->dispatcher(), config_->tls(), used_config_->descriptors(), + decoder_callbacks_->dispatcher(), used_config_->descriptors(), used_config_->consumeDefaultTokenBucket()); decoder_callbacks_->streamInfo().filterState()->setData( PerConnectionRateLimiter::key(), limiter, StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::Connection); - return limiter->value(); + return const_cast(limiter->value()); } - return typed_state->value(); + return const_cast(typed_state->value()); } void Filter::populateDescriptors(std::vector& descriptors, diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index a43986cfbb509..038a7001add1c 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -52,14 +52,16 @@ class PerConnectionRateLimiter : public StreamInfo::FilterState::Object { public: PerConnectionRateLimiter( const std::chrono::milliseconds& fill_interval, uint32_t max_tokens, uint32_t tokens_per_fill, - Envoy::Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, + Envoy::Event::Dispatcher& dispatcher, const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptor, bool always_consume_default_token_bucket) - : rate_limiter_(fill_interval, max_tokens, tokens_per_fill, dispatcher, tls, descriptor, + : rate_limiter_(fill_interval, max_tokens, tokens_per_fill, dispatcher, descriptor, always_consume_default_token_bucket) {} static const std::string& key(); - Filters::Common::LocalRateLimit::LocalRateLimiterImpl& value() { return rate_limiter_; } + const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& value() const { + return rate_limiter_; + } private: Filters::Common::LocalRateLimit::LocalRateLimiterImpl rate_limiter_; diff --git a/source/extensions/filters/listener/local_ratelimit/config.cc b/source/extensions/filters/listener/local_ratelimit/config.cc index 5b729944bedd6..5a2dbde8252ce 100644 --- a/source/extensions/filters/listener/local_ratelimit/config.cc +++ b/source/extensions/filters/listener/local_ratelimit/config.cc @@ -29,8 +29,7 @@ class LocalRateLimitConfigFactory : public Server::Configuration::NamedListenerF message, context.messageValidationVisitor()); FilterConfigSharedPtr config = std::make_shared( - proto_config, context.serverFactoryContext().mainThreadDispatcher(), - context.serverFactoryContext().threadLocal(), context.scope(), + proto_config, context.serverFactoryContext().mainThreadDispatcher(), context.scope(), context.serverFactoryContext().runtime()); return [listener_filter_matcher, config](Network::ListenerFilterManager& filter_manager) -> void { diff --git a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc index f57c9e103464a..85f810ad409ef 100644 --- a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.cc @@ -9,8 +9,7 @@ namespace LocalRateLimit { FilterConfig::FilterConfig( const envoy::extensions::filters::listener::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, - Runtime::Loader& runtime) + Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime) : enabled_(proto_config.runtime_enabled(), runtime), stats_(generateStats(proto_config.stat_prefix(), scope)), rate_limiter_( @@ -18,7 +17,7 @@ FilterConfig::FilterConfig( PROTOBUF_GET_MS_REQUIRED(proto_config.token_bucket(), fill_interval)), proto_config.token_bucket().max_tokens(), PROTOBUF_GET_WRAPPED_OR_DEFAULT(proto_config.token_bucket(), tokens_per_fill, 1), - dispatcher, tls, + dispatcher, Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>()) {} diff --git a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h index ab95802faa11b..ae9e0bf19154f 100644 --- a/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/listener/local_ratelimit/local_ratelimit.h @@ -36,8 +36,7 @@ class FilterConfig : Logger::Loggable { public: FilterConfig( const envoy::extensions::filters::listener::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, - Runtime::Loader& runtime); + Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime); ~FilterConfig() = default; diff --git a/source/extensions/filters/network/local_ratelimit/config.cc b/source/extensions/filters/network/local_ratelimit/config.cc index 1bf44494d245c..f2b935479e9ba 100644 --- a/source/extensions/filters/network/local_ratelimit/config.cc +++ b/source/extensions/filters/network/local_ratelimit/config.cc @@ -14,8 +14,7 @@ Network::FilterFactoryCb LocalRateLimitConfigFactory::createFilterFactoryFromPro const envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit& proto_config, Server::Configuration::FactoryContext& context) { ConfigSharedPtr filter_config(std::make_shared( - proto_config, context.serverFactoryContext().mainThreadDispatcher(), - context.serverFactoryContext().threadLocal(), context.scope(), + proto_config, context.serverFactoryContext().mainThreadDispatcher(), context.scope(), context.serverFactoryContext().runtime(), context.serverFactoryContext().singletonManager())); return [filter_config](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(std::make_shared(filter_config)); diff --git a/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc index 3bf4356624537..09594692dd13a 100644 --- a/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/network/local_ratelimit/local_ratelimit.cc @@ -61,8 +61,8 @@ void SharedRateLimitSingleton::removeIfUnused(const Key* key) { Config::Config( const envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, - Runtime::Loader& runtime, Singleton::Manager& singleton_manager) + Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime, + Singleton::Manager& singleton_manager) : enabled_(proto_config.runtime_enabled(), runtime), stats_(generateStats(proto_config.stat_prefix(), scope)), shared_bucket_registry_(singleton_manager.getTyped( @@ -75,7 +75,7 @@ Config::Config( PROTOBUF_GET_MS_REQUIRED(proto_config.token_bucket(), fill_interval)), proto_config.token_bucket().max_tokens(), PROTOBUF_GET_WRAPPED_OR_DEFAULT(proto_config.token_bucket(), tokens_per_fill, 1), - dispatcher, tls, + dispatcher, Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>()); }); diff --git a/source/extensions/filters/network/local_ratelimit/local_ratelimit.h b/source/extensions/filters/network/local_ratelimit/local_ratelimit.h index 1ce182a1515c9..2ed721e62e3da 100644 --- a/source/extensions/filters/network/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/network/local_ratelimit/local_ratelimit.h @@ -70,8 +70,8 @@ class Config : Logger::Loggable { public: Config( const envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit& proto_config, - Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, Stats::Scope& scope, - Runtime::Loader& runtime, Singleton::Manager& singleton_manager); + Event::Dispatcher& dispatcher, Stats::Scope& scope, Runtime::Loader& runtime, + Singleton::Manager& singleton_manager); ~Config(); diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 064fcbf4d5755..9becf109faa5d 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -127,17 +127,17 @@ class LocalRateLimiterImplTest : public testing::Test { initializeTimer(); - rate_limiter_ = std::make_shared(fill_interval, max_tokens, - tokens_per_fill, dispatcher_, tls_, - descriptors_, true, share_provider); + rate_limiter_ = + std::make_shared(fill_interval, max_tokens, tokens_per_fill, + dispatcher_, descriptors_, true, share_provider); } void initializeWithAtomicTokenBucket(const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, const uint32_t tokens_per_fill, ShareProviderSharedPtr share_provider = nullptr) { - rate_limiter_ = std::make_shared(fill_interval, max_tokens, - tokens_per_fill, dispatcher_, tls_, - descriptors_, true, share_provider); + rate_limiter_ = + std::make_shared(fill_interval, max_tokens, tokens_per_fill, + dispatcher_, descriptors_, true, share_provider); } Thread::ThreadSynchronizer& synchronizer() { return rate_limiter_->synchronizer_; } @@ -147,7 +147,6 @@ class LocalRateLimiterImplTest : public testing::Test { std::vector route_descriptors_; NiceMock dispatcher_; - NiceMock tls_; Event::MockTimer* fill_timer_{}; std::shared_ptr rate_limiter_; }; @@ -155,7 +154,7 @@ class LocalRateLimiterImplTest : public testing::Test { // Make sure we fail with a fill rate this is too fast. TEST_F(LocalRateLimiterImplTest, TooFastFillRate) { EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(49), 100, 1, dispatcher_, tls_, descriptors_), + LocalRateLimiterImpl(std::chrono::milliseconds(49), 100, 1, dispatcher_, descriptors_), EnvoyException, "local rate limit token bucket fill timer must be >= 50ms"); } @@ -372,14 +371,14 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { initializeTimer(); rate_limiter_ = std::make_shared( - fill_interval, max_tokens, tokens_per_fill, dispatcher_, tls_, descriptors_); + fill_interval, max_tokens, tokens_per_fill, dispatcher_, descriptors_); } void initializeWithAtomicTokenBucketDescriptor(const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, const uint32_t tokens_per_fill) { rate_limiter_ = std::make_shared( - fill_interval, max_tokens, tokens_per_fill, dispatcher_, tls_, descriptors_); + fill_interval, max_tokens, tokens_per_fill, dispatcher_, descriptors_); } static constexpr absl::string_view single_descriptor_config_yaml = R"( @@ -415,7 +414,7 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitDivisibleByTokenFi *descriptors_.Add()); EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, tls_, descriptors_), + LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_), EnvoyException, "local rate descriptor limit is not a multiple of token bucket fill timer"); } @@ -426,14 +425,14 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DuplicateDescriptor) { *descriptors_.Add()); EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(50), 1, 1, dispatcher_, tls_, descriptors_), + LocalRateLimiterImpl(std::chrono::milliseconds(50), 1, 1, dispatcher_, descriptors_), EnvoyException, "duplicate descriptor in the local rate descriptor: foo2=bar2"); } // Verify no exception for per route config without descriptors. TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitNoExceptionWithoutDescriptor) { - VERBOSE_EXPECT_NO_THROW(LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, - tls_, descriptors_)); + VERBOSE_EXPECT_NO_THROW( + LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_)); } // Verify various token bucket CAS edge cases for descriptors. diff --git a/test/extensions/filters/listener/local_ratelimit/BUILD b/test/extensions/filters/listener/local_ratelimit/BUILD index 16d644b851e97..184d5402a6f79 100644 --- a/test/extensions/filters/listener/local_ratelimit/BUILD +++ b/test/extensions/filters/listener/local_ratelimit/BUILD @@ -24,7 +24,6 @@ envoy_extension_cc_test( "//source/extensions/filters/listener/local_ratelimit:local_ratelimit_lib", "//test/mocks/network:network_mocks", "//test/mocks/stats:stats_mocks", - "//test/mocks/thread_local:thread_local_mocks", ], ) diff --git a/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc index dd9942838401c..251c3c575dd8d 100644 --- a/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/listener/local_ratelimit/local_ratelimit_test.cc @@ -7,7 +7,6 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/stats/mocks.h" -#include "test/mocks/thread_local/mocks.h" #include "absl/strings/str_format.h" #include "gmock/gmock.h" @@ -60,8 +59,8 @@ class LocalRateLimitTest : public testing::Test, public Event::TestUsingSimulate uint64_t initialize(const std::string& filter_yaml) { envoy::extensions::filters::listener::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYaml(filter_yaml, proto_config); - config_ = std::make_shared(proto_config, dispatcher_, tls_, - *stats_store_.rootScope(), runtime_); + config_ = std::make_shared(proto_config, dispatcher_, *stats_store_.rootScope(), + runtime_); return proto_config.token_bucket().max_tokens(); } @@ -69,7 +68,6 @@ class LocalRateLimitTest : public testing::Test, public Event::TestUsingSimulate Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; FilterConfigSharedPtr config_; - NiceMock tls_; }; // Basic no rate limit case. diff --git a/test/extensions/filters/network/local_ratelimit/BUILD b/test/extensions/filters/network/local_ratelimit/BUILD index 2461c8e7e0802..25d160adae3a4 100644 --- a/test/extensions/filters/network/local_ratelimit/BUILD +++ b/test/extensions/filters/network/local_ratelimit/BUILD @@ -24,7 +24,6 @@ envoy_extension_cc_test( "//test/mocks/event:event_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", - "//test/mocks/thread_local:thread_local_mocks", "@envoy_api//envoy/extensions/filters/network/local_ratelimit/v3:pkg_cc_proto", ], ) @@ -67,7 +66,6 @@ envoy_cc_fuzz_test( "//test/mocks/event:event_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", - "//test/mocks/thread_local:thread_local_mocks", "@envoy_api//envoy/extensions/filters/network/local_ratelimit/v3:pkg_cc_proto", ], ) diff --git a/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc b/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc index cf7fe71a476f1..45bcd52280972 100644 --- a/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc +++ b/test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc @@ -11,7 +11,6 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/thread_local/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -56,7 +55,6 @@ DEFINE_PROTO_FUZZER( return; } static NiceMock dispatcher; - NiceMock tls; // TODO(zhxie): The GlobalTimeSystem in MockDispatcher will initialize itself into a // TestRealTimeSystem by default which is incompatible with the SimulatedTimeSystem in // MockReadFilterCallbacks. We will not need to change the time system after the switching of @@ -70,8 +68,8 @@ DEFINE_PROTO_FUZZER( input.config(); ConfigSharedPtr config = nullptr; try { - config = std::make_shared(proto_config, dispatcher, tls, *stats_store.rootScope(), - runtime, singleton_manager); + config = std::make_shared(proto_config, dispatcher, *stats_store.rootScope(), runtime, + singleton_manager); } catch (EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException in config's constructor: {}", e.what()); return; diff --git a/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc index bf5232da8a991..94af0da7d1348 100644 --- a/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/network/local_ratelimit/local_ratelimit_test.cc @@ -8,7 +8,6 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/thread_local/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -29,13 +28,12 @@ class LocalRateLimitTestBase : public testing::Test, public Event::TestUsingSimu uint64_t initialize(const std::string& filter_yaml) { envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYamlAndValidate(filter_yaml, proto_config); - config_ = std::make_shared(proto_config, dispatcher_, tls_, *stats_store_.rootScope(), + config_ = std::make_shared(proto_config, dispatcher_, *stats_store_.rootScope(), runtime_, singleton_manager_); return proto_config.token_bucket().max_tokens(); } NiceMock dispatcher_; - NiceMock tls_; Stats::IsolatedStoreImpl stats_store_; NiceMock runtime_; Singleton::ManagerImpl singleton_manager_; @@ -139,7 +137,7 @@ class LocalRateLimitSharedTokenBucketTest : public LocalRateLimitTestBase { envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYamlAndValidate(filter_yaml2, proto_config); const uint64_t config2_tokens = proto_config.token_bucket().max_tokens(); - config2_ = std::make_shared(proto_config, dispatcher_, tls_, *stats_store_.rootScope(), + config2_ = std::make_shared(proto_config, dispatcher_, *stats_store_.rootScope(), runtime_, singleton_manager_); // This test just uses the initial tokens without ever refilling. @@ -231,8 +229,8 @@ share_key: key_{} std::string yaml = fmt::format(yaml_template, i); envoy::extensions::filters::network::local_ratelimit::v3::LocalRateLimit proto_config; TestUtility::loadFromYamlAndValidate(yaml, proto_config); - configs.push_back(std::make_unique( - proto_config, dispatcher_, tls_, *stats_store_.rootScope(), runtime_, singleton_manager_)); + configs.push_back(std::make_unique(proto_config, dispatcher_, *stats_store_.rootScope(), + runtime_, singleton_manager_)); } configs.clear(); From 53e84bb717629aab566e12ff1034a1d14116a5bc Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sat, 19 Oct 2024 08:35:25 +0530 Subject: [PATCH 11/41] fix Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/common/local_ratelimit/local_ratelimit_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index c27e62162d218..b4931af0df0b9 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -309,7 +309,7 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( ENVOY_LOG(debug, "dynamic_descriptors is empty"); } else { auto dynamic_iter = dynamic_descriptors.find(request_descriptor); - if (dynamic_iter != dynamic_descriptors_.end()) { + if (dynamic_iter != dynamic_descriptors.end()) { matched_descriptors.push_back(dynamic_iter->second.get()); continue; } From bb341798282adea96849e7fd807fe922bf0d6c3e Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Wed, 23 Oct 2024 06:24:30 +0530 Subject: [PATCH 12/41] DynamicDescriptor interface Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/v3/local_rate_limit.proto | 7 +- .../local_ratelimit/local_ratelimit_impl.cc | 229 +++++++++--------- .../local_ratelimit/local_ratelimit_impl.h | 65 +++-- .../http/local_ratelimit/local_ratelimit.cc | 2 +- 4 files changed, 174 insertions(+), 129 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index a370cf45330d3..5cbcaf64a90e7 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -168,7 +168,12 @@ message LocalRateLimit { // 4. :ref:`override limit `. repeated config.route.v3.RateLimit rate_limits = 17; - // Specifies limit for the lru cache which is used to store dynamic descriptors. + // Specifies the max dynamic descriptors kept in the cache for a particular wildcarded descriptor + // configured in the global :ref:`descriptors`. + // Wildcarded descriptor means descriptor has one or more entries `value` omitted. For example if user has configured two descriptors + // with blank value entries, then max dynamic descriptors stored in the LRU cache will be 2 * dynamic_descripters_lru_cache_limit. + // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted + // values. // Default is 20. uint32 dynamic_descripters_lru_cache_limit = 18; } diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index b4931af0df0b9..037bb8e4885e9 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -175,16 +175,15 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors, bool always_consume_default_token_bucket, ShareProviderSharedPtr shared_provider, - uint32_t lru_size, ThreadLocal::SlotAllocator* tls, bool per_connection) + uint32_t lru_size, bool per_connection) : fill_timer_(fill_interval > std::chrono::milliseconds(0) ? dispatcher.createTimer([this] { onFillTimer(); }) : nullptr), - time_source_(dispatcher.timeSource()), tls_(tls != nullptr ? tls->allocateSlot() : nullptr), - share_provider_(std::move(shared_provider)), + time_source_(dispatcher.timeSource()), share_provider_(std::move(shared_provider)), always_consume_default_token_bucket_(always_consume_default_token_bucket), no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")), - dispatcher_(dispatcher), lru_size_(lru_size == 0 ? 20 : lru_size) { + dispatcher_(dispatcher) { if (fill_timer_ && fill_interval < std::chrono::milliseconds(50)) { throw EnvoyException("local rate limit token bucket fill timer must be >= 50ms"); } @@ -201,26 +200,18 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( !no_timer_based_rate_limit_token_bucket_) { fill_timer_->enableTimer(default_token_bucket_->fillInterval()); } - RateLimit::LocalDescriptor::Map blank_descriptors; - - ThreadLocalDynamicDescriptorsCacheSharedPtr empty( - new ThreadLocalDynamicDescriptorsCache(blank_descriptors)); - // in per connection rate limit mode, the dynamic descriptors are stored in the per connection - // which is per worker thread. So we don't need to store the dynamic descriptors in the main - // thread and share them with worker threads through thread local storage. - // In per connection mode, therefore, tls is nullptr. - if (tls_) { - tls_->set( - [empty](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return empty; }); - } for (const auto& descriptor : descriptors) { RateLimit::LocalDescriptor new_descriptor; + bool wildcard_found = false; new_descriptor.entries_.reserve(descriptor.entries_size()); for (const auto& entry : descriptor.entries()) { - if (entry.value().empty() && per_connection) { - throw EnvoyException( - "local rate descriptor value cannot be empty in per connection rate limit mode"); + if (entry.value().empty()) { + if (per_connection) { + throw EnvoyException( + "local rate descriptor value cannot be empty in per connection rate limit mode"); + } + wildcard_found = true; } new_descriptor.entries_.push_back({entry.key(), entry.value()}); } @@ -248,7 +239,13 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( per_descriptor_max_tokens, per_descriptor_tokens_per_fill, per_descriptor_fill_interval, per_descriptor_multiplier, *this); } - + if (wildcard_found) { + DynamicDescriptorSharedPtr dynamic_descriptor = std::make_shared( + per_descriptor_token_bucket, (lru_size == 0 ? 20 : lru_size), dispatcher.timeSource(), + *this); + dynamic_descriptors_.addDescriptor(std::move(new_descriptor), std::move(dynamic_descriptor)); + continue; + } auto result = descriptors_.emplace(std::move(new_descriptor), std::move(per_descriptor_token_bucket)); if (!result.second) { @@ -278,9 +275,7 @@ void LocalRateLimiterImpl::onFillTimer() { for (const auto& descriptor : descriptors_) { descriptor.second->onFillTimer(refill_counter_, share_factor); } - for (const auto& descriptor : dynamic_descriptors_) { - descriptor.second->onFillTimer(refill_counter_, share_factor); - } + dynamic_descriptors_.onFillTimer(refill_counter_, share_factor); fill_timer_->enableTimer(default_token_bucket_->fillInterval()); } @@ -297,61 +292,10 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( auto iter = descriptors_.find(request_descriptor); if (iter != descriptors_.end()) { matched_descriptors.push_back(iter->second.get()); - } else if (tls_) { // tls_ is not nullptr means rate limiting is per envoy instance and not per - // connection. - // If the request descriptor is not found in the user descriptors, it could be a wildcard case - // where value is left blank in the user configured descriptor entry. In this case, we need to - // check if it matches any of the dynamic descriptors. - // find request descriptor in the existing dynamic descriptors. If it exists, add token - // bucket to matched_descriptors. - auto dynamic_descriptors = threadLocal().cache(); - if (dynamic_descriptors.empty()) { - ENVOY_LOG(debug, "dynamic_descriptors is empty"); - } else { - auto dynamic_iter = dynamic_descriptors.find(request_descriptor); - if (dynamic_iter != dynamic_descriptors.end()) { - matched_descriptors.push_back(dynamic_iter->second.get()); - continue; - } - } - - // If it does not exist, then it could be the first request for a dynamic descriptor. Verify - // it matches any of the user configured descriptors by key where value is left blank - for (const auto& pair : descriptors_) { - auto user_descriptor = pair.first; - auto user_bucket = pair.second.get(); - if (user_descriptor.entries_.size() != request_descriptor.entries_.size()) { - continue; - } - RateLimit::LocalDescriptor new_descriptor; - bool wildcard_found = false; - new_descriptor.entries_.reserve(user_descriptor.entries_.size()); - wildcard_found = compareDescriptorEntries( - request_descriptor.entries_, user_descriptor.entries_, new_descriptor.entries_); - - if (!wildcard_found) { - continue; - } - RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; - if (no_timer_based_rate_limit_token_bucket_) { - ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); - ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", - user_bucket->maxTokens(), user_bucket->fillRate(), - std::chrono::duration(user_bucket->fillInterval()).count()); - per_descriptor_token_bucket = std::make_shared( - user_bucket->maxTokens(), - uint32_t(user_bucket->fillRate() * - std::chrono::duration(user_bucket->fillInterval()).count()), - user_bucket->fillInterval(), time_source_); - } else { - per_descriptor_token_bucket = std::make_shared( - user_bucket->maxTokens(), - uint32_t(user_bucket->fillRate() * - std::chrono::duration(user_bucket->fillInterval()).count()), - user_bucket->fillInterval(), user_bucket->multiplier(), *this); - } - matched_descriptors.push_back(per_descriptor_token_bucket.get()); - notifyMainThread(move(per_descriptor_token_bucket), std::move(new_descriptor)); + } else { + auto token_bucket = dynamic_descriptors_.getBucket(request_descriptor); + if (token_bucket != nullptr) { + matched_descriptors.push_back(token_bucket.get()); } } } @@ -400,42 +344,9 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( return {true, makeOptRefFromPtr(matched_descriptors[0])}; } -void LocalRateLimiterImpl::notifyMainThread(RateLimitTokenBucketSharedPtr token_bucket, - RateLimit::LocalDescriptor new_descriptor) { - dispatcher_.post([this, token_bucket, new_descriptor]() -> void { - ENVOY_LOG(trace, "notifyMainThread: creating dynamic descriptor: {}", - new_descriptor.toString()); - // add this bucket to cache. - // After updating cache, make a copy of the cache and update the tls with the new cache. - auto result = dynamic_descriptors_.emplace(new_descriptor, std::move(token_bucket)); - if (!result.second) { - ENVOY_LOG(trace, "notifyMainThread: descriptor, {}, already in the cache", - result.first->first.toString()); - return; - } - lru_list_.emplace_front(new_descriptor); - if (lru_list_.size() > lru_size_) { - auto lru_entry = lru_list_.back(); - auto erased = dynamic_descriptors_.erase(lru_entry); - ENVOY_LOG(trace, "max size: {}, current size: {}, lru entry(descriptor): {}, erased: {}", - lru_size_, lru_list_.size(), lru_entry.toString(), erased); - lru_list_.pop_back(); - } - - // copy the cache and update the tls. - auto cache = dynamic_descriptors_; - - tls_->set([cache](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { - auto copy = - std::make_shared>(cache); - return std::make_shared(*copy); - }); - }); -} - // Compare the request descriptor entries with the user descriptor entries. If all non-empty user // descriptor values match the request descriptor values, return true and fill the new descriptor -bool LocalRateLimiterImpl::compareDescriptorEntries( +bool DynamicDescriptorMap::compareDescriptorEntries( const std::vector& request_entries, const std::vector& user_entries, std::vector& new_descriptor_entries) { @@ -465,6 +376,100 @@ bool LocalRateLimiterImpl::compareDescriptorEntries( return has_empty_value; } +void DynamicDescriptorMap::addDescriptor(const RateLimit::LocalDescriptor& user_descriptor, + DynamicDescriptorSharedPtr dynamic_descriptor) { + auto result = user_descriptors_.emplace(user_descriptor, std::move(dynamic_descriptor)); + if (!result.second) { + throw EnvoyException(absl::StrCat("duplicate descriptor in the local rate descriptor: ", + result.first->first.toString())); + } +} + +RateLimitTokenBucketSharedPtr +DynamicDescriptorMap::getBucket(RateLimit::LocalDescriptor request_descriptor) { + for (const auto& pair : user_descriptors_) { + auto user_descriptor = pair.first; + if (user_descriptor.entries_.size() != request_descriptor.entries_.size()) { + continue; + } + RateLimit::LocalDescriptor new_descriptor; + bool wildcard_found = false; + new_descriptor.entries_.reserve(user_descriptor.entries_.size()); + wildcard_found = compareDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_, + new_descriptor.entries_); + + if (!wildcard_found) { + continue; + } + + // we found a user configured wildcard descriptor that matches the request descriptor. + return pair.second->addOrGetDescriptor(request_descriptor); + } + return nullptr; +} + +void DynamicDescriptorMap::onFillTimer(uint64_t refill_counter, double factor) { + for (const auto& pair : user_descriptors_) { + pair.second->onFillTimer(refill_counter, factor); + } +} + +DynamicDescriptor::DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, + TimeSource& time_source, LocalRateLimiterImpl& parent) + : parent_token_bucket_(token_bucket), lru_size_(lru_size), time_source_(time_source), + no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")), + parent_(parent) {} + +RateLimitTokenBucketSharedPtr +DynamicDescriptor::addOrGetDescriptor(const RateLimit::LocalDescriptor& request_descriptor) { + absl::WriterMutexLock lock(&dyn_desc_lock_); + auto iter = dynamic_descriptors_.find(request_descriptor); + if (iter != dynamic_descriptors_.end()) { + lru_list_.splice(lru_list_.begin(), lru_list_, iter->second.second); + return iter->second.first; + } + // add a new descriptor to the set along with its toekn bucket + RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; + if (no_timer_based_rate_limit_token_bucket_) { + ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); + ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", + parent_token_bucket_->maxTokens(), parent_token_bucket_->fillRate(), + std::chrono::duration(parent_token_bucket_->fillInterval()).count()); + per_descriptor_token_bucket = std::make_shared( + parent_token_bucket_->maxTokens(), + uint32_t(parent_token_bucket_->fillRate() * + std::chrono::duration(parent_token_bucket_->fillInterval()).count()), + parent_token_bucket_->fillInterval(), time_source_); + } else { + per_descriptor_token_bucket = std::make_shared( + parent_token_bucket_->maxTokens(), + uint32_t(parent_token_bucket_->fillRate() * + std::chrono::duration(parent_token_bucket_->fillInterval()).count()), + parent_token_bucket_->fillInterval(), parent_token_bucket_->multiplier(), parent_); + } + + ENVOY_LOG(trace, "DynamicDescriptor::addorGetDescriptor: adding dynamic descriptor: {}", + request_descriptor.toString()); + // add this bucket to cache. + // After updating cache, make a copy of the cache and update the tls with the new cache. + auto result = dynamic_descriptors_.emplace( + request_descriptor, std::pair(per_descriptor_token_bucket, lru_list_.begin())); + lru_list_.emplace_front(request_descriptor); + if (lru_list_.size() >= lru_size_) { + dynamic_descriptors_.erase(lru_list_.back()); + lru_list_.pop_back(); + } + return result.first->second.first; +} + +void DynamicDescriptor::onFillTimer(uint64_t refill_counter, double factor) { + absl::WriterMutexLock lock(&dyn_desc_lock_); + for (auto& pair : dynamic_descriptors_) { + pair.second.first->onFillTimer(refill_counter, factor); + } +} + } // namespace LocalRateLimit } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index a935c2b43934a..8ea5b6f7fe4e0 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -20,8 +20,56 @@ namespace Filters { namespace Common { namespace LocalRateLimit { +class LocalRateLimiterImpl; +class RateLimitTokenBucket; +using RateLimitTokenBucketSharedPtr = std::shared_ptr; using ProtoLocalClusterRateLimit = envoy::extensions::common::ratelimit::v3::LocalClusterRateLimit; +class DynamicDescriptor : public Logger::Loggable { +public: + DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, TimeSource&, + LocalRateLimiterImpl& parent); + ~DynamicDescriptor() = default; + // add a new user configured descriptor to the set. + RateLimitTokenBucketSharedPtr + addOrGetDescriptor(const RateLimit::LocalDescriptor& request_descriptor); + void onFillTimer(uint64_t refill_counter, double factor); + +private: + RateLimitTokenBucketSharedPtr parent_token_bucket_; + using LruList = std::list; + + mutable absl::Mutex dyn_desc_lock_; + RateLimit::LocalDescriptor::Map> + dynamic_descriptors_ ABSL_GUARDED_BY(dyn_desc_lock_); + + LruList lru_list_; + uint32_t lru_size_; + TimeSource& time_source_; + const bool no_timer_based_rate_limit_token_bucket_{}; + LocalRateLimiterImpl& parent_; +}; + +using DynamicDescriptorSharedPtr = std::shared_ptr; + +class DynamicDescriptorMap : public Logger::Loggable { +public: + DynamicDescriptorMap() = default; + ~DynamicDescriptorMap() = default; + // add a new user configured descriptor to the set. + void addDescriptor(const RateLimit::LocalDescriptor& descriptor, + DynamicDescriptorSharedPtr dynamic_descriptor); + // pass request_descriptors to the dynamic descriptor set to get the token bucket. + RateLimitTokenBucketSharedPtr getBucket(RateLimit::LocalDescriptor); + void onFillTimer(uint64_t refill_counter, double factor); + +private: + bool compareDescriptorEntries(const std::vector& request_entries, + const std::vector& user_entries, + std::vector& new_descriptor_entries); + RateLimit::LocalDescriptor::Map user_descriptors_; +}; + class ShareProvider { public: virtual ~ShareProvider() = default; @@ -75,9 +123,6 @@ class RateLimitTokenBucket : public TokenBucketContext { virtual double fillRate() const PURE; virtual uint64_t multiplier() const { return 1; } }; -using RateLimitTokenBucketSharedPtr = std::shared_ptr; - -class LocalRateLimiterImpl; class ThreadLocalDynamicDescriptorsCache : public ThreadLocal::ThreadLocalObject { public: @@ -163,7 +208,7 @@ class LocalRateLimiterImpl : public Logger::Loggable& descriptors, bool always_consume_default_token_bucket = true, ShareProviderSharedPtr shared_provider = nullptr, const uint32_t lru_size = 20, - ThreadLocal::SlotAllocator* tls = nullptr, bool per_connection = false); + bool per_connection = false); ~LocalRateLimiterImpl(); Result requestAllowed(absl::Span request_descriptors); @@ -173,11 +218,6 @@ class LocalRateLimiterImpl : public Logger::Loggable& request_entries, const std::vector& user_entries, std::vector& new_descriptor_entries); - void notifyMainThread(RateLimitTokenBucketSharedPtr token_bucket, - RateLimit::LocalDescriptor new_descriptor); - const ThreadLocalDynamicDescriptorsCache& threadLocal() const { - return tls_->getTyped(); - } RateLimitTokenBucketSharedPtr default_token_bucket_; @@ -185,8 +225,7 @@ class LocalRateLimiterImpl : public Logger::Loggable descriptors_; - RateLimit::LocalDescriptor::Map dynamic_descriptors_; - ThreadLocal::SlotPtr tls_; + DynamicDescriptorMap dynamic_descriptors_{}; // Refill counter is incremented per each refill timer hit. uint64_t refill_counter_{0}; @@ -198,10 +237,6 @@ class LocalRateLimiterImpl : public Logger::Loggable; - LruList lru_list_; - uint32_t lru_size_; - friend class LocalRateLimiterImplTest; friend class TimerTokenBucket; }; diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 28f1a169fcc27..c79d76ddf4b11 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -111,7 +111,7 @@ FilterConfig::FilterConfig( rate_limiter_ = std::make_unique( fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, descriptors_, always_consume_default_token_bucket_, std::move(share_provider), - config.dynamic_descripters_lru_cache_limit(), &(context.threadLocal()), + config.dynamic_descripters_lru_cache_limit(), config.local_rate_limit_per_downstream_connection()); } From b8fa581e6d5248e64a4e23374c29cc30ee1050b1 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Tue, 14 Jan 2025 17:30:50 +0530 Subject: [PATCH 13/41] Address review comments Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/v3/local_rate_limit.proto | 4 +- source/common/runtime/runtime_features.cc | 4 ++ .../local_ratelimit/local_ratelimit_impl.cc | 57 ++++++++++--------- .../local_ratelimit/local_ratelimit_impl.h | 5 +- .../http/local_ratelimit/local_ratelimit.cc | 9 ++- .../http/local_ratelimit/local_ratelimit.h | 2 +- .../filters/http/local_ratelimit/BUILD | 1 + .../local_ratelimit_integration_test.cc | 4 ++ 8 files changed, 51 insertions(+), 35 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index 5cbcaf64a90e7..79325aec29b5d 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -171,9 +171,9 @@ message LocalRateLimit { // Specifies the max dynamic descriptors kept in the cache for a particular wildcarded descriptor // configured in the global :ref:`descriptors`. // Wildcarded descriptor means descriptor has one or more entries `value` omitted. For example if user has configured two descriptors - // with blank value entries, then max dynamic descriptors stored in the LRU cache will be 2 * dynamic_descripters_lru_cache_limit. + // with blank value entries, then max dynamic descriptors stored in the LRU cache will be 2 * max_dynamic_descriptors. // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted // values. // Default is 20. - uint32 dynamic_descripters_lru_cache_limit = 18; + google.protobuf.UInt32Value max_dynamic_descriptors = 18; } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index a06e49b182eaa..37b07681d190f 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -169,6 +169,10 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_logging_with_fast_json_formatter); // before downstream. FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); +// A flag to enable the usage of dynamic buckets for local rate limiting. For example, dynamically +// created token buckets for each unique value of a request header. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_local_rate_limiting_with_dynamic_buckets); + // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT ABSL_FLAG(uint64_t, re2_max_program_size_warn_level, // NOLINT diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 037bb8e4885e9..290dc529c1271 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -207,6 +207,11 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( new_descriptor.entries_.reserve(descriptor.entries_size()); for (const auto& entry : descriptor.entries()) { if (entry.value().empty()) { + if (!(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets"))) { + throw EnvoyException("local_rate_limiting_with_dynamic_buckets is disabled. Local rate " + "descriptor value cannot be empty"); + } if (per_connection) { throw EnvoyException( "local rate descriptor value cannot be empty in per connection rate limit mode"); @@ -241,8 +246,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( } if (wildcard_found) { DynamicDescriptorSharedPtr dynamic_descriptor = std::make_shared( - per_descriptor_token_bucket, (lru_size == 0 ? 20 : lru_size), dispatcher.timeSource(), - *this); + per_descriptor_token_bucket, lru_size, dispatcher.timeSource(), *this); dynamic_descriptors_.addDescriptor(std::move(new_descriptor), std::move(dynamic_descriptor)); continue; } @@ -285,17 +289,17 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( // In most cases the request descriptors has only few elements. We use a inlined vector to // avoid heap allocation. - absl::InlinedVector matched_descriptors; + absl::InlinedVector matched_descriptors; // Find all matched descriptors. for (const auto& request_descriptor : request_descriptors) { auto iter = descriptors_.find(request_descriptor); if (iter != descriptors_.end()) { - matched_descriptors.push_back(iter->second.get()); + matched_descriptors.push_back(iter->second); } else { auto token_bucket = dynamic_descriptors_.getBucket(request_descriptor); if (token_bucket != nullptr) { - matched_descriptors.push_back(token_bucket.get()); + matched_descriptors.push_back(token_bucket); } } } @@ -304,7 +308,7 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( // Sort the matched descriptors by token bucket fill rate to ensure the descriptor with the // smallest fill rate is consumed first. std::sort(matched_descriptors.begin(), matched_descriptors.end(), - [](const RateLimitTokenBucket* lhs, const RateLimitTokenBucket* rhs) { + [](const RateLimitTokenBucketSharedPtr lhs, const RateLimitTokenBucketSharedPtr rhs) { return lhs->fillRate() < rhs->fillRate(); }); } @@ -313,11 +317,11 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( share_provider_ != nullptr ? share_provider_->getTokensShareFactor() : 1.0; // See if the request is forbidden by any of the matched descriptors. - for (auto descriptor : matched_descriptors) { + for (const auto& descriptor : matched_descriptors) { if (!descriptor->consume(share_factor)) { // If the request is forbidden by a descriptor, return the result and the descriptor // token bucket. - return {false, makeOptRefFromPtr(descriptor)}; + return {false, std::shared_ptr(descriptor)}; } else { ENVOY_LOG(trace, "request allowed by descriptor with fill rate: {}, maxToken: {}, remainingToken {}", @@ -330,26 +334,25 @@ LocalRateLimiterImpl::Result LocalRateLimiterImpl::requestAllowed( if (const bool result = default_token_bucket_->consume(share_factor); !result) { // If the request is forbidden by the default token bucket, return the result and the // default token bucket. - return {false, makeOptRefFromPtr(default_token_bucket_.get())}; + return {false, std::shared_ptr(default_token_bucket_)}; } // If the request is allowed then return the result the token bucket. The descriptor // token bucket will be selected as priority if it exists. - return {true, makeOptRefFromPtr(matched_descriptors.empty() - ? default_token_bucket_.get() - : matched_descriptors[0])}; + return {true, matched_descriptors.empty() ? default_token_bucket_ : matched_descriptors[0]}; }; ASSERT(!matched_descriptors.empty()); - return {true, makeOptRefFromPtr(matched_descriptors[0])}; + std::shared_ptr bucket_context = + std::shared_ptr(matched_descriptors[0]); + return {true, bucket_context}; } // Compare the request descriptor entries with the user descriptor entries. If all non-empty user -// descriptor values match the request descriptor values, return true and fill the new descriptor +// descriptor values match the request descriptor values, return true bool DynamicDescriptorMap::compareDescriptorEntries( const std::vector& request_entries, - const std::vector& user_entries, - std::vector& new_descriptor_entries) { + const std::vector& user_entries) { // Check for equality of sizes if (request_entries.size() != user_entries.size()) { return false; @@ -371,7 +374,6 @@ bool DynamicDescriptorMap::compareDescriptorEntries( if (user_entries[i].value_.empty()) { has_empty_value = true; } - new_descriptor_entries.push_back({request_entries[i].key_, request_entries[i].value_}); } return has_empty_value; } @@ -392,11 +394,9 @@ DynamicDescriptorMap::getBucket(RateLimit::LocalDescriptor request_descriptor) { if (user_descriptor.entries_.size() != request_descriptor.entries_.size()) { continue; } - RateLimit::LocalDescriptor new_descriptor; bool wildcard_found = false; - new_descriptor.entries_.reserve(user_descriptor.entries_.size()); - wildcard_found = compareDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_, - new_descriptor.entries_); + wildcard_found = + compareDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_); if (!wildcard_found) { continue; @@ -426,10 +426,12 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::LocalDescriptor& request_ absl::WriterMutexLock lock(&dyn_desc_lock_); auto iter = dynamic_descriptors_.find(request_descriptor); if (iter != dynamic_descriptors_.end()) { - lru_list_.splice(lru_list_.begin(), lru_list_, iter->second.second); + if (iter->second.second != lru_list_.begin()) { + lru_list_.splice(lru_list_.begin(), lru_list_, iter->second.second); + } return iter->second.first; } - // add a new descriptor to the set along with its toekn bucket + // add a new descriptor to the set along with its token bucket RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; if (no_timer_based_rate_limit_token_bucket_) { ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); @@ -451,15 +453,18 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::LocalDescriptor& request_ ENVOY_LOG(trace, "DynamicDescriptor::addorGetDescriptor: adding dynamic descriptor: {}", request_descriptor.toString()); - // add this bucket to cache. - // After updating cache, make a copy of the cache and update the tls with the new cache. + lru_list_.emplace_front(request_descriptor); auto result = dynamic_descriptors_.emplace( request_descriptor, std::pair(per_descriptor_token_bucket, lru_list_.begin())); - lru_list_.emplace_front(request_descriptor); if (lru_list_.size() >= lru_size_) { + ENVOY_LOG(trace, + "DynamicDescriptor::addorGetDescriptor: lru_size({}) overflow. Removing dynamic " + "descriptor: {}", + lru_size_, lru_list_.back().toString()); dynamic_descriptors_.erase(lru_list_.back()); lru_list_.pop_back(); } + ASSERT(lru_list_.size() == dynamic_descriptors_.size()); return result.first->second.first; } diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 8ea5b6f7fe4e0..6c2d50ee40ed4 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -65,8 +65,7 @@ class DynamicDescriptorMap : public Logger::Loggable& request_entries, - const std::vector& user_entries, - std::vector& new_descriptor_entries); + const std::vector& user_entries); RateLimit::LocalDescriptor::Map user_descriptors_; }; @@ -198,7 +197,7 @@ class LocalRateLimiterImpl : public Logger::Loggable token_bucket_context{}; + std::shared_ptr token_bucket_context{}; }; LocalRateLimiterImpl( diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index c79d76ddf4b11..941b2c33c2de9 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -107,11 +107,14 @@ FilterConfig::FilterConfig( share_provider = share_provider_manager_->getShareProvider(config.local_cluster_rate_limit()); } + uint32_t max_dynamic_descriptors = 20; + if (config.has_max_dynamic_descriptors()) { + max_dynamic_descriptors = config.max_dynamic_descriptors().value(); + } rate_limiter_ = std::make_unique( fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, descriptors_, - always_consume_default_token_bucket_, std::move(share_provider), - config.dynamic_descripters_lru_cache_limit(), + always_consume_default_token_bucket_, std::move(share_provider), max_dynamic_descriptors, config.local_rate_limit_per_downstream_connection()); } @@ -199,7 +202,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { // We can never assume the decodeHeaders() was called before encodeHeaders(). - if (used_config_->enableXRateLimitHeaders() && token_bucket_context_.has_value()) { + if (used_config_->enableXRateLimitHeaders() && token_bucket_context_) { headers.addReferenceKey( HttpFilters::Common::RateLimit::XRateLimitHeaders::get().XRateLimitLimit, token_bucket_context_->maxTokens()); diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index 038a7001add1c..17639f4762fed 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -203,7 +203,7 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable token_bucket_context_; + std::shared_ptr token_bucket_context_; VhRateLimitOptions vh_rate_limits_; }; diff --git a/test/extensions/filters/http/local_ratelimit/BUILD b/test/extensions/filters/http/local_ratelimit/BUILD index e71f31db0857e..123c974affd3d 100644 --- a/test/extensions/filters/http/local_ratelimit/BUILD +++ b/test/extensions/filters/http/local_ratelimit/BUILD @@ -51,6 +51,7 @@ envoy_extension_cc_test( ], deps = [ "//source/extensions/filters/http/local_ratelimit:config", + "//test/test_common:test_runtime_lib", "//test/integration:http_protocol_integration_lib", ], ) diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index 7ce0030dfc820..2ce0706583658 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -1,6 +1,7 @@ #include "source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h" #include "test/integration/http_protocol_integration.h" +#include "test/test_common/test_runtime.h" #include "gtest/gtest.h" @@ -376,6 +377,9 @@ INSTANTIATE_TEST_SUITE_P( HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); initializeFilter(fmt::format(filter_config_with_blank_value_descriptor_, "false")); // filter is adding dynamic descriptors based on the request header // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval From e9690d4494c357b2fb869834a4ff30fa61abf07b Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Wed, 15 Jan 2025 06:24:21 +0530 Subject: [PATCH 14/41] some cleanup Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/common/local_ratelimit/local_ratelimit_impl.cc | 3 +-- .../filters/common/local_ratelimit/local_ratelimit_impl.h | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index ae28d65ff8434..d6746bd580295 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -184,8 +184,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( time_source_(dispatcher.timeSource()), share_provider_(std::move(shared_provider)), always_consume_default_token_bucket_(always_consume_default_token_bucket), no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")), - dispatcher_(dispatcher) { + "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")) { if (fill_timer_ && fill_interval < std::chrono::milliseconds(50)) { throw EnvoyException("local rate limit token bucket fill timer must be >= 50ms"); } diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 03ab5ba3920d3..224619c0baec9 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -217,7 +217,6 @@ class LocalRateLimiterImpl : public Logger::Loggable Date: Wed, 15 Jan 2025 08:59:32 +0530 Subject: [PATCH 15/41] format Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/http/local_ratelimit/v3/local_rate_limit.proto | 4 ++-- test/extensions/filters/http/local_ratelimit/BUILD | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index 79325aec29b5d..973d1e861ad6b 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -168,9 +168,9 @@ message LocalRateLimit { // 4. :ref:`override limit `. repeated config.route.v3.RateLimit rate_limits = 17; - // Specifies the max dynamic descriptors kept in the cache for a particular wildcarded descriptor + // Specifies the max dynamic descriptors kept in the cache for a particular wildcard descriptor // configured in the global :ref:`descriptors`. - // Wildcarded descriptor means descriptor has one or more entries `value` omitted. For example if user has configured two descriptors + // Wildcard descriptor means descriptor has one or more entries `value` omitted. For example if user has configured two descriptors // with blank value entries, then max dynamic descriptors stored in the LRU cache will be 2 * max_dynamic_descriptors. // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted // values. diff --git a/test/extensions/filters/http/local_ratelimit/BUILD b/test/extensions/filters/http/local_ratelimit/BUILD index 381efd2a4bf81..366f0e675f80e 100644 --- a/test/extensions/filters/http/local_ratelimit/BUILD +++ b/test/extensions/filters/http/local_ratelimit/BUILD @@ -51,7 +51,7 @@ envoy_extension_cc_test( ], deps = [ "//source/extensions/filters/http/local_ratelimit:config", - "//test/test_common:test_runtime_lib", "//test/integration:http_protocol_integration_lib", + "//test/test_common:test_runtime_lib", ], ) From d0b0433d52be3cf53862a2c61508a7c1cc22f2db Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Thu, 16 Jan 2025 06:24:53 +0530 Subject: [PATCH 16/41] format Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/http/local_ratelimit/v3/local_rate_limit.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index 973d1e861ad6b..d426d97893aae 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -170,7 +170,7 @@ message LocalRateLimit { // Specifies the max dynamic descriptors kept in the cache for a particular wildcard descriptor // configured in the global :ref:`descriptors`. - // Wildcard descriptor means descriptor has one or more entries `value` omitted. For example if user has configured two descriptors + // Wildcard descriptor means descriptor has one or more entries with just key and value omitted. For example if user has configured two descriptors // with blank value entries, then max dynamic descriptors stored in the LRU cache will be 2 * max_dynamic_descriptors. // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted // values. From 3463d2829943f109896e6ee7ca8231ce7fe8f85a Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sat, 18 Jan 2025 11:44:34 +0530 Subject: [PATCH 17/41] some cleanup Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/local_ratelimit_impl.cc | 40 ++++++++----------- .../local_ratelimit/local_ratelimit_impl.h | 5 +-- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index d6746bd580295..194fb558fd25d 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -213,6 +213,11 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( throw EnvoyException("local_rate_limiting_with_dynamic_buckets is disabled. Local rate " "descriptor value cannot be empty"); } + if (!no_timer_based_rate_limit_token_bucket_) { + throw EnvoyException("local rate limit descriptor value cannot be empty without " + "no_timer_based_rate_limit_token_bucket"); + } + if (per_connection) { throw EnvoyException( "local rate descriptor value cannot be empty in per connection rate limit mode"); @@ -253,7 +258,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( } if (wildcard_found) { DynamicDescriptorSharedPtr dynamic_descriptor = std::make_shared( - per_descriptor_token_bucket, lru_size, dispatcher.timeSource(), *this); + per_descriptor_token_bucket, lru_size, dispatcher.timeSource()); dynamic_descriptors_.addDescriptor(std::move(new_descriptor), std::move(dynamic_descriptor)); continue; } @@ -428,11 +433,8 @@ void DynamicDescriptorMap::onFillTimer(uint64_t refill_counter, double factor) { } DynamicDescriptor::DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, - TimeSource& time_source, LocalRateLimiterImpl& parent) - : parent_token_bucket_(token_bucket), lru_size_(lru_size), time_source_(time_source), - no_timer_based_rate_limit_token_bucket_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.no_timer_based_rate_limit_token_bucket")), - parent_(parent) {} + TimeSource& time_source) + : parent_token_bucket_(token_bucket), lru_size_(lru_size), time_source_(time_source) {} RateLimitTokenBucketSharedPtr DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descriptor) { @@ -446,23 +448,15 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descr } // add a new descriptor to the set along with its token bucket RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; - if (no_timer_based_rate_limit_token_bucket_) { - ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); - ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", - parent_token_bucket_->maxTokens(), parent_token_bucket_->fillRate(), - std::chrono::duration(parent_token_bucket_->fillInterval()).count()); - per_descriptor_token_bucket = std::make_shared( - parent_token_bucket_->maxTokens(), - uint32_t(parent_token_bucket_->fillRate() * - std::chrono::duration(parent_token_bucket_->fillInterval()).count()), - parent_token_bucket_->fillInterval(), time_source_); - } else { - per_descriptor_token_bucket = std::make_shared( - parent_token_bucket_->maxTokens(), - uint32_t(parent_token_bucket_->fillRate() * - std::chrono::duration(parent_token_bucket_->fillInterval()).count()), - parent_token_bucket_->fillInterval(), parent_token_bucket_->multiplier(), parent_); - } + ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); + ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", + parent_token_bucket_->maxTokens(), parent_token_bucket_->fillRate(), + std::chrono::duration(parent_token_bucket_->fillInterval()).count()); + per_descriptor_token_bucket = std::make_shared( + parent_token_bucket_->maxTokens(), + uint32_t(parent_token_bucket_->fillRate() * + std::chrono::duration(parent_token_bucket_->fillInterval()).count()), + parent_token_bucket_->fillInterval(), time_source_); ENVOY_LOG(trace, "DynamicDescriptor::addorGetDescriptor: adding dynamic descriptor: {}", request_descriptor.toString()); diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 224619c0baec9..d385328de7a80 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -27,8 +27,7 @@ using ProtoLocalClusterRateLimit = envoy::extensions::common::ratelimit::v3::Loc class DynamicDescriptor : public Logger::Loggable { public: - DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, TimeSource&, - LocalRateLimiterImpl& parent); + DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, TimeSource&); ~DynamicDescriptor() = default; // add a new user configured descriptor to the set. RateLimitTokenBucketSharedPtr addOrGetDescriptor(const RateLimit::Descriptor& request_descriptor); @@ -45,8 +44,6 @@ class DynamicDescriptor : public Logger::Loggable LruList lru_list_; uint32_t lru_size_; TimeSource& time_source_; - const bool no_timer_based_rate_limit_token_bucket_{}; - LocalRateLimiterImpl& parent_; }; using DynamicDescriptorSharedPtr = std::shared_ptr; From 25aa2ab0c0a0483ff390696d553c5c34b97a84e8 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sat, 18 Jan 2025 15:33:53 +0530 Subject: [PATCH 18/41] cleanup Signed-off-by: Vikas Choudhary (vikasc) --- .../common/local_ratelimit/local_ratelimit_impl.cc | 14 -------------- .../common/local_ratelimit/local_ratelimit_impl.h | 2 -- 2 files changed, 16 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 194fb558fd25d..19eeaccd6ad43 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -291,7 +291,6 @@ void LocalRateLimiterImpl::onFillTimer() { for (const auto& descriptor : descriptors_) { descriptor.second->onFillTimer(refill_counter_, share_factor); } - dynamic_descriptors_.onFillTimer(refill_counter_, share_factor); fill_timer_->enableTimer(default_token_bucket_->fillInterval()); } @@ -426,12 +425,6 @@ DynamicDescriptorMap::getBucket(const RateLimit::Descriptor request_descriptor) return nullptr; } -void DynamicDescriptorMap::onFillTimer(uint64_t refill_counter, double factor) { - for (const auto& pair : user_descriptors_) { - pair.second->onFillTimer(refill_counter, factor); - } -} - DynamicDescriptor::DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, TimeSource& time_source) : parent_token_bucket_(token_bucket), lru_size_(lru_size), time_source_(time_source) {} @@ -475,13 +468,6 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descr return result.first->second.first; } -void DynamicDescriptor::onFillTimer(uint64_t refill_counter, double factor) { - absl::WriterMutexLock lock(&dyn_desc_lock_); - for (auto& pair : dynamic_descriptors_) { - pair.second.first->onFillTimer(refill_counter, factor); - } -} - } // namespace LocalRateLimit } // namespace Common } // namespace Filters diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index d385328de7a80..265148c1a22f9 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -31,7 +31,6 @@ class DynamicDescriptor : public Logger::Loggable ~DynamicDescriptor() = default; // add a new user configured descriptor to the set. RateLimitTokenBucketSharedPtr addOrGetDescriptor(const RateLimit::Descriptor& request_descriptor); - void onFillTimer(uint64_t refill_counter, double factor); private: RateLimitTokenBucketSharedPtr parent_token_bucket_; @@ -57,7 +56,6 @@ class DynamicDescriptorMap : public Logger::Loggable& request_entries, From eb50e8510992be0649d178668cd8a33823b0d915 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sun, 19 Jan 2025 08:40:05 +0530 Subject: [PATCH 19/41] cleanup and more tests Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/v3/local_rate_limit.proto | 2 +- .../local_ratelimit/local_ratelimit_impl.cc | 16 +++--- .../local_ratelimit/local_ratelimit_impl.h | 13 +---- .../local_ratelimit/local_ratelimit_test.cc | 57 +++++++++++++++++++ .../local_ratelimit_integration_test.cc | 57 ++++++++++--------- 5 files changed, 100 insertions(+), 45 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index d426d97893aae..c08c56dafca52 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -174,6 +174,6 @@ message LocalRateLimit { // with blank value entries, then max dynamic descriptors stored in the LRU cache will be 2 * max_dynamic_descriptors. // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted // values. - // Default is 20. + // Minimum is 1. Default is 20. google.protobuf.UInt32Value max_dynamic_descriptors = 18; } diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 19eeaccd6ad43..250b639e3a8f4 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -218,6 +218,10 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( "no_timer_based_rate_limit_token_bucket"); } + if (lru_size == 0) { + throw EnvoyException("minimum allowed value for max_dynamic_descriptors is 1"); + } + if (per_connection) { throw EnvoyException( "local rate descriptor value cannot be empty in per connection rate limit mode"); @@ -367,7 +371,7 @@ LocalRateLimiterImpl::requestAllowed(absl::Span req // Compare the request descriptor entries with the user descriptor entries. If all non-empty user // descriptor values match the request descriptor values, return true -bool DynamicDescriptorMap::compareDescriptorEntries( +bool DynamicDescriptorMap::matchDescriptorEntries( const std::vector& request_entries, const std::vector& user_entries) { // Check for equality of sizes @@ -411,11 +415,8 @@ DynamicDescriptorMap::getBucket(const RateLimit::Descriptor request_descriptor) if (user_descriptor.entries_.size() != request_descriptor.entries_.size()) { continue; } - bool wildcard_found = false; - wildcard_found = - compareDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_); - if (!wildcard_found) { + if (!matchDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_)) { continue; } @@ -456,7 +457,8 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descr lru_list_.emplace_front(request_descriptor); auto result = dynamic_descriptors_.emplace( request_descriptor, std::pair(per_descriptor_token_bucket, lru_list_.begin())); - if (lru_list_.size() >= lru_size_) { + auto token_bucket = result.first->second.first; + if (lru_list_.size() > lru_size_) { ENVOY_LOG(trace, "DynamicDescriptor::addorGetDescriptor: lru_size({}) overflow. Removing dynamic " "descriptor: {}", @@ -465,7 +467,7 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descr lru_list_.pop_back(); } ASSERT(lru_list_.size() == dynamic_descriptors_.size()); - return result.first->second.first; + return token_bucket; } } // namespace LocalRateLimit diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 265148c1a22f9..4cb83b9ba20a3 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -28,7 +28,6 @@ using ProtoLocalClusterRateLimit = envoy::extensions::common::ratelimit::v3::Loc class DynamicDescriptor : public Logger::Loggable { public: DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, TimeSource&); - ~DynamicDescriptor() = default; // add a new user configured descriptor to the set. RateLimitTokenBucketSharedPtr addOrGetDescriptor(const RateLimit::Descriptor& request_descriptor); @@ -49,8 +48,6 @@ using DynamicDescriptorSharedPtr = std::shared_ptr; class DynamicDescriptorMap : public Logger::Loggable { public: - DynamicDescriptorMap() = default; - ~DynamicDescriptorMap() = default; // add a new user configured descriptor to the set. void addDescriptor(const RateLimit::LocalDescriptor& descriptor, DynamicDescriptorSharedPtr dynamic_descriptor); @@ -58,8 +55,8 @@ class DynamicDescriptorMap : public Logger::Loggable& request_entries, - const std::vector& user_entries); + bool matchDescriptorEntries(const std::vector& request_entries, + const std::vector& user_entries); RateLimit::LocalDescriptor::Map user_descriptors_; }; @@ -114,7 +111,6 @@ class RateLimitTokenBucket : public TokenBucketContext { virtual void onFillTimer(uint64_t refill_counter, double factor = 1.0) PURE; virtual std::chrono::milliseconds fillInterval() const PURE; virtual double fillRate() const PURE; - virtual uint64_t multiplier() const { return 1; } }; // Token bucket that implements based on the periodic timer. @@ -129,7 +125,6 @@ class TimerTokenBucket : public RateLimitTokenBucket { void onFillTimer(uint64_t refill_counter, double factor) override; std::chrono::milliseconds fillInterval() const override { return fill_interval_; } double fillRate() const override { return fill_rate_; } - uint64_t multiplier() const override { return multiplier_; } uint64_t maxTokens() const override { return max_tokens_; } uint64_t remainingTokens() const override { return tokens_.load(); } absl::optional remainingFillInterval() const override; @@ -192,10 +187,6 @@ class LocalRateLimiterImpl : public Logger::Loggable& request_entries, - const std::vector& user_entries, - std::vector& new_descriptor_entries); - RateLimitTokenBucketSharedPtr default_token_bucket_; const Event::TimerPtr fill_timer_; diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index c83b8b5f735ea..ac2ed3fcdc443 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -391,6 +391,15 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { fill_interval: {} )"; + static constexpr absl::string_view wildcard_descriptor_config_yaml = R"( + entries: + - key: foo + token_bucket: + max_tokens: {} + tokens_per_fill: {} + fill_interval: {} + )"; + static constexpr absl::string_view multiple_descriptor_config_yaml = R"( entries: - key: hello @@ -408,6 +417,54 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { std::vector descriptor2_{{{{"hello", "world"}, {"foo", "bar"}}}}; }; +// Make sure blank values are not allowed if the dynamic bucket feature is disabled. +TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketNotEnabled) { + TestUtility::loadFromYaml( + fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), + *descriptors_.Add()); + + EXPECT_THROW_WITH_MESSAGE( + LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_), + EnvoyException, + "local_rate_limiting_with_dynamic_buckets is disabled. Local rate descriptor value cannot be " + "empty"); +} + +// Make sure no_timer_based_rate_limit_token_bucket is enabled if the dynamic bucket feature is +// enabled. +TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledbutNoTimerBasedTokenBucketDisabled) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "false"}, + {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml( + fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), + *descriptors_.Add()); + + EXPECT_THROW_WITH_MESSAGE( + LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_), + EnvoyException, + "local rate limit descriptor value cannot be empty without " + "no_timer_based_rate_limit_token_bucket"); +} + +// Make sure valid max_dynamic_descriptors is required if the dynamic bucket feature is enabled. +TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledbutInvalidMaxDynamicDescriptors) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "true"}, + {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml( + fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), + *descriptors_.Add()); + + EXPECT_THROW_WITH_MESSAGE(LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, + dispatcher_, descriptors_, true, nullptr, 0), + + EnvoyException, + "minimum allowed value for max_dynamic_descriptors is 1"); +} + // Verify descriptor rate limit time interval is multiple of token bucket fill interval. TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitDivisibleByTokenFillInterval) { TestUtility::loadFromYaml(fmt::format(single_descriptor_config_yaml, 10, 10, "60s"), diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index 2ce0706583658..fa69135a23a96 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -209,6 +209,7 @@ name: envoy.filters.http.local_ratelimit typed_config: "@type": type.googleapis.com/envoy.extensions.filters.http.local_ratelimit.v3.LocalRateLimit stat_prefix: http_local_rate_limiter + max_dynamic_descriptors: {} token_bucket: max_tokens: 2 tokens_per_fill: 1 @@ -380,32 +381,36 @@ TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - initializeFilter(fmt::format(filter_config_with_blank_value_descriptor_, "false")); - // filter is adding dynamic descriptors based on the request header - // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval - // of 1000s which means only one request is allowed per 1000s for each unique value of - // 'x-envoy-downstream-service-cluster' header. - - codec_client_ = makeHttpConnection(lookupPort("http")); - sendAndVerifyRequest("foo", "200", 0); - cleanupUpstreamAndDownstream(); - - // 1 token is exhausted for 'foo' cluster, so the next request with the same cluster should be - // rate limited. - codec_client_ = makeHttpConnection(lookupPort("http")); - sendRateLimitedRequest("foo"); - cleanupUpstreamAndDownstream(); - - // The next request with a different cluster, 'bar', should be allowed. - codec_client_ = makeHttpConnection(lookupPort("http")); - sendAndVerifyRequest("bar", "200", 0); - cleanupUpstreamAndDownstream(); - - // 1 token is exhausted for 'bar' cluster as well, so the next request with the same cluster - // should be rate limited. - codec_client_ = makeHttpConnection(lookupPort("http")); - sendRateLimitedRequest("bar"); - cleanupUpstreamAndDownstream(); + for (int i = 1; i < 2; ++i) { + auto max_dynamic_descriptors = i; + initializeFilter( + fmt::format(filter_config_with_blank_value_descriptor_, max_dynamic_descriptors, "false")); + // filter is adding dynamic descriptors based on the request header + // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval + // of 1000s which means only one request is allowed per 1000s for each unique value of + // 'x-envoy-downstream-service-cluster' header. + + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("foo", "200", 0); + cleanupUpstreamAndDownstream(); + + // 1 token is exhausted for 'foo' cluster, so the next request with the same cluster should be + // rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("foo"); + cleanupUpstreamAndDownstream(); + + // The next request with a different cluster, 'bar', should be allowed. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("bar", "200", 0); + cleanupUpstreamAndDownstream(); + + // 1 token is exhausted for 'bar' cluster as well, so the next request with the same cluster + // should be rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("bar"); + cleanupUpstreamAndDownstream(); + } } TEST_P(LocalRateLimitFilterIntegrationTest, DenyRequestPerProcess) { From 80a770baba5dc28964e7a85c77042b48eafd49d6 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sun, 19 Jan 2025 08:42:04 +0530 Subject: [PATCH 20/41] test fix Signed-off-by: Vikas Choudhary (vikasc) --- .../http/local_ratelimit/local_ratelimit_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index fa69135a23a96..62fab796354dc 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -381,7 +381,7 @@ TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - for (int i = 1; i < 2; ++i) { + for (int i = 1; i < 3; ++i) { auto max_dynamic_descriptors = i; initializeFilter( fmt::format(filter_config_with_blank_value_descriptor_, max_dynamic_descriptors, "false")); From 9a2448e7ab92e914f298b0448a05b8ecf5985ba5 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sun, 19 Jan 2025 08:44:11 +0530 Subject: [PATCH 21/41] fix Signed-off-by: Vikas Choudhary (vikasc) --- .../http/local_ratelimit/local_ratelimit_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index 62fab796354dc..fa69135a23a96 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -381,7 +381,7 @@ TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - for (int i = 1; i < 3; ++i) { + for (int i = 1; i < 2; ++i) { auto max_dynamic_descriptors = i; initializeFilter( fmt::format(filter_config_with_blank_value_descriptor_, max_dynamic_descriptors, "false")); From 04eadbad2189adb017e4969916fd7ed70727744d Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sun, 19 Jan 2025 08:51:57 +0530 Subject: [PATCH 22/41] more test Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit_integration_test.cc | 91 +++++++++++++------ 1 file changed, 61 insertions(+), 30 deletions(-) diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index fa69135a23a96..2e0cdf677bc6a 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -381,36 +381,67 @@ TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - for (int i = 1; i < 2; ++i) { - auto max_dynamic_descriptors = i; - initializeFilter( - fmt::format(filter_config_with_blank_value_descriptor_, max_dynamic_descriptors, "false")); - // filter is adding dynamic descriptors based on the request header - // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval - // of 1000s which means only one request is allowed per 1000s for each unique value of - // 'x-envoy-downstream-service-cluster' header. - - codec_client_ = makeHttpConnection(lookupPort("http")); - sendAndVerifyRequest("foo", "200", 0); - cleanupUpstreamAndDownstream(); - - // 1 token is exhausted for 'foo' cluster, so the next request with the same cluster should be - // rate limited. - codec_client_ = makeHttpConnection(lookupPort("http")); - sendRateLimitedRequest("foo"); - cleanupUpstreamAndDownstream(); - - // The next request with a different cluster, 'bar', should be allowed. - codec_client_ = makeHttpConnection(lookupPort("http")); - sendAndVerifyRequest("bar", "200", 0); - cleanupUpstreamAndDownstream(); - - // 1 token is exhausted for 'bar' cluster as well, so the next request with the same cluster - // should be rate limited. - codec_client_ = makeHttpConnection(lookupPort("http")); - sendRateLimitedRequest("bar"); - cleanupUpstreamAndDownstream(); - } + initializeFilter(fmt::format(filter_config_with_blank_value_descriptor_, 20, "false")); + // filter is adding dynamic descriptors based on the request header + // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval + // of 1000s which means only one request is allowed per 1000s for each unique value of + // 'x-envoy-downstream-service-cluster' header. + + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("foo", "200", 0); + cleanupUpstreamAndDownstream(); + + // 1 token is exhausted for 'foo' cluster, so the next request with the same cluster should be + // rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("foo"); + cleanupUpstreamAndDownstream(); + + // The next request with a different cluster, 'bar', should be allowed. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("bar", "200", 0); + cleanupUpstreamAndDownstream(); + + // 1 token is exhausted for 'bar' cluster as well, so the next request with the same cluster + // should be rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("bar"); + cleanupUpstreamAndDownstream(); +} + +TEST_P(LocalRateLimitFilterIntegrationTest, DesciptorsBasicTestWithMinimumMaxDynamicDescriptors) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + + auto max_dynamic_descriptors = 1; + initializeFilter( + fmt::format(filter_config_with_blank_value_descriptor_, max_dynamic_descriptors, "false")); + // filter is adding dynamic descriptors based on the request header + // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval + // of 1000s which means only one request is allowed per 1000s for each unique value of + // 'x-envoy-downstream-service-cluster' header. + + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("foo", "200", 0); + cleanupUpstreamAndDownstream(); + + // 1 token is exhausted for 'foo' cluster, so the next request with the same cluster should be + // rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("foo"); + cleanupUpstreamAndDownstream(); + + // The next request with a different cluster, 'bar', should be allowed. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendAndVerifyRequest("bar", "200", 0); + cleanupUpstreamAndDownstream(); + + // 1 token is exhausted for 'bar' cluster as well, so the next request with the same cluster + // should be rate limited. + codec_client_ = makeHttpConnection(lookupPort("http")); + sendRateLimitedRequest("bar"); + cleanupUpstreamAndDownstream(); } TEST_P(LocalRateLimitFilterIntegrationTest, DenyRequestPerProcess) { From 210f2696f801816505ed62351950515db19936b9 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sun, 19 Jan 2025 09:26:19 +0530 Subject: [PATCH 23/41] ci trigger Signed-off-by: Vikas Choudhary (vikasc) From 44eefb0c32849fffcddfdbf66d3b7a21af8f5a2b Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Sun, 19 Jan 2025 12:01:14 +0530 Subject: [PATCH 24/41] ci trigger Signed-off-by: Vikas Choudhary (vikasc) From c3b36d7110b295469dabf6ce3c253077910dff98 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 06:31:42 +0530 Subject: [PATCH 25/41] more tests Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/local_ratelimit_test.cc | 100 +++++++++++++++++- 1 file changed, 96 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index ac2ed3fcdc443..20847dd94eb2b 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -376,9 +376,11 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { void initializeWithAtomicTokenBucketDescriptor(const std::chrono::milliseconds fill_interval, const uint32_t max_tokens, - const uint32_t tokens_per_fill) { - rate_limiter_ = std::make_shared( - fill_interval, max_tokens, tokens_per_fill, dispatcher_, descriptors_); + const uint32_t tokens_per_fill, + uint32_t lru_size = 20) { + rate_limiter_ = + std::make_shared(fill_interval, max_tokens, tokens_per_fill, + dispatcher_, descriptors_, true, nullptr, lru_size); } static constexpr absl::string_view single_descriptor_config_yaml = R"( @@ -393,7 +395,7 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { static constexpr absl::string_view wildcard_descriptor_config_yaml = R"( entries: - - key: foo + - key: user token_bucket: max_tokens: {} tokens_per_fill: {} @@ -465,6 +467,96 @@ TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledbutInvalidMaxDynamicDe "minimum allowed value for max_dynamic_descriptors is 1"); } +// Make sure error raised in case values are omitted with per connection rate limiting. +TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledPerConnectionRateLimiting) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "true"}, + {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml( + fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), + *descriptors_.Add()); + + EXPECT_THROW_WITH_MESSAGE( + LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_, true, + nullptr, 1, true), + + EnvoyException, + "local rate descriptor value cannot be empty in per connection rate limit mode"); +} + +// Make sure error raised in case duplicate/replicated descriptors are found. +TEST_F(LocalRateLimiterImplTest, DuplicatedDynamicTokenBucketDescriptor) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "true"}, + {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml( + fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), + *descriptors_.Add()); + TestUtility::loadFromYaml( + fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), + *descriptors_.Add()); + + EXPECT_THROW_WITH_MESSAGE(LocalRateLimiterImpl(std::chrono::milliseconds(60000), 2, 1, + dispatcher_, descriptors_, true, nullptr, 1), + + EnvoyException, + "duplicate descriptor in the local rate descriptor: user="); +} + +// Verify dynamic token bucket lru cache functionality +TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBuckets) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "1s"), + *descriptors_.Add()); + initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 4, 2, 1); + + std::vector descriptors{{{{"user", "A"}}}}; + + // Descriptor from 2 -> 1 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 1 -> 0 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 0 -> 0 tokens + EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors).allowed); + + std::vector descriptors2{{{{"user", "B"}}}}; + // Descriptor from 2 -> 1 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + // Descriptor from 1 -> 0 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + // Descriptor from 0 -> 0 tokens + EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors2).allowed); +} + +TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsMixedRequestOrder) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "100s"), + *descriptors_.Add()); + initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(100000), 4, 2, 2); + + std::vector descriptors{{{{"user", "A"}}}}; + std::vector descriptors2{{{{"user", "B"}}}}; + + // Descriptor from 2 -> 1 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 2 -> 1 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + // Descriptor from 1 -> 0 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 1 -> 0 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + // Descriptor from 0 -> 0 tokens + EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 0 -> 0 tokens + EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors2).allowed); +} + // Verify descriptor rate limit time interval is multiple of token bucket fill interval. TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitDivisibleByTokenFillInterval) { TestUtility::loadFromYaml(fmt::format(single_descriptor_config_yaml, 10, 10, "60s"), From 97455e8a2f5ef5ac512f9beafbd4f4be73fc1b7f Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 06:41:17 +0530 Subject: [PATCH 26/41] kick ci Signed-off-by: Vikas Choudhary (vikasc) From 8c2a558e168fc9bccd419520407db4cf50d03151 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 10:29:05 +0530 Subject: [PATCH 27/41] more tests Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/local_ratelimit_impl.cc | 4 - .../local_ratelimit/local_ratelimit_test.cc | 78 ++++++++++++++++++- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 250b639e3a8f4..a090ea389a27a 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -412,10 +412,6 @@ RateLimitTokenBucketSharedPtr DynamicDescriptorMap::getBucket(const RateLimit::Descriptor request_descriptor) { for (const auto& pair : user_descriptors_) { auto user_descriptor = pair.first; - if (user_descriptor.entries_.size() != request_descriptor.entries_.size()) { - continue; - } - if (!matchDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_)) { continue; } diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 20847dd94eb2b..06e2c95780410 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -402,6 +402,17 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { fill_interval: {} )"; + static constexpr absl::string_view multiple_wildcard_descriptor_config_yaml = R"( + entries: + - key: user + - key: org + value: test + token_bucket: + max_tokens: {} + tokens_per_fill: {} + fill_interval: {} + )"; + static constexpr absl::string_view multiple_descriptor_config_yaml = R"( entries: - key: hello @@ -505,14 +516,14 @@ TEST_F(LocalRateLimiterImplTest, DuplicatedDynamicTokenBucketDescriptor) { "duplicate descriptor in the local rate descriptor: user="); } -// Verify dynamic token bucket lru cache functionality +// Verify dynamic token bucket functionality with a single entry descriptor. TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBuckets) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "1s"), *descriptors_.Add()); - initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 4, 2, 1); + initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 20, 2, 1); std::vector descriptors{{{{"user", "A"}}}}; @@ -530,15 +541,74 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBuckets) { EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); // Descriptor from 0 -> 0 tokens EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors2).allowed); + + // this must not be rate-limited because it will be handled by default bucket which uses + // max_tokens i.e 20 + std::vector extra_entries_descriptor{{{{"user", "C"}, {"key", "value"}}}}; + EXPECT_TRUE(rate_limiter_->requestAllowed(extra_entries_descriptor).allowed); + EXPECT_TRUE(rate_limiter_->requestAllowed(extra_entries_descriptor).allowed); + EXPECT_TRUE(rate_limiter_->requestAllowed(extra_entries_descriptor).allowed); + + // this must not be rate-limited because it will be handled by default bucket which uses + // max_tokens i.e 20 + std::vector different_key_descriptor{{{{"notuser", "A"}}}}; + EXPECT_TRUE(rate_limiter_->requestAllowed(different_key_descriptor).allowed); + EXPECT_TRUE(rate_limiter_->requestAllowed(different_key_descriptor).allowed); + EXPECT_TRUE(rate_limiter_->requestAllowed(different_key_descriptor).allowed); +} + +// Verify dynamic token bucket functionality with multiple entries descriptor. +TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsWilcardWithMultipleEntries) { + TestScopedRuntime runtime; + runtime.mergeValues( + {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); + TestUtility::loadFromYaml(fmt::format(multiple_wildcard_descriptor_config_yaml, 2, 2, "1s"), + *descriptors_.Add()); + initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 20, 2, 1); + + std::vector descriptors{{{{"user", "A"}}}}; + // Descriptor from 2 -> 2 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 2 -> 2 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + // Descriptor from 2 -> 2 tokens this is conformatory test to check if the tokens are not consumed + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); + + // same size entries but non-matching key for wildcard descriptor. Should not be rate-limited. + std::vector descriptors2{{{{"user", "A"}, {"key", "value"}}}}; + // Descriptor from 2 -> 2 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + // Descriptor from 2 -> 2 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + // Descriptor from 2 -> 2 tokens this is conformatory test to check if the tokens are not consumed + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); + + // same size entries but non-wildcard key's values does notmatch. Should not be rate-limited. + std::vector descriptors3{{{{"user", "A"}, {"org", "not-test"}}}}; + // Descriptor from 2 -> 2 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors3).allowed); + // Descriptor from 2 -> 2 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors3).allowed); + // Descriptor from 2 -> 2 tokens this is conformatory test to check if the tokens are not consumed + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors3).allowed); + + // this must be rate-limited because non-wildcard key-value matches and wilcard key matches + std::vector descriptors4{{{{"user", "A"}, {"org", "test"}}}}; + // Descriptor from 2 -> 1 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors4).allowed); + // Descriptor from 1 -> 0 tokens + EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors4).allowed); + // Descriptor from 0 -> 0 tokens + EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors4).allowed); } TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsMixedRequestOrder) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "100s"), + TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "1s"), *descriptors_.Add()); - initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(100000), 4, 2, 2); + initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 4, 2, 2); std::vector descriptors{{{{"user", "A"}}}}; std::vector descriptors2{{{{"user", "B"}}}}; From ff7d24d506ea392a89d1b29bc806d3381fffe3c5 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 10:57:15 +0530 Subject: [PATCH 28/41] format Signed-off-by: Vikas Choudhary (vikasc) --- .../common/local_ratelimit/local_ratelimit_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 06e2c95780410..f831e2adff4cb 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -558,7 +558,7 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBuckets) { } // Verify dynamic token bucket functionality with multiple entries descriptor. -TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsWilcardWithMultipleEntries) { +TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketswildcardWithMultipleEntries) { TestScopedRuntime runtime; runtime.mergeValues( {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); @@ -571,7 +571,7 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsWilcardWithMultipl EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); // Descriptor from 2 -> 2 tokens EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); - // Descriptor from 2 -> 2 tokens this is conformatory test to check if the tokens are not consumed + // Descriptor from 2 -> 2 tokens. This is to check if the tokens are not consumed EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors).allowed); // same size entries but non-matching key for wildcard descriptor. Should not be rate-limited. @@ -580,19 +580,19 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsWilcardWithMultipl EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); // Descriptor from 2 -> 2 tokens EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); - // Descriptor from 2 -> 2 tokens this is conformatory test to check if the tokens are not consumed + // Descriptor from 2 -> 2 tokens. This is to check if the tokens are not consumed EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors2).allowed); - // same size entries but non-wildcard key's values does notmatch. Should not be rate-limited. + // same size entries but non-wildcard key's values does not match. Should not be rate-limited. std::vector descriptors3{{{{"user", "A"}, {"org", "not-test"}}}}; // Descriptor from 2 -> 2 tokens EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors3).allowed); // Descriptor from 2 -> 2 tokens EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors3).allowed); - // Descriptor from 2 -> 2 tokens this is conformatory test to check if the tokens are not consumed + // Descriptor from 2 -> 2 tokens. This is to check if the tokens are not consumed EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors3).allowed); - // this must be rate-limited because non-wildcard key-value matches and wilcard key matches + // this must be rate-limited because non-wildcard key-value matches and wildcard key matches std::vector descriptors4{{{{"user", "A"}, {"org", "test"}}}}; // Descriptor from 2 -> 1 tokens EXPECT_TRUE(rate_limiter_->requestAllowed(descriptors4).allowed); From 78d3be06188c4dc7895ddebe05e136263962c617 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 11:45:46 +0530 Subject: [PATCH 29/41] kick ci Signed-off-by: Vikas Choudhary (vikasc) From 039f45237f590131b2922429e42ccdf3d398493c Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 14:23:13 +0530 Subject: [PATCH 30/41] ci kick Signed-off-by: Vikas Choudhary (vikasc) From d1d70cacac14b016da8d3b2c2b1c7ee3fa56d777 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 14:54:43 +0530 Subject: [PATCH 31/41] ci Signed-off-by: Vikas Choudhary (vikasc) From c7401eb8c0a7fb7e9a8f5d626187a814a13d14dd Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 15:27:31 +0530 Subject: [PATCH 32/41] ci Signed-off-by: Vikas Choudhary (vikasc) From a439c5da066246faa85146c5d2c09a5c755318ab Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 20 Jan 2025 16:14:56 +0530 Subject: [PATCH 33/41] ci kick Signed-off-by: Vikas Choudhary (vikasc) From db7a6662cdcd3585cfef1fba53a5783ae89727b3 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 31 Jan 2025 05:52:01 +0530 Subject: [PATCH 34/41] fix merging issues Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/local_ratelimit_impl.cc | 16 ++++------------ .../local_ratelimit/local_ratelimit_impl.h | 3 ++- .../local_ratelimit/local_ratelimit_test.cc | 12 +----------- 3 files changed, 7 insertions(+), 24 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index c920ea58158a0..c6686637e9c54 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -82,7 +82,7 @@ RateLimitTokenBucket::RateLimitTokenBucket(uint64_t max_tokens, uint64_t tokens_ : token_bucket_(max_tokens, time_source, // Calculate the fill rate in tokens per second. tokens_per_fill / std::chrono::duration(fill_interval).count()), - fill_interval_(fill_interval) {} + fill_interval_(fill_interval) {} bool RateLimitTokenBucket::consume(double factor, uint64_t to_consume) { ASSERT(!(factor <= 0.0 || factor > 1.0)); @@ -146,14 +146,6 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( throw EnvoyException("local rate limit descriptor token bucket fill timer must be >= 50ms"); } - if (per_descriptor_fill_interval.count() % fill_interval.count() != 0) { - throw EnvoyException( - "local rate descriptor limit is not a multiple of token bucket fill timer"); - } - // Save the multiplicative factor to control the descriptor refill frequency. - const auto per_descriptor_multiplier = per_descriptor_fill_interval / fill_interval; - - RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; RateLimitTokenBucketSharedPtr per_descriptor_token_bucket = std::make_shared(per_descriptor_max_tokens, per_descriptor_tokens_per_fill, @@ -229,8 +221,8 @@ LocalRateLimiterImpl::requestAllowed(absl::Span req if (matched_results.empty() || always_consume_default_token_bucket_) { if (default_token_bucket_ == nullptr) { return {true, matched_results.empty() - ? makeOptRefFromPtr(nullptr) - : makeOptRef(matched_results[0].token_bucket.get())}; + ? std::shared_ptr(nullptr) + : std::shared_ptr(matched_results[0].token_bucket)}; } ASSERT(default_token_bucket_ != nullptr); @@ -325,7 +317,7 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descr ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", parent_token_bucket_->maxTokens(), parent_token_bucket_->fillRate(), std::chrono::duration(parent_token_bucket_->fillInterval()).count()); - per_descriptor_token_bucket = std::make_shared( + per_descriptor_token_bucket = std::make_shared( parent_token_bucket_->maxTokens(), uint32_t(parent_token_bucket_->fillRate() * std::chrono::duration(parent_token_bucket_->fillInterval()).count()), diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index e9fdcfdcd9b24..b34a061a48769 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -113,6 +113,7 @@ class RateLimitTokenBucket : public TokenBucketContext, // RateLimitTokenBucket bool consume(double factor = 1.0, uint64_t tokens = 1); double fillRate() const { return token_bucket_.fillRate(); } + std::chrono::milliseconds fillInterval() const { return fill_interval_; } uint64_t maxTokens() const override { return static_cast(token_bucket_.maxTokens()); } uint64_t remainingTokens() const override { @@ -121,7 +122,7 @@ class RateLimitTokenBucket : public TokenBucketContext, private: AtomicTokenBucketImpl token_bucket_; - //const std::chrono::milliseconds fill_interval_; + const std::chrono::milliseconds fill_interval_; }; using RateLimitTokenBucketSharedPtr = std::shared_ptr; diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 541fd56e35ef8..60e08fee5ea70 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -375,16 +375,6 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsMixedRequestOrder) EXPECT_FALSE(rate_limiter_->requestAllowed(descriptors2).allowed); } -// Verify descriptor rate limit time interval is multiple of token bucket fill interval. -TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitDivisibleByTokenFillInterval) { - TestUtility::loadFromYaml(fmt::format(single_descriptor_config_yaml, 10, 10, "60s"), - *descriptors_.Add()); - - EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_), - EnvoyException, "local rate descriptor limit is not a multiple of token bucket fill timer"); -} - // Verify descriptor rate limit time with small fill interval is rejected. TEST_F(LocalRateLimiterDescriptorImplTest, DescriptorRateLimitSmallFillInterval) { // Set fill interval to 10 milliseconds. @@ -762,7 +752,7 @@ TEST_F(LocalRateLimiterDescriptorImplTest, NullDefaultTokenBucket) { // Not match any descriptor and default token bucket is null. auto no_match_result = rate_limiter_->requestAllowed(no_match_descriptor_); EXPECT_TRUE(no_match_result.allowed); - EXPECT_FALSE(no_match_result.token_bucket_context.has_value()); + EXPECT_FALSE(no_match_result.token_bucket_context); } } // Namespace LocalRateLimit From 538d5977dfa6b047164679d8c2404316bfa64f5e Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 31 Jan 2025 08:22:46 +0530 Subject: [PATCH 35/41] format Signed-off-by: Vikas Choudhary (vikasc) --- .../filters/common/local_ratelimit/local_ratelimit_impl.cc | 4 ++-- .../filters/common/local_ratelimit/local_ratelimit_impl.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index c6686637e9c54..e6e595b5f519e 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -82,7 +82,7 @@ RateLimitTokenBucket::RateLimitTokenBucket(uint64_t max_tokens, uint64_t tokens_ : token_bucket_(max_tokens, time_source, // Calculate the fill rate in tokens per second. tokens_per_fill / std::chrono::duration(fill_interval).count()), - fill_interval_(fill_interval) {} + fill_interval_(fill_interval) {} bool RateLimitTokenBucket::consume(double factor, uint64_t to_consume) { ASSERT(!(factor <= 0.0 || factor > 1.0)); @@ -146,7 +146,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( throw EnvoyException("local rate limit descriptor token bucket fill timer must be >= 50ms"); } - RateLimitTokenBucketSharedPtr per_descriptor_token_bucket = + RateLimitTokenBucketSharedPtr per_descriptor_token_bucket = std::make_shared(per_descriptor_max_tokens, per_descriptor_tokens_per_fill, per_descriptor_fill_interval, time_source_); diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index b34a061a48769..6d601c3ccf3c1 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -105,7 +105,7 @@ class TokenBucketContext { }; class RateLimitTokenBucket : public TokenBucketContext, - public Logger::Loggable { + public Logger::Loggable { public: RateLimitTokenBucket(uint64_t max_tokens, uint64_t tokens_per_fill, std::chrono::milliseconds fill_interval, TimeSource& time_source); From e46f6810205e4f355c7553d0add7b9ca9e5993b1 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Fri, 31 Jan 2025 08:52:20 +0530 Subject: [PATCH 36/41] ci Signed-off-by: Vikas Choudhary (vikasc) From 605ef6ec5e8b81dc45a1f9f046c7721310d5aed4 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Tue, 4 Feb 2025 04:37:36 +0530 Subject: [PATCH 37/41] Address review comments Signed-off-by: Vikas Choudhary (vikasc) --- .../local_ratelimit/v3/local_rate_limit.proto | 2 +- changelogs/current.yaml | 4 +- source/common/common/logger.h | 1 + source/common/runtime/runtime_features.cc | 3 - .../local_ratelimit/local_ratelimit_impl.cc | 64 ++++++++----------- .../local_ratelimit/local_ratelimit_impl.h | 9 ++- .../local_ratelimit/local_ratelimit_test.cc | 47 -------------- .../local_ratelimit_integration_test.cc | 7 -- 8 files changed, 37 insertions(+), 100 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index c08c56dafca52..b0199c04b7263 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -175,5 +175,5 @@ message LocalRateLimit { // Actual number of dynamic descriptors will depend on the cardinality of unique values received from the http request for the omitted // values. // Minimum is 1. Default is 20. - google.protobuf.UInt32Value max_dynamic_descriptors = 18; + google.protobuf.UInt32Value max_dynamic_descriptors = 18 [(validate.rules).uint32 = {gte: 1}]; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 62593c8c3d1f5..3373c901cda09 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -127,5 +127,7 @@ new_features: change: | Reporting a locality_stats to LRS server when ``rq_issued > 0``, disable by setting runtime guard ``envoy.reloadable_features.report_load_with_rq_issued`` to ``false``. - +- area: local_rate_limit + change: | + Added support for dynamic token buckets in local rate limit filter for http requests. deprecated: diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 761da45a97ca3..8db5f52bc52b1 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -71,6 +71,7 @@ const static bool should_log = true; FUNCTION(kafka) \ FUNCTION(key_value_store) \ FUNCTION(lua) \ + FUNCTION(local_rate_limit) \ FUNCTION(main) \ FUNCTION(matcher) \ FUNCTION(misc) \ diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 05f9469ff15a9..04c0d7a8fb9d0 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -162,9 +162,6 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13); // before downstream. FALSE_RUNTIME_GUARD(envoy_reloadable_features_allow_multiplexed_upstream_half_close); -// A flag to enable the usage of dynamic buckets for local rate limiting. For example, dynamically -// created token buckets for each unique value of a request header. -FALSE_RUNTIME_GUARD(envoy_reloadable_features_local_rate_limiting_with_dynamic_buckets); // TODO(renjietang): Flip to true after prod testing. FALSE_RUNTIME_GUARD(envoy_reloadable_features_use_network_type_socket_option); // TODO(fredyw): Remove after prod testing. diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index e6e595b5f519e..959a0950d6c5d 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -115,16 +115,6 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( new_descriptor.entries_.reserve(descriptor.entries_size()); for (const auto& entry : descriptor.entries()) { if (entry.value().empty()) { - if (!(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets"))) { - throw EnvoyException("local_rate_limiting_with_dynamic_buckets is disabled. Local rate " - "descriptor value cannot be empty"); - } - - if (lru_size == 0) { - throw EnvoyException("minimum allowed value for max_dynamic_descriptors is 1"); - } - if (per_connection) { throw EnvoyException( "local rate descriptor value cannot be empty in per connection rate limit mode"); @@ -146,16 +136,17 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( throw EnvoyException("local rate limit descriptor token bucket fill timer must be >= 50ms"); } - RateLimitTokenBucketSharedPtr per_descriptor_token_bucket = - std::make_shared(per_descriptor_max_tokens, - per_descriptor_tokens_per_fill, - per_descriptor_fill_interval, time_source_); if (wildcard_found) { DynamicDescriptorSharedPtr dynamic_descriptor = std::make_shared( - per_descriptor_token_bucket, lru_size, dispatcher.timeSource()); + per_descriptor_max_tokens, per_descriptor_tokens_per_fill, per_descriptor_fill_interval, + lru_size, dispatcher.timeSource()); dynamic_descriptors_.addDescriptor(std::move(new_descriptor), std::move(dynamic_descriptor)); continue; } + RateLimitTokenBucketSharedPtr per_descriptor_token_bucket = + std::make_shared(per_descriptor_max_tokens, + per_descriptor_tokens_per_fill, + per_descriptor_fill_interval, time_source_); auto result = descriptors_.emplace(std::move(new_descriptor), std::move(per_descriptor_token_bucket)); if (!result.second) { @@ -248,30 +239,27 @@ LocalRateLimiterImpl::requestAllowed(absl::Span req // descriptor values match the request descriptor values, return true bool DynamicDescriptorMap::matchDescriptorEntries( const std::vector& request_entries, - const std::vector& user_entries) { + const std::vector& config_entries) { // Check for equality of sizes - if (request_entries.size() != user_entries.size()) { + if (request_entries.size() != config_entries.size()) { return false; } - bool has_empty_value = false; for (size_t i = 0; i < request_entries.size(); ++i) { - // Check if the keys are equal - if (request_entries[i].key_ != user_entries[i].key_) { + // Check if the keys are equal. + if (request_entries[i].key_ != config_entries[i].key_) { return false; } - // all non-blank user values must match the request values - if (!user_entries[i].value_.empty() && user_entries[i].value_ != request_entries[i].value_) { - return false; + // Check values are equal or wildcard value is used. + if (config_entries[i].value_.empty()) { + continue; } - - // Check for empty value in user entries - if (user_entries[i].value_.empty()) { - has_empty_value = true; + if (request_entries[i].value_ != config_entries[i].value_) { + return false; } } - return has_empty_value; + return true; } void DynamicDescriptorMap::addDescriptor(const RateLimit::LocalDescriptor& user_descriptor, @@ -297,9 +285,13 @@ DynamicDescriptorMap::getBucket(const RateLimit::Descriptor request_descriptor) return nullptr; } -DynamicDescriptor::DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, - TimeSource& time_source) - : parent_token_bucket_(token_bucket), lru_size_(lru_size), time_source_(time_source) {} +DynamicDescriptor::DynamicDescriptor(uint64_t per_descriptor_max_tokens, + uint64_t per_descriptor_tokens_per_fill, + std::chrono::milliseconds per_descriptor_fill_interval, + uint32_t lru_size, TimeSource& time_source) + : max_tokens_(per_descriptor_max_tokens), tokens_per_fill_(per_descriptor_tokens_per_fill), + fill_interval_(per_descriptor_fill_interval), lru_size_(lru_size), time_source_(time_source) { +} RateLimitTokenBucketSharedPtr DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descriptor) { @@ -314,14 +306,10 @@ DynamicDescriptor::addOrGetDescriptor(const RateLimit::Descriptor& request_descr // add a new descriptor to the set along with its token bucket RateLimitTokenBucketSharedPtr per_descriptor_token_bucket; ENVOY_LOG(trace, "creating atomic token bucket for dynamic descriptor"); - ENVOY_LOG(trace, "max_tokens: {}, fill_rate: {}, fill_interval: {}", - parent_token_bucket_->maxTokens(), parent_token_bucket_->fillRate(), - std::chrono::duration(parent_token_bucket_->fillInterval()).count()); + ENVOY_LOG(trace, "max_tokens: {}, tokens_per_fill: {}, fill_interval: {}", max_tokens_, + tokens_per_fill_, std::chrono::duration(fill_interval_).count()); per_descriptor_token_bucket = std::make_shared( - parent_token_bucket_->maxTokens(), - uint32_t(parent_token_bucket_->fillRate() * - std::chrono::duration(parent_token_bucket_->fillInterval()).count()), - parent_token_bucket_->fillInterval(), time_source_); + max_tokens_, tokens_per_fill_, fill_interval_, time_source_); ENVOY_LOG(trace, "DynamicDescriptor::addorGetDescriptor: adding dynamic descriptor: {}", request_descriptor.toString()); diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 6d601c3ccf3c1..27a28aed0ecf9 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -27,18 +27,21 @@ using ProtoLocalClusterRateLimit = envoy::extensions::common::ratelimit::v3::Loc class DynamicDescriptor : public Logger::Loggable { public: - DynamicDescriptor(RateLimitTokenBucketSharedPtr token_bucket, uint32_t lru_size, TimeSource&); + DynamicDescriptor(uint64_t max_tokens, uint64_t tokens_per_fill, + std::chrono::milliseconds fill_interval, uint32_t lru_size, TimeSource&); // add a new user configured descriptor to the set. RateLimitTokenBucketSharedPtr addOrGetDescriptor(const RateLimit::Descriptor& request_descriptor); private: - RateLimitTokenBucketSharedPtr parent_token_bucket_; using LruList = std::list; mutable absl::Mutex dyn_desc_lock_; RateLimit::Descriptor::Map> dynamic_descriptors_ ABSL_GUARDED_BY(dyn_desc_lock_); + uint64_t max_tokens_; + uint64_t tokens_per_fill_; + const std::chrono::milliseconds fill_interval_; LruList lru_list_; uint32_t lru_size_; TimeSource& time_source_; @@ -105,7 +108,7 @@ class TokenBucketContext { }; class RateLimitTokenBucket : public TokenBucketContext, - public Logger::Loggable { + public Logger::Loggable { public: RateLimitTokenBucket(uint64_t max_tokens, uint64_t tokens_per_fill, std::chrono::milliseconds fill_interval, TimeSource& time_source); diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 60e08fee5ea70..415192ce51584 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -196,42 +196,8 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { std::vector no_match_descriptor_{{{{"no_match", "no_match"}}}}; }; -// Make sure blank values are not allowed if the dynamic bucket feature is disabled. -TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketNotEnabled) { - TestUtility::loadFromYaml( - fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), - *descriptors_.Add()); - - EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_), - EnvoyException, - "local_rate_limiting_with_dynamic_buckets is disabled. Local rate descriptor value cannot be " - "empty"); -} - -// Make sure valid max_dynamic_descriptors is required if the dynamic bucket feature is enabled. -TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledbutInvalidMaxDynamicDescriptors) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "true"}, - {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - TestUtility::loadFromYaml( - fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), - *descriptors_.Add()); - - EXPECT_THROW_WITH_MESSAGE(LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, - dispatcher_, descriptors_, true, nullptr, 0), - - EnvoyException, - "minimum allowed value for max_dynamic_descriptors is 1"); -} - // Make sure error raised in case values are omitted with per connection rate limiting. TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledPerConnectionRateLimiting) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "true"}, - {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); TestUtility::loadFromYaml( fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), *descriptors_.Add()); @@ -246,10 +212,6 @@ TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledPerConnectionRateLimit // Make sure error raised in case duplicate/replicated descriptors are found. TEST_F(LocalRateLimiterImplTest, DuplicatedDynamicTokenBucketDescriptor) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.no_timer_based_rate_limit_token_bucket", "true"}, - {"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); TestUtility::loadFromYaml( fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), *descriptors_.Add()); @@ -266,9 +228,6 @@ TEST_F(LocalRateLimiterImplTest, DuplicatedDynamicTokenBucketDescriptor) { // Verify dynamic token bucket functionality with a single entry descriptor. TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBuckets) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "1s"), *descriptors_.Add()); initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 20, 2, 1); @@ -307,9 +266,6 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBuckets) { // Verify dynamic token bucket functionality with multiple entries descriptor. TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketswildcardWithMultipleEntries) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); TestUtility::loadFromYaml(fmt::format(multiple_wildcard_descriptor_config_yaml, 2, 2, "1s"), *descriptors_.Add()); initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 20, 2, 1); @@ -351,9 +307,6 @@ TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketswildcardWithMultip } TEST_F(LocalRateLimiterDescriptorImplTest, DynamicTokenBucketsMixedRequestOrder) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); TestUtility::loadFromYaml(fmt::format(wildcard_descriptor_config_yaml, 2, 2, "1s"), *descriptors_.Add()); initializeWithAtomicTokenBucketDescriptor(std::chrono::milliseconds(50), 4, 2, 2); diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index df4db9f28a832..d3fa29ea12e09 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -378,9 +378,6 @@ INSTANTIATE_TEST_SUITE_P( HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); initializeFilter(fmt::format(filter_config_with_blank_value_descriptor_, 20, "false")); // filter is adding dynamic descriptors based on the request header // 'x-envoy-downstream-service-cluster' and the token bucket is set to 1 token per fill interval @@ -410,10 +407,6 @@ TEST_P(LocalRateLimitFilterIntegrationTest, DynamicDesciptorsBasicTest) { } TEST_P(LocalRateLimitFilterIntegrationTest, DesciptorsBasicTestWithMinimumMaxDynamicDescriptors) { - TestScopedRuntime runtime; - runtime.mergeValues( - {{"envoy.reloadable_features.local_rate_limiting_with_dynamic_buckets", "true"}}); - auto max_dynamic_descriptors = 1; initializeFilter( fmt::format(filter_config_with_blank_value_descriptor_, max_dynamic_descriptors, "false")); From 5527817cf9255b3f8a2251e63107e45b7e589bf0 Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Tue, 4 Feb 2025 05:13:19 +0530 Subject: [PATCH 38/41] PerConnection Signed-off-by: Vikas Choudhary (vikasc) --- .../common/local_ratelimit/local_ratelimit_impl.cc | 6 +----- .../common/local_ratelimit/local_ratelimit_impl.h | 3 +-- .../http/local_ratelimit/local_ratelimit.cc | 13 ++++++------- .../filters/http/local_ratelimit/local_ratelimit.h | 6 ++++-- .../common/local_ratelimit/local_ratelimit_test.cc | 14 -------------- 5 files changed, 12 insertions(+), 30 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 959a0950d6c5d..1c70703c40478 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -96,7 +96,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors, bool always_consume_default_token_bucket, ShareProviderSharedPtr shared_provider, - uint32_t lru_size, bool per_connection) + uint32_t lru_size) : time_source_(dispatcher.timeSource()), share_provider_(std::move(shared_provider)), always_consume_default_token_bucket_(always_consume_default_token_bucket) { // Ignore the default token bucket if fill_interval is 0 because 0 fill_interval means nothing @@ -115,10 +115,6 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( new_descriptor.entries_.reserve(descriptor.entries_size()); for (const auto& entry : descriptor.entries()) { if (entry.value().empty()) { - if (per_connection) { - throw EnvoyException( - "local rate descriptor value cannot be empty in per connection rate limit mode"); - } wildcard_found = true; } new_descriptor.entries_.push_back({entry.key(), entry.value()}); diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index 27a28aed0ecf9..a90eca50d05fa 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -142,8 +142,7 @@ class LocalRateLimiterImpl : public Logger::Loggable& descriptors, bool always_consume_default_token_bucket = true, - ShareProviderSharedPtr shared_provider = nullptr, const uint32_t lru_size = 20, - bool per_connection = false); + ShareProviderSharedPtr shared_provider = nullptr, const uint32_t lru_size = 20); ~LocalRateLimiterImpl(); Result requestAllowed(absl::Span request_descriptors); diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index d882861a53686..9c961af0cd26b 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -30,12 +30,15 @@ FilterConfig::FilterConfig( PROTOBUF_GET_MS_OR_DEFAULT(config.token_bucket(), fill_interval, 0))), max_tokens_(config.token_bucket().max_tokens()), tokens_per_fill_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.token_bucket(), tokens_per_fill, 1)), + max_dynamic_descriptors_( + config.has_max_dynamic_descriptors() ? config.max_dynamic_descriptors().value() : 20), descriptors_(config.descriptors()), rate_limit_per_connection_(config.local_rate_limit_per_downstream_connection()), always_consume_default_token_bucket_( config.has_always_consume_default_token_bucket() ? config.always_consume_default_token_bucket().value() : true), + local_info_(context.localInfo()), runtime_(context.runtime()), filter_enabled_( config.has_filter_enabled() @@ -106,14 +109,10 @@ FilterConfig::FilterConfig( share_provider = share_provider_manager_->getShareProvider(config.local_cluster_rate_limit()); } - uint32_t max_dynamic_descriptors = 20; - if (config.has_max_dynamic_descriptors()) { - max_dynamic_descriptors = config.max_dynamic_descriptors().value(); - } rate_limiter_ = std::make_unique( fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, descriptors_, - always_consume_default_token_bucket_, std::move(share_provider), max_dynamic_descriptors, + always_consume_default_token_bucket_, std::move(share_provider), max_dynamic_descriptors_, config.local_rate_limit_per_downstream_connection()); } @@ -230,8 +229,8 @@ Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionR if (typed_state == nullptr) { auto limiter = std::make_shared( used_config_->fillInterval(), used_config_->maxTokens(), used_config_->tokensPerFill(), - decoder_callbacks_->dispatcher(), used_config_->descriptors(), - used_config_->consumeDefaultTokenBucket()); + used_config_->maxDynamicDescriptors(), decoder_callbacks_->dispatcher(), + used_config_->descriptors(), used_config_->consumeDefaultTokenBucket()); decoder_callbacks_->streamInfo().filterState()->setData( PerConnectionRateLimiter::key(), limiter, StreamInfo::FilterState::StateType::ReadOnly, diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index 22f6befd3a006..303b2798d61ba 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -52,12 +52,12 @@ class PerConnectionRateLimiter : public StreamInfo::FilterState::Object { public: PerConnectionRateLimiter( const std::chrono::milliseconds& fill_interval, uint32_t max_tokens, uint32_t tokens_per_fill, - Envoy::Event::Dispatcher& dispatcher, + uint32_t max_dynamic_descriptors, Envoy::Event::Dispatcher& dispatcher, const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptor, bool always_consume_default_token_bucket) : rate_limiter_(fill_interval, max_tokens, tokens_per_fill, dispatcher, descriptor, - always_consume_default_token_bucket) {} + always_consume_default_token_bucket, nullptr, max_dynamic_descriptors) {} static const std::string& key(); const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& value() const { return rate_limiter_; @@ -99,6 +99,7 @@ class FilterConfig : public Router::RouteSpecificFilterConfig, const std::chrono::milliseconds& fillInterval() const { return fill_interval_; } uint32_t maxTokens() const { return max_tokens_; } uint32_t tokensPerFill() const { return tokens_per_fill_; } + uint32_t maxDynamicDescriptors() const { return max_dynamic_descriptors_; } const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors() const { @@ -145,6 +146,7 @@ class FilterConfig : public Router::RouteSpecificFilterConfig, const std::chrono::milliseconds fill_interval_; const uint32_t max_tokens_; const uint32_t tokens_per_fill_; + const uint32_t max_dynamic_descriptors_; const Protobuf::RepeatedPtrField< envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor> descriptors_; diff --git a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc index 415192ce51584..637ee43e6ca6a 100644 --- a/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc +++ b/test/extensions/filters/common/local_ratelimit/local_ratelimit_test.cc @@ -196,20 +196,6 @@ class LocalRateLimiterDescriptorImplTest : public LocalRateLimiterImplTest { std::vector no_match_descriptor_{{{{"no_match", "no_match"}}}}; }; -// Make sure error raised in case values are omitted with per connection rate limiting. -TEST_F(LocalRateLimiterImplTest, DynamicTokenBucketEnabledPerConnectionRateLimiting) { - TestUtility::loadFromYaml( - fmt::format(LocalRateLimiterDescriptorImplTest::wildcard_descriptor_config_yaml, 2, 1, "60s"), - *descriptors_.Add()); - - EXPECT_THROW_WITH_MESSAGE( - LocalRateLimiterImpl(std::chrono::milliseconds(59000), 2, 1, dispatcher_, descriptors_, true, - nullptr, 1, true), - - EnvoyException, - "local rate descriptor value cannot be empty in per connection rate limit mode"); -} - // Make sure error raised in case duplicate/replicated descriptors are found. TEST_F(LocalRateLimiterImplTest, DuplicatedDynamicTokenBucketDescriptor) { TestUtility::loadFromYaml( From 2bff2283a76cb718f898bb2830556fd3cfe0a80c Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Tue, 4 Feb 2025 05:55:55 +0530 Subject: [PATCH 39/41] cleanup Signed-off-by: Vikas Choudhary (vikasc) --- .../extensions/filters/http/local_ratelimit/local_ratelimit.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 9c961af0cd26b..75cce78fbfce7 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -112,8 +112,7 @@ FilterConfig::FilterConfig( rate_limiter_ = std::make_unique( fill_interval_, max_tokens_, tokens_per_fill_, dispatcher_, descriptors_, - always_consume_default_token_bucket_, std::move(share_provider), max_dynamic_descriptors_, - config.local_rate_limit_per_downstream_connection()); + always_consume_default_token_bucket_, std::move(share_provider), max_dynamic_descriptors_); } Filters::Common::LocalRateLimit::LocalRateLimiterImpl::Result From 45496e75b0608a95fcfed2325cb13e896f25b1eb Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Thu, 6 Feb 2025 04:20:05 +0530 Subject: [PATCH 40/41] ci kick Signed-off-by: Vikas Choudhary (vikasc) From eaaf2d469bb91417ff2ab3d75da9e64111e0d67f Mon Sep 17 00:00:00 2001 From: "Vikas Choudhary (vikasc)" Date: Mon, 10 Feb 2025 03:47:02 +0530 Subject: [PATCH 41/41] Address review comments Signed-off-by: Vikas Choudhary (vikasc) --- .../common/local_ratelimit/local_ratelimit_impl.cc | 12 ++++++------ .../common/local_ratelimit/local_ratelimit_impl.h | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc index 1c70703c40478..1ff6b295b0d52 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.cc @@ -258,9 +258,9 @@ bool DynamicDescriptorMap::matchDescriptorEntries( return true; } -void DynamicDescriptorMap::addDescriptor(const RateLimit::LocalDescriptor& user_descriptor, +void DynamicDescriptorMap::addDescriptor(const RateLimit::LocalDescriptor& config_descriptor, DynamicDescriptorSharedPtr dynamic_descriptor) { - auto result = user_descriptors_.emplace(user_descriptor, std::move(dynamic_descriptor)); + auto result = config_descriptors_.emplace(config_descriptor, std::move(dynamic_descriptor)); if (!result.second) { throw EnvoyException(absl::StrCat("duplicate descriptor in the local rate descriptor: ", result.first->first.toString())); @@ -269,13 +269,13 @@ void DynamicDescriptorMap::addDescriptor(const RateLimit::LocalDescriptor& user_ RateLimitTokenBucketSharedPtr DynamicDescriptorMap::getBucket(const RateLimit::Descriptor request_descriptor) { - for (const auto& pair : user_descriptors_) { - auto user_descriptor = pair.first; - if (!matchDescriptorEntries(request_descriptor.entries_, user_descriptor.entries_)) { + for (const auto& pair : config_descriptors_) { + auto config_descriptor = pair.first; + if (!matchDescriptorEntries(request_descriptor.entries_, config_descriptor.entries_)) { continue; } - // we found a user configured wildcard descriptor that matches the request descriptor. + // here is when a user configured wildcard descriptor matches the request descriptor. return pair.second->addOrGetDescriptor(request_descriptor); } return nullptr; diff --git a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h index a90eca50d05fa..cc3339e6580e2 100644 --- a/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h +++ b/source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h @@ -60,7 +60,7 @@ class DynamicDescriptorMap : public Logger::Loggable& request_entries, const std::vector& user_entries); - RateLimit::LocalDescriptor::Map user_descriptors_; + RateLimit::LocalDescriptor::Map config_descriptors_; }; class ShareProvider { @@ -129,7 +129,7 @@ class RateLimitTokenBucket : public TokenBucketContext, }; using RateLimitTokenBucketSharedPtr = std::shared_ptr; -class LocalRateLimiterImpl : public Logger::Loggable { +class LocalRateLimiterImpl : public Logger::Loggable { public: struct Result { bool allowed{};