From 5228a84cd6fe613053e2bbadffd700b13d0c810c Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Wed, 16 Dec 2020 12:25:12 -0800 Subject: [PATCH] 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 --- docs/root/version_history/current.rst | 1 + source/common/secret/sds_api.cc | 5 -- source/common/secret/sds_api.h | 16 ++---- source/common/secret/secret_manager_impl.h | 4 ++ test/integration/BUILD | 1 + test/integration/ads_integration_test.cc | 58 ++++++++++++++++++++++ test/integration/base_integration_test.h | 17 +++---- 7 files changed, 76 insertions(+), 26 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index bff52edb3330..cec10207a5b3 100644 --- a/docs/root/version_history/current.rst +++ b/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. diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 3f319f03e638..f239b725b1c8 100644 --- a/source/common/secret/sds_api.cc +++ b/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, diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 4be8fb63d4fb..9bba1d71db66 100644 --- a/source/common/secret/sds_api.h +++ b/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( + return std::make_shared( 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( + return std::make_shared( 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( + return std::make_shared( 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( + return std::make_shared( 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, diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 799c7415d7ce..a3045a87b4da 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/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; } diff --git a/test/integration/BUILD b/test/integration/BUILD index f960c0503ae1..79b8ed1e71b1 100644 --- a/test/integration/BUILD +++ b/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", diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index b178297fe127..afb5a1822b57 100644 --- a/test/integration/ads_integration_test.cc +++ b/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::core::v3::ApiVersion::V3); + const auto sds_type_url = + Config::getTypeUrl( + 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( + 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( + 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); diff --git a/test/integration/base_integration_test.h b/test/integration/base_integration_test.h index bc4b2d544097..a2844063eb3b 100644 --- a/test/integration/base_integration_test.h +++ b/test/integration/base_integration_test.h @@ -252,16 +252,15 @@ class BaseIntegrationTest : protected Logger::Loggable { } 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 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) { + 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_;