-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Sds api split #4231
Sds api split #4231
Changes from 4 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,18 @@ | ||
#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 |
---|---|---|
@@ -1,6 +1,7 @@ | ||
#pragma once | ||
|
||
#include "envoy/common/pure.h" | ||
#include "envoy/secret/secret_callbacks.h" | ||
#include "envoy/ssl/tls_certificate_config.h" | ||
|
||
namespace Envoy { | ||
|
@@ -18,7 +19,19 @@ template <class SecretType> class SecretProvider { | |
*/ | ||
virtual const SecretType* secret() const PURE; | ||
|
||
// TODO(lizan): Add more methods for dynamic secret provider. | ||
/** | ||
* Add secret callback into secret provider. | ||
* It is safe to call this method by main thread and is safe to be invoked | ||
* on main thread. | ||
* @param callback callback that is executed by secret provider. | ||
*/ | ||
virtual void addUpdateCallback(SecretCallbacks& callback) PURE; | ||
|
||
/** | ||
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. Nit: blank line between declaration and comment belonging to next block. 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. Added blank line. |
||
* Remove secret callback from secret provider. | ||
* @param callback callback that is executed by secret provider. | ||
*/ | ||
virtual void removeUpdateCallback(SecretCallbacks& callback) PURE; | ||
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. A general pattern in Envoy is to use RAII for managing removal of callbacks; it's a robust design pattern. We actually have a callback manager that models this today, see https://github.com/envoyproxy/envoy/blob/master/source/common/common/callback_impl.h 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. Are you looking at this one as well? 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, I am working on this one. I will push my update as soon I finish it. Thanks for asking. 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. I have added CallbackManager into SdsApi and removed removeUpdateCallback(). In PR #4176, I am going to add Common::CallbackHandle* secret_update_callback_handle_ into ContextConfigImpl as a private member, and modify ContextConfigImpl::setSecretUpdateCallback() like this.
I will also modify ~ContextConfigImpl() like this.
|
||
}; | ||
|
||
typedef SecretProvider<Ssl::TlsCertificateConfig> TlsCertificateConfigProvider; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
#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()> destructor_cb) | ||
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. This is better than before, thanks. I still think it can be cleaner if instead of having 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 for pointing this out. My concern is that SdsApi is held by a shared_ptr (ContextConfigImpland::tls_certficate_provider_) and could be owned by multiple objects (e.g. multiple listeners or clusters). The last owner needs to destroy SdsApi and ~SdsApi() invokes this cleanup function. I am not clear if there is a better place to own the Cleanup object other than SdsApi. 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. 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. Yeah, OK, I still think you could wrap |
||
: 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), clean_up_(destructor_cb) { | ||
// TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager | ||
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. Once this is resolved, do we still need NotReadySslSocket? 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 still need NotReadySslSocket. Chained init manager guarantees that If two listeners point to the same secret, both listeners would wait for secrets and then listen on port. A better explanation is here. NotReadySslSocket is to solve the issue that if a listener gets secret and listens on a port, but the secret is bad, we should not use the secret to handle connection, and close the connection right away. |
||
// 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. | ||
init_manager.registerTarget(*this); | ||
} | ||
|
||
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_); | ||
|
||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
#pragma once | ||
|
||
#include <functional> | ||
|
||
#include "envoy/api/v2/auth/cert.pb.h" | ||
#include "envoy/api/v2/core/config_source.pb.h" | ||
#include "envoy/config/subscription.h" | ||
#include "envoy/event/dispatcher.h" | ||
#include "envoy/init/init.h" | ||
#include "envoy/local_info/local_info.h" | ||
#include "envoy/runtime/runtime.h" | ||
#include "envoy/secret/secret_callbacks.h" | ||
#include "envoy/secret/secret_provider.h" | ||
#include "envoy/stats/stats.h" | ||
#include "envoy/upstream/cluster_manager.h" | ||
|
||
#include "common/common/cleanup.h" | ||
|
||
namespace Envoy { | ||
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, | ||
std::function<void()> destructor_cb); | ||
|
||
// Init::Target | ||
void initialize(std::function<void()> callback) override; | ||
|
||
// Config::SubscriptionCallbacks | ||
void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; | ||
void onConfigUpdateFailed(const EnvoyException* e) override; | ||
std::string resourceName(const ProtobufWkt::Any& resource) override { | ||
return MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resource).name(); | ||
} | ||
|
||
// SecretProvider | ||
const Ssl::TlsCertificateConfig* secret() const override { | ||
return tls_certificate_secrets_.get(); | ||
} | ||
|
||
void addUpdateCallback(SecretCallbacks& callback) override { | ||
update_callbacks_.push_back(&callback); | ||
} | ||
void removeUpdateCallback(SecretCallbacks& callback) override { | ||
update_callbacks_.remove(&callback); | ||
} | ||
|
||
private: | ||
void runInitializeCallbackIfAny(); | ||
|
||
const LocalInfo::LocalInfo& local_info_; | ||
Event::Dispatcher& dispatcher_; | ||
Runtime::RandomGenerator& random_; | ||
Stats::Store& stats_; | ||
Upstream::ClusterManager& cluster_manager_; | ||
|
||
const envoy::api::v2::core::ConfigSource sds_config_; | ||
std::unique_ptr<Config::Subscription<envoy::api::v2::auth::Secret>> subscription_; | ||
std::function<void()> initialize_callback_; | ||
const std::string sds_config_name_; | ||
|
||
uint64_t secret_hash_; | ||
Cleanup clean_up_; | ||
Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; | ||
std::list<SecretCallbacks*> update_callbacks_; | ||
}; | ||
|
||
typedef std::unique_ptr<SdsApi> SdsApiPtr; | ||
|
||
} // 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.
Thanks for splitting this PR out, this is very helpful.
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.
Glad that it helps. Will keep PR in good size.