Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
1940c6c
ratelimit: add support for x-ratelimit-* headers in local rate limiting
zhxie Sep 16, 2021
5482d0d
ratelimit: escape unabled requests from appending headers
zhxie Sep 17, 2021
e5230d9
ratelimit: fix crash on description-based rate limit
zhxie Sep 17, 2021
a71c1ca
test: add test for local rate limiter
zhxie Sep 17, 2021
b356032
ratelimit: escape from crash in test of network filter
zhxie Sep 18, 2021
36c31d6
test: fix build failure
zhxie Sep 18, 2021
98101be
test: fix format
zhxie Sep 18, 2021
0fe03e8
test: add test for x-ratelimit-* headers
zhxie Sep 22, 2021
612e6f1
ratelimit: remove generated_api_shadow
zhxie Sep 23, 2021
47c9f6d
ratelimit: merge x-ratelimit-* headers enum
zhxie Sep 24, 2021
92d18f2
ratelimit: deprecate original x-ratelimit-* headers enum
zhxie Sep 29, 2021
5f26554
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Sep 29, 2021
20907cc
ratelimit: fix docs
zhxie Sep 29, 2021
1eafb85
ratelimit: revert to avoid API change in ratelimit
zhxie Oct 8, 2021
ba6f194
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 8, 2021
266e72a
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 12, 2021
10ba430
docs: add release note
zhxie Oct 12, 2021
39e98c2
docs: fix release note
zhxie Oct 12, 2021
bee36ef
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 13, 2021
5a9b778
docs: fix release notes
zhxie Oct 13, 2021
e44d0ed
ratelimit: fix missing x- in ratelimit headers
zhxie Oct 14, 2021
0e2daa5
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 15, 2021
a458994
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 18, 2021
56787c8
docs: fix release notes
zhxie Oct 18, 2021
e3bd3be
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 20, 2021
8f6d427
ratelimit: consolidate header values
zhxie Oct 21, 2021
a4e76c0
ratelimit: move headers to place without sponsorship required
zhxie Oct 21, 2021
c2216e7
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Oct 25, 2021
ff59453
ratelimit: fix release notes
zhxie Oct 25, 2021
e897ad6
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Nov 5, 2021
d324244
docs: fix release notes
zhxie Nov 5, 2021
f52ea50
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Nov 29, 2021
5e3977d
docs: fix release notes
zhxie Nov 29, 2021
1928a6f
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Jan 19, 2022
c41acab
docs: fix release notes
zhxie Jan 19, 2022
0be82da
nit: move comments and change name
zhxie Jan 19, 2022
f4e540b
ratelimit: return remaining fill interval in int64_t
zhxie Jan 19, 2022
bda1a88
ratelimit: fill time in a single place
zhxie Jan 19, 2022
afe2035
ratelimit: guard headers encoding by checking enabled
zhxie Jan 19, 2022
d485bf0
ratelimit: remove unwanted flag
zhxie Jan 19, 2022
3993927
ratelimit: add a helper method for selecting descriptor
zhxie Jan 20, 2022
978ed0a
ratelimit: store descriptors in filter itself
zhxie Jan 20, 2022
2bd01c1
tests: add a unit test for multiple descriptors
zhxie Jan 20, 2022
a52570b
nit: fix typo and format and remove redundant variables
zhxie Jan 21, 2022
85c5c3e
ratelimit: avoid header copy
zhxie Jan 21, 2022
d23c9a3
ratelimit: update descriptor helper to avoid adding a default descriptor
zhxie Jan 21, 2022
a78997b
tests: switch time system
zhxie Jan 21, 2022
67ea8ae
test: fix time system issue in filter test
zhxie Jan 27, 2022
9afce6f
nit: fix typo
zhxie Jan 30, 2022
8b043ef
ratelimit: replace StatusOr with OptRef and add comment
zhxie Jan 30, 2022
c601b82
nit: remove redundant
zhxie Jan 30, 2022
6a57280
Merge remote-tracking branch 'envoyproxy/main' into header-lrl
zhxie Feb 14, 2022
31c7daa
docs: update release notes
zhxie Feb 14, 2022
cdb1471
nit: remove extra comment slash
zhxie Feb 14, 2022
124f0ef
docs: fix release notes
zhxie Feb 14, 2022
4b74159
api: do not deprecate XRateLimitHeadersRFCVersion
zhxie Feb 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions api/envoy/extensions/common/ratelimit/v3/ratelimit.proto
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: Common rate limit components]

// Defines the version of the standard to use for X-RateLimit headers.
enum XRateLimitHeadersRFCVersion {
// X-RateLimit headers disabled.
OFF = 0;

// Use `draft RFC Version 03 <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.
DRAFT_VERSION_03 = 1;
}

// A RateLimitDescriptor is a list of hierarchical entries that are used by the service to
// determine the final rate limit key and overall allowed limit. Here are some examples of how
// they might be used for the domain "envoy".
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// Local Rate limit :ref:`configuration overview <config_http_filters_local_rate_limit>`.
// [#extension: envoy.filters.http.local_ratelimit]

// [#next-free-field: 12]
// [#next-free-field: 13]
message LocalRateLimit {
// The human readable prefix to use when emitting stats.
string stat_prefix = 1 [(validate.rules).string = {min_len: 1}];
Expand Down Expand Up @@ -106,4 +106,21 @@ message LocalRateLimit {
// one to rate limit requests on a per connection basis.
// If unspecified, the default value is false.
bool local_rate_limit_per_downstream_connection = 11;

// Defines the standard version to use for X-RateLimit headers emitted by the filter:
//
// * ``X-RateLimit-Limit`` - indicates the request-quota associated to the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the code, this header is ratelimit-limit (without the X). Should we update the doc here to remove the X-?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch is in parallel with #12410, so I think the prefix X- is reasonable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, yes, I found I missed the X- in implementations, thanks for your review.

// client in the current time-window followed by the description of the
// quota policy. The value is returned by the maximum tokens of the token bucket.
// * ``X-RateLimit-Remaining`` - indicates the remaining requests in the
// current time-window. The value is returned by the remaining tokens in the token bucket.
// * ``X-RateLimit-Reset`` - indicates the number of seconds until reset of
// the current time-window. The value is returned by the remaining fill interval of the token bucket.
//
// For more information about the headers specification see selected version of
// the `draft RFC <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this explanation of the v3 draft go on the enum, not where the enum is used? If we end up adding more values to the enum then we'd need to change this comment I imagine?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, I will move these comments to the enum.

//
// Disabled by default.
common.ratelimit.v3.XRateLimitHeadersRFCVersion enable_x_ratelimit_headers = 12

Copy link
Copy Markdown
Member

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_version or x_ratelimit_headers_version?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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's enable_x_ratelimit_headers. Maybe we can keep the name unchanged?

[(validate.rules).enum = {defined_only: true}];
}
1 change: 1 addition & 0 deletions api/envoy/extensions/filters/http/ratelimit/v3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ licenses(["notice"]) # Apache 2
api_proto_package(
deps = [
"//envoy/config/ratelimit/v3:pkg",
"//envoy/extensions/common/ratelimit/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
12 changes: 2 additions & 10 deletions api/envoy/extensions/filters/http/ratelimit/v3/rate_limit.proto
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ syntax = "proto3";
package envoy.extensions.filters.http.ratelimit.v3;

import "envoy/config/ratelimit/v3/rls.proto";
import "envoy/extensions/common/ratelimit/v3/ratelimit.proto";

import "google/protobuf/duration.proto";

Expand All @@ -24,15 +25,6 @@ message RateLimit {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.rate_limit.v2.RateLimit";

// Defines the version of the standard to use for X-RateLimit headers.
enum XRateLimitHeadersRFCVersion {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deprecated.

// X-RateLimit headers disabled.
OFF = 0;

// Use `draft RFC Version 03 <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.
DRAFT_VERSION_03 = 1;
}

// The rate limit domain to use when calling the rate limit service.
string domain = 1 [(validate.rules).string = {min_len: 1}];

Expand Down Expand Up @@ -96,7 +88,7 @@ message RateLimit {
// the `draft RFC <https://tools.ietf.org/id/draft-polli-ratelimit-headers-03.html>`_.
//
// Disabled by default.
XRateLimitHeadersRFCVersion enable_x_ratelimit_headers = 8
common.ratelimit.v3.XRateLimitHeadersRFCVersion enable_x_ratelimit_headers = 8

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this breaks backwards compatibility, and therefore the field must be deprecated and a new field with the new type should be introduced.

@htuch extracting the enum to a common place (see: api/envoy/extensions/common/ratelimit/v3/ratelimit.proto) is arguably the right way to go, but will cause some deprecation churn. Another option is to make the local_rate_limit proto use the enum directly from the (global) rate_limit proto. WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the local rate limiting, put it in the common location and use it there. Either deprecate or eave a TODO to cleanup for global. I'm not super keen on API dependencies between extensions, since we're creating an unnecessary dependency in build/protos.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, if we treat local rate limit and rate limit as two independent extensions (like if we can decouple envoy and extensions someday in the future), both local rate limit and rate limit should maintain their own enum values. But if we consider the local rate limit and rate limit are inseparable, extract common enum is a good choice.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take on this is that similar to the original issue (#15293) that requested a local rate-limiting feature support similar to what the global rate-limiting does, then in the future they should be in-sync.
I think that adding the enum to the common proto (as you've already done) is a good thing, and to avoid changes to the global rate-limiting API (following @htuch suggestion), we should not deprecate this enum at this stage, but add a [#next-major-version: comment (see: example here).

[(validate.rules).enum = {defined_only: true}];

// Disables emitting the :ref:`x-envoy-ratelimited<config_http_filters_router_x-envoy-ratelimited>` header
Expand Down
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 {
Expand All @@ -12,7 +14,8 @@ LocalRateLimiterImpl::LocalRateLimiterImpl(
const std::chrono::milliseconds fill_interval, const uint32_t max_tokens,
const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher,
const Protobuf::RepeatedPtrField<
envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors)
envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors,
bool init_fill_time)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you always give init_fill_time = true. Do we need the param in the first place?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in local rate limit network filter, init_fill_time = false. A test of local rate limit network filter will throw an exception for different time system if we initialize fill_time_ in LocalRateLimiterImpl for local rate limit network filter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why this is the case? What is the implication of not initializing the fill time at ctor time?

@tyxia tyxia Jan 18, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this is used for calculating the default per-route RateLimit_Reset which is new header field for HTTP only, I am interested to learn why it fails in network filter? This code seems not to be something HTTP filter specific?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it is a mistake, and I suppose it is because I was not not familiar about the time system in the test before.

There is a assert failure threw by the test shows there is an inconsistent time system. And today I find the problem is only in the test code, not in the source code.

But I cannot fix the test. There is a conflict between the MockDispatcher which uses the GlobalTimeSystem and the MockStreamInfo in MockReadFilterCallbacks which uses the SimulatedTimeSystem. Do you have some ideas?

@tyxia tyxia Jan 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a TODO(#4160) in class GlobalTimeSystem: Switch default testSystem to SimulatedTimeSystem. I guess switching to that system might resolve the issue? I don't have the context on that TODO though.

If that is a lot of work to do or if that doesn't work, a quick workaround off top of my head (just for ideas not necessary to be the final solution): wrapping this line under if condition like below:

if (!descriptors.empty()) {
    tokens_.fill_time_ = time_source_.monotonicTime();
  }

This implicitly limits this code path to local descriptor override case which is only available in HTTP filter.
It is a bit hacky workaround :) but, IMO, it is still better than having bool flag in the interface all over the places.

@zhxie zhxie Jan 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I do not think it might work. In fact, a HTTP filter should load time to fill_time_ while its descriptors are empty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: initialize_fill_time

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

: fill_timer_(fill_interval > std::chrono::milliseconds(0)
? dispatcher.createTimer([this] { onFillTimer(); })
: nullptr),
Expand All @@ -25,6 +28,9 @@ LocalRateLimiterImpl::LocalRateLimiterImpl(
token_bucket_.tokens_per_fill_ = tokens_per_fill;
token_bucket_.fill_interval_ = absl::FromChrono(fill_interval);
tokens_.tokens_ = max_tokens;
if (init_fill_time) {
tokens_.fill_time_ = time_source_.monotonicTime();
}

if (fill_timer_) {
fill_timer_->enableTimer(fill_interval);
Expand Down Expand Up @@ -68,6 +74,7 @@ LocalRateLimiterImpl::~LocalRateLimiterImpl() {

void LocalRateLimiterImpl::onFillTimer() {
onFillTimerHelper(tokens_, token_bucket_);
tokens_.fill_time_ = time_source_.monotonicTime();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to move this line into function onFillTimerHelper for consistency with tokens_ setup.

Then, we can remove descriptor.token_state_->fill_time_ = current_time; from function onFillTimerDescriptorHelper to make this logic happened in a centralized place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is reasonable, I will change it.

onFillTimerDescriptorHelper();
fill_timer_->enableTimer(absl::ToChronoMilliseconds(token_bucket_.fill_interval_));
}
Expand Down Expand Up @@ -136,6 +143,51 @@ bool LocalRateLimiterImpl::requestAllowed(
return requestAllowedHelper(tokens_);
}

uint32_t LocalRateLimiterImpl::maxTokens(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 maxTokensForApplicableDescriptor or something that more clearly conveys what this does?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 {
if (!descriptors_.empty() && !request_descriptors.empty()) {
for (const auto& request_descriptor : request_descriptors) {
auto it = descriptors_.find(request_descriptor);
if (it != descriptors_.end()) {
return it->token_bucket_.max_tokens_;
}
}
}
return token_bucket_.max_tokens_;
}
Comment on lines +146 to +160

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 maxTokens is a bit confusing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 macTokens.

@tyxia tyxia Jan 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Just for my own learning purpose:
You are passing a set of request_descriptors into this function and iterating on it. Is it possible that we can find multiple entries from request_descriptors in descriptors_. i.e., it != descriptors_.end() happens multiple times in the for loop?

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.

@tyxia tyxia Jan 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM. I found the answer:)
From the doc: The override rate limit descriptor is selected by the first full match from the request descriptors.
So this pattern is intended behavior.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 {
if (!descriptors_.empty() && !request_descriptors.empty()) {
for (const auto& request_descriptor : request_descriptors) {
auto it = descriptors_.find(request_descriptor);
if (it != descriptors_.end()) {
return it->token_state_->tokens_.load(std::memory_order_relaxed);
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern of finding the first matching descriptor seems to be the same for these three functions, so maybe a helper to DRY this up would help readability?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will add a general method for finding the descriptor.

return tokens_.tokens_.load(std::memory_order_relaxed);
}

uint32_t LocalRateLimiterImpl::remainingFillInterval(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why you want to cast it to uint32_t here?

It seems quite natural for duration(i.e., not negative) in seconds to be int64_t

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you are right. I will fix it.

absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const {
using namespace std::literals;

if (!descriptors_.empty() && !request_descriptors.empty()) {
for (const auto& request_descriptor : request_descriptors) {
auto it = descriptors_.find(request_descriptor);
if (it != descriptors_.end()) {
return static_cast<uint32_t>(absl::ToInt64Seconds(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some expected bound to the result of ToInt64Seconds(...)? Could it overflow or cause issues with the cast to uint32_t?

@zhxie zhxie Feb 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the return type from uint32_t to int64_t in previous commits, so we do not need the cast anymore.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It might be a good idea to assert like current_time <= fill_time + fill_interval here (if it's easy to do) to enusure the correctness ?

I understand the correctness of this calculation is guaranteed by the check below in onFillTimerDescriptorHelper function:

if (std::chrono::duration_cast<std::chrono::milliseconds>(
            current_time - descriptor.token_state_->fill_time_) >=
        absl::ToChronoMilliseconds(descriptor.token_bucket_.fill_interval_))

but it is seems safer to have assert in case something goes wrong in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will add a simple assert here.

it->token_bucket_.fill_interval_ -
absl::Seconds((time_source_.monotonicTime() - it->token_state_->fill_time_) / 1s)));
}
}
}
return static_cast<uint32_t>(absl::ToInt64Seconds(
token_bucket_.fill_interval_ -
absl::Seconds((time_source_.monotonicTime() - tokens_.fill_time_) / 1s)));
}

} // namespace LocalRateLimit
} // namespace Common
} // namespace Filters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,15 @@ class LocalRateLimiterImpl {
const std::chrono::milliseconds fill_interval, const uint32_t max_tokens,
const uint32_t tokens_per_fill, Event::Dispatcher& dispatcher,
const Protobuf::RepeatedPtrField<
envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors);
envoy::extensions::common::ratelimit::v3::LocalRateLimitDescriptor>& descriptors,
bool init_fill_time);
~LocalRateLimiterImpl();

bool requestAllowed(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const;
uint32_t maxTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const;
uint32_t remainingTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const;
uint32_t
remainingFillInterval(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) const;

private:
struct TokenState {
Expand Down
1 change: 1 addition & 0 deletions source/extensions/filters/http/local_ratelimit/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ envoy_cc_library(
"//source/extensions/filters/common/local_ratelimit:local_ratelimit_lib",
"//source/extensions/filters/common/ratelimit:ratelimit_lib",
"//source/extensions/filters/http/common:pass_through_filter_lib",
"@envoy_api//envoy/extensions/common/ratelimit/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/filters/http/local_ratelimit/v3:pkg_cc_proto",
],
)
Expand Down
110 changes: 108 additions & 2 deletions source/extensions/filters/http/local_ratelimit/local_ratelimit.cc
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
#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"
Expand All @@ -17,6 +20,26 @@ const std::string& PerConnectionRateLimiter::key() {
CONSTRUCT_ON_FIRST_USE(std::string, "per_connection_local_rate_limiter");
}

LocalRateLimitRequestDescriptorsQueue::LocalRateLimitRequestDescriptorsQueue()
: request_descriptors_queue_(
std::queue<absl::optional<std::vector<RateLimit::LocalDescriptor>>>()) {}

void LocalRateLimitRequestDescriptorsQueue::push(
absl::optional<std::vector<RateLimit::LocalDescriptor>> request_descriptors) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Imagine we don't need to have optional here. Do you consider just avoiding push(abls::nullopt)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need an optional here. In any request the filter is not enabled, we will not attach x-ratelimit-* headers.

request_descriptors_queue_.push(request_descriptors);
}

absl::optional<std::vector<RateLimit::LocalDescriptor>>
LocalRateLimitRequestDescriptorsQueue::pop() {
auto request_descriptors = std::move(request_descriptors_queue_.front());

@tyxia tyxia Jan 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using queue here, you should check if queue is empty before retrieving the front() otherwise, front and pop will be undefined behavior if empty

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will add a assert in the corresponding method.

request_descriptors_queue_.pop();
return request_descriptors;
}

const std::string& LocalRateLimitRequestDescriptorsQueue::key() {
CONSTRUCT_ON_FIRST_USE(std::string, "local_rate_limit_requestdescriptors_queue");
}

FilterConfig::FilterConfig(
const envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit& config,
const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Stats::Scope& scope,
Expand All @@ -30,7 +53,7 @@ FilterConfig::FilterConfig(
descriptors_(config.descriptors()),
rate_limit_per_connection_(config.local_rate_limit_per_downstream_connection()),
rate_limiter_(Filters::Common::LocalRateLimit::LocalRateLimiterImpl(
fill_interval_, max_tokens_, tokens_per_fill_, dispatcher, descriptors_)),
fill_interval_, max_tokens_, tokens_per_fill_, dispatcher, descriptors_, true)),
local_info_(local_info), runtime_(runtime),
filter_enabled_(
config.has_filter_enabled()
Expand All @@ -47,7 +70,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
Expand All @@ -63,6 +88,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);
}

uint32_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))};
Expand All @@ -80,6 +120,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
const auto* config = getConfig();

if (!config->enabled()) {
pushRequestDescriptors(absl::nullopt);
return Http::FilterHeadersStatus::Continue;
}

Expand All @@ -89,6 +130,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
if (config->hasDescriptors()) {
populateDescriptors(descriptors, headers);
}
pushRequestDescriptors(descriptors);

if (requestAllowed(descriptors)) {
config->stats().ok_.inc();
Expand All @@ -115,13 +157,77 @@ 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->enableXRateLimitHeaders()) {

@tyxia tyxia Jan 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to also check if (config->enabled()) here?

Also, can we avoid absl::optional vector<LocalDescriptor> completely and push(absl::nullopt) in Filter::decodeHeaders() by having the check above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and it is changed.

auto descriptors = decoder_callbacks_->streamInfo()
.filterState()
->getDataMutable<LocalRateLimitRequestDescriptorsQueue>(
LocalRateLimitRequestDescriptorsQueue::key())
.pop();
if (descriptors.has_value()) {
auto limit = maxTokens(descriptors.value());
auto remaining = remainingTokens(descriptors.value());
auto reset = remainingFillInterval(descriptors.value());

headers.addCopy(Http::LowerCaseString{"ratelimit-limit"}, limit);
headers.addCopy(Http::LowerCaseString{"ratelimit-remaining"}, remaining);
headers.addCopy(Http::LowerCaseString{"ratelimit-reset"}, reset);
}

@esmet esmet Oct 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remote ratelimit filter has XRateLimitHeaderUtils in ratelimit_headers.cc to consolidate header logic. Should we consider moving this to a common file for later code sharing? The hope here is to have shared logic across local_ratelimit and remote ratelimit where possible. What do you / others think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is reasonable. Shall we move source/extensions/filters/http/ratelimit/ratelimit_headers to someplace for code sharing? @adisuissa

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, shared code should be in a common location (refactoring the code, not the API, shouldn't cause an issue).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And there is another question, where shall we move the ratelimit_headers? I prefer to move to a new file source/extensions/common/ratelimit/ratelimit_headers.h. Do you have any opinion? @adisuissa

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adisuissa Any thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes a common file SGTM.
One other thing to consider is making these custom inline headers (see example), but not sure if it needs to be as part of this PR.

}

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

uint32_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);
}

void Filter::pushRequestDescriptors(
absl::optional<std::vector<RateLimit::LocalDescriptor>> request_descriptors) {
if (!decoder_callbacks_->streamInfo()
.filterState()
->hasData<LocalRateLimitRequestDescriptorsQueue>(
LocalRateLimitRequestDescriptorsQueue::key())) {
decoder_callbacks_->streamInfo().filterState()->setData(
LocalRateLimitRequestDescriptorsQueue::key(),
std::make_unique<LocalRateLimitRequestDescriptorsQueue>(),
StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection);
}

decoder_callbacks_->streamInfo()
.filterState()
->getDataMutable<LocalRateLimitRequestDescriptorsQueue>(
LocalRateLimitRequestDescriptorsQueue::key())
.push(request_descriptors);
}

const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionRateLimiter() {
const auto* config = getConfig();
ASSERT(config->rateLimitPerConnection());
Expand Down
Loading