-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fetch certificate validation context using SDS service. #4355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
9cb2e20
cd35a2c
61bdf6c
cf3cbf3
a905c0a
d9414bd
a12b042
058d3d0
820f6a2
99b9957
91996d6
0925dd4
d7532e8
d89a836
259322a
a02fef6
4e2594c
1557732
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,11 +11,14 @@ | |
| #include "envoy/runtime/runtime.h" | ||
| #include "envoy/secret/secret_callbacks.h" | ||
| #include "envoy/secret/secret_provider.h" | ||
| #include "envoy/server/transport_socket_config.h" | ||
| #include "envoy/stats/stats.h" | ||
| #include "envoy/upstream/cluster_manager.h" | ||
|
|
||
| #include "common/common/callback_impl.h" | ||
| #include "common/common/cleanup.h" | ||
| #include "common/ssl/certificate_validation_context_config_impl.h" | ||
| #include "common/ssl/tls_certificate_config_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
@@ -24,13 +27,12 @@ namespace Secret { | |
| * SDS API implementation that fetches secrets from SDS server via Subscription. | ||
| */ | ||
| class SdsApi : public Init::Target, | ||
| public TlsCertificateConfigProvider, | ||
| public Config::SubscriptionCallbacks<envoy::api::v2::auth::Secret> { | ||
| public: | ||
| SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, | ||
| Runtime::RandomGenerator& random, Stats::Store& stats, | ||
| Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, | ||
| std::function<void()> destructor_cb); | ||
|
|
||
| // Init::Target | ||
|
|
@@ -43,14 +45,11 @@ class SdsApi : public Init::Target, | |
| return MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resource).name(); | ||
| } | ||
|
|
||
| // SecretProvider | ||
| const Ssl::TlsCertificateConfig* secret() const override { | ||
| return tls_certificate_secrets_.get(); | ||
| } | ||
|
|
||
| Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override { | ||
| return update_callback_manager_.add(callback); | ||
| } | ||
| protected: | ||
| // Creates new secrets. | ||
| virtual void set_secret(const envoy::api::v2::auth::Secret&) PURE; | ||
| Common::CallbackManager<> update_callback_manager_; | ||
| uint64_t secret_hash_; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be private, thanks for catching this. |
||
|
|
||
| private: | ||
| void runInitializeCallbackIfAny(); | ||
|
|
@@ -66,13 +65,98 @@ class SdsApi : public Init::Target, | |
| std::function<void()> initialize_callback_; | ||
| const std::string sds_config_name_; | ||
|
|
||
| uint64_t secret_hash_; | ||
| Cleanup clean_up_; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<SdsApi> SdsApiSharedPtr; | ||
|
|
||
| /** | ||
| * TlsCertificateSdsApi implementation maintains and updates dynamic TLS certificate secrets. | ||
| */ | ||
| class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider { | ||
| public: | ||
| static SdsApiSharedPtr | ||
| create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, | ||
| std::function<void()> destructor_cb) { | ||
| return std::make_shared<TlsCertificateSdsApi>( | ||
| secret_provider_context.localInfo(), secret_provider_context.dispatcher(), | ||
| secret_provider_context.random(), secret_provider_context.stats(), | ||
| secret_provider_context.clusterManager(), *secret_provider_context.initManager(), | ||
| sds_config, sds_config_name, destructor_cb); | ||
| } | ||
|
|
||
| TlsCertificateSdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, | ||
| Runtime::RandomGenerator& random, Stats::Store& stats, | ||
| Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, | ||
| const std::string& sds_config_name, std::function<void()> destructor_cb) | ||
| : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config, | ||
| sds_config_name, destructor_cb) {} | ||
|
|
||
| // SecretProvider | ||
| const Ssl::TlsCertificateConfig* secret() const override { | ||
| return tls_certificate_secrets_.get(); | ||
| } | ||
| Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override { | ||
| return update_callback_manager_.add(callback); | ||
| } | ||
|
|
||
| protected: | ||
| void set_secret(const envoy::api::v2::auth::Secret& secret) override { | ||
| tls_certificate_secrets_ = | ||
| std::make_unique<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate()); | ||
| } | ||
|
|
||
| private: | ||
| Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; | ||
| Common::CallbackManager<> update_callback_manager_; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<SdsApi> SdsApiPtr; | ||
| /** | ||
| * CertificateValidationContextSdsApi implementation maintains and updates dynamic certificate | ||
| * validation context secrets. | ||
| */ | ||
| class CertificateValidationContextSdsApi : public SdsApi, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I thought templates might be a nice thing here to avoid the boiler plate, but then we have another problem, namely how to marry templates and virtual inheritance. Worth thinking about if you have any way to reduce this repetition.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We tried using templates at first (cd35a2c), and then we found that causes many duplicated methods. We have to add more methods into secret manager to create each type of provider, and the provider creation methods have duplicated code. We also need two maps for each type of providers in secret manager. Besides, each type of provider also overrides some methods of provider interface, and those methods have duplicated code, too. Then we decide to switch to this way and simplify the code a lot. We don't find a better way for now. |
||
| public CertificateValidationContextConfigProvider { | ||
| public: | ||
| static SdsApiSharedPtr | ||
| create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, | ||
| std::function<void()> destructor_cb) { | ||
| return std::make_shared<CertificateValidationContextSdsApi>( | ||
| secret_provider_context.localInfo(), secret_provider_context.dispatcher(), | ||
| secret_provider_context.random(), secret_provider_context.stats(), | ||
| secret_provider_context.clusterManager(), *secret_provider_context.initManager(), | ||
| sds_config, sds_config_name, destructor_cb); | ||
| } | ||
| CertificateValidationContextSdsApi(const LocalInfo::LocalInfo& local_info, | ||
| Event::Dispatcher& dispatcher, | ||
| Runtime::RandomGenerator& random, Stats::Store& stats, | ||
| Upstream::ClusterManager& cluster_manager, | ||
| Init::Manager& init_manager, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, | ||
| std::string sds_config_name, | ||
| std::function<void()> destructor_cb) | ||
| : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config, | ||
| sds_config_name, destructor_cb) {} | ||
|
|
||
| // SecretProvider | ||
| const Ssl::CertificateValidationContextConfig* secret() const override { | ||
| return certificate_validation_context_secrets_.get(); | ||
| } | ||
| Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override { | ||
| return update_callback_manager_.add(callback); | ||
| } | ||
|
|
||
| protected: | ||
| void set_secret(const envoy::api::v2::auth::Secret& secret) override { | ||
| certificate_validation_context_secrets_ = | ||
| std::make_unique<Ssl::CertificateValidationContextConfigImpl>(secret.validation_context()); | ||
| } | ||
|
|
||
| private: | ||
| Ssl::CertificateValidationContextConfigPtr certificate_validation_context_secrets_; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,37 +65,57 @@ SecretManagerImpl::createInlineCertificateValidationContextProvider( | |
| } | ||
|
|
||
| void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { | ||
| ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); | ||
| ENVOY_LOG(debug, "Unregister tls certificate provider. hash key: {}", map_key); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this definitely TLS? It seems the general
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this message is not correct. Changed to secret provider. |
||
|
|
||
| auto num_deleted = dynamic_secret_providers_.erase(map_key); | ||
| ASSERT(num_deleted == 1, ""); | ||
| } | ||
|
|
||
| TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( | ||
| SdsApiSharedPtr SecretManagerImpl::innerFindOrCreate( | ||
| const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { | ||
| std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn) { | ||
| const std::string map_key = sds_config_source.SerializeAsString() + config_name; | ||
|
|
||
| TlsCertificateConfigProviderSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); | ||
| SdsApiSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); | ||
| if (!secret_provider) { | ||
| ASSERT(secret_provider_context.initManager() != nullptr); | ||
|
|
||
| // SdsApi is owned by ListenerImpl and ClusterInfo which are destroyed before | ||
| // SecretManagerImpl. It is safe to invoke this callback at the destructor of SdsApi. | ||
| std::function<void()> unregister_secret_provider = [map_key, this]() { | ||
| removeDynamicSecretProvider(map_key); | ||
| }; | ||
|
|
||
| secret_provider = std::make_shared<SdsApi>( | ||
| secret_provider_context.localInfo(), secret_provider_context.dispatcher(), | ||
| secret_provider_context.random(), secret_provider_context.stats(), | ||
| secret_provider_context.clusterManager(), *secret_provider_context.initManager(), | ||
| sds_config_source, config_name, unregister_secret_provider); | ||
| secret_provider = create_fn(unregister_secret_provider); | ||
| dynamic_secret_providers_[map_key] = secret_provider; | ||
| } | ||
|
|
||
| return secret_provider; | ||
| } | ||
|
|
||
| TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( | ||
| const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { | ||
| auto create_fn = [&](std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to use explicit capture rather than
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, done. |
||
| ASSERT(secret_provider_context.initManager() != nullptr); | ||
| return TlsCertificateSdsApi::create(secret_provider_context, sds_config_source, config_name, | ||
| unregister_secret_provider); | ||
| }; | ||
| SdsApiSharedPtr secret_provider = innerFindOrCreate(sds_config_source, config_name, create_fn); | ||
|
|
||
| return std::dynamic_pointer_cast<TlsCertificateConfigProvider>(secret_provider); | ||
| } | ||
|
|
||
| CertificateValidationContextConfigProviderSharedPtr | ||
| SecretManagerImpl::findOrCreateCertificateValidationContextProvider( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code is similar for this function and the other. try to share them by create a SdsApiSharedPtr innerFindOrCreate(..., create_fn); Then, each function will provide its creation_function to create proper object,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { | ||
| auto create_fn = [&](std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr { | ||
| ASSERT(secret_provider_context.initManager() != nullptr); | ||
| return CertificateValidationContextSdsApi::create(secret_provider_context, sds_config_source, | ||
| config_name, unregister_secret_provider); | ||
| }; | ||
| SdsApiSharedPtr secret_provider = innerFindOrCreate(sds_config_source, config_name, create_fn); | ||
|
|
||
| return std::dynamic_pointer_cast<CertificateValidationContextConfigProvider>(secret_provider); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we templatize
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are building sidecar with SDS feature from istio:collab-gcp-identity branch, and @quanjielin is working on merging istio:collab-gcp-identity branch into istio:master. This PR blocks the merge. |
||
| } | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| #include "envoy/ssl/tls_certificate_config.h" | ||
|
|
||
| #include "common/common/logger.h" | ||
| #include "common/secret/sds_api.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
@@ -35,9 +36,18 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::sec | |
| const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override; | ||
|
|
||
| CertificateValidationContextConfigProviderSharedPtr | ||
| findOrCreateCertificateValidationContextProvider( | ||
| const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override; | ||
|
|
||
| private: | ||
| // Remove dynamic secret provider which has been deleted. | ||
| // Removes dynamic secret provider which has been deleted. | ||
| void removeDynamicSecretProvider(const std::string& map_key); | ||
| // Finds or creates SdsApi object. | ||
| SdsApiSharedPtr innerFindOrCreate( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Changed to findOrCreate. Thanks. |
||
| const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, | ||
| std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn); | ||
|
|
||
| // Manages pairs of secret name and TlsCertificateConfigProviderSharedPtr. | ||
| std::unordered_map<std::string, TlsCertificateConfigProviderSharedPtr> | ||
|
|
@@ -48,8 +58,7 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::sec | |
| static_certificate_validation_context_providers_; | ||
|
|
||
| // map hash code of SDS config source and SdsApi object. | ||
| std::unordered_map<std::string, std::weak_ptr<TlsCertificateConfigProvider>> | ||
| dynamic_secret_providers_; | ||
| std::unordered_map<std::string, std::weak_ptr<SdsApi>> dynamic_secret_providers_; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
setSecretThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks!