Stringmatcher: cleanup temporary code (envoyproxy#33072)

Signed-off-by: Greg Greenway <[email protected]>
ggreenway · Mar 22, 2024 · b68af06 · b68af06
1 parent efae60f
commit b68af06
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 87 deletions.
diff --git 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<StringMatcherExtensionFactory>(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
@@ -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 StringMatcherType = envoy::type::matcher::v3::StringMatcher>
-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 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:
-  // 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
@@ -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<const ::envoy::extensions::string_matcher::lua::v3::Lua&>(
+          untyped_config, context.messageValidationContext().staticValidationVisitor());
+
   absl::StatusOr<std::string> 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<LuaStringMatcherThreadWrapper>(*result, tls);
+  return std::make_unique<LuaStringMatcherThreadWrapper>(*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
@@ -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
@@ -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<ThreadLocal::SlotAllocator>::clear();
-  InjectableSingleton<ThreadLocal::SlotAllocator>::initialize(&thread_local_);
-  InjectableSingleton<Api::Api>::clear();
-  InjectableSingleton<Api::Api>::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<ThreadLocal::SlotAllocator>::clear();
-  InjectableSingleton<Api::Api>::clear();
 }
 
 Upstream::ClusterManager& InstanceBase::clusterManager() {

diff --git 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<Envoy::StreamInfo::MockStreamInfo> stream_info;
   static NiceMock<Server::Configuration::MockServerFactoryContext> factory_context;
   static ScopedInjectableLoader<Regex::Engine> engine(std::make_unique<Regex::GoogleReEngine>());
-  static ScopedInjectableLoader<ThreadLocal::SlotAllocator> tls_inject(
-      std::make_unique<ThreadLocal::MockInstance>());
-  static ScopedInjectableLoader<Api::Api> api_inject(std::make_unique<Api::MockApi>());
 
   try {
     if (!validateConfig(input)) {

diff --git 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
@@ -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<Server::Configuration::MockServerFactoryContext> 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
@@ -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.