From b882bde4d94f8b271833e12d1078912d8a358c6b Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Sat, 22 Aug 2020 23:30:07 +0900 Subject: [PATCH 01/22] cluster manager: avoid immediate activation for dynamic inserted cluster when initialize Signed-off-by: Shikugawa --- .../common/upstream/cluster_manager_impl.cc | 46 +++++++++++-------- source/common/upstream/cluster_manager_impl.h | 1 + .../upstream/cluster_manager_impl_test.cc | 4 +- 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index f9c8c40977672..fc65d9160942e 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -417,6 +417,11 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // 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. auto cluster_data = active_clusters_.find(cluster.info()->name()); + if (cluster_data == active_clusters_.end()) { + clusterWarmingToActive(cluster.info()->name()); + } + cluster_data = active_clusters_.find(cluster.info()->name()); + if (cluster_data->second->thread_aware_lb_ != nullptr) { cluster_data->second->thread_aware_lb_->initialize(); } @@ -613,32 +618,22 @@ 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 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) { + 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) { 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); - createOrUpdateThreadLocalCluster(cluster_entry); - onClusterInit(*cluster_entry.cluster_); + clusterWarmingToActive(cluster_name); + auto state_changed_cluster_entry = active_clusters_.find(cluster_name); + createOrUpdateThreadLocalCluster(*state_changed_cluster_entry->second); + onClusterInit(*state_changed_cluster_entry->second->cluster_); updateClusterCounts(); }); } @@ -647,6 +642,18 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl 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([this, new_cluster = cluster.cluster_->info(), thread_aware_lb_factory = cluster.loadBalancerFactory()]() -> void { @@ -695,6 +702,7 @@ 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); } diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index c229395c1353a..7a76acee5396c 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -483,6 +483,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable Date: Sun, 13 Sep 2020 19:09:05 +0900 Subject: [PATCH 02/22] update count Signed-off-by: Shikugawa --- source/common/upstream/cluster_manager_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index fc65d9160942e..b1aef80df7a60 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -419,6 +419,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { auto cluster_data = active_clusters_.find(cluster.info()->name()); if (cluster_data == active_clusters_.end()) { clusterWarmingToActive(cluster.info()->name()); + updateClusterCounts(); } cluster_data = active_clusters_.find(cluster.info()->name()); @@ -638,7 +639,6 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl }); } - updateClusterCounts(); return true; } @@ -805,7 +805,9 @@ 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. - if (ads_mux_) { + const bool all_clusters_initialized = + init_helper_.state() == ClusterManagerInitHelper::State::AllClustersInitialized; + if (all_clusters_initialized && 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()) { From 8e99e814d0c5a0f423b9c1c2df311e3e8edd48ec Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Wed, 30 Sep 2020 01:54:49 +0000 Subject: [PATCH 03/22] cleanup Signed-off-by: Shikugawa --- source/common/upstream/cluster_manager_impl.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index b1aef80df7a60..2f6a597f984b6 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -416,8 +416,8 @@ 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. - auto cluster_data = active_clusters_.find(cluster.info()->name()); - if (cluster_data == active_clusters_.end()) { + auto cluster_data = warming_clusters_.find(cluster.info()->name()); + if (cluster_data != warming_clusters_.end()) { clusterWarmingToActive(cluster.info()->name()); updateClusterCounts(); } @@ -631,11 +631,9 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl ENVOY_LOG(debug, "add/update cluster {} starting warming", cluster_name); cluster_entry->cluster_->initialize([this, cluster_name] { ENVOY_LOG(debug, "warming cluster {} complete", cluster_name); - clusterWarmingToActive(cluster_name); - auto state_changed_cluster_entry = active_clusters_.find(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_); - updateClusterCounts(); }); } From 478f578669496b5db93eb3d70736bd35c9cb03ac Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 1 Oct 2020 03:15:02 +0000 Subject: [PATCH 04/22] sds: keep warming when dynamic inserted cluster can't be extracted secret entity Signed-off-by: Shikugawa --- include/envoy/secret/secret_manager.h | 20 +++++++++ include/envoy/upstream/upstream.h | 6 +++ source/common/secret/secret_manager_impl.cc | 10 +++++ source/common/secret/secret_manager_impl.h | 18 ++++++++ source/common/upstream/BUILD | 1 + .../common/upstream/cluster_manager_impl.cc | 42 ++++++++++++++++--- source/common/upstream/cluster_manager_impl.h | 5 ++- source/common/upstream/upstream_impl.cc | 1 + source/common/upstream/upstream_impl.h | 4 ++ .../config_validation/cluster_manager.cc | 8 ++-- .../config_validation/cluster_manager.h | 2 +- .../upstream/cluster_manager_impl_test.cc | 7 +++- test/common/upstream/test_cluster_manager.h | 10 +++-- test/mocks/secret/mocks.h | 8 ++++ test/mocks/upstream/cluster_info.cc | 1 + test/mocks/upstream/cluster_info.h | 2 + 16 files changed, 129 insertions(+), 16 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index ce13f6eba2145..dd7e2f6d1d215 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -109,6 +109,16 @@ class SecretManager { const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) PURE; + /** + * Check whether TLS Certificate entity was being extracted from SDS server. + * + * @param config_source a protobuf message object containing a SDS config source. + * @param config_name a name that uniquely refers to the SDS config source. + */ + virtual bool + checkTlsCertificateEntityExists(const envoy::config::core::v3::ConfigSource& config_source, + const std::string& config_name) PURE; + /** * Finds and returns a dynamic secret provider associated to SDS config. Create * a new one if such provider does not exist. @@ -125,6 +135,16 @@ class SecretManager { const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) PURE; + /** + * Check whether certificate validation context entity was being extracted from SDS server. + * + * @param config_source a protobuf message object containing a SDS config source. + * @param config_name a name that uniquely refers to the SDS config source. + */ + virtual bool checkCertificateValidationContextEntityExists( + const envoy::config::core::v3::ConfigSource& config_source, + const std::string& config_name) PURE; + /** * Finds and returns a dynamic secret provider associated to SDS config. Create * a new one if such provider does not exist. diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 9330b0e8fdb5a..87345338c785d 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -827,6 +827,12 @@ class ClusterInfo { virtual const absl::optional& upstreamConfig() const PURE; + /** + * @return const envoy::config::core::v3::TransportSocket& the transport socket config for + * upstream connection. + */ + virtual const envoy::config::core::v3::TransportSocket& transportSocket() const PURE; + /** * @return Whether the cluster is currently in maintenance mode and should not be routed to. * Different filters may handle this situation in different ways. The implementation diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index f99a1205caa0f..934f056a99b15 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -128,6 +128,11 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertific secret_provider_context); } +bool SecretManagerImpl::checkTlsCertificateEntityExists( + const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name) { + return certificate_providers_.checkSecretEntityExists(config_source, config_name); +} + CertificateValidationContextConfigProviderSharedPtr SecretManagerImpl::findOrCreateCertificateValidationContextProvider( const envoy::config::core::v3::ConfigSource& sds_config_source, const std::string& config_name, @@ -136,6 +141,11 @@ SecretManagerImpl::findOrCreateCertificateValidationContextProvider( secret_provider_context); } +bool SecretManagerImpl::checkCertificateValidationContextEntityExists( + const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name) { + return validation_context_providers_.checkSecretEntityExists(config_source, config_name); +} + TlsSessionTicketKeysConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsSessionTicketKeysContextProvider( const envoy::config::core::v3::ConfigSource& sds_config_source, const std::string& config_name, diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 799c7415d7ce0..f5b7c6d88cc45 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -54,11 +54,18 @@ class SecretManagerImpl : public SecretManager { const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override; + bool checkTlsCertificateEntityExists(const envoy::config::core::v3::ConfigSource& config_source, + const std::string& config_name) override; + CertificateValidationContextConfigProviderSharedPtr findOrCreateCertificateValidationContextProvider( const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override; + bool checkCertificateValidationContextEntityExists( + const envoy::config::core::v3::ConfigSource& config_source, + const std::string& config_name) override; + TlsSessionTicketKeysConfigProviderSharedPtr findOrCreateTlsSessionTicketKeysContextProvider( const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override; @@ -95,6 +102,17 @@ class SecretManagerImpl : public SecretManager { return secret_provider; } + bool checkSecretEntityExists(const envoy::config::core::v3::ConfigSource& sds_config_source, + const std::string& config_name) { + const std::string map_key = + absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name); + std::shared_ptr secret_provider = dynamic_secret_providers_[map_key].lock(); + if (!secret_provider) { + return false; + } + return secret_provider->secret() != nullptr; + } + std::vector> allSecretProviders() { std::vector> providers; for (const auto& secret_entry : dynamic_secret_providers_) { diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 2d0fb940cf003..2879c25e36f86 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -63,6 +63,7 @@ envoy_cc_library( "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", "//source/common/router:shadow_writer_lib", + "//source/common/secret:secret_manager_impl_lib", "//source/common/shared_pool:shared_pool_lib", "//source/common/tcp:conn_pool_lib", "//source/common/upstream:priority_conn_pool_map_impl_lib", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 2f6a597f984b6..cf5b8d026f7fa 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -16,6 +16,7 @@ #include "envoy/network/dns.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" +#include "envoy/upstream/upstream.h" #include "common/common/assert.h" #include "common/common/enum_to_int.h" @@ -237,7 +238,7 @@ ClusterManagerImpl::ClusterManagerImpl( Random::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, - Http::Context& http_context, Grpc::Context& grpc_context) + Http::Context& http_context, Grpc::Context& grpc_context, Secret::SecretManager& secret_manager) : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()), random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()), local_info_(local_info), cm_stats_(generateStats(stats)), @@ -247,7 +248,8 @@ ClusterManagerImpl::ClusterManagerImpl( time_source_(main_thread_dispatcher.timeSource()), dispatcher_(main_thread_dispatcher), http_context_(http_context), subscription_factory_(local_info, main_thread_dispatcher, *this, random, - validation_context.dynamicValidationVisitor(), api, runtime_) { + validation_context.dynamicValidationVisitor(), api, runtime_), + secret_manager_(secret_manager) { async_client_manager_ = std::make_unique( *this, tls, time_source_, api, grpc_context.statNames()); const auto& cm_config = bootstrap.cluster_manager(); @@ -418,6 +420,35 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // needed. This must happen first so cluster updates are heard first by the load balancer. auto cluster_data = warming_clusters_.find(cluster.info()->name()); if (cluster_data != warming_clusters_.end()) { + if (cluster.info()->transportSocket().has_typed_config()) { + auto upstream_tls_context = MessageUtil::anyConvert< + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext>( + cluster.info()->transportSocket().typed_config()); + const auto& common_tls_context = upstream_tls_context.common_tls_context(); + + for (const auto& sds_secret_config : + common_tls_context.tls_certificate_sds_secret_configs()) { + auto& config_name = sds_secret_config.name(); + auto& config_source = sds_secret_config.sds_config(); + if (!secret_manager_.checkTlsCertificateEntityExists(config_source, config_name)) { + ENVOY_LOG(info, "Failed to activate {} on {}", config_name, cluster.info()->name()); + return; + } + } + + if (common_tls_context.has_validation_context_sds_secret_config()) { + const auto& validation_context_sds_secret_config = + common_tls_context.validation_context_sds_secret_config(); + auto& config_name = validation_context_sds_secret_config.name(); + auto& config_source = validation_context_sds_secret_config.sds_config(); + if (!secret_manager_.checkCertificateValidationContextEntityExists(config_source, + config_name)) { + ENVOY_LOG(info, "Failed to activate {} on {}", config_name, cluster.info()->name()); + return; + } + } + } + clusterWarmingToActive(cluster.info()->name()); updateClusterCounts(); } @@ -1399,9 +1430,10 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPool( ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - return ClusterManagerPtr{new ClusterManagerImpl( - bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_, - main_thread_dispatcher_, admin_, validation_context_, api_, http_context_, grpc_context_)}; + return ClusterManagerPtr{ + new ClusterManagerImpl(bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, + log_manager_, main_thread_dispatcher_, admin_, validation_context_, + api_, http_context_, grpc_context_, secret_manager_)}; } Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 7a76acee5396c..ff21380a9d91f 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -28,6 +28,7 @@ #include "common/config/grpc_mux_impl.h" #include "common/config/subscription_factory_impl.h" #include "common/http/async_client_impl.h" +#include "common/secret/secret_manager_impl.h" #include "common/upstream/load_stats_reporter.h" #include "common/upstream/priority_conn_pool_map.h" #include "common/upstream/upstream_impl.h" @@ -197,7 +198,8 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable extension_protocol_options_; mutable ResourceManagers resource_managers_; const std::string maintenance_mode_runtime_key_; diff --git a/source/server/config_validation/cluster_manager.cc b/source/server/config_validation/cluster_manager.cc index d5f4489c918c5..2a23ed6333fdb 100644 --- a/source/server/config_validation/cluster_manager.cc +++ b/source/server/config_validation/cluster_manager.cc @@ -2,6 +2,7 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/core/v3/config_source.pb.h" +#include "envoy/secret/secret_manager.h" #include "common/common/utility.h" @@ -13,7 +14,7 @@ ClusterManagerPtr ValidationClusterManagerFactory::clusterManagerFromProto( return std::make_unique( bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_, main_thread_dispatcher_, admin_, validation_context_, api_, http_context_, grpc_context_, - time_system_); + time_system_, secret_manager_); } CdsApiPtr @@ -31,10 +32,11 @@ ValidationClusterManager::ValidationClusterManager( Random::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, - Http::Context& http_context, Grpc::Context& grpc_context, Event::TimeSystem& time_system) + Http::Context& http_context, Grpc::Context& grpc_context, Event::TimeSystem& time_system, + Secret::SecretManager& secret_manager) : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context), + grpc_context, secret_manager), async_client_(api, time_system) {} Http::ConnectionPool::Instance* ValidationClusterManager::httpConnPoolForCluster( diff --git a/source/server/config_validation/cluster_manager.h b/source/server/config_validation/cluster_manager.h index 6ce2c46941fee..9dd3293b106d5 100644 --- a/source/server/config_validation/cluster_manager.h +++ b/source/server/config_validation/cluster_manager.h @@ -62,7 +62,7 @@ class ValidationClusterManager : public ClusterManagerImpl { Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, Http::Context& http_context, Grpc::Context& grpc_context, - Event::TimeSystem& time_system); + Event::TimeSystem& time_system, Secret::SecretManager& secret_manager); Http::ConnectionPool::Instance* httpConnPoolForCluster(const std::string&, ResourcePriority, absl::optional, diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 40304fb2065f4..907f3b50c1a37 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3,6 +3,7 @@ #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/secret/secret_manager.h" #include "test/common/upstream/test_cluster_manager.h" @@ -42,7 +43,7 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *api_, http_context_, grpc_context_); + *api_, http_context_, grpc_context_, secret_manager_); cluster_manager_->setPrimaryClustersInitializedCb( [this, bootstrap]() { cluster_manager_->initializeSecondaryClusters(bootstrap); }); } @@ -86,7 +87,8 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_); + *api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_, + secret_manager_); } void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active, @@ -133,6 +135,7 @@ class ClusterManagerImplTest : public testing::Test { std::unique_ptr cluster_manager_; AccessLog::MockAccessLogManager log_manager_; NiceMock admin_; + NiceMock secret_manager_; MockLocalClusterUpdate local_cluster_update_; MockLocalHostsRemoved local_hosts_removed_; Http::ContextImpl http_context_; diff --git a/test/common/upstream/test_cluster_manager.h b/test/common/upstream/test_cluster_manager.h index 20edfaa9b59be..5b2708b3facff 100644 --- a/test/common/upstream/test_cluster_manager.h +++ b/test/common/upstream/test_cluster_manager.h @@ -165,10 +165,11 @@ class TestClusterManagerImpl : public ClusterManagerImpl { AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, - Http::Context& http_context, Grpc::Context& grpc_context) + Http::Context& http_context, Grpc::Context& grpc_context, + Secret::SecretManager& secret_manager) : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context) {} + grpc_context, secret_manager) {} std::map> activeClusters() { std::map> clusters; @@ -190,10 +191,11 @@ class MockedUpdatedClusterManagerImpl : public TestClusterManagerImpl { AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, MockLocalClusterUpdate& local_cluster_update, MockLocalHostsRemoved& local_hosts_removed, - Http::Context& http_context, Grpc::Context& grpc_context) + Http::Context& http_context, Grpc::Context& grpc_context, + Secret::SecretManager& secret_manager) : TestClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, - http_context, grpc_context), + http_context, grpc_context, secret_manager), local_cluster_update_(local_cluster_update), local_hosts_removed_(local_hosts_removed) {} protected: diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index d415fcfd4ff67..acfa15b3a6543 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -43,18 +43,26 @@ class MockSecretManager : public SecretManager { MOCK_METHOD(TlsCertificateConfigProviderSharedPtr, findOrCreateTlsCertificateProvider, (const envoy::config::core::v3::ConfigSource&, const std::string&, Server::Configuration::TransportSocketFactoryContext&)); + MOCK_METHOD(bool, checkTlsCertificateEntityExists, + (const envoy::config::core::v3::ConfigSource&, const std::string&)); MOCK_METHOD(CertificateValidationContextConfigProviderSharedPtr, findOrCreateCertificateValidationContextProvider, (const envoy::config::core::v3::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context)); + MOCK_METHOD(bool, checkCertificateValidationContextEntityExists, + (const envoy::config::core::v3::ConfigSource&, const std::string&)); MOCK_METHOD(TlsSessionTicketKeysConfigProviderSharedPtr, findOrCreateTlsSessionTicketKeysContextProvider, (const envoy::config::core::v3::ConfigSource&, const std::string&, Server::Configuration::TransportSocketFactoryContext&)); + MOCK_METHOD(bool, checkTlsSessionTicketKeysEntityExists, + (const envoy::config::core::v3::ConfigSource&, const std::string&)); MOCK_METHOD(GenericSecretConfigProviderSharedPtr, findOrCreateGenericSecretProvider, (const envoy::config::core::v3::ConfigSource&, const std::string&, Server::Configuration::TransportSocketFactoryContext&)); + MOCK_METHOD(bool, checkGenericSecretEntityExists, + (const envoy::config::core::v3::ConfigSource&, const std::string&)); }; class MockSecretCallbacks : public SecretCallbacks { diff --git a/test/mocks/upstream/cluster_info.cc b/test/mocks/upstream/cluster_info.cc index b6b8a59631cee..948d249a51c7a 100644 --- a/test/mocks/upstream/cluster_info.cc +++ b/test/mocks/upstream/cluster_info.cc @@ -95,6 +95,7 @@ MockClusterInfo::MockClusterInfo() ON_CALL(*this, metadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, upstreamHttpProtocolOptions()) .WillByDefault(ReturnRef(upstream_http_protocol_options_)); + ON_CALL(*this, transportSocket()).WillByDefault(ReturnRef(transport_socket_)); // Delayed construction of typed_metadata_, to allow for injection of metadata ON_CALL(*this, typedMetadata()) .WillByDefault(Invoke([this]() -> const Envoy::Config::TypedMetadata& { diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 3848154f73555..19048a9f9963a 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -96,6 +96,7 @@ class MockClusterInfo : public ClusterInfo { MOCK_METHOD(const envoy::config::core::v3::Http2ProtocolOptions&, http2Options, (), (const)); MOCK_METHOD(const envoy::config::core::v3::HttpProtocolOptions&, commonHttpProtocolOptions, (), (const)); + MOCK_METHOD(const envoy::config::core::v3::TransportSocket&, transportSocket, (), (const)); MOCK_METHOD(ProtocolOptionsConfigConstSharedPtr, extensionProtocolOptions, (const std::string&), (const)); MOCK_METHOD(const envoy::config::cluster::v3::Cluster::CommonLbConfig&, lbConfig, (), (const)); @@ -144,6 +145,7 @@ class MockClusterInfo : public ClusterInfo { Http::Http1Settings http1_settings_; envoy::config::core::v3::Http2ProtocolOptions http2_options_; envoy::config::core::v3::HttpProtocolOptions common_http_protocol_options_; + envoy::config::core::v3::TransportSocket transport_socket_; ProtocolOptionsConfigConstSharedPtr extension_protocol_options_; uint64_t max_requests_per_connection_{}; uint32_t max_response_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; From 8cda02f0ad51ad5b4235240f2deeda71f5971144 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 1 Oct 2020 08:57:10 +0000 Subject: [PATCH 05/22] add unit test Signed-off-by: Shikugawa --- test/common/upstream/BUILD | 1 + .../upstream/cluster_manager_impl_test.cc | 89 +++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index b6b19139a7b55..f7db79b901145 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -50,6 +50,7 @@ envoy_cc_test( "@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 7f9538e0e1fc9..4d0800815a4fa 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3,6 +3,7 @@ #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 "envoy/secret/secret_manager.h" #include "test/common/upstream/test_cluster_manager.h" @@ -2310,6 +2311,94 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { factory_.tls_.shutdownThread(); } +TEST_F(ClusterManagerImplTest, + DynamicAddedAndKeepWarmingWithoutCertificateValidationContextEntity) { + 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); + + envoy::config::core::v3::TransportSocket transport_socket; + transport_socket.set_name("envoy.transport_sockets.tls"); + transport_socket.mutable_typed_config()->PackFrom(tls_context); + + create(defaultConfig()); + + InSequence s; + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::shared_ptr cluster1(new NiceMock()); + cluster1->info_->name_ = "fake_cluster"; + cluster1->info_->transport_socket_ = transport_socket; + + 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 keet 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, DynamicAddedAndKeepWarmingWithoutTlsCertificateEntity) { + 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; + + envoy::config::core::v3::TransportSocket transport_socket; + transport_socket.set_name("envoy.transport_sockets.tls"); + transport_socket.mutable_typed_config()->PackFrom(tls_context); + + create(defaultConfig()); + + InSequence s; + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::shared_ptr cluster1(new NiceMock()); + cluster1->info_->name_ = "fake_cluster"; + cluster1->info_->transport_socket_ = transport_socket; + + 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 keet 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())); +} + class MockConnPoolWithDestroy : public Http::ConnectionPool::MockInstance { public: ~MockConnPoolWithDestroy() override { onDestroy(); } From 5841c9e11c8bfbad230e0306e7fd362dfd7b6813 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 1 Oct 2020 10:36:27 +0000 Subject: [PATCH 06/22] typo Signed-off-by: Shikugawa --- test/common/upstream/cluster_manager_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 4d0800815a4fa..abb324e059f06 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -2346,7 +2346,7 @@ TEST_F(ClusterManagerImplTest, EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); cluster1->initialize_callback_(); - // Check to be keet warming fake_cluster after callback invoked. + // 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()); @@ -2391,7 +2391,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutTlsCertificateEn EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster")); cluster1->initialize_callback_(); - // Check to be keet warming fake_cluster after callback invoked. + // 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()); From 99276bc1e765ac7f43e981d7cc8139974cd4691a Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 1 Oct 2020 20:38:17 +0900 Subject: [PATCH 07/22] fix build Signed-off-by: Shikugawa --- test/extensions/clusters/aggregate/cluster_update_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/extensions/clusters/aggregate/cluster_update_test.cc b/test/extensions/clusters/aggregate/cluster_update_test.cc index b13d1e76db99f..0fbd53512c1f5 100644 --- a/test/extensions/clusters/aggregate/cluster_update_test.cc +++ b/test/extensions/clusters/aggregate/cluster_update_test.cc @@ -40,7 +40,7 @@ class AggregateClusterUpdateTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *api_, http_context_, grpc_context_); + *api_, http_context_, grpc_context_, secret_manager_); cluster_manager_->initializeSecondaryClusters(bootstrap); EXPECT_EQ(cluster_manager_->activeClusters().size(), 1); cluster_ = cluster_manager_->get("aggregate_cluster"); @@ -49,6 +49,7 @@ class AggregateClusterUpdateTest : public testing::Test { Stats::IsolatedStoreImpl stats_store_; NiceMock admin_; NiceMock factory_; + NiceMock secret_manager_; Api::ApiPtr api_{Api::createApiForTest(stats_store_, factory_.random_)}; Upstream::ThreadLocalCluster* cluster_; @@ -264,7 +265,7 @@ TEST_F(AggregateClusterUpdateTest, InitializeAggregateClusterAfterOtherClusters) cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, *api_, - http_context_, grpc_context_); + http_context_, grpc_context_, secret_manager_); cluster_manager_->initializeSecondaryClusters(bootstrap); EXPECT_EQ(cluster_manager_->activeClusters().size(), 2); cluster_ = cluster_manager_->get("aggregate_cluster"); From 6e95a07d583689aeed3b4fbed48b0da77aa4f72f Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 2 Oct 2020 11:07:41 +0900 Subject: [PATCH 08/22] test coverage Signed-off-by: Shikugawa --- test/common/secret/secret_manager_impl_test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 58304e1a1106e..4e5c0a72e5efc 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -360,6 +360,7 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { auto secret_provider = secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context); + EXPECT_FALSE(secret_manager->checkTlsCertificateEntityExists(config_source, "abc.com")); const std::string yaml = R"EOF( name: "abc.com" @@ -384,6 +385,7 @@ name: "abc.com" "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/selfsigned_key.pem"; EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), tls_config.privateKey()); + EXPECT_TRUE(secret_manager->checkTlsCertificateEntityExists(config_source, "abc.com")); } TEST_F(SecretManagerImplTest, SdsDynamicGenericSecret) { @@ -504,6 +506,8 @@ name: "abc.com" time_system_.setSystemTime(std::chrono::milliseconds(1234567899000)); auto context_secret_provider = secret_manager->findOrCreateCertificateValidationContextProvider( config_source, "abc.com.validation", secret_context); + EXPECT_FALSE(secret_manager->checkCertificateValidationContextEntityExists(config_source, + "abc.com.validation")); const std::string validation_yaml = R"EOF( name: "abc.com.validation" validation_context: @@ -518,6 +522,8 @@ name: "abc.com.validation" decoded_resources_2.refvec_, "validation-context-v1"); Ssl::CertificateValidationContextConfigImpl cert_validation_context( *context_secret_provider->secret(), *api_); + EXPECT_TRUE(secret_manager->checkCertificateValidationContextEntityExists(config_source, + "abc.com.validation")); EXPECT_EQ("DUMMY_INLINE_STRING_TRUSTED_CA", cert_validation_context.caCert()); const std::string updated_config_dump = R"EOF( dynamic_active_secrets: From 3c600ec266081cd54cb0d937a4bd953866ad368e Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 2 Oct 2020 14:22:44 +0900 Subject: [PATCH 09/22] Kick CI Signed-off-by: Shikugawa From cb5a3736dd11a8a8e1c5d62fba3f79e45b9b2034 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 8 Oct 2020 04:06:17 +0000 Subject: [PATCH 10/22] add comment Signed-off-by: Shikugawa --- source/common/upstream/cluster_manager_impl.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 45edee63bc5a6..58ee70d333526 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -428,6 +428,11 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { cluster.info()->transportSocket().typed_config()); const auto& common_tls_context = upstream_tls_context.common_tls_context(); + // 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): Consider retry strategy of clusters which failed to activate at once. for (const auto& sds_secret_config : common_tls_context.tls_certificate_sds_secret_configs()) { auto& config_name = sds_secret_config.name(); From 456b5b933d64f6a14409a77b31661fd39527c807 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 8 Oct 2020 14:35:25 +0900 Subject: [PATCH 11/22] build fix Signed-off-by: Shikugawa --- .../upstream/cluster_manager_impl_test.cc | 12 ++---------- test/common/upstream/test_cluster_manager.h | 2 +- .../clusters/aggregate/cluster_update_test.cc | 18 ++---------------- 3 files changed, 5 insertions(+), 27 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 90cb20bd45c87..168745adeb7cc 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -52,11 +52,7 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, -<<<<<<< HEAD - *api_, http_context_, grpc_context_, secret_manager_); -======= - *factory_.api_, http_context_, grpc_context_); ->>>>>>> 8888ee7e24999354c320bd5062fe500077fa142e + *factory_.api_, http_context_, grpc_context_, secret_manager_); cluster_manager_->setPrimaryClustersInitializedCb( [this, bootstrap]() { cluster_manager_->initializeSecondaryClusters(bootstrap); }); } @@ -100,12 +96,8 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, -<<<<<<< HEAD - *api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_, + *factory_.api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_, secret_manager_); -======= - *factory_.api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_); ->>>>>>> 8888ee7e24999354c320bd5062fe500077fa142e } void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active, diff --git a/test/common/upstream/test_cluster_manager.h b/test/common/upstream/test_cluster_manager.h index c70a649945129..ff9733e7e5eea 100644 --- a/test/common/upstream/test_cluster_manager.h +++ b/test/common/upstream/test_cluster_manager.h @@ -197,7 +197,7 @@ class MockedUpdatedClusterManagerImpl : public TestClusterManagerImpl { Secret::SecretManager& secret_manager) : TestClusterManagerImpl(bootstrap, factory, stats, tls, runtime, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context, secret_manager_), + grpc_context, secret_manager), local_cluster_update_(local_cluster_update), local_hosts_removed_(local_hosts_removed) {} protected: diff --git a/test/extensions/clusters/aggregate/cluster_update_test.cc b/test/extensions/clusters/aggregate/cluster_update_test.cc index 38b191356d9b8..0dc1f31d32573 100644 --- a/test/extensions/clusters/aggregate/cluster_update_test.cc +++ b/test/extensions/clusters/aggregate/cluster_update_test.cc @@ -40,11 +40,7 @@ class AggregateClusterUpdateTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, -<<<<<<< HEAD - *api_, http_context_, grpc_context_, secret_manager_); -======= - *factory_.api_, http_context_, grpc_context_); ->>>>>>> 8888ee7e24999354c320bd5062fe500077fa142e + *factory_.api_, http_context_, grpc_context_, secret_manager_); cluster_manager_->initializeSecondaryClusters(bootstrap); EXPECT_EQ(cluster_manager_->activeClusters().size(), 1); cluster_ = cluster_manager_->get("aggregate_cluster"); @@ -53,11 +49,7 @@ class AggregateClusterUpdateTest : public testing::Test { Stats::IsolatedStoreImpl stats_store_; NiceMock admin_; NiceMock factory_; -<<<<<<< HEAD NiceMock secret_manager_; - Api::ApiPtr api_{Api::createApiForTest(stats_store_, factory_.random_)}; -======= ->>>>>>> 8888ee7e24999354c320bd5062fe500077fa142e Upstream::ThreadLocalCluster* cluster_; Event::SimulatedTimeSystem time_system_; @@ -270,15 +262,9 @@ TEST_F(AggregateClusterUpdateTest, InitializeAggregateClusterAfterOtherClusters) auto bootstrap = parseBootstrapFromV2Yaml(config); cluster_manager_ = std::make_unique( -<<<<<<< HEAD - bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, *api_, - http_context_, grpc_context_, secret_manager_); -======= bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, *factory_.api_, - http_context_, grpc_context_); ->>>>>>> 8888ee7e24999354c320bd5062fe500077fa142e + http_context_, grpc_context_, secret_manager_); cluster_manager_->initializeSecondaryClusters(bootstrap); EXPECT_EQ(cluster_manager_->activeClusters().size(), 2); cluster_ = cluster_manager_->get("aggregate_cluster"); From 476d70679f6f92d6f4455a807cee4e9f40b4bcf6 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Sun, 18 Oct 2020 04:56:33 +0000 Subject: [PATCH 12/22] transport socket abstraction Signed-off-by: Shikugawa --- include/envoy/network/transport_socket.h | 5 + include/envoy/ssl/context_config.h | 12 +++ include/envoy/upstream/upstream.h | 6 -- source/common/network/raw_buffer_socket.cc | 2 + source/common/network/raw_buffer_socket.h | 1 + source/common/secret/secret_manager_impl.h | 2 +- .../common/upstream/cluster_manager_impl.cc | 46 ++------- source/common/upstream/cluster_manager_impl.h | 4 +- source/common/upstream/upstream_impl.cc | 1 - source/common/upstream/upstream_impl.h | 4 - .../quiche/quic_transport_socket_factory.h | 1 + .../transport_sockets/alts/tsi_socket.cc | 1 + .../transport_sockets/alts/tsi_socket.h | 1 + .../proxy_protocol/proxy_protocol.cc | 6 +- .../proxy_protocol/proxy_protocol.h | 1 + .../extensions/transport_sockets/tap/tap.cc | 2 + source/extensions/transport_sockets/tap/tap.h | 1 + .../transport_sockets/tls/config.cc | 2 +- .../tls/context_config_impl.cc | 5 + .../tls/context_config_impl.h | 11 +++ .../transport_sockets/tls/ssl_socket.cc | 26 ++++- .../transport_sockets/tls/ssl_socket.h | 9 +- .../config_validation/cluster_manager.cc | 9 +- .../config_validation/cluster_manager.h | 2 +- .../grpc_client_integration_test_harness.h | 3 +- .../upstream/cluster_manager_impl_test.cc | 54 ++++------- test/common/upstream/test_cluster_manager.h | 10 +- .../upstream/transport_socket_matcher_test.cc | 2 + .../clusters/aggregate/cluster_update_test.cc | 5 +- .../transport_sockets/tls/ssl_socket_test.cc | 95 ++++++++++++++++--- test/integration/ssl_utility.cc | 5 +- test/integration/xfcc_integration_test.cc | 3 +- test/mocks/network/transport_socket.h | 1 + test/mocks/ssl/mocks.h | 8 ++ test/mocks/upstream/cluster_info.cc | 1 - test/mocks/upstream/cluster_info.h | 2 - 36 files changed, 217 insertions(+), 132 deletions(-) diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h index db500f86a8a66..3e52d625f6582 100644 --- a/include/envoy/network/transport_socket.h +++ b/include/envoy/network/transport_socket.h @@ -233,6 +233,11 @@ class TransportSocketFactory { * are already loaded. */ virtual void addReadyCb(std::function callback) PURE; + + /** + * Check whether matched transport socket which required to use secret information is available. + */ + virtual bool secureTransportReady() const PURE; }; using TransportSocketFactoryPtr = std::unique_ptr; diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 675bddde27def..47fefd4a82ee7 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -84,6 +84,18 @@ class ContextConfig { * @return the set of capabilities for handshaker instances created by this context. */ virtual HandshakerCapabilities capabilities() const PURE; + + /** + * @return the set of sds configurations for TLS certificate entity. + */ + virtual const std::vector& + tlsCertificateSdsConfigs() const PURE; + + /** + * @return sds configuration for validation context entity. + */ + virtual const envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig& + validationContextSdsConfig() const PURE; }; class ClientContextConfig : public virtual ContextConfig { diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index dd94a57c85246..2c51a7c261c48 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -847,12 +847,6 @@ class ClusterInfo { virtual const absl::optional& upstreamConfig() const PURE; - /** - * @return const envoy::config::core::v3::TransportSocket& the transport socket config for - * upstream connection. - */ - virtual const envoy::config::core::v3::TransportSocket& transportSocket() const PURE; - /** * @return Whether the cluster is currently in maintenance mode and should not be routed to. * Different filters may handle this situation in different ways. The implementation diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc index c539add2f71ad..317ed9bdc3016 100644 --- a/source/common/network/raw_buffer_socket.cc +++ b/source/common/network/raw_buffer_socket.cc @@ -93,5 +93,7 @@ RawBufferSocketFactory::createTransportSocket(TransportSocketOptionsSharedPtr) c } bool RawBufferSocketFactory::implementsSecureTransport() const { return false; } + +bool RawBufferSocketFactory::secureTransportReady() const { return false; } } // namespace Network } // namespace Envoy diff --git a/source/common/network/raw_buffer_socket.h b/source/common/network/raw_buffer_socket.h index 8f17279890aa9..73d97966e4138 100644 --- a/source/common/network/raw_buffer_socket.h +++ b/source/common/network/raw_buffer_socket.h @@ -33,6 +33,7 @@ class RawBufferSocketFactory : public TransportSocketFactory { TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; void addReadyCb(std::function callback) override { callback(); } + bool secureTransportReady() const override; }; } // namespace Network diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index f5b7c6d88cc45..ff4e142952e70 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -108,7 +108,7 @@ class SecretManagerImpl : public SecretManager { absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name); std::shared_ptr secret_provider = dynamic_secret_providers_[map_key].lock(); if (!secret_provider) { - return false; + return true; } return secret_provider->secret() != nullptr; } diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 88227dc9c30dc..860888a25fbe8 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -235,11 +235,10 @@ void ClusterManagerInitHelper::setPrimaryClustersInitializedCb( ClusterManagerImpl::ClusterManagerImpl( const envoy::config::bootstrap::v3::Bootstrap& bootstrap, ClusterManagerFactory& factory, Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime, - const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, - Http::Context& http_context, Grpc::Context& grpc_context, Secret::SecretManager& secret_manager) + Http::Context& http_context, Grpc::Context& grpc_context) : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()), random_(api.randomGenerator()), bind_config_(bootstrap.cluster_manager().upstream_bind_config()), local_info_(local_info), @@ -250,8 +249,7 @@ ClusterManagerImpl::ClusterManagerImpl( time_source_(main_thread_dispatcher.timeSource()), dispatcher_(main_thread_dispatcher), http_context_(http_context), subscription_factory_(local_info, main_thread_dispatcher, *this, - validation_context.dynamicValidationVisitor(), api, runtime_), - secret_manager_(secret_manager) { + validation_context.dynamicValidationVisitor(), api, runtime_) { async_client_manager_ = std::make_unique( *this, tls, time_source_, api, grpc_context.statNames()); const auto& cm_config = bootstrap.cluster_manager(); @@ -425,40 +423,12 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // 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()) { - if (cluster.info()->transportSocket().has_typed_config()) { - auto upstream_tls_context = MessageUtil::anyConvert< - envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext>( - cluster.info()->transportSocket().typed_config()); - const auto& common_tls_context = upstream_tls_context.common_tls_context(); - - // 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): Consider retry strategy of clusters which failed to activate at once. - for (const auto& sds_secret_config : - common_tls_context.tls_certificate_sds_secret_configs()) { - auto& config_name = sds_secret_config.name(); - auto& config_source = sds_secret_config.sds_config(); - if (!secret_manager_.checkTlsCertificateEntityExists(config_source, config_name)) { - ENVOY_LOG(warn, "Failed to activate {} on {}", config_name, cluster.info()->name()); - return; - } - } - - if (common_tls_context.has_validation_context_sds_secret_config()) { - const auto& validation_context_sds_secret_config = - common_tls_context.validation_context_sds_secret_config(); - auto& config_name = validation_context_sds_secret_config.name(); - auto& config_source = validation_context_sds_secret_config.sds_config(); - if (!secret_manager_.checkCertificateValidationContextEntityExists(config_source, - config_name)) { - ENVOY_LOG(warn, "Failed to activate {} on {}", config_name, cluster.info()->name()); - return; - } - } + Network::TransportSocketFactory& factory = + cluster.info()->transportSocketMatcher().resolve(&cluster.info()->metadata()).factory_; + if (factory.implementsSecureTransport() && !factory.secureTransportReady()) { + ENVOY_LOG(warn, "Failed to activate {}", cluster.info()->name()); + return; } - clusterWarmingToActive(cluster.info()->name()); updateClusterCounts(); } @@ -1495,7 +1465,7 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { return ClusterManagerPtr{new ClusterManagerImpl( bootstrap, *this, stats_, tls_, runtime_, local_info_, log_manager_, main_thread_dispatcher_, - admin_, validation_context_, api_, http_context_, grpc_context_, secret_manager_)}; + admin_, validation_context_, api_, http_context_, grpc_context_)}; } Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool( diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 9345700f0d9d2..897125f933728 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -199,8 +199,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable extension_protocol_options_; mutable ResourceManagers resource_managers_; const std::string maintenance_mode_runtime_key_; 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 4fbe83286b387..daa4f4b3af2bd 100644 --- a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h +++ b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h @@ -27,6 +27,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { // TODO(mpuncel) only invoke callback() once secrets are ready. void addReadyCb(std::function callback) override { callback(); }; + bool secureTransportReady() const override { NOT_REACHED_GCOVR_EXCL_LINE; } }; // 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 0fe5b752ceca3..2a9b39dbace1d 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.cc +++ b/source/extensions/transport_sockets/alts/tsi_socket.cc @@ -261,6 +261,7 @@ TsiSocketFactory::createTransportSocket(Network::TransportSocketOptionsSharedPtr return std::make_unique(handshaker_factory_, handshake_validator_); } +bool TsiSocketFactory::secureTransportReady() const { NOT_REACHED_GCOVR_EXCL_LINE; } } // 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 7bf5877870ab3..e281000d33ce0 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.h +++ b/source/extensions/transport_sockets/alts/tsi_socket.h @@ -103,6 +103,7 @@ class TsiSocketFactory : public Network::TransportSocketFactory { // TODO(mpuncel) only invoke callback() once secrets are ready. void addReadyCb(std::function callback) override { callback(); }; + bool secureTransportReady() 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 3d4f716421e72..ad13bfd4a1b2e 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc @@ -123,7 +123,11 @@ bool UpstreamProxyProtocolSocketFactory::implementsSecureTransport() const { return transport_socket_factory_->implementsSecureTransport(); } +bool UpstreamProxyProtocolSocketFactory::secureTransportReady() const { + NOT_REACHED_GCOVR_EXCL_LINE; +} + } // namespace ProxyProtocol } // namespace TransportSockets } // namespace Extensions -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h index bcddef7bf547b..e493b0f50577a 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h @@ -50,6 +50,7 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override; bool implementsSecureTransport() const override; void addReadyCb(std::function callback) override { callback(); }; + bool secureTransportReady() 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 7674ba6b584d4..6379741a70c58 100644 --- a/source/extensions/transport_sockets/tap/tap.cc +++ b/source/extensions/transport_sockets/tap/tap.cc @@ -66,6 +66,8 @@ bool TapSocketFactory::implementsSecureTransport() const { return transport_socket_factory_->implementsSecureTransport(); } +bool TapSocketFactory::secureTransportReady() const { NOT_REACHED_GCOVR_EXCL_LINE; } + } // 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 d04712b2a50a8..b6e159331b8a5 100644 --- a/source/extensions/transport_sockets/tap/tap.h +++ b/source/extensions/transport_sockets/tap/tap.h @@ -43,6 +43,7 @@ class TapSocketFactory : public Network::TransportSocketFactory, bool implementsSecureTransport() const override; // TODO(mpuncel) only invoke callback() once secrets are ready. void addReadyCb(std::function callback) override { callback(); }; + bool secureTransportReady() const override; private: Network::TransportSocketFactoryPtr transport_socket_factory_; diff --git a/source/extensions/transport_sockets/tls/config.cc b/source/extensions/transport_sockets/tls/config.cc index c743f5f6def86..403cb8ae2cf25 100644 --- a/source/extensions/transport_sockets/tls/config.cc +++ b/source/extensions/transport_sockets/tls/config.cc @@ -21,7 +21,7 @@ Network::TransportSocketFactoryPtr UpstreamSslSocketFactory::createTransportSock const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext&>( message, context.messageValidationVisitor()), context); - return std::make_unique(std::move(client_config), + return std::make_unique(context.secretManager(), std::move(client_config), context.sslContextManager(), context.scope()); } diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc index 546489232e742..dbdd93f23fdf1 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -234,6 +234,11 @@ ContextConfigImpl::ContextConfigImpl( factory_context.messageValidationVisitor()); } capabilities_ = handshaker_factory->capabilities(); + + for (const auto& sds_secret_config : config.tls_certificate_sds_secret_configs()) { + tls_certificate_sds_configs_.emplace_back(sds_secret_config); + } + validation_context_sds_config_ = config.validation_context_sds_secret_config(); } Ssl::CertificateValidationContextConfigPtr ContextConfigImpl::getCombinedValidationContextConfig( diff --git a/source/extensions/transport_sockets/tls/context_config_impl.h b/source/extensions/transport_sockets/tls/context_config_impl.h index 44c5a8cc619d0..285010d11cb63 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.h +++ b/source/extensions/transport_sockets/tls/context_config_impl.h @@ -37,6 +37,14 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { } return configs; } + const std::vector& + tlsCertificateSdsConfigs() const override { + return tls_certificate_sds_configs_; + } + const envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig& + validationContextSdsConfig() const override { + return validation_context_sds_config_; + } const Envoy::Ssl::CertificateValidationContextConfig* certificateValidationContext() const override { return validation_context_config_.get(); @@ -81,6 +89,9 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { std::vector tls_certificate_configs_; Ssl::CertificateValidationContextConfigPtr validation_context_config_; + std::vector + tls_certificate_sds_configs_; + envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig validation_context_sds_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. diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 523242f8fada0..9754b32143926 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -322,11 +322,12 @@ SslSocketFactoryStats generateStats(const std::string& prefix, Stats::Scope& sto } } // namespace -ClientSslSocketFactory::ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config, +ClientSslSocketFactory::ClientSslSocketFactory(Secret::SecretManager& secret_manager, + Envoy::Ssl::ClientContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope) - : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), - config_(std::move(config)), + : secret_manager_(secret_manager), manager_(manager), stats_scope_(stats_scope), + stats_(generateStats("client", stats_scope)), config_(std::move(config)), ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } @@ -388,6 +389,24 @@ void ClientSslSocketFactory::addReadyCb(std::function callback) { } } +bool ClientSslSocketFactory::secureTransportReady() const { + if (!implementsSecureTransport()) { + return false; + } + const auto& tls_certificate_sds_configs = config_->tlsCertificateSdsConfigs(); + for (const auto& config : tls_certificate_sds_configs) { + if (!secret_manager_.checkTlsCertificateEntityExists(config.sds_config(), config.name())) { + return false; + } + } + const auto& validation_context_sds_config = config_->validationContextSdsConfig(); + if (!secret_manager_.checkCertificateValidationContextEntityExists( + validation_context_sds_config.sds_config(), validation_context_sds_config.name())) { + return false; + } + return true; +} + ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope, @@ -455,6 +474,7 @@ void ServerSslSocketFactory::addReadyCb(std::function callback) { } } +bool ServerSslSocketFactory::secureTransportReady() const { NOT_REACHED_GCOVR_EXCL_LINE; } } // 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 b0dcb139a3193..9550bf05359f7 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -6,6 +6,7 @@ #include "envoy/network/connection.h" #include "envoy/network/transport_socket.h" #include "envoy/secret/secret_callbacks.h" +#include "envoy/secret/secret_manager.h" #include "envoy/ssl/handshaker.h" #include "envoy/ssl/private_key/private_key_callbacks.h" #include "envoy/ssl/ssl_socket_extended_info.h" @@ -102,19 +103,22 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, public Secret::SecretCallbacks, Logger::Loggable { public: - ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config, + ClientSslSocketFactory(Secret::SecretManager& secret_manager, + 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; - void addReadyCb(std::function callback) override; + bool secureTransportReady() const override; // Secret::SecretCallbacks void onAddOrUpdateSecret() override; private: + Secret::SecretManager& secret_manager_; Envoy::Ssl::ContextManager& manager_; Stats::Scope& stats_scope_; SslSocketFactoryStats stats_; @@ -137,6 +141,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, bool implementsSecureTransport() const override; void addReadyCb(std::function callback) override; + bool secureTransportReady() const override; // Secret::SecretCallbacks void onAddOrUpdateSecret() override; diff --git a/source/server/config_validation/cluster_manager.cc b/source/server/config_validation/cluster_manager.cc index b41b5051d62de..38e1413da9d9e 100644 --- a/source/server/config_validation/cluster_manager.cc +++ b/source/server/config_validation/cluster_manager.cc @@ -2,7 +2,6 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/core/v3/config_source.pb.h" -#include "envoy/secret/secret_manager.h" #include "common/common/utility.h" @@ -13,8 +12,7 @@ ClusterManagerPtr ValidationClusterManagerFactory::clusterManagerFromProto( const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { return std::make_unique( bootstrap, *this, stats_, tls_, runtime_, local_info_, log_manager_, main_thread_dispatcher_, - admin_, validation_context_, api_, http_context_, grpc_context_, time_system_, - secret_manager_); + admin_, validation_context_, api_, http_context_, grpc_context_, time_system_); } CdsApiPtr @@ -32,11 +30,10 @@ ValidationClusterManager::ValidationClusterManager( const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, - Http::Context& http_context, Grpc::Context& grpc_context, Event::TimeSystem& time_system, - Secret::SecretManager& secret_manager) + Http::Context& http_context, Grpc::Context& grpc_context, Event::TimeSystem& time_system) : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context, secret_manager), + grpc_context), async_client_(api, time_system) {} Http::ConnectionPool::Instance* ValidationClusterManager::httpConnPoolForCluster( diff --git a/source/server/config_validation/cluster_manager.h b/source/server/config_validation/cluster_manager.h index d5efca9eba354..38bda5ad2f8d3 100644 --- a/source/server/config_validation/cluster_manager.h +++ b/source/server/config_validation/cluster_manager.h @@ -62,7 +62,7 @@ class ValidationClusterManager : public ClusterManagerImpl { Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, Http::Context& http_context, Grpc::Context& grpc_context, - Event::TimeSystem& time_system, Secret::SecretManager& secret_manager); + Event::TimeSystem& time_system); Http::ConnectionPool::Instance* httpConnPoolForCluster(const std::string&, ResourcePriority, absl::optional, diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index d610a221f8b74..bfea31dbe72f6 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -478,6 +478,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest { Router::MockShadowWriter* mock_shadow_writer_ = new Router::MockShadowWriter(); Router::ShadowWriterPtr shadow_writer_ptr_{mock_shadow_writer_}; Network::ClientConnectionPtr client_connection_; + Secret::MockSecretManager secret_manager_; }; // SSL connection credential validation tests. @@ -519,7 +520,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { mock_host_description_->socket_factory_ = std::make_unique( - std::move(cfg), context_manager_, *stats_store_); + secret_manager_, std::move(cfg), context_manager_, *stats_store_); async_client_transport_socket_ = mock_host_description_->socket_factory_->createTransportSocket(nullptr); fake_upstream_ = std::make_unique(createUpstreamSslContext(), 0, diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 168745adeb7cc..151c4cd910b58 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -4,7 +4,6 @@ #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 "envoy/secret/secret_manager.h" #include "test/common/upstream/test_cluster_manager.h" #include "test/mocks/upstream/cds_api.h" @@ -52,7 +51,7 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *factory_.api_, http_context_, grpc_context_, secret_manager_); + *factory_.api_, http_context_, grpc_context_); cluster_manager_->setPrimaryClustersInitializedCb( [this, bootstrap]() { cluster_manager_->initializeSecondaryClusters(bootstrap); }); } @@ -96,8 +95,7 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *factory_.api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_, - secret_manager_); + *factory_.api_, local_cluster_update_, local_hosts_removed_, http_context_, grpc_context_); } void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active, @@ -143,7 +141,6 @@ class ClusterManagerImplTest : public testing::Test { std::unique_ptr cluster_manager_; AccessLog::MockAccessLogManager log_manager_; NiceMock admin_; - NiceMock secret_manager_; MockLocalClusterUpdate local_cluster_update_; MockLocalHostsRemoved local_hosts_removed_; Http::ContextImpl http_context_; @@ -2312,28 +2309,22 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutCertificateValidationContextEntity) { - 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); - - envoy::config::core::v3::TransportSocket transport_socket; - transport_socket.set_name("envoy.transport_sockets.tls"); - transport_socket.mutable_typed_config()->PackFrom(tls_context); - create(defaultConfig()); - InSequence s; ReadyWatcher initialized; EXPECT_CALL(initialized, ready()); cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); std::shared_ptr cluster1(new NiceMock()); cluster1->info_->name_ = "fake_cluster"; - cluster1->info_->transport_socket_ = transport_socket; + + auto transport_socket_factory = std::make_unique(); + EXPECT_CALL(*transport_socket_factory, implementsSecureTransport()).WillOnce(Return(true)); + EXPECT_CALL(*transport_socket_factory, secureTransportReady()).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))); @@ -2354,31 +2345,22 @@ TEST_F(ClusterManagerImplTest, } TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutTlsCertificateEntity) { - 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; - - envoy::config::core::v3::TransportSocket transport_socket; - transport_socket.set_name("envoy.transport_sockets.tls"); - transport_socket.mutable_typed_config()->PackFrom(tls_context); - create(defaultConfig()); - InSequence s; ReadyWatcher initialized; EXPECT_CALL(initialized, ready()); cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); std::shared_ptr cluster1(new NiceMock()); cluster1->info_->name_ = "fake_cluster"; - cluster1->info_->transport_socket_ = transport_socket; + + auto transport_socket_factory = std::make_unique(); + EXPECT_CALL(*transport_socket_factory, implementsSecureTransport()).WillOnce(Return(true)); + EXPECT_CALL(*transport_socket_factory, secureTransportReady()).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))); diff --git a/test/common/upstream/test_cluster_manager.h b/test/common/upstream/test_cluster_manager.h index ff9733e7e5eea..d76c5b06b0f10 100644 --- a/test/common/upstream/test_cluster_manager.h +++ b/test/common/upstream/test_cluster_manager.h @@ -165,11 +165,10 @@ class TestClusterManagerImpl : public ClusterManagerImpl { AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, - Http::Context& http_context, Grpc::Context& grpc_context, - Secret::SecretManager& secret_manager) + Http::Context& http_context, Grpc::Context& grpc_context) : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context, secret_manager) {} + grpc_context) {} std::map> activeClusters() { std::map> clusters; @@ -193,11 +192,10 @@ class MockedUpdatedClusterManagerImpl : public TestClusterManagerImpl { ProtobufMessage::ValidationContext& validation_context, Api::Api& api, MockLocalClusterUpdate& local_cluster_update, MockLocalHostsRemoved& local_hosts_removed, - Http::Context& http_context, Grpc::Context& grpc_context, - Secret::SecretManager& secret_manager) + Http::Context& http_context, Grpc::Context& grpc_context) : TestClusterManagerImpl(bootstrap, factory, stats, tls, runtime, local_info, log_manager, main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context, secret_manager), + grpc_context), local_cluster_update_(local_cluster_update), local_hosts_removed_(local_hosts_removed) {} protected: diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index f770d1f4fdd87..ea62feacade95 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -34,6 +34,7 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); MOCK_METHOD(void, addReadyCb, (std::function)); + MOCK_METHOD(bool, secureTransportReady, (), (const)); FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} std::string id() const { return id_; } @@ -50,6 +51,7 @@ class FooTransportSocketFactory MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsSharedPtr), (const)); MOCK_METHOD(void, addReadyCb, (std::function)); + MOCK_METHOD(bool, secureTransportReady, (), (const)); Network::TransportSocketFactoryPtr createTransportSocketFactory(const Protobuf::Message& proto, diff --git a/test/extensions/clusters/aggregate/cluster_update_test.cc b/test/extensions/clusters/aggregate/cluster_update_test.cc index 0dc1f31d32573..8755ba8a29ef6 100644 --- a/test/extensions/clusters/aggregate/cluster_update_test.cc +++ b/test/extensions/clusters/aggregate/cluster_update_test.cc @@ -40,7 +40,7 @@ class AggregateClusterUpdateTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, - *factory_.api_, http_context_, grpc_context_, secret_manager_); + *factory_.api_, http_context_, grpc_context_); cluster_manager_->initializeSecondaryClusters(bootstrap); EXPECT_EQ(cluster_manager_->activeClusters().size(), 1); cluster_ = cluster_manager_->get("aggregate_cluster"); @@ -49,7 +49,6 @@ class AggregateClusterUpdateTest : public testing::Test { Stats::IsolatedStoreImpl stats_store_; NiceMock admin_; NiceMock factory_; - NiceMock secret_manager_; Upstream::ThreadLocalCluster* cluster_; Event::SimulatedTimeSystem time_system_; @@ -264,7 +263,7 @@ TEST_F(AggregateClusterUpdateTest, InitializeAggregateClusterAfterOtherClusters) cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, *factory_.api_, - http_context_, grpc_context_, secret_manager_); + http_context_, grpc_context_); cluster_manager_->initializeSecondaryClusters(bootstrap); EXPECT_EQ(cluster_manager_->activeClusters().size(), 2); cluster_ = cluster_manager_->get("aggregate_cluster"); diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index b06a9ac662033..ee3b215d9196a 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -333,7 +333,8 @@ void testUtil(const TestUtilOptions& options) { auto client_cfg = std::make_unique(client_tls_context, client_factory_context); - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + auto secret_manager = Secret::MockSecretManager(); + ClientSslSocketFactory client_ssl_socket_factory(secret_manager, std::move(client_cfg), manager, client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -640,9 +641,10 @@ const std::string testUtilV2(const TestUtilOptionsV2& options) { client_factory_context; ON_CALL(client_factory_context, api()).WillByDefault(ReturnRef(*client_api)); + auto secret_manager = Secret::MockSecretManager(); auto client_cfg = std::make_unique(options.clientCtxProto(), client_factory_context); - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + ClientSslSocketFactory client_ssl_socket_factory(secret_manager, std::move(client_cfg), manager, client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -830,6 +832,7 @@ class SslSocketTest : public SslCertsTest, Event::DispatcherPtr dispatcher_; StreamInfo::StreamInfoImpl stream_info_; + Secret::MockSecretManager secret_manager_; }; INSTANTIATE_TEST_SUITE_P(IpVersions, SslSocketTest, @@ -2490,7 +2493,7 @@ TEST_P(SslSocketTest, HalfClose) { TestUtility::loadFromYaml(TestEnvironment::substitute(client_ctx_yaml), tls_context); auto client_cfg = std::make_unique(tls_context, factory_context_); Stats::TestUtil::TestStore client_stats_store; - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + ClientSslSocketFactory client_ssl_socket_factory(secret_manager_, std::move(client_cfg), manager, client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2576,7 +2579,8 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { TestUtility::loadFromYaml(TestEnvironment::substitute(client_ctx_yaml), tls_context); auto client_cfg = std::make_unique(tls_context, factory_context_); Stats::TestUtil::TestStore client_stats_store; - ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, client_stats_store); + ClientSslSocketFactory ssl_socket_factory(secret_manager_, std::move(client_cfg), manager, + client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(nullptr), nullptr); @@ -2673,9 +2677,11 @@ void testTicketSessionResumption(const std::string& server_ctx_yaml1, client_factory_context; ON_CALL(client_factory_context, api()).WillByDefault(ReturnRef(*client_api)); + auto secret_manager = Secret::MockSecretManager(); auto client_cfg = std::make_unique(client_tls_context, client_factory_context); - ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, client_stats_store); + ClientSslSocketFactory ssl_socket_factory(secret_manager, std::move(client_cfg), manager, + client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher->createClientConnection( socket1->localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(nullptr), nullptr); @@ -2809,9 +2815,11 @@ void testSupportForStatelessSessionResumption(const std::string& server_ctx_yaml client_factory_context; ON_CALL(client_factory_context, api()).WillByDefault(ReturnRef(*client_api)); + auto secret_manager = Secret::MockSecretManager(); auto client_cfg = std::make_unique(client_tls_context, client_factory_context); - ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, client_stats_store); + ClientSslSocketFactory ssl_socket_factory(secret_manager, std::move(client_cfg), manager, + client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher->createClientConnection( tcp_socket->localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(nullptr), nullptr); @@ -3258,7 +3266,8 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { auto client_cfg = std::make_unique(tls_context, factory_context_); Stats::TestUtil::TestStore client_stats_store; - ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, client_stats_store); + ClientSslSocketFactory ssl_socket_factory(secret_manager_, std::move(client_cfg), manager, + client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), ssl_socket_factory.createTransportSocket(nullptr), nullptr); @@ -3374,7 +3383,7 @@ void SslSocketTest::testClientSessionResumption(const std::string& server_ctx_ya auto client_cfg = std::make_unique(client_ctx_proto, client_factory_context); - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + ClientSslSocketFactory client_ssl_socket_factory(secret_manager_, std::move(client_cfg), manager, client_stats_store); Network::ClientConnectionPtr client_connection = dispatcher->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -4531,7 +4540,8 @@ 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); + ClientSslSocketFactory client_ssl_socket_factory(secret_manager_, std::move(client_cfg), manager, + stats_store); // Add a secrets ready callback that should not be invoked. MockFunction mock_callback_; @@ -4549,6 +4559,63 @@ TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { EXPECT_EQ("TLS error: Secret is not supplied by SDS", transport_socket->failureReason()); } +TEST_P(SslSocketTest, TransportSocketNotReadyWhenNoTlsCertEntity) { + 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(secret_manager_, std::move(client_cfg), + context_manager, stats_store); + EXPECT_CALL(secret_manager_, checkTlsCertificateEntityExists(_, _)).WillOnce(Return(false)); + EXPECT_CALL(secret_manager_, checkCertificateValidationContextEntityExists(_, _)).Times(0); + EXPECT_FALSE(client_ssl_socket_factory.secureTransportReady()); +} + +TEST_P(SslSocketTest, TransportSocketNotReadyWhenNoValidationContextEntity) { + 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_config = + tls_context.mutable_common_tls_context()->mutable_validation_context_sds_secret_config(); + sds_secret_config->set_name("abc.com"); + sds_secret_config->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(secret_manager_, std::move(client_cfg), + context_manager, stats_store); + EXPECT_CALL(secret_manager_, checkTlsCertificateEntityExists(_, _)).Times(0); + EXPECT_CALL(secret_manager_, checkCertificateValidationContextEntityExists(_, _)) + .WillOnce(Return(false)); + EXPECT_FALSE(client_ssl_socket_factory.secureTransportReady()); +} + // Validate that secrets callbacks are invoked when secrets become ready. TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { Stats::TestUtil::TestStore stats_store; @@ -4571,8 +4638,8 @@ TEST_P(SslSocketTest, ClientAddSecretsReadyCallback) { EXPECT_FALSE(client_cfg->isReady()); NiceMock context_manager; - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), context_manager, - stats_store); + ClientSslSocketFactory client_ssl_socket_factory(secret_manager_, 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_; @@ -4657,7 +4724,8 @@ TEST_P(SslSocketTest, TestTransportSocketCallback) { auto client_cfg = std::make_unique(tls_context, factory_context); ContextManagerImpl manager(time_system_); - ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); + ClientSslSocketFactory client_ssl_socket_factory(secret_manager_, std::move(client_cfg), manager, + stats_store); Network::TransportSocketPtr transport_socket = client_ssl_socket_factory.createTransportSocket(nullptr); @@ -4693,7 +4761,7 @@ class SslReadBufferLimitTest : public SslSocketTest { std::make_unique(upstream_tls_context_, factory_context_); client_ssl_socket_factory_ = std::make_unique( - std::move(client_cfg), *manager_, client_stats_store_); + secret_manager_, std::move(client_cfg), *manager_, client_stats_store_); auto transport_socket = client_ssl_socket_factory_->createTransportSocket(nullptr); client_transport_socket_ = transport_socket.get(); client_connection_ = dispatcher_->createClientConnection( @@ -4870,6 +4938,7 @@ class SslReadBufferLimitTest : public SslSocketTest { std::shared_ptr read_filter_; StrictMock client_callbacks_; Network::Address::InstanceConstSharedPtr source_address_; + Secret::MockSecretManager secret_manager_; }; INSTANTIATE_TEST_SUITE_P(IpVersions, SslReadBufferLimitTest, diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index aab3dd5d4adf8..a9c80ec751c58 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -77,9 +77,10 @@ createClientSslTransportSocketFactory(const ClientSslTransportOptions& options, auto cfg = std::make_unique( tls_context, options.sigalgs_, mock_factory_ctx); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); + auto secret_manager = Secret::MockSecretManager(); return Network::TransportSocketFactoryPtr{ - new Extensions::TransportSockets::Tls::ClientSslSocketFactory(std::move(cfg), context_manager, - *client_stats_store)}; + new Extensions::TransportSockets::Tls::ClientSslSocketFactory( + secret_manager, std::move(cfg), context_manager, *client_stats_store)}; } Network::TransportSocketFactoryPtr createUpstreamSslContext(ContextManager& context_manager, diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index 3b299de13c513..8f16dc6dd1cba 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -79,9 +79,10 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b auto cfg = std::make_unique( config, factory_context_); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); + auto secret_manager = Secret::MockSecretManager(); return Network::TransportSocketFactoryPtr{ new Extensions::TransportSockets::Tls::ClientSslSocketFactory( - std::move(cfg), *context_manager_, *client_stats_store)}; + secret_manager, std::move(cfg), *context_manager_, *client_stats_store)}; } Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext() { diff --git a/test/mocks/network/transport_socket.h b/test/mocks/network/transport_socket.h index af8949aea99b6..b19a10eb62ff0 100644 --- a/test/mocks/network/transport_socket.h +++ b/test/mocks/network/transport_socket.h @@ -39,6 +39,7 @@ class MockTransportSocketFactory : public TransportSocketFactory { MOCK_METHOD(TransportSocketPtr, createTransportSocket, (TransportSocketOptionsSharedPtr), (const)); MOCK_METHOD(void, addReadyCb, (std::function)); + MOCK_METHOD(bool, secureTransportReady, (), (const)); }; } // namespace Network diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index beafd9de8720a..5a401dce615de 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -83,6 +83,10 @@ class MockClientContextConfig : public ClientContextConfig { MOCK_METHOD(std::vector>, tlsCertificates, (), (const)); MOCK_METHOD(const CertificateValidationContextConfig*, certificateValidationContext, (), (const)); + MOCK_METHOD(const std::vector&, + tlsCertificateSdsConfigs, (), (const)); + MOCK_METHOD(const envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig&, + validationContextSdsConfig, (), (const)); MOCK_METHOD(unsigned, minProtocolVersion, (), (const)); MOCK_METHOD(unsigned, maxProtocolVersion, (), (const)); MOCK_METHOD(bool, isReady, (), (const)); @@ -119,6 +123,10 @@ class MockServerContextConfig : public ServerContextConfig { MOCK_METHOD(std::vector>, tlsCertificates, (), (const)); MOCK_METHOD(const CertificateValidationContextConfig*, certificateValidationContext, (), (const)); + MOCK_METHOD(const std::vector&, + tlsCertificateSdsConfigs, (), (const)); + MOCK_METHOD(const envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig&, + validationContextSdsConfig, (), (const)); MOCK_METHOD(unsigned, minProtocolVersion, (), (const)); MOCK_METHOD(unsigned, maxProtocolVersion, (), (const)); MOCK_METHOD(bool, isReady, (), (const)); diff --git a/test/mocks/upstream/cluster_info.cc b/test/mocks/upstream/cluster_info.cc index ce5f49b8629f2..87afe77c30147 100644 --- a/test/mocks/upstream/cluster_info.cc +++ b/test/mocks/upstream/cluster_info.cc @@ -96,7 +96,6 @@ MockClusterInfo::MockClusterInfo() ON_CALL(*this, metadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, upstreamHttpProtocolOptions()) .WillByDefault(ReturnRef(upstream_http_protocol_options_)); - ON_CALL(*this, transportSocket()).WillByDefault(ReturnRef(transport_socket_)); // Delayed construction of typed_metadata_, to allow for injection of metadata ON_CALL(*this, typedMetadata()) .WillByDefault(Invoke([this]() -> const Envoy::Config::TypedMetadata& { diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 1b78738964997..7e1427b2de036 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -101,7 +101,6 @@ class MockClusterInfo : public ClusterInfo { MOCK_METHOD(const envoy::config::core::v3::Http2ProtocolOptions&, http2Options, (), (const)); MOCK_METHOD(const envoy::config::core::v3::HttpProtocolOptions&, commonHttpProtocolOptions, (), (const)); - MOCK_METHOD(const envoy::config::core::v3::TransportSocket&, transportSocket, (), (const)); MOCK_METHOD(ProtocolOptionsConfigConstSharedPtr, extensionProtocolOptions, (const std::string&), (const)); MOCK_METHOD(const envoy::config::cluster::v3::Cluster::CommonLbConfig&, lbConfig, (), (const)); @@ -153,7 +152,6 @@ class MockClusterInfo : public ClusterInfo { Http::Http1Settings http1_settings_; envoy::config::core::v3::Http2ProtocolOptions http2_options_; envoy::config::core::v3::HttpProtocolOptions common_http_protocol_options_; - envoy::config::core::v3::TransportSocket transport_socket_; ProtocolOptionsConfigConstSharedPtr extension_protocol_options_; uint64_t max_requests_per_connection_{}; uint32_t max_response_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; From 96a71c412b9720626c2fa78fc48e0dfe6d6a7c83 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Mon, 19 Oct 2020 08:39:52 +0000 Subject: [PATCH 13/22] coverage Signed-off-by: Shikugawa --- .../transport_sockets/proxy_protocol/proxy_protocol.cc | 2 +- source/extensions/transport_sockets/tls/ssl_socket.cc | 5 +++++ .../proxy_protocol/proxy_protocol_test.cc | 7 +++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc index ad13bfd4a1b2e..3c06e86875d65 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc @@ -124,7 +124,7 @@ bool UpstreamProxyProtocolSocketFactory::implementsSecureTransport() const { } bool UpstreamProxyProtocolSocketFactory::secureTransportReady() const { - NOT_REACHED_GCOVR_EXCL_LINE; + return transport_socket_factory_->secureTransportReady(); } } // namespace ProxyProtocol diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 9754b32143926..622950ce29b76 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -393,6 +393,11 @@ bool ClientSslSocketFactory::secureTransportReady() const { if (!implementsSecureTransport()) { return false; } + // 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): Consider retry strategy of clusters which failed to activate at once. const auto& tls_certificate_sds_configs = config_->tlsCertificateSdsConfigs(); for (const auto& config : tls_certificate_sds_configs) { if (!secret_manager_.checkTlsCertificateEntityExists(config.sds_config(), config.name())) { 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 953e5999a5fbd..ba990b78b2ee7 100644 --- a/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc @@ -469,6 +469,13 @@ TEST_F(ProxyProtocolSocketFactoryTest, ImplementsSecureTransportCallInnerFactory ASSERT_TRUE(factory_->implementsSecureTransport()); } +// Test SecureTransportReady calls inner factory +TEST_F(ProxyProtocolSocketFactoryTest, SecureTransportReadyCallInnerFactory) { + initialize(); + EXPECT_CALL(*inner_factory_, secureTransportReady()).WillOnce(Return(true)); + ASSERT_TRUE(factory_->secureTransportReady()); +} + } // namespace } // namespace ProxyProtocol } // namespace TransportSockets From 34ea49c6f62cc6cd1b39db25f8e60f932f70ff48 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 22 Oct 2020 11:31:32 +0900 Subject: [PATCH 14/22] coverage Signed-off-by: Shikugawa --- .../transport_sockets/tls/ssl_socket_test.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index a3d1c554868ed..680b394e2abaa 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -5730,6 +5730,16 @@ 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, checkTlsCertificateEntityExists()).WillOnce(Return(true)); + EXPECT_CALL(*client_cfg, checkCertificateValidationContextEntityExists()).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 From b2b99be42ccb8ec284e7ab6af838a5143c033817 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 22 Oct 2020 15:36:41 +0900 Subject: [PATCH 15/22] cleanup and coverage Signed-off-by: Shikugawa --- source/common/upstream/BUILD | 1 - .../common/upstream/cluster_manager_impl.cc | 1 - source/common/upstream/cluster_manager_impl.h | 1 - .../tls/context_impl_test.cc | 56 +++++++++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 1012b29e1a6d0..9219e59d768df 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -63,7 +63,6 @@ envoy_cc_library( "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", "//source/common/router:shadow_writer_lib", - "//source/common/secret:secret_manager_impl_lib", "//source/common/shared_pool:shared_pool_lib", "//source/common/tcp:conn_pool_lib", "//source/common/upstream:priority_conn_pool_map_impl_lib", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index e5e4b46c11c05..a4ed0dd521b5d 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -16,7 +16,6 @@ #include "envoy/network/dns.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" -#include "envoy/upstream/upstream.h" #include "common/common/assert.h" #include "common/common/enum_to_int.h" diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 897125f933728..1aa14c4be78cf 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -28,7 +28,6 @@ #include "common/config/grpc_mux_impl.h" #include "common/config/subscription_factory_impl.h" #include "common/http/async_client_impl.h" -#include "common/secret/secret_manager_impl.h" #include "common/upstream/load_stats_reporter.h" #include "common/upstream/priority_conn_pool_map.h" #include "common/upstream/upstream_impl.h" diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 0307ebb2daef9..2019b2c306d92 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1604,6 +1604,62 @@ 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.checkCertificateValidationContextEntityExists()); + + 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.checkTlsCertificateEntityExists()); + + 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. From 5bbae5fae2f4eb2841da2b517b5e1828f05dfa78 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 22 Oct 2020 17:15:20 +0900 Subject: [PATCH 16/22] Kick CI Signed-off-by: Shikugawa From 1e4f9507be5cccb2f64c0af865cba3454e7448c9 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 22 Oct 2020 18:48:00 +0900 Subject: [PATCH 17/22] follow up Signed-off-by: Shikugawa --- include/envoy/ssl/context_config.h | 9 ++------- .../quiche/quic_transport_socket_factory.h | 2 +- .../transport_sockets/alts/tsi_socket.cc | 2 +- source/extensions/transport_sockets/tap/tap.cc | 2 +- .../transport_sockets/tls/context_config_impl.cc | 14 +++++--------- .../transport_sockets/tls/context_config_impl.h | 11 ++++------- .../extensions/transport_sockets/tls/ssl_socket.cc | 7 ++----- .../grpc/grpc_client_integration_test_harness.h | 1 - .../transport_sockets/tls/context_impl_test.cc | 4 ++-- .../transport_sockets/tls/ssl_socket_test.cc | 3 +-- test/mocks/ssl/mocks.h | 7 +------ 11 files changed, 20 insertions(+), 42 deletions(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 01aa2c29bf136..91bdddf57a366 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -113,14 +113,9 @@ class ClientContextConfig : public virtual ContextConfig { virtual const std::string& signingAlgorithmsForTest() const PURE; /** - * Check whether TLS Certificate entity was being extracted from SDS server. + * Check whether TLS certificate entity and certificate validation context entity is available */ - virtual bool checkTlsCertificateEntityExists() const PURE; - - /** - * Check whether certificate validation context entity was being extracted from SDS server. - */ - virtual bool checkCertificateValidationContextEntityExists() const PURE; + virtual bool isSecretReady() const PURE; }; using ClientContextConfigPtr = std::unique_ptr; 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 586a86fcbacec..b974a36725d18 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,7 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory { NOT_REACHED_GCOVR_EXCL_LINE; } bool implementsSecureTransport() const override { return true; } - bool isReady() const override { NOT_REACHED_GCOVR_EXCL_LINE; } + 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 cbc87eb38a62b..7ba6b2cab798f 100644 --- a/source/extensions/transport_sockets/alts/tsi_socket.cc +++ b/source/extensions/transport_sockets/alts/tsi_socket.cc @@ -261,7 +261,7 @@ TsiSocketFactory::createTransportSocket(Network::TransportSocketOptionsSharedPtr return std::make_unique(handshaker_factory_, handshake_validator_); } -bool TsiSocketFactory::isReady() const { NOT_REACHED_GCOVR_EXCL_LINE; } +bool TsiSocketFactory::isReady() const { return true; } } // namespace Alts } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/tap/tap.cc b/source/extensions/transport_sockets/tap/tap.cc index 1f631ecc81624..5b8d83fc8186a 100644 --- a/source/extensions/transport_sockets/tap/tap.cc +++ b/source/extensions/transport_sockets/tap/tap.cc @@ -66,7 +66,7 @@ bool TapSocketFactory::implementsSecureTransport() const { return transport_socket_factory_->implementsSecureTransport(); } -bool TapSocketFactory::isReady() const { NOT_REACHED_GCOVR_EXCL_LINE; } +bool TapSocketFactory::isReady() const { return transport_socket_factory_->isReady(); } } // namespace Tap } // namespace TransportSockets diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc index 4d5a5ea8dc78d..7e6a59d859193 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.cc +++ b/source/extensions/transport_sockets/tls/context_config_impl.cc @@ -169,6 +169,9 @@ 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)), @@ -177,10 +180,7 @@ ContextConfigImpl::ContextConfigImpl( 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(), - default_max_protocol_version)), - tls_certificate_providers_(getTlsCertificateConfigProviders(config, factory_context)), - certificate_validation_context_provider_( - getCertificateValidationContextConfigProvider(config, factory_context, &default_cvc_)) { + default_max_protocol_version)) { if (certificate_validation_context_provider_ != nullptr) { if (default_cvc_) { // We need to validate combined certificate validation context. @@ -375,16 +375,12 @@ ClientContextConfigImpl::ClientContextConfigImpl( } } -bool ClientContextConfigImpl::checkTlsCertificateEntityExists() const { +bool ClientContextConfigImpl::isSecretReady() const { for (const auto& provider : tls_certificate_providers_) { if (provider->secret() == nullptr) { return false; } } - return true; -} - -bool ClientContextConfigImpl::checkCertificateValidationContextEntityExists() const { return certificate_validation_context_provider_->secret() != nullptr; } diff --git a/source/extensions/transport_sockets/tls/context_config_impl.h b/source/extensions/transport_sockets/tls/context_config_impl.h index 83cfd9861b963..e1857ea2be556 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.h +++ b/source/extensions/transport_sockets/tls/context_config_impl.h @@ -69,6 +69,9 @@ 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_; + std::vector tls_certificate_providers_; + Secret::CertificateValidationContextConfigProviderSharedPtr + certificate_validation_context_provider_; private: static unsigned tlsVersionFromProto( @@ -96,11 +99,6 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { Ssl::HandshakerFactoryCb handshaker_factory_cb_; Ssl::HandshakerCapabilities capabilities_; - -protected: - std::vector tls_certificate_providers_; - Secret::CertificateValidationContextConfigProviderSharedPtr - certificate_validation_context_provider_; }; class ClientContextConfigImpl : public ContextConfigImpl, public Envoy::Ssl::ClientContextConfig { @@ -122,8 +120,7 @@ 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 checkTlsCertificateEntityExists() const override; - bool checkCertificateValidationContextEntityExists() const override; + 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 216175a20b8c6..0bc6dcc400c89 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -362,10 +362,7 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() { stats_.ssl_context_update_by_sds_.inc(); } -bool ClientSslSocketFactory::isReady() const { - return config_->checkTlsCertificateEntityExists() && - config_->checkCertificateValidationContextEntityExists(); -} +bool ClientSslSocketFactory::isReady() const { return config_->isSecretReady(); } ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config, Envoy::Ssl::ContextManager& manager, @@ -408,7 +405,7 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() { stats_.ssl_context_update_by_sds_.inc(); } -bool ServerSslSocketFactory::isReady() const { NOT_REACHED_GCOVR_EXCL_LINE; } +bool ServerSslSocketFactory::isReady() const { return true; } } // namespace Tls } // namespace TransportSockets diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index c5180d015d5cc..d610a221f8b74 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -478,7 +478,6 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest { Router::MockShadowWriter* mock_shadow_writer_ = new Router::MockShadowWriter(); Router::ShadowWriterPtr shadow_writer_ptr_{mock_shadow_writer_}; Network::ClientConnectionPtr client_connection_; - Secret::MockSecretManager secret_manager_; }; // SSL connection credential validation tests. diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 2019b2c306d92..3b361d88beace 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1623,7 +1623,7 @@ TEST_F(ClientContextConfigImplTest, ValidationContextEntityNotExist) { EXPECT_CALL(factory_context_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - EXPECT_FALSE(client_context_config.checkCertificateValidationContextEntityExists()); + EXPECT_FALSE(client_context_config.isSecretReady()); NiceMock secret_callback; client_context_config.setSecretUpdateCallback( @@ -1652,7 +1652,7 @@ TEST_F(ClientContextConfigImplTest, TlsCertificateEntityNotExist) { EXPECT_CALL(factory_context_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher)); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - EXPECT_FALSE(client_context_config.checkTlsCertificateEntityExists()); + EXPECT_FALSE(client_context_config.isSecretReady()); NiceMock secret_callback; client_context_config.setSecretUpdateCallback( diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 680b394e2abaa..c0874d16019f1 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -5734,8 +5734,7 @@ TEST_P(SslSocketTest, ClientSocketFactoryIsReadyTest) { ContextManagerImpl manager(time_system_); Stats::TestUtil::TestStore stats_store; auto client_cfg = std::make_unique>(); - EXPECT_CALL(*client_cfg, checkTlsCertificateEntityExists()).WillOnce(Return(true)); - EXPECT_CALL(*client_cfg, checkCertificateValidationContextEntityExists()).WillOnce(Return(true)); + 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()); } diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 3720bd368b929..31da44badabd8 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -95,8 +95,7 @@ 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, checkTlsCertificateEntityExists, (), (const)); - MOCK_METHOD(bool, checkCertificateValidationContextEntityExists, (), (const)); + MOCK_METHOD(bool, isSecretReady, (), (const)); }; class MockServerContextConfig : public ServerContextConfig { @@ -110,10 +109,6 @@ class MockServerContextConfig : public ServerContextConfig { MOCK_METHOD(std::vector>, tlsCertificates, (), (const)); MOCK_METHOD(const CertificateValidationContextConfig*, certificateValidationContext, (), (const)); - MOCK_METHOD(const std::vector&, - tlsCertificateSdsConfigs, (), (const)); - MOCK_METHOD(const envoy::extensions::transport_sockets::tls::v3::SdsSecretConfig&, - validationContextSdsConfig, (), (const)); MOCK_METHOD(unsigned, minProtocolVersion, (), (const)); MOCK_METHOD(unsigned, maxProtocolVersion, (), (const)); MOCK_METHOD(bool, isReady, (), (const)); From 31354e4886c0fe59fded262658b84f735fd9d0c4 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Thu, 22 Oct 2020 20:51:25 +0900 Subject: [PATCH 18/22] ci Signed-off-by: Shikugawa --- source/extensions/transport_sockets/tls/ssl_socket.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index d522bd78d2030..82834b133d602 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -6,7 +6,6 @@ #include "envoy/network/connection.h" #include "envoy/network/transport_socket.h" #include "envoy/secret/secret_callbacks.h" -#include "envoy/secret/secret_manager.h" #include "envoy/ssl/handshaker.h" #include "envoy/ssl/private_key/private_key_callbacks.h" #include "envoy/ssl/ssl_socket_extended_info.h" From 0ad27733d808e17699031bd39f660771108b2b1a Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Fri, 23 Oct 2020 02:13:24 +0900 Subject: [PATCH 19/22] reorder Signed-off-by: Shikugawa --- .../transport_sockets/tls/context_config_impl.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_config_impl.h b/source/extensions/transport_sockets/tls/context_config_impl.h index e1857ea2be556..d40bb627a51de 100644 --- a/source/extensions/transport_sockets/tls/context_config_impl.h +++ b/source/extensions/transport_sockets/tls/context_config_impl.h @@ -69,6 +69,11 @@ 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_; @@ -84,11 +89,6 @@ 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_; // Handle for TLS certificate dynamic secret callback. Envoy::Common::CallbackHandle* tc_update_callback_handle_{}; // Handle for certificate validation context dynamic secret callback. From 186c3051fe07d00e9a451e7bd37a2941db385de6 Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Mon, 26 Oct 2020 11:57:23 +0900 Subject: [PATCH 20/22] add runtime feature flag Signed-off-by: Shikugawa --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 3 ++- source/common/upstream/cluster_manager_impl.cc | 8 ++++++-- test/common/upstream/BUILD | 1 + test/common/upstream/cluster_manager_impl_test.cc | 7 +++++++ 5 files changed, 17 insertions(+), 3 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a7405c8f72a90..79eb01ec1f0f7 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -10,6 +10,7 @@ Minor Behavior Changes *Changes that may cause incompatibilities for some users, but should not for most* * build: the Alpine based debug images are no longer built in CI, use Ubuntu based images instead. +* cluster manager: the cluster which can't extract secret entity by SDS to be warming and never activate. This feature is disabled by default and is controlled by runtime guard `envoy.reloadable_features.cluser_keep_warming_no_secret_entity`. * ext_authz filter: disable `envoy.reloadable_features.ext_authz_measure_timeout_on_check_created` by default. * ext_authz filter: the deprecated field :ref:`use_alpha ` is no longer supported and cannot be set anymore. * grpc_web filter: if a `grpc-accept-encoding` header is present it's passed as-is to the upstream and if it isn't `grpc-accept-encoding:identity` is sent instead. The header was always overwriten with `grpc-accept-encoding:identity,deflate,gzip` before. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index dea20addee385..3e7c45af91d87 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -109,7 +109,8 @@ constexpr const char* disabled_runtime_features[] = { "envoy.reloadable_features.test_feature_false", // gRPC Timeout header is missing (#13580) "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", - + // The cluster which can't extract secret entity by SDS to be warming and never activate. + "envoy.reloadable_features.cluser_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 a4ed0dd521b5d..e52ff79eda83e 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -428,8 +428,12 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // 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): Consider retry strategy of clusters which failed to activate at once. - if (!factory.isReady()) { + // TODO(shikugawa): To implement to be available by keeping warming after no-available secret + // entity behavior occured. And remove + // `envoy.reloadable_features.cluser_keep_warming_no_secret_entity` runtime feature flag. + auto keep_warming_enabled = Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.cluser_keep_warming_no_secret_entity"); + if (!factory.isReady() && keep_warming_enabled) { ENVOY_LOG(warn, "Failed to activate {}", cluster.info()->name()); return; } diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index 75dc2f42f7585..e1561389bacb7 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -46,6 +46,7 @@ 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", diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index b8bd4b4b97a55..9736f71cc741b 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -13,6 +13,7 @@ #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 { @@ -2309,6 +2310,9 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutCertificateValidationContextEntity) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.cluser_keep_warming_no_secret_entity", "true"}}); create(defaultConfig()); ReadyWatcher initialized; @@ -2344,6 +2348,9 @@ TEST_F(ClusterManagerImplTest, } TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutTlsCertificateEntity) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.cluser_keep_warming_no_secret_entity", "true"}}); create(defaultConfig()); ReadyWatcher initialized; From 5f83864f7f3fd50e73f2e21a043c5ad03da9950a Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Mon, 26 Oct 2020 11:58:05 +0900 Subject: [PATCH 21/22] typo Signed-off-by: Shikugawa --- source/common/upstream/cluster_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index e52ff79eda83e..afe80e18dc10a 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -429,7 +429,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // 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 occured. And remove + // entity behavior occurred. And remove // `envoy.reloadable_features.cluser_keep_warming_no_secret_entity` runtime feature flag. auto keep_warming_enabled = Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.cluser_keep_warming_no_secret_entity"); From 1cdfbe625b80c265558e8d2696f20fab38cd827f Mon Sep 17 00:00:00 2001 From: Shikugawa Date: Mon, 26 Oct 2020 19:58:40 +0900 Subject: [PATCH 22/22] follow up Signed-off-by: Shikugawa --- docs/root/version_history/current.rst | 2 +- source/common/runtime/runtime_features.cc | 2 +- .../common/upstream/cluster_manager_impl.cc | 6 +- .../upstream/cluster_manager_impl_test.cc | 75 ++++++++++++++++++- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 79eb01ec1f0f7..dd4ed11789268 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -10,7 +10,7 @@ Minor Behavior Changes *Changes that may cause incompatibilities for some users, but should not for most* * build: the Alpine based debug images are no longer built in CI, use Ubuntu based images instead. -* cluster manager: the cluster which can't extract secret entity by SDS to be warming and never activate. This feature is disabled by default and is controlled by runtime guard `envoy.reloadable_features.cluser_keep_warming_no_secret_entity`. +* cluster manager: the cluster which can't extract secret entity by SDS to be warming and never activate. This feature is disabled by default and is controlled by runtime guard `envoy.reloadable_features.cluster_keep_warming_no_secret_entity`. * ext_authz filter: disable `envoy.reloadable_features.ext_authz_measure_timeout_on_check_created` by default. * ext_authz filter: the deprecated field :ref:`use_alpha ` is no longer supported and cannot be set anymore. * grpc_web filter: if a `grpc-accept-encoding` header is present it's passed as-is to the upstream and if it isn't `grpc-accept-encoding:identity` is sent instead. The header was always overwriten with `grpc-accept-encoding:identity,deflate,gzip` before. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 3e7c45af91d87..e1bd6f608a955 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -110,7 +110,7 @@ constexpr const char* disabled_runtime_features[] = { // gRPC Timeout header is missing (#13580) "envoy.reloadable_features.ext_authz_measure_timeout_on_check_created", // The cluster which can't extract secret entity by SDS to be warming and never activate. - "envoy.reloadable_features.cluser_keep_warming_no_secret_entity", + "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 afe80e18dc10a..924c0a6feb07b 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -430,9 +430,9 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) { // 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.cluser_keep_warming_no_secret_entity` runtime feature flag. - auto keep_warming_enabled = Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.cluser_keep_warming_no_secret_entity"); + // `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; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 9736f71cc741b..34d8403cf5d0b 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -2312,7 +2312,7 @@ TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutCertificateValidationContextEntity) { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.cluser_keep_warming_no_secret_entity", "true"}}); + {{"envoy.reloadable_features.cluster_keep_warming_no_secret_entity", "true"}}); create(defaultConfig()); ReadyWatcher initialized; @@ -2347,10 +2347,46 @@ TEST_F(ClusterManagerImplTest, 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.cluser_keep_warming_no_secret_entity", "true"}}); + {{"envoy.reloadable_features.cluster_keep_warming_no_secret_entity", "true"}}); create(defaultConfig()); ReadyWatcher initialized; @@ -2385,6 +2421,41 @@ TEST_F(ClusterManagerImplTest, DynamicAddedAndKeepWarmingWithoutTlsCertificateEn 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(); }