-
Notifications
You must be signed in to change notification settings - Fork 5.4k
ratelimit: add support for x-ratelimit-* headers in local rate limiting #18157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 48 commits
1940c6c
5482d0d
e5230d9
a71c1ca
b356032
36c31d6
98101be
0fe03e8
612e6f1
47c9f6d
92d18f2
5f26554
20907cc
1eafb85
ba6f194
266e72a
10ba430
39e98c2
bee36ef
5a9b778
e44d0ed
0e2daa5
a458994
56787c8
e3bd3be
8f6d427
a4e76c0
c2216e7
ff59453
e897ad6
d324244
f52ea50
5e3977d
1928a6f
c41acab
0be82da
f4e540b
bda1a88
afe2035
d485bf0
3993927
978ed0a
2bd01c1
a52570b
85c5c3e
d23c9a3
a78997b
67ea8ae
9afce6f
8b043ef
c601b82
6a57280
31c7daa
cdb1471
124f0ef
4b74159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,10 @@ message RateLimit { | |
| "envoy.config.filter.http.rate_limit.v2.RateLimit"; | ||
|
|
||
| // Defines the version of the standard to use for X-RateLimit headers. | ||
| // This field is deprecated in favor of :ref:`XRateLimitHeadersRFCVersion <envoy_v3_api_enum_extensions.common.ratelimit.v3.XRateLimitHeadersRFCVersion>`. | ||
| enum XRateLimitHeadersRFCVersion { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be deprecated. |
||
| option deprecated = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm on the fence whether this should be deprecated while
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, we are going to unify the |
||
|
|
||
| // X-RateLimit headers disabled. | ||
| OFF = 0; | ||
|
|
||
|
|
@@ -100,6 +103,8 @@ message RateLimit { | |
| // the `draft RFC <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_. | ||
| // | ||
| // Disabled by default. | ||
| // | ||
| // [#next-major-version: unify with local ratelimit, should use common.ratelimit.v3.XRateLimitHeadersRFCVersion instead.] | ||
| XRateLimitHeadersRFCVersion enable_x_ratelimit_headers = 8 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that the comment shows
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which line do you mean? I think the code does mean draft 03.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like draft 03 uses "RateLimit-Limit" while draft 00-02 uses "X-RateLimit-Limit". The enum claims draft 03 but the code is prefixed with X-. What's the desired state?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The prefix "X-" is attached since Envoy is a proxy but not a client. A proxy will generally add headers with a prefix "X-". |
||
| [(validate.rules).enum = {defined_only: true}]; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| #include "source/extensions/filters/common/local_ratelimit/local_ratelimit_impl.h" | ||
|
|
||
| #include <chrono> | ||
|
|
||
| #include "source/common/protobuf/utility.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -25,6 +27,7 @@ LocalRateLimiterImpl::LocalRateLimiterImpl( | |
| token_bucket_.tokens_per_fill_ = tokens_per_fill; | ||
| token_bucket_.fill_interval_ = absl::FromChrono(fill_interval); | ||
| tokens_.tokens_ = max_tokens; | ||
| tokens_.fill_time_ = time_source_.monotonicTime(); | ||
|
|
||
| if (fill_timer_) { | ||
| fill_timer_->enableTimer(fill_interval); | ||
|
|
@@ -72,7 +75,7 @@ void LocalRateLimiterImpl::onFillTimer() { | |
| fill_timer_->enableTimer(absl::ToChronoMilliseconds(token_bucket_.fill_interval_)); | ||
| } | ||
|
|
||
| void LocalRateLimiterImpl::onFillTimerHelper(const TokenState& tokens, | ||
| void LocalRateLimiterImpl::onFillTimerHelper(TokenState& tokens, | ||
| const RateLimit::TokenBucket& bucket) { | ||
| // Relaxed consistency is used for all operations because we don't care about ordering, just the | ||
| // final atomic correctness. | ||
|
|
@@ -88,6 +91,9 @@ void LocalRateLimiterImpl::onFillTimerHelper(const TokenState& tokens, | |
| // Loop while the weak CAS fails trying to update the tokens value. | ||
| } while (!tokens.tokens_.compare_exchange_weak(expected_tokens, new_tokens_value, | ||
| std::memory_order_relaxed)); | ||
|
|
||
| // Update fill time at last. | ||
| tokens.fill_time_ = time_source_.monotonicTime(); | ||
| } | ||
|
|
||
| void LocalRateLimiterImpl::onFillTimerDescriptorHelper() { | ||
|
|
@@ -97,7 +103,6 @@ void LocalRateLimiterImpl::onFillTimerDescriptorHelper() { | |
| current_time - descriptor.token_state_->fill_time_) >= | ||
| absl::ToChronoMilliseconds(descriptor.token_bucket_.fill_interval_)) { | ||
| onFillTimerHelper(*descriptor.token_state_, descriptor.token_bucket_); | ||
| descriptor.token_state_->fill_time_ = current_time; | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -123,17 +128,61 @@ bool LocalRateLimiterImpl::requestAllowedHelper(const TokenState& tokens) const | |
| return true; | ||
| } | ||
|
|
||
| bool LocalRateLimiterImpl::requestAllowed( | ||
| StatusOr<std::reference_wrapper<const LocalRateLimiterImpl::LocalDescriptorImpl>> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think perhaps using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I will change it. |
||
| LocalRateLimiterImpl::descriptorHelper( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| if (!descriptors_.empty() && !request_descriptors.empty()) { | ||
| // The override rate limit descriptor is selected by the first full match from the request | ||
| // descriptors. | ||
| for (const auto& request_descriptor : request_descriptors) { | ||
| auto it = descriptors_.find(request_descriptor); | ||
| if (it != descriptors_.end()) { | ||
| return requestAllowedHelper(*it->token_state_); | ||
| return *it; | ||
| } | ||
| } | ||
| } | ||
| return requestAllowedHelper(tokens_); | ||
| return absl::Status(absl::StatusCode::kNotFound, "no descriptor found"); | ||
| } | ||
|
|
||
| bool LocalRateLimiterImpl::requestAllowed( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| auto descriptor = descriptorHelper(request_descriptors); | ||
|
|
||
| return descriptor.ok() ? requestAllowedHelper(*descriptor->get().token_state_) | ||
| : requestAllowedHelper(tokens_); | ||
| } | ||
|
|
||
| uint32_t LocalRateLimiterImpl::maxTokens( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the previous discussion on this maybe we should have these functions be called
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if we need this change because it would make the method names very long.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We tend to favor longer names if it helps readability, but it's probably ok as is |
||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| auto descriptor = descriptorHelper(request_descriptors); | ||
|
|
||
| return descriptor.ok() ? descriptor->get().token_bucket_.max_tokens_ : token_bucket_.max_tokens_; | ||
| } | ||
|
Comment on lines
+146
to
+160
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be finding the max of all the descriptors? Seems like it's finding the max of the first descriptor we know about? Docs might be good here if this is the intended implementation, I'm not that familiar with this code and seeing the impl for a function called
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, it seems that we should return the max token of all matched request descriptors?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, each request will correspond to a certain descriptor, and these descriptors have their own token bucket. So what we need to do here is to find the corresponding token bucket and return its
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. Just for my own learning purpose: My previous comment about max_token was referring to such case. But I could be just wrong as I am not that familiar with code here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NVM. I found the answer:) It might be a good idea to add this comment to the code when you move this pattern into a helper method? in case other people are wondering this as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I will add the comment. |
||
|
|
||
| uint32_t LocalRateLimiterImpl::remainingTokens( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| auto descriptor = descriptorHelper(request_descriptors); | ||
|
|
||
| return descriptor.ok() ? descriptor->get().token_state_->tokens_.load(std::memory_order_relaxed) | ||
| : tokens_.tokens_.load(std::memory_order_relaxed); | ||
| } | ||
|
|
||
| int64_t LocalRateLimiterImpl::remainingFillInterval( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| using namespace std::literals; | ||
|
|
||
| auto current_time = time_source_.monotonicTime(); | ||
| auto descriptor = descriptorHelper(request_descriptors); | ||
| if (descriptor.ok()) { | ||
| ASSERT(std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| current_time - descriptor->get().token_state_->fill_time_) <= | ||
| absl::ToChronoMilliseconds(descriptor->get().token_bucket_.fill_interval_)); | ||
| return absl::ToInt64Seconds( | ||
| descriptor->get().token_bucket_.fill_interval_ - | ||
| absl::Seconds((current_time - descriptor->get().token_state_->fill_time_) / 1s)); | ||
| } | ||
| return absl::ToInt64Seconds(token_bucket_.fill_interval_ - | ||
| absl::Seconds((current_time - tokens_.fill_time_) / 1s)); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some comments would be helpful here, it's a bit hard to parse what's going on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I will add comment here. |
||
|
|
||
| } // namespace LocalRateLimit | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/http/header_map.h" | ||
|
|
||
| #include "source/common/singleton/const_singleton.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace HttpFilters { | ||
| namespace Common { | ||
| namespace RateLimit { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: newline after namespace (would have thought clang-tidy would complain here).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I will run clang-tidy and fix related issues. |
||
|
|
||
| class XRateLimitHeaderValues { | ||
| public: | ||
| const Http::LowerCaseString XRateLimitLimit{"x-ratelimit-limit"}; | ||
| const Http::LowerCaseString XRateLimitRemaining{"x-ratelimit-remaining"}; | ||
| const Http::LowerCaseString XRateLimitReset{"x-ratelimit-reset"}; | ||
|
|
||
| struct { | ||
| const std::string Window{"w"}; | ||
| const std::string Name{"name"}; | ||
| } QuotaPolicyKeys; | ||
| }; | ||
|
|
||
| using XRateLimitHeaders = ConstSingleton<XRateLimitHeaderValues>; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
| } // namespace RateLimit | ||
| } // namespace Common | ||
| } // namespace HttpFilters | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,16 @@ | ||
| #include "source/extensions/filters/http/local_ratelimit/local_ratelimit.h" | ||
|
|
||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "envoy/extensions/common/ratelimit/v3/ratelimit.pb.h" | ||
| #include "envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.pb.h" | ||
| #include "envoy/http/codes.h" | ||
|
|
||
| #include "source/common/http/utility.h" | ||
| #include "source/common/router/config_impl.h" | ||
| #include "source/extensions/filters/http/common/ratelimit_headers.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
|
|
@@ -47,7 +51,9 @@ FilterConfig::FilterConfig( | |
| request_headers_parser_(Envoy::Router::HeaderParser::configure( | ||
| config.request_headers_to_add_when_not_enforced())), | ||
| stage_(static_cast<uint64_t>(config.stage())), | ||
| has_descriptors_(!config.descriptors().empty()) { | ||
| has_descriptors_(!config.descriptors().empty()), | ||
| enable_x_rate_limit_headers_(config.enable_x_ratelimit_headers() == | ||
| envoy::extensions::common::ratelimit::v3::DRAFT_VERSION_03) { | ||
| // 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 | ||
|
|
@@ -63,6 +69,21 @@ bool FilterConfig::requestAllowed( | |
| return rate_limiter_.requestAllowed(request_descriptors); | ||
| } | ||
|
|
||
| uint32_t | ||
| FilterConfig::maxTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| return rate_limiter_.maxTokens(request_descriptors); | ||
| } | ||
|
|
||
| uint32_t FilterConfig::remainingTokens( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| return rate_limiter_.remainingTokens(request_descriptors); | ||
| } | ||
|
|
||
| int64_t FilterConfig::remainingFillInterval( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| return rate_limiter_.remainingFillInterval(request_descriptors); | ||
| } | ||
|
|
||
| LocalRateLimitStats FilterConfig::generateStats(const std::string& prefix, Stats::Scope& scope) { | ||
| const std::string final_prefix = prefix + ".http_local_rate_limit"; | ||
| return {ALL_LOCAL_RATE_LIMIT_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; | ||
|
|
@@ -90,6 +111,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, | |
| populateDescriptors(descriptors, headers); | ||
| } | ||
|
|
||
| // Store descriptors which is used to generate x-ratelimit-* headers in encoding response headers. | ||
| stored_descriptors_ = descriptors; | ||
|
|
||
| if (requestAllowed(descriptors)) { | ||
| config->stats().ok_.inc(); | ||
| return Http::FilterHeadersStatus::Continue; | ||
|
|
@@ -115,13 +139,56 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, | |
| return Http::FilterHeadersStatus::StopIteration; | ||
| } | ||
|
|
||
| Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { | ||
| const auto* config = getConfig(); | ||
|
|
||
| if (config->enabled() && config->enableXRateLimitHeaders()) { | ||
| ASSERT(stored_descriptors_.has_value()); | ||
| auto limit = maxTokens(stored_descriptors_.value()); | ||
| auto remaining = remainingTokens(stored_descriptors_.value()); | ||
| auto reset = remainingFillInterval(stored_descriptors_.value()); | ||
| stored_descriptors_.reset(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to reset this, this whole filter instance gets cleaned up once the stream is done |
||
|
|
||
| headers.addReferenceKey( | ||
| HttpFilters::Common::RateLimit::XRateLimitHeaders::get().XRateLimitLimit, limit); | ||
| headers.addReferenceKey( | ||
| HttpFilters::Common::RateLimit::XRateLimitHeaders::get().XRateLimitRemaining, remaining); | ||
| headers.addReferenceKey( | ||
| HttpFilters::Common::RateLimit::XRateLimitHeaders::get().XRateLimitReset, reset); | ||
| } | ||
|
|
||
| return Http::FilterHeadersStatus::Continue; | ||
| } | ||
|
|
||
| bool Filter::requestAllowed(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) { | ||
| const auto* config = getConfig(); | ||
| return config->rateLimitPerConnection() | ||
| ? getPerConnectionRateLimiter().requestAllowed(request_descriptors) | ||
| : config->requestAllowed(request_descriptors); | ||
| } | ||
|
|
||
| uint32_t Filter::maxTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) { | ||
| const auto* config = getConfig(); | ||
| return config->rateLimitPerConnection() | ||
| ? getPerConnectionRateLimiter().maxTokens(request_descriptors) | ||
| : config->maxTokens(request_descriptors); | ||
| } | ||
|
|
||
| uint32_t Filter::remainingTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) { | ||
| const auto* config = getConfig(); | ||
| return config->rateLimitPerConnection() | ||
| ? getPerConnectionRateLimiter().remainingTokens(request_descriptors) | ||
| : config->remainingTokens(request_descriptors); | ||
| } | ||
|
|
||
| int64_t | ||
| Filter::remainingFillInterval(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) { | ||
| const auto* config = getConfig(); | ||
| return config->rateLimitPerConnection() | ||
| ? getPerConnectionRateLimiter().remainingFillInterval(request_descriptors) | ||
| : config->remainingFillInterval(request_descriptors); | ||
| } | ||
|
|
||
| const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionRateLimiter() { | ||
| const auto* config = getConfig(); | ||
| ASSERT(config->rateLimitPerConnection()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe just
ratelimit_headers_versionorx_ratelimit_headers_version?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will changed to
x_ratelimit_headers_version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts, I think it is worth considering since I want to align with
ratelimit'senable_x_ratelimit_headers. Maybe we can keep the name unchanged?