sds: keep warming when dynamic inserted cluster can't be extracted se…

…cret entity (#13344) This PR highly depends on #12783. Changed to keep warming if dynamic inserted clusters (when initialize doesn't finished) failed to extract TLS certificate and certificate validation context. They shouldn't be indicated as ACTIVE cluster. Risk Level: Mid Testing: Unit Docs Changes: Release Notes: Added Fixes #11120, future work: #13777 Signed-off-by: Shikugawa <rei@tetrate.io>
envoyproxy · Oct 27, 2020 · e5a8c21 · e5a8c21
1 parent 40de149
commit e5a8c21
Show file tree

Hide file tree

Showing 27 changed files with 302 additions and 17 deletions.
diff --git 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.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 <envoy_api_field_config.filter.http.ext_authz.v2.ExtAuthz.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/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h
@@ -226,6 +226,11 @@ 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,6 +111,11 @@ 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,5 +93,7 @@ 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,6 +32,7 @@ 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,7 +111,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.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
@@ -422,6 +422,21 @@ 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()) {
+    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();
   }

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,6 +24,7 @@ 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,6 +261,7 @@ 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,6 +100,7 @@ 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,7 +123,11 @@ 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,6 +49,7 @@ 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,6 +66,8 @@ 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,6 +41,7 @@ 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,6 +375,15 @@ 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,6 +69,14 @@ 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(
@@ -81,16 +89,8 @@ 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,6 +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 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,6 +362,8 @@ 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,
@@ -403,6 +405,8 @@ 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,10 +105,11 @@ 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;
 
@@ -132,7 +133,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,10 +46,12 @@ 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",
     ],
 )