From bb9a2776578a2f307566237a480c4fd3f73036cd Mon Sep 17 00:00:00 2001 From: Brian Surber Date: Mon, 23 Dec 2024 21:56:49 +0000 Subject: [PATCH] Move matcher-tree creation to config.cc so that it isn't recreated with every instance of the rate_limit_quota filter Signed-off-by: Brian Surber --- .../filters/http/rate_limit_quota/config.cc | 16 ++++-- .../filters/http/rate_limit_quota/filter.cc | 53 +++++++------------ .../filters/http/rate_limit_quota/filter.h | 14 ++--- .../http/rate_limit_quota/filter_test.cc | 20 +++++-- 4 files changed, 54 insertions(+), 49 deletions(-) diff --git a/source/extensions/filters/http/rate_limit_quota/config.cc b/source/extensions/filters/http/rate_limit_quota/config.cc index 4c06da5a7c15..0ef4e6532aa0 100644 --- a/source/extensions/filters/http/rate_limit_quota/config.cc +++ b/source/extensions/filters/http/rate_limit_quota/config.cc @@ -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 matcher_factory( + action_context, context.serverFactoryContext(), visitor); + + Matcher::MatchTreeSharedPtr 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 local_client = createLocalRateLimitClient(tls_store->global_client_tls, tls_store->buckets_tls); callbacks.addStreamFilter(std::make_shared( - config, context, std::move(local_client), config_with_hash_key)); + config, context, std::move(local_client), config_with_hash_key, matcher)); }; } diff --git a/source/extensions/filters/http/rate_limit_quota/filter.cc b/source/extensions/filters/http/rate_limit_quota/filter.cc index 0c17bef61627..de3141e3a317 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.cc +++ b/source/extensions/filters/http/rate_limit_quota/filter.cc @@ -155,15 +155,6 @@ Http::FilterHeadersStatus RateLimitQuotaFilter::decodeHeaders(Http::RequestHeade StreamInfo::CoreResponseFlag::ResponseFromCacheFilter); } -void RateLimitQuotaFilter::createMatcher() { - RateLimitOnMatchActionContext context; - Matcher::MatchTreeFactory 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 @@ -171,38 +162,32 @@ 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(callbacks_->streamInfo()); - } else { + if (!data_ptr_) { + if (!callbacks_) { return absl::InternalError("Filter callback has not been initialized successfully yet."); } + data_ptr_ = std::make_unique(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(*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(*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() { diff --git a/source/extensions/filters/http/rate_limit_quota/filter.h b/source/extensions/filters/http/rate_limit_quota/filter.h index 78fe4b7f7e9d..f052e2c56401 100644 --- a/source/extensions/filters/http/rate_limit_quota/filter.h +++ b/source/extensions/filters/http/rate_limit_quota/filter.h @@ -49,12 +49,11 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter, RateLimitQuotaFilter(FilterConfigConstSharedPtr config, Server::Configuration::FactoryContext& factory_context, std::unique_ptr local_client, - Grpc::GrpcServiceConfigWithHashKey config_with_hash_key) + Grpc::GrpcServiceConfigWithHashKey config_with_hash_key, + Matcher::MatchTreeSharedPtr 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; @@ -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); @@ -83,7 +79,7 @@ class RateLimitQuotaFilter : public Http::PassThroughFilter, Server::Configuration::FactoryContext& factory_context_; Http::StreamDecoderFilterCallbacks* callbacks_ = nullptr; RateLimitQuotaValidationVisitor visitor_ = {}; - Matcher::MatchTreeSharedPtr matcher_ = nullptr; + Matcher::MatchTreeSharedPtr matcher_; std::unique_ptr data_ptr_ = nullptr; // Own a local, filter-specific client to provider functions needed by worker diff --git a/test/extensions/filters/http/rate_limit_quota/filter_test.cc b/test/extensions/filters/http/rate_limit_quota/filter_test.cc index a87b762399cd..72add92fb1e5 100644 --- a/test/extensions/filters/http/rate_limit_quota/filter_test.cc +++ b/test/extensions/filters/http/rate_limit_quota/filter_test.cc @@ -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; @@ -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); } } @@ -106,8 +111,9 @@ class FilterTest : public testing::Test { Grpc::GrpcServiceConfigWithHashKey(filter_config_->rlqs_server()); mock_local_client_ = new MockRateLimitClient(); - filter_ = std::make_unique( - filter_config_, context_, absl::WrapUnique(mock_local_client_), config_with_hash_key); + filter_ = std::make_unique(filter_config_, context_, + absl::WrapUnique(mock_local_client_), + config_with_hash_key, match_tree_); if (set_callback) { filter_->setDecoderFilterCallbacks(decoder_callbacks_); } @@ -163,11 +169,18 @@ class FilterTest : public testing::Test { NiceMock dispatcher_; NiceMock context_; + RateLimitOnMatchActionContext action_context_ = {}; + RateLimitQuotaValidationVisitor visitor_ = {}; + Matcher::MatchTreeFactory + matcher_factory_ = + Matcher::MatchTreeFactory( + action_context_, context_.serverFactoryContext(), visitor_); NiceMock decoder_callbacks_; MockRateLimitClient* mock_local_client_ = nullptr; FilterConfigConstSharedPtr filter_config_; FilterConfig config_; + Matcher::MatchTreeSharedPtr match_tree_ = nullptr; std::unique_ptr filter_; Http::TestRequestHeaderMapImpl default_headers_{ {":method", "GET"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; @@ -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.