Skip to content
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

Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

Merged
merged 26 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
673a527
hits_addend_dynamic_metadata
EItanya May 16, 2024
79b6103
clang-format
EItanya May 16, 2024
560bbd1
docs
EItanya May 24, 2024
5c123a2
merge commit
EItanya May 24, 2024
b93fed1
Merge branch 'main' of https://github.com/envoyproxy/envoy into hits_…
EItanya May 30, 2024
5c357aa
formatting in changelog
EItanya Jun 3, 2024
77c211f
docs
EItanya Jun 3, 2024
41ea807
changelog merge
EItanya Jun 4, 2024
1eb1b1c
add IS_ENVOY_BUG when hits_addens is not a number value
EItanya Jun 13, 2024
8afebc2
PR comments
EItanya Jun 17, 2024
581d9ab
swap impl to filter_state from dynamic metadata
EItanya Jul 25, 2024
de668ad
update docs
EItanya Jul 25, 2024
a28f00d
Merge branch 'main' into hits_addend_dynamic_metadata
EItanya Jul 25, 2024
e254738
Merge branch 'main' of https://github.com/envoyproxy/envoy into hits_…
EItanya Jul 25, 2024
95ca05b
clang-formatting
EItanya Jul 26, 2024
339234b
Merge branch 'main' into hits_addend_dynamic_metadata
EItanya Jul 26, 2024
449822d
yaml formatting
EItanya Jul 26, 2024
224db24
Merge branch 'hits_addend_dynamic_metadata' of github.com:eitanya/env…
EItanya Jul 26, 2024
4131e40
add well known filter state doc
EItanya Jul 26, 2024
c56320a
test was incorrect
EItanya Jul 26, 2024
9288df9
rename from earlier copy/paste
EItanya Jul 26, 2024
e4d8c8e
Merge branch 'main' of https://github.com/envoyproxy/envoy into hits_…
EItanya Jul 26, 2024
73b73e3
PR comments for coverage, testing, and formatting
EItanya Jul 29, 2024
9e7bc55
move test into anonymous NS
EItanya Jul 29, 2024
20a2109
double backticks
EItanya Jul 29, 2024
19f5e43
Merge branch 'main' into hits_addend_dynamic_metadata
wbpcode Aug 1, 2024
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
2 changes: 2 additions & 0 deletions api/envoy/service/ratelimit/v3/rls.proto
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ message RateLimitRequest {

// Rate limit requests can optionally specify the number of hits a request adds to the matched
// limit. If the value is not set in the message, a request increases the matched limit by 1.
// This value can be modified by setting "envoy.ratelimit:hits_addend" in the dynamic metadata
// for a given request.
Copy link
Member

@wbpcode wbpcode Jun 5, 2024

Choose a reason for hiding this comment

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

In most of cases, we prefer do similar things in the FilterState rather than dynamic metadata (like various sock options in the filter state and other dynamic configuration from filter state)? Is there any reason prefer the dynamic metadata?

cc @mattklein123

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I based this off of envoy_lb style decisions which can be made via DynamicMetadata. I don't have super strong feelings either way, but there are more easier ways to set DynamicMetadata via other filters so I slightly prefer this method.

Copy link
Member

Choose a reason for hiding this comment

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

envoy.lb use the metadata because the subset lb construct the match structure from the endpoint metadata.

Then, in this scenario, I think filter state should be preferred for these typed options.

Copy link
Contributor

Choose a reason for hiding this comment

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

in this case we use dynamic metadata as it allows to feed data in without the source filter being aware that it is for rate limit.
For example, we can use ext-auth server to add the metadata for the rate limit filter.

Copy link
Member

@wbpcode wbpcode Jun 11, 2024

Choose a reason for hiding this comment

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

in this case we use dynamic metadata as it allows to feed data in without the source filter being aware that it is for rate limit. For example, we can use ext-auth server to add the metadata for the rate limit filter.

As far as I know, the ext_authz could only inject the dynamic metadata with namespace envoy.filters.http.ext_authz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use-case is that we want to dynamically increment the hits_addend value based on the contents of the body. We have a filter to process the body and then write the value to the dynamic_metadata. Maybe I'm not understanding the distinction between dynamic_metadata and filter_state. Typically I've used dynamic_metadata to internally pass data between multiple filters, but it seems like there's nuance here I'm missing.

Copy link
Member

Choose a reason for hiding this comment

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

Here are some discussion about the filter state and dynamic metadata #25130

Basically, we prefer to use filter state for non-proto/JSON-like data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I plan to use the UInt32Accessor class. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wbpcode does that sound correct?

Copy link

@austince austince Jul 2, 2024

Choose a reason for hiding this comment

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

We have a filter to process the body and then write the value to the dynamic_metadata

Would this also allow forwarding the call to the downstream service and e.g. taking something off the response header for hits_addend before getting to the rate limiter filter, as described here? envoyproxy/ratelimit#636 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

This value can be overridden by setting dynamic metadata with namespace ``envoy.ratelimit`` and metadata key ``hits_addend`` to the desired value. Invalid number (< 0) or number will be ignored.

uint32 hits_addend = 3;
}

Expand Down
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ new_features:
change: |
Added :ref:`bypass_overload_manager <envoy_v3_api_field_config.listener.v3.Listener.bypass_overload_manager>`
to bypass the overload manager for a listener. When set to true, the listener will not be subject to overload protection.
- area: ratelimit
change: |
Added the ability to modify :ref:`hits_addend <envoy_v3_api_field_service.ratelimit.v3.RateLimitRequest.hits_addend>`
by setting ``envoy.ratelimit.hits_addend`` to the desired value.
Copy link
Member

Choose a reason for hiding this comment

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

by setting dynamic metadata with namespace ``envoy.ratelimit`` and metadata key ``hits_addend`` to the desired value.


deprecated:
- area: tracing
Expand Down
12 changes: 11 additions & 1 deletion source/extensions/filters/http/ratelimit/ratelimit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,21 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers) {
break;
}

double hits_addend = 0;
EItanya marked this conversation as resolved.
Show resolved Hide resolved
const auto& request_metadata = callbacks_->streamInfo().dynamicMetadata().filter_metadata();
const auto filter_it = request_metadata.find("envoy.ratelimit");
if (filter_it != request_metadata.end()) {
const auto field_it = filter_it->second.fields().find("hits_addend");
if (field_it != filter_it->second.fields().end()) {
hits_addend = field_it->second.number_value();
wbpcode marked this conversation as resolved.
Show resolved Hide resolved
}
}

if (!descriptors.empty()) {
state_ = State::Calling;
initiating_call_ = true;
client_->limit(*this, getDomain(), descriptors, callbacks_->activeSpan(),
callbacks_->streamInfo(), 0);
callbacks_->streamInfo(), hits_addend);
initiating_call_ = false;
}
}
Expand Down
48 changes: 48 additions & 0 deletions test/extensions/filters/http/ratelimit/ratelimit_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,54 @@ TEST_F(HttpRateLimitFilterTest, OkResponse) {
1U, filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(ratelimit_ok_).value());
}

TEST_F(HttpRateLimitFilterTest, OkResponseWithAdditionalHitsAddend) {
setUpTest(filter_config_);
InSequence s;

ProtobufWkt::Struct request_struct;
request_struct.mutable_fields()->insert({"hits_addend", ValueUtil::numberValue(5)});
(*filter_callbacks_.stream_info_.metadata_.mutable_filter_metadata())["envoy.ratelimit"] =
request_struct;
EXPECT_CALL(filter_callbacks_.route_->route_entry_.rate_limit_policy_, getApplicableRateLimit(0));

EXPECT_CALL(route_rate_limit_, populateDescriptors(_, _, _, _))
.WillOnce(SetArgReferee<0>(descriptor_));

EXPECT_CALL(filter_callbacks_.route_->route_entry_.virtual_host_.rate_limit_policy_,
getApplicableRateLimit(0));

EXPECT_CALL(*client_, limit(_, "foo",
testing::ContainerEq(std::vector<RateLimit::Descriptor>{
{{{"descriptor_key", "descriptor_value"}}}}),
_, _, 5))
.WillOnce(
WithArgs<0>(Invoke([&](Filters::Common::RateLimit::RequestCallbacks& callbacks) -> void {
request_callbacks_ = &callbacks;
})));

request_headers_.addCopy(Http::Headers::get().RequestId, "requestid");
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, false));
Http::MetadataMap metadata_map{{"metadata", "metadata"}};
EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_->decodeMetadata(metadata_map));
EXPECT_EQ(Http::FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_->decodeTrailers(request_trailers_));
EXPECT_EQ(Http::Filter1xxHeadersStatus::Continue, filter_->encode1xxHeaders(response_headers_));
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers_, false));
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data_, false));
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers_));

EXPECT_CALL(filter_callbacks_, continueDecoding());
EXPECT_CALL(filter_callbacks_.stream_info_,
setResponseFlag(StreamInfo::CoreResponseFlag::RateLimited))
.Times(0);
request_callbacks_->complete(Filters::Common::RateLimit::LimitStatus::OK, nullptr, nullptr,
nullptr, "", nullptr);

EXPECT_EQ(
1U, filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(ratelimit_ok_).value());
}

TEST_F(HttpRateLimitFilterTest, OkResponseWithHeaders) {
setUpTest(filter_config_);
InSequence s;
Expand Down