diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h index 63169d3624e7c..fe054ce2f16dd 100644 --- a/include/envoy/network/transport_socket.h +++ b/include/envoy/network/transport_socket.h @@ -226,11 +226,6 @@ class TransportSocketFactory { */ virtual TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const PURE; - - /** - * Check whether matched transport socket which required to use secret information is available. - */ - virtual bool isReady() const PURE; }; using TransportSocketFactoryPtr = std::unique_ptr; diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 91bdddf57a366..675bddde27def 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -111,11 +111,6 @@ class ClientContextConfig : public virtual ContextConfig { * for names. */ virtual const std::string& signingAlgorithmsForTest() const PURE; - - /** - * Check whether TLS certificate entity and certificate validation context entity is available - */ - virtual bool isSecretReady() const PURE; }; using ClientContextConfigPtr = std::unique_ptr; diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index 69f9d70953135..c539add2f71ad 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -93,7 +93,5 @@ RawBufferSocketFactory::createTransportSocket(TransportSocketOptionsSharedPtr) c } bool RawBufferSocketFactory::implementsSecureTransport() const { return false; } - -bool RawBufferSocketFactory::isReady() const { return true; } } // namespace Network } // namespace Envoy diff --git a/source/common/network/raw_buffer_socket.h b/source/common/network/raw_buffer_socket.h index 9fb37b31613df..fe87bbeda6055 100644 --- a/source/common/network/raw_buffer_socket.h +++ b/source/common/network/raw_buffer_socket.h @@ -32,7 +32,6 @@ class RawBufferSocketFactory : public TransportSocketFactory { // Network::TransportSocketFactory TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - bool isReady() const override; }; } // namespace Network diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index b8a495f0a8276..56a7eb8189cbb 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -111,8 +111,6 @@ constexpr const char* disabled_runtime_features[] = { "envoy.reloadable_features.new_tcp_connection_pool", // Sentinel and test flag. "envoy.reloadable_features.test_feature_false", - // The cluster which can't extract secret entity by SDS to be warming and never activate. - "envoy.reloadable_features.cluster_keep_warming_no_secret_entity", }; RuntimeFeatures::RuntimeFeatures() { diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 924c0a6feb07b..15df928be990e 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -417,31 +417,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // been setup for cross-thread updates to avoid needless updates during initialization. The order // of operations here is important. We start by initializing the thread aware load balancer if // needed. This must happen first so cluster updates are heard first by the load balancer. - // Also, it assures that all of clusters which this function is called should be always active. - auto cluster_data = warming_clusters_.find(cluster.info()->name()); - // We have a situation that clusters will be immediately active, such as static and primary - // cluster. So we must have this prevention logic here. - if (cluster_data != warming_clusters_.end()) { - Network::TransportSocketFactory& factory = - cluster.info()->transportSocketMatcher().resolve(&cluster.info()->metadata()).factory_; - // If there is no secret entity, currently supports only TLS Certificate and Validation - // Context, when it failed to extract them via SDS, it will fail to change cluster status from - // warming to active. In current implementation, there is no strategy to activate clusters - // which failed to initialize at once. - // TODO(shikugawa): To implement to be available by keeping warming after no-available secret - // entity behavior occurred. And remove - // `envoy.reloadable_features.cluster_keep_warming_no_secret_entity` runtime feature flag. - const bool keep_warming_enabled = Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.cluster_keep_warming_no_secret_entity"); - if (!factory.isReady() && keep_warming_enabled) { - ENVOY_LOG(warn, "Failed to activate {}", cluster.info()->name()); - return; - } - clusterWarmingToActive(cluster.info()->name()); - updateClusterCounts(); - } - cluster_data = active_clusters_.find(cluster.info()->name()); - + auto cluster_data = active_clusters_.find(cluster.info()->name()); if (cluster_data->second->thread_aware_lb_ != nullptr) { cluster_data->second->thread_aware_lb_->initialize(); } @@ -611,6 +587,17 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl // The following init manager remove call is a NOP in the case we are already initialized. // It's just kept here to avoid additional logic. init_helper_.removeCluster(*existing_active_cluster->second->cluster_); + } else { + // Validate that warming clusters are not added to the init_helper_. + // NOTE: This loop is compiled out in optimized builds. + for (const std::list& cluster_list : + {std::cref(init_helper_.primary_init_clusters_), + std::cref(init_helper_.secondary_init_clusters_)}) { + ASSERT(!std::any_of(cluster_list.begin(), cluster_list.end(), + [&existing_warming_cluster](Cluster* cluster) { + return existing_warming_cluster->second->cluster_.get() == cluster; + })); + } } cm_stats_.cluster_modified_.inc(); } else { @@ -627,39 +614,40 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl // the future we may decide to undergo a refactor to unify the logic but the effort/risk to // do that right now does not seem worth it given that the logic is generally pretty clean // and easy to understand. - const bool all_clusters_initialized = - init_helper_.state() == ClusterManagerInitHelper::State::AllClustersInitialized; - loadCluster(cluster, version_info, true, warming_clusters_); - auto& cluster_entry = warming_clusters_.at(cluster_name); - if (!all_clusters_initialized) { + const bool use_active_map = + init_helper_.state() != ClusterManagerInitHelper::State::AllClustersInitialized; + loadCluster(cluster, version_info, true, use_active_map ? active_clusters_ : warming_clusters_); + + if (use_active_map) { ENVOY_LOG(debug, "add/update cluster {} during init", cluster_name); + auto& cluster_entry = active_clusters_.at(cluster_name); createOrUpdateThreadLocalCluster(*cluster_entry); init_helper_.addCluster(*cluster_entry->cluster_); } else { + auto& cluster_entry = warming_clusters_.at(cluster_name); ENVOY_LOG(debug, "add/update cluster {} starting warming", cluster_name); cluster_entry->cluster_->initialize([this, cluster_name] { + auto warming_it = warming_clusters_.find(cluster_name); + auto& cluster_entry = *warming_it->second; + + // If the cluster is being updated, we need to cancel any pending merged updates. + // Otherwise, applyUpdates() will fire with a dangling cluster reference. + updates_map_.erase(cluster_name); + + active_clusters_[cluster_name] = std::move(warming_it->second); + warming_clusters_.erase(warming_it); + ENVOY_LOG(debug, "warming cluster {} complete", cluster_name); - auto state_changed_cluster_entry = warming_clusters_.find(cluster_name); - createOrUpdateThreadLocalCluster(*state_changed_cluster_entry->second); - onClusterInit(*state_changed_cluster_entry->second->cluster_); + createOrUpdateThreadLocalCluster(cluster_entry); + onClusterInit(*cluster_entry.cluster_); + updateClusterCounts(); }); } + updateClusterCounts(); return true; } -void ClusterManagerImpl::clusterWarmingToActive(const std::string& cluster_name) { - auto warming_it = warming_clusters_.find(cluster_name); - ASSERT(warming_it != warming_clusters_.end()); - - // If the cluster is being updated, we need to cancel any pending merged updates. - // Otherwise, applyUpdates() will fire with a dangling cluster reference. - updates_map_.erase(cluster_name); - - active_clusters_[cluster_name] = std::move(warming_it->second); - warming_clusters_.erase(warming_it); -} - void ClusterManagerImpl::createOrUpdateThreadLocalCluster(ClusterData& cluster) { tls_->runOnAllThreads([new_cluster = cluster.cluster_->info(), thread_aware_lb_factory = cluster.loadBalancerFactory()]( @@ -714,7 +702,6 @@ bool ClusterManagerImpl::removeCluster(const std::string& cluster_name) { if (existing_warming_cluster != warming_clusters_.end() && existing_warming_cluster->second->added_via_api_) { removed = true; - init_helper_.removeCluster(*existing_warming_cluster->second->cluster_); warming_clusters_.erase(existing_warming_cluster); ENVOY_LOG(info, "removing warming cluster {}", cluster_name); } @@ -817,9 +804,7 @@ void ClusterManagerImpl::updateClusterCounts() { // Once cluster is warmed up, CDS is resumed, and ACK is sent to ADS, providing a // signal to ADS to proceed with RDS updates. // If we're in the middle of shutting down (ads_mux_ already gone) then this is irrelevant. - const bool all_clusters_initialized = - init_helper_.state() == ClusterManagerInitHelper::State::AllClustersInitialized; - if (all_clusters_initialized && ads_mux_) { + if (ads_mux_) { const auto type_urls = Config::getAllVersionTypeUrls(); const uint64_t previous_warming = cm_stats_.warming_clusters_.value(); if (previous_warming == 0 && !warming_clusters_.empty()) { diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 1aa14c4be78cf..920681bff0ef5 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -482,7 +482,6 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable prefetch_pool); 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 b974a36725d18..2ada9e2de17b4 100644 --- a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h +++ b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h @@ -24,7 +24,6 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { NOT_REACHED_GCOVR_EXCL_LINE; } bool implementsSecureTransport() const override { return true; } - bool isReady() const override { return true; } }; // TODO(danzh): when implement ProofSource, examine of it's necessary to diff --git a/source/extensions/transport_sockets/alts/tsi_socket.cc b/source/extensions/transport_sockets/alts/tsi_socket.cc index 7ba6b2cab798f..0fe5b752ceca3 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.cc +++ b/source/extensions/transport_sockets/alts/tsi_socket.cc @@ -261,7 +261,6 @@ TsiSocketFactory::createTransportSocket(Network::TransportSocketOptionsSharedPtr return std::make_unique(handshaker_factory_, handshake_validator_); } -bool TsiSocketFactory::isReady() const { return true; } } // namespace Alts } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/alts/tsi_socket.h b/source/extensions/transport_sockets/alts/tsi_socket.h index 529e316c95ffd..0acba405022dc 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.h +++ b/source/extensions/transport_sockets/alts/tsi_socket.h @@ -100,7 +100,6 @@ class TsiSocketFactory : public Network::TransportSocketFactory { bool implementsSecureTransport() const override; Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; - bool isReady() const override; private: HandshakerFactory handshaker_factory_; diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc index ac2716c96d728..3d4f716421e72 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc @@ -123,11 +123,7 @@ bool UpstreamProxyProtocolSocketFactory::implementsSecureTransport() const { return transport_socket_factory_->implementsSecureTransport(); } -bool UpstreamProxyProtocolSocketFactory::isReady() const { - return transport_socket_factory_->isReady(); -} - } // namespace ProxyProtocol } // namespace TransportSockets } // namespace Extensions -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h index cc7fcee7e79a3..4a191ebf539d7 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h @@ -49,7 +49,6 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - bool isReady() const override; private: Network::TransportSocketFactoryPtr transport_socket_factory_; diff --git a/source/extensions/transport_sockets/tap/tap.cc b/source/extensions/transport_sockets/tap/tap.cc index 5b8d83fc8186a..7674ba6b584d4 100644 --- a/source/extensions/transport_sockets/tap/tap.cc +++ b/source/extensions/transport_sockets/tap/tap.cc @@ -66,8 +66,6 @@ bool TapSocketFactory::implementsSecureTransport() const { return transport_socket_factory_->implementsSecureTransport(); } -bool TapSocketFactory::isReady() const { return transport_socket_factory_->isReady(); } - } // namespace Tap } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/tap/tap.h b/source/extensions/transport_sockets/tap/tap.h index c2d91e1f571a8..33156b705153d 100644 --- a/source/extensions/transport_sockets/tap/tap.h +++ b/source/extensions/transport_sockets/tap/tap.h @@ -41,7 +41,6 @@ class TapSocketFactory : public Network::TransportSocketFactory, Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - bool isReady() const override; private: Network::TransportSocketFactoryPtr transport_socket_factory_; diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc index 7e6a59d859193..546489232e742 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -169,14 +169,14 @@ ContextConfigImpl::ContextConfigImpl( const std::string& default_cipher_suites, const std::string& default_curves, Server::Configuration::TransportSocketFactoryContext& factory_context) : api_(factory_context.api()), - tls_certificate_providers_(getTlsCertificateConfigProviders(config, factory_context)), - certificate_validation_context_provider_( - getCertificateValidationContextConfigProvider(config, factory_context, &default_cvc_)), alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), cipher_suites_(StringUtil::nonEmptyStringOrDefault( RepeatedPtrUtil::join(config.tls_params().cipher_suites(), ":"), default_cipher_suites)), ecdh_curves_(StringUtil::nonEmptyStringOrDefault( RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), default_curves)), + tls_certificate_providers_(getTlsCertificateConfigProviders(config, factory_context)), + certificate_validation_context_provider_( + getCertificateValidationContextConfigProvider(config, factory_context, &default_cvc_)), min_protocol_version_(tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(), default_min_protocol_version)), max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), @@ -375,15 +375,6 @@ ClientContextConfigImpl::ClientContextConfigImpl( } } -bool ClientContextConfigImpl::isSecretReady() const { - for (const auto& provider : tls_certificate_providers_) { - if (provider->secret() == nullptr) { - return false; - } - } - return certificate_validation_context_provider_->secret() != nullptr; -} - const unsigned ServerContextConfigImpl::DEFAULT_MIN_VERSION = TLS1_VERSION; const unsigned ServerContextConfigImpl::DEFAULT_MAX_VERSION = TLS1_3_VERSION; diff --git a/source/extensions/transport_sockets/tls/context_config_impl.h b/source/extensions/transport_sockets/tls/context_config_impl.h index d40bb627a51de..44c5a8cc619d0 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.h +++ b/source/extensions/transport_sockets/tls/context_config_impl.h @@ -69,14 +69,6 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string& default_cipher_suites, const std::string& default_curves, Server::Configuration::TransportSocketFactoryContext& factory_context); Api::Api& api_; - // If certificate validation context type is combined_validation_context. default_cvc_ - // holds a copy of CombinedCertificateValidationContext::default_validation_context. - // Otherwise, default_cvc_ is nullptr. - std::unique_ptr - default_cvc_; - std::vector tls_certificate_providers_; - Secret::CertificateValidationContextConfigProviderSharedPtr - certificate_validation_context_provider_; private: static unsigned tlsVersionFromProto( @@ -89,8 +81,16 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { std::vector tls_certificate_configs_; Ssl::CertificateValidationContextConfigPtr validation_context_config_; + // If certificate validation context type is combined_validation_context. default_cvc_ + // holds a copy of CombinedCertificateValidationContext::default_validation_context. + // Otherwise, default_cvc_ is nullptr. + std::unique_ptr + default_cvc_; + std::vector tls_certificate_providers_; // Handle for TLS certificate dynamic secret callback. Envoy::Common::CallbackHandle* tc_update_callback_handle_{}; + Secret::CertificateValidationContextConfigProviderSharedPtr + certificate_validation_context_provider_; // Handle for certificate validation context dynamic secret callback. Envoy::Common::CallbackHandle* cvc_update_callback_handle_{}; Envoy::Common::CallbackHandle* cvc_validation_callback_handle_{}; @@ -120,7 +120,6 @@ class ClientContextConfigImpl : public ContextConfigImpl, public Envoy::Ssl::Cli bool allowRenegotiation() const override { return allow_renegotiation_; } size_t maxSessionKeys() const override { return max_session_keys_; } const std::string& signingAlgorithmsForTest() const override { return sigalgs_; } - bool isSecretReady() const override; private: static const unsigned DEFAULT_MIN_VERSION; diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 0bc6dcc400c89..4854684430963 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -362,8 +362,6 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() { stats_.ssl_context_update_by_sds_.inc(); } -bool ClientSslSocketFactory::isReady() const { return config_->isSecretReady(); } - ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope, @@ -405,8 +403,6 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() { stats_.ssl_context_update_by_sds_.inc(); } -bool ServerSslSocketFactory::isReady() const { return true; } - } // 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 82834b133d602..c14cb502bed15 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -105,11 +105,10 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope); - // Network::TransportSocketFactory Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - bool isReady() const override; + // Secret::SecretCallbacks void onAddOrUpdateSecret() override; @@ -133,7 +132,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; - bool isReady() const override; + // Secret::SecretCallbacks void onAddOrUpdateSecret() override; diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 0c9ca2ed6a17b..b6b19139a7b55 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -46,12 +46,10 @@ envoy_cc_test( "//test/mocks/upstream:health_checker_mocks", "//test/mocks/upstream:load_balancer_context_mock", "//test/mocks/upstream:thread_aware_load_balancer_mocks", - "//test/test_common:test_runtime_lib", "@envoy_api//envoy/admin/v3:pkg_cc_proto", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", - "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", ], ) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 34d8403cf5d0b..b24c45330de52 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3,7 +3,6 @@ #include "envoy/config/cluster/v3/cluster.pb.h" #include "envoy/config/cluster/v3/cluster.pb.validate.h" #include "envoy/config/core/v3/base.pb.h" -#include "envoy/extensions/transport_sockets/tls/v3/secret.pb.h" #include "test/common/upstream/test_cluster_manager.h" #include "test/mocks/upstream/cds_api.h" @@ -13,7 +12,6 @@ #include "test/mocks/upstream/health_checker.h" #include "test/mocks/upstream/load_balancer_context.h" #include "test/mocks/upstream/thread_aware_load_balancer.h" -#include "test/test_common/test_runtime.h" namespace Envoy { namespace Upstream { @@ -1057,7 +1055,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { last_updated: seconds: 1234567891 nanos: 234000000 - dynamic_warming_clusters: + dynamic_active_clusters: - version_info: "version1" cluster: "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster @@ -1109,7 +1107,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { last_updated: seconds: 1234567891 nanos: 234000000 - dynamic_active_clusters: + dynamic_warming_clusters: )EOF"); EXPECT_CALL(*cluster3, initialize(_)); @@ -2308,154 +2306,6 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { factory_.tls_.shutdownThread(); } -TEST_F(ClusterManagerImplTest, - DynamicAddedAndKeepWarmingWithoutCertificateValidationContextEntity) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.cluster_keep_warming_no_secret_entity", "true"}}); - create(defaultConfig()); - - ReadyWatcher initialized; - EXPECT_CALL(initialized, ready()); - cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); - - std::shared_ptr cluster1(new NiceMock()); - cluster1->info_->name_ = "fake_cluster"; - - auto transport_socket_factory = std::make_unique(); - EXPECT_CALL(*transport_socket_factory, isReady()).WillOnce(Return(false)); - - auto transport_socket_matcher = std::make_unique>( - std::move(transport_socket_factory)); - cluster1->info_->transport_socket_matcher_ = std::move(transport_socket_matcher); - - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) - .WillOnce(Return(std::make_pair(cluster1, nullptr))); - EXPECT_CALL(*cluster1, initializePhase()).Times(0); - EXPECT_CALL(*cluster1, initialize(_)); - EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "")); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); - EXPECT_EQ(1, cluster_manager_->warmingClusterCount()); - EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); - cluster1->initialize_callback_(); - - // Check to be keep warming fake_cluster after callback invoked. - EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); - EXPECT_EQ(1, cluster_manager_->warmingClusterCount()); - - EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); -} - -TEST_F(ClusterManagerImplTest, - DynamicAddedAndKeepWarmingDisabledWithoutCertificateValidationContextEntity) { - create(defaultConfig()); - - ReadyWatcher initialized; - EXPECT_CALL(initialized, ready()); - cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); - - std::shared_ptr cluster1(new NiceMock()); - cluster1->info_->name_ = "fake_cluster"; - - auto transport_socket_factory = std::make_unique(); - EXPECT_CALL(*transport_socket_factory, isReady()).WillOnce(Return(false)); - - auto transport_socket_matcher = std::make_unique>( - std::move(transport_socket_factory)); - cluster1->info_->transport_socket_matcher_ = std::move(transport_socket_matcher); - - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) - .WillOnce(Return(std::make_pair(cluster1, nullptr))); - EXPECT_CALL(*cluster1, initializePhase()).Times(0); - EXPECT_CALL(*cluster1, initialize(_)); - EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "")); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); - EXPECT_EQ(1, cluster_manager_->warmingClusterCount()); - EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); - cluster1->initialize_callback_(); - - // Check to be keep warming fake_cluster after callback invoked. - EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); - EXPECT_EQ(0, cluster_manager_->warmingClusterCount()); - - EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); -} - -TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutTlsCertificateEntity) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.cluster_keep_warming_no_secret_entity", "true"}}); - create(defaultConfig()); - - ReadyWatcher initialized; - EXPECT_CALL(initialized, ready()); - cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); - - std::shared_ptr cluster1(new NiceMock()); - cluster1->info_->name_ = "fake_cluster"; - - auto transport_socket_factory = std::make_unique(); - EXPECT_CALL(*transport_socket_factory, isReady()).WillOnce(Return(false)); - - auto transport_socket_matcher = std::make_unique>( - std::move(transport_socket_factory)); - cluster1->info_->transport_socket_matcher_ = std::move(transport_socket_matcher); - - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) - .WillOnce(Return(std::make_pair(cluster1, nullptr))); - EXPECT_CALL(*cluster1, initializePhase()).Times(0); - EXPECT_CALL(*cluster1, initialize(_)); - EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "")); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); - EXPECT_EQ(1, cluster_manager_->warmingClusterCount()); - EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); - cluster1->initialize_callback_(); - - // Check to be keep warming fake_cluster after callback invoked. - EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); - EXPECT_EQ(1, cluster_manager_->warmingClusterCount()); - - EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); -} - -TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingDisabledWithoutTlsCertificateEntity) { - create(defaultConfig()); - - ReadyWatcher initialized; - EXPECT_CALL(initialized, ready()); - cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); - - std::shared_ptr cluster1(new NiceMock()); - cluster1->info_->name_ = "fake_cluster"; - - auto transport_socket_factory = std::make_unique(); - EXPECT_CALL(*transport_socket_factory, isReady()).WillOnce(Return(false)); - - auto transport_socket_matcher = std::make_unique>( - std::move(transport_socket_factory)); - cluster1->info_->transport_socket_matcher_ = std::move(transport_socket_matcher); - - EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) - .WillOnce(Return(std::make_pair(cluster1, nullptr))); - EXPECT_CALL(*cluster1, initializePhase()).Times(0); - EXPECT_CALL(*cluster1, initialize(_)); - EXPECT_TRUE(cluster_manager_->addOrUpdateCluster(defaultStaticCluster("fake_cluster"), "")); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 0 /*active*/, 1 /*warming*/); - EXPECT_EQ(1, cluster_manager_->warmingClusterCount()); - EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); - cluster1->initialize_callback_(); - - // Check to be keep warming fake_cluster after callback invoked. - EXPECT_EQ(cluster1->info_, cluster_manager_->get("fake_cluster")->info()); - checkStats(1 /*added*/, 0 /*modified*/, 0 /*removed*/, 1 /*active*/, 0 /*warming*/); - EXPECT_EQ(0, cluster_manager_->warmingClusterCount()); - - EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); -} - class MockConnPoolWithDestroy : public Http::ConnectionPool::MockInstance { public: ~MockConnPoolWithDestroy() override { onDestroy(); } diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index 61e5ab2cec433..cfde130d1d1f8 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -33,7 +33,6 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(bool, isReady, (), (const)); FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} std::string id() const { return id_; } @@ -49,7 +48,6 @@ class FooTransportSocketFactory MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(bool, isReady, (), (const)); Network::TransportSocketFactoryPtr createTransportSocketFactory(const Protobuf::Message& proto, diff --git a/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc b/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc index 32a2281cf0af9..953e5999a5fbd 100644 --- a/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc @@ -469,13 +469,6 @@ TEST_F(ProxyProtocolSocketFactoryTest, ImplementsSecureTransportCallInnerFactory ASSERT_TRUE(factory_->implementsSecureTransport()); } -// Test isReady calls inner factory -TEST_F(ProxyProtocolSocketFactoryTest, IsReadyCallInnerFactory) { - initialize(); - EXPECT_CALL(*inner_factory_, isReady()).WillOnce(Return(true)); - ASSERT_TRUE(factory_->isReady()); -} - } // namespace } // namespace ProxyProtocol } // namespace TransportSockets diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 3b361d88beace..0307ebb2daef9 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1604,62 +1604,6 @@ TEST_F(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { "Unknown static certificate validation context: missing"); } -TEST_F(ClientContextConfigImplTest, ValidationContextEntityNotExist) { - envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; - auto* validation_context_sds_secret_config = - tls_context.mutable_common_tls_context()->mutable_validation_context_sds_secret_config(); - validation_context_sds_secret_config->set_name("sds_validation_context"); - auto* config_source = validation_context_sds_secret_config->mutable_sds_config(); - auto* api_config_source = config_source->mutable_api_config_source(); - api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); - - NiceMock local_info; - Stats::IsolatedStoreImpl stats; - NiceMock init_manager; - NiceMock dispatcher; - EXPECT_CALL(factory_context_, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context_, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(factory_context_, initManager()).WillRepeatedly(ReturnRef(init_manager)); - EXPECT_CALL(factory_context_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); - - ClientContextConfigImpl client_context_config(tls_context, factory_context_); - EXPECT_FALSE(client_context_config.isSecretReady()); - - NiceMock secret_callback; - client_context_config.setSecretUpdateCallback( - [&secret_callback]() { secret_callback.onAddOrUpdateSecret(); }); - client_context_config.setSecretUpdateCallback([]() {}); -} - -TEST_F(ClientContextConfigImplTest, TlsCertificateEntityNotExist) { - envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig secret_config; - secret_config.set_name("sds_tls_certificate"); - auto* config_source = secret_config.mutable_sds_config(); - auto* api_config_source = config_source->mutable_api_config_source(); - api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); - envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; - auto tls_certificate_sds_secret_configs = - tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs(); - *tls_certificate_sds_secret_configs->Add() = secret_config; - - NiceMock local_info; - Stats::IsolatedStoreImpl stats; - NiceMock init_manager; - NiceMock dispatcher; - EXPECT_CALL(factory_context_, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(factory_context_, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(factory_context_, initManager()).WillRepeatedly(ReturnRef(init_manager)); - EXPECT_CALL(factory_context_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); - - ClientContextConfigImpl client_context_config(tls_context, factory_context_); - EXPECT_FALSE(client_context_config.isSecretReady()); - - NiceMock secret_callback; - client_context_config.setSecretUpdateCallback( - [&secret_callback]() { secret_callback.onAddOrUpdateSecret(); }); - client_context_config.setSecretUpdateCallback([]() {}); -} - class ServerContextConfigImplTest : public SslCertsTest {}; // Multiple TLS certificates are supported. diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index c0874d16019f1..b4bdb84e57370 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -4524,7 +4524,6 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { EXPECT_FALSE(client_cfg->isReady()); ContextManagerImpl manager(time_system_); - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); auto transport_socket = client_ssl_socket_factory.createTransportSocket(nullptr); EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); @@ -5730,15 +5729,6 @@ TEST_P(SslSocketTest, TestConnectionFailsOnMultipleCertificatesNonePassOcspPolic testUtil(test_options.setExpectedServerStats("ssl.ocsp_staple_failed").enableOcspStapling()); } -TEST_P(SslSocketTest, ClientSocketFactoryIsReadyTest) { - ContextManagerImpl manager(time_system_); - Stats::TestUtil::TestStore stats_store; - auto client_cfg = std::make_unique>(); - EXPECT_CALL(*client_cfg, isSecretReady()).WillOnce(Return(true)); - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); - EXPECT_TRUE(client_ssl_socket_factory.isReady()); -} - } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 01aae9dc9f733..b49b217464bfd 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -1132,26 +1132,6 @@ class AdsClusterV3Test : public AdsIntegrationTest { INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, AdsClusterV3Test, DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); -TEST_P(AdsClusterV3Test, BasicClusterInitialWarming) { - initialize(); - const auto cds_type_url = Config::getTypeUrl( - envoy::config::core::v3::ApiVersion::V3); - const auto eds_type_url = Config::getTypeUrl( - envoy::config::core::v3::ApiVersion::V3); - - EXPECT_TRUE(compareDiscoveryRequest(cds_type_url, "", {}, {}, {}, true)); - sendDiscoveryResponse( - cds_type_url, {buildCluster("cluster_0")}, {buildCluster("cluster_0")}, {}, "1", false); - test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 1); - EXPECT_TRUE(compareDiscoveryRequest(eds_type_url, "", {"cluster_0"}, {"cluster_0"}, {})); - sendDiscoveryResponse( - eds_type_url, {buildClusterLoadAssignment("cluster_0")}, - {buildClusterLoadAssignment("cluster_0")}, {}, "1", false); - - test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 0); - test_server_->waitForGaugeGe("cluster_manager.active_clusters", 2); -} - // Verify CDS is paused during cluster warming. TEST_P(AdsClusterV3Test, CdsPausedDuringWarming) { initialize(); diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index 7fa5dee966964..3b299de13c513 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -46,7 +46,7 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b validation_context: trusted_ca: filename: {{ test_rundir }}/test/config/integration/certs/cacert.pem - match_subject_alt_names: + match_subject_alt_names: exact: "spiffe://lyft.com/backend-team" exact: "lyft.com" exact: "www.lyft.com" @@ -57,7 +57,7 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b validation_context: trusted_ca: filename: {{ test_rundir }}/test/config/integration/certs/cacert.pem - match_subject_alt_names: + match_subject_alt_names: exact: "spiffe://lyft.com/backend-team" exact: "lyft.com" exact: "www.lyft.com" diff --git a/test/mocks/network/transport_socket.h b/test/mocks/network/transport_socket.h index 164e393c23ff9..ee53570c20ace 100644 --- a/test/mocks/network/transport_socket.h +++ b/test/mocks/network/transport_socket.h @@ -38,7 +38,6 @@ class MockTransportSocketFactory : public TransportSocketFactory { MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(TransportSocketPtr, createTransportSocket, (TransportSocketOptionsSharedPtr), (const)); - MOCK_METHOD(bool, isReady, (), (const)); }; } // namespace Network diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 31da44badabd8..6a5cbe8df6490 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -95,7 +95,6 @@ class MockClientContextConfig : public ClientContextConfig { MOCK_METHOD(bool, allowRenegotiation, (), (const)); MOCK_METHOD(size_t, maxSessionKeys, (), (const)); MOCK_METHOD(const std::string&, signingAlgorithmsForTest, (), (const)); - MOCK_METHOD(bool, isSecretReady, (), (const)); }; class MockServerContextConfig : public ServerContextConfig {