-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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 all 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,6 +29,8 @@ message RateLimit { | |
| "envoy.config.filter.http.rate_limit.v2.RateLimit"; | ||
|
|
||
| // Defines the version of the standard to use for X-RateLimit headers. | ||
| // | ||
| // [#next-major-version: unify with local ratelimit, should use common.ratelimit.v3.XRateLimitHeadersRFCVersion instead.] | ||
| 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. |
||
| // X-RateLimit headers disabled. | ||
| OFF = 0; | ||
|
|
@@ -100,6 +102,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,63 @@ bool LocalRateLimiterImpl::requestAllowedHelper(const TokenState& tokens) const | |
| return true; | ||
| } | ||
|
|
||
| bool LocalRateLimiterImpl::requestAllowed( | ||
| OptRef<const LocalRateLimiterImpl::LocalDescriptorImpl> 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 {}; | ||
| } | ||
|
|
||
| bool LocalRateLimiterImpl::requestAllowed( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| auto descriptor = descriptorHelper(request_descriptors); | ||
|
|
||
| return descriptor.has_value() ? requestAllowedHelper(*descriptor.value().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.has_value() ? descriptor.value().get().token_bucket_.max_tokens_ | ||
| : token_bucket_.max_tokens_; | ||
| } | ||
|
|
||
| uint32_t LocalRateLimiterImpl::remainingTokens( | ||
| absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const { | ||
| auto descriptor = descriptorHelper(request_descriptors); | ||
|
|
||
| return descriptor.has_value() | ||
| ? descriptor.value().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); | ||
| // Remaining time to next fill = fill interval - (current time - last fill time). | ||
| if (descriptor.has_value()) { | ||
| ASSERT(std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| current_time - descriptor.value().get().token_state_->fill_time_) <= | ||
| absl::ToChronoMilliseconds(descriptor.value().get().token_bucket_.fill_interval_)); | ||
| return absl::ToInt64Seconds( | ||
| descriptor.value().get().token_bucket_.fill_interval_ - | ||
| absl::Seconds((current_time - descriptor.value().get().token_state_->fill_time_) / 1s)); | ||
| } | ||
| return absl::ToInt64Seconds(token_bucket_.fill_interval_ - | ||
| absl::Seconds((current_time - tokens_.fill_time_) / 1s)); | ||
| } | ||
|
|
||
| } // 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 | ||
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?