-
Notifications
You must be signed in to change notification settings - Fork 5.5k
config: Scoped RDS (PR 1/4): Introduce {static,dynamic} config provider framework #5243
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 5 commits
6c744cd
1bccfaf
66e4c45
02b5e97
09a0846
acf19cf
c0366a7
4b1bef4
550723c
2c79e9c
854c2f2
e67b5ee
821aab9
ca583a0
15c9d19
f69eb74
ba44ad9
142d9c1
5c71955
7441143
a36a3e8
bb34fdd
ccc244a
6d7cba8
9cf7f27
58ac876
bb990fa
b49127f
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,98 @@ | ||||
| #pragma once | ||||
|
|
||||
| #include <memory> | ||||
|
|
||||
| #include "envoy/common/time.h" | ||||
|
|
||||
| #include "common/protobuf/protobuf.h" | ||||
|
|
||||
| #include "absl/types/optional.h" | ||||
|
|
||||
| namespace Envoy { | ||||
| namespace Config { | ||||
|
|
||||
| /** | ||||
| * A provider for configuration obtained ether statically or dynamically via xDS APIs. | ||||
|
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. One high level question to kick this off with. Is the plan to also replace the static/dynamic treatment of SDS resoureces (e.g. see https://github.com/envoyproxy/envoy/blob/master/source/common/ssl/context_config_impl.cc#L57) with this class framework? It would be helpful to have in mind the concrete types that this will be instantiated with and places used when reading.
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. Yes, SDS looks like a good candidate as well, although I was not planning to transition it to the new API as part of this PR chain so there are likely a few gaps that will need to be addressed. One of those gaps is that the
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. I think it would be ideal from a tech debt perspective if we could at least file an issue for this if not do the conversion at the end of the PR chain. There is a lot of commonality here between RDS and SDS, having multiple methods of managing static/dynamic resources seems redundant. Anyway, I'll do a deep dive now.
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. Agreed, I'll file an issue to keep track of SDS. I plan to take a deeper look at it once I get through these PRs and will likely take on the work myself.
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. Issue filed: #5329.
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. I think I might be being dense here. However, this discussion and @lizan's comment here leave me with a few questions:
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. Yeah, more emphasis on the shared aspect in this comment would make sense; a short sentence or two explaining where this fits into the taxonomy of configuration management in Envoy would be rad.
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. Answering @junr03 's questions below:
Done.
This framework can also be leveraged for use cases where there is no expected sharing of dynamic resources, since that use case is a simplification of the target use case and the publicly exposed ConfigProvider API should suffice as well. However, I have avoided introducing this framework as the solution for config management, since it's likely that some of these "singly owned" use cases could make due with simpler implementations than what is imposed by this framework.
I have changed the naming of the *ImplBase classes and added comments to clarify how this framework can be leveraged for SDS as well and the static/inline distinction that it makes. Essentially, the
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.
Done. |
||||
| */ | ||||
| class ConfigProvider { | ||||
| public: | ||||
| /** | ||||
| * The "implementation" of the configuration. | ||||
| * Use config() to obtain a typed object that corresponds to the specific configuration | ||||
| * represented by this abstract type. | ||||
| */ | ||||
| class Config { | ||||
| public: | ||||
| virtual ~Config() {} | ||||
| }; | ||||
| using ConfigConstSharedPtr = std::shared_ptr<const Config>; | ||||
|
|
||||
| /** | ||||
| * Stores the config proto as well as the associated version. | ||||
| */ | ||||
| template <typename P> struct ConfigInfo { | ||||
| const P& config_; | ||||
|
|
||||
| std::string version_; | ||||
| }; | ||||
|
|
||||
| virtual ~ConfigProvider() {} | ||||
|
|
||||
| /** | ||||
| * Returns a ConfigInfo associated with the provider. | ||||
| * @return absl::optional<ConfigInfo<P>> an optional ConfigInfo; the value is set when a config is | ||||
| * available. | ||||
| */ | ||||
| template <typename P> absl::optional<ConfigInfo<P>> configInfo() { | ||||
| static_assert(std::is_base_of<Protobuf::Message, P>::value, | ||||
| "Proto type must derive from Protobuf::Message"); | ||||
|
|
||||
| const auto* config_proto = dynamic_cast<const P*>(getConfigProto()); | ||||
| if (!config_proto) { | ||||
|
AndresGuedez marked this conversation as resolved.
Outdated
|
||||
| return {}; | ||||
|
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. nit:
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. Done. |
||||
| } | ||||
| return ConfigInfo<P>{*config_proto, getConfigVersion()}; | ||||
|
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. I'm struck by the similarity to
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. This is a pattern that repeats several times across the codebase; other classes that implement it are ThreadLocal::Slot and Tcp::ConnectionPool::ConnectionData. I'll experiment with/prototype an abstraction to figure out whether it makes sense to generalize it into a common class or whether it's better left as a design pattern separately implemented by each class.
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. Yeah, no need to do gymnastics or anything here, just maybe there is an opportunity for some lightweight refactor. Thanks for taking a look. |
||||
| } | ||||
|
|
||||
| /** | ||||
| * Returns the Config corresponding to the provider. | ||||
| * @return std::shared_ptr<const C> a shared pointer to the Config. | ||||
| */ | ||||
| template <typename C> std::shared_ptr<const C> config() { | ||||
| static_assert(std::is_base_of<Config, C>::value, | ||||
| "Config type must derive from ConfigProvider::Config"); | ||||
|
|
||||
| return std::dynamic_pointer_cast<const C>(getConfig()); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Returns the timestamp associated with the last update to the Config. | ||||
| * @return SystemTime the timestamp corresponding to the last config update. | ||||
| */ | ||||
| virtual SystemTime lastUpdated() const PURE; | ||||
|
|
||||
| protected: | ||||
| /** | ||||
| * Returns the config proto associated with the provider. | ||||
| * @return Protobuf::Message* the config proto corresponding to the Config instantiated by the | ||||
| * provider. | ||||
| */ | ||||
| virtual const Protobuf::Message* getConfigProto() const PURE; | ||||
|
|
||||
| /** | ||||
| * Returns the config version associated with the provider. | ||||
| * @return std::string the config version. | ||||
| */ | ||||
| virtual std::string getConfigVersion() const PURE; | ||||
|
|
||||
| /** | ||||
| * Returns the config implementation associated with the provider. | ||||
| * @return ConfigConstSharedPtr the config as the base type. | ||||
| */ | ||||
| virtual ConfigConstSharedPtr getConfig() const PURE; | ||||
| }; | ||||
|
|
||||
| using ConfigProviderPtr = std::unique_ptr<ConfigProvider>; | ||||
|
|
||||
| } // namespace Config | ||||
| } // namespace Envoy | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "envoy/config/config_provider.h" | ||
| #include "envoy/server/filter_config.h" | ||
|
|
||
| #include "common/protobuf/protobuf.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Config { | ||
|
|
||
| /** | ||
| * A ConfigProvider manager which instantiates static and dynamic (xDS) providers. | ||
|
lizan marked this conversation as resolved.
|
||
| * | ||
| * ConfigProvider objects are owned by the caller of the | ||
| * createXdsConfigProvider()/createStaticConfigProvider() functions. The ConfigProviderManager holds | ||
| * raw pointers to those objects. | ||
|
AndresGuedez marked this conversation as resolved.
|
||
| */ | ||
| class ConfigProviderManager { | ||
| public: | ||
| virtual ~ConfigProviderManager() {} | ||
|
|
||
| /** | ||
| * Returns a dynamic ConfigProvider which receives configuration via an xDS API. | ||
| * A shared ownership model is used, such that the underlying subscription, config proto | ||
| * and Config are shared amongst all providers relying on the same config source. | ||
| * @param config_source_proto supplies the proto containing the xDS API configuration. | ||
| * @param factory_context is the context to use for the provider. | ||
| * @param stat_prefix supplies the prefix to use for statistics. | ||
| * @return ConfigProviderPtr a newly allocated dynamic config provider which shares underlying | ||
| * data structures with other dynamic providers configured with the same | ||
| * API source. | ||
| */ | ||
| virtual ConfigProviderPtr | ||
| createXdsConfigProvider(const Protobuf::Message& config_source_proto, | ||
|
AndresGuedez marked this conversation as resolved.
|
||
| Server::Configuration::FactoryContext& factory_context, | ||
| const std::string& stat_prefix) PURE; | ||
|
|
||
| /** | ||
| * Returns a ConfigProvider associated with a statically specified configuration. | ||
| * @param config_proto supplies the configuration proto. | ||
| * @param factory_context is the context to use for the provider. | ||
| * @return ConfigProviderPtr a newly allocated static config provider. | ||
| */ | ||
| virtual ConfigProviderPtr | ||
| createStaticConfigProvider(const Protobuf::Message& config_proto, | ||
| Server::Configuration::FactoryContext& factory_context) PURE; | ||
| }; | ||
|
|
||
| } // namespace Config | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| #include "common/config/config_provider_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Config { | ||
|
|
||
| StaticConfigProviderImpl::StaticConfigProviderImpl( | ||
| Server::Configuration::FactoryContext& factory_context, | ||
| ConfigProviderManagerImpl& config_provider_manager) | ||
| : last_updated_(factory_context.timeSource().systemTime()), | ||
| config_provider_manager_(config_provider_manager) { | ||
| config_provider_manager_.static_config_providers_.insert(this); | ||
| } | ||
|
|
||
| StaticConfigProviderImpl::~StaticConfigProviderImpl() { | ||
| config_provider_manager_.static_config_providers_.erase(this); | ||
| } | ||
|
|
||
| ConfigSubscriptionInstance::~ConfigSubscriptionInstance() { | ||
| runInitializeCallbackIfAny(); | ||
| config_provider_manager_.unbindSubscription(manager_identifier_); | ||
| } | ||
|
|
||
| void ConfigSubscriptionInstance::runInitializeCallbackIfAny() { | ||
| if (initialize_callback_) { | ||
| initialize_callback_(); | ||
| initialize_callback_ = nullptr; | ||
| } | ||
| } | ||
|
|
||
| bool ConfigSubscriptionInstance::checkAndApplyConfig(const Protobuf::Message& config_proto, | ||
| const std::string& config_name, | ||
| const std::string& version_info) { | ||
| const uint64_t new_hash = MessageUtil::hash(config_proto); | ||
| if (config_info_ && config_info_.value().last_config_hash_ == new_hash) { | ||
| return false; | ||
| } | ||
|
|
||
| config_info_ = {new_hash, version_info}; | ||
| ENVOY_LOG(debug, "{}: loading new configuration: config_name={} hash={}", name_, config_name, | ||
| new_hash); | ||
|
|
||
| ASSERT(!dynamic_config_providers_.empty()); | ||
| ConfigProvider::ConfigConstSharedPtr new_config; | ||
| for (auto* provider : dynamic_config_providers_) { | ||
|
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. I'm scratching my head and trying to remember why we have/need multiple dynamic config providers for a single config proto update?
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. This is another carryover from the existing RDS implementation of config providers. Currently, each HttpConnectionManager instantiates its own RdsRouteConfigProviderImpl (if RDS is configured). I found some context on the decision to avoid sharing the RouteConfigProvider in #3960 and #3967. Based on those PRs, it appears the goal was to avoid use of shared_ptr outside the *ProviderImpl.
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. Yeah, anything you could do to document this design history would be great. I feel this is something I will forget in 6 months again.
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. Added class level comment explaining this.
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.
This is not quite true, see #3960 description. FactoryContext is tied to Listener (some of refs in it), and they will be dangling after they are updated by LDS, while the config provider may outlive them. |
||
| if (new_config == nullptr) { | ||
|
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. This loop looks wrong based on a quick glance; the
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. These dynamic providers are all instances of the same class, and they share the underlying ConfigSubscriptionInstance on which this function is taking effect. I will add the comments and assert()s.
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. Added comments and an ASSERT to bindConfigProvider(). The assertion is tricky due to the lack of concrete type information in these functions, so PTAL and let me know your thoughts on my approach using RTTI. |
||
| new_config = provider->onConfigProtoUpdate(config_proto); | ||
|
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. this could return a nullptr, right? If so should checkAndApply config still return true?
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. Good catch, thanks. Added error handling. |
||
| } | ||
| provider->onConfigUpdate(new_config); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| ConfigProviderManagerImpl::ConfigProviderManagerImpl(Server::Admin& admin, | ||
| const std::string& config_name) { | ||
| config_tracker_entry_ = | ||
| admin.getConfigTracker().add(config_name, [this] { return dumpConfigs(); }); | ||
| // ConfigTracker keys must be unique. We are asserting that no one has stolen the key | ||
| // from us, since the returned entry will be nullptr if the key already exists. | ||
| RELEASE_ASSERT(config_tracker_entry_, ""); | ||
| } | ||
|
|
||
| } // namespace Config | ||
| } // namespace Envoy | ||
Uh oh!
There was an error while loading. Please reload this page.