-
Notifications
You must be signed in to change notification settings - Fork 5.3k
High level design PR: Add SDS dynamic secret #3748
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 all commits
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 |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "envoy/secret/secret_callbacks.h" | ||
| #include "envoy/ssl/tls_certificate_config.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| /** | ||
| * An interface to fetch dynamic secret. | ||
| */ | ||
| class DynamicSecretProvider { | ||
| public: | ||
| virtual ~DynamicSecretProvider() {} | ||
|
|
||
| /** | ||
| * @return the TlsCertificate secret. Returns nullptr if the secret is not found. | ||
| */ | ||
| virtual const Ssl::TlsCertificateConfigSharedPtr secret() const PURE; | ||
|
|
||
| virtual void addUpdateCallback(SecretCallbacks& callback) PURE; | ||
| virtual void removeUpdateCallback(SecretCallbacks& callback) PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<DynamicSecretProvider> DynamicSecretProviderSharedPtr; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| #pragma once | ||
|
|
||
| #include <memory> | ||
| #include <string> | ||
|
|
||
| #include "envoy/common/pure.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| /** | ||
| * Callbacks invoked by a secret manager. | ||
| */ | ||
| class SecretCallbacks { | ||
| public: | ||
| virtual ~SecretCallbacks() {} | ||
|
|
||
| virtual void onAddOrUpdateSecret() PURE; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,31 +3,46 @@ | |
| #include <string> | ||
|
|
||
| #include "envoy/api/v2/auth/cert.pb.h" | ||
| #include "envoy/secret/dynamic_secret_provider.h" | ||
| #include "envoy/ssl/tls_certificate_config.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| /** | ||
| * A manager for static secrets. | ||
| * | ||
| * TODO(jaebong) Support dynamic secrets. | ||
| * A manager for static and dynamic secrets. | ||
| */ | ||
| class SecretManager { | ||
| public: | ||
| virtual ~SecretManager() {} | ||
|
|
||
| /** | ||
| * @param config_source_hash a hash string of normalized config source. If it is empty string, | ||
| * find secret from the static secrets. | ||
| * @param secret a protobuf message of envoy::api::v2::auth::Secret. | ||
| * @throw an EnvoyException if the secret is invalid or not supported. | ||
| */ | ||
| virtual void addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) PURE; | ||
| virtual void addStaticSecret(const envoy::api::v2::auth::Secret& secret) PURE; | ||
|
|
||
| /** | ||
| * @param sds_config_source_hash hash string of normalized config source. | ||
| * @param name a name of the Ssl::TlsCertificateConfig. | ||
| * @return the TlsCertificate secret. Returns nullptr if the secret is not found. | ||
| */ | ||
| virtual const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& name) const PURE; | ||
| virtual const Ssl::TlsCertificateConfigSharedPtr | ||
| findStaticTlsCertificate(const std::string& name) const PURE; | ||
|
|
||
| /** | ||
| * Add or update SDS config source. SecretManager starts downloading secrets from registered | ||
| * config source. | ||
| * | ||
| * @param sdsConfigSource a protobuf message object contains SDS config source. | ||
| * @param config_name a name that uniquely refers to the SDS config source. | ||
| * @return a hash string of normalized config source. | ||
| */ | ||
| virtual DynamicSecretProviderSharedPtr | ||
| createDynamicSecretProvider(const envoy::api::v2::core::ConfigSource& config_source, | ||
|
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. from impl, this is not a pure create, but find one if there is existing, need a better name.
Contributor
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. how about "findOrCreateDynamicSecretProvider"? |
||
| std::string config_name) PURE; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,12 +32,15 @@ class Context { | |
| */ | ||
| virtual std::string getCertChainInformation() const PURE; | ||
| }; | ||
| typedef std::shared_ptr<Context> ContextSharedPtr; | ||
|
|
||
| class ClientContext : public virtual Context {}; | ||
| typedef std::unique_ptr<ClientContext> ClientContextPtr; | ||
| typedef std::shared_ptr<ClientContext> ClientContextSharedPtr; | ||
|
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. Do we intended to make all *Context to SharedPtr? What is the usecase of unique_ptrs?
Contributor
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. unique_ptr can be removed.
Contributor
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. Actually, I am working on a separate PR of making ssl_socket not to hold context as its member, only use ctx at constructor. If this works, we will continue to use unique_ptr for ctx. |
||
|
|
||
| class ServerContext : public virtual Context {}; | ||
| typedef std::unique_ptr<ServerContext> ServerContextPtr; | ||
| typedef std::shared_ptr<ServerContext> ServerContextSharedPtr; | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,14 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/service/discovery/v2/ads.pb.h" | ||
| #include "envoy/service/discovery/v2/sds.pb.h" | ||
| #include "envoy/service/ratelimit/v2/rls.pb.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| // Hack to force linking of the service: https://github.com/google/protobuf/issues/4221. | ||
| // This file should be included ONLY if this hack is required. | ||
| const envoy::service::discovery::v2::AdsDummy _ads_dummy; | ||
| const envoy::service::discovery::v2::SdsDummy _sds_dummy; | ||
| const envoy::service::ratelimit::v2::RateLimitRequest _rls_dummy; | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| #include "common/secret/sds_api.h" | ||
|
|
||
| #include <unordered_map> | ||
|
|
||
| #include "envoy/api/v2/auth/cert.pb.validate.h" | ||
|
|
||
| #include "common/config/resources.h" | ||
| #include "common/config/subscription_factory.h" | ||
| #include "common/secret/secret_manager_util.h" | ||
| #include "common/ssl/tls_certificate_config_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| SdsApi::SdsApi(Server::Instance& server, const envoy::api::v2::core::ConfigSource& sds_config, | ||
| std::string sds_config_name) | ||
| : server_(server), sds_config_(sds_config), sds_config_name_(sds_config_name) { | ||
| server_.initManager().registerTarget(*this); | ||
| } | ||
|
|
||
| void SdsApi::initialize(std::function<void()> callback) { | ||
| initialize_callback_ = callback; | ||
| subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< | ||
| envoy::api::v2::auth::Secret>( | ||
| sds_config_, server_.localInfo().node(), server_.dispatcher(), server_.clusterManager(), | ||
| server_.random(), server_.stats(), /* rest_legacy_constructor */ nullptr, | ||
| "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", | ||
| // TODO(jaebong): replace next line with | ||
| // "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets" to support streaming | ||
| // service | ||
| "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets"); | ||
|
|
||
| Config::Utility::checkLocalInfo("sds", server_.localInfo()); | ||
|
|
||
| subscription_->start({sds_config_name_}, *this); | ||
| } | ||
|
|
||
| void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) { | ||
| if (resources.empty()) { | ||
| runInitializeCallbackIfAny(); | ||
| return; | ||
| } | ||
| if (resources.size() != 1) { | ||
| throw EnvoyException(fmt::format("Unexpected RDS resource length: {}", resources.size())); | ||
| } | ||
| const auto& secret = resources[0]; | ||
| MessageUtil::validate(secret); | ||
| if (!(secret.name() == sds_config_name_)) { | ||
| throw EnvoyException(fmt::format("Unexpected RDS configuration (expecting {}): {}", | ||
| sds_config_name_, secret.name())); | ||
| } | ||
|
|
||
| const uint64_t new_hash = MessageUtil::hash(secret); | ||
| if (new_hash != secret_hash_) { | ||
| if (secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate) { | ||
| tls_certificate_secrets_ = | ||
| std::make_shared<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate()); | ||
| secret_hash_ = new_hash; | ||
|
|
||
| for (auto cb : update_callbacks_) { | ||
| cb->onAddOrUpdateSecret(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| runInitializeCallbackIfAny(); | ||
| } | ||
|
|
||
| void SdsApi::onConfigUpdateFailed(const EnvoyException*) { | ||
| // We need to allow server startup to continue, even if we have a bad config. | ||
| runInitializeCallbackIfAny(); | ||
| } | ||
|
|
||
| void SdsApi::runInitializeCallbackIfAny() { | ||
| if (initialize_callback_) { | ||
| initialize_callback_(); | ||
| initialize_callback_ = nullptr; | ||
| } | ||
| } | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy |
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.
I'm curious why we need this and
createDynamicSecretProvider, but not make SecretManager to manage callbacks.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.
Managing update callbacks by DynamicSecretProvider is more efficient since it only holds the callbacks that using that secret. If done in the secretManager, you need a map of hash + name to the vector of callbacks, you need map lookup. Beside, DynamicSecretProvider is implemented by SdsAPI. It has the onConfigUpate() call, so it is more nature to call its callbacks when onConfigUpdate() is called. It doesn't need to hold secretManager object either.