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

…#13516)" (#13639)

This reverts commit 34b67f9.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

docs/root/version_history/current.rst

@@ -17,8 +17,6 @@ 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,13 +226,6 @@ 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,15 +112,6 @@ 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,7 +32,6 @@ 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,14 +384,6 @@ 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();
+    void start() { onInitialInterval(); }
 
   protected:
     ActiveHealthCheckSession(HealthCheckerImplBase& parent, HostSharedPtr host);

source/common/upstream/upstream_impl.cc

@@ -356,14 +356,6 @@ 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,8 +189,6 @@ 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,9 +24,6 @@ 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,9 +101,6 @@ 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,7 +49,6 @@ 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,8 +41,6 @@ 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,39 +355,13 @@ 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,
@@ -422,39 +396,13 @@ 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,8 +109,6 @@ 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;
 
@@ -121,7 +119,6 @@ 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,
@@ -136,8 +133,6 @@ 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;
 
@@ -149,7 +144,6 @@ 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,8 +942,6 @@ 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);
@@ -2431,19 +2429,13 @@ 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 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.
+  // 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.
   EXPECT_CALL(*transport_socket_match, resolve(nullptr));
   EXPECT_CALL(*transport_socket_match, resolve(MetadataEq(metadata)))
-      .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(); }));
+      .WillOnce(Return(TransportSocketMatcher::MatchData(
+          *health_check_only_socket_factory, health_transport_socket_stats, "health_check_only")));
   // 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(_));
@@ -2479,9 +2471,6 @@ 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,7 +33,6 @@ 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_; }
 
@@ -49,7 +48,6 @@ 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,