From ce3fdb9a1753b418a397ab2a8c72ffa21a9ae764 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Fri, 9 Oct 2020 12:31:35 -0400 Subject: [PATCH 01/12] delay health checks until transport socket secrets are ready. This fixes a sequencing issue that could result in undesirable Envoy bootup times. When active health checks are configured on a cluster using a transport socket that uses SDS to fetch secrets, it was possible for the initial health check to run before secrets are loaded, which results in a failure. The health check would then wait for the no_traffic_interval (default of 60s) before trying again. This commit makes health checks wait until secrets are loaded before starting, implemented by adding support for registering a callback on TransportSocketFactory. SslSocketFactory supports invoking the callback when secrets are loaded, and all other socket types currently invoke the callback immediately, preserving existing behavior. Signed-off-by: Michael Puncel --- include/envoy/network/transport_socket.h | 7 +++ include/envoy/upstream/upstream.h | 9 ++++ source/common/network/raw_buffer_socket.h | 1 + .../upstream/health_checker_base_impl.cc | 10 ++++ .../upstream/health_checker_base_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 8 +++ source/common/upstream/upstream_impl.h | 2 + .../quiche/quic_transport_socket_factory.h | 3 ++ .../transport_sockets/alts/tsi_socket.h | 3 ++ .../proxy_protocol/proxy_protocol.h | 1 + source/extensions/transport_sockets/tap/tap.h | 2 + .../transport_sockets/tls/ssl_socket.cc | 32 +++++++++++ .../transport_sockets/tls/ssl_socket.h | 6 +++ .../upstream/health_checker_impl_test.cc | 32 ++++++++--- .../upstream/transport_socket_matcher_test.cc | 4 ++ .../transport_sockets/tls/ssl_socket_test.cc | 54 +++++++++++++++++++ test/mocks/network/transport_socket.h | 1 + test/mocks/upstream/host.h | 2 + 18 files changed, 171 insertions(+), 8 deletions(-) diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h index fe054ce2f16d..9e08cc653bae 100644 --- a/include/envoy/network/transport_socket.h +++ b/include/envoy/network/transport_socket.h @@ -226,6 +226,13 @@ class TransportSocketFactory { */ virtual TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const PURE; + + /** + * @param a callback to be invoked when the secrets required by the created transport + * sockets are ready. Will be invoked immediately if no secrets are required or if they + * are already loaded. + */ + virtual void addSecretsReadyCb(std::function callback) PURE; }; using TransportSocketFactoryPtr = std::unique_ptr; diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 127df14c923a..936470d47af8 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -112,6 +112,15 @@ class Host : virtual public HostDescription { Network::TransportSocketOptionsSharedPtr transport_socket_options, const envoy::config::core::v3::Metadata* metadata) const PURE; + /** + * Register a callback to be invoked when secrets are ready for the transport socket that + * corresponds to the provided metadata. + * @param callback supplies the callback to be invoked. + * @param metadata supplises the metadata to be used for resolving transport socket matches. + */ + virtual void addSecretsReadyCb(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 fe87bbeda605..09ed947b205c 100644 --- a/source/common/network/raw_buffer_socket.h +++ b/source/common/network/raw_buffer_socket.h @@ -32,6 +32,7 @@ class RawBufferSocketFactory : public TransportSocketFactory { // Network::TransportSocketFactory TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; + void addSecretsReadyCb(std::function callback) override { callback(); } }; } // namespace Network diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc index 47a67c3f29a7..bcb9b767fd78 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -379,6 +379,16 @@ 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_->addSecretsReadyCb([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 ff2f62101f57..d1dbe0a8c2e4 100644 --- a/source/common/upstream/health_checker_base_impl.h +++ b/source/common/upstream/health_checker_base_impl.h @@ -60,7 +60,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 8e6a6db3c507..994d0c84146b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -356,6 +356,14 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu return connection; } +void HostImpl::addSecretsReadyCb(std::function callback, + const envoy::config::core::v3::Metadata* metadata) const { + Network::TransportSocketFactory& factory = + (metadata != nullptr) ? cluster_->transportSocketMatcher().resolve(metadata).factory_ + : socket_factory_; + factory.addSecretsReadyCb(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 c74e489384f0..5e7b88f64e2f 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -189,6 +189,8 @@ class HostImpl : public HostDescriptionImpl, createHealthCheckConnection(Event::Dispatcher& dispatcher, Network::TransportSocketOptionsSharedPtr transport_socket_options, const envoy::config::core::v3::Metadata* metadata) const override; + void addSecretsReadyCb(std::function callback, + const envoy::config::core::v3::Metadata* metadata) const override; std::vector> gauges() const override { diff --git a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h index 2ada9e2de17b..a4b035beaafc 100644 --- a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h +++ b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h @@ -24,6 +24,9 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { NOT_REACHED_GCOVR_EXCL_LINE; } bool implementsSecureTransport() const override { return true; } + + // TODO(mpuncel) only invoke callback() once secrets are ready. + void addSecretsReadyCb(std::function callback) override { callback() }; }; // TODO(danzh): when implement ProofSource, examine of it's necessary to diff --git a/source/extensions/transport_sockets/alts/tsi_socket.h b/source/extensions/transport_sockets/alts/tsi_socket.h index 0acba405022d..42bc6df83693 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.h +++ b/source/extensions/transport_sockets/alts/tsi_socket.h @@ -101,6 +101,9 @@ class TsiSocketFactory : public Network::TransportSocketFactory { Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; + // TODO(mpuncel) only invoke callback() once secrets are ready. + void addSecretsReadyCb(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 4a191ebf539d..08826b55b260 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h @@ -49,6 +49,7 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; + void addSecretsReadyCb(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 33156b705153..b8e3463bad47 100644 --- a/source/extensions/transport_sockets/tap/tap.h +++ b/source/extensions/transport_sockets/tap/tap.h @@ -41,6 +41,8 @@ class TapSocketFactory : public Network::TransportSocketFactory, Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; + // TODO(mpuncel) only invoke callback() once secrets are ready. + void addSecretsReadyCb(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 485468443096..d53b41577940 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -358,10 +358,26 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() { { absl::WriterMutexLock l(&ssl_ctx_mu_); ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_); + if (ssl_ctx_) { + for (const auto& cb : secrets_ready_callbacks_) { + cb(); + } + } } stats_.ssl_context_update_by_sds_.inc(); } +void ClientSslSocketFactory::addSecretsReadyCb(std::function callback) { + { + absl::ReaderMutexLock l(&ssl_ctx_mu_); + if (ssl_ctx_) { + callback(); + } else { + secrets_ready_callbacks_.push_back(callback); + } + } +} + ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope, @@ -399,10 +415,26 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() { { absl::WriterMutexLock l(&ssl_ctx_mu_); ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_); + if (ssl_ctx_) { + for (const auto& cb : secrets_ready_callbacks_) { + cb(); + } + } } stats_.ssl_context_update_by_sds_.inc(); } +void ServerSslSocketFactory::addSecretsReadyCb(std::function callback) { + { + absl::ReaderMutexLock l(&ssl_ctx_mu_); + if (ssl_ctx_) { + 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 c14cb502bed1..a3f447fd9e08 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -109,6 +109,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; + void addSecretsReadyCb(std::function callback) override; + // Secret::SecretCallbacks void onAddOrUpdateSecret() override; @@ -119,6 +121,7 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, Envoy::Ssl::ClientContextConfigPtr config_; mutable absl::Mutex ssl_ctx_mu_; Envoy::Ssl::ClientContextSharedPtr ssl_ctx_ ABSL_GUARDED_BY(ssl_ctx_mu_); + std::list> secrets_ready_callbacks_; }; class ServerSslSocketFactory : public Network::TransportSocketFactory, @@ -133,6 +136,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; + void addSecretsReadyCb(std::function callback) override; + // Secret::SecretCallbacks void onAddOrUpdateSecret() override; @@ -144,6 +149,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 a3c0cf04b1cb..1ba79200ce61 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -923,11 +923,15 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) { Network::TransportSocketFactoryPtr(socket_factory)); cluster_->info_->transport_socket_matcher_.reset(transport_socket_match); + EXPECT_CALL(*socket_factory, addSecretsReadyCb(_)) + .WillOnce(Invoke( + [&](std::function callback) -> void { + callback(); + })); EXPECT_CALL(*socket_factory, createTransportSocket(ApplicationProtocolListEq("http1"))); allocHealthChecker(yaml); - cluster_->prioritySet().getMockHostSet(0)->hosts_ = { - makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->prioritySet().getMockHostSet(0)->hosts_ = {makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; cluster_->info_->stats().upstream_cx_total_.inc(); expectSessionCreate(); expectStreamCreate(0); @@ -2387,13 +2391,21 @@ 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, addSecretsReadyCb(_)) + .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(_)); @@ -2429,6 +2441,12 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) { )EOF"; auto default_socket_factory = std::make_unique(); + + EXPECT_CALL(*default_socket_factory, addSecretsReadyCb(_)) + .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 cfde130d1d1f..3ac0485acb21 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -33,6 +33,8 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); + MOCK_METHOD(void, addSecretsReadyCb, + (std::function)); FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} std::string id() const { return id_; } @@ -48,6 +50,8 @@ class FooTransportSocketFactory MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); + MOCK_METHOD(void, addSecretsReadyCb, + (std::function), (const)); 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 b4bdb84e5737..fe3068db737f 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; @@ -4490,6 +4491,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.addSecretsReadyCb(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()); @@ -4525,6 +4532,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.addSecretsReadyCb(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()); @@ -4536,6 +4549,47 @@ 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, AddSecretsReadyCallback) { + 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.addSecretsReadyCb(mock_callback_.AsStdFunction()); + + 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.addSecretsReadyCb(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 ee53570c20ac..29678f0ddf3d 100644 --- a/test/mocks/network/transport_socket.h +++ b/test/mocks/network/transport_socket.h @@ -38,6 +38,7 @@ class MockTransportSocketFactory : public TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(TransportSocketPtr, createTransportSocket, (TransportSocketOptionsSharedPtr), (const)); + MOCK_METHOD(void, addSecretsReadyCb, (std::function)); }; } // namespace Network diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index 95183622dbb7..fc652eb07569 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -192,6 +192,8 @@ class MockHost : public Host { MOCK_METHOD(uint32_t, priority, (), (const)); MOCK_METHOD(void, priority, (uint32_t)); MOCK_METHOD(bool, warmed, (), (const)); + MOCK_METHOD(void, addSecretsReadyCb, + (std::function, const envoy::config::core::v3::Metadata*), (const)); testing::NiceMock cluster_; Network::TransportSocketFactoryPtr socket_factory_; From 122a8ffe3776e27724804fc091393e7db075ec51 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Mon, 12 Oct 2020 14:35:41 -0400 Subject: [PATCH 02/12] fix format, git add release notes Signed-off-by: Michael Puncel --- docs/root/version_history/current.rst | 2 ++ .../upstream/health_checker_base_impl.cc | 5 ++-- source/common/upstream/upstream_impl.cc | 2 +- .../quiche/quic_transport_socket_factory.h | 2 +- .../upstream/health_checker_impl_test.cc | 27 +++++++------------ .../upstream/transport_socket_matcher_test.cc | 6 ++--- test/mocks/upstream/host.h | 2 +- 7 files changed, 19 insertions(+), 27 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b958b448dfe1..bfef1b5ddd25 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* active health checks: Health checks using a TLS transport socket and secrets delivered via :ref:`SDS ` 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. + Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc index bcb9b767fd78..80021d006b7e 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -384,9 +384,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_->addSecretsReadyCb([this]{ - onInitialInterval(); - }, parent_.transportSocketMatchMetadata().get()); + host_->addSecretsReadyCb([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 994d0c84146b..06b50bca92c7 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -357,7 +357,7 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu } void HostImpl::addSecretsReadyCb(std::function callback, - const envoy::config::core::v3::Metadata* metadata) const { + const envoy::config::core::v3::Metadata* metadata) const { Network::TransportSocketFactory& factory = (metadata != nullptr) ? cluster_->transportSocketMatcher().resolve(metadata).factory_ : socket_factory_; diff --git a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h index a4b035beaafc..9f9064294dcf 100644 --- a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h +++ b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h @@ -26,7 +26,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { bool implementsSecureTransport() const override { return true; } // TODO(mpuncel) only invoke callback() once secrets are ready. - void addSecretsReadyCb(std::function callback) override { callback() }; + void addSecretsReadyCb(std::function callback) override{callback()}; }; // TODO(danzh): when implement ProofSource, examine of it's necessary to diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 1ba79200ce61..fbba7de38507 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -924,14 +924,12 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) { cluster_->info_->transport_socket_matcher_.reset(transport_socket_match); EXPECT_CALL(*socket_factory, addSecretsReadyCb(_)) - .WillOnce(Invoke( - [&](std::function callback) -> void { - callback(); - })); + .WillOnce(Invoke([&](std::function callback) -> void { callback(); })); EXPECT_CALL(*socket_factory, createTransportSocket(ApplicationProtocolListEq("http1"))); allocHealthChecker(yaml); - cluster_->prioritySet().getMockHostSet(0)->hosts_ = {makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; + cluster_->prioritySet().getMockHostSet(0)->hosts_ = { + makeTestHost(cluster_->info_, "tcp://127.0.0.1:80")}; cluster_->info_->stats().upstream_cx_total_.inc(); expectSessionCreate(); expectStreamCreate(0); @@ -2391,10 +2389,11 @@ 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 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 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))) .Times(2) @@ -2402,10 +2401,7 @@ TEST_F(HttpHealthCheckerImplTest, TransportSocketMatchCriteria) { *health_check_only_socket_factory, health_transport_socket_stats, "health_check_only"))) .RetiresOnSaturation(); EXPECT_CALL(*health_check_only_socket_factory, addSecretsReadyCb(_)) - .WillOnce(Invoke( - [&](std::function callback) -> void { - callback(); - })); + .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(_)); @@ -2443,10 +2439,7 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) { auto default_socket_factory = std::make_unique(); EXPECT_CALL(*default_socket_factory, addSecretsReadyCb(_)) - .WillOnce(Invoke( - [&](std::function callback) -> void { - callback(); - })); + .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 3ac0485acb21..9065776c45cb 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -33,8 +33,7 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(void, addSecretsReadyCb, - (std::function)); + MOCK_METHOD(void, addSecretsReadyCb, (std::function)); FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} std::string id() const { return id_; } @@ -50,8 +49,7 @@ class FooTransportSocketFactory MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(void, addSecretsReadyCb, - (std::function), (const)); + MOCK_METHOD(void, addSecretsReadyCb, (std::function), (const)); Network::TransportSocketFactoryPtr createTransportSocketFactory(const Protobuf::Message& proto, diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index fc652eb07569..4f9a9e8fb5ae 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(void, addSecretsReadyCb, - (std::function, const envoy::config::core::v3::Metadata*), (const)); + (std::function, const envoy::config::core::v3::Metadata*), (const)); testing::NiceMock cluster_; Network::TransportSocketFactoryPtr socket_factory_; From 1b1a5b2119beb1c7886a452cf3ba1d075a8464d0 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Mon, 12 Oct 2020 14:50:27 -0400 Subject: [PATCH 03/12] fix spelling error Signed-off-by: Michael Puncel --- include/envoy/upstream/upstream.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 936470d47af8..7e0a889cfad6 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -116,7 +116,7 @@ class Host : virtual public HostDescription { * 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 supplises the metadata to be used for resolving transport socket matches. + * @param metadata supplies the metadata to be used for resolving transport socket matches. */ virtual void addSecretsReadyCb(std::function callback, const envoy::config::core::v3::Metadata* metadata) const PURE; From c7c0aead53c82bafff14b0f6f8733c9ff74ee3bc Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Mon, 12 Oct 2020 15:57:50 -0400 Subject: [PATCH 04/12] make check_format.py regex less strict on version history lines Signed-off-by: Michael Puncel --- docs/root/version_history/current.rst | 2 +- tools/code_format/check_format.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index bfef1b5ddd25..90c09a167222 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,7 +15,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. +* 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. Removed Config or Runtime ------------------------- diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 5fd0fa59ded3..c91c4be73725 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -129,7 +129,7 @@ COMMENT_REGEX = re.compile(r"//|\*") DURATION_VALUE_REGEX = re.compile(r'\b[Dd]uration\(([0-9.]+)') PROTO_VALIDATION_STRING = re.compile(r'\bmin_bytes\b') -VERSION_HISTORY_NEW_LINE_REGEX = re.compile("\* ([a-z \-_]+): ([a-z:`]+)") +VERSION_HISTORY_NEW_LINE_REGEX = re.compile("\* ([a-z \-_]+): ([A-Za-z:. `]+)") VERSION_HISTORY_SECTION_NAME = re.compile("^[A-Z][A-Za-z ]*$") RELOADABLE_FLAG_REGEX = re.compile(".*(.)(envoy.reloadable_features.[^ ]*)\s.*") # Check for punctuation in a terminal ref clause, e.g. From 6ed4855a1b88649bde64e20cc9ef31d7b6f12eb0 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Mon, 12 Oct 2020 21:46:54 -0400 Subject: [PATCH 05/12] Undo changes to check_format.py Signed-off-by: Michael Puncel --- tools/code_format/check_format.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index c91c4be73725..5fd0fa59ded3 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -129,7 +129,7 @@ COMMENT_REGEX = re.compile(r"//|\*") DURATION_VALUE_REGEX = re.compile(r'\b[Dd]uration\(([0-9.]+)') PROTO_VALIDATION_STRING = re.compile(r'\bmin_bytes\b') -VERSION_HISTORY_NEW_LINE_REGEX = re.compile("\* ([a-z \-_]+): ([A-Za-z:. `]+)") +VERSION_HISTORY_NEW_LINE_REGEX = re.compile("\* ([a-z \-_]+): ([a-z:`]+)") VERSION_HISTORY_SECTION_NAME = re.compile("^[A-Z][A-Za-z ]*$") RELOADABLE_FLAG_REGEX = re.compile(".*(.)(envoy.reloadable_features.[^ ]*)\s.*") # Check for punctuation in a terminal ref clause, e.g. From df066b94ddd9bd9181b14c9c70ea03e580718314 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Mon, 12 Oct 2020 22:27:00 -0400 Subject: [PATCH 06/12] fix missing semicolon Signed-off-by: Michael Puncel --- .../quic_listeners/quiche/quic_transport_socket_factory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h index 9f9064294dcf..f6b4c92e39dc 100644 --- a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h +++ b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h @@ -26,7 +26,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { bool implementsSecureTransport() const override { return true; } // TODO(mpuncel) only invoke callback() once secrets are ready. - void addSecretsReadyCb(std::function callback) override{callback()}; + void addSecretsReadyCb(std::function callback) override { callback(); }; }; // TODO(danzh): when implement ProofSource, examine of it's necessary to From 47075f3a90e7240bab7f1dab9e5f29c5644ad9f1 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Tue, 13 Oct 2020 07:09:22 -0400 Subject: [PATCH 07/12] fix a mock with the wrong signature Signed-off-by: Michael Puncel --- test/common/upstream/transport_socket_matcher_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index 9065776c45cb..2c7faaa27b6f 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -49,7 +49,7 @@ class FooTransportSocketFactory MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(void, addSecretsReadyCb, (std::function), (const)); + MOCK_METHOD(void, addSecretsReadyCb, (std::function)); Network::TransportSocketFactoryPtr createTransportSocketFactory(const Protobuf::Message& proto, From 24243d8d96a5a7cab35a1e71c63a11e911a3654c Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Tue, 13 Oct 2020 10:41:18 -0400 Subject: [PATCH 08/12] fix deadlock running callbacks Signed-off-by: Michael Puncel --- .../transport_sockets/tls/ssl_socket.cc | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index d53b41577940..710e7cc745c5 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -355,13 +355,17 @@ 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_) { - for (const auto& cb : secrets_ready_callbacks_) { - cb(); - } + should_run_callbacks = true; + } + } + if (should_run_callbacks) { + for (const auto& cb : secrets_ready_callbacks_) { + cb(); } } stats_.ssl_context_update_by_sds_.inc(); @@ -412,13 +416,18 @@ 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_) { - for (const auto& cb : secrets_ready_callbacks_) { - cb(); - } + should_run_callbacks = true; + } + } + if (should_run_callbacks) { + for (const auto& cb : secrets_ready_callbacks_) { + cb(); } } stats_.ssl_context_update_by_sds_.inc(); From d3bba479368b28902215c790acfdbb174c4f154a Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Tue, 13 Oct 2020 12:58:58 -0400 Subject: [PATCH 09/12] fix another deadlock in addSecretsReadyCb Signed-off-by: Michael Puncel --- .../transport_sockets/tls/ssl_socket.cc | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 710e7cc745c5..2900446dd2bb 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -372,14 +372,19 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() { } void ClientSslSocketFactory::addSecretsReadyCb(std::function callback) { + bool immediately_run_callback = false; { absl::ReaderMutexLock l(&ssl_ctx_mu_); if (ssl_ctx_) { - callback(); - } else { - secrets_ready_callbacks_.push_back(callback); + immediately_run_callback = true; } } + + if (immediately_run_callback) { + callback(); + } else { + secrets_ready_callbacks_.push_back(callback); + } } ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config, @@ -434,14 +439,18 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() { } void ServerSslSocketFactory::addSecretsReadyCb(std::function callback) { + bool immediately_run_callback = false; { absl::ReaderMutexLock l(&ssl_ctx_mu_); if (ssl_ctx_) { - callback(); - } else { - secrets_ready_callbacks_.push_back(callback); + immediately_run_callback = true; } } + if (immediately_run_callback) { + callback(); + } else { + secrets_ready_callbacks_.push_back(callback); + } } } // namespace Tls From 177e31c49bcf646dc9f3ac03504c23141fe15fb6 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Tue, 13 Oct 2020 20:38:18 -0400 Subject: [PATCH 10/12] add more unit test coverage Signed-off-by: Michael Puncel --- .../transport_sockets/tls/ssl_socket_test.cc | 52 ++++++++++++++++++- test/mocks/ssl/mocks.cc | 3 ++ test/mocks/ssl/mocks.h | 11 ++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index fe3068db737f..b7bb15038410 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4550,7 +4550,7 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { } // Validate that secrets callbacks are invoked when secrets become ready. -TEST_P(SslSocketTest, AddSecretsReadyCallback) { +TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { Stats::TestUtil::TestStore stats_store; NiceMock local_info; testing::NiceMock factory_context; @@ -4579,6 +4579,10 @@ TEST_P(SslSocketTest, AddSecretsReadyCallback) { EXPECT_CALL(mock_callback_, Call()).Times(0); client_ssl_socket_factory.addSecretsReadyCb(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)); @@ -4590,6 +4594,52 @@ TEST_P(SslSocketTest, AddSecretsReadyCallback) { client_ssl_socket_factory.addSecretsReadyCb(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.addSecretsReadyCb(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.addSecretsReadyCb(second_callback_.AsStdFunction()); +} + TEST_P(SslSocketTest, TestTransportSocketCallback) { // Make MockTransportSocketCallbacks. Network::MockIoHandle io_handle; diff --git a/test/mocks/ssl/mocks.cc b/test/mocks/ssl/mocks.cc index 50ed3f3ae6c0..14ee85239d87 100644 --- a/test/mocks/ssl/mocks.cc +++ b/test/mocks/ssl/mocks.cc @@ -15,6 +15,9 @@ MockClientContext::~MockClientContext() = default; MockClientContextConfig::MockClientContextConfig() = default; 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 6a5cbe8df649..beafd9de8720 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -97,6 +97,17 @@ class MockClientContextConfig : public ClientContextConfig { MOCK_METHOD(const std::string&, signingAlgorithmsForTest, (), (const)); }; +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(); From 2cd1719213d8d267c1942a052d8ecae9b319d0d0 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Tue, 13 Oct 2020 20:57:01 -0400 Subject: [PATCH 11/12] PR feedback: rename addSecretsReadyCb to addReadyCb Signed-off-by: Michael Puncel --- include/envoy/network/transport_socket.h | 2 +- include/envoy/upstream/upstream.h | 4 ++-- source/common/network/raw_buffer_socket.h | 2 +- source/common/upstream/health_checker_base_impl.cc | 3 +-- source/common/upstream/upstream_impl.cc | 6 +++--- source/common/upstream/upstream_impl.h | 4 ++-- .../quiche/quic_transport_socket_factory.h | 2 +- .../extensions/transport_sockets/alts/tsi_socket.h | 2 +- .../proxy_protocol/proxy_protocol.h | 2 +- source/extensions/transport_sockets/tap/tap.h | 2 +- .../extensions/transport_sockets/tls/ssl_socket.cc | 4 ++-- source/extensions/transport_sockets/tls/ssl_socket.h | 4 ++-- test/common/upstream/health_checker_impl_test.cc | 6 +++--- .../common/upstream/transport_socket_matcher_test.cc | 4 ++-- .../transport_sockets/tls/ssl_socket_test.cc | 12 ++++++------ test/mocks/network/transport_socket.h | 2 +- test/mocks/upstream/host.h | 4 ++-- 17 files changed, 32 insertions(+), 33 deletions(-) diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h index 9e08cc653bae..db500f86a8a6 100644 --- a/include/envoy/network/transport_socket.h +++ b/include/envoy/network/transport_socket.h @@ -232,7 +232,7 @@ class TransportSocketFactory { * sockets are ready. Will be invoked immediately if no secrets are required or if they * are already loaded. */ - virtual void addSecretsReadyCb(std::function callback) PURE; + 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 7e0a889cfad6..2c51a7c261c4 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -118,8 +118,8 @@ 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 addSecretsReadyCb(std::function callback, - const envoy::config::core::v3::Metadata* metadata) const PURE; + 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 09ed947b205c..8f17279890aa 100644 --- a/source/common/network/raw_buffer_socket.h +++ b/source/common/network/raw_buffer_socket.h @@ -32,7 +32,7 @@ class RawBufferSocketFactory : public TransportSocketFactory { // Network::TransportSocketFactory TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - void addSecretsReadyCb(std::function callback) override { callback(); } + void addReadyCb(std::function callback) override { callback(); } }; } // namespace Network diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc index 759c06822c12..29e2aa6493c4 100644 --- a/source/common/upstream/health_checker_base_impl.cc +++ b/source/common/upstream/health_checker_base_impl.cc @@ -389,8 +389,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_->addSecretsReadyCb([this] { onInitialInterval(); }, - parent_.transportSocketMatchMetadata().get()); + host_->addReadyCb([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 06b50bca92c7..48d426f65250 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -356,12 +356,12 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu return connection; } -void HostImpl::addSecretsReadyCb(std::function callback, - const envoy::config::core::v3::Metadata* metadata) const { +void HostImpl::addReadyCb(std::function callback, + const envoy::config::core::v3::Metadata* metadata) const { Network::TransportSocketFactory& factory = (metadata != nullptr) ? cluster_->transportSocketMatcher().resolve(metadata).factory_ : socket_factory_; - factory.addSecretsReadyCb(callback); + factory.addReadyCb(callback); } void HostImpl::weight(uint32_t new_weight) { weight_ = std::max(1U, new_weight); } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 5e7b88f64e2f..ed8ef917b047 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -189,8 +189,8 @@ class HostImpl : public HostDescriptionImpl, createHealthCheckConnection(Event::Dispatcher& dispatcher, Network::TransportSocketOptionsSharedPtr transport_socket_options, const envoy::config::core::v3::Metadata* metadata) const override; - void addSecretsReadyCb(std::function callback, - 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/quic_listeners/quiche/quic_transport_socket_factory.h b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h index f6b4c92e39dc..4fbe83286b38 100644 --- a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h +++ b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h @@ -26,7 +26,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { bool implementsSecureTransport() const override { return true; } // TODO(mpuncel) only invoke callback() once secrets are ready. - void addSecretsReadyCb(std::function callback) override { callback(); }; + void addReadyCb(std::function callback) override { callback(); }; }; // TODO(danzh): when implement ProofSource, examine of it's necessary to diff --git a/source/extensions/transport_sockets/alts/tsi_socket.h b/source/extensions/transport_sockets/alts/tsi_socket.h index 42bc6df83693..7bf5877870ab 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.h +++ b/source/extensions/transport_sockets/alts/tsi_socket.h @@ -102,7 +102,7 @@ class TsiSocketFactory : public Network::TransportSocketFactory { createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; // TODO(mpuncel) only invoke callback() once secrets are ready. - void addSecretsReadyCb(std::function callback) override { callback(); }; + void addReadyCb(std::function callback) override { callback(); }; private: HandshakerFactory handshaker_factory_; diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h index 08826b55b260..bcddef7bf547 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h @@ -49,7 +49,7 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - void addSecretsReadyCb(std::function callback) override { callback(); }; + 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 b8e3463bad47..d04712b2a50a 100644 --- a/source/extensions/transport_sockets/tap/tap.h +++ b/source/extensions/transport_sockets/tap/tap.h @@ -42,7 +42,7 @@ class TapSocketFactory : public Network::TransportSocketFactory, createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; // TODO(mpuncel) only invoke callback() once secrets are ready. - void addSecretsReadyCb(std::function callback) override { callback(); }; + 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 2900446dd2bb..cea7eec5e348 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -371,7 +371,7 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() { stats_.ssl_context_update_by_sds_.inc(); } -void ClientSslSocketFactory::addSecretsReadyCb(std::function callback) { +void ClientSslSocketFactory::addReadyCb(std::function callback) { bool immediately_run_callback = false; { absl::ReaderMutexLock l(&ssl_ctx_mu_); @@ -438,7 +438,7 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() { stats_.ssl_context_update_by_sds_.inc(); } -void ServerSslSocketFactory::addSecretsReadyCb(std::function callback) { +void ServerSslSocketFactory::addReadyCb(std::function callback) { bool immediately_run_callback = false; { absl::ReaderMutexLock l(&ssl_ctx_mu_); diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index a3f447fd9e08..b0dcb139a319 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -109,7 +109,7 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - void addSecretsReadyCb(std::function callback) override; + void addReadyCb(std::function callback) override; // Secret::SecretCallbacks void onAddOrUpdateSecret() override; @@ -136,7 +136,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - void addSecretsReadyCb(std::function callback) override; + void addReadyCb(std::function callback) override; // Secret::SecretCallbacks void onAddOrUpdateSecret() override; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index f34b402e7ab1..cc24eff5273c 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -942,7 +942,7 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) { Network::TransportSocketFactoryPtr(socket_factory)); cluster_->info_->transport_socket_matcher_.reset(transport_socket_match); - EXPECT_CALL(*socket_factory, addSecretsReadyCb(_)) + EXPECT_CALL(*socket_factory, addReadyCb(_)) .WillOnce(Invoke([&](std::function callback) -> void { callback(); })); EXPECT_CALL(*socket_factory, createTransportSocket(ApplicationProtocolListEq("http1"))); @@ -2442,7 +2442,7 @@ TEST_F(HttpHealthCheckerImplTest, TransportSocketMatchCriteria) { .WillRepeatedly(Return(TransportSocketMatcher::MatchData( *health_check_only_socket_factory, health_transport_socket_stats, "health_check_only"))) .RetiresOnSaturation(); - EXPECT_CALL(*health_check_only_socket_factory, addSecretsReadyCb(_)) + 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. @@ -2480,7 +2480,7 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) { auto default_socket_factory = std::make_unique(); - EXPECT_CALL(*default_socket_factory, addSecretsReadyCb(_)) + 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. diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index 2c7faaa27b6f..f770d1f4fdd8 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -33,7 +33,7 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(void, addSecretsReadyCb, (std::function)); + MOCK_METHOD(void, addReadyCb, (std::function)); FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} std::string id() const { return id_; } @@ -49,7 +49,7 @@ class FooTransportSocketFactory MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(void, addSecretsReadyCb, (std::function)); + 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 b7bb15038410..b06a9ac66203 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4495,7 +4495,7 @@ TEST_P(SslSocketTest, DownstreamNotReadySslSocket) { // Add a secrets ready callback that should not be invoked. MockFunction mock_callback_; EXPECT_CALL(mock_callback_, Call()).Times(0); - server_ssl_socket_factory.addSecretsReadyCb(mock_callback_.AsStdFunction()); + server_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); auto transport_socket = server_ssl_socket_factory.createTransportSocket(nullptr); EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); @@ -4536,7 +4536,7 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { // Add a secrets ready callback that should not be invoked. MockFunction mock_callback_; EXPECT_CALL(mock_callback_, Call()).Times(0); - client_ssl_socket_factory.addSecretsReadyCb(mock_callback_.AsStdFunction()); + client_ssl_socket_factory.addReadyCb(mock_callback_.AsStdFunction()); auto transport_socket = client_ssl_socket_factory.createTransportSocket(nullptr); EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); @@ -4577,7 +4577,7 @@ TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { // 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.addSecretsReadyCb(mock_callback_.AsStdFunction()); + 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)); @@ -4591,7 +4591,7 @@ TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { // Add another callback, it should be invoked immediately. MockFunction second_callback_; EXPECT_CALL(second_callback_, Call()); - client_ssl_socket_factory.addSecretsReadyCb(second_callback_.AsStdFunction()); + client_ssl_socket_factory.addReadyCb(second_callback_.AsStdFunction()); } // Validate that secrets callbacks are invoked when secrets become ready. @@ -4622,7 +4622,7 @@ TEST_P(SslSocketTest, ServerAddSecretsReadyCallback) { // 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.addSecretsReadyCb(mock_callback_.AsStdFunction()); + 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)); @@ -4637,7 +4637,7 @@ TEST_P(SslSocketTest, ServerAddSecretsReadyCallback) { // Add another callback, it should be invoked immediately. MockFunction second_callback_; EXPECT_CALL(second_callback_, Call()); - server_ssl_socket_factory.addSecretsReadyCb(second_callback_.AsStdFunction()); + server_ssl_socket_factory.addReadyCb(second_callback_.AsStdFunction()); } TEST_P(SslSocketTest, TestTransportSocketCallback) { diff --git a/test/mocks/network/transport_socket.h b/test/mocks/network/transport_socket.h index 29678f0ddf3d..af8949aea99b 100644 --- a/test/mocks/network/transport_socket.h +++ b/test/mocks/network/transport_socket.h @@ -38,7 +38,7 @@ class MockTransportSocketFactory : public TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(TransportSocketPtr, createTransportSocket, (TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(void, addSecretsReadyCb, (std::function)); + MOCK_METHOD(void, addReadyCb, (std::function)); }; } // namespace Network diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index 4f9a9e8fb5ae..b5d857a5c184 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -192,8 +192,8 @@ class MockHost : public Host { MOCK_METHOD(uint32_t, priority, (), (const)); MOCK_METHOD(void, priority, (uint32_t)); MOCK_METHOD(bool, warmed, (), (const)); - MOCK_METHOD(void, addSecretsReadyCb, - (std::function, const envoy::config::core::v3::Metadata*), (const)); + MOCK_METHOD(void, addReadyCb, (std::function, const envoy::config::core::v3::Metadata*), + (const)); testing::NiceMock cluster_; Network::TransportSocketFactoryPtr socket_factory_; From 6b736edaaf1b28cb8328e022abe69f3be92a2f74 Mon Sep 17 00:00:00 2001 From: Michael Puncel Date: Fri, 16 Oct 2020 07:19:54 -0400 Subject: [PATCH 12/12] clear callbacks vector after invoking callbacks Signed-off-by: Michael Puncel --- source/extensions/transport_sockets/tls/ssl_socket.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index cea7eec5e348..523242f8fada 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -367,6 +367,7 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() { for (const auto& cb : secrets_ready_callbacks_) { cb(); } + secrets_ready_callbacks_.clear(); } stats_.ssl_context_update_by_sds_.inc(); } @@ -434,6 +435,7 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() { for (const auto& cb : secrets_ready_callbacks_) { cb(); } + secrets_ready_callbacks_.clear(); } stats_.ssl_context_update_by_sds_.inc(); }