http: moving HttpServerPropertiesCacheManager from custom singleton (e…

…nvoyproxy#35503)

HttpServerPropertiesCacheManager was created in a weird way using the
singleton manager and creating at whichever call site instantiated it
first. Changing to be a proper server-wide manager as most managers are.

Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a

---------

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

envoy/http/http_server_properties_cache.h

@@ -199,18 +199,5 @@ class HttpServerPropertiesCacheManager {
 
 using HttpServerPropertiesCacheManagerSharedPtr = std::shared_ptr<HttpServerPropertiesCacheManager>;
 
-/**
- * Factory for getting an alternate protocols cache manager.
- */
-class HttpServerPropertiesCacheManagerFactory {
-public:
-  virtual ~HttpServerPropertiesCacheManagerFactory() = default;
-
-  /**
-   * Get the alternate protocols cache manager.
-   */
-  virtual HttpServerPropertiesCacheManagerSharedPtr get() PURE;
-};
-
 } // namespace Http
 } // namespace Envoy

envoy/server/BUILD

@@ -181,6 +181,7 @@ envoy_cc_library(
         "//envoy/http:codes_interface",
         "//envoy/http:context_interface",
         "//envoy/http:filter_interface",
+        "//envoy/http:http_server_properties_cache_interface",
         "//envoy/init:manager_interface",
         "//envoy/local_info:local_info_interface",
         "//envoy/network:drain_decision_interface",

envoy/server/factory_context.h

@@ -13,6 +13,7 @@
 #include "envoy/http/codes.h"
 #include "envoy/http/context.h"
 #include "envoy/http/filter.h"
+#include "envoy/http/http_server_properties_cache.h"
 #include "envoy/init/manager.h"
 #include "envoy/network/drain_decision.h"
 #include "envoy/network/filter.h"
@@ -120,6 +121,11 @@ class CommonFactoryContext {
    */
   virtual Upstream::ClusterManager& clusterManager() PURE;
 
+  /**
+   * @return const Http::HttpServerPropertiesCacheManager& instance for use by the entire server.
+   */
+  virtual Http::HttpServerPropertiesCacheManager& httpServerPropertiesCacheManager() PURE;
+
   /**
    * @return TimeSource& a reference to the time source.
    */

envoy/server/instance.h

@@ -13,6 +13,7 @@
 #include "envoy/event/timer.h"
 #include "envoy/grpc/context.h"
 #include "envoy/http/context.h"
+#include "envoy/http/http_server_properties_cache.h"
 #include "envoy/init/manager.h"
 #include "envoy/local_info/local_info.h"
 #include "envoy/network/listen_socket.h"
@@ -71,6 +72,11 @@ class Instance {
    */
   virtual const Upstream::ClusterManager& clusterManager() const PURE;
 
+  /**
+   * @return const Http::HttpServerPropertiesCacheManager& instance for use by the entire server.
+   */
+  virtual Http::HttpServerPropertiesCacheManager& httpServerPropertiesCacheManager() PURE;
+
   /**
    * @return Ssl::ContextManager& singleton for use by the entire server.
    */

source/common/http/http_server_properties_cache_manager_impl.cc

@@ -13,11 +13,10 @@
 namespace Envoy {
 namespace Http {
 
-SINGLETON_MANAGER_REGISTRATION(alternate_protocols_cache_manager);
-
 HttpServerPropertiesCacheManagerImpl::HttpServerPropertiesCacheManagerImpl(
-    AlternateProtocolsData& data, ThreadLocal::SlotAllocator& tls)
-    : data_(data), slot_(tls) {
+    Server::Configuration::ServerFactoryContext& context,
+    ProtobufMessage::ValidationVisitor& validation_visitor, ThreadLocal::SlotAllocator& tls)
+    : data_(context, validation_visitor), slot_(tls) {
   slot_.set([](Event::Dispatcher& /*dispatcher*/) { return std::make_shared<State>(); });
 }
 
@@ -74,11 +73,5 @@ HttpServerPropertiesCacheSharedPtr HttpServerPropertiesCacheManagerImpl::getCach
   return new_cache;
 }
 
-HttpServerPropertiesCacheManagerSharedPtr HttpServerPropertiesCacheManagerFactoryImpl::get() {
-  return singleton_manager_.getTyped<HttpServerPropertiesCacheManager>(
-      SINGLETON_MANAGER_REGISTERED_NAME(alternate_protocols_cache_manager),
-      [this] { return std::make_shared<HttpServerPropertiesCacheManagerImpl>(data_, tls_); });
-}
-
 } // namespace Http
 } // namespace Envoy

source/common/http/http_server_properties_cache_manager_impl.h

@@ -27,7 +27,8 @@ struct AlternateProtocolsData {
 class HttpServerPropertiesCacheManagerImpl : public HttpServerPropertiesCacheManager,
                                              public Singleton::Instance {
 public:
-  HttpServerPropertiesCacheManagerImpl(AlternateProtocolsData& data,
+  HttpServerPropertiesCacheManagerImpl(Server::Configuration::ServerFactoryContext& context,
+                                       ProtobufMessage::ValidationVisitor& validation_visitor,
                                        ThreadLocal::SlotAllocator& tls);
 
   // HttpServerPropertiesCacheManager
@@ -52,26 +53,11 @@ class HttpServerPropertiesCacheManagerImpl : public HttpServerPropertiesCacheMan
     absl::flat_hash_map<std::string, CacheWithOptions> caches_;
   };
 
-  AlternateProtocolsData& data_;
+  AlternateProtocolsData data_;
 
   // Thread local state for the cache.
   ThreadLocal::TypedSlot<State> slot_;
 };
 
-class HttpServerPropertiesCacheManagerFactoryImpl : public HttpServerPropertiesCacheManagerFactory {
-public:
-  HttpServerPropertiesCacheManagerFactoryImpl(Singleton::Manager& singleton_manager,
-                                              ThreadLocal::SlotAllocator& tls,
-                                              AlternateProtocolsData data)
-      : singleton_manager_(singleton_manager), tls_(tls), data_(data) {}
-
-  HttpServerPropertiesCacheManagerSharedPtr get() override;
-
-private:
-  Singleton::Manager& singleton_manager_;
-  ThreadLocal::SlotAllocator& tls_;
-  AlternateProtocolsData data_;
-};
-
 } // namespace Http
 } // namespace Envoy

source/common/upstream/cluster_manager_impl.cc

@@ -2215,8 +2215,8 @@ Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool(
   Http::HttpServerPropertiesCacheSharedPtr alternate_protocols_cache;
   if (alternate_protocol_options.has_value()) {
     // If there is configuration for an alternate protocols cache, always create one.
-    alternate_protocols_cache = alternate_protocols_cache_manager_->getCache(
-        alternate_protocol_options.value(), dispatcher);
+    alternate_protocols_cache =
+        alternate_protocols_cache_manager_.getCache(alternate_protocol_options.value(), dispatcher);
   } else if (!alternate_protocol_options.has_value() &&
              (protocols.size() == 2 ||
               (protocols.size() == 1 && protocols[0] == Http::Protocol::Http2))) {
@@ -2227,7 +2227,7 @@ Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool(
     envoy::config::core::v3::AlternateProtocolsCacheOptions default_options;
     default_options.set_name(host->cluster().name());
     alternate_protocols_cache =
-        alternate_protocols_cache_manager_->getCache(default_options, dispatcher);
+        alternate_protocols_cache_manager_.getCache(default_options, dispatcher);
   }
 
   absl::optional<Http::HttpServerPropertiesCache::Origin> origin =
@@ -2259,7 +2259,7 @@ Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool(
       envoy::config::core::v3::AlternateProtocolsCacheOptions default_options;
       default_options.set_name(host->cluster().name());
       alternate_protocols_cache =
-          alternate_protocols_cache_manager_->getCache(default_options, dispatcher);
+          alternate_protocols_cache_manager_.getCache(default_options, dispatcher);
     }
 
     ASSERT(contains(protocols, {Http::Protocol::Http11, Http::Protocol::Http2}));

source/common/upstream/cluster_manager_impl.h

@@ -61,9 +61,7 @@ class ProdClusterManagerFactory : public ClusterManagerFactory {
       : context_(context), stats_(stats), tls_(tls), http_context_(http_context),
         dns_resolver_fn_(dns_resolver_fn), ssl_context_manager_(ssl_context_manager),
         secret_manager_(secret_manager), quic_stat_names_(quic_stat_names),
-        alternate_protocols_cache_manager_factory_(context.singletonManager(), tls,
-                                                   {context, context.messageValidationVisitor()}),
-        alternate_protocols_cache_manager_(alternate_protocols_cache_manager_factory_.get()),
+        alternate_protocols_cache_manager_(context.httpServerPropertiesCacheManager()),
         server_(server) {}
 
   // Upstream::ClusterManagerFactory
@@ -104,8 +102,7 @@ class ProdClusterManagerFactory : public ClusterManagerFactory {
   Ssl::ContextManager& ssl_context_manager_;
   Secret::SecretManager& secret_manager_;
   Quic::QuicStatNames& quic_stat_names_;
-  Http::HttpServerPropertiesCacheManagerFactoryImpl alternate_protocols_cache_manager_factory_;
-  Http::HttpServerPropertiesCacheManagerSharedPtr alternate_protocols_cache_manager_;
+  Http::HttpServerPropertiesCacheManager& alternate_protocols_cache_manager_;
   Server::Instance& server_;
 };
 

source/extensions/filters/http/alternate_protocols_cache/config.cc

@@ -18,12 +18,9 @@ Http::FilterFactoryCb AlternateProtocolsCacheFilterFactory::createFilterFactoryF
 
   auto& server_context = context.serverFactoryContext();
 
-  Http::HttpServerPropertiesCacheManagerFactoryImpl alternate_protocol_cache_manager_factory(
-      server_context.singletonManager(), server_context.threadLocal(),
-      {context.serverFactoryContext(), context.messageValidationVisitor()});
-  FilterConfigSharedPtr filter_config(
-      std::make_shared<FilterConfig>(proto_config, alternate_protocol_cache_manager_factory,
-                                     server_context.mainThreadDispatcher().timeSource()));
+  FilterConfigSharedPtr filter_config(std::make_shared<FilterConfig>(
+      proto_config, context.serverFactoryContext().httpServerPropertiesCacheManager(),
+      server_context.mainThreadDispatcher().timeSource()));
 
   return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
     callbacks.addStreamEncoderFilter(

source/extensions/filters/http/alternate_protocols_cache/filter.cc

@@ -19,10 +19,8 @@ using CustomClusterType = envoy::config::cluster::v3::Cluster::CustomClusterType
 
 FilterConfig::FilterConfig(
     const envoy::extensions::filters::http::alternate_protocols_cache::v3::FilterConfig& config,
-    Http::HttpServerPropertiesCacheManagerFactory& alternate_protocol_cache_manager_factory,
-    TimeSource& time_source)
-    : alternate_protocol_cache_manager_(alternate_protocol_cache_manager_factory.get()),
-      time_source_(time_source) {
+    Http::HttpServerPropertiesCacheManager& cache_manager, TimeSource& time_source)
+    : alternate_protocol_cache_manager_(cache_manager), time_source_(time_source) {
   if (config.has_alternate_protocols_cache_options()) {
     ENVOY_LOG_MISC(warn, "Using deprecated and ignored alternate_protocols_cache_options in "
                          "alternate_protocols_cache config.");
@@ -42,7 +40,7 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers
   Http::HttpServerPropertiesCacheSharedPtr cache;
   auto info = encoder_callbacks_->streamInfo().upstreamClusterInfo();
   if (info && (*info)->alternateProtocolsCacheOptions()) {
-    cache = config_->alternateProtocolCacheManager()->getCache(
+    cache = config_->alternateProtocolCacheManager().getCache(
         *((*info)->alternateProtocolsCacheOptions()), dispatcher_);
   }
   if (!cache) {

source/extensions/filters/http/alternate_protocols_cache/filter.h

@@ -16,19 +16,17 @@ namespace AlternateProtocolsCache {
  */
 class FilterConfig {
 public:
-  FilterConfig(
-      const envoy::extensions::filters::http::alternate_protocols_cache::v3::FilterConfig&
-          proto_config,
-      Http::HttpServerPropertiesCacheManagerFactory& alternate_protocol_cache_manager_factory,
-      TimeSource& time_source);
+  FilterConfig(const envoy::extensions::filters::http::alternate_protocols_cache::v3::FilterConfig&
+                   proto_config,
+               Http::HttpServerPropertiesCacheManager& cache_manager, TimeSource& time_source);
 
   TimeSource& timeSource() { return time_source_; }
-  const Http::HttpServerPropertiesCacheManagerSharedPtr alternateProtocolCacheManager() {
+  Http::HttpServerPropertiesCacheManager& alternateProtocolCacheManager() {
     return alternate_protocol_cache_manager_;
   }
 
 private:
-  const Http::HttpServerPropertiesCacheManagerSharedPtr alternate_protocol_cache_manager_;
+  Http::HttpServerPropertiesCacheManager& alternate_protocol_cache_manager_;
   TimeSource& time_source_;
 };
 

source/server/config_validation/server.cc

@@ -139,6 +139,11 @@ void ValidationInstance::initialize(const Options& options,
   ssl_context_manager_ =
       std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(server_contexts_);
 
+  http_server_properties_cache_manager_ =
+      std::make_unique<Http::HttpServerPropertiesCacheManagerImpl>(
+          serverFactoryContext(), messageValidationContext().staticValidationVisitor(),
+          thread_local_);
+
   cluster_manager_factory_ = std::make_unique<Upstream::ValidationClusterManagerFactory>(
       server_contexts_, stats(), threadLocal(), http_context_,
       [this]() -> Network::DnsResolverSharedPtr { return this->dnsResolver(); },

source/server/config_validation/server.h

@@ -80,6 +80,9 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
   const Upstream::ClusterManager& clusterManager() const override {
     return *config_.clusterManager();
   }
+  Http::HttpServerPropertiesCacheManager& httpServerPropertiesCacheManager() override {
+    return *http_server_properties_cache_manager_;
+  }
   Ssl::ContextManager& sslContextManager() override { return *ssl_context_manager_; }
   Event::Dispatcher& dispatcher() override { return *dispatcher_; }
   Network::DnsResolverSharedPtr dnsResolver() override;
@@ -179,6 +182,7 @@ class ValidationInstance final : Logger::Loggable<Logger::Id::main>,
   Configuration::MainImpl config_;
   LocalInfo::LocalInfoPtr local_info_;
   AccessLog::AccessLogManagerImpl access_log_manager_;
+  std::unique_ptr<Http::HttpServerPropertiesCacheManager> http_server_properties_cache_manager_;
   std::unique_ptr<Upstream::ValidationClusterManagerFactory> cluster_manager_factory_;
   std::unique_ptr<ListenerManager> listener_manager_;
   std::unique_ptr<OverloadManager> overload_manager_;

source/server/server.cc

@@ -740,6 +740,11 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar
   ssl_context_manager_ =
       std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(server_contexts_);
 
+  http_server_properties_cache_manager_ =
+      std::make_unique<Http::HttpServerPropertiesCacheManagerImpl>(
+          serverFactoryContext(), messageValidationContext().staticValidationVisitor(),
+          thread_local_);
+
   cluster_manager_factory_ = std::make_unique<Upstream::ProdClusterManagerFactory>(
       serverFactoryContext(), stats_store_, thread_local_, http_context_,
       [this]() -> Network::DnsResolverSharedPtr { return this->getOrCreateDnsResolver(); },

source/server/server.h

@@ -178,6 +178,9 @@ class ServerFactoryContextImpl : public Configuration::ServerFactoryContext,
 
   // Configuration::ServerFactoryContext
   Upstream::ClusterManager& clusterManager() override { return server_.clusterManager(); }
+  Http::HttpServerPropertiesCacheManager& httpServerPropertiesCacheManager() override {
+    return server_.httpServerPropertiesCacheManager();
+  }
   Event::Dispatcher& mainThreadDispatcher() override { return server_.dispatcher(); }
   const Server::Options& options() override { return server_.options(); }
   const LocalInfo::LocalInfo& localInfo() const override { return server_.localInfo(); }
@@ -264,6 +267,9 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
   Api::Api& api() override { return *api_; }
   Upstream::ClusterManager& clusterManager() override;
   const Upstream::ClusterManager& clusterManager() const override;
+  Http::HttpServerPropertiesCacheManager& httpServerPropertiesCacheManager() override {
+    return *http_server_properties_cache_manager_;
+  }
   Ssl::ContextManager& sslContextManager() override { return *ssl_context_manager_; }
   Event::Dispatcher& dispatcher() override { return *dispatcher_; }
   Network::DnsResolverSharedPtr dnsResolver() override { return dns_resolver_; }
@@ -408,6 +414,7 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
   std::unique_ptr<OverloadManager> overload_manager_;
   std::unique_ptr<OverloadManager> null_overload_manager_;
   std::vector<BootstrapExtensionPtr> bootstrap_extensions_;
+  std::unique_ptr<Http::HttpServerPropertiesCacheManager> http_server_properties_cache_manager_;
   Envoy::MutexTracer* mutex_tracer_;
   Grpc::ContextImpl grpc_context_;
   Http::ContextImpl http_context_;

test/common/http/http_server_properties_cache_manager_test.cc

@@ -23,17 +23,13 @@ class HttpServerPropertiesCacheManagerTest : public testing::Test,
     options2_.mutable_max_entries()->set_value(max_entries2_);
   }
   void initialize() {
-    AlternateProtocolsData data(context_.server_factory_context_,
-                                context_.messageValidationVisitor());
-    factory_ = std::make_unique<Http::HttpServerPropertiesCacheManagerFactoryImpl>(
-        singleton_manager_, tls_, data);
-    manager_ = factory_->get();
+    manager_ = std::make_unique<HttpServerPropertiesCacheManagerImpl>(
+        context_.server_factory_context_, context_.messageValidationVisitor(), tls_);
   }
 
   Singleton::ManagerImpl singleton_manager_;
   NiceMock<Server::Configuration::MockFactoryContext> context_;
   testing::NiceMock<ThreadLocal::MockInstance> tls_;
-  std::unique_ptr<Http::HttpServerPropertiesCacheManagerFactoryImpl> factory_;
   HttpServerPropertiesCacheManagerSharedPtr manager_;
   const std::string name1_ = "name1";
   const std::string name2_ = "name2";
@@ -45,13 +41,6 @@ class HttpServerPropertiesCacheManagerTest : public testing::Test,
   envoy::config::core::v3::AlternateProtocolsCacheOptions options2_;
 };
 
-TEST_F(HttpServerPropertiesCacheManagerTest, FactoryGet) {
-  initialize();
-
-  EXPECT_NE(nullptr, manager_);
-  EXPECT_EQ(manager_, factory_->get());
-}
-
 TEST_F(HttpServerPropertiesCacheManagerTest, GetCache) {
   initialize();
   HttpServerPropertiesCacheSharedPtr cache = manager_->getCache(options1_, dispatcher_);

test/extensions/filters/http/alternate_protocols_cache/filter_test.cc

@@ -27,23 +27,19 @@ class FilterTest : public testing::Test, public Event::TestUsingSimulatedTime {
   }
 
   void initialize(bool populate_config) {
-    EXPECT_CALL(alternate_protocols_cache_manager_factory_, get())
-        .WillOnce(Return(alternate_protocols_cache_manager_));
-
     envoy::extensions::filters::http::alternate_protocols_cache::v3::FilterConfig proto_config;
     if (populate_config) {
       EXPECT_CALL(*alternate_protocols_cache_manager_, getCache(_, _))
           .Times(testing::AnyNumber())
           .WillOnce(Return(alternate_protocols_cache_));
     }
-    filter_config_ = std::make_shared<FilterConfig>(
-        proto_config, alternate_protocols_cache_manager_factory_, simTime());
+    filter_config_ = std::make_shared<FilterConfig>(proto_config,
+                                                    *alternate_protocols_cache_manager_, simTime());
     filter_ = std::make_unique<Filter>(filter_config_, dispatcher_);
     filter_->setEncoderFilterCallbacks(callbacks_);
   }
 
   Event::MockDispatcher dispatcher_;
-  Http::MockHttpServerPropertiesCacheManagerFactory alternate_protocols_cache_manager_factory_;
   std::shared_ptr<Http::MockHttpServerPropertiesCacheManager> alternate_protocols_cache_manager_;
   std::shared_ptr<Http::MockHttpServerPropertiesCache> alternate_protocols_cache_;
   FilterConfigSharedPtr filter_config_;

test/extensions/key_value/file_based/alternate_protocols_cache_impl_test.cc

@@ -20,16 +20,12 @@ class HttpServerPropertiesCacheManagerTest : public testing::Test,
     options_.mutable_max_entries()->set_value(10);
   }
   void initialize() {
-    Http::AlternateProtocolsData data{context_.server_factory_context_,
-                                      context_.messageValidationVisitor()};
-    factory_ = std::make_unique<Http::HttpServerPropertiesCacheManagerFactoryImpl>(
-        singleton_manager_, tls_, data);
-    manager_ = factory_->get();
+    manager_ = std::make_unique<Http::HttpServerPropertiesCacheManagerImpl>(
+        context_.server_factory_context_, context_.messageValidationVisitor(), tls_);
   }
   Singleton::ManagerImpl singleton_manager_;
   NiceMock<Server::Configuration::MockFactoryContext> context_;
   testing::NiceMock<ThreadLocal::MockInstance> tls_;
-  std::unique_ptr<Http::HttpServerPropertiesCacheManagerFactoryImpl> factory_;
   Http::HttpServerPropertiesCacheManagerSharedPtr manager_;
   envoy::config::core::v3::AlternateProtocolsCacheOptions options_;
   testing::NiceMock<Event::MockDispatcher> dispatcher_;