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.

Sign up for GitHub

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

Fix matcher-tree creation happening per filter instance #37797

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+54 −49
Open

Fix matcher-tree creation happening per filter instance #37797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 13 additions & 3 deletions source/extensions/filters/http/rate_limit_quota/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,23 @@ Http::FilterFactoryCb RateLimitQuotaFilterFactory::createFilterFactoryFromProtoT
return tl_global_client;
});

return [&, config = std::move(config), config_with_hash_key,
tls_store](Http::FilterChainFactoryCallbacks& callbacks) -> void {
RateLimitOnMatchActionContext action_context;
RateLimitQuotaValidationVisitor visitor;
Matcher::MatchTreeFactory<Http::HttpMatchingData, RateLimitOnMatchActionContext> matcher_factory(
action_context, context.serverFactoryContext(), visitor);

Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> matcher = nullptr;
if (config->has_bucket_matchers()) {
matcher = matcher_factory.create(config->bucket_matchers())();
}

return [&, config = std::move(config), config_with_hash_key, tls_store,
matcher](Http::FilterChainFactoryCallbacks& callbacks) -> void {
Copy link
Member

@tyxia tyxia Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for the matcher shared pointer here, you only need to make copy (i.e., increase ref count once) once.

For example, in the RateLimitQuotaFilter constructor, it is passed by value which will achieve the need of copy above. Other places can just be move

std::unique_ptr<RateLimitClient> local_client =
createLocalRateLimitClient(tls_store->global_client_tls, tls_store->buckets_tls);

callbacks.addStreamFilter(std::make_shared<RateLimitQuotaFilter>(
config, context, std::move(local_client), config_with_hash_key));
config, context, std::move(local_client), config_with_hash_key, matcher));
};
}

Expand Down
53 changes: 19 additions & 34 deletions source/extensions/filters/http/rate_limit_quota/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,54 +155,39 @@ Http::FilterHeadersStatus RateLimitQuotaFilter::decodeHeaders(Http::RequestHeade
StreamInfo::CoreResponseFlag::ResponseFromCacheFilter);
}

void RateLimitQuotaFilter::createMatcher() {
RateLimitOnMatchActionContext context;
Matcher::MatchTreeFactory<Http::HttpMatchingData, RateLimitOnMatchActionContext> factory(
context, factory_context_.serverFactoryContext(), visitor_);
if (config_->has_bucket_matchers()) {
matcher_ = factory.create(config_->bucket_matchers())();
}
}

// TODO(tyxia) Currently request matching is only performed on the request
// header.
absl::StatusOr<Matcher::ActionPtr>
RateLimitQuotaFilter::requestMatching(const Http::RequestHeaderMap& headers) {
// Initialize the data pointer on first use and reuse it for subsequent
// requests. This avoids creating the data object for every request, which
// is expensive.
if (data_ptr_ == nullptr) {
if (callbacks_ != nullptr) {
data_ptr_ = std::make_unique<Http::Matching::HttpMatchingDataImpl>(callbacks_->streamInfo());
} else {
if (!data_ptr_) {
Copy link
Member

Choose a reason for hiding this comment

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

i actually prefer data_ptr == nullptr style throughout this file. That is more readable for pointer type.

if (!callbacks_) {
return absl::InternalError("Filter callback has not been initialized successfully yet.");
}
data_ptr_ = std::make_unique<Http::Matching::HttpMatchingDataImpl>(callbacks_->streamInfo());
}

if (matcher_ == nullptr) {
if (!matcher_) {
return absl::InternalError("Matcher tree has not been initialized yet.");
} else {
// Populate the request header.
if (!headers.empty()) {
data_ptr_->onRequestHeaders(headers);
}

// Perform the matching.
auto match_result = Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, *data_ptr_);
}
// Populate the request header.
if (!headers.empty()) {
data_ptr_->onRequestHeaders(headers);
}

if (match_result.match_state_ == Matcher::MatchState::MatchComplete) {
if (match_result.result_) {
// Return the matched result for `on_match` case.
return match_result.result_();
} else {
return absl::NotFoundError("Matching completed but no match result was found.");
}
} else {
// The returned state from `evaluateMatch` function is
// `MatchState::UnableToMatch` here.
return absl::InternalError("Unable to match due to the required data not being available.");
}
// Perform the matching.
auto match_result = Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, *data_ptr_);
if (match_result.match_state_ != Matcher::MatchState::MatchComplete) {
// The returned state from `evaluateMatch` function is `MatchState::UnableToMatch` here.
return absl::InternalError("Unable to match due to the required data not being available.");
}
if (!match_result.result_) {
return absl::NotFoundError("Matching completed but no match result was found.");
}
// Return the matched result for `on_match` case.
return match_result.result_();
}

void RateLimitQuotaFilter::onDestroy() {
Expand Down
14 changes: 5 additions & 9 deletions source/extensions/filters/http/rate_limit_quota/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,11 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter,
RateLimitQuotaFilter(FilterConfigConstSharedPtr config,
Server::Configuration::FactoryContext& factory_context,
std::unique_ptr<RateLimitClient> local_client,
Grpc::GrpcServiceConfigWithHashKey config_with_hash_key)
Grpc::GrpcServiceConfigWithHashKey config_with_hash_key,
Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> matcher)
: config_(std::move(config)), config_with_hash_key_(config_with_hash_key),
factory_context_(factory_context), client_(std::move(local_client)),
time_source_(factory_context.serverFactoryContext().mainThreadDispatcher().timeSource()) {
createMatcher();
}
factory_context_(factory_context), matcher_(matcher), client_(std::move(local_client)),
time_source_(factory_context.serverFactoryContext().mainThreadDispatcher().timeSource()) {}

Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool end_stream) override;
void onDestroy() override;
Expand All @@ -72,9 +71,6 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter,
}

private:
// Create the matcher factory and matcher.
void createMatcher();

Http::FilterHeadersStatus processCachedBucket(CachedBucket& cached_bucket);
bool shouldAllowRequest(const CachedBucket& cached_bucket);

Expand All @@ -83,7 +79,7 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter,
Server::Configuration::FactoryContext& factory_context_;
Http::StreamDecoderFilterCallbacks* callbacks_ = nullptr;
RateLimitQuotaValidationVisitor visitor_ = {};
Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> matcher_ = nullptr;
Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> matcher_;
std::unique_ptr<Http::Matching::HttpMatchingDataImpl> data_ptr_ = nullptr;

// Own a local, filter-specific client to provider functions needed by worker
Expand Down
20 changes: 17 additions & 3 deletions test/extensions/filters/http/rate_limit_quota/filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ class FilterTest : public testing::Test {
TestUtility::loadFromYaml(std::string(GoogleGrpcConfig), config_);
}

void addMatcherConfig(xds::type::matcher::v3::Matcher& matcher) {
config_.mutable_bucket_matchers()->MergeFrom(matcher);
match_tree_ = matcher_factory_.create(matcher)();
}

void addMatcherConfig(MatcherConfigType config_type) {
// Add the matcher configuration.
xds::type::matcher::v3::Matcher matcher;
Expand Down Expand Up @@ -96,7 +101,7 @@ class FilterTest : public testing::Test {

// Empty matcher config will not have the bucket matcher configured.
if (config_type != MatcherConfigType::Empty) {
config_.mutable_bucket_matchers()->MergeFrom(matcher);
addMatcherConfig(matcher);
}
}

Expand All @@ -106,8 +111,9 @@ class FilterTest : public testing::Test {
Grpc::GrpcServiceConfigWithHashKey(filter_config_->rlqs_server());

mock_local_client_ = new MockRateLimitClient();
filter_ = std::make_unique<RateLimitQuotaFilter>(
filter_config_, context_, absl::WrapUnique(mock_local_client_), config_with_hash_key);
filter_ = std::make_unique<RateLimitQuotaFilter>(filter_config_, context_,
absl::WrapUnique(mock_local_client_),
config_with_hash_key, match_tree_);
if (set_callback) {
filter_->setDecoderFilterCallbacks(decoder_callbacks_);
}
Expand Down Expand Up @@ -163,11 +169,18 @@ class FilterTest : public testing::Test {

NiceMock<Event::MockDispatcher> dispatcher_;
NiceMock<MockFactoryContext> context_;
RateLimitOnMatchActionContext action_context_ = {};
RateLimitQuotaValidationVisitor visitor_ = {};
Matcher::MatchTreeFactory<Http::HttpMatchingData, RateLimitOnMatchActionContext>
matcher_factory_ =
Matcher::MatchTreeFactory<Http::HttpMatchingData, RateLimitOnMatchActionContext>(
action_context_, context_.serverFactoryContext(), visitor_);
NiceMock<Envoy::Http::MockStreamDecoderFilterCallbacks> decoder_callbacks_;

MockRateLimitClient* mock_local_client_ = nullptr;
FilterConfigConstSharedPtr filter_config_;
FilterConfig config_;
Matcher::MatchTreeSharedPtr<Http::HttpMatchingData> match_tree_ = nullptr;
std::unique_ptr<RateLimitQuotaFilter> filter_;
Http::TestRequestHeaderMapImpl default_headers_{
{":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}};
Expand Down Expand Up @@ -391,6 +404,7 @@ TEST_F(FilterTest, RequestMatchingSucceededWithCelMatcher) {
Protobuf::TextFormat::ParseFromString(on_match_str, &on_match);
inner_matcher->mutable_on_match()->MergeFrom(on_match);
config_.mutable_bucket_matchers()->MergeFrom(matcher);
addMatcherConfig(matcher);
createFilter();
// Define the key value pairs that is used to build the bucket_id dynamically
// via `custom_value` in the config.
Expand Down