From 50f8f1daf098fd271b9cd05c4daa7213d44a2aa2 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Fri, 20 Dec 2024 15:57:49 +0000 Subject: [PATCH 01/13] string-matcher: reduce per-matcher memory Signed-off-by: Adi Suissa-Peleg --- envoy/common/regex.h | 6 + source/common/common/matchers.cc | 6 +- source/common/common/matchers.h | 249 +++++++++++++----- source/common/common/regex.h | 3 + source/common/http/header_utility.h | 6 +- source/common/matcher/matcher.h | 3 +- source/common/matcher/value_input_matcher.h | 4 +- source/common/router/config_impl.cc | 17 +- source/common/router/config_impl.h | 20 +- source/common/router/config_utility.cc | 2 +- source/common/router/config_utility.h | 3 +- source/common/stats/histogram_impl.cc | 3 +- source/common/stats/histogram_impl.h | 3 +- source/common/stats/stats_matcher_impl.h | 2 +- .../common/tls/cert_validator/san_matcher.h | 3 +- .../extensions/common/aws/signer_base_impl.h | 7 +- .../common/ext_authz/check_request_utils.cc | 8 +- .../common/ext_authz/ext_authz_http_impl.cc | 8 +- .../extensions/filters/common/rbac/matchers.h | 18 +- .../filters/http/cache/cache_headers_utils.cc | 4 +- .../filters/http/csrf/csrf_filter.h | 3 +- .../filters/http/ext_proc/matching_utils.cc | 4 +- .../filters/http/jwt_authn/jwks_cache.cc | 2 +- .../dubbo_proxy/router/route_matcher.h | 2 +- .../filters/network/generic_proxy/match.cc | 12 +- .../http/health_checker_impl.h | 3 +- test/common/common/matchers_test.cc | 36 +++ tools/spelling/spelling_dictionary.txt | 1 + 28 files changed, 287 insertions(+), 151 deletions(-) diff --git a/envoy/common/regex.h b/envoy/common/regex.h index b5261476708d7..84d3c98cbc035 100644 --- a/envoy/common/regex.h +++ b/envoy/common/regex.h @@ -22,6 +22,12 @@ class CompiledMatcher : public Matchers::StringMatcher { */ virtual std::string replaceAll(absl::string_view value, absl::string_view substitution) const PURE; + + /** + * Returns a string representation of the Regex matcher (the pattern to be + * matched). + */ + virtual const std::string& stringRepresentation() const PURE; }; using CompiledMatcherPtr = std::unique_ptr; diff --git a/source/common/common/matchers.cc b/source/common/common/matchers.cc index 8e79819db1487..a4119960e78a9 100644 --- a/source/common/common/matchers.cc +++ b/source/common/common/matchers.cc @@ -26,8 +26,7 @@ ValueMatcher::create(const envoy::type::matcher::v3::ValueMatcher& v, case envoy::type::matcher::v3::ValueMatcher::MatchPatternCase::kDoubleMatch: return std::make_shared(v.double_match()); case envoy::type::matcher::v3::ValueMatcher::MatchPatternCase::kStringMatch: - return std::make_shared>>( - v.string_match(), context); + return std::make_shared(v.string_match(), context); case envoy::type::matcher::v3::ValueMatcher::MatchPatternCase::kBoolMatch: return std::make_shared(v.bool_match()); case envoy::type::matcher::v3::ValueMatcher::MatchPatternCase::kPresentMatch: @@ -126,8 +125,7 @@ StringMatcherPtr valueMatcherFromProto(const envoy::type::matcher::v3::FilterSta Server::Configuration::CommonFactoryContext& context) { switch (matcher.matcher_case()) { case envoy::type::matcher::v3::FilterStateMatcher::MatcherCase::kStringMatch: - return std::make_unique>( - matcher.string_match(), context); + return std::make_unique(matcher.string_match(), context); break; default: PANIC_DUE_TO_PROTO_UNSET; diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 3e4b85fcc5196..5616770c8a532 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -90,63 +90,156 @@ class UniversalStringMatcher : public StringMatcher { StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config, Server::Configuration::CommonFactoryContext& context); -template +// A matcher for the `exact` StringMatcher. +class ExactStringMatcher : public StringMatcher { +public: + ExactStringMatcher(absl::string_view exact, bool ignore_case) + : exact_(exact), ignore_case_(ignore_case) {} + + // StringMatcher + bool match(const absl::string_view value) const override { + return ignore_case_ ? absl::EqualsIgnoreCase(value, exact_) : value == exact_; + } + + const std::string& stringRepresentation() const { return exact_; } + +private: + const std::string exact_; + const bool ignore_case_; +}; + +// A matcher for the `prefix` StringMatcher. +class PrefixStringMatcher : public StringMatcher { +public: + PrefixStringMatcher(absl::string_view prefix, bool ignore_case) + : prefix_(prefix), ignore_case_(ignore_case) {} + + // StringMatcher + bool match(const absl::string_view value) const override { + return ignore_case_ ? absl::StartsWithIgnoreCase(value, prefix_) + : absl::StartsWith(value, prefix_); + } + + const std::string& stringRepresentation() const { return prefix_; } + +private: + friend class StringMatcherImpl; + const std::string prefix_; + const bool ignore_case_; +}; + +// A matcher for the `suffix` StringMatcher. +class SuffixStringMatcher : public StringMatcher { +public: + SuffixStringMatcher(absl::string_view suffix, bool ignore_case) + : suffix_(suffix), ignore_case_(ignore_case) {} + + // StringMatcher + bool match(const absl::string_view value) const override { + return ignore_case_ ? absl::EndsWithIgnoreCase(value, suffix_) : absl::EndsWith(value, suffix_); + } + + const std::string& stringRepresentation() const { return suffix_; } + +private: + const std::string suffix_; + const bool ignore_case_; +}; + +// A matcher for the `safe_regex` StringMatcher. +class RegexStringMatcher : public StringMatcher { +public: + // The RegexMatcher can either be from the ::envoy or ::xds type, + // and the templated c'tor handles both cases. + template + RegexStringMatcher(const RegexMatcherType& safe_regex, + Server::Configuration::CommonFactoryContext& context) + : regex_(THROW_OR_RETURN_VALUE(Regex::Utility::parseRegex(safe_regex, context.regexEngine()), + Regex::CompiledMatcherPtr)) {} + + RegexStringMatcher(RegexStringMatcher&& other) { regex_ = std::move(other.regex_); } + + // StringMatcher + bool match(const absl::string_view value) const override { return regex_->match(value); } + + const std::string& stringRepresentation() const { return regex_->stringRepresentation(); } + +private: + Regex::CompiledMatcherPtr regex_; +}; + +// A matcher for the `contains` StringMatcher. +class ContainsStringMatcher : public StringMatcher { +public: + ContainsStringMatcher(absl::string_view contents, bool ignore_case) + : contents_(ignore_case ? absl::AsciiStrToLower(contents) : contents), + ignore_case_(ignore_case) {} + + // StringMatcher + bool match(const absl::string_view value) const override { + return ignore_case_ ? absl::StrContains(absl::AsciiStrToLower(value), contents_) + : absl::StrContains(value, contents_); + } + + const std::string& stringRepresentation() const { + // Note that in case where the matcher is configured to be case-insensitive + // this will return the lower-case configured string (as opposed to the + // other matchers). This is incompatible with the other matchers, but achieves + // the intent of this method. + return contents_; + } + +private: + // If ignore_case is set the contents_ will contain lower-case letters. + const std::string contents_; + const bool ignore_case_; +}; + +// A matcher for the `custom` StringMatcher. +class CustomStringMatcher : public StringMatcher { +public: + CustomStringMatcher(const xds::core::v3::TypedExtensionConfig& custom, + Server::Configuration::CommonFactoryContext& context) + : custom_(getExtensionStringMatcher(custom, context)) {} + + // StringMatcher + bool match(const absl::string_view value) const override { return custom_->match(value); } + + const std::string& stringRepresentation() const { return EMPTY_STRING; } + +private: + StringMatcherPtr custom_; +}; + +// An implementation that holds one of the string matchers. class StringMatcherImpl : public ValueMatcher, public StringMatcher { public: + // Creates a StringMatcher given a config. + // Note that Envoy supports both matchers that are created using the + // `envoy::type::matcher::v3::StringMatcher` type and the + // `xds::core::v3::StringMatcher` type and therefore this function is templated. + template explicit StringMatcherImpl(const StringMatcherType& matcher, Server::Configuration::CommonFactoryContext& context) - : matcher_(matcher) { - if (matcher.match_pattern_case() == StringMatcherType::MatchPatternCase::kSafeRegex) { - if (matcher.ignore_case()) { - ExceptionUtil::throwEnvoyException("ignore_case has no effect for safe_regex."); - } - regex_ = THROW_OR_RETURN_VALUE( - Regex::Utility::parseRegex(matcher_.safe_regex(), context.regexEngine()), - Regex::CompiledMatcherPtr); - } else if (matcher.match_pattern_case() == StringMatcherType::MatchPatternCase::kContains) { - if (matcher_.ignore_case()) { - // Cache the lowercase conversion of the Contains matcher for future use - lowercase_contains_match_ = absl::AsciiStrToLower(matcher_.contains()); - } - } else if (matcher.has_custom()) { - custom_ = getExtensionStringMatcher(matcher.custom(), context); - } - } + : matcher_(createVariant(matcher, context)) {} // Helper to create an exact matcher in contexts where there is no factory context available. // This is a static member instead of constructor so that it can be named for clarity of what it // produces. static StringMatcherImpl createExactMatcher(absl::string_view match) { - return StringMatcherImpl(match); + return StringMatcherImpl(ExactStringMatcher(match, false)); } // StringMatcher bool match(const absl::string_view value) const override { - switch (matcher_.match_pattern_case()) { - case StringMatcherType::MatchPatternCase::kExact: - return matcher_.ignore_case() ? absl::EqualsIgnoreCase(value, matcher_.exact()) - : value == matcher_.exact(); - case StringMatcherType::MatchPatternCase::kPrefix: - return matcher_.ignore_case() ? absl::StartsWithIgnoreCase(value, matcher_.prefix()) - : absl::StartsWith(value, matcher_.prefix()); - case StringMatcherType::MatchPatternCase::kSuffix: - return matcher_.ignore_case() ? absl::EndsWithIgnoreCase(value, matcher_.suffix()) - : absl::EndsWith(value, matcher_.suffix()); - case StringMatcherType::MatchPatternCase::kContains: - return matcher_.ignore_case() - ? absl::StrContains(absl::AsciiStrToLower(value), lowercase_contains_match_) - : absl::StrContains(value, matcher_.contains()); - case StringMatcherType::MatchPatternCase::kSafeRegex: - return regex_->match(value); - case StringMatcherType::MatchPatternCase::kCustom: - return custom_->match(value); - default: - PANIC("unexpected"); - } + // Implementing polymorphism for match(absl::string_value) on the different + // types that can be in the matcher_ variant. + auto call_match = [value](const auto& obj) -> bool { return obj.match(value); }; + return std::visit(call_match, matcher_); } + // ValueMatcher bool match(const ProtobufWkt::Value& value) const override { - if (value.kind_case() != ProtobufWkt::Value::kStringValue) { return false; } @@ -154,8 +247,6 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { return match(value.string_value()); } - const StringMatcherType& matcher() const { return matcher_; } - /** * Helps applications optimize the case where a matcher is a case-sensitive * prefix-match. @@ -164,27 +255,61 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { * @return true if the matcher is a case-sensitive prefix-match. */ bool getCaseSensitivePrefixMatch(std::string& prefix) const { - if (matcher_.match_pattern_case() == - envoy::type::matcher::v3::StringMatcher::MatchPatternCase::kPrefix && - !matcher_.ignore_case()) { - prefix = matcher_.prefix(); - return true; + if (const PrefixStringMatcher* prefix_matcher = std::get_if(&matcher_)) { + if (!prefix_matcher->ignore_case_) { + prefix = prefix_matcher->prefix_; + return true; + } } return false; } + /** + * Returns a string representation of the matcher (the contents to be + * matched). + */ + const std::string& stringRepresentation() const { + // Implementing polymorphism for stringRepresentation() on the different + // types that can be in the matcher_ variant. + auto call_func = [](const auto& obj) -> const std::string& { + return obj.stringRepresentation(); + }; + return std::visit(call_func, matcher_); + } + private: - StringMatcherImpl(absl::string_view exact_match) - : matcher_([&]() -> StringMatcherType { - StringMatcherType cfg; - cfg.set_exact(exact_match); - return cfg; - }()) {} - - const StringMatcherType matcher_; - Regex::CompiledMatcherPtr regex_; - std::string lowercase_contains_match_; - StringMatcherPtr custom_; + explicit StringMatcherImpl(ExactStringMatcher&& exact_matcher) + : matcher_(std::move(exact_matcher)) {} + + using StringMatcherVariant = + std::variant; + + template + static StringMatcherVariant createVariant(const StringMatcherType& matcher, + Server::Configuration::CommonFactoryContext& context) { + switch (matcher.match_pattern_case()) { + case StringMatcherType::MatchPatternCase::kExact: + return ExactStringMatcher(matcher.exact(), matcher.ignore_case()); + case StringMatcherType::MatchPatternCase::kPrefix: + return PrefixStringMatcher(matcher.prefix(), matcher.ignore_case()); + case StringMatcherType::MatchPatternCase::kSuffix: + return SuffixStringMatcher(matcher.suffix(), matcher.ignore_case()); + case StringMatcherType::MatchPatternCase::kSafeRegex: + if (matcher.ignore_case()) { + ExceptionUtil::throwEnvoyException("ignore_case has no effect for safe_regex."); + } + return RegexStringMatcher(matcher.safe_regex(), context); + case StringMatcherType::MatchPatternCase::kContains: + return ContainsStringMatcher(matcher.contains(), matcher.ignore_case()); + case StringMatcherType::MatchPatternCase::kCustom: + return CustomStringMatcher(matcher.custom(), context); + default: + PANIC("unexpected"); + } + } + + StringMatcherVariant matcher_; }; class StringMatcherExtensionFactory : public Config::TypedFactory { @@ -274,12 +399,10 @@ class PathMatcher : public StringMatcher { Server::Configuration::CommonFactoryContext& context); bool match(const absl::string_view path) const override; - const StringMatcherImpl& matcher() const { - return matcher_; - } + const std::string& stringRepresentation() const { return matcher_.stringRepresentation(); } private: - const StringMatcherImpl matcher_; + const StringMatcherImpl matcher_; }; } // namespace Matchers diff --git a/source/common/common/regex.h b/source/common/common/regex.h index 63b4314d46818..5acfae796831d 100644 --- a/source/common/common/regex.h +++ b/source/common/common/regex.h @@ -38,6 +38,9 @@ class CompiledGoogleReMatcher : public CompiledMatcher { return result; } + // CompiledMatcher + const std::string& stringRepresentation() const override { return regex_.pattern(); } + protected: explicit CompiledGoogleReMatcher(const std::string& regex) : regex_(regex, re2::RE2::Quiet) { ENVOY_BUG(regex_.ok(), "Invalid regex"); diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 3ca4ce2b1473b..00cb00d163fc4 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -233,10 +233,8 @@ class HeaderUtility { public: HeaderDataStringMatch(const envoy::config::route::v3::HeaderMatcher& config, Server::Configuration::CommonFactoryContext& factory_context) - : HeaderDataBaseImpl(config), - string_match_(std::make_unique< - Matchers::StringMatcherImpl>( - config.string_match(), factory_context)) {} + : HeaderDataBaseImpl(config), string_match_(std::make_unique( + config.string_match(), factory_context)) {} private: bool specificMatchesHeaders(absl::string_view header_value) const override { diff --git a/source/common/matcher/matcher.h b/source/common/matcher/matcher.h index 3f534b980820c..c6c13479d4953 100644 --- a/source/common/matcher/matcher.h +++ b/source/common/matcher/matcher.h @@ -349,8 +349,7 @@ class MatchTreeFactory : public OnMatchFactory { switch (predicate.matcher_case()) { case SinglePredicateType::kValueMatch: return [&context = server_factory_context_, value_match = predicate.value_match()]() { - return std::make_unique>>( - value_match, context); + return std::make_unique(value_match, context); }; case SinglePredicateType::kCustomMatch: { auto& factory = diff --git a/source/common/matcher/value_input_matcher.h b/source/common/matcher/value_input_matcher.h index f527b2c873646..30bc00d4cf713 100644 --- a/source/common/matcher/value_input_matcher.h +++ b/source/common/matcher/value_input_matcher.h @@ -7,9 +7,9 @@ namespace Envoy { namespace Matcher { -template class StringInputMatcher : public InputMatcher, Logger::Loggable { public: + template explicit StringInputMatcher(const StringMatcherType& matcher, Server::Configuration::CommonFactoryContext& context) : matcher_(matcher, context) {} @@ -23,7 +23,7 @@ class StringInputMatcher : public InputMatcher, Logger::Loggable matcher_; + const Matchers::StringMatcherImpl matcher_; }; } // namespace Matcher diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index f158bca8b07cc..3cb73f84c30b5 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1660,7 +1660,10 @@ PrefixRouteEntryImpl::PrefixRouteEntryImpl( ProtobufMessage::ValidationVisitor& validator, absl::Status& creation_status) : RouteEntryImplBase(vhost, route, factory_context, validator, creation_status), path_matcher_(Matchers::PathMatcher::createPrefix(route.match().prefix(), !case_sensitive(), - factory_context)) {} + factory_context)) { + // The createPrefix function never returns nullptr. + ASSERT(path_matcher_ != nullptr); +} void PrefixRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers, bool insert_envoy_original_path) const { @@ -1689,7 +1692,10 @@ PathRouteEntryImpl::PathRouteEntryImpl(const CommonVirtualHostSharedPtr& vhost, absl::Status& creation_status) : RouteEntryImplBase(vhost, route, factory_context, validator, creation_status), path_matcher_(Matchers::PathMatcher::createExact(route.match().path(), !case_sensitive(), - factory_context)) {} + factory_context)) { + // The createExact function never returns nullptr. + ASSERT(path_matcher_ != nullptr); +} void PathRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers, bool insert_envoy_original_path) const { @@ -1721,6 +1727,8 @@ RegexRouteEntryImpl::RegexRouteEntryImpl( Matchers::PathMatcher::createSafeRegex(route.match().safe_regex(), factory_context)) { ASSERT(route.match().path_specifier_case() == envoy::config::route::v3::RouteMatch::PathSpecifierCase::kSafeRegex); + // The createSafeRegex function never returns nullptr. + ASSERT(path_matcher_ != nullptr); } void RegexRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers, @@ -1784,7 +1792,10 @@ PathSeparatedPrefixRouteEntryImpl::PathSeparatedPrefixRouteEntryImpl( ProtobufMessage::ValidationVisitor& validator, absl::Status& creation_status) : RouteEntryImplBase(vhost, route, factory_context, validator, creation_status), path_matcher_(Matchers::PathMatcher::createPrefix(route.match().path_separated_prefix(), - !case_sensitive(), factory_context)) {} + !case_sensitive(), factory_context)) { + // The createPrefix function never returns nullptr. + ASSERT(path_matcher_ != nullptr); +} void PathSeparatedPrefixRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers, bool insert_envoy_original_path) const { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 344a31326ad53..22300348bcf75 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -179,8 +179,7 @@ template class CorsPolicyImplBase : public CorsPolicy { max_age_(config.max_age()) { for (const auto& string_match : config.allow_origin_string_match()) { allow_origins_.push_back( - std::make_unique>( - string_match, factory_context)); + std::make_unique(string_match, factory_context)); } if (config.has_allow_credentials()) { allow_credentials_ = PROTOBUF_GET_WRAPPED_REQUIRED(config, allow_credentials); @@ -1310,9 +1309,7 @@ class UriTemplateMatcherRouteEntryImpl : public RouteEntryImplBase { class PrefixRouteEntryImpl : public RouteEntryImplBase { public: // Router::PathMatchCriterion - const std::string& matcher() const override { - return path_matcher_ != nullptr ? path_matcher_->matcher().matcher().prefix() : EMPTY_STRING; - } + const std::string& matcher() const override { return path_matcher_->stringRepresentation(); } PathMatchType matchType() const override { return PathMatchType::Prefix; } // Router::Matchable @@ -1345,9 +1342,7 @@ class PrefixRouteEntryImpl : public RouteEntryImplBase { class PathRouteEntryImpl : public RouteEntryImplBase { public: // Router::PathMatchCriterion - const std::string& matcher() const override { - return path_matcher_ != nullptr ? path_matcher_->matcher().matcher().exact() : EMPTY_STRING; - } + const std::string& matcher() const override { return path_matcher_->stringRepresentation(); } PathMatchType matchType() const override { return PathMatchType::Exact; } // Router::Matchable @@ -1379,10 +1374,7 @@ class PathRouteEntryImpl : public RouteEntryImplBase { class RegexRouteEntryImpl : public RouteEntryImplBase { public: // Router::PathMatchCriterion - const std::string& matcher() const override { - return path_matcher_ != nullptr ? path_matcher_->matcher().matcher().safe_regex().regex() - : EMPTY_STRING; - } + const std::string& matcher() const override { return path_matcher_->stringRepresentation(); } PathMatchType matchType() const override { return PathMatchType::Regex; } // Router::Matchable @@ -1446,9 +1438,7 @@ class ConnectRouteEntryImpl : public RouteEntryImplBase { class PathSeparatedPrefixRouteEntryImpl : public RouteEntryImplBase { public: // Router::PathMatchCriterion - const std::string& matcher() const override { - return path_matcher_ != nullptr ? path_matcher_->matcher().matcher().prefix() : EMPTY_STRING; - } + const std::string& matcher() const override { return path_matcher_->stringRepresentation(); } PathMatchType matchType() const override { return PathMatchType::PathSeparatedPrefix; } // Router::Matchable diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 4d0a6c393c2ee..7d30cf3c4b275 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -15,7 +15,7 @@ namespace Envoy { namespace Router { namespace { -absl::optional> +absl::optional maybeCreateStringMatcher(const envoy::config::route::v3::QueryParameterMatcher& config, Server::Configuration::CommonFactoryContext& context) { switch (config.query_parameter_match_specifier_case()) { diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index 83f60406ee346..79e20a4dbfc29 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -44,8 +44,7 @@ class ConfigUtility { private: const std::string name_; - const absl::optional> - matcher_; + const absl::optional matcher_; }; using QueryParameterMatcherPtr = std::unique_ptr; diff --git a/source/common/stats/histogram_impl.cc b/source/common/stats/histogram_impl.cc index 50d1418198f5d..4f9adc9a459f0 100644 --- a/source/common/stats/histogram_impl.cc +++ b/source/common/stats/histogram_impl.cc @@ -106,8 +106,7 @@ HistogramSettingsImpl::HistogramSettingsImpl(const envoy::config::metrics::v3::S for (const auto& matcher : config.histogram_bucket_settings()) { std::vector buckets{matcher.buckets().begin(), matcher.buckets().end()}; std::sort(buckets.begin(), buckets.end()); - configs.emplace_back(Matchers::StringMatcherImpl( - matcher.match(), context), + configs.emplace_back(Matchers::StringMatcherImpl(matcher.match(), context), std::move(buckets)); } diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index 1f07412193269..3b30f1b289df7 100644 --- a/source/common/stats/histogram_impl.h +++ b/source/common/stats/histogram_impl.h @@ -29,8 +29,7 @@ class HistogramSettingsImpl : public HistogramSettings { static ConstSupportedBuckets& defaultBuckets(); private: - using Config = std::pair, - ConstSupportedBuckets>; + using Config = std::pair; const std::vector configs_{}; }; diff --git a/source/common/stats/stats_matcher_impl.h b/source/common/stats/stats_matcher_impl.h index 3738bf63e8d2f..9f62c5c7bae80 100644 --- a/source/common/stats/stats_matcher_impl.h +++ b/source/common/stats/stats_matcher_impl.h @@ -52,7 +52,7 @@ class StatsMatcherImpl : public StatsMatcher { OptRef symbol_table_; std::unique_ptr stat_name_pool_; - std::vector> matchers_; + std::vector matchers_; std::vector prefixes_; }; diff --git a/source/common/tls/cert_validator/san_matcher.h b/source/common/tls/cert_validator/san_matcher.h index 1bd5b3d702430..9c3624aee5d1b 100644 --- a/source/common/tls/cert_validator/san_matcher.h +++ b/source/common/tls/cert_validator/san_matcher.h @@ -32,7 +32,6 @@ using SanMatcherPtr = std::unique_ptr; class StringSanMatcher : public SanMatcher { public: - using StringMatcherImpl = Matchers::StringMatcherImpl; bool match(const GENERAL_NAME* general_name) const override; ~StringSanMatcher() override = default; @@ -50,7 +49,7 @@ class StringSanMatcher : public SanMatcher { private: const int general_name_type_; - const StringMatcherImpl matcher_; + const Envoy::Matchers::StringMatcherImpl matcher_; bssl::UniquePtr general_name_oid_; }; diff --git a/source/extensions/common/aws/signer_base_impl.h b/source/extensions/common/aws/signer_base_impl.h index 241b22b62a9b0..c17348ffd2b88 100644 --- a/source/extensions/common/aws/signer_base_impl.h +++ b/source/extensions/common/aws/signer_base_impl.h @@ -76,8 +76,7 @@ class SignerBaseImpl : public Signer, public Logger::Loggable { short_date_formatter_(std::string(SignatureConstants::ShortDateFormat)) { for (const auto& matcher : matcher_config) { excluded_header_matchers_.emplace_back( - std::make_unique>( - matcher, context)); + std::make_unique(matcher, context)); } } @@ -141,9 +140,7 @@ class SignerBaseImpl : public Signer, public Logger::Loggable { for (const auto& header : default_excluded_headers_) { envoy::type::matcher::v3::StringMatcher m; m.set_exact(header); - matcher_ptrs.emplace_back( - std::make_unique>( - m, context)); + matcher_ptrs.emplace_back(std::make_unique(m, context)); } return matcher_ptrs; } diff --git a/source/extensions/filters/common/ext_authz/check_request_utils.cc b/source/extensions/filters/common/ext_authz/check_request_utils.cc index cddf4dc9c07d2..b1d79d95a133e 100644 --- a/source/extensions/filters/common/ext_authz/check_request_utils.cc +++ b/source/extensions/filters/common/ext_authz/check_request_utils.cc @@ -306,9 +306,7 @@ CheckRequestUtils::toRequestMatchers(const envoy::type::matcher::v3::ListStringM for (const auto& key : keys) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact(key.get()); - matchers.push_back( - std::make_unique>( - matcher, context)); + matchers.push_back(std::make_unique(matcher, context)); } } @@ -320,9 +318,7 @@ CheckRequestUtils::createStringMatchers(const envoy::type::matcher::v3::ListStri Server::Configuration::CommonFactoryContext& context) { std::vector matchers; for (const auto& matcher : list.patterns()) { - matchers.push_back( - std::make_unique>( - matcher, context)); + matchers.push_back(std::make_unique(matcher, context)); } return matchers; } diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index c83ea223f81a3..03ddd880e139e 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -160,9 +160,7 @@ ClientConfig::toClientMatchers(const envoy::type::matcher::v3::ListStringMatcher if (matchers.empty()) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact(Http::Headers::get().Host.get()); - matchers.push_back( - std::make_unique>( - matcher, context)); + matchers.push_back(std::make_unique(matcher, context)); return std::make_shared(std::move(matchers)); } @@ -176,9 +174,7 @@ ClientConfig::toClientMatchers(const envoy::type::matcher::v3::ListStringMatcher for (const auto& key : keys) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact(key.get()); - matchers.push_back( - std::make_unique>( - matcher, context)); + matchers.push_back(std::make_unique(matcher, context)); } return std::make_shared(std::move(matchers)); diff --git a/source/extensions/filters/common/rbac/matchers.h b/source/extensions/filters/common/rbac/matchers.h index 2a0b0883c7597..26ac74498216e 100644 --- a/source/extensions/filters/common/rbac/matchers.h +++ b/source/extensions/filters/common/rbac/matchers.h @@ -205,18 +205,15 @@ class AuthenticatedMatcher : public Matcher { public: AuthenticatedMatcher(const envoy::config::rbac::v3::Principal::Authenticated& auth, Server::Configuration::CommonFactoryContext& context) - : matcher_(auth.has_principal_name() - ? absl::make_optional< - Matchers::StringMatcherImpl>( - auth.principal_name(), context) - : absl::nullopt) {} + : matcher_(auth.has_principal_name() ? absl::make_optional( + auth.principal_name(), context) + : absl::nullopt) {} bool matches(const Network::Connection& connection, const Envoy::Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo&) const override; private: - const absl::optional> - matcher_; + const absl::optional matcher_; }; /** @@ -277,14 +274,11 @@ class FilterStateMatcher : public Matcher { * Perform a match against the request server from the client's connection * request. This is typically TLS SNI. */ -class RequestedServerNameMatcher - : public Matcher, - Envoy::Matchers::StringMatcherImpl { +class RequestedServerNameMatcher : public Matcher, Envoy::Matchers::StringMatcherImpl { public: RequestedServerNameMatcher(const envoy::type::matcher::v3::StringMatcher& requested_server_name, Server::Configuration::CommonFactoryContext& context) - : Envoy::Matchers::StringMatcherImpl( - requested_server_name, context) {} + : Envoy::Matchers::StringMatcherImpl(requested_server_name, context) {} bool matches(const Network::Connection& connection, const Envoy::Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo&) const override; diff --git a/source/extensions/filters/http/cache/cache_headers_utils.cc b/source/extensions/filters/http/cache/cache_headers_utils.cc index c6d05aec6d587..06fd13bf898d2 100644 --- a/source/extensions/filters/http/cache/cache_headers_utils.cc +++ b/source/extensions/filters/http/cache/cache_headers_utils.cc @@ -288,9 +288,7 @@ VaryAllowList::VaryAllowList( Server::Configuration::CommonFactoryContext& context) { for (const auto& rule : allow_list) { - allow_list_.emplace_back( - std::make_unique>( - rule, context)); + allow_list_.emplace_back(std::make_unique(rule, context)); } } diff --git a/source/extensions/filters/http/csrf/csrf_filter.h b/source/extensions/filters/http/csrf/csrf_filter.h index 85eb1c78fed11..09a1a60dfbd12 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.h +++ b/source/extensions/filters/http/csrf/csrf_filter.h @@ -39,8 +39,7 @@ class CsrfPolicy : public Router::RouteSpecificFilterConfig { : policy_(policy), runtime_(context.runtime()) { for (const auto& additional_origin : policy.additional_origins()) { additional_origins_.emplace_back( - std::make_unique>( - additional_origin, context)); + std::make_unique(additional_origin, context)); } } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 35ffaf9ced5de..f87f44e89612c 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -109,9 +109,7 @@ initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_lis Server::Configuration::CommonFactoryContext& context) { std::vector header_matchers; for (const auto& matcher : header_list.patterns()) { - header_matchers.push_back( - std::make_unique>( - matcher, context)); + header_matchers.push_back(std::make_unique(matcher, context)); } return header_matchers; } diff --git a/source/extensions/filters/http/jwt_authn/jwks_cache.cc b/source/extensions/filters/http/jwt_authn/jwks_cache.cc index 6732dfa66c0bf..661ccc43900fa 100644 --- a/source/extensions/filters/http/jwt_authn/jwks_cache.cc +++ b/source/extensions/filters/http/jwt_authn/jwks_cache.cc @@ -248,7 +248,7 @@ class JwksDataImpl : public JwksCache::JwksData, public Logger::Loggable tls_; // async fetcher JwksAsyncFetcherPtr async_fetcher_; - absl::optional> sub_matcher_; + absl::optional sub_matcher_; absl::optional max_exp_; // For exponential backoff, when fetch/verify fails on KID mismatch. diff --git a/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h b/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h index 9ac09e453dd26..13cea3044061f 100644 --- a/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h +++ b/source/extensions/filters/network/dubbo_proxy/router/route_matcher.h @@ -129,7 +129,7 @@ class MethodRouteEntryImpl : public RouteEntryImplBase { uint64_t random_value) const override; private: - const Matchers::StringMatcherImpl method_name_; + const Matchers::StringMatcherImpl method_name_; std::shared_ptr parameter_route_; }; diff --git a/source/extensions/filters/network/generic_proxy/match.cc b/source/extensions/filters/network/generic_proxy/match.cc index b9244deedd568..380134be4891c 100644 --- a/source/extensions/filters/network/generic_proxy/match.cc +++ b/source/extensions/filters/network/generic_proxy/match.cc @@ -17,24 +17,22 @@ REGISTER_FACTORY(PropertyMatchDataInputFactory, Matcher::DataInputFactory); -using StringMatcherImpl = Matchers::StringMatcherImpl; - RequestMatchInputMatcher::RequestMatchInputMatcher( const RequestMatcherProto& proto_config, Server::Configuration::CommonFactoryContext& context) { if (proto_config.has_host()) { - host_ = std::make_unique(proto_config.host(), context); + host_ = std::make_unique(proto_config.host(), context); } if (proto_config.has_path()) { - path_ = std::make_unique(proto_config.path(), context); + path_ = std::make_unique(proto_config.path(), context); } if (proto_config.has_method()) { - method_ = std::make_unique(proto_config.method(), context); + method_ = std::make_unique(proto_config.method(), context); } for (const auto& property : proto_config.properties()) { - properties_.push_back( - {property.name(), std::make_unique(property.string_match(), context)}); + properties_.push_back({property.name(), std::make_unique( + property.string_match(), context)}); } } diff --git a/source/extensions/health_checkers/http/health_checker_impl.h b/source/extensions/health_checkers/http/health_checker_impl.h index 939062b7365f3..a92226471a7d6 100644 --- a/source/extensions/health_checkers/http/health_checker_impl.h +++ b/source/extensions/health_checkers/http/health_checker_impl.h @@ -169,8 +169,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { PayloadMatcher::MatchSegments receive_bytes_; const envoy::config::core::v3::RequestMethod method_; uint64_t response_buffer_size_; - absl::optional> - service_name_matcher_; + absl::optional service_name_matcher_; Router::HeaderParserPtr request_headers_parser_; const HttpStatusChecker http_status_checker_; diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index b61689b6b1b23..4d67f5bd753be 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -345,6 +345,15 @@ TEST_F(StringMatcher, ExactMatchIgnoreCase) { EXPECT_FALSE(Matchers::StringMatcherImpl(matcher, context_).match("other")); } +TEST_F(StringMatcher, ExactMatchIgnoreCaseStringRepresentation) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.set_exact("eXaCt"); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "eXaCt"); + + matcher.set_ignore_case(true); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "eXaCt"); +} + TEST_F(StringMatcher, PrefixMatchIgnoreCase) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_prefix("prefix"); @@ -360,6 +369,15 @@ TEST_F(StringMatcher, PrefixMatchIgnoreCase) { EXPECT_FALSE(Matchers::StringMatcherImpl(matcher, context_).match("other")); } +TEST_F(StringMatcher, PrefixMatchIgnoreCaseStringRepresentation) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.set_prefix("pReFix"); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "pReFix"); + + matcher.set_ignore_case(true); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "pReFix"); +} + TEST_F(StringMatcher, SuffixMatchIgnoreCase) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_suffix("suffix"); @@ -375,6 +393,15 @@ TEST_F(StringMatcher, SuffixMatchIgnoreCase) { EXPECT_FALSE(Matchers::StringMatcherImpl(matcher, context_).match("other")); } +TEST_F(StringMatcher, SuffixMatchIgnoreCaseStringRepresentation) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.set_suffix("sUfFix"); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "sUfFix"); + + matcher.set_ignore_case(true); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "sUfFix"); +} + TEST_F(StringMatcher, ContainsMatchIgnoreCase) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_contains("contained-str"); @@ -391,6 +418,15 @@ TEST_F(StringMatcher, ContainsMatchIgnoreCase) { EXPECT_FALSE(Matchers::StringMatcherImpl(matcher, context_).match("other")); } +TEST_F(StringMatcher, ContainsMatchIgnoreCaseStringRepresentation) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.set_contains("ConTained-STR"); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "ConTained-STR"); + + matcher.set_ignore_case(true); + EXPECT_EQ(Matchers::StringMatcherImpl(matcher, context_).stringRepresentation(), "contained-str"); +} + TEST_F(StringMatcher, SafeRegexValue) { envoy::type::matcher::v3::StringMatcher matcher; matcher.mutable_safe_regex()->mutable_google_re2(); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 3dbef85c4e3e0..664bcf75b8b7b 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1104,6 +1104,7 @@ plaintext pluggable pointee poller +polymorphism popen pos posix From 247a58f6f83fb0070f07f2910aa04502aadb9bd9 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Fri, 20 Dec 2024 16:36:35 +0000 Subject: [PATCH 02/13] addressing CI errors Signed-off-by: Adi Suissa-Peleg --- .../filters/network/source/router/route_matcher.h | 2 +- source/common/common/matchers.h | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contrib/rocketmq_proxy/filters/network/source/router/route_matcher.h b/contrib/rocketmq_proxy/filters/network/source/router/route_matcher.h index 67c02c9914633..2bb7c91f0715e 100644 --- a/contrib/rocketmq_proxy/filters/network/source/router/route_matcher.h +++ b/contrib/rocketmq_proxy/filters/network/source/router/route_matcher.h @@ -44,7 +44,7 @@ class RouteEntryImpl : public RouteEntry, private: bool headersMatch(const Http::HeaderMap& headers) const; - const Matchers::StringMatcherImpl topic_name_; + const Matchers::StringMatcherImpl topic_name_; const std::string cluster_name_; const std::vector config_headers_; Envoy::Router::MetadataMatchCriteriaConstPtr metadata_match_criteria_; diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 5616770c8a532..9ace91ab2405c 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -235,7 +235,7 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { // Implementing polymorphism for match(absl::string_value) on the different // types that can be in the matcher_ variant. auto call_match = [value](const auto& obj) -> bool { return obj.match(value); }; - return std::visit(call_match, matcher_); + return absl::visit(call_match, matcher_); } // ValueMatcher @@ -255,7 +255,7 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { * @return true if the matcher is a case-sensitive prefix-match. */ bool getCaseSensitivePrefixMatch(std::string& prefix) const { - if (const PrefixStringMatcher* prefix_matcher = std::get_if(&matcher_)) { + if (const PrefixStringMatcher* prefix_matcher = absl::get_if(&matcher_)) { if (!prefix_matcher->ignore_case_) { prefix = prefix_matcher->prefix_; return true; @@ -274,7 +274,7 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { auto call_func = [](const auto& obj) -> const std::string& { return obj.stringRepresentation(); }; - return std::visit(call_func, matcher_); + return absl::visit(call_func, matcher_); } private: @@ -282,8 +282,8 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { : matcher_(std::move(exact_matcher)) {} using StringMatcherVariant = - std::variant; + absl::variant; template static StringMatcherVariant createVariant(const StringMatcherType& matcher, From a50f1d5f5ab782a9fa20063b23e752e8c52a29db Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Fri, 20 Dec 2024 17:21:19 +0000 Subject: [PATCH 03/13] CI issues Signed-off-by: Adi Suissa-Peleg --- source/common/common/matchers.h | 2 +- source/extensions/filters/network/generic_proxy/match.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 9ace91ab2405c..b8089300652cc 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -283,7 +283,7 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { using StringMatcherVariant = absl::variant; + RegexStringMatcher, ContainsStringMatcher, CustomStringMatcher>; template static StringMatcherVariant createVariant(const StringMatcherType& matcher, diff --git a/source/extensions/filters/network/generic_proxy/match.cc b/source/extensions/filters/network/generic_proxy/match.cc index 380134be4891c..fc91e1407f8a0 100644 --- a/source/extensions/filters/network/generic_proxy/match.cc +++ b/source/extensions/filters/network/generic_proxy/match.cc @@ -21,7 +21,7 @@ RequestMatchInputMatcher::RequestMatchInputMatcher( const RequestMatcherProto& proto_config, Server::Configuration::CommonFactoryContext& context) { if (proto_config.has_host()) { - host_ = std::make_unique(proto_config.host(), context); + host_ = std::make_unique(proto_config.host(), context); } if (proto_config.has_path()) { path_ = std::make_unique(proto_config.path(), context); From 14320a56f2d1f372afa61f6bf79f47cd8132a76f Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Fri, 20 Dec 2024 21:22:10 +0000 Subject: [PATCH 04/13] tests Signed-off-by: Adi Suissa-Peleg --- .../matching/input_matchers/source/matcher.h | 2 ++ test/extensions/common/aws/utility_test.cc | 8 ++--- .../http/cache/cache_headers_utils_test.cc | 20 +++-------- .../filters/http/cors/cors_filter_test.cc | 6 ++-- .../http/ext_proc/mutation_utils_test.cc | 33 +++++-------------- 5 files changed, 20 insertions(+), 49 deletions(-) diff --git a/contrib/hyperscan/matching/input_matchers/source/matcher.h b/contrib/hyperscan/matching/input_matchers/source/matcher.h index d49743ae0c5da..7aac429c122a1 100644 --- a/contrib/hyperscan/matching/input_matchers/source/matcher.h +++ b/contrib/hyperscan/matching/input_matchers/source/matcher.h @@ -44,6 +44,8 @@ class Matcher : public Envoy::Regex::CompiledMatcher, public Envoy::Matcher::Inp // Envoy::Matcher::InputMatcher bool match(const ::Envoy::Matcher::MatchingDataType& input) override; + const std::string& stringRepresentation() const override { return EMPTY_STRING; } + private: hs_database_t* database_{}; hs_database_t* start_of_match_database_{}; diff --git a/test/extensions/common/aws/utility_test.cc b/test/extensions/common/aws/utility_test.cc index 6cabe0ebd79e3..3358454a64fba 100644 --- a/test/extensions/common/aws/utility_test.cc +++ b/test/extensions/common/aws/utility_test.cc @@ -165,17 +165,13 @@ TEST(UtilityTest, CanonicalizeHeadersDropExcludedMatchers) { for (auto& str : exact_matches) { envoy::type::matcher::v3::StringMatcher config; config.set_exact(str); - exclusion_list.emplace_back( - std::make_unique>( - config, context)); + exclusion_list.emplace_back(std::make_unique(config, context)); } std::vector prefixes = {"x-envoy"}; for (auto& match_str : prefixes) { envoy::type::matcher::v3::StringMatcher config; config.set_prefix(match_str); - exclusion_list.emplace_back( - std::make_unique>( - config, context)); + exclusion_list.emplace_back(std::make_unique(config, context)); } const auto map = Utility::canonicalizeHeaders(headers, exclusion_list); EXPECT_THAT(map, diff --git a/test/extensions/filters/http/cache/cache_headers_utils_test.cc b/test/extensions/filters/http/cache/cache_headers_utils_test.cc index f794ad5f16b9a..c6dfe1e8bd7de 100644 --- a/test/extensions/filters/http/cache/cache_headers_utils_test.cc +++ b/test/extensions/filters/http/cache/cache_headers_utils_test.cc @@ -496,9 +496,7 @@ TEST(GetAllMatchingHeaderNames, EmptyHeaderMap) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact("accept"); - ruleset.emplace_back( - std::make_unique>( - matcher, context)); + ruleset.emplace_back(std::make_unique(matcher, context)); CacheHeadersUtils::getAllMatchingHeaderNames(headers, ruleset, result); @@ -513,9 +511,7 @@ TEST(GetAllMatchingHeaderNames, SingleMatchSingleValue) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact("accept"); - ruleset.emplace_back( - std::make_unique>( - matcher, context)); + ruleset.emplace_back(std::make_unique(matcher, context)); CacheHeadersUtils::getAllMatchingHeaderNames(headers, ruleset, result); @@ -531,9 +527,7 @@ TEST(GetAllMatchingHeaderNames, SingleMatchMultiValue) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact("accept"); - ruleset.emplace_back( - std::make_unique>( - matcher, context)); + ruleset.emplace_back(std::make_unique(matcher, context)); CacheHeadersUtils::getAllMatchingHeaderNames(headers, ruleset, result); @@ -549,13 +543,9 @@ TEST(GetAllMatchingHeaderNames, MultipleMatches) { envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact("accept"); - ruleset.emplace_back( - std::make_unique>( - matcher, context)); + ruleset.emplace_back(std::make_unique(matcher, context)); matcher.set_exact("accept-language"); - ruleset.emplace_back( - std::make_unique>( - matcher, context)); + ruleset.emplace_back(std::make_unique(matcher, context)); CacheHeadersUtils::getAllMatchingHeaderNames(headers, ruleset, result); diff --git a/test/extensions/filters/http/cors/cors_filter_test.cc b/test/extensions/filters/http/cors/cors_filter_test.cc index 5d960f6f6fd65..66a00e86f2c83 100644 --- a/test/extensions/filters/http/cors/cors_filter_test.cc +++ b/test/extensions/filters/http/cors/cors_filter_test.cc @@ -27,16 +27,14 @@ Matchers::StringMatcherPtr makeExactStringMatcher(const std::string& exact_match NiceMock context; envoy::type::matcher::v3::StringMatcher config; config.set_exact(exact_match); - return std::make_unique>( - config, context); + return std::make_unique(config, context); } Matchers::StringMatcherPtr makeStdRegexStringMatcher(const std::string& regex) { NiceMock context; envoy::type::matcher::v3::StringMatcher config; config.MergeFrom(TestUtility::createRegexMatcher(regex)); - return std::make_unique>( - config, context); + return std::make_unique(config, context); } } // namespace diff --git a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc index ea0efe3b6cf2d..210836d0288be 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -337,13 +337,9 @@ TEST_F(MutationUtilsTest, TestAllowHeadersExactCaseSensitive) { std::vector disallow_headers; envoy::type::matcher::v3::StringMatcher string_matcher; string_matcher.set_exact(":method"); - allow_headers.push_back( - std::make_unique>( - string_matcher, context)); + allow_headers.push_back(std::make_unique(string_matcher, context)); string_matcher.set_exact(":Path"); - allow_headers.push_back( - std::make_unique>( - string_matcher, context)); + allow_headers.push_back(std::make_unique(string_matcher, context)); MutationUtils::headersToProto(headers, allow_headers, disallow_headers, proto_headers); Http::TestRequestHeaderMapImpl expected{{":method", "GET"}}; @@ -364,14 +360,10 @@ TEST_F(MutationUtilsTest, TestAllowHeadersExactIgnoreCase) { std::vector disallow_headers; envoy::type::matcher::v3::StringMatcher string_matcher; string_matcher.set_exact(":method"); - allow_headers.push_back( - std::make_unique>( - string_matcher, context)); + allow_headers.push_back(std::make_unique(string_matcher, context)); string_matcher.set_exact(":Path"); string_matcher.set_ignore_case(true); - allow_headers.push_back( - std::make_unique>( - string_matcher, context)); + allow_headers.push_back(std::make_unique(string_matcher, context)); MutationUtils::headersToProto(headers, allow_headers, disallow_headers, proto_headers); Http::TestRequestHeaderMapImpl expected{{":method", "GET"}, {":path", "/foo/the/bar?size=123"}}; EXPECT_THAT(proto_headers, HeaderProtosEqual(expected)); @@ -394,19 +386,14 @@ TEST_F(MutationUtilsTest, TestBothAllowAndDisallowHeadersSet) { // Set allow_headers. string_matcher.set_exact(":method"); - allow_headers.push_back( - std::make_unique>( - string_matcher, context)); + allow_headers.push_back(std::make_unique(string_matcher, context)); string_matcher.set_exact(":path"); - allow_headers.push_back( - std::make_unique>( - string_matcher, context)); + allow_headers.push_back(std::make_unique(string_matcher, context)); // Set disallow_headers string_matcher.set_exact(":method"); disallow_headers.push_back( - std::make_unique>( - string_matcher, context)); + std::make_unique(string_matcher, context)); MutationUtils::headersToProto(headers, allow_headers, disallow_headers, proto_headers); Http::TestRequestHeaderMapImpl expected{{":path", "/foo/the/bar?size=123"}}; @@ -431,12 +418,10 @@ TEST_F(MutationUtilsTest, TestDisallowHeaderSetNotAllowHeader) { // Set disallow_headers. string_matcher.set_exact(":method"); disallow_headers.push_back( - std::make_unique>( - string_matcher, context)); + std::make_unique(string_matcher, context)); string_matcher.set_exact(":path"); disallow_headers.push_back( - std::make_unique>( - string_matcher, context)); + std::make_unique(string_matcher, context)); MutationUtils::headersToProto(headers, allow_headers, disallow_headers, proto_headers); Http::TestRequestHeaderMapImpl expected{{"content-type", "text/plain; encoding=UTF8"}, From 0186a85a0daa861db239e6796ecddc4cef8432e8 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Mon, 23 Dec 2024 21:12:12 +0000 Subject: [PATCH 05/13] fix oauth2 test Signed-off-by: Adi Suissa-Peleg --- source/common/common/matchers.h | 3 ++- test/common/common/matchers_test.cc | 8 ++++++++ test/extensions/filters/http/oauth2/filter_test.cc | 7 ++++++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index b8089300652cc..4a5fc04c53387 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -305,7 +305,8 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher { case StringMatcherType::MatchPatternCase::kCustom: return CustomStringMatcher(matcher.custom(), context); default: - PANIC("unexpected"); + ExceptionUtil::throwEnvoyException( + fmt::format("Configuration must define a matcher: {}", matcher.DebugString())); } } diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index 4d67f5bd753be..5811e3f01aaad 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -445,6 +445,14 @@ TEST_F(StringMatcher, SafeRegexValueIgnoreCase) { EnvoyException, "ignore_case has no effect for safe_regex."); } +TEST_F(StringMatcher, NoMatcherRejected) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.set_ignore_case(true); + EXPECT_THROW_WITH_MESSAGE( + Matchers::StringMatcherImpl(matcher, context_).match("foo"), EnvoyException, + fmt::format("Configuration must define a matcher: {}", matcher.DebugString())); +} + class PathMatcher : public BaseTest {}; TEST_F(PathMatcher, MatchExactPath) { diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 3013966470dde..d456e8f524d0d 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -329,8 +329,13 @@ TEST_F(OAuth2Test, InvalidAuthorizationEndpoint) { auto* endpoint = p.mutable_token_endpoint(); endpoint->set_cluster("auth.example.com"); p.set_authorization_endpoint("INVALID_URL"); - auto secret_reader = std::make_shared(); + // Add mandatory fields. + p.set_redirect_uri("%REQ(:scheme)%://%REQ(:authority)%/redirected"); + p.mutable_redirect_path_matcher()->mutable_path()->set_exact("redirected"); + p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); + // Attempt to create the OAuth config. + auto secret_reader = std::make_shared(); EXPECT_THROW_WITH_MESSAGE( std::make_shared(p, factory_context_.server_factory_context_, secret_reader, scope_, "test."), From 3a1fbf2661c23f42045418bcbb9885b3312f8834 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Thu, 2 Jan 2025 20:58:26 +0000 Subject: [PATCH 06/13] improve coverage Signed-off-by: Adi Suissa-Peleg --- source/common/matcher/matcher.h | 15 +++++++-------- source/common/router/config_impl.cc | 4 ++-- source/common/router/config_utility.cc | 10 +++++----- test/common/router/config_impl_test.cc | 9 +++++++++ 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/source/common/matcher/matcher.h b/source/common/matcher/matcher.h index c6c13479d4953..1b4ef246c1421 100644 --- a/source/common/matcher/matcher.h +++ b/source/common/matcher/matcher.h @@ -32,7 +32,6 @@ template class ActionBase : public Base { static absl::string_view staticTypeUrl() { const static std::string typeUrl(ProtoType().GetTypeName()); - return typeUrl; } }; @@ -166,7 +165,7 @@ class MatchTreeFactory : public OnMatchFactory { case MatcherType::MATCHER_TYPE_NOT_SET: return createAnyMatcher(config); } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } absl::optional> @@ -260,9 +259,9 @@ class MatchTreeFactory : public OnMatchFactory { }; } case PredicateType::MATCH_TYPE_NOT_SET: - PANIC_DUE_TO_PROTO_UNSET; + PANIC_DUE_TO_PROTO_UNSET; // GCOVR_EXCL_LINE } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } template @@ -281,7 +280,7 @@ class MatchTreeFactory : public OnMatchFactory { &PrefixMapMatcher::create); } case MatcherType::MatcherTree::TREE_TYPE_NOT_SET: - PANIC("unexpected matcher type"); + PANIC("unexpected matcher type"); // GCOVR_EXCL_LINE case MatcherType::MatcherTree::kCustomMatch: { auto& factory = Config::Utility::getAndCheckFactory>( matcher.matcher_tree().custom_match()); @@ -292,7 +291,7 @@ class MatchTreeFactory : public OnMatchFactory { on_no_match, *this); } } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } using MapCreationFunction = std::function>>( @@ -360,9 +359,9 @@ class MatchTreeFactory : public OnMatchFactory { return factory.createInputMatcherFactoryCb(*message, server_factory_context_); } case SinglePredicateType::MATCHER_NOT_SET: - PANIC_DUE_TO_PROTO_UNSET; + PANIC_DUE_TO_PROTO_UNSET; // GCOVR_EXCL_LINE } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } const std::string stats_prefix_; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 3cb73f84c30b5..0f201f9b41232 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1995,7 +1995,7 @@ VirtualHostImpl::VirtualHostImpl( shared_virtual_host_ = std::move(host_or_error.value()); switch (virtual_host.require_tls()) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; // GCOVR_EXCL_LINE case envoy::config::route::v3::VirtualHost::NONE: ssl_requirements_ = SslRequirements::None; break; @@ -2112,7 +2112,7 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb return getRouteFromRoutes(cb, headers, stream_info, random_value, action.routes()); } - PANIC("Action in router matcher should be Route or RouteList"); + PANIC("Action in router matcher should be Route or RouteList"); // GCOVR_EXCL_LINE } ENVOY_LOG(debug, "failed to match incoming request: {}", static_cast(match.match_state_)); diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 7d30cf3c4b275..070ab3699bd3f 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -65,7 +65,7 @@ ConfigUtility::parsePriority(const envoy::config::core::v3::RoutingPriority& pri case envoy::config::core::v3::HIGH: return Upstream::ResourcePriority::High; } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } bool ConfigUtility::matchQueryParams( @@ -83,7 +83,7 @@ bool ConfigUtility::matchQueryParams( Http::Code ConfigUtility::parseRedirectResponseCode( const envoy::config::route::v3::RedirectAction::RedirectResponseCode& code) { switch (code) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; // GCOVR_EXCL_LINE case envoy::config::route::v3::RedirectAction::MOVED_PERMANENTLY: return Http::Code::MovedPermanently; case envoy::config::route::v3::RedirectAction::FOUND: @@ -95,7 +95,7 @@ Http::Code ConfigUtility::parseRedirectResponseCode( case envoy::config::route::v3::RedirectAction::PERMANENT_REDIRECT: return Http::Code::PermanentRedirect; } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } absl::optional @@ -111,7 +111,7 @@ ConfigUtility::parseDirectResponseCode(const envoy::config::route::v3::Route& ro Http::Code ConfigUtility::parseClusterNotFoundResponseCode( const envoy::config::route::v3::RouteAction::ClusterNotFoundResponseCode& code) { switch (code) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; // GCOVR_EXCL_LINE case envoy::config::route::v3::RouteAction::SERVICE_UNAVAILABLE: return Http::Code::ServiceUnavailable; case envoy::config::route::v3::RouteAction::NOT_FOUND: @@ -119,7 +119,7 @@ Http::Code ConfigUtility::parseClusterNotFoundResponseCode( case envoy::config::route::v3::RouteAction::INTERNAL_SERVER_ERROR: return Http::Code::InternalServerError; } - PANIC_DUE_TO_CORRUPT_ENUM; + PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE } } // namespace Router diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 8a25c063734f9..1c0bddc3006f2 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -476,6 +476,12 @@ TEST_F(RouteMatcherTest, TestConnectRoutes) { // Header matching (for HTTP/2) EXPECT_EQ("connect_header_match", config.route(genHeaders("bat5.com", " ", "CONNECT"), 0)->routeEntry()->clusterName()); + + // Increase line coverage for the ConnectRouteEntryImpl class. + { + checkPathMatchCriterion(config.route(genHeaders("bat3.com", " ", "CONNECT"), 0).get(), + EMPTY_STRING, PathMatchType::None); + } } TEST_F(RouteMatcherTest, TestRoutes) { @@ -9504,6 +9510,9 @@ TEST_F(RouteConfigurationV2, TemplatePatternIsFilledFromConfigInRouteAction) { const auto& pattern_rewrite_policy = config.route(headers, 0)->routeEntry()->pathRewriter(); EXPECT_TRUE(pattern_rewrite_policy != nullptr); EXPECT_EQ(pattern_rewrite_policy->uriTemplate(), "/bar/{lang}/{country}"); + + checkPathMatchCriterion(config.route(headers, 0).get(), "/bar/{country}/{lang}", + PathMatchType::Template); } TEST_F(RouteMatcherTest, SimplePathPatternMatchOnly) { From d1baf1f5511e2dc59548ec7e0a8351810f013fcd Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Thu, 2 Jan 2025 21:50:52 +0000 Subject: [PATCH 07/13] coverage test fix Signed-off-by: Adi Suissa-Peleg --- test/common/router/config_impl_test.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 1c0bddc3006f2..6f0df3ee99c0f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -344,6 +344,16 @@ most_specific_header_mutations_wins: {0} absl::Status creation_status_ = absl::OkStatus(); }; +void checkPathMatchCriterion(const Route* route, const std::string& expected_matcher, + PathMatchType expected_type) { + ASSERT_NE(nullptr, route); + const auto route_entry = route->routeEntry(); + ASSERT_NE(nullptr, route_entry); + const auto& match_criterion = route_entry->pathMatchCriterion(); + EXPECT_EQ(expected_matcher, match_criterion.matcher()); + EXPECT_EQ(expected_type, match_criterion.matchType()); +} + class RouteMatcherTest : public testing::Test, public ConfigImplTestBase, public TestScopedRuntime {}; @@ -7801,16 +7811,6 @@ TEST_F(RouteConfigurationV2, DirectResponseTooLarge) { EXPECT_EQ(creation_status_.message(), "response body size is 4097 bytes; maximum is 4096"); } -void checkPathMatchCriterion(const Route* route, const std::string& expected_matcher, - PathMatchType expected_type) { - ASSERT_NE(nullptr, route); - const auto route_entry = route->routeEntry(); - ASSERT_NE(nullptr, route_entry); - const auto& match_criterion = route_entry->pathMatchCriterion(); - EXPECT_EQ(expected_matcher, match_criterion.matcher()); - EXPECT_EQ(expected_type, match_criterion.matchType()); -} - // Test loading broken config throws EnvoyException. TEST_F(RouteConfigurationV2, BrokenTypedMetadata) { const std::string yaml = R"EOF( From 05cd33720b1039c6d9a2f465f2e74255892efe8f Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Fri, 3 Jan 2025 15:29:18 +0000 Subject: [PATCH 08/13] removing unnecessary GCOVR_EXCL_LINE as they don't impact coverage Signed-off-by: Adi Suissa-Peleg --- source/common/matcher/matcher.h | 14 +++++++------- source/common/router/config_impl.cc | 4 ++-- source/common/router/config_utility.cc | 10 +++++----- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/source/common/matcher/matcher.h b/source/common/matcher/matcher.h index 1b4ef246c1421..ab906846307cf 100644 --- a/source/common/matcher/matcher.h +++ b/source/common/matcher/matcher.h @@ -165,7 +165,7 @@ class MatchTreeFactory : public OnMatchFactory { case MatcherType::MATCHER_TYPE_NOT_SET: return createAnyMatcher(config); } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } absl::optional> @@ -259,9 +259,9 @@ class MatchTreeFactory : public OnMatchFactory { }; } case PredicateType::MATCH_TYPE_NOT_SET: - PANIC_DUE_TO_PROTO_UNSET; // GCOVR_EXCL_LINE + PANIC_DUE_TO_PROTO_UNSET; } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } template @@ -280,7 +280,7 @@ class MatchTreeFactory : public OnMatchFactory { &PrefixMapMatcher::create); } case MatcherType::MatcherTree::TREE_TYPE_NOT_SET: - PANIC("unexpected matcher type"); // GCOVR_EXCL_LINE + PANIC("unexpected matcher type"); case MatcherType::MatcherTree::kCustomMatch: { auto& factory = Config::Utility::getAndCheckFactory>( matcher.matcher_tree().custom_match()); @@ -291,7 +291,7 @@ class MatchTreeFactory : public OnMatchFactory { on_no_match, *this); } } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } using MapCreationFunction = std::function>>( @@ -359,9 +359,9 @@ class MatchTreeFactory : public OnMatchFactory { return factory.createInputMatcherFactoryCb(*message, server_factory_context_); } case SinglePredicateType::MATCHER_NOT_SET: - PANIC_DUE_TO_PROTO_UNSET; // GCOVR_EXCL_LINE + PANIC_DUE_TO_PROTO_UNSET; } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } const std::string stats_prefix_; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 0f201f9b41232..3cb73f84c30b5 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1995,7 +1995,7 @@ VirtualHostImpl::VirtualHostImpl( shared_virtual_host_ = std::move(host_or_error.value()); switch (virtual_host.require_tls()) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; // GCOVR_EXCL_LINE + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case envoy::config::route::v3::VirtualHost::NONE: ssl_requirements_ = SslRequirements::None; break; @@ -2112,7 +2112,7 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb return getRouteFromRoutes(cb, headers, stream_info, random_value, action.routes()); } - PANIC("Action in router matcher should be Route or RouteList"); // GCOVR_EXCL_LINE + PANIC("Action in router matcher should be Route or RouteList"); } ENVOY_LOG(debug, "failed to match incoming request: {}", static_cast(match.match_state_)); diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 070ab3699bd3f..7d30cf3c4b275 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -65,7 +65,7 @@ ConfigUtility::parsePriority(const envoy::config::core::v3::RoutingPriority& pri case envoy::config::core::v3::HIGH: return Upstream::ResourcePriority::High; } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } bool ConfigUtility::matchQueryParams( @@ -83,7 +83,7 @@ bool ConfigUtility::matchQueryParams( Http::Code ConfigUtility::parseRedirectResponseCode( const envoy::config::route::v3::RedirectAction::RedirectResponseCode& code) { switch (code) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; // GCOVR_EXCL_LINE + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case envoy::config::route::v3::RedirectAction::MOVED_PERMANENTLY: return Http::Code::MovedPermanently; case envoy::config::route::v3::RedirectAction::FOUND: @@ -95,7 +95,7 @@ Http::Code ConfigUtility::parseRedirectResponseCode( case envoy::config::route::v3::RedirectAction::PERMANENT_REDIRECT: return Http::Code::PermanentRedirect; } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } absl::optional @@ -111,7 +111,7 @@ ConfigUtility::parseDirectResponseCode(const envoy::config::route::v3::Route& ro Http::Code ConfigUtility::parseClusterNotFoundResponseCode( const envoy::config::route::v3::RouteAction::ClusterNotFoundResponseCode& code) { switch (code) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; // GCOVR_EXCL_LINE + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case envoy::config::route::v3::RouteAction::SERVICE_UNAVAILABLE: return Http::Code::ServiceUnavailable; case envoy::config::route::v3::RouteAction::NOT_FOUND: @@ -119,7 +119,7 @@ Http::Code ConfigUtility::parseClusterNotFoundResponseCode( case envoy::config::route::v3::RouteAction::INTERNAL_SERVER_ERROR: return Http::Code::InternalServerError; } - PANIC_DUE_TO_CORRUPT_ENUM; // GCOVR_EXCL_LINE + PANIC_DUE_TO_CORRUPT_ENUM; } } // namespace Router From de601d6f62332c83df2162458e884ea8b9da6143 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Tue, 28 Jan 2025 02:09:29 +0000 Subject: [PATCH 09/13] update regex interface Signed-off-by: Adi Suissa-Peleg --- envoy/common/regex.h | 5 ++--- source/common/common/matchers.h | 2 +- source/common/common/regex.h | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/envoy/common/regex.h b/envoy/common/regex.h index 84d3c98cbc035..26449d3a35971 100644 --- a/envoy/common/regex.h +++ b/envoy/common/regex.h @@ -24,10 +24,9 @@ class CompiledMatcher : public Matchers::StringMatcher { absl::string_view substitution) const PURE; /** - * Returns a string representation of the Regex matcher (the pattern to be - * matched). + * Returns the pattern of the Regex matcher. */ - virtual const std::string& stringRepresentation() const PURE; + virtual const std::string& pattern() const PURE; }; using CompiledMatcherPtr = std::unique_ptr; diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 1c2865f76cac5..82783a08d5acb 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -163,7 +163,7 @@ class RegexStringMatcher : public StringMatcher { // StringMatcher bool match(const absl::string_view value) const override { return regex_->match(value); } - const std::string& stringRepresentation() const { return regex_->stringRepresentation(); } + const std::string& stringRepresentation() const { return regex_->pattern(); } private: Regex::CompiledMatcherPtr regex_; diff --git a/source/common/common/regex.h b/source/common/common/regex.h index 5acfae796831d..ac7909a406689 100644 --- a/source/common/common/regex.h +++ b/source/common/common/regex.h @@ -39,7 +39,7 @@ class CompiledGoogleReMatcher : public CompiledMatcher { } // CompiledMatcher - const std::string& stringRepresentation() const override { return regex_.pattern(); } + const std::string& pattern() const override { return regex_.pattern(); } protected: explicit CompiledGoogleReMatcher(const std::string& regex) : regex_(regex, re2::RE2::Quiet) { From aa617fbb50635b6d548e606835ad4995b2bc4fe7 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Tue, 28 Jan 2025 02:24:26 +0000 Subject: [PATCH 10/13] remove unnecessary inheritance Signed-off-by: Adi Suissa-Peleg --- source/common/common/matchers.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 82783a08d5acb..91b47d4ecac04 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -92,13 +92,13 @@ StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtension Server::Configuration::CommonFactoryContext& context); // A matcher for the `exact` StringMatcher. -class ExactStringMatcher : public StringMatcher { +class ExactStringMatcher { public: ExactStringMatcher(absl::string_view exact, bool ignore_case) : exact_(exact), ignore_case_(ignore_case) {} // StringMatcher - bool match(const absl::string_view value) const override { + bool match(const absl::string_view value) const { return ignore_case_ ? absl::EqualsIgnoreCase(value, exact_) : value == exact_; } @@ -110,13 +110,13 @@ class ExactStringMatcher : public StringMatcher { }; // A matcher for the `prefix` StringMatcher. -class PrefixStringMatcher : public StringMatcher { +class PrefixStringMatcher { public: PrefixStringMatcher(absl::string_view prefix, bool ignore_case) : prefix_(prefix), ignore_case_(ignore_case) {} // StringMatcher - bool match(const absl::string_view value) const override { + bool match(const absl::string_view value) const { return ignore_case_ ? absl::StartsWithIgnoreCase(value, prefix_) : absl::StartsWith(value, prefix_); } @@ -130,13 +130,13 @@ class PrefixStringMatcher : public StringMatcher { }; // A matcher for the `suffix` StringMatcher. -class SuffixStringMatcher : public StringMatcher { +class SuffixStringMatcher { public: SuffixStringMatcher(absl::string_view suffix, bool ignore_case) : suffix_(suffix), ignore_case_(ignore_case) {} // StringMatcher - bool match(const absl::string_view value) const override { + bool match(const absl::string_view value) const { return ignore_case_ ? absl::EndsWithIgnoreCase(value, suffix_) : absl::EndsWith(value, suffix_); } @@ -148,7 +148,7 @@ class SuffixStringMatcher : public StringMatcher { }; // A matcher for the `safe_regex` StringMatcher. -class RegexStringMatcher : public StringMatcher { +class RegexStringMatcher { public: // The RegexMatcher can either be from the ::envoy or ::xds type, // and the templated c'tor handles both cases. @@ -161,7 +161,7 @@ class RegexStringMatcher : public StringMatcher { RegexStringMatcher(RegexStringMatcher&& other) { regex_ = std::move(other.regex_); } // StringMatcher - bool match(const absl::string_view value) const override { return regex_->match(value); } + bool match(const absl::string_view value) const { return regex_->match(value); } const std::string& stringRepresentation() const { return regex_->pattern(); } @@ -170,14 +170,14 @@ class RegexStringMatcher : public StringMatcher { }; // A matcher for the `contains` StringMatcher. -class ContainsStringMatcher : public StringMatcher { +class ContainsStringMatcher { public: ContainsStringMatcher(absl::string_view contents, bool ignore_case) : contents_(ignore_case ? absl::AsciiStrToLower(contents) : contents), ignore_case_(ignore_case) {} // StringMatcher - bool match(const absl::string_view value) const override { + bool match(const absl::string_view value) const { return ignore_case_ ? absl::StrContains(absl::AsciiStrToLower(value), contents_) : absl::StrContains(value, contents_); } @@ -197,14 +197,14 @@ class ContainsStringMatcher : public StringMatcher { }; // A matcher for the `custom` StringMatcher. -class CustomStringMatcher : public StringMatcher { +class CustomStringMatcher { public: CustomStringMatcher(const xds::core::v3::TypedExtensionConfig& custom, Server::Configuration::CommonFactoryContext& context) : custom_(getExtensionStringMatcher(custom, context)) {} // StringMatcher - bool match(const absl::string_view value) const override { return custom_->match(value); } + bool match(const absl::string_view value) const { return custom_->match(value); } const std::string& stringRepresentation() const { return EMPTY_STRING; } From b6dd7d5e027da586bc42558285f04d34e2e39ea7 Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Tue, 28 Jan 2025 02:56:09 +0000 Subject: [PATCH 11/13] contrib matcher Signed-off-by: Adi Suissa-Peleg --- contrib/hyperscan/matching/input_matchers/source/matcher.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/hyperscan/matching/input_matchers/source/matcher.h b/contrib/hyperscan/matching/input_matchers/source/matcher.h index 7aac429c122a1..97b2352613052 100644 --- a/contrib/hyperscan/matching/input_matchers/source/matcher.h +++ b/contrib/hyperscan/matching/input_matchers/source/matcher.h @@ -44,7 +44,7 @@ class Matcher : public Envoy::Regex::CompiledMatcher, public Envoy::Matcher::Inp // Envoy::Matcher::InputMatcher bool match(const ::Envoy::Matcher::MatchingDataType& input) override; - const std::string& stringRepresentation() const override { return EMPTY_STRING; } + const std::string& pattern() const override { return EMPTY_STRING; } private: hs_database_t* database_{}; From a3694e310df320b13c49abd369529550d2e9d08e Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Thu, 6 Feb 2025 14:50:54 +0000 Subject: [PATCH 12/13] adding memory tests Signed-off-by: Adi Suissa-Peleg --- test/common/common/matchers_test.cc | 41 +++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index 7a97e21c66a27..23037f8bf65ee 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -453,6 +453,47 @@ TEST_F(StringMatcher, NoMatcherRejected) { fmt::format("Configuration must define a matcher: {}", matcher.DebugString())); } +// Validates the amount of memory that is being used by the different string +// matchers. Requested as part of https://github.com/envoyproxy/envoy/pull/37782. +TEST_F(StringMatcher, Memory) { + const uint32_t matchers_num = 1000; + // Prefix matcher. + { + // Add 1000 Prefix-String Matchers of varying string lengths (1 to 1000). + std::vector all_matchers; + all_matchers.reserve(matchers_num); + Memory::TestUtil::MemoryTest memory_test; + for (uint32_t i = 0; i < matchers_num; ++i) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.set_prefix(std::string(i + 1, 'a')); + all_matchers.emplace_back(Matchers::StringMatcherImpl(matcher, context_)); + } + const size_t prefix_consumed_bytes = memory_test.consumedBytes(); + // The memory constraints were added to ensure that the amount of memory + // used by matchers is carefully analyzed. These constraints can be relaxed + // when additional features are added, but it should be done in a thoughtful manner. + EXPECT_MEMORY_EQ(prefix_consumed_bytes, 530176); + } + // Regex matcher. + { + // Add 1000 Regex-String Matchers of varying string lengths (1 to 1000). + std::vector all_matchers; + all_matchers.reserve(matchers_num); + Memory::TestUtil::MemoryTest memory_test; + for (uint32_t i = 0; i < matchers_num; ++i) { + envoy::type::matcher::v3::StringMatcher matcher; + matcher.mutable_safe_regex()->mutable_google_re2(); + matcher.mutable_safe_regex()->set_regex(std::string(i + 1, 'a')); + all_matchers.emplace_back(Matchers::StringMatcherImpl(matcher, context_)); + } + const size_t regex_consumed_bytes = memory_test.consumedBytes(); + // The memory constraints were added to ensure that the amount of memory + // used by matchers is carefully analyzed. These constraints can be relaxed + // when additional features are added, but it should be done in a thoughtful manner. + EXPECT_MEMORY_EQ(regex_consumed_bytes, 15038016); + } +} + class PathMatcher : public BaseTest {}; TEST_F(PathMatcher, MatchExactPath) { From feb84c13dc28b30f84d169e0ca5fa624f0fda80d Mon Sep 17 00:00:00 2001 From: Adi Suissa-Peleg Date: Thu, 6 Feb 2025 20:24:49 +0000 Subject: [PATCH 13/13] memory tests are now working, but memory estimation is flaky Signed-off-by: Adi Suissa-Peleg --- test/common/common/matchers_test.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/common/common/matchers_test.cc b/test/common/common/matchers_test.cc index 23037f8bf65ee..468cf2fcf6719 100644 --- a/test/common/common/matchers_test.cc +++ b/test/common/common/matchers_test.cc @@ -471,8 +471,10 @@ TEST_F(StringMatcher, Memory) { const size_t prefix_consumed_bytes = memory_test.consumedBytes(); // The memory constraints were added to ensure that the amount of memory // used by matchers is carefully analyzed. These constraints can be relaxed - // when additional features are added, but it should be done in a thoughtful manner. - EXPECT_MEMORY_EQ(prefix_consumed_bytes, 530176); + // when additional features are added, but it should be done in a thoughtful manner. + // Adding 3*8192 bytes because tcmalloc consumption estimation may return + // different values depending on memory alignment. + EXPECT_MEMORY_LE(prefix_consumed_bytes, 530176 + 3 * 8192); } // Regex matcher. { @@ -490,7 +492,9 @@ TEST_F(StringMatcher, Memory) { // The memory constraints were added to ensure that the amount of memory // used by matchers is carefully analyzed. These constraints can be relaxed // when additional features are added, but it should be done in a thoughtful manner. - EXPECT_MEMORY_EQ(regex_consumed_bytes, 15038016); + // Adding 10*8192 bytes because tcmalloc consumption estimation may return + // different values depending on memory alignment. + EXPECT_MEMORY_LE(regex_consumed_bytes, 15038016 + 10 * 8192); } }