Begin process of removing singleton use by StringMatcher (#32829)

Part of #32792

Signed-off-by: Greg Greenway <[email protected]>

contrib/postgres_proxy/filters/network/test/postgres_integration_test.cc

@@ -295,7 +295,8 @@ class UpstreamSSLBaseIntegrationTest : public PostgresBaseIntegrationTest {
     // The tls transport socket will be inserted into fake_upstream when
     // Envoy's upstream starttls transport socket is converted to secure mode.
     std::unique_ptr<Ssl::ContextManager> tls_context_manager =
-        std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(timeSystem());
+        std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(
+            server_factory_context_);
 
     envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext downstream_tls_context;
 
@@ -527,7 +528,8 @@ class UpstreamAndDownstreamSSLIntegrationTest : public UpstreamSSLBaseIntegratio
     // The tls transport socket will be inserted into fake_upstream when
     // Envoy's upstream starttls transport socket is converted to secure mode.
     std::unique_ptr<Ssl::ContextManager> tls_context_manager =
-        std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(timeSystem());
+        std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(
+            server_factory_context_);
 
     envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext upstream_tls_context;
 

envoy/server/factory_context.h

@@ -36,6 +36,11 @@
 #include "source/common/protobuf/protobuf.h"
 
 namespace Envoy {
+
+namespace Regex {
+class Engine;
+}
+
 namespace Server {
 namespace Configuration {
 
@@ -129,6 +134,11 @@ class CommonFactoryContext {
    * @return ServerLifecycleNotifier& the lifecycle notifier for the server.
    */
   virtual ServerLifecycleNotifier& lifecycleNotifier() PURE;
+
+  /**
+   * @return the server regex engine.
+   */
+  virtual Regex::Engine& regexEngine() PURE;
 };
 
 /**

envoy/server/instance.h

@@ -253,6 +253,11 @@ class Instance {
    */
   virtual Configuration::StatsConfig& statsConfig() PURE;
 
+  /**
+   * @return the server regex engine.
+   */
+  virtual Regex::Engine& regexEngine() PURE;
+
   /**
    * @return envoy::config::bootstrap::v3::Bootstrap& the servers bootstrap configuration.
    */

envoy/ssl/context_manager.h

@@ -10,6 +10,13 @@
 #include "envoy/stats/scope.h"
 
 namespace Envoy {
+
+namespace Server {
+namespace Configuration {
+class CommonFactoryContext;
+} // namespace Configuration
+} // namespace Server
+
 namespace Ssl {
 
 // Opaque type defined and used by the ``ServerContext``.
@@ -73,7 +80,8 @@ using ContextManagerPtr = std::unique_ptr<ContextManager>;
 class ContextManagerFactory : public Config::UntypedFactory {
 public:
   ~ContextManagerFactory() override = default;
-  virtual ContextManagerPtr createContextManager(TimeSource& time_source) PURE;
+  virtual ContextManagerPtr
+  createContextManager(Server::Configuration::CommonFactoryContext& factory_context) PURE;
 
   // There could be only one factory thus the name is static.
   std::string name() const override { return "ssl_context_manager"; }

mobile/library/common/extensions/cert_validator/platform_bridge/config.cc

@@ -9,7 +9,7 @@ namespace Tls {
 
 CertValidatorPtr PlatformBridgeCertValidatorFactory::createCertValidator(
     const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
-    TimeSource& /*time_source*/) {
+    Server::Configuration::CommonFactoryContext& /*context*/) {
   return std::make_unique<PlatformBridgeCertValidator>(config, stats);
 }
 

mobile/library/common/extensions/cert_validator/platform_bridge/config.h

@@ -15,8 +15,9 @@ namespace Tls {
 class PlatformBridgeCertValidatorFactory : public CertValidatorFactory,
                                            public Config::TypedFactory {
 public:
-  CertValidatorPtr createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config,
-                                       SslStats& stats, TimeSource& time_source) override;
+  CertValidatorPtr
+  createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
+                      Server::Configuration::CommonFactoryContext& context) override;
 
   std::string name() const override {
     return "envoy_mobile.cert_validator.platform_bridge_cert_validator";

mobile/test/common/integration/BUILD

@@ -184,6 +184,7 @@ envoy_cc_test_library(
         "@envoy//source/exe:process_wide_lib",
         "@envoy//test/integration:autonomous_upstream_lib",
         "@envoy//test/integration:utility_lib",
+        "@envoy//test/mocks/server:server_factory_context_mocks",
         "@envoy//test/mocks/server:transport_socket_factory_context_mocks",
         "@envoy//test/test_common:environment_lib",
         "@envoy_build_config//:extension_registry",
@@ -213,6 +214,7 @@ envoy_cc_test_library(
         "@envoy//source/exe:process_wide_lib",
         "@envoy//test/integration:autonomous_upstream_lib",
         "@envoy//test/integration:utility_lib",
+        "@envoy//test/mocks/server:server_factory_context_mocks",
         "@envoy//test/mocks/server:transport_socket_factory_context_mocks",
         "@envoy//test/test_common:environment_lib",
         "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto",

mobile/test/common/integration/test_server.cc

@@ -25,7 +25,7 @@ namespace Envoy {
 Network::DownstreamTransportSocketFactoryPtr TestServer::createQuicUpstreamTlsContext(
     testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext>& factory_context) {
   envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context;
-  Extensions::TransportSockets::Tls::ContextManagerImpl context_manager{time_system_};
+  Extensions::TransportSockets::Tls::ContextManagerImpl context_manager{server_factory_context_};
   tls_context.mutable_common_tls_context()->add_alpn_protocols("h3");
   envoy::extensions::transport_sockets::tls::v3::TlsCertificate* certs =
       tls_context.mutable_common_tls_context()->add_tls_certificates();

mobile/test/common/integration/test_server.h

@@ -8,6 +8,7 @@
 
 #include "envoy/extensions/transport_sockets/quic/v3/quic_transport.pb.h"
 #include "test/integration/autonomous_upstream.h"
+#include "test/mocks/server/server_factory_context.h"
 #include "test/mocks/server/transport_socket_factory_context.h"
 #include "test/integration/server.h"
 
@@ -26,6 +27,7 @@ enum class TestServerType {
 class TestServer : public ListenerHooks {
 private:
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context_;
+  testing::NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
   Stats::IsolatedStoreImpl stats_store_;
   Event::GlobalTimeSystem time_system_;
   Api::ApiPtr api_;
@@ -35,7 +37,7 @@ class TestServer : public ListenerHooks {
   Thread::SkipAsserts skip_asserts_;
   ProcessWide process_wide;
   Thread::MutexBasicLockable lock;
-  Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{time_system_};
+  Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{server_factory_context_};
   std::unique_ptr<bazel::tools::cpp::runfiles::Runfiles> runfiles_;
 
   // Either test_server_ will be set for test_server_type is a proxy, otherwise upstream_ will be

mobile/test/common/integration/xds_test_server.h

@@ -7,6 +7,7 @@
 
 #include "test/integration/fake_upstream.h"
 #include "test/integration/server.h"
+#include "test/mocks/server/server_factory_context.h"
 #include "test/mocks/server/transport_socket_factory_context.h"
 #include "test/test_common/test_time.h"
 
@@ -36,6 +37,7 @@ class XdsTestServer {
 
 private:
   testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context_;
+  testing::NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
   Stats::IsolatedStoreImpl stats_store_;
   Event::GlobalTimeSystem time_system_;
   Api::ApiPtr api_;
@@ -44,7 +46,7 @@ class XdsTestServer {
   Event::DispatcherPtr dispatcher_;
   FakeUpstreamConfig upstream_config_;
   Thread::MutexBasicLockable lock_;
-  Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{time_system_};
+  Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{server_factory_context_};
   std::unique_ptr<bazel::tools::cpp::runfiles::Runfiles> runfiles_;
   std::unique_ptr<FakeUpstream> xds_upstream_;
   FakeHttpConnectionPtr xds_connection_;

mobile/test/common/integration/xds_test_server_interface.cc

@@ -12,6 +12,10 @@ static std::weak_ptr<Envoy::XdsTestServer> weak_test_server_;
 static std::shared_ptr<Envoy::XdsTestServer> testServer() { return weak_test_server_.lock(); }
 
 void initXdsServer() {
+  // This is called via JNI from kotlin tests, and Envoy doesn't consider it a test thread
+  // which triggers some failures of `ASSERT_IS_MAIN_OR_TEST_THREAD()`.
+  Envoy::Thread::SkipAsserts skip;
+
   Envoy::ExtensionRegistry::registerFactories();
   strong_test_server_ = std::make_shared<Envoy::XdsTestServer>();
   weak_test_server_ = strong_test_server_;

source/common/common/matchers.cc

@@ -201,9 +201,10 @@ bool PathMatcher::match(const absl::string_view path) const {
   return matcher_.match(Http::PathUtil::removeQueryAndFragment(path));
 }
 
-StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config) {
+StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config,
+                                           ThreadLocal::SlotAllocator& tls, Api::Api& api) {
   auto factory = Config::Utility::getAndCheckFactory<StringMatcherExtensionFactory>(config, false);
-  return factory->createStringMatcher(config.typed_config());
+  return factory->createStringMatcher(config.typed_config(), tls, api);
 }
 
 } // namespace Matchers

source/common/common/matchers.h

@@ -86,24 +86,36 @@ class UniversalStringMatcher : public StringMatcher {
   bool match(absl::string_view) const override { return true; }
 };
 
-StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config);
+StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config,
+                                           ThreadLocal::SlotAllocator& tls, Api::Api& api);
 
 template <class StringMatcherType = envoy::type::matcher::v3::StringMatcher>
-class StringMatcherImpl : public ValueMatcher, public StringMatcher {
+class PrivateStringMatcherImpl : public ValueMatcher, public StringMatcher {
 public:
-  explicit StringMatcherImpl(const StringMatcherType& matcher) : matcher_(matcher) {
+  // 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)
+      : 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_ = Regex::Utility::parseRegex(matcher_.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());
+      }
     } 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 {
-      initialize(matcher);
+      initialize(matcher, tls, api);
     }
   }
 
@@ -143,11 +155,13 @@ class StringMatcherImpl : public ValueMatcher, public StringMatcher {
   // overloading to only handle that case for type `envoy::type::matcher::v3::StringMatcher` to
   // prevent compilation errors on use of `kCustom`.
 
-  void initialize(const xds::type::matcher::v3::StringMatcher&) {}
+  void initialize(const xds::type::matcher::v3::StringMatcher&, ThreadLocal::SlotAllocator*,
+                  Api::Api*) {}
 
-  void initialize(const envoy::type::matcher::v3::StringMatcher& matcher) {
+  void initialize(const envoy::type::matcher::v3::StringMatcher& matcher,
+                  ThreadLocal::SlotAllocator* tls, Api::Api* api) {
     if (matcher.has_custom()) {
-      custom_ = getExtensionStringMatcher(matcher.custom());
+      custom_ = getExtensionStringMatcher(matcher.custom(), *tls, *api);
     }
   }
 
@@ -193,9 +207,34 @@ class StringMatcherImpl : 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 StringMatcherType = envoy::type::matcher::v3::StringMatcher>
+class StringMatcherImplWithContext : public PrivateStringMatcherImpl<StringMatcherType> {
+public:
+  explicit StringMatcherImplWithContext(const StringMatcherType& matcher,
+                                        Server::Configuration::CommonFactoryContext& context)
+      : PrivateStringMatcherImpl<StringMatcherType>(matcher, &context.regexEngine(),
+                                                    &context.threadLocal(), &context.api()) {}
+};
+
+template <class StringMatcherType = envoy::type::matcher::v3::StringMatcher>
+class StringMatcherImpl : public PrivateStringMatcherImpl<StringMatcherType> {
+public:
+  explicit StringMatcherImpl(const StringMatcherType& matcher)
+      : PrivateStringMatcherImpl<StringMatcherType>(
+            matcher, Regex::EngineSingleton::getExisting(),
+            InjectableSingleton<ThreadLocal::SlotAllocator>::getExisting(),
+            InjectableSingleton<Api::Api>::getExisting()) {}
+};
+
 class StringMatcherExtensionFactory : public Config::TypedFactory {
 public:
-  virtual StringMatcherPtr createStringMatcher(const ProtobufWkt::Any& config) PURE;
+  // TODO(ggreenway): Convert all but first parameter to `CommonFactoryContext`.
+  virtual StringMatcherPtr createStringMatcher(const ProtobufWkt::Any& config,
+                                               ThreadLocal::SlotAllocator& tls, Api::Api& api) PURE;
 
   std::string category() const override { return "envoy.string_matcher"; }
 };

source/common/common/regex.h

@@ -79,6 +79,16 @@ class Utility {
 
     return EngineSingleton::get().matcher(matcher.regex());
   }
+
+  template <class RegexMatcherType>
+  static CompiledMatcherPtr parseRegex(const RegexMatcherType& matcher, Engine& engine) {
+    // Fallback deprecated engine type in regex matcher.
+    if (matcher.has_google_re2()) {
+      return std::make_unique<CompiledGoogleReMatcher>(matcher);
+    }
+
+    return engine.matcher(matcher.regex());
+  }
 };
 
 } // namespace Regex

source/common/tls/cert_validator/default_validator.cc

@@ -44,8 +44,8 @@ namespace Tls {
 
 DefaultCertValidator::DefaultCertValidator(
     const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
-    TimeSource& time_source)
-    : config_(config), stats_(stats), time_source_(time_source) {
+    Server::Configuration::CommonFactoryContext& context)
+    : config_(config), stats_(stats), context_(context) {
   if (config_ != nullptr) {
     allow_untrusted_certificate_ = config_->trustChainVerification() ==
                                    envoy::extensions::transport_sockets::tls::v3::
@@ -155,7 +155,7 @@ int DefaultCertValidator::initializeSslContexts(std::vector<SSL_CTX*> contexts,
     if (!cert_validation_config->subjectAltNameMatchers().empty()) {
       for (const envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher& matcher :
            cert_validation_config->subjectAltNameMatchers()) {
-        auto san_matcher = createStringSanMatcher(matcher);
+        auto san_matcher = createStringSanMatcher(matcher, context_);
         if (san_matcher == nullptr) {
           throwEnvoyExceptionOrPanic(
               absl::StrCat("Failed to create string SAN matcher of type ", matcher.san_type()));
@@ -548,18 +548,19 @@ Envoy::Ssl::CertificateDetailsPtr DefaultCertValidator::getCaCertInformation() c
   if (ca_cert_ == nullptr) {
     return nullptr;
   }
-  return Utility::certificateDetails(ca_cert_.get(), getCaFileName(), time_source_);
+  return Utility::certificateDetails(ca_cert_.get(), getCaFileName(), context_.timeSource());
 }
 
 absl::optional<uint32_t> DefaultCertValidator::daysUntilFirstCertExpires() const {
-  return Utility::getDaysUntilExpiration(ca_cert_.get(), time_source_);
+  return Utility::getDaysUntilExpiration(ca_cert_.get(), context_.timeSource());
 }
 
 class DefaultCertValidatorFactory : public CertValidatorFactory {
 public:
-  CertValidatorPtr createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config,
-                                       SslStats& stats, TimeSource& time_source) override {
-    return std::make_unique<DefaultCertValidator>(config, stats, time_source);
+  CertValidatorPtr
+  createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
+                      Server::Configuration::CommonFactoryContext& context) override {
+    return std::make_unique<DefaultCertValidator>(config, stats, context);
   }
 
   std::string name() const override { return "envoy.tls.cert_validator.default"; }

source/common/tls/cert_validator/default_validator.h

@@ -35,7 +35,7 @@ namespace Tls {
 class DefaultCertValidator : public CertValidator, Logger::Loggable<Logger::Id::connection> {
 public:
   DefaultCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config,
-                       SslStats& stats, TimeSource& time_source);
+                       SslStats& stats, Server::Configuration::CommonFactoryContext& context);
 
   ~DefaultCertValidator() override = default;
 
@@ -110,7 +110,7 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable<Logger::Id::
 
   const Envoy::Ssl::CertificateValidationContextConfig* config_;
   SslStats& stats_;
-  TimeSource& time_source_;
+  Server::Configuration::CommonFactoryContext& context_;
 
   bool allow_untrusted_certificate_{false};
   bssl::UniquePtr<X509> ca_cert_;

source/common/tls/cert_validator/factory.h

@@ -21,7 +21,7 @@ class CertValidatorFactory : public Config::UntypedFactory {
 public:
   virtual CertValidatorPtr
   createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
-                      TimeSource& time_source) PURE;
+                      Server::Configuration::CommonFactoryContext& context) PURE;
 
   std::string category() const override { return "envoy.tls.cert_validator"; }
 };