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 23 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 overridden by setting filter state value `envoy.ratelimit.hits_addend`
Copy link
Member

Choose a reason for hiding this comment

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

``envoy.ratelimit.hits_addend``

Please fix the CI, these is a issue that I know. Double backticks should be used.

// to the desired number. Invalid number (< 0) or number will be ignored.
uint32 hits_addend = 3;
}

Expand Down
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,10 @@ new_features:
<envoy_v3_api_field_extensions.transport_sockets.tls.v3.CommonTlsContext.custom_tls_certificate_selector>`
to allow overriding TLS certificate selection behavior.
An extension can select certificate base on the incoming SNI, in both sync and async mode.
- area: ratelimit
change: |
Added the ability to modify :ref:`hits_addend <envoy_v3_api_field_service.ratelimit.v3.RateLimitRequest.hits_addend>`
by setting by setting filter state value ``envoy.ratelimit.hits_addend`` to the desired value.


deprecated:
5 changes: 5 additions & 0 deletions docs/root/configuration/advanced/well_known_filter_state.rst
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ The following lists the filter state object keys used by the Envoy extensions:
<envoy_v3_api_field_extensions.filters.network.tcp_proxy.v3.TcpProxy.idle_timeout>` override on a per-connection
basis. Accepts a count of milliseconds number string as a constructor.

``envoy.ratelimit.hits_addend``
:ref:`Rate Limit Hits Addend
<envoy_v3_api_field_service.ratelimit.v3.RateLimitRequest.hits_addend>` override on a per-route basis.
Accepts a number string as a constructor.

Filter state object fields
--------------------------

Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/http/ratelimit/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ envoy_cc_library(
":ratelimit_headers_lib",
"//envoy/http:codes_interface",
"//envoy/ratelimit:ratelimit_interface",
"//envoy/stream_info:uint32_accessor_interface",
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
"//source/common/common:enum_to_int",
"//source/common/http:codes_lib",
"//source/common/router:config_lib",
"//source/common/stream_info:uint32_accessor_lib",
"//source/extensions/filters/common/ratelimit:ratelimit_client_interface",
"//source/extensions/filters/common/ratelimit:stat_names_lib",
"@envoy_api//envoy/extensions/filters/http/ratelimit/v3:pkg_cc_proto",
Expand Down
32 changes: 31 additions & 1 deletion source/extensions/filters/http/ratelimit/ratelimit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,42 @@
#include <vector>

#include "envoy/http/codes.h"
#include "envoy/stream_info/stream_info.h"

#include "source/common/common/assert.h"
#include "source/common/common/enum_to_int.h"
#include "source/common/common/fmt.h"
#include "source/common/http/codes.h"
#include "source/common/http/header_utility.h"
#include "source/common/router/config_impl.h"
#include "source/common/stream_info/uint32_accessor_impl.h"
#include "source/extensions/filters/http/ratelimit/ratelimit_headers.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace RateLimitFilter {

namespace {
constexpr absl::string_view HitsAddendFilterStateKey = "envoy.ratelimit.hits_addend";
Copy link
Contributor

Choose a reason for hiding this comment

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

@mattklein123 can we make this key configurable per filter?


class HitsAddendObjectFactory : public StreamInfo::FilterState::ObjectFactory {
public:
std::string name() const override { return std::string(HitsAddendFilterStateKey); }
std::unique_ptr<StreamInfo::FilterState::Object>
createFromBytes(absl::string_view data) const override {
uint32_t hits_addend = 0;
if (absl::SimpleAtoi(data, &hits_addend)) {
return std::make_unique<StreamInfo::UInt32AccessorImpl>(hits_addend);
}
return nullptr;
}
};

REGISTER_FACTORY(HitsAddendObjectFactory, StreamInfo::FilterState::ObjectFactory);

} // namespace
EItanya marked this conversation as resolved.
Show resolved Hide resolved

struct RcDetailsValues {
// This request went above the configured limits for the rate limit filter.
const std::string RateLimited = "request_rate_limited";
Expand Down Expand Up @@ -64,11 +86,19 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers) {
break;
}

const StreamInfo::UInt32Accessor* hits_addend_filter_state =
callbacks_->streamInfo().filterState()->getDataReadOnly<StreamInfo::UInt32Accessor>(
HitsAddendFilterStateKey);
double hits_addend = 0;
EItanya marked this conversation as resolved.
Show resolved Hide resolved
if (hits_addend_filter_state != nullptr) {
hits_addend = hits_addend_filter_state->value();
}

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
62 changes: 62 additions & 0 deletions test/extensions/filters/http/ratelimit/ratelimit_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
#include <vector>

#include "envoy/extensions/filters/http/ratelimit/v3/rate_limit.pb.h"
#include "envoy/stream_info/stream_info.h"

#include "source/common/buffer/buffer_impl.h"
#include "source/common/common/empty_string.h"
#include "source/common/http/context_impl.h"
#include "source/common/http/headers.h"
#include "source/common/stream_info/uint32_accessor_impl.h"
#include "source/extensions/filters/http/ratelimit/ratelimit.h"

#include "test/extensions/filters/common/ratelimit/mocks.h"
Expand Down Expand Up @@ -257,6 +259,53 @@ TEST_F(HttpRateLimitFilterTest, OkResponse) {
1U, filter_callbacks_.clusterInfo()->statsScope().counterFromStatName(ratelimit_ok_).value());
}

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

filter_callbacks_.stream_info_.filter_state_->setData(
"envoy.ratelimit.hits_addend", std::make_unique<StreamInfo::UInt32AccessorImpl>(5),
StreamInfo::FilterState::StateType::ReadOnly);
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_->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 Expand Up @@ -1668,6 +1717,19 @@ TEST_F(HttpRateLimitFilterTest, StatsWithPrefix) {
}

} // namespace

TEST(ObjectFactory, HitsAddend) {
const std::string name = "envoy.ratelimit.hits_addend";
auto* factory =
Registry::FactoryRegistry<StreamInfo::FilterState::ObjectFactory>::getFactory(name);
ASSERT_NE(nullptr, factory);
EXPECT_EQ(name, factory->name());
const std::string hits_addend = std::to_string(1234);
auto object = factory->createFromBytes(hits_addend);
ASSERT_NE(nullptr, object);
EXPECT_EQ(hits_addend, object->serializeAsString());
}

EItanya marked this conversation as resolved.
Show resolved Hide resolved
} // namespace RateLimitFilter
} // namespace HttpFilters
} // namespace Extensions
Expand Down
Loading