sds: allow multiple init managers share sds target (#14357)

Fixes #11120, allows more than one init manager to watch the same SDS init target so clusters/listeners won't be marked active immediately.

Additional Description:
Risk Level: Medium
Testing: integration test
Docs Changes: N/A
Release Notes: Added.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

docs/root/version_history/current.rst

@@ -43,6 +43,7 @@ Bug Fixes
 * http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests.
 * proxy_proto: fixed a bug where the wrong downstream address got sent to upstream connections.
 * proxy_proto: fixed a bug where network filters would not have the correct downstreamRemoteAddress() when accessed from the StreamInfo. This could result in incorrect enforcement of RBAC rules in the RBAC network filter (but not in the RBAC HTTP filter), or incorrect access log addresses from tcp_proxy.
+* sds: fix a bug that clusters sharing same sds target are marked active immediately.
 * tls: fix detection of the upstream connection close event.
 * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers.
 * udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash.

source/common/secret/sds_api.cc

@@ -34,11 +34,6 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi
   // This has to happen here (rather than in initialize()) as it can throw exceptions.
   subscription_ = subscription_factory_.subscriptionFromConfigSource(
       sds_config_, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_);
-
-  // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager
-  // can be chained together to behave as one init_manager. In that way, we let
-  // two listeners which share same SdsApi to register at separate init managers, and
-  // each init manager has a chance to initialize its targets.
 }
 
 void SdsApi::resolveDataSource(const FileContentMap& files,

source/common/secret/sds_api.h

@@ -138,13 +138,11 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider
     // We need to do this early as we invoke the subscription factory during initialization, which
     // is too late to throw.
     Config::Utility::checkLocalInfo("TlsCertificateSdsApi", secret_provider_context.localInfo());
-    auto ret = std::make_shared<TlsCertificateSdsApi>(
+    return std::make_shared<TlsCertificateSdsApi>(
         sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(),
         secret_provider_context.dispatcher().timeSource(),
         secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(),
         destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api());
-    secret_provider_context.initManager().add(*ret->initTarget());
-    return ret;
   }
 
   TlsCertificateSdsApi(const envoy::config::core::v3::ConfigSource& sds_config,
@@ -223,13 +221,11 @@ class CertificateValidationContextSdsApi : public SdsApi,
     // is too late to throw.
     Config::Utility::checkLocalInfo("CertificateValidationContextSdsApi",
                                     secret_provider_context.localInfo());
-    auto ret = std::make_shared<CertificateValidationContextSdsApi>(
+    return std::make_shared<CertificateValidationContextSdsApi>(
         sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(),
         secret_provider_context.dispatcher().timeSource(),
         secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(),
         destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api());
-    secret_provider_context.initManager().add(*ret->initTarget());
-    return ret;
   }
   CertificateValidationContextSdsApi(const envoy::config::core::v3::ConfigSource& sds_config,
                                      const std::string& sds_config_name,
@@ -318,13 +314,11 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon
     // is too late to throw.
     Config::Utility::checkLocalInfo("TlsSessionTicketKeysSdsApi",
                                     secret_provider_context.localInfo());
-    auto ret = std::make_shared<TlsSessionTicketKeysSdsApi>(
+    return std::make_shared<TlsSessionTicketKeysSdsApi>(
         sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(),
         secret_provider_context.dispatcher().timeSource(),
         secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(),
         destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api());
-    secret_provider_context.initManager().add(*ret->initTarget());
-    return ret;
   }
 
   TlsSessionTicketKeysSdsApi(const envoy::config::core::v3::ConfigSource& sds_config,
@@ -391,13 +385,11 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider {
     // We need to do this early as we invoke the subscription factory during initialization, which
     // is too late to throw.
     Config::Utility::checkLocalInfo("GenericSecretSdsApi", secret_provider_context.localInfo());
-    auto ret = std::make_shared<GenericSecretSdsApi>(
+    return std::make_shared<GenericSecretSdsApi>(
         sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(),
         secret_provider_context.dispatcher().timeSource(),
         secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(),
         destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api());
-    secret_provider_context.initManager().add(*ret->initTarget());
-    return ret;
   }
 
   GenericSecretSdsApi(const envoy::config::core::v3::ConfigSource& sds_config,

source/common/secret/secret_manager_impl.h

@@ -92,6 +92,10 @@ class SecretManagerImpl : public SecretManager {
                                              config_name, unregister_secret_provider);
         dynamic_secret_providers_[map_key] = secret_provider;
       }
+      // It is important to add the init target to the manager regardless the secret provider is new
+      // or existing. Different clusters / listeners can share same secret so they have to be marked
+      // warming correctly.
+      secret_provider_context.initManager().add(*secret_provider->initTarget());
       return secret_provider;
     }
 

test/integration/BUILD

@@ -63,6 +63,7 @@ envoy_cc_test(
         "//test/test_common:utility_lib",
         "@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/config/endpoint/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/listener/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/route/v3:pkg_cc_proto",

test/integration/ads_integration_test.cc

@@ -1,5 +1,6 @@
 #include "envoy/config/bootstrap/v3/bootstrap.pb.h"
 #include "envoy/config/cluster/v3/cluster.pb.h"
+#include "envoy/config/core/v3/base.pb.h"
 #include "envoy/config/endpoint/v3/endpoint.pb.h"
 #include "envoy/config/listener/v3/listener.pb.h"
 #include "envoy/config/route/v3/route.pb.h"
@@ -168,6 +169,63 @@ TEST_P(AdsIntegrationTest, ClusterInitializationUpdateOneOfThe2Warming) {
   test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 0);
   test_server_->waitForGaugeGe("cluster_manager.active_clusters", 4);
 }
+
+// Make sure two clusters sharing same secret are both kept warming before secret
+// arrives. Verify that the clusters are eventually initialized.
+// This is a regression test of #11120.
+TEST_P(AdsIntegrationTest, ClusterSharingSecretWarming) {
+  initialize();
+  const auto cds_type_url = Config::getTypeUrl<envoy::config::cluster::v3::Cluster>(
+      envoy::config::core::v3::ApiVersion::V3);
+  const auto sds_type_url =
+      Config::getTypeUrl<envoy::extensions::transport_sockets::tls::v3::Secret>(
+          envoy::config::core::v3::ApiVersion::V3);
+
+  envoy::config::core::v3::TransportSocket sds_transport_socket;
+  TestUtility::loadFromYaml(R"EOF(
+      name: envoy.transport_sockets.tls
+      typed_config:
+        "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext
+        common_tls_context:
+          validation_context_sds_secret_config:
+            name: validation_context
+            sds_config:
+              resource_api_version: V3
+              ads: {}
+  )EOF",
+                            sds_transport_socket);
+  auto cluster_template = ConfigHelper::buildStaticCluster("cluster", 8000, "127.0.0.1");
+  *cluster_template.mutable_transport_socket() = sds_transport_socket;
+
+  auto cluster_0 = cluster_template;
+  cluster_0.set_name("cluster_0");
+  auto cluster_1 = cluster_template;
+  cluster_1.set_name("cluster_1");
+
+  EXPECT_TRUE(compareDiscoveryRequest(cds_type_url, "", {}, {}, {}, true));
+  sendDiscoveryResponse<envoy::config::cluster::v3::Cluster>(
+      cds_type_url, {cluster_0, cluster_1}, {cluster_0, cluster_1}, {}, "1", false);
+
+  EXPECT_TRUE(compareDiscoveryRequest(sds_type_url, "", {"validation_context"},
+                                      {"validation_context"}, {}));
+  test_server_->waitForGaugeGe("cluster_manager.warming_clusters", 2);
+
+  envoy::extensions::transport_sockets::tls::v3::Secret validation_context;
+  TestUtility::loadFromYaml(fmt::format(R"EOF(
+    name: validation_context
+    validation_context:
+      trusted_ca:
+        filename: {}
+  )EOF",
+                                        TestEnvironment::runfilesPath(
+                                            "test/config/integration/certs/upstreamcacert.pem")),
+                            validation_context);
+
+  sendDiscoveryResponse<envoy::extensions::transport_sockets::tls::v3::Secret>(
+      sds_type_url, {validation_context}, {validation_context}, {}, "1");
+  test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 0);
+}
+
 // Validate basic config delivery and upgrade with RateLimiting.
 TEST_P(AdsIntegrationTest, BasicWithRateLimiting) {
   initializeAds(true);

test/integration/base_integration_test.h

@@ -252,16 +252,15 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
   }
 
 private:
-  std::string intResourceName(const envoy::config::listener::v3::Listener& m) { return m.name(); }
-  std::string intResourceName(const envoy::config::route::v3::RouteConfiguration& m) {
-    return m.name();
-  }
-  std::string intResourceName(const envoy::config::cluster::v3::Cluster& m) { return m.name(); }
-  std::string intResourceName(const envoy::config::endpoint::v3::ClusterLoadAssignment& m) {
-    return m.cluster_name();
+  template <class T> std::string intResourceName(const T& m) {
+    // gcc doesn't allow inline template function to be specialized, using a constexpr if to
+    // workaround.
+    if constexpr (std::is_same_v<T, envoy::config::endpoint::v3::ClusterLoadAssignment>) {
+      return m.cluster_name();
+    } else {
+      return m.name();
+    }
   }
-  std::string intResourceName(const envoy::config::route::v3::VirtualHost& m) { return m.name(); }
-  std::string intResourceName(const envoy::service::runtime::v3::Runtime& m) { return m.name(); }
 
   Event::GlobalTimeSystem time_system_;