delay health checks until transport socket secrets are ready. (envoyp…

…roxy#13516)

* delay health checks until transport socket secrets are ready.

This fixes a sequencing issue that could result in undesirable Envoy
bootup times. When active health checks are configured on a cluster
using a transport socket that uses SDS to fetch secrets, it was possible
for the initial health check to run before secrets are loaded, which
results in a failure. The health check would then wait for the
no_traffic_interval (default of 60s) before trying again.

This commit makes health checks wait until secrets are loaded before
starting, implemented by adding support for registering a callback on
TransportSocketFactory. SslSocketFactory supports invoking the callback
when secrets are loaded, and all other socket types currently invoke the
callback immediately, preserving existing behavior.

Signed-off-by: Michael Puncel <mpuncel@squareup.com>

docs/root/version_history/current.rst

@@ -16,6 +16,8 @@ Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
+* active health checks: health checks using a TLS transport socket and secrets delivered via :ref:`SDS <config_secret_discovery_service>` will now wait until secrets are loaded before the first health check attempt. This should improve startup times by not having to wait for the :ref:`no_traffic_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_interval>` until the next attempt.
+
 * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
 
 Removed Config or Runtime

include/envoy/network/transport_socket.h

@@ -226,6 +226,13 @@ class TransportSocketFactory {
    */
   virtual TransportSocketPtr
   createTransportSocket(TransportSocketOptionsSharedPtr options) const PURE;
+
+  /**
+   * @param a callback to be invoked when the secrets required by the created transport
+   * sockets are ready. Will be invoked immediately if no secrets are required or if they
+   * are already loaded.
+   */
+  virtual void addReadyCb(std::function<void()> callback) PURE;
 };
 
 using TransportSocketFactoryPtr = std::unique_ptr<TransportSocketFactory>;

include/envoy/upstream/upstream.h

@@ -112,6 +112,15 @@ class Host : virtual public HostDescription {
                               Network::TransportSocketOptionsSharedPtr transport_socket_options,
                               const envoy::config::core::v3::Metadata* metadata) const PURE;
 
+  /**
+   * Register a callback to be invoked when secrets are ready for the transport socket that
+   * corresponds to the provided metadata.
+   * @param callback supplies the callback to be invoked.
+   * @param metadata supplies the metadata to be used for resolving transport socket matches.
+   */
+  virtual void addReadyCb(std::function<void()> callback,
+                          const envoy::config::core::v3::Metadata* metadata) const PURE;
+
   /**
    * @return host specific gauges.
    */

source/common/network/raw_buffer_socket.h

@@ -32,6 +32,7 @@ class RawBufferSocketFactory : public TransportSocketFactory {
   // Network::TransportSocketFactory
   TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  void addReadyCb(std::function<void()> callback) override { callback(); }
 };
 
 } // namespace Network

source/common/upstream/health_checker_base_impl.cc

@@ -384,6 +384,14 @@ void HealthCheckerImplBase::ActiveHealthCheckSession::onTimeoutBase() {
   handleFailure(envoy::data::core::v3::NETWORK);
 }
 
+void HealthCheckerImplBase::ActiveHealthCheckSession::start() {
+  // Start health checks only after secrets are ready for the transport socket
+  // that health checks will be performed on. If health checks start
+  // immediately, they may fail with "network" errors due to TLS credentials
+  // not yet being loaded, which can result in long startup times.
+  host_->addReadyCb([this] { onInitialInterval(); }, parent_.transportSocketMatchMetadata().get());
+}
+
 void HealthCheckerImplBase::ActiveHealthCheckSession::onInitialInterval() {
   if (parent_.initial_jitter_.count() == 0) {
     onIntervalBase();

source/common/upstream/health_checker_base_impl.h

@@ -77,7 +77,7 @@ class HealthCheckerImplBase : public HealthChecker,
     ~ActiveHealthCheckSession() override;
     HealthTransition setUnhealthy(envoy::data::core::v3::HealthCheckFailureType type);
     void onDeferredDeleteBase();
-    void start() { onInitialInterval(); }
+    void start();
 
   protected:
     ActiveHealthCheckSession(HealthCheckerImplBase& parent, HostSharedPtr host);

source/common/upstream/upstream_impl.cc

@@ -356,6 +356,14 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu
   return connection;
 }
 
+void HostImpl::addReadyCb(std::function<void()> callback,
+                          const envoy::config::core::v3::Metadata* metadata) const {
+  Network::TransportSocketFactory& factory =
+      (metadata != nullptr) ? cluster_->transportSocketMatcher().resolve(metadata).factory_
+                            : socket_factory_;
+  factory.addReadyCb(callback);
+}
+
 void HostImpl::weight(uint32_t new_weight) { weight_ = std::max(1U, new_weight); }
 
 std::vector<HostsPerLocalityConstSharedPtr> HostsPerLocalityImpl::filter(

source/common/upstream/upstream_impl.h

@@ -189,6 +189,8 @@ class HostImpl : public HostDescriptionImpl,
   createHealthCheckConnection(Event::Dispatcher& dispatcher,
                               Network::TransportSocketOptionsSharedPtr transport_socket_options,
                               const envoy::config::core::v3::Metadata* metadata) const override;
+  void addReadyCb(std::function<void()> callback,
+                  const envoy::config::core::v3::Metadata* metadata) const override;
 
   std::vector<std::pair<absl::string_view, Stats::PrimitiveGaugeReference>>
   gauges() const override {

source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h

@@ -24,6 +24,9 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory {
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
   bool implementsSecureTransport() const override { return true; }
+
+  // TODO(mpuncel) only invoke callback() once secrets are ready.
+  void addReadyCb(std::function<void()> callback) override { callback(); };
 };
 
 // TODO(danzh): when implement ProofSource, examine of it's necessary to

source/extensions/transport_sockets/alts/tsi_socket.h

@@ -101,6 +101,9 @@ class TsiSocketFactory : public Network::TransportSocketFactory {
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
 
+  // TODO(mpuncel) only invoke callback() once secrets are ready.
+  void addReadyCb(std::function<void()> callback) override { callback(); };
+
 private:
   HandshakerFactory handshaker_factory_;
   HandshakeValidator handshake_validator_;

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h

@@ -49,6 +49,7 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  void addReadyCb(std::function<void()> callback) override { callback(); };
 
 private:
   Network::TransportSocketFactoryPtr transport_socket_factory_;

source/extensions/transport_sockets/tap/tap.h

@@ -41,6 +41,8 @@ class TapSocketFactory : public Network::TransportSocketFactory,
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
+  // TODO(mpuncel) only invoke callback() once secrets are ready.
+  void addReadyCb(std::function<void()> callback) override { callback(); };
 
 private:
   Network::TransportSocketFactoryPtr transport_socket_factory_;

source/extensions/transport_sockets/tls/ssl_socket.cc

@@ -355,13 +355,39 @@ bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ClientSslSocketFactory::onAddOrUpdateSecret() {
   ENVOY_LOG(debug, "Secret is updated.");
+  bool should_run_callbacks = false;
   {
     absl::WriterMutexLock l(&ssl_ctx_mu_);
     ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
+    if (ssl_ctx_) {
+      should_run_callbacks = true;
+    }
+  }
+  if (should_run_callbacks) {
+    for (const auto& cb : secrets_ready_callbacks_) {
+      cb();
+    }
+    secrets_ready_callbacks_.clear();
   }
   stats_.ssl_context_update_by_sds_.inc();
 }
 
+void ClientSslSocketFactory::addReadyCb(std::function<void()> callback) {
+  bool immediately_run_callback = false;
+  {
+    absl::ReaderMutexLock l(&ssl_ctx_mu_);
+    if (ssl_ctx_) {
+      immediately_run_callback = true;
+    }
+  }
+
+  if (immediately_run_callback) {
+    callback();
+  } else {
+    secrets_ready_callbacks_.push_back(callback);
+  }
+}
+
 ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config,
                                                Envoy::Ssl::ContextManager& manager,
                                                Stats::Scope& stats_scope,
@@ -396,13 +422,39 @@ bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ServerSslSocketFactory::onAddOrUpdateSecret() {
   ENVOY_LOG(debug, "Secret is updated.");
+  bool should_run_callbacks = false;
   {
     absl::WriterMutexLock l(&ssl_ctx_mu_);
     ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
+
+    if (ssl_ctx_) {
+      should_run_callbacks = true;
+    }
+  }
+  if (should_run_callbacks) {
+    for (const auto& cb : secrets_ready_callbacks_) {
+      cb();
+    }
+    secrets_ready_callbacks_.clear();
   }
   stats_.ssl_context_update_by_sds_.inc();
 }
 
+void ServerSslSocketFactory::addReadyCb(std::function<void()> callback) {
+  bool immediately_run_callback = false;
+  {
+    absl::ReaderMutexLock l(&ssl_ctx_mu_);
+    if (ssl_ctx_) {
+      immediately_run_callback = true;
+    }
+  }
+  if (immediately_run_callback) {
+    callback();
+  } else {
+    secrets_ready_callbacks_.push_back(callback);
+  }
+}
+
 } // namespace Tls
 } // namespace TransportSockets
 } // namespace Extensions

source/extensions/transport_sockets/tls/ssl_socket.h

@@ -109,6 +109,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
 
+  void addReadyCb(std::function<void()> callback) override;
+
   // Secret::SecretCallbacks
   void onAddOrUpdateSecret() override;
 
@@ -119,6 +121,7 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   Envoy::Ssl::ClientContextConfigPtr config_;
   mutable absl::Mutex ssl_ctx_mu_;
   Envoy::Ssl::ClientContextSharedPtr ssl_ctx_ ABSL_GUARDED_BY(ssl_ctx_mu_);
+  std::list<std::function<void()>> secrets_ready_callbacks_;
 };
 
 class ServerSslSocketFactory : public Network::TransportSocketFactory,
@@ -133,6 +136,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
 
+  void addReadyCb(std::function<void()> callback) override;
+
   // Secret::SecretCallbacks
   void onAddOrUpdateSecret() override;
 
@@ -144,6 +149,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
   const std::vector<std::string> server_names_;
   mutable absl::Mutex ssl_ctx_mu_;
   Envoy::Ssl::ServerContextSharedPtr ssl_ctx_ ABSL_GUARDED_BY(ssl_ctx_mu_);
+  std::list<std::function<void()>> secrets_ready_callbacks_;
 };
 
 } // namespace Tls

test/common/upstream/health_checker_impl_test.cc

@@ -942,6 +942,8 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) {
       Network::TransportSocketFactoryPtr(socket_factory));
   cluster_->info_->transport_socket_matcher_.reset(transport_socket_match);
 
+  EXPECT_CALL(*socket_factory, addReadyCb(_))
+      .WillOnce(Invoke([&](std::function<void()> callback) -> void { callback(); }));
   EXPECT_CALL(*socket_factory, createTransportSocket(ApplicationProtocolListEq("http1")));
 
   allocHealthChecker(yaml);
@@ -2429,13 +2431,19 @@ TEST_F(HttpHealthCheckerImplTest, TransportSocketMatchCriteria) {
       ALL_TRANSPORT_SOCKET_MATCH_STATS(POOL_COUNTER_PREFIX(stats_store, "test"))};
   auto health_check_only_socket_factory = std::make_unique<Network::MockTransportSocketFactory>();
 
-  // We expect resolve() to be called twice, once for endpoint socket matching (with no metadata in
-  // this test) and once for health check socket matching. In the latter we expect metadata that
-  // matches the above object.
+  // We expect resolve() to be called 3 times, once for endpoint socket matching (with no metadata
+  // in this test) and twice for health check socket matching (once for checking if secrets are
+  // ready on the transport socket, and again for actually getting the health check transport socket
+  // to create a connection). In the latter 2 calls, we expect metadata that matches the above
+  // object.
   EXPECT_CALL(*transport_socket_match, resolve(nullptr));
   EXPECT_CALL(*transport_socket_match, resolve(MetadataEq(metadata)))
-      .WillOnce(Return(TransportSocketMatcher::MatchData(
-          *health_check_only_socket_factory, health_transport_socket_stats, "health_check_only")));
+      .Times(2)
+      .WillRepeatedly(Return(TransportSocketMatcher::MatchData(
+          *health_check_only_socket_factory, health_transport_socket_stats, "health_check_only")))
+      .RetiresOnSaturation();
+  EXPECT_CALL(*health_check_only_socket_factory, addReadyCb(_))
+      .WillOnce(Invoke([&](std::function<void()> callback) -> void { callback(); }));
   // The health_check_only_socket_factory should be used to create a transport socket for the health
   // check connection.
   EXPECT_CALL(*health_check_only_socket_factory, createTransportSocket(_));
@@ -2471,6 +2479,9 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) {
     )EOF";
 
   auto default_socket_factory = std::make_unique<Network::MockTransportSocketFactory>();
+
+  EXPECT_CALL(*default_socket_factory, addReadyCb(_))
+      .WillOnce(Invoke([&](std::function<void()> callback) -> void { callback(); }));
   // The default_socket_factory should be used to create a transport socket for the health check
   // connection.
   EXPECT_CALL(*default_socket_factory, createTransportSocket(_));

test/common/upstream/transport_socket_matcher_test.cc

@@ -33,6 +33,7 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory {
   MOCK_METHOD(bool, implementsSecureTransport, (), (const));
   MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket,
               (Network::TransportSocketOptionsSharedPtr), (const));
+  MOCK_METHOD(void, addReadyCb, (std::function<void()>));
   FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {}
   std::string id() const { return id_; }
 
@@ -48,6 +49,7 @@ class FooTransportSocketFactory
   MOCK_METHOD(bool, implementsSecureTransport, (), (const));
   MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket,
               (Network::TransportSocketOptionsSharedPtr), (const));
+  MOCK_METHOD(void, addReadyCb, (std::function<void()>));
 
   Network::TransportSocketFactoryPtr
   createTransportSocketFactory(const Protobuf::Message& proto,