diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 39ffddd2085f4..574e461d45015 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -24,6 +24,7 @@ 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: port stripping now works for CONNECT requests, though the port will be restored if the CONNECT request is sent upstream. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.strip_port_from_connect`` to false. * 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. 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..72a812811631a 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -109,6 +109,16 @@ 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 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 + addHealthCheckingReadyCb(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..b35faab92288e 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -411,6 +411,15 @@ 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_->addHealthCheckingReadyCb([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..36c97aadf4095 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::addHealthCheckingReadyCb(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..d90eabee60fca 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 addHealthCheckingReadyCb(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/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_; 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..60b82eefac69c 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -380,13 +380,38 @@ 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; + } else { + secrets_ready_callbacks_.push_back(callback); + } + } + if (immediately_run_callback) { + callback(); + } +} + ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope, @@ -421,13 +446,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; + } else { + secrets_ready_callbacks_.push_back(callback); + } + } + if (immediately_run_callback) { + 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..ac3357e63f02d 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(_)); @@ -2604,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) { @@ -2624,6 +2636,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..aa309828a216e 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,183 @@ 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()); +} + +// 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()); + }); + + 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()); + }); + + client_ssl_socket_factory.onAddOrUpdateSecret(); + + t.join(); +} + 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..a973783a5a09f 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, addHealthCheckingReadyCb, + (std::function, const envoy::config::core::v3::Metadata*), (const)); testing::NiceMock cluster_; Network::TransportSocketFactoryPtr socket_factory_;