From e98cbec34ccdfaf4f0c9b55721aa98d100c8eafb Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Tue, 20 Oct 2020 08:50:23 -0400 Subject: [PATCH 1/6] health_checker: wait for secrets to load before starting heatlth checks This commit is a revival of https://github.com/envoyproxy/envoy/pull/13516 which aimed to solve https://github.com/envoyproxy/envoy/issues/12389. That PR was merged and then reverted due to a deadlock bug, which is still present in this commit. The fix will be applied in a subsequent commit in order to make the resolution clearer to PR reviewers. Signed-off-by: Michael Puncel --- docs/root/version_history/current.rst | 3 + include/envoy/network/transport_socket.h | 7 ++ include/envoy/upstream/upstream.h | 9 ++ source/common/network/raw_buffer_socket.h | 1 + .../quic/quic_transport_socket_factory.h | 1 + .../upstream/health_checker_base_impl.cc | 8 ++ .../upstream/health_checker_base_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 8 ++ source/common/upstream/upstream_impl.h | 2 + .../transport_sockets/alts/tsi_socket.h | 3 + .../proxy_protocol/proxy_protocol.h | 1 + source/extensions/transport_sockets/tap/tap.h | 3 + .../transport_sockets/tls/ssl_socket.cc | 52 +++++++++ .../transport_sockets/tls/ssl_socket.h | 6 + .../upstream/health_checker_impl_test.cc | 21 +++- .../upstream/transport_socket_matcher_test.cc | 2 + .../transport_sockets/tls/ssl_socket_test.cc | 104 ++++++++++++++++++ test/mocks/network/transport_socket.h | 1 + test/mocks/ssl/mocks.cc | 3 + test/mocks/ssl/mocks.h | 11 ++ test/mocks/upstream/host.h | 2 + 21 files changed, 244 insertions(+), 6 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 50b877436139f..be445a0a2a0d4 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,6 +23,9 @@ Bug Fixes * validation: fix an issue that causes TAP sockets to panic during config validation mode. * xray: fix the default sampling 'rate' for AWS X-Ray tracer extension to be 5% as opposed to 50%. * zipkin: fix timestamp serializaiton in annotations. A prior bug fix exposed an issue with timestamps being serialized as strings. +* active health checks: health checks using a TLS transport socket and secrets delivered via :ref:`SDS ` 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 ` until the next attempt. + +* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. Removed Config or Runtime ------------------------- diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h index e71bcbed89d25..ee8d1897e1668 100644 --- a/include/envoy/network/transport_socket.h +++ b/include/envoy/network/transport_socket.h @@ -247,6 +247,13 @@ class TransportSocketFactory { * negotiation. */ virtual bool supportsAlpn() const { return false; } + + /** + * @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 callback) PURE; }; using TransportSocketFactoryPtr = std::unique_ptr; diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index dc628e6139dbc..4828dd71af215 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -109,6 +109,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 callback, + const envoy::config::core::v3::Metadata* metadata) const PURE; + /** * @return host specific gauges. */ diff --git a/source/common/network/raw_buffer_socket.h b/source/common/network/raw_buffer_socket.h index 24e498ebf59a9..ff8679d5c8f65 100644 --- a/source/common/network/raw_buffer_socket.h +++ b/source/common/network/raw_buffer_socket.h @@ -34,6 +34,7 @@ class RawBufferSocketFactory : public TransportSocketFactory { TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; bool usesProxyProtocolOptions() const override { return false; } + void addReadyCb(std::function callback) override { callback(); } }; } // namespace Network diff --git a/source/common/quic/quic_transport_socket_factory.h b/source/common/quic/quic_transport_socket_factory.h index ec8d182ce17e7..878f556873915 100644 --- a/source/common/quic/quic_transport_socket_factory.h +++ b/source/common/quic/quic_transport_socket_factory.h @@ -53,6 +53,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory, bool implementsSecureTransport() const override { return true; } bool usesProxyProtocolOptions() const override { return false; } bool supportsAlpn() const override { return true; } + void addReadyCb(std::function callback) override { callback(); } protected: virtual void onSecretUpdated() PURE; diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc index c396402a118c1..9db5f98d18209 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -411,6 +411,14 @@ void HealthCheckerImplBase::ActiveHealthCheckSession::onTimeoutBase() { handleFailure(envoy::data::core::v3::NETWORK_TIMEOUT); } +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(); diff --git a/source/common/upstream/health_checker_base_impl.h b/source/common/upstream/health_checker_base_impl.h index 4053cd3f623d6..7c24d35c1a005 100644 --- a/source/common/upstream/health_checker_base_impl.h +++ b/source/common/upstream/health_checker_base_impl.h @@ -78,7 +78,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); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 07db4f18f2ff6..d19452b432e69 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -349,6 +349,14 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu return connection; } +void HostImpl::addReadyCb(std::function callback, + const envoy::config::core::v3::Metadata* metadata) const { + Network::TransportSocketFactory& factory = + (metadata != nullptr) ? resolveTransportSocketFactory(healthCheckAddress(), metadata) + : transportSocketFactory(); + factory.addReadyCb(callback); +} + void HostImpl::weight(uint32_t new_weight) { weight_ = std::max(1U, new_weight); } std::vector HostsPerLocalityImpl::filter( diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 28ff4a7b60218..21344e3a2986b 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -212,6 +212,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 callback, + const envoy::config::core::v3::Metadata* metadata) const override; std::vector> gauges() const override { diff --git a/source/extensions/transport_sockets/alts/tsi_socket.h b/source/extensions/transport_sockets/alts/tsi_socket.h index 1d1fa547c6a3e..836aa7e597702 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.h +++ b/source/extensions/transport_sockets/alts/tsi_socket.h @@ -121,6 +121,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 callback) override { callback(); }; + private: HandshakerFactory handshaker_factory_; HandshakeValidator handshake_validator_; diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h index c4c9a80f629e9..f107a3fdcda0f 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h @@ -50,6 +50,7 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; bool usesProxyProtocolOptions() const override { return true; } + void addReadyCb(std::function callback) override { callback(); }; private: Network::TransportSocketFactoryPtr transport_socket_factory_; diff --git a/source/extensions/transport_sockets/tap/tap.h b/source/extensions/transport_sockets/tap/tap.h index 2971c3e846ba6..5b8464d7d2681 100644 --- a/source/extensions/transport_sockets/tap/tap.h +++ b/source/extensions/transport_sockets/tap/tap.h @@ -43,6 +43,9 @@ class TapSocketFactory : public Network::TransportSocketFactory, bool implementsSecureTransport() const override; bool usesProxyProtocolOptions() const override; + // TODO(mpuncel) only invoke callback() once secrets are ready. + void addReadyCb(std::function callback) override { callback(); }; + private: Network::TransportSocketFactoryPtr transport_socket_factory_; }; diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index a9fcfbf84ed52..ed2048a830041 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -380,13 +380,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_, ssl_ctx_); + 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 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, @@ -421,13 +447,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_, ssl_ctx_); + + 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 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 diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index 5b9239d266053..c184a8091c390 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -113,6 +113,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, bool usesProxyProtocolOptions() const override { return false; } bool supportsAlpn() const override { return true; } + void addReadyCb(std::function callback) override; + // Secret::SecretCallbacks void onAddOrUpdateSecret() override; @@ -125,6 +127,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> secrets_ready_callbacks_; }; class ServerSslSocketFactory : public Network::TransportSocketFactory, @@ -140,6 +143,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, bool implementsSecureTransport() const override; bool usesProxyProtocolOptions() const override { return false; } + void addReadyCb(std::function callback) override; + // Secret::SecretCallbacks void onAddOrUpdateSecret() override; @@ -151,6 +156,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, const std::vector server_names_; mutable absl::Mutex ssl_ctx_mu_; Envoy::Ssl::ServerContextSharedPtr ssl_ctx_ ABSL_GUARDED_BY(ssl_ctx_mu_); + std::list> secrets_ready_callbacks_; }; } // namespace Tls diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index efb4b3b6c9615..ee198ad1ba763 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -1104,6 +1104,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 callback) -> void { callback(); })); EXPECT_CALL(*socket_factory, createTransportSocket(ApplicationProtocolListEq("http1"))); allocHealthChecker(yaml); @@ -2582,13 +2584,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(); - // 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 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(_)); @@ -2624,6 +2632,9 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) { )EOF"; auto default_socket_factory = std::make_unique(); + + EXPECT_CALL(*default_socket_factory, addReadyCb(_)) + .WillOnce(Invoke([&](std::function 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(_)); diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index b564192f860eb..ab155e0023413 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -34,6 +34,7 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { MOCK_METHOD(bool, usesProxyProtocolOptions, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); + MOCK_METHOD(void, addReadyCb, (std::function)); FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} std::string id() const { return id_; } @@ -50,6 +51,7 @@ class FooTransportSocketFactory MOCK_METHOD(bool, usesProxyProtocolOptions, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); + MOCK_METHOD(void, addReadyCb, (std::function)); Network::TransportSocketFactoryPtr createTransportSocketFactory(const Protobuf::Message& proto, diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 80c246b949ca8..457b5c9683957 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -59,6 +59,7 @@ using testing::ContainsRegex; using testing::DoAll; using testing::InSequence; using testing::Invoke; +using testing::MockFunction; using testing::NiceMock; using testing::Return; using testing::ReturnRef; @@ -4708,6 +4709,12 @@ TEST_P(SslSocketTest, DownstreamNotReadySslSocket) { ContextManagerImpl manager(time_system_); ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); + + // Add a secrets ready callback that should not be invoked. + MockFunction mock_callback_; + EXPECT_CALL(mock_callback_, Call()).Times(0); + server_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); + auto transport_socket = server_ssl_socket_factory.createTransportSocket(nullptr); EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); EXPECT_EQ(nullptr, transport_socket->ssl()); @@ -4744,6 +4751,12 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { ContextManagerImpl manager(time_system_); ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); + + // Add a secrets ready callback that should not be invoked. + MockFunction mock_callback_; + EXPECT_CALL(mock_callback_, Call()).Times(0); + client_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); + auto transport_socket = client_ssl_socket_factory.createTransportSocket(nullptr); EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); EXPECT_EQ(nullptr, transport_socket->ssl()); @@ -4756,6 +4769,97 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { EXPECT_EQ("TLS error: Secret is not supplied by SDS", transport_socket->failureReason()); } +// Validate that secrets callbacks are invoked when secrets become ready. +TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { + Stats::TestUtil::TestStore stats_store; + NiceMock local_info; + testing::NiceMock factory_context; + NiceMock init_manager; + NiceMock dispatcher; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(ReturnRef(init_manager)); + EXPECT_CALL(factory_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + auto client_cfg = std::make_unique(tls_context, factory_context); + EXPECT_TRUE(client_cfg->tlsCertificates().empty()); + EXPECT_FALSE(client_cfg->isReady()); + + NiceMock context_manager; + ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), context_manager, + stats_store); + + // Add a secrets ready callback. It should not be invoked until onAddOrUpdateSecret() is called. + MockFunction mock_callback_; + EXPECT_CALL(mock_callback_, Call()).Times(0); + client_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); + + // Call onAddOrUpdateSecret, but return a null ssl_ctx. This should not invoke the callback. + EXPECT_CALL(context_manager, createSslClientContext(_, _)).WillOnce(Return(nullptr)); + client_ssl_socket_factory.onAddOrUpdateSecret(); + + EXPECT_CALL(mock_callback_, Call()); + Ssl::ClientContextSharedPtr mock_context = std::make_shared(); + EXPECT_CALL(context_manager, createSslClientContext(_, _)).WillOnce(Return(mock_context)); + client_ssl_socket_factory.onAddOrUpdateSecret(); + + // Add another callback, it should be invoked immediately. + MockFunction second_callback_; + EXPECT_CALL(second_callback_, Call()); + client_ssl_socket_factory.addReadyCb(second_callback_.AsStdFunction()); +} + +// Validate that secrets callbacks are invoked when secrets become ready. +TEST_P(SslSocketTest, ServerAddSecretsReadyCallback) { + Stats::TestUtil::TestStore stats_store; + NiceMock local_info; + testing::NiceMock factory_context; + NiceMock init_manager; + NiceMock dispatcher; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(ReturnRef(init_manager)); + EXPECT_CALL(factory_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); + + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + auto server_cfg = std::make_unique(tls_context, factory_context); + EXPECT_TRUE(server_cfg->tlsCertificates().empty()); + EXPECT_FALSE(server_cfg->isReady()); + + NiceMock context_manager; + ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), context_manager, + stats_store, std::vector{}); + + // Add a secrets ready callback. It should not be invoked until onAddOrUpdateSecret() is called. + MockFunction mock_callback_; + EXPECT_CALL(mock_callback_, Call()).Times(0); + server_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); + + // Call onAddOrUpdateSecret, but return a null ssl_ctx. This should not invoke the callback. + EXPECT_CALL(context_manager, createSslServerContext(_, _, _)).WillOnce(Return(nullptr)); + server_ssl_socket_factory.onAddOrUpdateSecret(); + + // Now return a ssl context which should result in the callback being invoked. + EXPECT_CALL(mock_callback_, Call()); + Ssl::ServerContextSharedPtr mock_context = std::make_shared(); + EXPECT_CALL(context_manager, createSslServerContext(_, _, _)).WillOnce(Return(mock_context)); + server_ssl_socket_factory.onAddOrUpdateSecret(); + + // Add another callback, it should be invoked immediately. + MockFunction second_callback_; + EXPECT_CALL(second_callback_, Call()); + server_ssl_socket_factory.addReadyCb(second_callback_.AsStdFunction()); +} + TEST_P(SslSocketTest, TestTransportSocketCallback) { // Make MockTransportSocketCallbacks. Network::MockIoHandle io_handle; diff --git a/test/mocks/network/transport_socket.h b/test/mocks/network/transport_socket.h index 2e9e906f36b11..edfa5dcc16753 100644 --- a/test/mocks/network/transport_socket.h +++ b/test/mocks/network/transport_socket.h @@ -41,6 +41,7 @@ class MockTransportSocketFactory : public TransportSocketFactory { MOCK_METHOD(bool, supportsAlpn, (), (const)); MOCK_METHOD(TransportSocketPtr, createTransportSocket, (TransportSocketOptionsSharedPtr), (const)); + MOCK_METHOD(void, addReadyCb, (std::function)); }; } // namespace Network diff --git a/test/mocks/ssl/mocks.cc b/test/mocks/ssl/mocks.cc index 6e3ea24b32598..24e8a9f263b88 100644 --- a/test/mocks/ssl/mocks.cc +++ b/test/mocks/ssl/mocks.cc @@ -22,6 +22,9 @@ MockClientContextConfig::MockClientContextConfig() { } MockClientContextConfig::~MockClientContextConfig() = default; +MockServerContext::MockServerContext() = default; +MockServerContext::~MockServerContext() = default; + MockServerContextConfig::MockServerContextConfig() = default; MockServerContextConfig::~MockServerContextConfig() = default; diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 94cf360a3e706..8bf9670c52f5b 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -107,6 +107,17 @@ class MockClientContextConfig : public ClientContextConfig { std::string test_{}; }; +class MockServerContext : public ServerContext { +public: + MockServerContext(); + ~MockServerContext() override; + + MOCK_METHOD(size_t, daysUntilFirstCertExpires, (), (const)); + MOCK_METHOD(absl::optional, secondsUntilFirstOcspResponseExpires, (), (const)); + MOCK_METHOD(CertificateDetailsPtr, getCaCertInformation, (), (const)); + MOCK_METHOD(std::vector, getCertChainInformation, (), (const)); +}; + class MockServerContextConfig : public ServerContextConfig { public: MockServerContextConfig(); diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index 82ea4bd8e5df1..be46ff312a532 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -193,6 +193,8 @@ class MockHost : public Host { MOCK_METHOD(void, priority, (uint32_t)); MOCK_METHOD(bool, warmed, (), (const)); MOCK_METHOD(MonotonicTime, creationTime, (), (const)); + MOCK_METHOD(void, addReadyCb, (std::function, const envoy::config::core::v3::Metadata*), + (const)); testing::NiceMock cluster_; Network::TransportSocketFactoryPtr socket_factory_; From e43c9f8e9e4b5c6a077110e83725db0753aa5ece Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Thu, 29 Apr 2021 11:54:35 -0400 Subject: [PATCH 2/6] Fix concurrency bug in TLS socket The bug was caused by not queuing callbacks in addReadyCb() while holding the lock. This means that you could make the decision to NOT run the callback, get a secret concurrently which would run all callbacks, and then add it to the queue at which point it would never run. This also changed the name of addReadyCb() to addHealthCheckingReadyCb() on HostImpl, and made the logic add a callback for the health checking transport socket. That's the socket that we actually want health checks to wait on which may be different from the data plane socket. Signed-off-by: Michael Puncel --- include/envoy/upstream/upstream.h | 6 +- .../upstream/health_checker_base_impl.cc | 2 +- source/common/upstream/upstream_impl.cc | 2 +- source/common/upstream/upstream_impl.h | 2 +- .../transport_sockets/tls/ssl_socket.cc | 9 +- .../transport_sockets/tls/ssl_socket_test.cc | 99 ++++++++++++++++++- 6 files changed, 105 insertions(+), 15 deletions(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 4828dd71af215..5bd8b309a23ab 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -110,12 +110,12 @@ class Host : virtual public HostDescription { 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. + * Register a callback to be invoked when secrets are ready for the health + * checking 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 callback, + virtual void addHealthCheckingReadyCb(std::function callback, const envoy::config::core::v3::Metadata* metadata) const PURE; /** diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc index 9db5f98d18209..d4d5c942ce16d 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -416,7 +416,7 @@ void HealthCheckerImplBase::ActiveHealthCheckSession::start() { // 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()); + host_->addHealthCheckingReadyCb([this] { onInitialInterval(); }, parent_.transportSocketMatchMetadata().get()); } void HealthCheckerImplBase::ActiveHealthCheckSession::onInitialInterval() { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index d19452b432e69..a4237002209d5 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -349,7 +349,7 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu return connection; } -void HostImpl::addReadyCb(std::function callback, +void HostImpl::addHealthCheckingReadyCb(std::function callback, const envoy::config::core::v3::Metadata* metadata) const { Network::TransportSocketFactory& factory = (metadata != nullptr) ? resolveTransportSocketFactory(healthCheckAddress(), metadata) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 21344e3a2986b..df10b1f8c6544 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -212,7 +212,7 @@ 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 callback, + void addHealthCheckingReadyCb(std::function callback, const envoy::config::core::v3::Metadata* metadata) const override; std::vector> diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index ed2048a830041..60b82eefac69c 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -403,13 +403,12 @@ void ClientSslSocketFactory::addReadyCb(std::function callback) { absl::ReaderMutexLock l(&ssl_ctx_mu_); if (ssl_ctx_) { immediately_run_callback = true; + } else { + secrets_ready_callbacks_.push_back(callback); } } - if (immediately_run_callback) { callback(); - } else { - secrets_ready_callbacks_.push_back(callback); } } @@ -471,12 +470,12 @@ void ServerSslSocketFactory::addReadyCb(std::function callback) { absl::ReaderMutexLock l(&ssl_ctx_mu_); if (ssl_ctx_) { immediately_run_callback = true; + } else { + secrets_ready_callbacks_.push_back(callback); } } if (immediately_run_callback) { callback(); - } else { - secrets_ready_callbacks_.push_back(callback); } } diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 457b5c9683957..dec020b58e387 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4800,12 +4800,12 @@ TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { client_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); // Call onAddOrUpdateSecret, but return a null ssl_ctx. This should not invoke the callback. - EXPECT_CALL(context_manager, createSslClientContext(_, _)).WillOnce(Return(nullptr)); + EXPECT_CALL(context_manager, createSslClientContext(_, _, _)).WillOnce(Return(nullptr)); client_ssl_socket_factory.onAddOrUpdateSecret(); EXPECT_CALL(mock_callback_, Call()); Ssl::ClientContextSharedPtr mock_context = std::make_shared(); - EXPECT_CALL(context_manager, createSslClientContext(_, _)).WillOnce(Return(mock_context)); + EXPECT_CALL(context_manager, createSslClientContext(_, _, _)).WillOnce(Return(mock_context)); client_ssl_socket_factory.onAddOrUpdateSecret(); // Add another callback, it should be invoked immediately. @@ -4845,13 +4845,13 @@ TEST_P(SslSocketTest, ServerAddSecretsReadyCallback) { server_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); // Call onAddOrUpdateSecret, but return a null ssl_ctx. This should not invoke the callback. - EXPECT_CALL(context_manager, createSslServerContext(_, _, _)).WillOnce(Return(nullptr)); + EXPECT_CALL(context_manager, createSslServerContext(_, _, _, _)).WillOnce(Return(nullptr)); server_ssl_socket_factory.onAddOrUpdateSecret(); // Now return a ssl context which should result in the callback being invoked. EXPECT_CALL(mock_callback_, Call()); Ssl::ServerContextSharedPtr mock_context = std::make_shared(); - EXPECT_CALL(context_manager, createSslServerContext(_, _, _)).WillOnce(Return(mock_context)); + EXPECT_CALL(context_manager, createSslServerContext(_, _, _, _)).WillOnce(Return(mock_context)); server_ssl_socket_factory.onAddOrUpdateSecret(); // Add another callback, it should be invoked immediately. @@ -4860,6 +4860,97 @@ TEST_P(SslSocketTest, ServerAddSecretsReadyCallback) { server_ssl_socket_factory.addReadyCb(second_callback_.AsStdFunction()); } +// Tests adding a callback and adding secrets in parallel. This is intended to +// catch a race condition introduced in +// https://github.com/envoyproxy/envoy/pull/13516 where a callback would never +// be called due to a concurrency bug. +TEST_P(SslSocketTest, ServerAddSecretsReadyCallbackParallel) { + Stats::TestUtil::TestStore stats_store; + NiceMock local_info; + testing::NiceMock factory_context; + NiceMock init_manager; + NiceMock dispatcher; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(ReturnRef(init_manager)); + EXPECT_CALL(factory_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); + + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + auto server_cfg = std::make_unique(tls_context, factory_context); + EXPECT_TRUE(server_cfg->tlsCertificates().empty()); + EXPECT_FALSE(server_cfg->isReady()); + + NiceMock context_manager; + ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), context_manager, + stats_store, std::vector{}); + + MockFunction mock_callback_; + EXPECT_CALL(mock_callback_, Call()); + Ssl::ServerContextSharedPtr mock_context = std::make_shared(); + EXPECT_CALL(context_manager, createSslServerContext(_, _, _, _)).WillOnce(Return(mock_context)); + + // Add a callback in a thread to potentially tickle the concurrency bug. + std::thread t([&server_ssl_socket_factory, &mock_callback_]() { + server_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); + }); + + // 1 microsecond of sleep seems to be a sweet spot for giving the addReadyCb + // thread enough of a head start to reliably cause the test to fail with buggy + // code. + std::this_thread::sleep_for(std::chrono::microseconds(1)); + server_ssl_socket_factory.onAddOrUpdateSecret(); + + t.join(); +} + +// Tests adding a callback and adding secrets in parallel. This is intended to +// catch a race condition introduced in +// https://github.com/envoyproxy/envoy/pull/13516 where a callback would never +// be called due to a concurrency bug. +TEST_P(SslSocketTest, ClientAddSecretsReadyCallbackParallel) { + Stats::TestUtil::TestStore stats_store; + NiceMock local_info; + testing::NiceMock factory_context; + NiceMock init_manager; + NiceMock dispatcher; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(ReturnRef(init_manager)); + EXPECT_CALL(factory_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + auto client_cfg = std::make_unique(tls_context, factory_context); + EXPECT_TRUE(client_cfg->tlsCertificates().empty()); + EXPECT_FALSE(client_cfg->isReady()); + + NiceMock context_manager; + ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), context_manager, + stats_store); + + MockFunction mock_callback_; + EXPECT_CALL(mock_callback_, Call()); + Ssl::ClientContextSharedPtr mock_context = std::make_shared(); + EXPECT_CALL(context_manager, createSslClientContext(_, _, _)).WillOnce(Return(mock_context)); + + // Add a callback in a thread to potentially tickle the concurrency bug. + std::thread t([&client_ssl_socket_factory, &mock_callback_]() { + client_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); + }); + + std::this_thread::sleep_for(std::chrono::microseconds(1)); + client_ssl_socket_factory.onAddOrUpdateSecret(); + + t.join(); +} + TEST_P(SslSocketTest, TestTransportSocketCallback) { // Make MockTransportSocketCallbacks. Network::MockIoHandle io_handle; From 6e61b4d6e48383383a3bf05ec4ab2d1ec08b1199 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Thu, 29 Apr 2021 12:16:37 -0400 Subject: [PATCH 3/6] fix release notes and mock Signed-off-by: Michael Puncel --- docs/root/version_history/current.rst | 4 +--- test/mocks/upstream/host.h | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index be445a0a2a0d4..36198f3a0703b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -19,13 +19,11 @@ 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 ` 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 ` until the next attempt. * http: raise max configurable max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB in http connection manager. * validation: fix an issue that causes TAP sockets to panic during config validation mode. * xray: fix the default sampling 'rate' for AWS X-Ray tracer extension to be 5% as opposed to 50%. * zipkin: fix timestamp serializaiton in annotations. A prior bug fix exposed an issue with timestamps being serialized as strings. -* active health checks: health checks using a TLS transport socket and secrets delivered via :ref:`SDS ` 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 ` until the next attempt. - -* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. Removed Config or Runtime ------------------------- diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index be46ff312a532..b3484c88fdd6b 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -193,7 +193,7 @@ class MockHost : public Host { MOCK_METHOD(void, priority, (uint32_t)); MOCK_METHOD(bool, warmed, (), (const)); MOCK_METHOD(MonotonicTime, creationTime, (), (const)); - MOCK_METHOD(void, addReadyCb, (std::function, const envoy::config::core::v3::Metadata*), + MOCK_METHOD(void, addHealthCheckingReadyCb, (std::function, const envoy::config::core::v3::Metadata*), (const)); testing::NiceMock cluster_; From cf186156ec72963dc9a7f83af917bff8de7bdadd Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Fri, 30 Apr 2021 11:04:33 -0400 Subject: [PATCH 4/6] fix format, remove sleep from test Signed-off-by: Michael Puncel --- include/envoy/upstream/upstream.h | 5 +++-- source/common/upstream/health_checker_base_impl.cc | 3 ++- source/common/upstream/upstream_impl.cc | 2 +- source/common/upstream/upstream_impl.h | 2 +- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 9 ++------- test/mocks/upstream/host.h | 4 ++-- 6 files changed, 11 insertions(+), 14 deletions(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 5bd8b309a23ab..72a812811631a 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -115,8 +115,9 @@ class Host : virtual public HostDescription { * @param callback supplies the callback to be invoked. * @param metadata supplies the metadata to be used for resolving transport socket matches. */ - virtual void addHealthCheckingReadyCb(std::function callback, - const envoy::config::core::v3::Metadata* metadata) const PURE; + virtual void + addHealthCheckingReadyCb(std::function callback, + const envoy::config::core::v3::Metadata* metadata) const PURE; /** * @return host specific gauges. diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc index d4d5c942ce16d..b35faab92288e 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -416,7 +416,8 @@ void HealthCheckerImplBase::ActiveHealthCheckSession::start() { // 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_->addHealthCheckingReadyCb([this] { onInitialInterval(); }, parent_.transportSocketMatchMetadata().get()); + host_->addHealthCheckingReadyCb([this] { onInitialInterval(); }, + parent_.transportSocketMatchMetadata().get()); } void HealthCheckerImplBase::ActiveHealthCheckSession::onInitialInterval() { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index a4237002209d5..36c97aadf4095 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -350,7 +350,7 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu } void HostImpl::addHealthCheckingReadyCb(std::function callback, - const envoy::config::core::v3::Metadata* metadata) const { + const envoy::config::core::v3::Metadata* metadata) const { Network::TransportSocketFactory& factory = (metadata != nullptr) ? resolveTransportSocketFactory(healthCheckAddress(), metadata) : transportSocketFactory(); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index df10b1f8c6544..d90eabee60fca 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -213,7 +213,7 @@ class HostImpl : public HostDescriptionImpl, Network::TransportSocketOptionsSharedPtr transport_socket_options, const envoy::config::core::v3::Metadata* metadata) const override; void addHealthCheckingReadyCb(std::function callback, - const envoy::config::core::v3::Metadata* metadata) const override; + const envoy::config::core::v3::Metadata* metadata) const override; std::vector> gauges() const override { diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index dec020b58e387..aa309828a216e 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4860,7 +4860,7 @@ TEST_P(SslSocketTest, ServerAddSecretsReadyCallback) { server_ssl_socket_factory.addReadyCb(second_callback_.AsStdFunction()); } -// Tests adding a callback and adding secrets in parallel. This is intended to +// Tests adding a callback and adding secrets in parallel. This is intended to // catch a race condition introduced in // https://github.com/envoyproxy/envoy/pull/13516 where a callback would never // be called due to a concurrency bug. @@ -4898,16 +4898,12 @@ TEST_P(SslSocketTest, ServerAddSecretsReadyCallbackParallel) { server_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); }); - // 1 microsecond of sleep seems to be a sweet spot for giving the addReadyCb - // thread enough of a head start to reliably cause the test to fail with buggy - // code. - std::this_thread::sleep_for(std::chrono::microseconds(1)); server_ssl_socket_factory.onAddOrUpdateSecret(); t.join(); } -// Tests adding a callback and adding secrets in parallel. This is intended to +// Tests adding a callback and adding secrets in parallel. This is intended to // catch a race condition introduced in // https://github.com/envoyproxy/envoy/pull/13516 where a callback would never // be called due to a concurrency bug. @@ -4945,7 +4941,6 @@ TEST_P(SslSocketTest, ClientAddSecretsReadyCallbackParallel) { client_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); }); - std::this_thread::sleep_for(std::chrono::microseconds(1)); client_ssl_socket_factory.onAddOrUpdateSecret(); t.join(); diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index b3484c88fdd6b..a973783a5a09f 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -193,8 +193,8 @@ class MockHost : public Host { MOCK_METHOD(void, priority, (uint32_t)); MOCK_METHOD(bool, warmed, (), (const)); MOCK_METHOD(MonotonicTime, creationTime, (), (const)); - MOCK_METHOD(void, addHealthCheckingReadyCb, (std::function, const envoy::config::core::v3::Metadata*), - (const)); + MOCK_METHOD(void, addHealthCheckingReadyCb, + (std::function, const envoy::config::core::v3::Metadata*), (const)); testing::NiceMock cluster_; Network::TransportSocketFactoryPtr socket_factory_; From 7da3fee79aa3fbd205900984ee741e9dddbb57ea Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Fri, 30 Apr 2021 13:48:28 -0400 Subject: [PATCH 5/6] implement no-op addReadyCb() on starttls socket factory Signed-off-by: Michael Puncel --- source/extensions/transport_sockets/starttls/starttls_socket.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/transport_sockets/starttls/starttls_socket.h b/source/extensions/transport_sockets/starttls/starttls_socket.h index cb35e4d00eadf..77afdbb64a7c9 100644 --- a/source/extensions/transport_sockets/starttls/starttls_socket.h +++ b/source/extensions/transport_sockets/starttls/starttls_socket.h @@ -82,6 +82,7 @@ class ServerStartTlsSocketFactory : public Network::TransportSocketFactory, createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override { return false; } bool usesProxyProtocolOptions() const override { return false; } + void addReadyCb(std::function callback) override { callback(); } private: Network::TransportSocketFactoryPtr raw_socket_factory_; From c3a2f41a7e2f6e35fd19e3b6c8a820e577f010eb Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Fri, 30 Apr 2021 15:43:34 -0400 Subject: [PATCH 6/6] fix test failure Signed-off-by: Michael Puncel --- test/common/upstream/health_checker_impl_test.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index ee198ad1ba763..ac3357e63f02d 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -2612,7 +2612,11 @@ TEST_F(HttpHealthCheckerImplTest, TransportSocketMatchCriteria) { expectStreamCreate(0); EXPECT_CALL(*test_sessions_[0]->timeout_timer_, enableTimer(_, _)); health_checker_->start(); - EXPECT_EQ(health_transport_socket_stats.total_match_count_.value(), 1); + + // We expect 2 transport socket matches: one for when + // addHealthCheckingReadyCb() evaluates the match to register a callback on + // the socket, and once when the health checks are actually performed. + EXPECT_EQ(health_transport_socket_stats.total_match_count_.value(), 2); } TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) {