Implementation of SDS #4176
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/common/pure.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| /** | ||
| * Callbacks invoked by a dynamic secret provider. | ||
| */ | ||
| class SecretCallbacks { | ||
| public: | ||
| virtual ~SecretCallbacks() {} | ||
|
|
||
| virtual void onAddOrUpdateSecret() PURE; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| #include <string> | ||
|
|
||
| #include "envoy/event/dispatcher.h" | ||
| #include "envoy/init/init.h" | ||
| #include "envoy/local_info/local_info.h" | ||
| #include "envoy/network/transport_socket.h" | ||
| #include "envoy/runtime/runtime.h" | ||
|
|
@@ -63,6 +64,18 @@ class TransportSocketFactoryContext { | |
| * @return the server-wide stats store. | ||
| */ | ||
| virtual Stats::Store& stats() PURE; | ||
|
|
||
| /** | ||
| * Pass an init manager to register dynamic secret provider. | ||
| * @param init_manager instance of init manager. | ||
| */ | ||
| virtual void setInitManager(Init::Manager& init_manager) PURE; | ||
|
There was a problem hiding this comment. It's not optimal that this interface has a setter and a getter for the init manager. Is there any way to simplify this so that the init manager is known ahead of time and we only need a getter? There was a problem hiding this comment. Ideally the init manager should exist when we create TransportSocketFactoryContext, so that we only need a getter. But we create TransportSocketFactoryContext at ClusterManagerFactory and then create init manager per cluster, we have to add it into factory context. @qiwzhang proposed #3831, I think once we are working on that, we can get rid of this setter. There was a problem hiding this comment. Shouldn't the init manager then be part of the factory context? There was a problem hiding this comment. For now, init manager is per cluster. At the time of the construction of TransportSocketFactoryContext, per cluster init manager is not available. We have to set it when init manager is available. |
||
|
|
||
| /** | ||
| * @return a pointer pointing to the instance of an init manager, or nullptr | ||
| * if not set. | ||
| */ | ||
| virtual Init::Manager* initManager() PURE; | ||
| }; | ||
|
|
||
| class TransportSocketConfigFactory { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| #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/protobuf/utility.h" | ||
| #include "common/ssl/tls_certificate_config_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| SdsApi::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, | ||
| std::function<void()> unregister_secret_provider) | ||
| : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), | ||
| cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), | ||
| secret_hash_(0), unregister_secret_provider_cb_(unregister_secret_provider) { | ||
| // 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. | ||
|
There was a problem hiding this comment. I'm not sure I fully understand the implication here. If two listeners point to the same secret today, what happens? Does one listener just immediately initialize? There was a problem hiding this comment. In the current implementation, yes. The second listener will not wait, its initialization will finish right away since its init_manager is not used. Only the first one wait for the sds server response. I have a rough idea of how to solve this: design a InitManagerChain; it can register multiple init_managers, but behaves like one init_manager. Each indivicual init_manager's callbacks will be called when the combined_init_manager callback is called. That is what this TODO about. like to implement it as a separate PR. There was a problem hiding this comment. Can you also file an issue? This seems a major issue, since you will not have warming on SDS work correctly otherwise. There was a problem hiding this comment. I have filed an issue here #4224. Please take a look and let me know if any modification is required. Thanks. |
||
| init_manager.registerTarget(*this); | ||
| } | ||
|
|
||
| SdsApi::~SdsApi() { unregister_secret_provider_cb_(); } | ||
|
|
||
| void SdsApi::initialize(std::function<void()> callback) { | ||
| initialize_callback_ = callback; | ||
|
|
||
| subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< | ||
| envoy::api::v2::auth::Secret>( | ||
| sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, | ||
| /* rest_legacy_constructor */ nullptr, | ||
| "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", | ||
| "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets"); | ||
| Config::Utility::checkLocalInfo("sds", local_info_); | ||
|
There was a problem hiding this comment. Can move subcription_ creation into the constructor so we don't need to store so many object references. Only call start in the initialize() function There was a problem hiding this comment. We cannot move subcription_ into constructor. Envoy::Config::SubscriptionFactory::subscriptionFromConfigSourceenvoy::api::v2::auth::Secre will access members of those objects, which are not ready when SdsApi constructor is called. That causes segmentation fault in test runs. |
||
|
|
||
| subscription_->start({sds_config_name_}, *this); | ||
| } | ||
|
|
||
| void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) { | ||
| if (resources.empty()) { | ||
| throw EnvoyException( | ||
| fmt::format("Missing SDS resources for {} in onConfigUpdate()", sds_config_name_)); | ||
| } | ||
| if (resources.size() != 1) { | ||
| throw EnvoyException(fmt::format("Unexpected SDS secrets length: {}", resources.size())); | ||
| } | ||
| const auto& secret = resources[0]; | ||
| MessageUtil::validate(secret); | ||
| if (!(secret.name() == sds_config_name_)) { | ||
|
|
||
| throw EnvoyException( | ||
| fmt::format("Unexpected SDS secret (expecting {}): {}", sds_config_name_, secret.name())); | ||
| } | ||
|
|
||
| const uint64_t new_hash = MessageUtil::hash(secret); | ||
| if (new_hash != secret_hash_ && | ||
| secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate) { | ||
| secret_hash_ = new_hash; | ||
| tls_certificate_secrets_ = | ||
| std::make_unique<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate()); | ||
|
|
||
| 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.
nit: It would probably be better to just move this interface into the secret or SSL/TLS namespace but not a big deal.
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.
Thanks, I will keep this.