istio · istio-testing · Nov 6, 2020 · Nov 5, 2020
diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h
@@ -226,11 +226,6 @@ class TransportSocketFactory {
    */
   virtual TransportSocketPtr
   createTransportSocket(TransportSocketOptionsSharedPtr options) const PURE;
-
-  /**
-   * Check whether matched transport socket which required to use secret information is available.
-   */
-  virtual bool isReady() const PURE;
 };
 
 using TransportSocketFactoryPtr = std::unique_ptr<TransportSocketFactory>;

diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h
@@ -111,11 +111,6 @@ class ClientContextConfig : public virtual ContextConfig {
    *         for names.
    */
   virtual const std::string& signingAlgorithmsForTest() const PURE;
-
-  /**
-   * Check whether TLS certificate entity and certificate validation context entity is available
-   */
-  virtual bool isSecretReady() const PURE;
 };
 
 using ClientContextConfigPtr = std::unique_ptr<ClientContextConfig>;

diff --git a/source/common/network/raw_buffer_socket.cc b/source/common/network/raw_buffer_socket.cc
@@ -93,7 +93,5 @@ RawBufferSocketFactory::createTransportSocket(TransportSocketOptionsSharedPtr) c
 }
 
 bool RawBufferSocketFactory::implementsSecureTransport() const { return false; }
-
-bool RawBufferSocketFactory::isReady() const { return true; }
 } // namespace Network
 } // namespace Envoy
diff --git a/source/common/network/raw_buffer_socket.h b/source/common/network/raw_buffer_socket.h
@@ -32,7 +32,6 @@ class RawBufferSocketFactory : public TransportSocketFactory {
   // Network::TransportSocketFactory
   TransportSocketPtr createTransportSocket(TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
-  bool isReady() const override;
 };
 
 } // namespace Network

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -111,8 +111,6 @@ constexpr const char* disabled_runtime_features[] = {
     "envoy.reloadable_features.new_tcp_connection_pool",
     // Sentinel and test flag.
     "envoy.reloadable_features.test_feature_false",
-    // The cluster which can't extract secret entity by SDS to be warming and never activate.
-    "envoy.reloadable_features.cluster_keep_warming_no_secret_entity",
 };
 
 RuntimeFeatures::RuntimeFeatures() {

diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc
@@ -417,31 +417,7 @@ void ClusterManagerImpl::onClusterInit(Cluster& cluster) {
   // been setup for cross-thread updates to avoid needless updates during initialization. The order
   // of operations here is important. We start by initializing the thread aware load balancer if
   // needed. This must happen first so cluster updates are heard first by the load balancer.
-  // Also, it assures that all of clusters which this function is called should be always active.
-  auto cluster_data = warming_clusters_.find(cluster.info()->name());
-  // We have a situation that clusters will be immediately active, such as static and primary
-  // cluster. So we must have this prevention logic here.
-  if (cluster_data != warming_clusters_.end()) {
-    Network::TransportSocketFactory& factory =
-        cluster.info()->transportSocketMatcher().resolve(&cluster.info()->metadata()).factory_;
-    // If there is no secret entity, currently supports only TLS Certificate and Validation
-    // Context, when it failed to extract them via SDS, it will fail to change cluster status from
-    // warming to active. In current implementation, there is no strategy to activate clusters
-    // which failed to initialize at once.
-    // TODO(shikugawa): To implement to be available by keeping warming after no-available secret
-    // entity behavior occurred. And remove
-    // `envoy.reloadable_features.cluster_keep_warming_no_secret_entity` runtime feature flag.
-    const bool keep_warming_enabled = Runtime::runtimeFeatureEnabled(
-        "envoy.reloadable_features.cluster_keep_warming_no_secret_entity");
-    if (!factory.isReady() && keep_warming_enabled) {
-      ENVOY_LOG(warn, "Failed to activate {}", cluster.info()->name());
-      return;
-    }
-    clusterWarmingToActive(cluster.info()->name());
-    updateClusterCounts();
-  }
-  cluster_data = active_clusters_.find(cluster.info()->name());
-
+  auto cluster_data = active_clusters_.find(cluster.info()->name());
   if (cluster_data->second->thread_aware_lb_ != nullptr) {
     cluster_data->second->thread_aware_lb_->initialize();
   }
@@ -611,6 +587,17 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl
       // The following init manager remove call is a NOP in the case we are already initialized.
       // It's just kept here to avoid additional logic.
       init_helper_.removeCluster(*existing_active_cluster->second->cluster_);
+    } else {
+      // Validate that warming clusters are not added to the init_helper_.
+      // NOTE: This loop is compiled out in optimized builds.
+      for (const std::list<Cluster*>& cluster_list :
+           {std::cref(init_helper_.primary_init_clusters_),
+            std::cref(init_helper_.secondary_init_clusters_)}) {
+        ASSERT(!std::any_of(cluster_list.begin(), cluster_list.end(),
+                            [&existing_warming_cluster](Cluster* cluster) {
+                              return existing_warming_cluster->second->cluster_.get() == cluster;
+                            }));
+      }
     }
     cm_stats_.cluster_modified_.inc();
   } else {
@@ -627,39 +614,40 @@ bool ClusterManagerImpl::addOrUpdateCluster(const envoy::config::cluster::v3::Cl
   //       the future we may decide to undergo a refactor to unify the logic but the effort/risk to
   //       do that right now does not seem worth it given that the logic is generally pretty clean
   //       and easy to understand.
-  const bool all_clusters_initialized =
-      init_helper_.state() == ClusterManagerInitHelper::State::AllClustersInitialized;
-  loadCluster(cluster, version_info, true, warming_clusters_);
-  auto& cluster_entry = warming_clusters_.at(cluster_name);
-  if (!all_clusters_initialized) {
+  const bool use_active_map =
+      init_helper_.state() != ClusterManagerInitHelper::State::AllClustersInitialized;
+  loadCluster(cluster, version_info, true, use_active_map ? active_clusters_ : warming_clusters_);
+
+  if (use_active_map) {
     ENVOY_LOG(debug, "add/update cluster {} during init", cluster_name);
+    auto& cluster_entry = active_clusters_.at(cluster_name);
     createOrUpdateThreadLocalCluster(*cluster_entry);
     init_helper_.addCluster(*cluster_entry->cluster_);
   } else {
+    auto& cluster_entry = warming_clusters_.at(cluster_name);
     ENVOY_LOG(debug, "add/update cluster {} starting warming", cluster_name);
     cluster_entry->cluster_->initialize([this, cluster_name] {
+      auto warming_it = warming_clusters_.find(cluster_name);
+      auto& cluster_entry = *warming_it->second;
+
+      // If the cluster is being updated, we need to cancel any pending merged updates.
+      // Otherwise, applyUpdates() will fire with a dangling cluster reference.
+      updates_map_.erase(cluster_name);
+
+      active_clusters_[cluster_name] = std::move(warming_it->second);
+      warming_clusters_.erase(warming_it);
+
       ENVOY_LOG(debug, "warming cluster {} complete", cluster_name);
-      auto state_changed_cluster_entry = warming_clusters_.find(cluster_name);
-      createOrUpdateThreadLocalCluster(*state_changed_cluster_entry->second);
-      onClusterInit(*state_changed_cluster_entry->second->cluster_);
+      createOrUpdateThreadLocalCluster(cluster_entry);
+      onClusterInit(*cluster_entry.cluster_);
+      updateClusterCounts();
     });
   }
 
+  updateClusterCounts();
   return true;
 }
 
-void ClusterManagerImpl::clusterWarmingToActive(const std::string& cluster_name) {
-  auto warming_it = warming_clusters_.find(cluster_name);
-  ASSERT(warming_it != warming_clusters_.end());
-
-  // If the cluster is being updated, we need to cancel any pending merged updates.
-  // Otherwise, applyUpdates() will fire with a dangling cluster reference.
-  updates_map_.erase(cluster_name);
-
-  active_clusters_[cluster_name] = std::move(warming_it->second);
-  warming_clusters_.erase(warming_it);
-}
-
 void ClusterManagerImpl::createOrUpdateThreadLocalCluster(ClusterData& cluster) {
   tls_->runOnAllThreads([new_cluster = cluster.cluster_->info(),
                          thread_aware_lb_factory = cluster.loadBalancerFactory()](
@@ -714,7 +702,6 @@ bool ClusterManagerImpl::removeCluster(const std::string& cluster_name) {
   if (existing_warming_cluster != warming_clusters_.end() &&
       existing_warming_cluster->second->added_via_api_) {
     removed = true;
-    init_helper_.removeCluster(*existing_warming_cluster->second->cluster_);
     warming_clusters_.erase(existing_warming_cluster);
     ENVOY_LOG(info, "removing warming cluster {}", cluster_name);
   }
@@ -817,9 +804,7 @@ void ClusterManagerImpl::updateClusterCounts() {
   // Once cluster is warmed up, CDS is resumed, and ACK is sent to ADS, providing a
   // signal to ADS to proceed with RDS updates.
   // If we're in the middle of shutting down (ads_mux_ already gone) then this is irrelevant.
-  const bool all_clusters_initialized =
-      init_helper_.state() == ClusterManagerInitHelper::State::AllClustersInitialized;
-  if (all_clusters_initialized && ads_mux_) {
+  if (ads_mux_) {
     const auto type_urls = Config::getAllVersionTypeUrls<envoy::config::cluster::v3::Cluster>();
     const uint64_t previous_warming = cm_stats_.warming_clusters_.value();
     if (previous_warming == 0 && !warming_clusters_.empty()) {

diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h
@@ -482,7 +482,6 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u
   void onClusterInit(Cluster& cluster);
   void postThreadLocalHealthFailure(const HostSharedPtr& host);
   void updateClusterCounts();
-  void clusterWarmingToActive(const std::string& cluster_name);
   void maybePrefetch(ThreadLocalClusterManagerImpl::ClusterEntryPtr& cluster_entry,
                      std::function<ConnectionPool::Instance*()> prefetch_pool);
 

diff --git a/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h b/source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h
@@ -24,7 +24,6 @@ class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory {
     NOT_REACHED_GCOVR_EXCL_LINE;
   }
   bool implementsSecureTransport() const override { return true; }
-  bool isReady() const override { return true; }
 };
 
 // TODO(danzh): when implement ProofSource, examine of it's necessary to

diff --git a/source/extensions/transport_sockets/alts/tsi_socket.cc b/source/extensions/transport_sockets/alts/tsi_socket.cc
@@ -261,7 +261,6 @@ TsiSocketFactory::createTransportSocket(Network::TransportSocketOptionsSharedPtr
   return std::make_unique<TsiSocket>(handshaker_factory_, handshake_validator_);
 }
 
-bool TsiSocketFactory::isReady() const { return true; }
 } // namespace Alts
 } // namespace TransportSockets
 } // namespace Extensions

diff --git a/source/extensions/transport_sockets/alts/tsi_socket.h b/source/extensions/transport_sockets/alts/tsi_socket.h
@@ -100,7 +100,6 @@ class TsiSocketFactory : public Network::TransportSocketFactory {
   bool implementsSecureTransport() const override;
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
-  bool isReady() const override;
 
 private:
   HandshakerFactory handshaker_factory_;

diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc
@@ -123,11 +123,7 @@ bool UpstreamProxyProtocolSocketFactory::implementsSecureTransport() const {
   return transport_socket_factory_->implementsSecureTransport();
 }
 
-bool UpstreamProxyProtocolSocketFactory::isReady() const {
-  return transport_socket_factory_->isReady();
-}
-
 } // namespace ProxyProtocol
 } // namespace TransportSockets
 } // namespace Extensions
-} // namespace Envoy
+} // namespace Envoy
diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h
@@ -49,7 +49,6 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
-  bool isReady() const override;
 
 private:
   Network::TransportSocketFactoryPtr transport_socket_factory_;

diff --git a/source/extensions/transport_sockets/tap/tap.cc b/source/extensions/transport_sockets/tap/tap.cc
@@ -66,8 +66,6 @@ bool TapSocketFactory::implementsSecureTransport() const {
   return transport_socket_factory_->implementsSecureTransport();
 }
 
-bool TapSocketFactory::isReady() const { return transport_socket_factory_->isReady(); }
-
 } // namespace Tap
 } // namespace TransportSockets
 } // namespace Extensions

diff --git a/source/extensions/transport_sockets/tap/tap.h b/source/extensions/transport_sockets/tap/tap.h
@@ -41,7 +41,6 @@ class TapSocketFactory : public Network::TransportSocketFactory,
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
-  bool isReady() const override;
 
 private:
   Network::TransportSocketFactoryPtr transport_socket_factory_;

diff --git a/source/extensions/transport_sockets/tls/context_config_impl.cc b/source/extensions/transport_sockets/tls/context_config_impl.cc
@@ -169,14 +169,14 @@ ContextConfigImpl::ContextConfigImpl(
     const std::string& default_cipher_suites, const std::string& default_curves,
     Server::Configuration::TransportSocketFactoryContext& factory_context)
     : api_(factory_context.api()),
-      tls_certificate_providers_(getTlsCertificateConfigProviders(config, factory_context)),
-      certificate_validation_context_provider_(
-          getCertificateValidationContextConfigProvider(config, factory_context, &default_cvc_)),
       alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")),
       cipher_suites_(StringUtil::nonEmptyStringOrDefault(
           RepeatedPtrUtil::join(config.tls_params().cipher_suites(), ":"), default_cipher_suites)),
       ecdh_curves_(StringUtil::nonEmptyStringOrDefault(
           RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), default_curves)),
+      tls_certificate_providers_(getTlsCertificateConfigProviders(config, factory_context)),
+      certificate_validation_context_provider_(
+          getCertificateValidationContextConfigProvider(config, factory_context, &default_cvc_)),
       min_protocol_version_(tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(),
                                                 default_min_protocol_version)),
       max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(),
@@ -375,15 +375,6 @@ ClientContextConfigImpl::ClientContextConfigImpl(
   }
 }
 
-bool ClientContextConfigImpl::isSecretReady() const {
-  for (const auto& provider : tls_certificate_providers_) {
-    if (provider->secret() == nullptr) {
-      return false;
-    }
-  }
-  return certificate_validation_context_provider_->secret() != nullptr;
-}
-
 const unsigned ServerContextConfigImpl::DEFAULT_MIN_VERSION = TLS1_VERSION;
 const unsigned ServerContextConfigImpl::DEFAULT_MAX_VERSION = TLS1_3_VERSION;
 

diff --git a/source/extensions/transport_sockets/tls/context_config_impl.h b/source/extensions/transport_sockets/tls/context_config_impl.h
@@ -69,14 +69,6 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {
                     const std::string& default_cipher_suites, const std::string& default_curves,
                     Server::Configuration::TransportSocketFactoryContext& factory_context);
   Api::Api& api_;
-  // If certificate validation context type is combined_validation_context. default_cvc_
-  // holds a copy of CombinedCertificateValidationContext::default_validation_context.
-  // Otherwise, default_cvc_ is nullptr.
-  std::unique_ptr<envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>
-      default_cvc_;
-  std::vector<Secret::TlsCertificateConfigProviderSharedPtr> tls_certificate_providers_;
-  Secret::CertificateValidationContextConfigProviderSharedPtr
-      certificate_validation_context_provider_;
 
 private:
   static unsigned tlsVersionFromProto(
@@ -89,8 +81,16 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig {
 
   std::vector<Ssl::TlsCertificateConfigImpl> 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<envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>
+      default_cvc_;
+  std::vector<Secret::TlsCertificateConfigProviderSharedPtr> tls_certificate_providers_;
   // Handle for TLS certificate dynamic secret callback.
   Envoy::Common::CallbackHandle* tc_update_callback_handle_{};
+  Secret::CertificateValidationContextConfigProviderSharedPtr
+      certificate_validation_context_provider_;
   // Handle for certificate validation context dynamic secret callback.
   Envoy::Common::CallbackHandle* cvc_update_callback_handle_{};
   Envoy::Common::CallbackHandle* cvc_validation_callback_handle_{};
@@ -120,7 +120,6 @@ class ClientContextConfigImpl : public ContextConfigImpl, public Envoy::Ssl::Cli
   bool allowRenegotiation() const override { return allow_renegotiation_; }
   size_t maxSessionKeys() const override { return max_session_keys_; }
   const std::string& signingAlgorithmsForTest() const override { return sigalgs_; }
-  bool isSecretReady() const override;
 
 private:
   static const unsigned DEFAULT_MIN_VERSION;

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc
@@ -362,8 +362,6 @@ void ClientSslSocketFactory::onAddOrUpdateSecret() {
   stats_.ssl_context_update_by_sds_.inc();
 }
 
-bool ClientSslSocketFactory::isReady() const { return config_->isSecretReady(); }
-
 ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPtr config,
                                                Envoy::Ssl::ContextManager& manager,
                                                Stats::Scope& stats_scope,
@@ -405,8 +403,6 @@ void ServerSslSocketFactory::onAddOrUpdateSecret() {
   stats_.ssl_context_update_by_sds_.inc();
 }
 
-bool ServerSslSocketFactory::isReady() const { return true; }
-
 } // namespace Tls
 } // namespace TransportSockets
 } // namespace Extensions

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h
@@ -105,11 +105,10 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config,
                          Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope);
 
-  // Network::TransportSocketFactory
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
-  bool isReady() const override;
+
   // Secret::SecretCallbacks
   void onAddOrUpdateSecret() override;
 
@@ -133,7 +132,7 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
   Network::TransportSocketPtr
   createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override;
   bool implementsSecureTransport() const override;
-  bool isReady() const override;
+
   // Secret::SecretCallbacks
   void onAddOrUpdateSecret() override;
 

diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD
@@ -46,12 +46,10 @@ envoy_cc_test(
         "//test/mocks/upstream:health_checker_mocks",
         "//test/mocks/upstream:load_balancer_context_mock",
         "//test/mocks/upstream:thread_aware_load_balancer_mocks",
-        "//test/test_common:test_runtime_lib",
         "@envoy_api//envoy/admin/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
-        "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto",
     ],
 )