SDS: reduce cost of secret update (envoyproxy#19971)

Signed-off-by: Yuchen Dai <[email protected]>
lambdai · Mar 3, 2022 · 1c2d160 · 1c2d160
1 parent 4aaf959
commit 1c2d160
Show file tree

Hide file tree

Showing 16 changed files with 162 additions and 124 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -9,6 +9,8 @@ Minor Behavior Changes
 ----------------------
 *Changes that may cause incompatibilities for some users, but should not for most*
 
+* perf: ssl contexts are now tracked without scan based garbage collection and greatly improved the performance on secret update.
+
 Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*

diff --git a/envoy/ssl/context_manager.h b/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>;

diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.cc b/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

diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.h b/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_{};
 };
 

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc
@@ -356,10 +356,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
@@ -384,10 +386,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();
 }
 
@@ -397,10 +401,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_;
@@ -430,10 +436,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();
 }
 

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/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;

diff --git a/source/server/ssl_context_manager.cc b/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");

diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc
@@ -691,7 +691,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);

diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc
@@ -25,8 +25,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public t
         Network::Test::getLoopbackAddressString(TestEnvironment::getIpVersionsForTest()[0]),
         ":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>),

diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc
@@ -101,6 +101,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 {

diff --git a/test/common/quic/envoy_quic_proof_verifier_test.cc b/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) {

diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc
@@ -43,7 +43,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