Skip to content

Commit

Permalink
rlqs: Fix matcher-tree creation happening per filter instance (#37797)
Browse files Browse the repository at this point in the history 
Commit Message: Move matcher-tree creation to config.cc so that it isn't
recreated with every instance of the rate_limit_quota filter
Additional Description:
Risk Level:
Testing: Manual testing in-progress
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes #37534

Signed-off-by: Brian Surber <[email protected]>
  • Loading branch information
@bsurber
bsurber authored Dec 27, 2024
1 parent f122866 commit 52e1e93
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 49 deletions.
16 changes: 13 additions & 3 deletions source/extensions/filters/http/rate_limit_quota/config.cc
View file
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 {
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
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_) {
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
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
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

0 comments on commit 52e1e93

Please sign in to comment.