Per connection local rate limiting #15843
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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] | ||
| message LocalRateLimit { | ||
| // The human readable prefix to use when emitting stats. | ||
| string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; | ||
|
|
@@ -97,4 +97,12 @@ message LocalRateLimit { | |
| // | ||
| // The filter supports a range of 0 - 10 inclusively for stage numbers. | ||
| uint32 stage = 9 [(validate.rules).uint32 = {lte: 10}]; | ||
|
|
||
| // Specifies the scope of the rate limiter's token bucket. | ||
| // If set to false, the token bucket is shared across all worker threads, | ||
|
alyssawilk marked this conversation as resolved.
|
||
| // thus the rate limits are applied per Envoy process. | ||
| // If set to true, a token bucket is allocated for each connection. | ||
| // Thus the rate limits are applied per connection thereby allowing | ||
| // one to rate limit requests on a per connection basis. | ||
| bool local_rate_limit_per_downstream_connection = 11; | ||
|
Contributor
There was a problem hiding this comment. Is it possible for a proxy to have both per-process and per downstream connection rate limits, or just one or the other? Another potential question is if we should consider some additional rate limit criteria in the future like downstream IP or HTTP Cookie. If we expect additional criteria in the future, we may want to make this an enum field.
Contributor
Author
There was a problem hiding this comment. It's definitely one or the other as it stands currently as I'm not sure the added complexity of dealing with potentially conflicting token bucket quotas between the per process and per connection configurations and the precedence rules we'd have to handle buys us much in the way of functionality. We did indeed consider an enum initially but it didn't seem like there were very many realistic use cases mainly because a lot of the toggles that were based on certain request characteristics such as IP, Cookie etc can be handled today by rate limiting on request descriptors ... |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ stat_prefix: test | |
| header: | ||
| key: x-local-ratelimited | ||
| value: 'true' | ||
| local_rate_limit_per_downstream_connection: {} | ||
|
Contributor
There was a problem hiding this comment. Seems like the {} is used in substitutions below. It would be good to add an end-of-line comment explaining the {}. I think that the yaml comment character is #
Contributor
Author
There was a problem hiding this comment. Added an explanation below. |
||
| )"; | ||
|
|
||
| class FilterTest : public testing::Test { | ||
|
|
@@ -58,12 +59,18 @@ class FilterTest : public testing::Test { | |
| testing::Matcher<const envoy::type::v3::FractionalPercent&>(Percent(100)))) | ||
| .WillRepeatedly(testing::Return(enforced)); | ||
|
|
||
| ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(dispatcher_)); | ||
| ON_CALL(decoder_callbacks_2_, dispatcher()).WillByDefault(ReturnRef(dispatcher_)); | ||
|
|
||
| envoy::extensions::filters::http::local_ratelimit::v3::LocalRateLimit config; | ||
| TestUtility::loadFromYaml(yaml, config); | ||
| config_ = std::make_shared<FilterConfig>(config, local_info_, dispatcher_, stats_, runtime_, | ||
| per_route); | ||
| filter_ = std::make_shared<Filter>(config_); | ||
| filter_->setDecoderFilterCallbacks(decoder_callbacks_); | ||
|
|
||
| filter_2_ = std::make_shared<Filter>(config_); | ||
| filter_2_->setDecoderFilterCallbacks(decoder_callbacks_2_); | ||
| } | ||
| void setup(const std::string& yaml, const bool enabled = true, const bool enforced = true) { | ||
| setupPerRoute(yaml, enabled, enforced); | ||
|
|
@@ -78,44 +85,59 @@ class FilterTest : public testing::Test { | |
|
|
||
| Stats::IsolatedStoreImpl stats_; | ||
| testing::NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks_; | ||
| testing::NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks_2_; | ||
| NiceMock<Event::MockDispatcher> dispatcher_; | ||
| NiceMock<Runtime::MockLoader> runtime_; | ||
| NiceMock<LocalInfo::MockLocalInfo> local_info_; | ||
| std::shared_ptr<FilterConfig> config_; | ||
| std::shared_ptr<Filter> filter_; | ||
| std::shared_ptr<Filter> filter_2_; | ||
| }; | ||
|
|
||
| TEST_F(FilterTest, Runtime) { | ||
| setup(fmt::format(config_yaml, "1", "false"), false, false); | ||
| EXPECT_EQ(&runtime_, &(config_->runtime())); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, ToErrorCode) { | ||
| setup(fmt::format(config_yaml, "1", "false"), false, false); | ||
| EXPECT_EQ(Http::Code::BadRequest, toErrorCode(400)); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, Disabled) { | ||
| setup(fmt::format(config_yaml, "1", "false"), false, false); | ||
| auto headers = Http::TestRequestHeaderMapImpl(); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); | ||
| EXPECT_EQ(0U, findCounter("test.http_local_rate_limit.enabled")); | ||
| EXPECT_EQ(0U, findCounter("test.http_local_rate_limit.enforced")); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, RequestOk) { | ||
| setup(fmt::format(config_yaml, "1", "false")); | ||
| auto headers = Http::TestRequestHeaderMapImpl(); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_2_->decodeHeaders(headers, false)); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.enabled")); | ||
| EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.enforced")); | ||
| EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.ok")); | ||
| EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.rate_limited")); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, RequestOkPerConnection) { | ||
| setup(fmt::format(config_yaml, "1", "true")); | ||
| auto headers = Http::TestRequestHeaderMapImpl(); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_2_->decodeHeaders(headers, false)); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.enabled")); | ||
| EXPECT_EQ(0U, findCounter("test.http_local_rate_limit.enforced")); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.ok")); | ||
| EXPECT_EQ(0U, findCounter("test.http_local_rate_limit.rate_limited")); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, RequestRateLimited) { | ||
| setup(fmt::format(config_yaml, "1", "false")); | ||
|
|
||
| EXPECT_CALL(decoder_callbacks_2_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)) | ||
| .WillOnce(Invoke([](Http::Code code, absl::string_view body, | ||
| std::function<void(Http::ResponseHeaderMap & headers)> modify_headers, | ||
| const absl::optional<Grpc::Status::GrpcStatus> grpc_status, | ||
|
|
@@ -136,16 +158,55 @@ TEST_F(FilterTest, RequestRateLimited) { | |
| auto request_headers = Http::TestRequestHeaderMapImpl(); | ||
| auto expected_headers = Http::TestRequestHeaderMapImpl(); | ||
|
|
||
| EXPECT_EQ(request_headers, expected_headers); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, | ||
| filter_2_->decodeHeaders(request_headers, false)); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.enabled")); | ||
| EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.enforced")); | ||
| EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.ok")); | ||
| EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.rate_limited")); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, RequestRateLimitedPerConnection) { | ||
|
Contributor
There was a problem hiding this comment. WDYT of adding an integration test? I think you can crib off of existing tests in ratelimit_integration_test.cc plus some of the h2 tests to make sure if you stick a limit of one stream, that a second stream on that connection won't (immediately) go upstream but a stream on a separate connection will pass through.
Contributor
Author
There was a problem hiding this comment. Sure, I can take a look at adding an integration test for the above scenario. The remaining comments should be addressed and good to go.
Contributor
Author
There was a problem hiding this comment. Ah! After taking a closer look, seems like the tests in ratelimit_integration_test.cc are all targeting the 'rate limiter as an external service' use case and it makes complete sense to have integration tests for that.
Contributor
There was a problem hiding this comment. I think integration tests generally have value - I wasn't convinced setting connection data on the per-request StreamInfo would work as expected (there are no currently filters which do this, but I went ahead and tweaked some existing code to verify it worked for my own satiscation =P) but given the review delay this PR already suffered and lack of local rate limit tests to crib off of, I'm inclined to let it go this once though if you want to do a follow-up you'd totally earn brownie points :-)
Contributor
Author
There was a problem hiding this comment. Brownie points it is then :-) I do have a draft up but I'm fighting the integration test api a bit at the moment, specifically with trying to get it to send multiple requests over the same connection while still ensuring that the first one generates an upstream request over its Thanks for taking a look!! |
||
| setup(fmt::format(config_yaml, "1", "true")); | ||
|
|
||
| EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)) | ||
| .WillOnce(Invoke([](Http::Code code, absl::string_view body, | ||
| std::function<void(Http::ResponseHeaderMap & headers)> modify_headers, | ||
| const absl::optional<Grpc::Status::GrpcStatus> grpc_status, | ||
| absl::string_view details) { | ||
| EXPECT_EQ(Http::Code::TooManyRequests, code); | ||
| EXPECT_EQ("local_rate_limited", body); | ||
|
|
||
| Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; | ||
| modify_headers(response_headers); | ||
| EXPECT_EQ("true", response_headers.get(Http::LowerCaseString("x-test-rate-limit"))[0] | ||
| ->value() | ||
| .getStringView()); | ||
|
|
||
| EXPECT_EQ(grpc_status, absl::nullopt); | ||
| EXPECT_EQ(details, "local_rate_limited"); | ||
| })); | ||
|
|
||
| auto request_headers = Http::TestRequestHeaderMapImpl(); | ||
| auto expected_headers = Http::TestRequestHeaderMapImpl(); | ||
|
|
||
| EXPECT_EQ(request_headers, expected_headers); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, | ||
| filter_->decodeHeaders(request_headers, false)); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_2_->decodeHeaders(request_headers, false)); | ||
| EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, | ||
| filter_2_->decodeHeaders(request_headers, false)); | ||
|
Contributor
There was a problem hiding this comment. A comment explaining the differences between this test and FilterTest.RequestRateLimited may be helpful for future readers. I think this shows that the limit is applied by connection by verifying that filter_2_ allows requests after filter_ hits the rate limit.
Contributor
Author
There was a problem hiding this comment. Added a brief explanation to the test. |
||
| EXPECT_EQ(4U, findCounter("test.http_local_rate_limit.enabled")); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.enforced")); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.ok")); | ||
| EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.rate_limited")); | ||
| } | ||
|
|
||
| TEST_F(FilterTest, RequestRateLimitedButNotEnforced) { | ||
| setup(fmt::format(config_yaml, "0", "false"), true, false); | ||
|
|
||
| EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)).Times(0); | ||
|
|
||
|
|
@@ -181,6 +242,7 @@ stat_prefix: test | |
| header: | ||
| key: x-test-rate-limit | ||
| value: 'true' | ||
| local_rate_limit_per_downstream_connection: true | ||
| descriptors: | ||
| - entries: | ||
| - key: hello | ||
|
|
||
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.
these shouldnt be here - i think they have just been removed