From 08e44a6662fe25709969be268235cae2c87fad63 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Thu, 10 Dec 2020 00:39:16 -0800 Subject: [PATCH] sds: refactor to use shared target for init manager registration Signed-off-by: Lizan Zhou --- include/envoy/secret/BUILD | 1 + include/envoy/secret/secret_provider.h | 7 +++++++ source/common/secret/sds_api.h | 26 +++++++++----------------- test/common/secret/sds_api_test.cc | 26 +++++++++++++------------- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index 219884c19e81..04adec2fab50 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -30,6 +30,7 @@ envoy_cc_library( hdrs = ["secret_manager.h"], deps = [ ":secret_provider_interface", + "//include/envoy/init:target_interface", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", ], diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index 2dbc31e5791a..0b6c098bc2cd 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -5,6 +5,7 @@ #include "envoy/common/callback.h" #include "envoy/common/pure.h" #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" +#include "envoy/init/target.h" #include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" @@ -41,6 +42,12 @@ template class SecretProvider { * @return CallbackHandle the handle which can remove that update callback. */ virtual Common::CallbackHandle* addUpdateCallback(std::function callback) PURE; + + /** + * @return const Init::Target* A shared init target that can be used by multiple init managers. + * nullptr if the provider isn't dynamic. + */ + virtual const Init::Target* initTarget() { return nullptr; } }; using TlsCertificatePtr = diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 1c58feb6c5bc..4be8fb63d4fb 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -59,19 +59,9 @@ class SdsApi : public Envoy::Config::SubscriptionBase< Config::SubscriptionFactory& subscription_factory, TimeSource& time_source, ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats, std::function destructor_cb, Event::Dispatcher& dispatcher, Api::Api& api); - ~SdsApi() override { - RELEASE_ASSERT(registered_init_target_, - "Init target was not registered with an init manager. registerInitTarget() must " - "be called after Sds api concrete class instantiation."); - }; SecretData secretData(); - void registerInitTarget(Init::Manager& init_manager) { - init_manager.add(init_target_); - registered_init_target_ = true; - } - protected: // Ordered for hash stability. using FileContentMap = std::map; @@ -97,7 +87,7 @@ class SdsApi : public Envoy::Config::SubscriptionBase< void resolveDataSource(const FileContentMap& files, envoy::config::core::v3::DataSource& data_source); - Init::TargetImpl init_target_; + Init::SharedTargetImpl init_target_; Event::Dispatcher& dispatcher_; Api::Api& api_; @@ -124,7 +114,6 @@ class SdsApi : public Envoy::Config::SubscriptionBase< TimeSource& time_source_; SecretData secret_data_; std::unique_ptr watcher_; - bool registered_init_target_{false}; }; class TlsCertificateSdsApi; @@ -154,7 +143,7 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider secret_provider_context.dispatcher().timeSource(), secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(), destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api()); - ret->registerInitTarget(secret_provider_context.initManager()); + secret_provider_context.initManager().add(*ret->initTarget()); return ret; } @@ -182,6 +171,7 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider } return update_callback_manager_.add(callback); } + const Init::Target* initTarget() override { return &init_target_; } protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { @@ -238,7 +228,7 @@ class CertificateValidationContextSdsApi : public SdsApi, secret_provider_context.dispatcher().timeSource(), secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(), destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api()); - ret->registerInitTarget(secret_provider_context.initManager()); + secret_provider_context.initManager().add(*ret->initTarget()); return ret; } CertificateValidationContextSdsApi(const envoy::config::core::v3::ConfigSource& sds_config, @@ -262,13 +252,13 @@ class CertificateValidationContextSdsApi : public SdsApi, } return update_callback_manager_.add(callback); } - Common::CallbackHandle* addValidationCallback( std::function< void(const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext&)> callback) override { return validation_callback_manager_.add(callback); } + const Init::Target* initTarget() override { return &init_target_; } protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { @@ -333,7 +323,7 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon secret_provider_context.dispatcher().timeSource(), secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(), destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api()); - ret->registerInitTarget(secret_provider_context.initManager()); + secret_provider_context.initManager().add(*ret->initTarget()); return ret; } @@ -366,6 +356,7 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon callback) override { return validation_callback_manager_.add(callback); } + const Init::Target* initTarget() override { return &init_target_; } protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { @@ -405,7 +396,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { secret_provider_context.dispatcher().timeSource(), secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(), destructor_cb, secret_provider_context.dispatcher(), secret_provider_context.api()); - ret->registerInitTarget(secret_provider_context.initManager()); + secret_provider_context.initManager().add(*ret->initTarget()); return ret; } @@ -430,6 +421,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { callback) override { return validation_callback_manager_.add(callback); } + const Init::Target* initTarget() override { return &init_target_; } protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 8e8908fa1296..90ccdec8ebd8 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -76,7 +76,7 @@ TEST_F(SdsApiTest, BasicTest) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); initialize(); } @@ -126,7 +126,7 @@ TEST_F(SdsApiTest, InitManagerInitialised) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - EXPECT_NO_THROW(sds_api.registerInitTarget(init_manager)); + EXPECT_NO_THROW(init_manager.add(*sds_api.initTarget())); } // Validate that bad ConfigSources are caught at construction time. This is a regression test for @@ -153,7 +153,7 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); initialize(); NiceMock secret_callback; auto handle = @@ -217,7 +217,7 @@ class TlsCertificateSdsRotationApiTest : public testing::TestWithParam, sds_api_ = std::make_unique( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, mock_dispatcher_, *api_); - sds_api_->registerInitTarget(init_manager_); + init_manager_.add(*sds_api_->initTarget()); initialize(); handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); } @@ -287,7 +287,7 @@ class CertificateValidationContextSdsRotationApiTest : public testing::TestWithP sds_api_ = std::make_unique( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, mock_dispatcher_, *api_); - sds_api_->registerInitTarget(init_manager_); + init_manager_.add(*sds_api_->initTarget()); initialize(); handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); } @@ -500,7 +500,7 @@ class PartialMockSds : public SdsApi { : SdsApi( config_source, "abc.com", subscription_factory, time_source, validation_visitor_, stats, []() {}, dispatcher, api) { - registerInitTarget(init_manager); + init_manager.add(init_target_); } MOCK_METHOD(void, onConfigUpdate, @@ -554,7 +554,7 @@ TEST_F(SdsApiTest, DeltaUpdateSuccess) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); NiceMock secret_callback; auto handle = @@ -599,7 +599,7 @@ TEST_F(SdsApiTest, DynamicCertificateValidationContextUpdateSuccess) { CertificateValidationContextSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); NiceMock secret_callback; auto handle = @@ -653,7 +653,7 @@ TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { CertificateValidationContextSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); NiceMock secret_callback; auto handle = @@ -741,7 +741,7 @@ TEST_F(SdsApiTest, GenericSecretSdsApiTest) { GenericSecretSdsApi sds_api( config_source, "encryption_key", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); NiceMock secret_callback; auto handle = @@ -786,7 +786,7 @@ TEST_F(SdsApiTest, EmptyResource) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); initialize(); EXPECT_THROW_WITH_MESSAGE(subscription_factory_.callbacks_->onConfigUpdate({}, ""), @@ -801,7 +801,7 @@ TEST_F(SdsApiTest, SecretUpdateWrongSize) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); std::string yaml = R"EOF( @@ -831,7 +831,7 @@ TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { TlsCertificateSdsApi sds_api( config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, []() {}, *dispatcher_, *api_); - sds_api.registerInitTarget(init_manager_); + init_manager_.add(*sds_api.initTarget()); std::string yaml = R"EOF(