diff --git a/source/common/common/matchers.cc b/source/common/common/matchers.cc index 4b4447d2224d..eef8c3ce122d 100644 --- a/source/common/common/matchers.cc +++ b/source/common/common/matchers.cc @@ -220,9 +220,11 @@ bool PathMatcher::match(const absl::string_view path) const { } StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config, - ThreadLocal::SlotAllocator& tls, Api::Api& api) { + Server::Configuration::CommonFactoryContext& context) { auto factory = Config::Utility::getAndCheckFactory(config, false); - return factory->createStringMatcher(config.typed_config(), tls, api); + ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig( + config, context.messageValidationVisitor(), *factory); + return factory->createStringMatcher(*message, context); } } // namespace Matchers diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 0a1bc5f8b846..c8c64f63d69b 100644 --- a/source/common/common/matchers.h +++ b/source/common/common/matchers.h @@ -88,35 +88,26 @@ class UniversalStringMatcher : public StringMatcher { }; StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config, - ThreadLocal::SlotAllocator& tls, Api::Api& api); + Server::Configuration::CommonFactoryContext& context); template -class PrivateStringMatcherImpl : public ValueMatcher, public StringMatcher { +class StringMatcherImplWithContext : public ValueMatcher, public StringMatcher { public: - // TODO(ggreenway): convert all but the first parameter into - // `Server::Configuration::CommonFactoryContext`. - explicit PrivateStringMatcherImpl(const StringMatcherType& matcher, Regex::Engine* regex_engine, - ThreadLocal::SlotAllocator* tls, Api::Api* api) + explicit StringMatcherImplWithContext(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."); } - if (regex_engine != nullptr) { - regex_ = Regex::Utility::parseRegex(matcher_.safe_regex(), *regex_engine); - } else { - // TODO(ggreenway): remove this branch when we always have an engine. This is only - // needed to make tests not complain about dereferencing a null pointer, even though - // the reference isn't actually used. - regex_ = Regex::Utility::parseRegex(matcher_.safe_regex()); - } + regex_ = Regex::Utility::parseRegex(matcher_.safe_regex(), context.regexEngine()); } 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(), *tls, *api); + custom_ = getExtensionStringMatcher(matcher.custom(), context); } } @@ -180,34 +171,11 @@ class PrivateStringMatcherImpl : public ValueMatcher, public StringMatcher { StringMatcherPtr custom_; }; -// Temporarily create two separate types with different constructors, inheriting from the same -// implementation, to make it easier to find and replace all usage of the old one. -// TODO(ggreenway): delete these two extra classes, make `PrivateStringMatcherImpl` back into -// `StringMatcherImpl`. -template -class StringMatcherImplWithContext : public PrivateStringMatcherImpl { -public: - explicit StringMatcherImplWithContext(const StringMatcherType& matcher, - Server::Configuration::CommonFactoryContext& context) - : PrivateStringMatcherImpl(matcher, &context.regexEngine(), - &context.threadLocal(), &context.api()) {} -}; - -template -class StringMatcherImpl : public PrivateStringMatcherImpl { -public: - explicit StringMatcherImpl(const StringMatcherType& matcher) - : PrivateStringMatcherImpl( - matcher, Regex::EngineSingleton::getExisting(), - InjectableSingleton::getExisting(), - InjectableSingleton::getExisting()) {} -}; - class StringMatcherExtensionFactory : public Config::TypedFactory { public: - // TODO(ggreenway): Convert all but first parameter to `CommonFactoryContext`. - virtual StringMatcherPtr createStringMatcher(const ProtobufWkt::Any& config, - ThreadLocal::SlotAllocator& tls, Api::Api& api) PURE; + virtual StringMatcherPtr + createStringMatcher(const Protobuf::Message& config, + Server::Configuration::CommonFactoryContext& context) PURE; std::string category() const override { return "envoy.string_matcher"; } }; diff --git a/source/extensions/string_matcher/lua/match.cc b/source/extensions/string_matcher/lua/match.cc index c6ee81e7022c..58a26430f8f2 100644 --- a/source/extensions/string_matcher/lua/match.cc +++ b/source/extensions/string_matcher/lua/match.cc @@ -1,6 +1,7 @@ #include "source/extensions/string_matcher/lua/match.h" #include "envoy/extensions/string_matcher/lua/v3/lua.pb.h" +#include "envoy/extensions/string_matcher/lua/v3/lua.pb.validate.h" #include "source/common/config/datasource.h" #include "source/common/config/utility.h" @@ -94,18 +95,19 @@ class LuaStringMatcherThreadWrapper : public Matchers::StringMatcher { }; Matchers::StringMatcherPtr -LuaStringMatcherFactory::createStringMatcher(const ProtobufWkt::Any& message, - ThreadLocal::SlotAllocator& tls, Api::Api& api) { - ::envoy::extensions::string_matcher::lua::v3::Lua config; - Config::Utility::translateOpaqueConfig(message, ProtobufMessage::getStrictValidationVisitor(), - config); +LuaStringMatcherFactory::createStringMatcher(const Protobuf::Message& untyped_config, + Server::Configuration::CommonFactoryContext& context) { + const auto& config = + MessageUtil::downcastAndValidate( + untyped_config, context.messageValidationContext().staticValidationVisitor()); + absl::StatusOr result = Config::DataSource::read( - config.source_code(), false /* allow_empty */, api, 0 /* max_size */); + config.source_code(), false /* allow_empty */, context.api(), 0 /* max_size */); if (!result.ok()) { throw EnvoyException( fmt::format("Failed to get lua string matcher code from source: {}", result.status())); } - return std::make_unique(*result, tls); + return std::make_unique(*result, context.threadLocal()); } ProtobufTypes::MessagePtr LuaStringMatcherFactory::createEmptyConfigProto() { diff --git a/source/extensions/string_matcher/lua/match.h b/source/extensions/string_matcher/lua/match.h index 2777bcf76220..436ea36c270b 100644 --- a/source/extensions/string_matcher/lua/match.h +++ b/source/extensions/string_matcher/lua/match.h @@ -29,9 +29,9 @@ class LuaStringMatcher : public Matchers::StringMatcher, public ThreadLocal::Thr class LuaStringMatcherFactory : public Matchers::StringMatcherExtensionFactory { public: - Matchers::StringMatcherPtr createStringMatcher(const ProtobufWkt::Any& message, - ThreadLocal::SlotAllocator& tls, - Api::Api& api) override; + Matchers::StringMatcherPtr + createStringMatcher(const Protobuf::Message& config, + Server::Configuration::CommonFactoryContext& context) override; std::string name() const override { return "envoy.string_matcher.lua"; } ProtobufTypes::MessagePtr createEmptyConfigProto() override; }; diff --git a/source/server/server.cc b/source/server/server.cc index 148dc1f55f77..065aebdb6b6a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -106,20 +106,7 @@ InstanceBase::InstanceBase(Init::Manager& init_manager, const Options& options, grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), quic_stat_names_(store.symbolTable()), server_contexts_(*this), - enable_reuse_port_default_(true), stats_flush_in_progress_(false) { - - // These are needed for string matcher extensions. It is too painful to pass these objects through - // all call chains that construct a `StringMatcherImpl`, so these are singletons. - // - // They must be cleared before being set to make the multi-envoy integration test pass. Note that - // this means that extensions relying on these singletons probably will not function correctly in - // some Envoy mobile setups where multiple Envoy engines are used in the same process. The same - // caveat also applies to a few other singletons, such as the global regex engine. - InjectableSingleton::clear(); - InjectableSingleton::initialize(&thread_local_); - InjectableSingleton::clear(); - InjectableSingleton::initialize(api_.get()); -} + enable_reuse_port_default_(true), stats_flush_in_progress_(false) {} InstanceBase::~InstanceBase() { terminate(); @@ -151,9 +138,6 @@ InstanceBase::~InstanceBase() { close(tracing_fd_); } #endif - - InjectableSingleton::clear(); - InjectableSingleton::clear(); } Upstream::ClusterManager& InstanceBase::clusterManager() { diff --git a/test/common/router/route_fuzz_test.cc b/test/common/router/route_fuzz_test.cc index 184795a864d6..b4488b50ef7d 100644 --- a/test/common/router/route_fuzz_test.cc +++ b/test/common/router/route_fuzz_test.cc @@ -140,9 +140,6 @@ DEFINE_PROTO_FUZZER(const test::common::router::RouteTestCase& input) { static NiceMock stream_info; static NiceMock factory_context; static ScopedInjectableLoader engine(std::make_unique()); - static ScopedInjectableLoader tls_inject( - std::make_unique()); - static ScopedInjectableLoader api_inject(std::make_unique()); try { if (!validateConfig(input)) { diff --git a/test/extensions/string_matcher/lua/BUILD b/test/extensions/string_matcher/lua/BUILD index b8688ea5de38..eefbc19aa297 100644 --- a/test/extensions/string_matcher/lua/BUILD +++ b/test/extensions/string_matcher/lua/BUILD @@ -17,8 +17,7 @@ envoy_extension_cc_test( extension_names = ["envoy.string_matcher.lua"], deps = [ "//source/extensions/string_matcher/lua:config", - "//test/mocks/api:api_mocks", - "//test/mocks/thread_local:thread_local_mocks", + "//test/mocks/server:server_factory_context_mocks", "//test/test_common:logging_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/extensions/string_matcher/lua/v3:pkg_cc_proto", diff --git a/test/extensions/string_matcher/lua/lua_test.cc b/test/extensions/string_matcher/lua/lua_test.cc index 3842b89f3469..cf368ec4a8ff 100644 --- a/test/extensions/string_matcher/lua/lua_test.cc +++ b/test/extensions/string_matcher/lua/lua_test.cc @@ -2,8 +2,7 @@ #include "source/extensions/string_matcher/lua/match.h" -#include "test/mocks/api/mocks.h" -#include "test/mocks/thread_local/mocks.h" +#include "test/mocks/server/server_factory_context.h" #include "test/test_common/logging.h" #include "test/test_common/utility.h" @@ -88,20 +87,16 @@ TEST(LuaStringMatcher, LuaStdLib) { } TEST(LuaStringMatcher, NoCode) { - Api::MockApi api; - ThreadLocal::MockInstance tls; + NiceMock context; LuaStringMatcherFactory factory; ::envoy::extensions::string_matcher::lua::v3::Lua empty_config; - ProtobufWkt::Any any; - any.PackFrom(empty_config); - EXPECT_THROW_WITH_MESSAGE(factory.createStringMatcher(any, tls, api), EnvoyException, - "Failed to get lua string matcher code from source: INVALID_ARGUMENT: " - "Unexpected DataSource::specifier_case(): 0"); + EXPECT_THROW_WITH_MESSAGE( + factory.createStringMatcher(empty_config, context), EnvoyException, + "Proto constraint validation failed (LuaValidationError.SourceCode: value is required): "); empty_config.mutable_source_code()->set_inline_string(""); - any.PackFrom(empty_config); - EXPECT_THROW_WITH_MESSAGE(factory.createStringMatcher(any, tls, api), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(factory.createStringMatcher(empty_config, context), EnvoyException, "Failed to get lua string matcher code from source: INVALID_ARGUMENT: " "DataSource cannot be empty"); } diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index 108ccdc5f239..83d284cb94b1 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -7,7 +7,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/common/api:84.5" # flaky due to posix: be careful adjusting "source/common/api/posix:83.8" # flaky (accept failover non-deterministic): be careful adjusting "source/common/common/posix:88.8" # No easy way to test pthread_create failure. -"source/common/config:95.8" +"source/common/config:95.6" "source/common/crypto:95.5" "source/common/event:95.1" # Emulated edge events guards don't report LCOV "source/common/filesystem/posix:96.2" # FileReadToEndNotReadable fails in some env; createPath can't test all failure branches.