SDS: reduce cost of secret update

docs/root/version_history/current.rst

@@ -18,6 +18,7 @@ Minor Behavior Changes
 * http: avoiding delay-close for HTTP/1.0 responses framed by connection: close as well as HTTP/1.1 if the request is fully read. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.skip_delay_close`` to false.
 * http: now the max concurrent streams of http2 connection can not only be adjusted down according to the SETTINGS frame but also can be adjusted up, of course, it can not exceed the configured upper bounds. This fix is guarded by ``envoy.reloadable_features.http2_allow_capacity_increase_by_settings``.
 * http: when writing custom filters, `injectEncodedDataToFilterChain` and `injectDecodedDataToFilterChain` now trigger sending of headers if they were not yet sent due to `StopIteration`. Previously, calling one of the inject functions in that state would trigger an assertion. See issue #19891 for more details.
+* perf: ssl contexts are now tracked without scan based garbage collection and greatly improved the performance on secret update.
 
 Bug Fixes
 ---------

envoy/ssl/context_manager.h

@@ -22,17 +22,15 @@ class ContextManager {
   /**
    * Builds a ClientContext from a ClientContextConfig.
    */
-  virtual ClientContextSharedPtr
-  createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config,
-                         Envoy::Ssl::ClientContextSharedPtr old_context) PURE;
+  virtual ClientContextSharedPtr createSslClientContext(Stats::Scope& scope,
+                                                        const ClientContextConfig& config) PURE;
 
   /**
    * Builds a ServerContext from a ServerContextConfig.
    */
   virtual ServerContextSharedPtr
   createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config,
-                         const std::vector<std::string>& server_names,
-                         Envoy::Ssl::ServerContextSharedPtr old_context) PURE;
+                         const std::vector<std::string>& server_names) PURE;
 
   /**
    * @return the number of days until the next certificate being managed will expire.
@@ -55,6 +53,11 @@ class ContextManager {
    * expire, or `absl::nullopt` if no OCSP responses exist.
    */
   virtual absl::optional<uint64_t> secondsUntilFirstOcspResponseExpires() const PURE;
+
+  /**
+   * Remove an existing ssl context.
+   */
+  virtual void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) PURE;
 };
 
 using ContextManagerPtr = std::unique_ptr<ContextManager>;

source/extensions/transport_sockets/tls/context_manager_impl.cc

@@ -14,62 +14,42 @@ namespace Extensions {
 namespace TransportSockets {
 namespace Tls {
 
+ContextManagerImpl::ContextManagerImpl(TimeSource& time_source) : time_source_(time_source) {}
+
 ContextManagerImpl::~ContextManagerImpl() {
-  removeEmptyContexts();
   KNOWN_ISSUE_ASSERT(contexts_.empty(), "https://github.com/envoyproxy/envoy/issues/10030");
 }
 
-void ContextManagerImpl::removeEmptyContexts() {
-  contexts_.remove_if([](const std::weak_ptr<Envoy::Ssl::Context>& n) { return n.expired(); });
-}
-
-void ContextManagerImpl::removeOldContext(std::shared_ptr<Envoy::Ssl::Context> old_context) {
-  if (old_context) {
-    contexts_.remove_if([old_context](const std::weak_ptr<Envoy::Ssl::Context>& n) {
-      std::shared_ptr<Envoy::Ssl::Context> sp = n.lock();
-      if (sp) {
-        return old_context == sp;
-      }
-      return false;
-    });
-  }
-}
-
 Envoy::Ssl::ClientContextSharedPtr
 ContextManagerImpl::createSslClientContext(Stats::Scope& scope,
-                                           const Envoy::Ssl::ClientContextConfig& config,
-                                           Envoy::Ssl::ClientContextSharedPtr old_context) {
+                                           const Envoy::Ssl::ClientContextConfig& config) {
   if (!config.isReady()) {
     return nullptr;
   }
 
   Envoy::Ssl::ClientContextSharedPtr context =
       std::make_shared<ClientContextImpl>(scope, config, time_source_);
-  removeOldContext(old_context);
-  removeEmptyContexts();
-  contexts_.emplace_back(context);
+  contexts_.insert(context);
   return context;
 }
 
-Envoy::Ssl::ServerContextSharedPtr ContextManagerImpl::createSslServerContext(
-    Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config,
-    const std::vector<std::string>& server_names, Envoy::Ssl::ServerContextSharedPtr old_context) {
+Envoy::Ssl::ServerContextSharedPtr
+ContextManagerImpl::createSslServerContext(Stats::Scope& scope,
+                                           const Envoy::Ssl::ServerContextConfig& config,
+                                           const std::vector<std::string>& server_names) {
   if (!config.isReady()) {
     return nullptr;
   }
 
   Envoy::Ssl::ServerContextSharedPtr context =
       std::make_shared<ServerContextImpl>(scope, config, server_names, time_source_);
-  removeOldContext(old_context);
-  removeEmptyContexts();
-  contexts_.emplace_back(context);
+  contexts_.insert(context);
   return context;
 }
 
 size_t ContextManagerImpl::daysUntilFirstCertExpires() const {
   size_t ret = std::numeric_limits<int>::max();
-  for (const auto& ctx_weak_ptr : contexts_) {
-    Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock();
+  for (const auto& context : contexts_) {
     if (context) {
       ret = std::min<size_t>(context->daysUntilFirstCertExpires(), ret);
     }
@@ -79,8 +59,7 @@ size_t ContextManagerImpl::daysUntilFirstCertExpires() const {
 
 absl::optional<uint64_t> ContextManagerImpl::secondsUntilFirstOcspResponseExpires() const {
   absl::optional<uint64_t> ret;
-  for (const auto& ctx_weak_ptr : contexts_) {
-    Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock();
+  for (const auto& context : contexts_) {
     if (context) {
       auto next_expiration = context->secondsUntilFirstOcspResponseExpires();
       if (next_expiration) {
@@ -93,14 +72,22 @@ absl::optional<uint64_t> ContextManagerImpl::secondsUntilFirstOcspResponseExpire
 }
 
 void ContextManagerImpl::iterateContexts(std::function<void(const Envoy::Ssl::Context&)> callback) {
-  for (const auto& ctx_weak_ptr : contexts_) {
-    Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock();
+  for (const auto& context : contexts_) {
     if (context) {
       callback(*context);
     }
   }
 }
 
+void ContextManagerImpl::removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) {
+  if (old_context != nullptr) {
+    auto erased = contexts_.erase(old_context);
+    // The contexts is expected to be added before is removed.
+    // And the prod ssl factory implementation guarantees any context is removed exactly once.
+    ASSERT(erased == 1);
+  }
+}
+
 } // namespace Tls
 } // namespace TransportSockets
 } // namespace Extensions

source/extensions/transport_sockets/tls/context_manager_impl.h

@@ -24,29 +24,27 @@ namespace Tls {
  */
 class ContextManagerImpl final : public Envoy::Ssl::ContextManager {
 public:
-  ContextManagerImpl(TimeSource& time_source) : time_source_(time_source) {}
+  explicit ContextManagerImpl(TimeSource& time_source);
   ~ContextManagerImpl() override;
 
   // Ssl::ContextManager
   Ssl::ClientContextSharedPtr
-  createSslClientContext(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config,
-                         Envoy::Ssl::ClientContextSharedPtr old_context) override;
+  createSslClientContext(Stats::Scope& scope,
+                         const Envoy::Ssl::ClientContextConfig& config) override;
   Ssl::ServerContextSharedPtr
   createSslServerContext(Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config,
-                         const std::vector<std::string>& server_names,
-                         Envoy::Ssl::ServerContextSharedPtr old_context) override;
+                         const std::vector<std::string>& server_names) override;
   size_t daysUntilFirstCertExpires() const override;
   absl::optional<uint64_t> secondsUntilFirstOcspResponseExpires() const override;
   void iterateContexts(std::function<void(const Envoy::Ssl::Context&)> callback) override;
   Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override {
     return private_key_method_manager_;
   };
+  void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) override;
 
 private:
-  void removeEmptyContexts();
-  void removeOldContext(std::shared_ptr<Envoy::Ssl::Context> old_context);
   TimeSource& time_source_;
-  std::list<std::weak_ptr<Envoy::Ssl::Context>> contexts_;
+  absl::flat_hash_set<Envoy::Ssl::ContextSharedPtr> contexts_;
   PrivateKeyMethodManagerImpl private_key_method_manager_{};
 };
 

source/extensions/transport_sockets/tls/ssl_socket.cc

@@ -359,10 +359,12 @@ ClientSslSocketFactory::ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPt
                                                Stats::Scope& stats_scope)
     : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)),
       config_(std::move(config)),
-      ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_, nullptr)) {
+      ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {
   config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); });
 }
 
+ClientSslSocketFactory::~ClientSslSocketFactory() { manager_.removeContext(ssl_ctx_); }
+
 Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket(
     Network::TransportSocketOptionsConstSharedPtr transport_socket_options) const {
   // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and
@@ -387,10 +389,12 @@ bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ClientSslSocketFactory::onAddOrUpdateSecret() {
   ENVOY_LOG(debug, "Secret is updated.");
+  auto ctx = manager_.createSslClientContext(stats_scope_, *config_);
   {
     absl::WriterMutexLock l(&ssl_ctx_mu_);
-    ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_, ssl_ctx_);
+    std::swap(ctx, ssl_ctx_);
   }
+  manager_.removeContext(ctx);
   stats_.ssl_context_update_by_sds_.inc();
 }
 
@@ -400,10 +404,12 @@ ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPt
                                                const std::vector<std::string>& server_names)
     : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("server", stats_scope)),
       config_(std::move(config)), server_names_(server_names),
-      ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_, nullptr)) {
+      ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) {
   config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); });
 }
 
+ServerSslSocketFactory::~ServerSslSocketFactory() { manager_.removeContext(ssl_ctx_); }
+
 Envoy::Ssl::ClientContextSharedPtr ClientSslSocketFactory::sslCtx() {
   absl::ReaderMutexLock l(&ssl_ctx_mu_);
   return ssl_ctx_;
@@ -433,10 +439,13 @@ bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ServerSslSocketFactory::onAddOrUpdateSecret() {
   ENVOY_LOG(debug, "Secret is updated.");
+  auto ctx = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
   {
     absl::WriterMutexLock l(&ssl_ctx_mu_);
-    ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_, ssl_ctx_);
+    std::swap(ctx, ssl_ctx_);
   }
+  manager_.removeContext(ctx);
+
   stats_.ssl_context_update_by_sds_.inc();
 }
 

source/extensions/transport_sockets/tls/ssl_socket.h

@@ -103,6 +103,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config,
                          Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope);
 
+  ~ClientSslSocketFactory() override;
+
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override;
   bool implementsSecureTransport() const override;
@@ -133,6 +135,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
                          Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope,
                          const std::vector<std::string>& server_names);
 
+  ~ServerSslSocketFactory() override;
+
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override;
   bool implementsSecureTransport() const override;

source/server/ssl_context_manager.cc

@@ -14,16 +14,14 @@ namespace Server {
 class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager {
   Ssl::ClientContextSharedPtr
   createSslClientContext(Stats::Scope& /* scope */,
-                         const Envoy::Ssl::ClientContextConfig& /* config */,
-                         Envoy::Ssl::ClientContextSharedPtr /* old_context */) override {
+                         const Envoy::Ssl::ClientContextConfig& /* config */) override {
     throwException();
   }
 
   Ssl::ServerContextSharedPtr
   createSslServerContext(Stats::Scope& /* scope */,
                          const Envoy::Ssl::ServerContextConfig& /* config */,
-                         const std::vector<std::string>& /* server_names */,
-                         Envoy::Ssl::ServerContextSharedPtr /* old_context */) override {
+                         const std::vector<std::string>& /* server_names */) override {
     throwException();
   }
 
@@ -36,6 +34,12 @@ class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager {
 
   Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override { throwException(); }
 
+  void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) override {
+    if (old_context) {
+      throw EnvoyException("SSL is not supported in this configuration");
+    }
+  }
+
 private:
   [[noreturn]] void throwException() {
     throw EnvoyException("SSL is not supported in this configuration");

test/common/http/conn_pool_grid_test.cc

@@ -828,7 +828,7 @@ TEST_F(ConnectivityGridTest, ConnectionCloseDuringCreation) {
   Envoy::Ssl::ClientContextConfigPtr config(new NiceMock<Ssl::MockClientContextConfig>());
   NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context;
   Ssl::ClientContextSharedPtr ssl_context(new Ssl::MockClientContext());
-  EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _, _))
+  EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _))
       .WillOnce(Return(ssl_context));
   auto factory =
       std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config), factory_context);

test/common/http/http3/conn_pool_test.cc

@@ -57,7 +57,7 @@ class MockPoolConnectResultCallback : public PoolConnectResultCallback {
 class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testing::Test {
 public:
   Http3ConnPoolImplTest() {
-    EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _, _))
+    EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _))
         .WillRepeatedly(Return(ssl_context_));
     factory_.emplace(std::unique_ptr<Envoy::Ssl::ClientContextConfig>(
                          new NiceMock<Ssl::MockClientContextConfig>),

test/common/quic/client_connection_factory_impl_test.cc

@@ -25,8 +25,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime,
     test_address_ = Network::Utility::resolveUrl(
         absl::StrCat("tcp://", Network::Test::getLoopbackAddressUrlString(GetParam()), ":30"));
     Ssl::ClientContextSharedPtr context{new Ssl::MockClientContext()};
-    EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _, _))
-        .WillOnce(Return(context));
+    EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _)).WillOnce(Return(context));
     factory_ = std::make_unique<Quic::QuicClientTransportSocketFactory>(
         std::unique_ptr<Envoy::Ssl::ClientContextConfig>(
             new NiceMock<Ssl::MockClientContextConfig>),

test/common/quic/envoy_quic_proof_source_test.cc

@@ -102,6 +102,7 @@ class SignatureVerifier {
   NiceMock<Ssl::MockClientContextConfig> client_context_config_;
   NiceMock<Ssl::MockCertificateValidationContextConfig> cert_validation_ctx_config_;
   std::unique_ptr<EnvoyQuicProofVerifier> verifier_;
+  NiceMock<Ssl::MockContextManager> tls_context_manager_;
 };
 
 class TestSignatureCallback : public quic::ProofSource::SignatureCallback {

test/common/quic/envoy_quic_proof_verifier_test.cc

@@ -89,6 +89,7 @@ class EnvoyQuicProofVerifierTest : public testing::Test {
   NiceMock<Ssl::MockClientContextConfig> client_context_config_;
   Ssl::MockCertificateValidationContextConfig cert_validation_ctx_config_;
   std::unique_ptr<EnvoyQuicProofVerifier> verifier_;
+  NiceMock<Ssl::MockContextManager> tls_context_manager_;
 };
 
 TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainSuccess) {

test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc

@@ -45,7 +45,7 @@ class ClusterTest : public testing::Test,
         admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_store_,
         singleton_manager_, tls_, validation_visitor_, *api_, options_);
     if (uses_tls) {
-      EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _, _));
+      EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _));
     }
     EXPECT_CALL(*dns_cache_manager_, getCache(_));
     // Below we return a nullptr handle which has no effect on the code under test but isn't