From 6c744cdcb32db8619d78f466655b1a72d00d2697 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 9 Nov 2018 16:26:43 -0500 Subject: [PATCH 01/25] Refactor RouteConfigProvider{,Manager} into a generalized framework. This commit introduces a framework for implementing configuration providers by generalizing the RouteConfigProvider{,Manager} which currently implement HttpConnectionManager routing configuration and the RDS API. This is the first deliverable (out of a planned 4 or 5 PRs) of the Scoped RDS implementation (#4704). Signed-off-by: Andres Guedez --- include/envoy/config/BUILD | 20 + include/envoy/config/config_provider.h | 98 +++++ .../envoy/config/config_provider_manager.h | 52 +++ source/common/config/BUILD | 17 + source/common/config/config_provider_impl.cc | 66 ++++ source/common/config/config_provider_impl.h | 323 ++++++++++++++++ test/common/config/BUILD | 17 + .../config/config_provider_impl_test.cc | 363 ++++++++++++++++++ test/common/config/dummy_config.proto | 31 ++ 9 files changed, 987 insertions(+) create mode 100644 include/envoy/config/config_provider.h create mode 100644 include/envoy/config/config_provider_manager.h create mode 100644 source/common/config/config_provider_impl.cc create mode 100644 source/common/config/config_provider_impl.h create mode 100644 test/common/config/config_provider_impl_test.cc create mode 100644 test/common/config/dummy_config.proto diff --git a/include/envoy/config/BUILD b/include/envoy/config/BUILD index bf35d5762ff5b..5c41821d10c80 100644 --- a/include/envoy/config/BUILD +++ b/include/envoy/config/BUILD @@ -33,3 +33,23 @@ envoy_cc_library( "//source/common/protobuf", ], ) + +envoy_cc_library( + name = "config_provider_interface", + hdrs = ["config_provider.h"], + external_deps = ["abseil_optional"], + deps = [ + "//include/envoy/common:time_interface", + "//source/common/protobuf", + ], +) + +envoy_cc_library( + name = "config_provider_manager_interface", + hdrs = ["config_provider_manager.h"], + deps = [ + ":config_provider_interface", + "//include/envoy/server:filter_config_interface", + "//source/common/protobuf", + ], +) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h new file mode 100644 index 0000000000000..027365fb9bd17 --- /dev/null +++ b/include/envoy/config/config_provider.h @@ -0,0 +1,98 @@ +#pragma once + +#include + +#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. + */ +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; + + /** + * Stores the config proto as well as the associated version. + */ + template struct ConfigInfo { + const P& config_; + + std::string version_; + }; + + virtual ~ConfigProvider() {} + + /** + * Returns ConfigInfo associated with the provider. + * @return absl::optional> an optional ConfigInfo; the value is set when a config is + * available. + */ + template absl::optional> configInfo() { + static_assert(std::is_base_of::value, + "Proto type must derive from Protobuf::Message"); + + const auto* config_proto = dynamic_cast(getConfigProto()); + if (!config_proto) { + return {}; + } + return ConfigInfo

{*config_proto, getConfigVersion()}; + } + + /** + * Returns the Config corresponding to the provider. + * @return std::shared_ptr a shared pointer to the Config. + */ + template std::shared_ptr config() { + static_assert(std::is_base_of::value, + "Config type must derive from ConfigProvider::Config"); + + return std::dynamic_pointer_cast(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 contained by the + * provider. + */ + virtual const Protobuf::Message* getConfigProto() const PURE; + + /** + * Returns the config version associated with dynamically delivered configuration. + * @return std::string the version associated with the config. + */ + 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; + +} // namespace Config +} // namespace Envoy diff --git a/include/envoy/config/config_provider_manager.h b/include/envoy/config/config_provider_manager.h new file mode 100644 index 0000000000000..be278daa7c099 --- /dev/null +++ b/include/envoy/config/config_provider_manager.h @@ -0,0 +1,52 @@ +#pragma once + +#include + +#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. + * + * ConfigProvider objects are owned by the caller of the + * createXdsConfigProvider()/createStaticConfigProvider() functions. The ConfigProviderManager holds + * raw pointers to those objects. + */ +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, + 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 diff --git a/source/common/config/BUILD b/source/common/config/BUILD index d2e34705c4479..723c351030dfb 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -366,3 +366,20 @@ envoy_cc_library( "//source/common/singleton:const_singleton", ], ) + +envoy_cc_library( + name = "config_provider_lib", + srcs = ["config_provider_impl.cc"], + hdrs = ["config_provider_impl.h"], + deps = [ + ":utility_lib", + "//include/envoy/config:config_provider_interface", + "//include/envoy/config:config_provider_manager_interface", + "//include/envoy/init:init_interface", + "//include/envoy/server:admin_interface", + "//include/envoy/server:config_tracker_interface", + "//include/envoy/singleton:instance_interface", + "//include/envoy/thread_local:thread_local_interface", + "//source/common/protobuf", + ], +) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc new file mode 100644 index 0000000000000..27cd590f05ef6 --- /dev/null +++ b/source/common/config/config_provider_impl.cc @@ -0,0 +1,66 @@ +#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}; + // route_config_proto_ = route_config; + // stats_.config_reload_.inc(); + ENVOY_LOG(debug, "{}: loading new configuration: config_name={} hash={}", name_, config_name, + new_hash); + // TODO(aguedez): let's share the actual config impl as well as the proto... + ASSERT(!dynamic_config_providers_.empty()); + ConfigProvider::ConfigConstSharedPtr new_config; + for (auto* provider : dynamic_config_providers_) { + if (new_config == nullptr) { + new_config = provider->onConfigProtoUpdate(config_proto); + } + 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 diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h new file mode 100644 index 0000000000000..45384dfe1e2bf --- /dev/null +++ b/source/common/config/config_provider_impl.h @@ -0,0 +1,323 @@ +#pragma once + +#include + +#include "envoy/config/config_provider.h" +#include "envoy/config/config_provider_manager.h" +#include "envoy/init/init.h" +#include "envoy/server/admin.h" +#include "envoy/server/config_tracker.h" +#include "envoy/singleton/instance.h" +#include "envoy/thread_local/thread_local.h" + +#include "common/config/utility.h" +#include "common/protobuf/protobuf.h" + +namespace Envoy { +namespace Config { + +class ConfigProviderManagerImpl; + +/** + * ConfigProvider implementation for statically specified configuration. + * + * This class can not instantiated directly; instead, it provides the foundation for + * static config provider implementations which derive from it. + */ +class StaticConfigProviderImpl : public ConfigProvider { +public: + StaticConfigProviderImpl() = delete; + + virtual ~StaticConfigProviderImpl(); + + // Envoy::Config::ConfigProvider + SystemTime lastUpdated() const override { return last_updated_; } + +protected: + StaticConfigProviderImpl(Server::Configuration::FactoryContext& factory_context, + ConfigProviderManagerImpl& config_provider_manager); + +private: + SystemTime last_updated_; + ConfigProviderManagerImpl& config_provider_manager_; +}; + +class DynamicConfigProviderImpl; + +/** + * Provides generic functionality required by all dynamic ConfigProvider subscriptions, including + * shared lifetime management via shared_ptr. + * + * This class can not be instantiated directly; instead, it provides the foundation for + * dynamic config subscription implementations which derive from it. + */ +class ConfigSubscriptionInstance : public Init::Target, + protected Logger::Loggable { +public: + struct LastConfigInfo { + uint64_t last_config_hash_; + std::string last_config_version_; + }; + + ConfigSubscriptionInstance() = delete; + + virtual ~ConfigSubscriptionInstance(); + + // Init::Target + void initialize(std::function callback) override { + initialize_callback_ = callback; + start(); + } + + /** + * Starts the subscription corresponding to a config source. + * A derived class must manage the configuration proto specific Envoy::Config::Subscription to be + * started. + */ + virtual void start() PURE; + + const SystemTime& lastUpdated() const { return last_updated_; } + + const absl::optional& configInfo() const { return config_info_; } + + /** + * Must be called by derived classes when the onConfigUpdate() callback associated with the + * underlying subscription is issued. + */ + void onConfigUpdate() { + setLastUpdated(); + runInitializeCallbackIfAny(); + } + + /** + * Must be called by derived classes when the onConfigUpdateFailed() callback associated with the + * underlying subscription is issued. + */ + void onConfigUpdateFailed() { runInitializeCallbackIfAny(); } + + /** + * Determines whether a configuration proto is a new update, and if so, propagates it to all + * config providers associated with this subscription. + * @param config_proto supplies the newly received config proto. + * @param config_name supplies the name associated with the config. + * @param version_info supplies the version associated with the config. + * @return bool false when the config proto has no delta from the previous config, true otherwise. + */ + bool checkAndApplyConfig(const Protobuf::Message& config_proto, const std::string& config_name, + const std::string& version_info); + + const std::unordered_set dynamic_config_providers() const { + return std::unordered_set(dynamic_config_providers_.begin(), + dynamic_config_providers_.end()); + } + +protected: + ConfigSubscriptionInstance(const std::string& name, const std::string& manager_identifier, + ConfigProviderManagerImpl& config_provider_manager, + TimeSource& time_source, const SystemTime& last_updated, + const LocalInfo::LocalInfo& local_info) + : name_(name), manager_identifier_(manager_identifier), + config_provider_manager_(config_provider_manager), time_source_(time_source), + last_updated_(last_updated) { + Envoy::Config::Utility::checkLocalInfo(name, local_info); + } + + void setLastUpdated() { last_updated_ = time_source_.systemTime(); } + + void runInitializeCallbackIfAny(); + +private: + void registerInitTarget(Init::Manager& init_manager) { init_manager.registerTarget(*this); } + + void bindConfigProvider(DynamicConfigProviderImpl* provider) { + dynamic_config_providers_.insert(provider); + } + + void unbindConfigProvider(DynamicConfigProviderImpl* provider) { + dynamic_config_providers_.erase(provider); + } + + const std::string name_; + std::function initialize_callback_; + std::unordered_set dynamic_config_providers_; + const std::string manager_identifier_; + ConfigProviderManagerImpl& config_provider_manager_; + TimeSource& time_source_; + SystemTime last_updated_; + absl::optional config_info_; + + // Subscriptions, dynamic config providers and config provider managers are tightly coupled with + // the current shared ownership model; use friend classes to explicitly denote the binding between + // them. + // + // TODO(AndresGuedez): Investigate whether a shared ownership model avoiding the s and + // instead centralizing lifetime management in the ConfigProviderManagerImpl with explicit + // reference counting would be more maintainable. + friend class DynamicConfigProviderImpl; + friend class ConfigProviderManagerImpl; +}; + +using ConfigSubscriptionInstanceSharedPtr = std::shared_ptr; + +/** + * Provides generic functionality required by all dynamic config providers, including distribution + * of config updates to all workers. + * + * This class can not be instantiated directly; instead, it provides the foundation for + * dynamic config provider implementations which derive from it. + */ +class DynamicConfigProviderImpl : public ConfigProvider { +public: + DynamicConfigProviderImpl() = delete; + + virtual ~DynamicConfigProviderImpl() { subscription_->unbindConfigProvider(this); } + + // Envoy::Config::ConfigProvider + SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } + + // Envoy::Config::ConfigProvider + ConfigConstSharedPtr getConfig() const override { + return tls_->getTyped().config_; + } + + virtual ConfigConstSharedPtr onConfigProtoUpdate(const Protobuf::Message& config_proto) PURE; + + /** + * Must be called by the derived class' constructor. + * @param initial_config supplies an initial Envoy::Config::ConfigProvider::Config associated with + * the underlying subscription. + */ + void initialize(ConfigConstSharedPtr initial_config) { + subscription_->bindConfigProvider(this); + tls_->set([initial_config](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { + return std::make_shared(initial_config); + }); + } + + /** + * Propagates a newly instantiated Envoy::Config::ConfigProvider::Config to all workers. + * @param config supplies the newly instantiated config. + */ + void onConfigUpdate(ConfigConstSharedPtr config) { + tls_->runOnAllThreads( + [this, config]() -> void { tls_->getTyped().config_ = config; }); + } + +protected: + DynamicConfigProviderImpl(ConfigSubscriptionInstanceSharedPtr&& subscription, + Server::Configuration::FactoryContext& factory_context) + : subscription_(subscription), tls_(factory_context.threadLocal().allocateSlot()) {} + + const ConfigSubscriptionInstanceSharedPtr& subscription() const { return subscription_; } + +private: + struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { + ThreadLocalConfig(ConfigProvider::ConfigConstSharedPtr initial_config) + : config_(initial_config) {} + + ConfigProvider::ConfigConstSharedPtr config_; + }; + + ConfigSubscriptionInstanceSharedPtr subscription_; + ThreadLocal::SlotPtr tls_; +}; + +/** + * Provides generic functionality required by all config provider managers, such as managing shared + * lifetime of subscriptions and dynamic config providers, along with determining which + * subscriptions should be associated with newly instantiated providers. + * + * This class can not be instantiated directly; instead, it provides the foundation for + * dynamic config provider implementations which derive from it. + */ +class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton::Instance { +public: + ConfigProviderManagerImpl() = delete; + + virtual ~ConfigProviderManagerImpl() {} + + /** + * This is invoked by the /config_dump admin handler. + * @return ProtobufTypes::MessagePtr the config dump proto corresponding to the associated + * {static,dynamic} config providers. + */ + virtual ProtobufTypes::MessagePtr dumpConfigs() const PURE; + +protected: + using ConfigProviderSet = std::unordered_set; + using ConfigSubscriptionMap = + std::unordered_map>; + + ConfigProviderManagerImpl(Server::Admin& admin, const std::string& config_name); + + const ConfigProviderSet static_config_providers() const { return static_config_providers_; } + + const ConfigSubscriptionMap& config_subscriptions() const { return config_subscriptions_; } + + /** + * Returns the subscription associated with the config_source_proto; if none exists, a new one is + * allocated according to the subscription_factory_fn. + * @param config_source_proto supplies the proto specifying the config subscription paremeters. + * @param init_manager supplies the init manager. + * @param subscription_factory_fn supplies a function to be called when a new subscription needs + * to be allocated. + * @return std::shared_ptr an existing (if a match is found) or newly allocated subscription. + */ + template + std::shared_ptr + getSubscription(const Protobuf::Message& config_source_proto, Init::Manager& init_manager, + std::function + subscription_factory_fn) { + static_assert(std::is_base_of::value, + "T must be a subclass of ConfigSubscriptionInstance"); + + ConfigSubscriptionInstanceSharedPtr subscription; + const std::string manager_identifier = config_source_proto.SerializeAsString(); + + auto it = config_subscriptions_.find(manager_identifier); + if (it == config_subscriptions_.end()) { + // std::make_shared does not work for classes with private constructors. There are ways + // around it. However, since this is not a performance critical path we err on the side + // of simplicity. + subscription = subscription_factory_fn(manager_identifier, *this); + + subscription->registerInitTarget(init_manager); + + bindSubscription(manager_identifier, subscription); + } else { + // Because the RouteConfigProviderManager's weak_ptrs only get cleaned up + // in the RdsRouteConfigSubscription destructor, and the single threaded nature + // of this code, locking the weak_ptr will not fail. + subscription = it->second.lock(); + } + ASSERT(subscription); + + return std::static_pointer_cast(subscription); + } + +private: + void bindSubscription(const std::string& manager_identifier, + ConfigSubscriptionInstanceSharedPtr& subscription) { + config_subscriptions_.insert({manager_identifier, subscription}); + } + + void unbindSubscription(const std::string& manager_identifier) { + config_subscriptions_.erase(manager_identifier); + } + + // TODO(jsedgwick) These two members are prime candidates for the owned-entry list/map + // as in ConfigTracker. I.e. the ProviderImpls would have an EntryOwner for these lists + // Then the lifetime management stuff is centralized and opaque. + ConfigSubscriptionMap config_subscriptions_; + ConfigProviderSet static_config_providers_; + Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; + + // See comment for friend classes in the ConfigSubscriptionInstance for more details on the use + // of friends. + friend class ConfigSubscriptionInstance; + friend class StaticConfigProviderImpl; +}; + +} // namespace Config +} // namespace Envoy diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 918e263d850a9..d80c5b07dd781 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -5,6 +5,7 @@ load( "envoy_cc_test", "envoy_cc_test_library", "envoy_package", + "envoy_proto_library", ) envoy_package() @@ -208,3 +209,19 @@ envoy_cc_test( "//source/common/config:filter_json_lib", ], ) + +envoy_proto_library( + name = "dummy_config_proto", + srcs = ["dummy_config.proto"], +) + +envoy_cc_test( + name = "config_provider_impl_test", + srcs = ["config_provider_impl_test.cc"], + deps = [ + ":dummy_config_proto_cc", + "//source/common/config:config_provider_lib", + "//source/common/protobuf:utility_lib", + "//test/mocks/server:server_mocks", + ], +) diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc new file mode 100644 index 0000000000000..ca1a2459c6381 --- /dev/null +++ b/test/common/config/config_provider_impl_test.cc @@ -0,0 +1,363 @@ +#include + +#include "common/config/config_provider_impl.h" +#include "common/protobuf/utility.h" + +#include "test/common/config/dummy_config.pb.h" +#include "test/mocks/server/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Config { +namespace { + +class DummyConfigProviderManager; + +class StaticDummyConfigProvider : public Envoy::Config::StaticConfigProviderImpl { +public: + StaticDummyConfigProvider(const test::common::config::DummyConfig& config_proto, + Server::Configuration::FactoryContext& factory_context, + DummyConfigProviderManager& config_provider_manager); + + ~StaticDummyConfigProvider() {} + + // Envoy::Config::ConfigProvider + const Protobuf::Message* getConfigProto() const override { return &config_proto_; } + + // Envoy::Config::ConfigProvider + std::string getConfigVersion() const override { return ""; } + + // Envoy::Config::ConfigProvider + ConfigConstSharedPtr getConfig() const override { return config_; } + +private: + ConfigConstSharedPtr config_; + test::common::config::DummyConfig config_proto_; +}; + +class DummyConfigSubscription + : public ConfigSubscriptionInstance, + Envoy::Config::SubscriptionCallbacks { +public: + DummyConfigSubscription(const std::string& manager_identifier, + Server::Configuration::FactoryContext& factory_context, + DummyConfigProviderManager& config_provider_manager); + + ~DummyConfigSubscription() {} + + // Envoy::Config::ConfigSubscriptionInstance + void start() override {} + + // Envoy::Config::SubscriptionCallbacks + void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override { + const auto& config = resources[0]; + if (checkAndApplyConfig(config, "dummy_config", version_info)) { + config_proto_ = config; + } + + ConfigSubscriptionInstance::onConfigUpdate(); + } + + // Envoy::Config::SubscriptionCallbacks + void onConfigUpdateFailed(const EnvoyException*) override {} + + // Envoy::Config::SubscriptionCallbacks + std::string resourceName(const ProtobufWkt::Any&) override { return ""; } + + const absl::optional& config_proto() const { + return config_proto_; + } + +private: + absl::optional config_proto_; +}; + +using DummyConfigSubscriptionSharedPtr = std::shared_ptr; + +class DummyConfig : public ConfigProvider::Config { +public: + DummyConfig(const test::common::config::DummyConfig&) {} +}; + +class DummyDynamicConfigProvider : public DynamicConfigProviderImpl { +public: + DummyDynamicConfigProvider(DummyConfigSubscriptionSharedPtr&& subscription, + ConfigConstSharedPtr initial_config, + Server::Configuration::FactoryContext& factory_context) + : DynamicConfigProviderImpl(std::move(subscription), factory_context), + subscription_(static_cast( + DynamicConfigProviderImpl::subscription().get())) { + initialize(initial_config); + } + + ~DummyDynamicConfigProvider() {} + + DummyConfigSubscription& subscription() { return *subscription_; } + + // Envoy::Config::DynamicConfigProviderImpl + ConfigProvider::ConfigConstSharedPtr + onConfigProtoUpdate(const Protobuf::Message& config) override { + return std::make_shared( + static_cast(config)); + } + + // Envoy::Config::ConfigProvider + const Protobuf::Message* getConfigProto() const override { + if (!subscription_->config_proto().has_value()) { + return nullptr; + } + return &subscription_->config_proto().value(); + } + + // Envoy::Config::ConfigProvider + std::string getConfigVersion() const override { return ""; } + +private: + // Lifetime of this pointer is owned by the shared_ptr held by the base class. + DummyConfigSubscription* subscription_; +}; + +class DummyConfigProviderManager : public ConfigProviderManagerImpl { +public: + DummyConfigProviderManager(Server::Admin& admin) : ConfigProviderManagerImpl(admin, "dummy") {} + + ~DummyConfigProviderManager() {} + + // Envoy::Config::ConfigProviderManagerImpl + ProtobufTypes::MessagePtr dumpConfigs() const override { + auto config_dump = std::make_unique(); + for (const auto& element : config_subscriptions()) { + auto subscription = element.second.lock(); + ASSERT(subscription); + + if (subscription->configInfo()) { + auto* dynamic_config = config_dump->mutable_dynamic_dummy_configs()->Add(); + dynamic_config->set_version_info(subscription->configInfo().value().last_config_version_); + dynamic_config->mutable_dummy_config()->MergeFrom( + static_cast(subscription.get())->config_proto().value()); + TimestampUtil::systemClockToTimestamp(subscription->lastUpdated(), + *dynamic_config->mutable_last_updated()); + } + } + + for (const auto& provider : static_config_providers()) { + ASSERT(provider->configInfo()); + auto* static_config = config_dump->mutable_static_dummy_configs()->Add(); + static_config->mutable_dummy_config()->MergeFrom( + provider->configInfo().value().config_); + TimestampUtil::systemClockToTimestamp(provider->lastUpdated(), + *static_config->mutable_last_updated()); + } + + return config_dump; + } + + // Envoy::Config::ConfigProviderManager + ConfigProviderPtr createXdsConfigProvider(const Protobuf::Message& config_source_proto, + Server::Configuration::FactoryContext& factory_context, + const std::string&) override { + DummyConfigSubscriptionSharedPtr subscription = getSubscription( + config_source_proto, factory_context.initManager(), + [&factory_context](const std::string& manager_identifier, + ConfigProviderManagerImpl& config_provider_manager) + -> ConfigSubscriptionInstanceSharedPtr { + return std::make_shared( + manager_identifier, factory_context, + static_cast(config_provider_manager)); + }); + + ConfigProvider::ConfigConstSharedPtr initial_config; + if (!subscription->dynamic_config_providers().empty()) { + initial_config = (*subscription->dynamic_config_providers().begin())->getConfig(); + } + return std::make_unique(std::move(subscription), initial_config, + factory_context); + } + + // Envoy::Config::ConfigProviderManager + ConfigProviderPtr + createStaticConfigProvider(const Protobuf::Message& config_proto, + Server::Configuration::FactoryContext& factory_context) override { + return std::make_unique( + dynamic_cast(config_proto), factory_context, + *this); + } +}; + +StaticDummyConfigProvider::StaticDummyConfigProvider( + const test::common::config::DummyConfig& config_proto, + Server::Configuration::FactoryContext& factory_context, + DummyConfigProviderManager& config_provider_manager) + : StaticConfigProviderImpl(factory_context, config_provider_manager), + config_(std::make_shared(config_proto)), config_proto_(config_proto) {} + +DummyConfigSubscription::DummyConfigSubscription( + const std::string& manager_identifier, Server::Configuration::FactoryContext& factory_context, + DummyConfigProviderManager& config_provider_manager) + : ConfigSubscriptionInstance( + "DummyDS", manager_identifier, config_provider_manager, factory_context.timeSource(), + factory_context.timeSource().systemTime(), factory_context.localInfo()) {} + +class ConfigProviderImplTest : public testing::Test { +public: + ConfigProviderImplTest() { + EXPECT_CALL(factory_context_.admin_.config_tracker_, add_("dummy", _)); + provider_manager_ = std::make_unique(factory_context_.admin_); + } + + Event::SimulatedTimeSystem& timeSystem() { return factory_context_.timeSystem(); } + +protected: + NiceMock factory_context_; + std::unique_ptr provider_manager_; +}; + +test::common::config::DummyConfig parseDummyConfigFromYaml(const std::string& yaml) { + test::common::config::DummyConfig config; + MessageUtil::loadFromYaml(yaml, config); + return config; +} + +// Tests that dynamic config providers share ownership of the config +// subscriptions, config protos and data structures generated as a result of the +// configurations (i.e., the ConfigProvider::Config). +TEST_F(ConfigProviderImplTest, SharedOwnership) { + factory_context_.init_manager_.initialize(); + + envoy::api::v2::core::ApiConfigSource config_source_proto; + config_source_proto.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + ConfigProviderPtr provider1 = provider_manager_->createXdsConfigProvider( + config_source_proto, factory_context_, "dummy_prefix"); + + // No config protos have been received via the subscription yet. + EXPECT_FALSE(provider1->configInfo().has_value()); + + Protobuf::RepeatedPtrField dummy_configs; + dummy_configs.Add()->MergeFrom(parseDummyConfigFromYaml("a: a dummy config")); + + DummyConfigSubscription& subscription = + dynamic_cast(*provider1).subscription(); + subscription.onConfigUpdate(dummy_configs, "1"); + + // Check that a newly created provider with the same config source will share + // the subscription, config proto and resulting ConfigProvider::Config. + ConfigProviderPtr provider2 = provider_manager_->createXdsConfigProvider( + config_source_proto, factory_context_, "dummy_prefix"); + + EXPECT_TRUE(provider2->configInfo().has_value()); + EXPECT_EQ(&dynamic_cast(*provider1).subscription(), + &dynamic_cast(*provider2).subscription()); + EXPECT_EQ(&provider1->configInfo().value().config_, + &provider2->configInfo().value().config_); + EXPECT_EQ(provider1->config().get(), + provider2->config().get()); + + // Change the config source and verify that a new subscription is used. + config_source_proto.set_api_type(envoy::api::v2::core::ApiConfigSource::REST); + ConfigProviderPtr provider3 = provider_manager_->createXdsConfigProvider( + config_source_proto, factory_context_, "dummy_prefix"); + + EXPECT_NE(&dynamic_cast(*provider1).subscription(), + &dynamic_cast(*provider3).subscription()); + EXPECT_NE(provider1->config().get(), + provider3->config().get()); + + dynamic_cast(*provider3) + .subscription() + .onConfigUpdate(dummy_configs, "provider3"); + + EXPECT_EQ(2UL, static_cast( + provider_manager_->dumpConfigs().get()) + ->dynamic_dummy_configs() + .size()); + + // Test that tear down of config providers leads to correctly updating + // centralized state; this is validated using the config dump. + provider1.reset(); + provider2.reset(); + + auto dynamic_dummy_configs = + static_cast(provider_manager_->dumpConfigs().get()) + ->dynamic_dummy_configs(); + EXPECT_EQ(1UL, dynamic_dummy_configs.size()); + + EXPECT_EQ("provider3", dynamic_dummy_configs[0].version_info()); + + provider3.reset(); + + EXPECT_EQ(0UL, static_cast( + provider_manager_->dumpConfigs().get()) + ->dynamic_dummy_configs() + .size()); +} + +// Tests that the base ConfigProvider*s are handling registration with the +// /config_dump admin handler as well as generic bookkeeping such as timestamp +// updates. +TEST_F(ConfigProviderImplTest, ConfigDump) { + // Empty dump first. + auto message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["dummy"](); + const auto& dummy_config_dump = + static_cast(*message_ptr); + + test::common::config::DummyConfigsDump expected_config_dump; + MessageUtil::loadFromYaml(R"EOF( +static_dummy_configs: +dynamic_dummy_configs: +)EOF", + expected_config_dump); + EXPECT_EQ(expected_config_dump.DebugString(), dummy_config_dump.DebugString()); + + // Static config dump only. + std::string config_yaml = "a: a static dummy config"; + timeSystem().setSystemTime(std::chrono::milliseconds(1234567891234)); + + ConfigProviderPtr static_config = provider_manager_->createStaticConfigProvider( + parseDummyConfigFromYaml(config_yaml), factory_context_); + message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["dummy"](); + const auto& dummy_config_dump2 = + static_cast(*message_ptr); + MessageUtil::loadFromYaml(R"EOF( +static_dummy_configs: + - dummy_config: { a: a static dummy config } + last_updated: { seconds: 1234567891, nanos: 234000000 } +dynamic_dummy_configs: +)EOF", + expected_config_dump); + EXPECT_EQ(expected_config_dump.DebugString(), dummy_config_dump2.DebugString()); + + envoy::api::v2::core::ApiConfigSource config_source_proto; + config_source_proto.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + ConfigProviderPtr dynamic_provider = provider_manager_->createXdsConfigProvider( + config_source_proto, factory_context_, "dummy_prefix"); + + // Static + dynamic config dump. + Protobuf::RepeatedPtrField dummy_configs; + dummy_configs.Add()->MergeFrom(parseDummyConfigFromYaml("a: a dynamic dummy config")); + + timeSystem().setSystemTime(std::chrono::milliseconds(1234567891567)); + DummyConfigSubscription& subscription = + dynamic_cast(*dynamic_provider).subscription(); + subscription.onConfigUpdate(dummy_configs, "v1"); + + message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["dummy"](); + const auto& dummy_config_dump3 = + static_cast(*message_ptr); + MessageUtil::loadFromYaml(R"EOF( +static_dummy_configs: + - dummy_config: { a: a static dummy config } + last_updated: { seconds: 1234567891, nanos: 234000000 } +dynamic_dummy_configs: + - version_info: v1 + dummy_config: { a: a dynamic dummy config } + last_updated: { seconds: 1234567891, nanos: 567000000 } +)EOF", + expected_config_dump); + EXPECT_EQ(expected_config_dump.DebugString(), dummy_config_dump3.DebugString()); +} + +} // namespace +} // namespace Config +} // namespace Envoy diff --git a/test/common/config/dummy_config.proto b/test/common/config/dummy_config.proto new file mode 100644 index 0000000000000..9964bd76fd36f --- /dev/null +++ b/test/common/config/dummy_config.proto @@ -0,0 +1,31 @@ +// Provides protos for testing source/common/config/config_provider_impl.{h,cc}. + +syntax = "proto3"; + +package test.common.config; + +import "google/protobuf/timestamp.proto"; + +message DummyConfig { + string a = 1; +} + +message DummyConfigsDump { + message StaticConfigs { + DummyConfig dummy_config = 1; + + google.protobuf.Timestamp last_updated = 2; + } + + message DynamicConfigs { + string version_info = 1; + + DummyConfig dummy_config = 2; + + google.protobuf.Timestamp last_updated = 3; + } + + repeated StaticConfigs static_dummy_configs = 1; + + repeated DynamicConfigs dynamic_dummy_configs = 2; +} From 1bccfafcfd9fa23e9f3e5a836499c494f8a705ed Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 6 Dec 2018 22:28:57 -0500 Subject: [PATCH 02/25] Add comments. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 45384dfe1e2bf..d38d7944edb90 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -16,12 +16,28 @@ namespace Envoy { namespace Config { +// This file provides a set of base classes, (StaticConfigProviderImpl, DynamicConfigProviderImpl, +// ConfigProviderManagerImpl, ConfigSubscriptionInstance), conforming to the +// ConfigProvider/ConfigProviderManager interfaces, which in tandem provide a framework for +// implementing static and dynamic configuration for Envoy. +// +// Envoy's dynamic configuration is distributed via xDS APIs (see +// https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md). The framework exposed +// by the classes below enables client xDS implementations following a shared ownership model, where +// according to the config source specification, a config subscription to the API server, config +// protos received over the subscription and the subsequent config "implementation" (i.e., data +// structures and associated business logic) are shared across ConfigProvider objects and Envoy +// worker threads. +// +// This approach enables linear configuration scalability based primarily on the size of the +// configuration set. + class ConfigProviderManagerImpl; /** * ConfigProvider implementation for statically specified configuration. * - * This class can not instantiated directly; instead, it provides the foundation for + * This class can not be instantiated directly; instead, it provides the foundation for * static config provider implementations which derive from it. */ class StaticConfigProviderImpl : public ConfigProvider { From 66e4c45a07f6047b482d71b6d32c1eb2f087c27f Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 6 Dec 2018 22:53:58 -0500 Subject: [PATCH 03/25] Minor cleanup. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 27cd590f05ef6..ab469991a712c 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -36,11 +36,9 @@ bool ConfigSubscriptionInstance::checkAndApplyConfig(const Protobuf::Message& co } config_info_ = {new_hash, version_info}; - // route_config_proto_ = route_config; - // stats_.config_reload_.inc(); ENVOY_LOG(debug, "{}: loading new configuration: config_name={} hash={}", name_, config_name, new_hash); - // TODO(aguedez): let's share the actual config impl as well as the proto... + ASSERT(!dynamic_config_providers_.empty()); ConfigProvider::ConfigConstSharedPtr new_config; for (auto* provider : dynamic_config_providers_) { From 02b5e973f066143ef31ea33e775d3e52149bf470 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 7 Dec 2018 10:25:08 -0500 Subject: [PATCH 04/25] More comments. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.h | 40 ++++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index d38d7944edb90..2a2bf7b324d89 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -21,16 +21,38 @@ namespace Config { // ConfigProvider/ConfigProviderManager interfaces, which in tandem provide a framework for // implementing static and dynamic configuration for Envoy. // -// Envoy's dynamic configuration is distributed via xDS APIs (see +// Dynamic configuration is distributed via xDS APIs (see // https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md). The framework exposed -// by the classes below enables client xDS implementations following a shared ownership model, where -// according to the config source specification, a config subscription to the API server, config -// protos received over the subscription and the subsequent config "implementation" (i.e., data -// structures and associated business logic) are shared across ConfigProvider objects and Envoy -// worker threads. +// by these classes simplifies creation of client xDS implementations following a shared ownership +// model, where according to the config source specification, a config subscription, config protos +// received over the subscription and the subsequent config "implementation" (i.e., data structures +// and associated business logic) are shared across ConfigProvider objects and Envoy worker threads. // -// This approach enables linear configuration scalability based primarily on the size of the -// configuration set. +// This approach enables linear memory scalability based primarily on the size of the configuration +// set. +// +// A blueprint to follow for implementing static and dynamic config providers is as follows: +// +// For both: +// 1) Create a class derived from ConfigProviderManagerImpl and implement the required interface. +// When implementing createXdsConfigProvider(), it is expected that getSubscription() will +// be called to fetch either an existing ConfigSubscriptionInstance if the config source +// configuration matches, or a newly instantiated subscription otherwise. +// +// For static providers: +// 1) Create a class derived from StaticConfigProviderImpl and implement the required interface. +// +// For dynamic (xDS) providers: +// 1) Create a class derived from DynamicConfigProviderImpl and implement the required interface. +// 2) Create a class derived from ConfigSubscriptionInstance; this is the entity responsible for +// owning and managing the Envoy::Config::Subscription that provides the underlying +// config subscription. +// - When subscription callbacks (onConfigUpdate, onConfigUpdateFailed) are issued by the +// underlying subscription, the corresponding ConfigSubscriptionInstance functions must be +// called as well. +// - On a successful config update, checkAndApplyConfig() should be called to instantiate the +// new config implementation and propagate it to the shared config providers and all +// worker threads. class ConfigProviderManagerImpl; @@ -87,7 +109,7 @@ class ConfigSubscriptionInstance : public Init::Target, /** * Starts the subscription corresponding to a config source. - * A derived class must manage the configuration proto specific Envoy::Config::Subscription to be + * A derived class must own the configuration proto specific Envoy::Config::Subscription to be * started. */ virtual void start() PURE; From 09a0846247cef4661dfc4e86eb2c86faf6a142b5 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 7 Dec 2018 10:54:17 -0500 Subject: [PATCH 05/25] Comments. Signed-off-by: Andres Guedez --- include/envoy/config/config_provider.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index 027365fb9bd17..2686c4e7121e4 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -39,7 +39,7 @@ class ConfigProvider { virtual ~ConfigProvider() {} /** - * Returns ConfigInfo associated with the provider. + * Returns a ConfigInfo associated with the provider. * @return absl::optional> an optional ConfigInfo; the value is set when a config is * available. */ @@ -74,14 +74,14 @@ class ConfigProvider { protected: /** * Returns the config proto associated with the provider. - * @return Protobuf::Message* the config proto corresponding to the Config contained by the + * @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 dynamically delivered configuration. - * @return std::string the version associated with the config. + * Returns the config version associated with the provider. + * @return std::string the config version. */ virtual std::string getConfigVersion() const PURE; From acf19cf4e5d462f3587a4cd6817717dbda1ce55a Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Dec 2018 13:17:37 -0500 Subject: [PATCH 06/25] Minor const correctness fixes. Signed-off-by: Andres Guedez --- include/envoy/config/config_provider.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index 2686c4e7121e4..c514741a941a6 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -43,7 +43,7 @@ class ConfigProvider { * @return absl::optional> an optional ConfigInfo; the value is set when a config is * available. */ - template absl::optional> configInfo() { + template absl::optional> configInfo() const { static_assert(std::is_base_of::value, "Proto type must derive from Protobuf::Message"); @@ -58,7 +58,7 @@ class ConfigProvider { * Returns the Config corresponding to the provider. * @return std::shared_ptr a shared pointer to the Config. */ - template std::shared_ptr config() { + template std::shared_ptr config() const { static_assert(std::is_base_of::value, "Config type must derive from ConfigProvider::Config"); From c0366a73e7a95e73eef64fa035ff901390af0e18 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Dec 2018 14:59:21 -0500 Subject: [PATCH 07/25] Add test. Signed-off-by: Andres Guedez --- test/common/config/config_provider_impl_test.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index ca1a2459c6381..f177520721ac2 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -358,6 +358,23 @@ TEST_F(ConfigProviderImplTest, ConfigDump) { EXPECT_EQ(expected_config_dump.DebugString(), dummy_config_dump3.DebugString()); } +// Tests that dynamic config providers enforce that the context's localInfo is +// set, since it is used to obtain the node/cluster attributes required for +// subscriptions. +TEST_F(ConfigProviderImplTest, LocalInfoNotDefined) { + factory_context_.local_info_.node_.set_cluster(""); + factory_context_.local_info_.node_.set_id(""); + + envoy::api::v2::core::ApiConfigSource config_source_proto; + config_source_proto.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + EXPECT_THROW_WITH_MESSAGE( + provider_manager_->createXdsConfigProvider(config_source_proto, factory_context_, + "dummy_prefix"), + EnvoyException, + "DummyDS: node 'id' and 'cluster' are required. Set it either in 'node' config or " + "via --service-node and --service-cluster options."); +} + } // namespace } // namespace Config } // namespace Envoy From 4b1bef41ccba581723b7b26ce58352b19bc8a570 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 14 Dec 2018 16:06:18 -0500 Subject: [PATCH 08/25] Comments, general cleanup and renaming. Signed-off-by: Andres Guedez --- include/envoy/config/config_provider.h | 21 ++++++++------ .../envoy/config/config_provider_manager.h | 6 ++++ source/common/common/assert.h | 28 +++++++++++++++++++ source/common/config/config_provider_impl.cc | 12 ++++++++ source/common/config/config_provider_impl.h | 6 ++-- .../config/config_provider_impl_test.cc | 12 ++++---- 6 files changed, 67 insertions(+), 18 deletions(-) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index c514741a941a6..6bfb6012bb204 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -13,6 +13,10 @@ namespace Config { /** * A provider for configuration obtained ether statically or dynamically via xDS APIs. + * + * Use config() to obtain a shared_ptr to the implementation of the config, and configProtoInfo() to + * obtain a reference to the underlying config proto and version (applicable only to dynamic config + * providers). */ class ConfigProvider { public: @@ -30,28 +34,29 @@ class ConfigProvider { /** * Stores the config proto as well as the associated version. */ - template struct ConfigInfo { - const P& config_; + template struct ConfigProtoInfo { + const P& config_proto_; + // Only populated by dynamic config providers. std::string version_; }; virtual ~ConfigProvider() {} /** - * Returns a ConfigInfo associated with the provider. - * @return absl::optional> an optional ConfigInfo; the value is set when a config is - * available. + * Returns a ConfigProtoInfo associated with the provider. + * @return absl::optional> an optional ConfigProtoInfo; the value is set when a + * config is available. */ - template absl::optional> configInfo() const { + template absl::optional> configProtoInfo() const { static_assert(std::is_base_of::value, "Proto type must derive from Protobuf::Message"); const auto* config_proto = dynamic_cast(getConfigProto()); - if (!config_proto) { + if (config_proto == nullptr) { return {}; } - return ConfigInfo

{*config_proto, getConfigVersion()}; + return ConfigProtoInfo

{*config_proto, getConfigVersion()}; } /** diff --git a/include/envoy/config/config_provider_manager.h b/include/envoy/config/config_provider_manager.h index be278daa7c099..1ab8449eeac5d 100644 --- a/include/envoy/config/config_provider_manager.h +++ b/include/envoy/config/config_provider_manager.h @@ -16,6 +16,12 @@ namespace Config { * ConfigProvider objects are owned by the caller of the * createXdsConfigProvider()/createStaticConfigProvider() functions. The ConfigProviderManager holds * raw pointers to those objects. + * + * Configuration implementations returned by ConfigProvider::config() are immutable, which allows + * them to share the underlying objects such as config protos and subscriptions (for dynamic + * providers) without synchronization related performance penalties. This enables linear memory + * growth based on the size of the configuration set, regardless of the number of threads/objects + * that must hold a reference/pointer to them. */ class ConfigProviderManager { public: diff --git a/source/common/common/assert.h b/source/common/common/assert.h index d19270e27296e..cd3a9f55362ff 100644 --- a/source/common/common/assert.h +++ b/source/common/common/assert.h @@ -48,6 +48,34 @@ namespace Envoy { } while (false) #endif +// clang-format off + +#ifndef NDEBUG +#if __clang__ +#define __CLANG_PRAGMA(X) \ + _Pragma(X) +#else +#define __CLANG_PRAGMA(X) +#endif + +// The clang specific pragmas below are required to suppress errors due to +// -Wpotentially-evaluated-expression, which can be ignored in the context of assertions since +// type checks using typeid can trigger it. +#define ASSERT_WITH_PRECOND(PRECOND, COND) \ + do { \ + if (PRECOND) { \ + __CLANG_PRAGMA("clang diagnostic push") \ + __CLANG_PRAGMA("clang diagnostic ignored \"-Wpotentially-evaluated-expression\"") \ + ASSERT(COND); \ + __CLANG_PRAGMA("clang diagnostic pop") \ + } \ + } while (0) +#else // NDEBUG +#define ASSERT_WITH_PRECOND(PRECOND, COND) +#endif + +// clang-format on + /** * Indicate a panic situation and exit. */ diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index ab469991a712c..9f1afcbd89572 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -42,6 +42,10 @@ bool ConfigSubscriptionInstance::checkAndApplyConfig(const Protobuf::Message& co ASSERT(!dynamic_config_providers_.empty()); ConfigProvider::ConfigConstSharedPtr new_config; for (auto* provider : dynamic_config_providers_) { + // All bound dynamic config providers must be of the same type (see the ASSERT... in + // bindConfigProvider()). + // This makes it safe to call any of the provider's onConfigProtoUpdate() to get a new config + // impl, which can then be passed to all providers. if (new_config == nullptr) { new_config = provider->onConfigProtoUpdate(config_proto); } @@ -51,6 +55,14 @@ bool ConfigSubscriptionInstance::checkAndApplyConfig(const Protobuf::Message& co return true; } +void ConfigSubscriptionInstance::bindConfigProvider(DynamicConfigProviderImpl* provider) { + // All config providers bound to a ConfigSubscriptionInstance must be of the same concrete type; + // this is assumed by checkAndApplyConfig() and is verified by the assertion below. + ASSERT_WITH_PRECOND(dynamic_config_providers_.empty() == false, + typeid(*provider) == typeid(**dynamic_config_providers_.begin())); + dynamic_config_providers_.insert(provider); +} + ConfigProviderManagerImpl::ConfigProviderManagerImpl(Server::Admin& admin, const std::string& config_name) { config_tracker_entry_ = diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 2a2bf7b324d89..3b6306686cf5f 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -167,9 +167,7 @@ class ConfigSubscriptionInstance : public Init::Target, private: void registerInitTarget(Init::Manager& init_manager) { init_manager.registerTarget(*this); } - void bindConfigProvider(DynamicConfigProviderImpl* provider) { - dynamic_config_providers_.insert(provider); - } + void bindConfigProvider(DynamicConfigProviderImpl* provider); void unbindConfigProvider(DynamicConfigProviderImpl* provider) { dynamic_config_providers_.erase(provider); @@ -288,7 +286,7 @@ class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton ConfigProviderManagerImpl(Server::Admin& admin, const std::string& config_name); - const ConfigProviderSet static_config_providers() const { return static_config_providers_; } + const ConfigProviderSet& static_config_providers() const { return static_config_providers_; } const ConfigSubscriptionMap& config_subscriptions() const { return config_subscriptions_; } diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index f177520721ac2..69e38b00e10af 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -143,10 +143,10 @@ class DummyConfigProviderManager : public ConfigProviderManagerImpl { } for (const auto& provider : static_config_providers()) { - ASSERT(provider->configInfo()); + ASSERT(provider->configProtoInfo()); auto* static_config = config_dump->mutable_static_dummy_configs()->Add(); static_config->mutable_dummy_config()->MergeFrom( - provider->configInfo().value().config_); + provider->configProtoInfo().value().config_proto_); TimestampUtil::systemClockToTimestamp(provider->lastUpdated(), *static_config->mutable_last_updated()); } @@ -232,7 +232,7 @@ TEST_F(ConfigProviderImplTest, SharedOwnership) { config_source_proto, factory_context_, "dummy_prefix"); // No config protos have been received via the subscription yet. - EXPECT_FALSE(provider1->configInfo().has_value()); + EXPECT_FALSE(provider1->configProtoInfo().has_value()); Protobuf::RepeatedPtrField dummy_configs; dummy_configs.Add()->MergeFrom(parseDummyConfigFromYaml("a: a dummy config")); @@ -246,11 +246,11 @@ TEST_F(ConfigProviderImplTest, SharedOwnership) { ConfigProviderPtr provider2 = provider_manager_->createXdsConfigProvider( config_source_proto, factory_context_, "dummy_prefix"); - EXPECT_TRUE(provider2->configInfo().has_value()); + EXPECT_TRUE(provider2->configProtoInfo().has_value()); EXPECT_EQ(&dynamic_cast(*provider1).subscription(), &dynamic_cast(*provider2).subscription()); - EXPECT_EQ(&provider1->configInfo().value().config_, - &provider2->configInfo().value().config_); + EXPECT_EQ(&provider1->configProtoInfo().value().config_proto_, + &provider2->configProtoInfo().value().config_proto_); EXPECT_EQ(provider1->config().get(), provider2->config().get()); From 550723cb3d22837683f5dd828cc38d2c07836643 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 14 Dec 2018 16:22:50 -0500 Subject: [PATCH 09/25] Minor cleanup. Signed-off-by: Andres Guedez --- source/common/common/assert.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/common/common/assert.h b/source/common/common/assert.h index cd3a9f55362ff..255e911729fd0 100644 --- a/source/common/common/assert.h +++ b/source/common/common/assert.h @@ -48,16 +48,15 @@ namespace Envoy { } while (false) #endif -// clang-format off - #ifndef NDEBUG #if __clang__ -#define __CLANG_PRAGMA(X) \ - _Pragma(X) +#define __CLANG_PRAGMA(X) _Pragma(X) #else #define __CLANG_PRAGMA(X) #endif +// clang-format off + // The clang specific pragmas below are required to suppress errors due to // -Wpotentially-evaluated-expression, which can be ignored in the context of assertions since // type checks using typeid can trigger it. From 2c79e9c13b263775e9ebfe366867c5e4c49eab86 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 17 Dec 2018 11:01:57 -0500 Subject: [PATCH 10/25] Cleanup and comments based on reviewer feedback. Signed-off-by: Andres Guedez --- source/common/common/assert.h | 17 ++++++----------- source/common/config/config_provider_impl.cc | 7 +++++-- source/common/config/config_provider_impl.h | 5 +++++ test/common/config/dummy_config.proto | 4 ---- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/source/common/common/assert.h b/source/common/common/assert.h index 255e911729fd0..fc2617a5beaae 100644 --- a/source/common/common/assert.h +++ b/source/common/common/assert.h @@ -57,20 +57,15 @@ namespace Envoy { // clang-format off -// The clang specific pragmas below are required to suppress errors due to -// -Wpotentially-evaluated-expression, which can be ignored in the context of assertions since -// type checks using typeid can trigger it. -#define ASSERT_WITH_PRECOND(PRECOND, COND) \ +#define ASSERT_IGNORE_POTENTIALLY_EVALUATED(COND) \ do { \ - if (PRECOND) { \ - __CLANG_PRAGMA("clang diagnostic push") \ - __CLANG_PRAGMA("clang diagnostic ignored \"-Wpotentially-evaluated-expression\"") \ - ASSERT(COND); \ - __CLANG_PRAGMA("clang diagnostic pop") \ - } \ + __CLANG_PRAGMA("clang diagnostic push") \ + __CLANG_PRAGMA("clang diagnostic ignored \"-Wpotentially-evaluated-expression\"") \ + ASSERT(COND); \ + __CLANG_PRAGMA("clang diagnostic pop") \ } while (0) #else // NDEBUG -#define ASSERT_WITH_PRECOND(PRECOND, COND) +#define ASSERT_IGNORE_POTENTIALLY_EVALUATED(COND) #endif // clang-format on diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 9f1afcbd89572..71fae98c914b5 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -58,8 +58,11 @@ bool ConfigSubscriptionInstance::checkAndApplyConfig(const Protobuf::Message& co void ConfigSubscriptionInstance::bindConfigProvider(DynamicConfigProviderImpl* provider) { // All config providers bound to a ConfigSubscriptionInstance must be of the same concrete type; // this is assumed by checkAndApplyConfig() and is verified by the assertion below. - ASSERT_WITH_PRECOND(dynamic_config_providers_.empty() == false, - typeid(*provider) == typeid(**dynamic_config_providers_.begin())); + // NOTE: a regular ASSERT() triggers a potentially evaluated expression warning from clang due to + // the args passed to the second typeid() call. + ASSERT_IGNORE_POTENTIALLY_EVALUATED(dynamic_config_providers_.empty() || + typeid(*provider) == + typeid(**dynamic_config_providers_.begin())); dynamic_config_providers_.insert(provider); } diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 3b6306686cf5f..e201796951a20 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -86,6 +86,11 @@ class DynamicConfigProviderImpl; * Provides generic functionality required by all dynamic ConfigProvider subscriptions, including * shared lifetime management via shared_ptr. * + * To do so, this class keeps track of a set of dynamic config providers associated with an + * underlying subscription; providers are bound/unbound as needed as they are created and destroyed. + * This is done to avoid exposing shared ownership semantics to the external API (see + * ConfigProvider). + * * This class can not be instantiated directly; instead, it provides the foundation for * dynamic config subscription implementations which derive from it. */ diff --git a/test/common/config/dummy_config.proto b/test/common/config/dummy_config.proto index 9964bd76fd36f..fcb2749e4f036 100644 --- a/test/common/config/dummy_config.proto +++ b/test/common/config/dummy_config.proto @@ -13,19 +13,15 @@ message DummyConfig { message DummyConfigsDump { message StaticConfigs { DummyConfig dummy_config = 1; - google.protobuf.Timestamp last_updated = 2; } message DynamicConfigs { string version_info = 1; - DummyConfig dummy_config = 2; - google.protobuf.Timestamp last_updated = 3; } repeated StaticConfigs static_dummy_configs = 1; - repeated DynamicConfigs dynamic_dummy_configs = 2; } From e67b5eed43e60a0117dec97209b8bc6a247124ec Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 19 Dec 2018 11:06:36 -0500 Subject: [PATCH 11/25] Rename *Impl -> *ImplBase. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.cc | 30 ++-- source/common/config/config_provider_impl.h | 132 +++++++++--------- .../config/config_provider_impl_test.cc | 31 ++-- 3 files changed, 98 insertions(+), 95 deletions(-) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 71fae98c914b5..4c159c28d8d0a 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -3,33 +3,33 @@ namespace Envoy { namespace Config { -StaticConfigProviderImpl::StaticConfigProviderImpl( +StaticConfigProviderImplBase::StaticConfigProviderImplBase( Server::Configuration::FactoryContext& factory_context, - ConfigProviderManagerImpl& config_provider_manager) + ConfigProviderManagerImplBase& 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() { +StaticConfigProviderImplBase::~StaticConfigProviderImplBase() { config_provider_manager_.static_config_providers_.erase(this); } -ConfigSubscriptionInstance::~ConfigSubscriptionInstance() { +ConfigSubscriptionInstanceBase::~ConfigSubscriptionInstanceBase() { runInitializeCallbackIfAny(); config_provider_manager_.unbindSubscription(manager_identifier_); } -void ConfigSubscriptionInstance::runInitializeCallbackIfAny() { +void ConfigSubscriptionInstanceBase::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) { +bool ConfigSubscriptionInstanceBase::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; @@ -55,19 +55,19 @@ bool ConfigSubscriptionInstance::checkAndApplyConfig(const Protobuf::Message& co return true; } -void ConfigSubscriptionInstance::bindConfigProvider(DynamicConfigProviderImpl* provider) { - // All config providers bound to a ConfigSubscriptionInstance must be of the same concrete type; - // this is assumed by checkAndApplyConfig() and is verified by the assertion below. - // NOTE: a regular ASSERT() triggers a potentially evaluated expression warning from clang due to - // the args passed to the second typeid() call. +void ConfigSubscriptionInstanceBase::bindConfigProvider(DynamicConfigProviderImplBase* provider) { + // All config providers bound to a ConfigSubscriptionInstanceBase must be of the same concrete + // type; this is assumed by checkAndApplyConfig() and is verified by the assertion below. NOTE: a + // regular ASSERT() triggers a potentially evaluated expression warning from clang due to the args + // passed to the second typeid() call. ASSERT_IGNORE_POTENTIALLY_EVALUATED(dynamic_config_providers_.empty() || typeid(*provider) == typeid(**dynamic_config_providers_.begin())); dynamic_config_providers_.insert(provider); } -ConfigProviderManagerImpl::ConfigProviderManagerImpl(Server::Admin& admin, - const std::string& config_name) { +ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(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 diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index e201796951a20..424220c928afa 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -16,10 +16,10 @@ namespace Envoy { namespace Config { -// This file provides a set of base classes, (StaticConfigProviderImpl, DynamicConfigProviderImpl, -// ConfigProviderManagerImpl, ConfigSubscriptionInstance), conforming to the -// ConfigProvider/ConfigProviderManager interfaces, which in tandem provide a framework for -// implementing static and dynamic configuration for Envoy. +// This file provides a set of base classes, (StaticConfigProviderImplBase, +// DynamicConfigProviderImplBase, ConfigProviderManagerImplBase, ConfigSubscriptionInstanceBase), +// conforming to the ConfigProvider/ConfigProviderManager interfaces, which in tandem provide a +// framework for implementing static and dynamic configuration for Envoy. // // Dynamic configuration is distributed via xDS APIs (see // https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md). The framework exposed @@ -34,27 +34,29 @@ namespace Config { // A blueprint to follow for implementing static and dynamic config providers is as follows: // // For both: -// 1) Create a class derived from ConfigProviderManagerImpl and implement the required interface. +// 1) Create a class derived from ConfigProviderManagerImplBase and implement the required +// interface. // When implementing createXdsConfigProvider(), it is expected that getSubscription() will -// be called to fetch either an existing ConfigSubscriptionInstance if the config source +// be called to fetch either an existing ConfigSubscriptionInstanceBase if the config source // configuration matches, or a newly instantiated subscription otherwise. // // For static providers: -// 1) Create a class derived from StaticConfigProviderImpl and implement the required interface. +// 1) Create a class derived from StaticConfigProviderImplBase and implement the required +// interface. // // For dynamic (xDS) providers: -// 1) Create a class derived from DynamicConfigProviderImpl and implement the required interface. -// 2) Create a class derived from ConfigSubscriptionInstance; this is the entity responsible for -// owning and managing the Envoy::Config::Subscription that provides the underlying -// config subscription. +// 1) Create a class derived from DynamicConfigProviderImplBase and implement the required +// interface. 2) Create a class derived from ConfigSubscriptionInstanceBase; this is the entity +// responsible for owning and managing the Envoy::Config::Subscription that provides +// the underlying config subscription. // - When subscription callbacks (onConfigUpdate, onConfigUpdateFailed) are issued by the -// underlying subscription, the corresponding ConfigSubscriptionInstance functions must be +// underlying subscription, the corresponding ConfigSubscriptionInstanceBase functions must be // called as well. // - On a successful config update, checkAndApplyConfig() should be called to instantiate the // new config implementation and propagate it to the shared config providers and all // worker threads. -class ConfigProviderManagerImpl; +class ConfigProviderManagerImplBase; /** * ConfigProvider implementation for statically specified configuration. @@ -62,25 +64,25 @@ class ConfigProviderManagerImpl; * This class can not be instantiated directly; instead, it provides the foundation for * static config provider implementations which derive from it. */ -class StaticConfigProviderImpl : public ConfigProvider { +class StaticConfigProviderImplBase : public ConfigProvider { public: - StaticConfigProviderImpl() = delete; + StaticConfigProviderImplBase() = delete; - virtual ~StaticConfigProviderImpl(); + virtual ~StaticConfigProviderImplBase(); // Envoy::Config::ConfigProvider SystemTime lastUpdated() const override { return last_updated_; } protected: - StaticConfigProviderImpl(Server::Configuration::FactoryContext& factory_context, - ConfigProviderManagerImpl& config_provider_manager); + StaticConfigProviderImplBase(Server::Configuration::FactoryContext& factory_context, + ConfigProviderManagerImplBase& config_provider_manager); private: SystemTime last_updated_; - ConfigProviderManagerImpl& config_provider_manager_; + ConfigProviderManagerImplBase& config_provider_manager_; }; -class DynamicConfigProviderImpl; +class DynamicConfigProviderImplBase; /** * Provides generic functionality required by all dynamic ConfigProvider subscriptions, including @@ -94,17 +96,17 @@ class DynamicConfigProviderImpl; * This class can not be instantiated directly; instead, it provides the foundation for * dynamic config subscription implementations which derive from it. */ -class ConfigSubscriptionInstance : public Init::Target, - protected Logger::Loggable { +class ConfigSubscriptionInstanceBase : public Init::Target, + protected Logger::Loggable { public: struct LastConfigInfo { uint64_t last_config_hash_; std::string last_config_version_; }; - ConfigSubscriptionInstance() = delete; + ConfigSubscriptionInstanceBase() = delete; - virtual ~ConfigSubscriptionInstance(); + virtual ~ConfigSubscriptionInstanceBase(); // Init::Target void initialize(std::function callback) override { @@ -149,16 +151,16 @@ class ConfigSubscriptionInstance : public Init::Target, bool checkAndApplyConfig(const Protobuf::Message& config_proto, const std::string& config_name, const std::string& version_info); - const std::unordered_set dynamic_config_providers() const { - return std::unordered_set(dynamic_config_providers_.begin(), - dynamic_config_providers_.end()); + const std::unordered_set dynamic_config_providers() const { + return std::unordered_set( + dynamic_config_providers_.begin(), dynamic_config_providers_.end()); } protected: - ConfigSubscriptionInstance(const std::string& name, const std::string& manager_identifier, - ConfigProviderManagerImpl& config_provider_manager, - TimeSource& time_source, const SystemTime& last_updated, - const LocalInfo::LocalInfo& local_info) + ConfigSubscriptionInstanceBase(const std::string& name, const std::string& manager_identifier, + ConfigProviderManagerImplBase& config_provider_manager, + TimeSource& time_source, const SystemTime& last_updated, + const LocalInfo::LocalInfo& local_info) : name_(name), manager_identifier_(manager_identifier), config_provider_manager_(config_provider_manager), time_source_(time_source), last_updated_(last_updated) { @@ -172,17 +174,17 @@ class ConfigSubscriptionInstance : public Init::Target, private: void registerInitTarget(Init::Manager& init_manager) { init_manager.registerTarget(*this); } - void bindConfigProvider(DynamicConfigProviderImpl* provider); + void bindConfigProvider(DynamicConfigProviderImplBase* provider); - void unbindConfigProvider(DynamicConfigProviderImpl* provider) { + void unbindConfigProvider(DynamicConfigProviderImplBase* provider) { dynamic_config_providers_.erase(provider); } const std::string name_; std::function initialize_callback_; - std::unordered_set dynamic_config_providers_; + std::unordered_set dynamic_config_providers_; const std::string manager_identifier_; - ConfigProviderManagerImpl& config_provider_manager_; + ConfigProviderManagerImplBase& config_provider_manager_; TimeSource& time_source_; SystemTime last_updated_; absl::optional config_info_; @@ -192,13 +194,13 @@ class ConfigSubscriptionInstance : public Init::Target, // them. // // TODO(AndresGuedez): Investigate whether a shared ownership model avoiding the s and - // instead centralizing lifetime management in the ConfigProviderManagerImpl with explicit + // instead centralizing lifetime management in the ConfigProviderManagerImplBase with explicit // reference counting would be more maintainable. - friend class DynamicConfigProviderImpl; - friend class ConfigProviderManagerImpl; + friend class DynamicConfigProviderImplBase; + friend class ConfigProviderManagerImplBase; }; -using ConfigSubscriptionInstanceSharedPtr = std::shared_ptr; +using ConfigSubscriptionInstanceBaseSharedPtr = std::shared_ptr; /** * Provides generic functionality required by all dynamic config providers, including distribution @@ -207,11 +209,11 @@ using ConfigSubscriptionInstanceSharedPtr = std::shared_ptrunbindConfigProvider(this); } + virtual ~DynamicConfigProviderImplBase() { subscription_->unbindConfigProvider(this); } // Envoy::Config::ConfigProvider SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } @@ -245,11 +247,11 @@ class DynamicConfigProviderImpl : public ConfigProvider { } protected: - DynamicConfigProviderImpl(ConfigSubscriptionInstanceSharedPtr&& subscription, - Server::Configuration::FactoryContext& factory_context) + DynamicConfigProviderImplBase(ConfigSubscriptionInstanceBaseSharedPtr&& subscription, + Server::Configuration::FactoryContext& factory_context) : subscription_(subscription), tls_(factory_context.threadLocal().allocateSlot()) {} - const ConfigSubscriptionInstanceSharedPtr& subscription() const { return subscription_; } + const ConfigSubscriptionInstanceBaseSharedPtr& subscription() const { return subscription_; } private: struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { @@ -259,7 +261,7 @@ class DynamicConfigProviderImpl : public ConfigProvider { ConfigProvider::ConfigConstSharedPtr config_; }; - ConfigSubscriptionInstanceSharedPtr subscription_; + ConfigSubscriptionInstanceBaseSharedPtr subscription_; ThreadLocal::SlotPtr tls_; }; @@ -271,11 +273,11 @@ class DynamicConfigProviderImpl : public ConfigProvider { * This class can not be instantiated directly; instead, it provides the foundation for * dynamic config provider implementations which derive from it. */ -class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton::Instance { +class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singleton::Instance { public: - ConfigProviderManagerImpl() = delete; + ConfigProviderManagerImplBase() = delete; - virtual ~ConfigProviderManagerImpl() {} + virtual ~ConfigProviderManagerImplBase() {} /** * This is invoked by the /config_dump admin handler. @@ -287,9 +289,9 @@ class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton protected: using ConfigProviderSet = std::unordered_set; using ConfigSubscriptionMap = - std::unordered_map>; + std::unordered_map>; - ConfigProviderManagerImpl(Server::Admin& admin, const std::string& config_name); + ConfigProviderManagerImplBase(Server::Admin& admin, const std::string& config_name); const ConfigProviderSet& static_config_providers() const { return static_config_providers_; } @@ -305,15 +307,15 @@ class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton * @return std::shared_ptr an existing (if a match is found) or newly allocated subscription. */ template - std::shared_ptr - getSubscription(const Protobuf::Message& config_source_proto, Init::Manager& init_manager, - std::function - subscription_factory_fn) { - static_assert(std::is_base_of::value, - "T must be a subclass of ConfigSubscriptionInstance"); - - ConfigSubscriptionInstanceSharedPtr subscription; + std::shared_ptr getSubscription(const Protobuf::Message& config_source_proto, + Init::Manager& init_manager, + std::function + subscription_factory_fn) { + static_assert(std::is_base_of::value, + "T must be a subclass of ConfigSubscriptionInstanceBase"); + + ConfigSubscriptionInstanceBaseSharedPtr subscription; const std::string manager_identifier = config_source_proto.SerializeAsString(); auto it = config_subscriptions_.find(manager_identifier); @@ -339,7 +341,7 @@ class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton private: void bindSubscription(const std::string& manager_identifier, - ConfigSubscriptionInstanceSharedPtr& subscription) { + ConfigSubscriptionInstanceBaseSharedPtr& subscription) { config_subscriptions_.insert({manager_identifier, subscription}); } @@ -354,10 +356,10 @@ class ConfigProviderManagerImpl : public ConfigProviderManager, public Singleton ConfigProviderSet static_config_providers_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; - // See comment for friend classes in the ConfigSubscriptionInstance for more details on the use - // of friends. - friend class ConfigSubscriptionInstance; - friend class StaticConfigProviderImpl; + // See comment for friend classes in the ConfigSubscriptionInstanceBase for more details on the + // use of friends. + friend class ConfigSubscriptionInstanceBase; + friend class StaticConfigProviderImplBase; }; } // namespace Config diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 69e38b00e10af..2c1483668d98a 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -15,7 +15,7 @@ namespace { class DummyConfigProviderManager; -class StaticDummyConfigProvider : public Envoy::Config::StaticConfigProviderImpl { +class StaticDummyConfigProvider : public Envoy::Config::StaticConfigProviderImplBase { public: StaticDummyConfigProvider(const test::common::config::DummyConfig& config_proto, Server::Configuration::FactoryContext& factory_context, @@ -38,7 +38,7 @@ class StaticDummyConfigProvider : public Envoy::Config::StaticConfigProviderImpl }; class DummyConfigSubscription - : public ConfigSubscriptionInstance, + : public ConfigSubscriptionInstanceBase, Envoy::Config::SubscriptionCallbacks { public: DummyConfigSubscription(const std::string& manager_identifier, @@ -47,7 +47,7 @@ class DummyConfigSubscription ~DummyConfigSubscription() {} - // Envoy::Config::ConfigSubscriptionInstance + // Envoy::Config::ConfigSubscriptionInstanceBase void start() override {} // Envoy::Config::SubscriptionCallbacks @@ -57,7 +57,7 @@ class DummyConfigSubscription config_proto_ = config; } - ConfigSubscriptionInstance::onConfigUpdate(); + ConfigSubscriptionInstanceBase::onConfigUpdate(); } // Envoy::Config::SubscriptionCallbacks @@ -81,14 +81,14 @@ class DummyConfig : public ConfigProvider::Config { DummyConfig(const test::common::config::DummyConfig&) {} }; -class DummyDynamicConfigProvider : public DynamicConfigProviderImpl { +class DummyDynamicConfigProvider : public DynamicConfigProviderImplBase { public: DummyDynamicConfigProvider(DummyConfigSubscriptionSharedPtr&& subscription, ConfigConstSharedPtr initial_config, Server::Configuration::FactoryContext& factory_context) - : DynamicConfigProviderImpl(std::move(subscription), factory_context), + : DynamicConfigProviderImplBase(std::move(subscription), factory_context), subscription_(static_cast( - DynamicConfigProviderImpl::subscription().get())) { + DynamicConfigProviderImplBase::subscription().get())) { initialize(initial_config); } @@ -96,7 +96,7 @@ class DummyDynamicConfigProvider : public DynamicConfigProviderImpl { DummyConfigSubscription& subscription() { return *subscription_; } - // Envoy::Config::DynamicConfigProviderImpl + // Envoy::Config::DynamicConfigProviderImplBase ConfigProvider::ConfigConstSharedPtr onConfigProtoUpdate(const Protobuf::Message& config) override { return std::make_shared( @@ -119,13 +119,14 @@ class DummyDynamicConfigProvider : public DynamicConfigProviderImpl { DummyConfigSubscription* subscription_; }; -class DummyConfigProviderManager : public ConfigProviderManagerImpl { +class DummyConfigProviderManager : public ConfigProviderManagerImplBase { public: - DummyConfigProviderManager(Server::Admin& admin) : ConfigProviderManagerImpl(admin, "dummy") {} + DummyConfigProviderManager(Server::Admin& admin) + : ConfigProviderManagerImplBase(admin, "dummy") {} ~DummyConfigProviderManager() {} - // Envoy::Config::ConfigProviderManagerImpl + // Envoy::Config::ConfigProviderManagerImplBase ProtobufTypes::MessagePtr dumpConfigs() const override { auto config_dump = std::make_unique(); for (const auto& element : config_subscriptions()) { @@ -161,8 +162,8 @@ class DummyConfigProviderManager : public ConfigProviderManagerImpl { DummyConfigSubscriptionSharedPtr subscription = getSubscription( config_source_proto, factory_context.initManager(), [&factory_context](const std::string& manager_identifier, - ConfigProviderManagerImpl& config_provider_manager) - -> ConfigSubscriptionInstanceSharedPtr { + ConfigProviderManagerImplBase& config_provider_manager) + -> ConfigSubscriptionInstanceBaseSharedPtr { return std::make_shared( manager_identifier, factory_context, static_cast(config_provider_manager)); @@ -190,13 +191,13 @@ StaticDummyConfigProvider::StaticDummyConfigProvider( const test::common::config::DummyConfig& config_proto, Server::Configuration::FactoryContext& factory_context, DummyConfigProviderManager& config_provider_manager) - : StaticConfigProviderImpl(factory_context, config_provider_manager), + : StaticConfigProviderImplBase(factory_context, config_provider_manager), config_(std::make_shared(config_proto)), config_proto_(config_proto) {} DummyConfigSubscription::DummyConfigSubscription( const std::string& manager_identifier, Server::Configuration::FactoryContext& factory_context, DummyConfigProviderManager& config_provider_manager) - : ConfigSubscriptionInstance( + : ConfigSubscriptionInstanceBase( "DummyDS", manager_identifier, config_provider_manager, factory_context.timeSource(), factory_context.timeSource().systemTime(), factory_context.localInfo()) {} From 821aab9003b4d08b2aefb946c34be10ce4f36f9f Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 19 Dec 2018 11:27:51 -0500 Subject: [PATCH 12/25] Cleanup based on review feedback. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.h | 10 +++++++--- test/common/config/config_provider_impl_test.cc | 6 ++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 424220c928afa..951e423f8fa06 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -151,9 +151,13 @@ class ConfigSubscriptionInstanceBase : public Init::Target, bool checkAndApplyConfig(const Protobuf::Message& config_proto, const std::string& config_name, const std::string& version_info); - const std::unordered_set dynamic_config_providers() const { - return std::unordered_set( - dynamic_config_providers_.begin(), dynamic_config_providers_.end()); + /** + * Returns one of the bound dynamic config providers. + * @return const DynamicConfigProviderImplBase* a const pointer to a + * bound DynamicConfigProviderImplBase or nullptr when there are none. + */ + const DynamicConfigProviderImplBase* getAnyBoundDynamicConfigProvider() const { + return !dynamic_config_providers_.empty() ? *dynamic_config_providers_.begin() : nullptr; } protected: diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 2c1483668d98a..9c0297814f632 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -170,8 +170,10 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase { }); ConfigProvider::ConfigConstSharedPtr initial_config; - if (!subscription->dynamic_config_providers().empty()) { - initial_config = (*subscription->dynamic_config_providers().begin())->getConfig(); + const DynamicConfigProviderImplBase* provider = + subscription->getAnyBoundDynamicConfigProvider(); + if (provider) { + initial_config = provider->getConfig(); } return std::make_unique(std::move(subscription), initial_config, factory_context); From ca583a0549c7474c0fff253eea2c1585f2ef168d Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 19 Dec 2018 15:16:24 -0500 Subject: [PATCH 13/25] More comments and cleanup. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.cc | 7 ++-- source/common/config/config_provider_impl.h | 37 +++++++++++++++++--- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 4c159c28d8d0a..a4fff8e1bbd9e 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -8,11 +8,11 @@ StaticConfigProviderImplBase::StaticConfigProviderImplBase( ConfigProviderManagerImplBase& config_provider_manager) : last_updated_(factory_context.timeSource().systemTime()), config_provider_manager_(config_provider_manager) { - config_provider_manager_.static_config_providers_.insert(this); + config_provider_manager_.bindStaticConfigProvider(this); } StaticConfigProviderImplBase::~StaticConfigProviderImplBase() { - config_provider_manager_.static_config_providers_.erase(this); + config_provider_manager_.unbindStaticConfigProvider(this); } ConfigSubscriptionInstanceBase::~ConfigSubscriptionInstanceBase() { @@ -67,7 +67,8 @@ void ConfigSubscriptionInstanceBase::bindConfigProvider(DynamicConfigProviderImp } ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(Server::Admin& admin, - const std::string& config_name) { + const std::string& config_name) + : owner_tid_(Thread::currentThreadId()) { 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 diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 951e423f8fa06..e9f775c18a168 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -11,6 +11,7 @@ #include "envoy/thread_local/thread_local.h" #include "common/config/utility.h" +#include "common/common/thread.h" #include "common/protobuf/protobuf.h" namespace Envoy { @@ -90,8 +91,12 @@ class DynamicConfigProviderImplBase; * * To do so, this class keeps track of a set of dynamic config providers associated with an * underlying subscription; providers are bound/unbound as needed as they are created and destroyed. - * This is done to avoid exposing shared ownership semantics to the external API (see - * ConfigProvider). + * + * Dynamic config providers and subscriptions are split to avoid lifetime issues with arguments + * required by the config providers. An example is the Server::Configuration::FactoryContext, which + * is owned by listeners and therefore may be destroyed while an associated config provider is still + * in use (see #3960). This split enables single ownership of the config providers, while enabling + * shared ownership of the underlying subscription. * * This class can not be instantiated directly; instead, it provides the foundation for * dynamic config subscription implementations which derive from it. @@ -274,6 +279,15 @@ class DynamicConfigProviderImplBase : public ConfigProvider { * lifetime of subscriptions and dynamic config providers, along with determining which * subscriptions should be associated with newly instantiated providers. * + * The implementation of this class is not thread safe. Note that StaticConfigProviderImplBase and + * ConfigSubscriptionInstanceBase call the corresponding {bind,unbind}* functions exposed by this + * class. + * + * All config processing is done on the main thread, so instantiation of *ConfigProvider* objects + * via createStaticConfigProvider() and createXdsConfigProvider() is naturally thread safe. Care + * must be taken with regards to destruction of these objects, since it must also happen on the main + * thread. + * * This class can not be instantiated directly; instead, it provides the foundation for * dynamic config provider implementations which derive from it. */ @@ -316,6 +330,8 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl std::function subscription_factory_fn) { + ASSERT(owner_tid_ == Thread::currentThreadId()); + static_assert(std::is_base_of::value, "T must be a subclass of ConfigSubscriptionInstanceBase"); @@ -333,8 +349,8 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl bindSubscription(manager_identifier, subscription); } else { - // Because the RouteConfigProviderManager's weak_ptrs only get cleaned up - // in the RdsRouteConfigSubscription destructor, and the single threaded nature + // Because the ConfigProviderManagerImplBase's weak_ptrs only get cleaned up + // in the ConfigSubscriptionInstanceBase destructor, and the single threaded nature // of this code, locking the weak_ptr will not fail. subscription = it->second.lock(); } @@ -346,19 +362,32 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl private: void bindSubscription(const std::string& manager_identifier, ConfigSubscriptionInstanceBaseSharedPtr& subscription) { + ASSERT(owner_tid_ == Thread::currentThreadId()); config_subscriptions_.insert({manager_identifier, subscription}); } void unbindSubscription(const std::string& manager_identifier) { + ASSERT(owner_tid_ == Thread::currentThreadId()); config_subscriptions_.erase(manager_identifier); } + void bindStaticConfigProvider(StaticConfigProviderImplBase* provider) { + ASSERT(owner_tid_ == Thread::currentThreadId()); + static_config_providers_.insert(provider); + } + + void unbindStaticConfigProvider(StaticConfigProviderImplBase* provider) { + ASSERT(owner_tid_ == Thread::currentThreadId()); + static_config_providers_.erase(provider); + } + // TODO(jsedgwick) These two members are prime candidates for the owned-entry list/map // as in ConfigTracker. I.e. the ProviderImpls would have an EntryOwner for these lists // Then the lifetime management stuff is centralized and opaque. ConfigSubscriptionMap config_subscriptions_; ConfigProviderSet static_config_providers_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; + Thread::ThreadId owner_tid_{}; // See comment for friend classes in the ConfigSubscriptionInstanceBase for more details on the // use of friends. From 15c9d193782cbce8a73848acd52d17df8d6f6eb6 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 20 Dec 2018 12:02:57 -0500 Subject: [PATCH 14/25] Comments and cleanup based on reviewer comments. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.cc | 4 +++- source/common/config/config_provider_impl.h | 17 +++++++++++------ test/common/config/config_provider_impl_test.cc | 4 ++-- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index a4fff8e1bbd9e..f884ae6d6928a 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -47,7 +47,9 @@ bool ConfigSubscriptionInstanceBase::checkAndApplyConfig(const Protobuf::Message // This makes it safe to call any of the provider's onConfigProtoUpdate() to get a new config // impl, which can then be passed to all providers. if (new_config == nullptr) { - new_config = provider->onConfigProtoUpdate(config_proto); + if ((new_config = provider->onConfigProtoUpdate(config_proto)) == nullptr) { + return false; + } } provider->onConfigUpdate(new_config); } diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index e9f775c18a168..0b6d85f2d7506 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -47,7 +47,8 @@ namespace Config { // // For dynamic (xDS) providers: // 1) Create a class derived from DynamicConfigProviderImplBase and implement the required -// interface. 2) Create a class derived from ConfigSubscriptionInstanceBase; this is the entity +// interface. +// 2) Create a class derived from ConfigSubscriptionInstanceBase; this is the entity // responsible for owning and managing the Envoy::Config::Subscription that provides // the underlying config subscription. // - When subscription callbacks (onConfigUpdate, onConfigUpdateFailed) are issued by the @@ -62,6 +63,9 @@ class ConfigProviderManagerImplBase; /** * ConfigProvider implementation for statically specified configuration. * + * TODO(AndresGuedez): support sharing of config protos and config impls, as is + * done with the DynamicConfigProviderImplBase. + * * This class can not be instantiated directly; instead, it provides the foundation for * static config provider implementations which derive from it. */ @@ -198,9 +202,9 @@ class ConfigSubscriptionInstanceBase : public Init::Target, SystemTime last_updated_; absl::optional config_info_; - // Subscriptions, dynamic config providers and config provider managers are tightly coupled with - // the current shared ownership model; use friend classes to explicitly denote the binding between - // them. + // ConfigSubscriptionInstanceBase, DynamicConfigProviderImplBase and ConfigProviderManagerImplBase + // are tightly coupled with the current shared ownership model; use friend classes to explicitly + // denote the binding between them. // // TODO(AndresGuedez): Investigate whether a shared ownership model avoiding the s and // instead centralizing lifetime management in the ConfigProviderManagerImplBase with explicit @@ -311,9 +315,9 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl ConfigProviderManagerImplBase(Server::Admin& admin, const std::string& config_name); - const ConfigProviderSet& static_config_providers() const { return static_config_providers_; } + const ConfigProviderSet& staticConfigProviders() const { return static_config_providers_; } - const ConfigSubscriptionMap& config_subscriptions() const { return config_subscriptions_; } + const ConfigSubscriptionMap& configSubscriptions() const { return config_subscriptions_; } /** * Returns the subscription associated with the config_source_proto; if none exists, a new one is @@ -386,6 +390,7 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl // Then the lifetime management stuff is centralized and opaque. ConfigSubscriptionMap config_subscriptions_; ConfigProviderSet static_config_providers_; + Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; Thread::ThreadId owner_tid_{}; diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 9c0297814f632..171dac24303ae 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -129,7 +129,7 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase { // Envoy::Config::ConfigProviderManagerImplBase ProtobufTypes::MessagePtr dumpConfigs() const override { auto config_dump = std::make_unique(); - for (const auto& element : config_subscriptions()) { + for (const auto& element : configSubscriptions()) { auto subscription = element.second.lock(); ASSERT(subscription); @@ -143,7 +143,7 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase { } } - for (const auto& provider : static_config_providers()) { + for (const auto& provider : staticConfigProviders()) { ASSERT(provider->configProtoInfo()); auto* static_config = config_dump->mutable_static_dummy_configs()->Add(); static_config->mutable_dummy_config()->MergeFrom( From f69eb7430ebb4b33c26fc39f3361c45694e6b28c Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 20 Dec 2018 12:05:11 -0500 Subject: [PATCH 15/25] Header include order fix. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 0b6d85f2d7506..a9fe114d23571 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -10,8 +10,8 @@ #include "envoy/singleton/instance.h" #include "envoy/thread_local/thread_local.h" -#include "common/config/utility.h" #include "common/common/thread.h" +#include "common/config/utility.h" #include "common/protobuf/protobuf.h" namespace Envoy { From ba44ad9221c679b0970a11770bd08de9c9d78cbf Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 20 Dec 2018 14:12:40 -0500 Subject: [PATCH 16/25] Remove superfluous constructor deletes. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index a9fe114d23571..881872dec8e9f 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -71,8 +71,6 @@ class ConfigProviderManagerImplBase; */ class StaticConfigProviderImplBase : public ConfigProvider { public: - StaticConfigProviderImplBase() = delete; - virtual ~StaticConfigProviderImplBase(); // Envoy::Config::ConfigProvider @@ -113,8 +111,6 @@ class ConfigSubscriptionInstanceBase : public Init::Target, std::string last_config_version_; }; - ConfigSubscriptionInstanceBase() = delete; - virtual ~ConfigSubscriptionInstanceBase(); // Init::Target @@ -224,8 +220,6 @@ using ConfigSubscriptionInstanceBaseSharedPtr = std::shared_ptrunbindConfigProvider(this); } // Envoy::Config::ConfigProvider @@ -297,8 +291,6 @@ class DynamicConfigProviderImplBase : public ConfigProvider { */ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singleton::Instance { public: - ConfigProviderManagerImplBase() = delete; - virtual ~ConfigProviderManagerImplBase() {} /** From 142d9c1413efde722fd2817acf148219c43c7c8b Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 20 Dec 2018 17:56:32 -0500 Subject: [PATCH 17/25] Rename ({Static,Dynamic} -> {Immutable,Mutable})ConfigProviderImplBase. Also, formalizes the concept of types of ConfigProvider instances via the introduction of a ConfigProviderInstanceType. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.cc | 69 ++++++++-- source/common/config/config_provider_impl.h | 125 ++++++++++-------- .../config/config_provider_impl_test.cc | 19 +-- 3 files changed, 139 insertions(+), 74 deletions(-) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index f884ae6d6928a..557edc7b65456 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -3,16 +3,20 @@ namespace Envoy { namespace Config { -StaticConfigProviderImplBase::StaticConfigProviderImplBase( +std::ostream& operator<<(std::ostream& os, ConfigProviderInstanceType type) { + return os << static_cast(type); +} + +ImmutableConfigProviderImplBase::ImmutableConfigProviderImplBase( Server::Configuration::FactoryContext& factory_context, - ConfigProviderManagerImplBase& config_provider_manager) + ConfigProviderManagerImplBase& config_provider_manager, ConfigProviderInstanceType type) : last_updated_(factory_context.timeSource().systemTime()), - config_provider_manager_(config_provider_manager) { - config_provider_manager_.bindStaticConfigProvider(this); + config_provider_manager_(config_provider_manager), type_(type) { + config_provider_manager_.bindImmutableConfigProvider(this); } -StaticConfigProviderImplBase::~StaticConfigProviderImplBase() { - config_provider_manager_.unbindStaticConfigProvider(this); +ImmutableConfigProviderImplBase::~ImmutableConfigProviderImplBase() { + config_provider_manager_.unbindImmutableConfigProvider(this); } ConfigSubscriptionInstanceBase::~ConfigSubscriptionInstanceBase() { @@ -39,10 +43,10 @@ bool ConfigSubscriptionInstanceBase::checkAndApplyConfig(const Protobuf::Message ENVOY_LOG(debug, "{}: loading new configuration: config_name={} hash={}", name_, config_name, new_hash); - ASSERT(!dynamic_config_providers_.empty()); + ASSERT(!mutable_config_providers_.empty()); ConfigProvider::ConfigConstSharedPtr new_config; - for (auto* provider : dynamic_config_providers_) { - // All bound dynamic config providers must be of the same type (see the ASSERT... in + for (auto* provider : mutable_config_providers_) { + // All bound mutable config providers must be of the same type (see the ASSERT... in // bindConfigProvider()). // This makes it safe to call any of the provider's onConfigProtoUpdate() to get a new config // impl, which can then be passed to all providers. @@ -57,15 +61,15 @@ bool ConfigSubscriptionInstanceBase::checkAndApplyConfig(const Protobuf::Message return true; } -void ConfigSubscriptionInstanceBase::bindConfigProvider(DynamicConfigProviderImplBase* provider) { +void ConfigSubscriptionInstanceBase::bindConfigProvider(MutableConfigProviderImplBase* provider) { // All config providers bound to a ConfigSubscriptionInstanceBase must be of the same concrete // type; this is assumed by checkAndApplyConfig() and is verified by the assertion below. NOTE: a // regular ASSERT() triggers a potentially evaluated expression warning from clang due to the args // passed to the second typeid() call. - ASSERT_IGNORE_POTENTIALLY_EVALUATED(dynamic_config_providers_.empty() || + ASSERT_IGNORE_POTENTIALLY_EVALUATED(mutable_config_providers_.empty() || typeid(*provider) == - typeid(**dynamic_config_providers_.begin())); - dynamic_config_providers_.insert(provider); + typeid(**mutable_config_providers_.begin())); + mutable_config_providers_.insert(provider); } ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(Server::Admin& admin, @@ -78,5 +82,44 @@ ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(Server::Admin& admi RELEASE_ASSERT(config_tracker_entry_, ""); } +const ConfigProviderManagerImplBase::ConfigProviderSet& +ConfigProviderManagerImplBase::immutableConfigProviders(ConfigProviderInstanceType type) const { + static ConfigProviderSet empty_set; + ConfigProviderMap::const_iterator it; + if ((it = immutable_config_providers_map_.find(type)) == immutable_config_providers_map_.end()) { + return empty_set; + } + + return *it->second.get(); +} + +void ConfigProviderManagerImplBase::bindImmutableConfigProvider( + ImmutableConfigProviderImplBase* provider) { + ASSERT(owner_tid_ == Thread::currentThreadId() && + (provider->type() == ConfigProviderInstanceType::Static || + provider->type() == ConfigProviderInstanceType::Inline)); + ConfigProviderMap::iterator it; + if ((it = immutable_config_providers_map_.find(provider->type())) == + immutable_config_providers_map_.end()) { + immutable_config_providers_map_.insert( + {provider->type(), + std::make_unique(std::initializer_list({provider}))}); + } else { + it->second->insert(provider); + } +} + +void ConfigProviderManagerImplBase::unbindImmutableConfigProvider( + ImmutableConfigProviderImplBase* provider) { + ASSERT(owner_tid_ == Thread::currentThreadId() && + (provider->type() == ConfigProviderInstanceType::Static || + provider->type() == ConfigProviderInstanceType::Inline)); + ConfigProviderMap::iterator it = immutable_config_providers_map_.find(provider->type()); + RELEASE_ASSERT(it != immutable_config_providers_map_.end(), + fmt::format("trying to unbind config provider, but failed to find type = {}", + provider->type())); + it->second->erase(provider); +} + } // namespace Config } // namespace Envoy diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 881872dec8e9f..1d001d48bf511 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -17,10 +17,11 @@ namespace Envoy { namespace Config { -// This file provides a set of base classes, (StaticConfigProviderImplBase, -// DynamicConfigProviderImplBase, ConfigProviderManagerImplBase, ConfigSubscriptionInstanceBase), +// This file provides a set of base classes, (ImmutableConfigProviderImplBase, +// MutableConfigProviderImplBase, ConfigProviderManagerImplBase, ConfigSubscriptionInstanceBase), // conforming to the ConfigProvider/ConfigProviderManager interfaces, which in tandem provide a -// framework for implementing static and dynamic configuration for Envoy. +// framework for implementing statically defined (i.e., immutable) and dynamic (mutable via +// subscriptions) configuration for Envoy. // // Dynamic configuration is distributed via xDS APIs (see // https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md). The framework exposed @@ -32,7 +33,7 @@ namespace Config { // This approach enables linear memory scalability based primarily on the size of the configuration // set. // -// A blueprint to follow for implementing static and dynamic config providers is as follows: +// A blueprint to follow for implementing {im,}mutable config providers is as follows: // // For both: // 1) Create a class derived from ConfigProviderManagerImplBase and implement the required @@ -41,12 +42,12 @@ namespace Config { // be called to fetch either an existing ConfigSubscriptionInstanceBase if the config source // configuration matches, or a newly instantiated subscription otherwise. // -// For static providers: -// 1) Create a class derived from StaticConfigProviderImplBase and implement the required +// For immutable providers: +// 1) Create a class derived from ImmutableConfigProviderImplBase and implement the required // interface. // -// For dynamic (xDS) providers: -// 1) Create a class derived from DynamicConfigProviderImplBase and implement the required +// For mutable (xDS) providers: +// 1) Create a class derived from MutableConfigProviderImplBase and implement the required // interface. // 2) Create a class derived from ConfigSubscriptionInstanceBase; this is the entity // responsible for owning and managing the Envoy::Config::Subscription that provides @@ -61,47 +62,67 @@ namespace Config { class ConfigProviderManagerImplBase; /** - * ConfigProvider implementation for statically specified configuration. + * Specifies the type of config associated with a ConfigProvider. + */ +enum class ConfigProviderInstanceType { + // Configuration defined as a static resource in the bootstrap config. + Static, + // Configuration defined inline in a resource that may be specified statically or obtained via + // xDS. + Inline, + // Configuration obtained from an xDS subscription. + xDS +}; +// Required for fmt::format(). +std::ostream& operator<<(std::ostream& os, ConfigProviderInstanceType type); + +/** + * ConfigProvider implementation for immutable configuration. * * TODO(AndresGuedez): support sharing of config protos and config impls, as is - * done with the DynamicConfigProviderImplBase. + * done with the MutableConfigProviderImplBase. * * This class can not be instantiated directly; instead, it provides the foundation for - * static config provider implementations which derive from it. + * immutable config provider implementations which derive from it. */ -class StaticConfigProviderImplBase : public ConfigProvider { +class ImmutableConfigProviderImplBase : public ConfigProvider { public: - virtual ~StaticConfigProviderImplBase(); + virtual ~ImmutableConfigProviderImplBase(); // Envoy::Config::ConfigProvider SystemTime lastUpdated() const override { return last_updated_; } + ConfigProviderInstanceType type() const { return type_; } + protected: - StaticConfigProviderImplBase(Server::Configuration::FactoryContext& factory_context, - ConfigProviderManagerImplBase& config_provider_manager); + ImmutableConfigProviderImplBase(Server::Configuration::FactoryContext& factory_context, + ConfigProviderManagerImplBase& config_provider_manager, + ConfigProviderInstanceType type); private: SystemTime last_updated_; ConfigProviderManagerImplBase& config_provider_manager_; + ConfigProviderInstanceType type_; }; -class DynamicConfigProviderImplBase; +class MutableConfigProviderImplBase; /** - * Provides generic functionality required by all dynamic ConfigProvider subscriptions, including + * Provides generic functionality required by all xDS ConfigProvider subscriptions, including * shared lifetime management via shared_ptr. * - * To do so, this class keeps track of a set of dynamic config providers associated with an - * underlying subscription; providers are bound/unbound as needed as they are created and destroyed. + * To do so, this class keeps track of a set of MutableConfigProviderImplBase instances associated + * with an underlying subscription; providers are bound/unbound as needed as they are created and + * destroyed. * - * Dynamic config providers and subscriptions are split to avoid lifetime issues with arguments + * xDS config providers and subscriptions are split to avoid lifetime issues with arguments * required by the config providers. An example is the Server::Configuration::FactoryContext, which * is owned by listeners and therefore may be destroyed while an associated config provider is still * in use (see #3960). This split enables single ownership of the config providers, while enabling * shared ownership of the underlying subscription. * * This class can not be instantiated directly; instead, it provides the foundation for - * dynamic config subscription implementations which derive from it. + * config subscription implementations which derive from it. */ class ConfigSubscriptionInstanceBase : public Init::Target, protected Logger::Loggable { @@ -157,12 +178,12 @@ class ConfigSubscriptionInstanceBase : public Init::Target, const std::string& version_info); /** - * Returns one of the bound dynamic config providers. - * @return const DynamicConfigProviderImplBase* a const pointer to a - * bound DynamicConfigProviderImplBase or nullptr when there are none. + * Returns one of the bound mutable config providers. + * @return const MutableConfigProviderImplBase* a const pointer to a + * bound MutableConfigProviderImplBase or nullptr when there are none. */ - const DynamicConfigProviderImplBase* getAnyBoundDynamicConfigProvider() const { - return !dynamic_config_providers_.empty() ? *dynamic_config_providers_.begin() : nullptr; + const MutableConfigProviderImplBase* getAnyBoundMutableConfigProvider() const { + return !mutable_config_providers_.empty() ? *mutable_config_providers_.begin() : nullptr; } protected: @@ -183,29 +204,29 @@ class ConfigSubscriptionInstanceBase : public Init::Target, private: void registerInitTarget(Init::Manager& init_manager) { init_manager.registerTarget(*this); } - void bindConfigProvider(DynamicConfigProviderImplBase* provider); + void bindConfigProvider(MutableConfigProviderImplBase* provider); - void unbindConfigProvider(DynamicConfigProviderImplBase* provider) { - dynamic_config_providers_.erase(provider); + void unbindConfigProvider(MutableConfigProviderImplBase* provider) { + mutable_config_providers_.erase(provider); } const std::string name_; std::function initialize_callback_; - std::unordered_set dynamic_config_providers_; + std::unordered_set mutable_config_providers_; const std::string manager_identifier_; ConfigProviderManagerImplBase& config_provider_manager_; TimeSource& time_source_; SystemTime last_updated_; absl::optional config_info_; - // ConfigSubscriptionInstanceBase, DynamicConfigProviderImplBase and ConfigProviderManagerImplBase + // ConfigSubscriptionInstanceBase, MutableConfigProviderImplBase and ConfigProviderManagerImplBase // are tightly coupled with the current shared ownership model; use friend classes to explicitly // denote the binding between them. // // TODO(AndresGuedez): Investigate whether a shared ownership model avoiding the s and // instead centralizing lifetime management in the ConfigProviderManagerImplBase with explicit // reference counting would be more maintainable. - friend class DynamicConfigProviderImplBase; + friend class MutableConfigProviderImplBase; friend class ConfigProviderManagerImplBase; }; @@ -218,9 +239,9 @@ using ConfigSubscriptionInstanceBaseSharedPtr = std::shared_ptrunbindConfigProvider(this); } + virtual ~MutableConfigProviderImplBase() { subscription_->unbindConfigProvider(this); } // Envoy::Config::ConfigProvider SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } @@ -254,7 +275,7 @@ class DynamicConfigProviderImplBase : public ConfigProvider { } protected: - DynamicConfigProviderImplBase(ConfigSubscriptionInstanceBaseSharedPtr&& subscription, + MutableConfigProviderImplBase(ConfigSubscriptionInstanceBaseSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context) : subscription_(subscription), tls_(factory_context.threadLocal().allocateSlot()) {} @@ -277,9 +298,9 @@ class DynamicConfigProviderImplBase : public ConfigProvider { * lifetime of subscriptions and dynamic config providers, along with determining which * subscriptions should be associated with newly instantiated providers. * - * The implementation of this class is not thread safe. Note that StaticConfigProviderImplBase and - * ConfigSubscriptionInstanceBase call the corresponding {bind,unbind}* functions exposed by this - * class. + * The implementation of this class is not thread safe. Note that ImmutableConfigProviderImplBase + * and ConfigSubscriptionInstanceBase call the corresponding {bind,unbind}* functions exposed by + * this class. * * All config processing is done on the main thread, so instantiation of *ConfigProvider* objects * via createStaticConfigProvider() and createXdsConfigProvider() is naturally thread safe. Care @@ -296,21 +317,28 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl /** * This is invoked by the /config_dump admin handler. * @return ProtobufTypes::MessagePtr the config dump proto corresponding to the associated - * {static,dynamic} config providers. + * config providers. */ virtual ProtobufTypes::MessagePtr dumpConfigs() const PURE; protected: using ConfigProviderSet = std::unordered_set; + using ConfigProviderMap = + std::unordered_map>; using ConfigSubscriptionMap = std::unordered_map>; ConfigProviderManagerImplBase(Server::Admin& admin, const std::string& config_name); - const ConfigProviderSet& staticConfigProviders() const { return static_config_providers_; } - const ConfigSubscriptionMap& configSubscriptions() const { return config_subscriptions_; } + /** + * Returns the set of bound ImmutableConfigProviderImplBase-derived providers of a given type. + * @param type supplies the type of config providers to return. + * @return const ConfigProviderSet* the set of config providers corresponding to the type. + */ + const ConfigProviderSet& immutableConfigProviders(ConfigProviderInstanceType type) const; + /** * Returns the subscription associated with the config_source_proto; if none exists, a new one is * allocated according to the subscription_factory_fn. @@ -367,21 +395,14 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl config_subscriptions_.erase(manager_identifier); } - void bindStaticConfigProvider(StaticConfigProviderImplBase* provider) { - ASSERT(owner_tid_ == Thread::currentThreadId()); - static_config_providers_.insert(provider); - } - - void unbindStaticConfigProvider(StaticConfigProviderImplBase* provider) { - ASSERT(owner_tid_ == Thread::currentThreadId()); - static_config_providers_.erase(provider); - } + void bindImmutableConfigProvider(ImmutableConfigProviderImplBase* provider); + void unbindImmutableConfigProvider(ImmutableConfigProviderImplBase* provider); // TODO(jsedgwick) These two members are prime candidates for the owned-entry list/map // as in ConfigTracker. I.e. the ProviderImpls would have an EntryOwner for these lists // Then the lifetime management stuff is centralized and opaque. ConfigSubscriptionMap config_subscriptions_; - ConfigProviderSet static_config_providers_; + ConfigProviderMap immutable_config_providers_map_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; Thread::ThreadId owner_tid_{}; @@ -389,7 +410,7 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl // See comment for friend classes in the ConfigSubscriptionInstanceBase for more details on the // use of friends. friend class ConfigSubscriptionInstanceBase; - friend class StaticConfigProviderImplBase; + friend class ImmutableConfigProviderImplBase; }; } // namespace Config diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 171dac24303ae..287acd519e3e0 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -15,7 +15,7 @@ namespace { class DummyConfigProviderManager; -class StaticDummyConfigProvider : public Envoy::Config::StaticConfigProviderImplBase { +class StaticDummyConfigProvider : public ImmutableConfigProviderImplBase { public: StaticDummyConfigProvider(const test::common::config::DummyConfig& config_proto, Server::Configuration::FactoryContext& factory_context, @@ -81,14 +81,14 @@ class DummyConfig : public ConfigProvider::Config { DummyConfig(const test::common::config::DummyConfig&) {} }; -class DummyDynamicConfigProvider : public DynamicConfigProviderImplBase { +class DummyDynamicConfigProvider : public MutableConfigProviderImplBase { public: DummyDynamicConfigProvider(DummyConfigSubscriptionSharedPtr&& subscription, ConfigConstSharedPtr initial_config, Server::Configuration::FactoryContext& factory_context) - : DynamicConfigProviderImplBase(std::move(subscription), factory_context), + : MutableConfigProviderImplBase(std::move(subscription), factory_context), subscription_(static_cast( - DynamicConfigProviderImplBase::subscription().get())) { + MutableConfigProviderImplBase::subscription().get())) { initialize(initial_config); } @@ -96,7 +96,7 @@ class DummyDynamicConfigProvider : public DynamicConfigProviderImplBase { DummyConfigSubscription& subscription() { return *subscription_; } - // Envoy::Config::DynamicConfigProviderImplBase + // Envoy::Config::MutableConfigProviderImplBase ConfigProvider::ConfigConstSharedPtr onConfigProtoUpdate(const Protobuf::Message& config) override { return std::make_shared( @@ -143,7 +143,7 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase { } } - for (const auto& provider : staticConfigProviders()) { + for (const auto* provider : immutableConfigProviders(ConfigProviderInstanceType::Static)) { ASSERT(provider->configProtoInfo()); auto* static_config = config_dump->mutable_static_dummy_configs()->Add(); static_config->mutable_dummy_config()->MergeFrom( @@ -170,8 +170,8 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase { }); ConfigProvider::ConfigConstSharedPtr initial_config; - const DynamicConfigProviderImplBase* provider = - subscription->getAnyBoundDynamicConfigProvider(); + const MutableConfigProviderImplBase* provider = + subscription->getAnyBoundMutableConfigProvider(); if (provider) { initial_config = provider->getConfig(); } @@ -193,7 +193,8 @@ StaticDummyConfigProvider::StaticDummyConfigProvider( const test::common::config::DummyConfig& config_proto, Server::Configuration::FactoryContext& factory_context, DummyConfigProviderManager& config_provider_manager) - : StaticConfigProviderImplBase(factory_context, config_provider_manager), + : ImmutableConfigProviderImplBase(factory_context, config_provider_manager, + ConfigProviderInstanceType::Static), config_(std::make_shared(config_proto)), config_proto_(config_proto) {} DummyConfigSubscription::DummyConfigSubscription( From 7441143d9da63374888458920a072bfc28f326af Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 21 Dec 2018 10:44:33 -0500 Subject: [PATCH 18/25] Resolve build issue with GCC 5.X. Also, removes thread safety assertions due to changes introduced in (#5072); it is no longer possible to statically call functions to obtain/check the current thread ID. I will reintroduce these checks once static accessors are available again. Signed-off-by: Andres Guedez --- source/common/common/utility.h | 11 +++++++++++ source/common/config/config_provider_impl.cc | 19 ++++++++----------- source/common/config/config_provider_impl.h | 11 ++++------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 86e77eb46d619..89db4c2ae0151 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -523,6 +523,17 @@ struct StringViewHash { std::size_t operator()(const absl::string_view& k) const { return HashUtil::xxHash64(k); } }; +/** + * Hashing functor for use with enum class types. + * This is needed for GCC 5.X; newer versions of GCC, as well as clang7, provide native hashing + * specializations. + */ +struct EnumClassHash { + template std::size_t operator()(T t) const { + return std::hash()(static_cast(t)); + } +}; + /** * Computes running standard-deviation using Welford's algorithm: * https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Online_algorithm diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 557edc7b65456..8b6d07d45eb79 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -73,8 +73,7 @@ void ConfigSubscriptionInstanceBase::bindConfigProvider(MutableConfigProviderImp } ConfigProviderManagerImplBase::ConfigProviderManagerImplBase(Server::Admin& admin, - const std::string& config_name) - : owner_tid_(Thread::currentThreadId()) { + 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 @@ -95,15 +94,14 @@ ConfigProviderManagerImplBase::immutableConfigProviders(ConfigProviderInstanceTy void ConfigProviderManagerImplBase::bindImmutableConfigProvider( ImmutableConfigProviderImplBase* provider) { - ASSERT(owner_tid_ == Thread::currentThreadId() && - (provider->type() == ConfigProviderInstanceType::Static || - provider->type() == ConfigProviderInstanceType::Inline)); + ASSERT(provider->type() == ConfigProviderInstanceType::Static || + provider->type() == ConfigProviderInstanceType::Inline); ConfigProviderMap::iterator it; if ((it = immutable_config_providers_map_.find(provider->type())) == immutable_config_providers_map_.end()) { - immutable_config_providers_map_.insert( - {provider->type(), - std::make_unique(std::initializer_list({provider}))}); + immutable_config_providers_map_.insert(std::make_pair( + provider->type(), + std::make_unique(std::initializer_list({provider})))); } else { it->second->insert(provider); } @@ -111,9 +109,8 @@ void ConfigProviderManagerImplBase::bindImmutableConfigProvider( void ConfigProviderManagerImplBase::unbindImmutableConfigProvider( ImmutableConfigProviderImplBase* provider) { - ASSERT(owner_tid_ == Thread::currentThreadId() && - (provider->type() == ConfigProviderInstanceType::Static || - provider->type() == ConfigProviderInstanceType::Inline)); + ASSERT(provider->type() == ConfigProviderInstanceType::Static || + provider->type() == ConfigProviderInstanceType::Inline); ConfigProviderMap::iterator it = immutable_config_providers_map_.find(provider->type()); RELEASE_ASSERT(it != immutable_config_providers_map_.end(), fmt::format("trying to unbind config provider, but failed to find type = {}", diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 1d001d48bf511..4615cb3bd43c3 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -11,6 +11,7 @@ #include "envoy/thread_local/thread_local.h" #include "common/common/thread.h" +#include "common/common/utility.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" @@ -73,6 +74,7 @@ enum class ConfigProviderInstanceType { // Configuration obtained from an xDS subscription. xDS }; + // Required for fmt::format(). std::ostream& operator<<(std::ostream& os, ConfigProviderInstanceType type); @@ -323,8 +325,8 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl protected: using ConfigProviderSet = std::unordered_set; - using ConfigProviderMap = - std::unordered_map>; + using ConfigProviderMap = std::unordered_map, EnumClassHash>; using ConfigSubscriptionMap = std::unordered_map>; @@ -354,8 +356,6 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl std::function subscription_factory_fn) { - ASSERT(owner_tid_ == Thread::currentThreadId()); - static_assert(std::is_base_of::value, "T must be a subclass of ConfigSubscriptionInstanceBase"); @@ -386,12 +386,10 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl private: void bindSubscription(const std::string& manager_identifier, ConfigSubscriptionInstanceBaseSharedPtr& subscription) { - ASSERT(owner_tid_ == Thread::currentThreadId()); config_subscriptions_.insert({manager_identifier, subscription}); } void unbindSubscription(const std::string& manager_identifier) { - ASSERT(owner_tid_ == Thread::currentThreadId()); config_subscriptions_.erase(manager_identifier); } @@ -405,7 +403,6 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl ConfigProviderMap immutable_config_providers_map_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; - Thread::ThreadId owner_tid_{}; // See comment for friend classes in the ConfigSubscriptionInstanceBase for more details on the // use of friends. From a36a3e8cc4d2cb56468511834ee1420339381cf1 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 21 Dec 2018 13:45:00 -0500 Subject: [PATCH 19/25] clang-tidy cleanup. Signed-off-by: Andres Guedez --- include/envoy/config/config_provider.h | 4 ++-- .../envoy/config/config_provider_manager.h | 2 +- source/common/config/config_provider_impl.cc | 8 +++---- source/common/config/config_provider_impl.h | 23 +++++++++---------- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index 6bfb6012bb204..e383fa6cbba8c 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -27,7 +27,7 @@ class ConfigProvider { */ class Config { public: - virtual ~Config() {} + virtual ~Config() = default; }; using ConfigConstSharedPtr = std::shared_ptr; @@ -41,7 +41,7 @@ class ConfigProvider { std::string version_; }; - virtual ~ConfigProvider() {} + virtual ~ConfigProvider() = default; /** * Returns a ConfigProtoInfo associated with the provider. diff --git a/include/envoy/config/config_provider_manager.h b/include/envoy/config/config_provider_manager.h index 1ab8449eeac5d..3c3eddba9c2f0 100644 --- a/include/envoy/config/config_provider_manager.h +++ b/include/envoy/config/config_provider_manager.h @@ -25,7 +25,7 @@ namespace Config { */ class ConfigProviderManager { public: - virtual ~ConfigProviderManager() {} + virtual ~ConfigProviderManager() = default; /** * Returns a dynamic ConfigProvider which receives configuration via an xDS API. diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 8b6d07d45eb79..81572abae4a8a 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -89,7 +89,7 @@ ConfigProviderManagerImplBase::immutableConfigProviders(ConfigProviderInstanceTy return empty_set; } - return *it->second.get(); + return *it->second; } void ConfigProviderManagerImplBase::bindImmutableConfigProvider( @@ -111,10 +111,8 @@ void ConfigProviderManagerImplBase::unbindImmutableConfigProvider( ImmutableConfigProviderImplBase* provider) { ASSERT(provider->type() == ConfigProviderInstanceType::Static || provider->type() == ConfigProviderInstanceType::Inline); - ConfigProviderMap::iterator it = immutable_config_providers_map_.find(provider->type()); - RELEASE_ASSERT(it != immutable_config_providers_map_.end(), - fmt::format("trying to unbind config provider, but failed to find type = {}", - provider->type())); + auto it = immutable_config_providers_map_.find(provider->type()); + ASSERT(it != immutable_config_providers_map_.end()); it->second->erase(provider); } diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 4615cb3bd43c3..f39705a8c6961 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -89,7 +89,7 @@ std::ostream& operator<<(std::ostream& os, ConfigProviderInstanceType type); */ class ImmutableConfigProviderImplBase : public ConfigProvider { public: - virtual ~ImmutableConfigProviderImplBase(); + ~ImmutableConfigProviderImplBase() override; // Envoy::Config::ConfigProvider SystemTime lastUpdated() const override { return last_updated_; } @@ -134,7 +134,7 @@ class ConfigSubscriptionInstanceBase : public Init::Target, std::string last_config_version_; }; - virtual ~ConfigSubscriptionInstanceBase(); + ~ConfigSubscriptionInstanceBase() override; // Init::Target void initialize(std::function callback) override { @@ -243,7 +243,7 @@ using ConfigSubscriptionInstanceBaseSharedPtr = std::shared_ptrunbindConfigProvider(this); } + ~MutableConfigProviderImplBase() override { subscription_->unbindConfigProvider(this); } // Envoy::Config::ConfigProvider SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } @@ -260,7 +260,7 @@ class MutableConfigProviderImplBase : public ConfigProvider { * @param initial_config supplies an initial Envoy::Config::ConfigProvider::Config associated with * the underlying subscription. */ - void initialize(ConfigConstSharedPtr initial_config) { + void initialize(const ConfigConstSharedPtr& initial_config) { subscription_->bindConfigProvider(this); tls_->set([initial_config](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(initial_config); @@ -271,7 +271,7 @@ class MutableConfigProviderImplBase : public ConfigProvider { * Propagates a newly instantiated Envoy::Config::ConfigProvider::Config to all workers. * @param config supplies the newly instantiated config. */ - void onConfigUpdate(ConfigConstSharedPtr config) { + void onConfigUpdate(const ConfigConstSharedPtr& config) { tls_->runOnAllThreads( [this, config]() -> void { tls_->getTyped().config_ = config; }); } @@ -286,7 +286,7 @@ class MutableConfigProviderImplBase : public ConfigProvider { private: struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { ThreadLocalConfig(ConfigProvider::ConfigConstSharedPtr initial_config) - : config_(initial_config) {} + : config_(std::move(initial_config)) {} ConfigProvider::ConfigConstSharedPtr config_; }; @@ -314,7 +314,7 @@ class MutableConfigProviderImplBase : public ConfigProvider { */ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singleton::Instance { public: - virtual ~ConfigProviderManagerImplBase() {} + ~ConfigProviderManagerImplBase() override = default; /** * This is invoked by the /config_dump admin handler. @@ -351,11 +351,10 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl * @return std::shared_ptr an existing (if a match is found) or newly allocated subscription. */ template - std::shared_ptr getSubscription(const Protobuf::Message& config_source_proto, - Init::Manager& init_manager, - std::function - subscription_factory_fn) { + std::shared_ptr getSubscription( + const Protobuf::Message& config_source_proto, Init::Manager& init_manager, + const std::function& subscription_factory_fn) { static_assert(std::is_base_of::value, "T must be a subclass of ConfigSubscriptionInstanceBase"); From bb34fddf55ba5b0b038a45b4c368e74fe76c08d2 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 21 Dec 2018 13:48:13 -0500 Subject: [PATCH 20/25] Remove unused function. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.cc | 4 ---- source/common/config/config_provider_impl.h | 3 --- 2 files changed, 7 deletions(-) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 81572abae4a8a..5fc05d933ab60 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -3,10 +3,6 @@ namespace Envoy { namespace Config { -std::ostream& operator<<(std::ostream& os, ConfigProviderInstanceType type) { - return os << static_cast(type); -} - ImmutableConfigProviderImplBase::ImmutableConfigProviderImplBase( Server::Configuration::FactoryContext& factory_context, ConfigProviderManagerImplBase& config_provider_manager, ConfigProviderInstanceType type) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index f39705a8c6961..35871f4f8206b 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -75,9 +75,6 @@ enum class ConfigProviderInstanceType { xDS }; -// Required for fmt::format(). -std::ostream& operator<<(std::ostream& os, ConfigProviderInstanceType type); - /** * ConfigProvider implementation for immutable configuration. * From 6d7cba88cfa663275087ea29212ead0524115aea Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 27 Dec 2018 15:10:50 -0500 Subject: [PATCH 21/25] Add comments. Signed-off-by: Andres Guedez --- source/common/config/config_provider_impl.h | 25 +++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 35871f4f8206b..12d6fa73665f6 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -24,6 +24,14 @@ namespace Config { // framework for implementing statically defined (i.e., immutable) and dynamic (mutable via // subscriptions) configuration for Envoy. // +// The mutability property applies to the ConfigProvider itself and _not_ the underlying config +// proto, which is always immutable. MutableConfigProviderImplBase objects receive config proto +// updates via xDS subscriptions, resulting in new ConfigProvider::Config objects being instantiated +// with the corresponding change in behavior corresponding to updated config. ConfigProvider::Config +// objects must be latched/associated with the appropriate objects in the connection and request +// processing pipeline, such that configuration stays consistent for the lifetime of the connection +// and/or stream/request (if required by the configuration being processed). +// // Dynamic configuration is distributed via xDS APIs (see // https://github.com/envoyproxy/data-plane-api/blob/master/XDS_PROTOCOL.md). The framework exposed // by these classes simplifies creation of client xDS implementations following a shared ownership @@ -59,6 +67,8 @@ namespace Config { // - On a successful config update, checkAndApplyConfig() should be called to instantiate the // new config implementation and propagate it to the shared config providers and all // worker threads. +// - On a successful return from checkAndApplyConfig(), the config proto must be latched into +// this class and returned via the getConfigProto() override. class ConfigProviderManagerImplBase; @@ -72,7 +82,7 @@ enum class ConfigProviderInstanceType { // xDS. Inline, // Configuration obtained from an xDS subscription. - xDS + Xds }; /** @@ -250,6 +260,16 @@ class MutableConfigProviderImplBase : public ConfigProvider { return tls_->getTyped().config_; } + /** + * Called when a new config proto is received via an xDS subscription. + * On successful validation of the config, must return a shared_ptr to a ConfigProvider::Config + * implementation that will be propagated to all mutable config providers sharing the + * subscription. + * Note that this function is called _once_ across all shared config providers per xDS + * subscription config update. + * @param config_proto supplies the configuration proto. + * @return ConfigConstSharedPtr the ConfigProvider::Config to share with other providers. + */ virtual ConfigConstSharedPtr onConfigProtoUpdate(const Protobuf::Message& config_proto) PURE; /** @@ -304,7 +324,8 @@ class MutableConfigProviderImplBase : public ConfigProvider { * All config processing is done on the main thread, so instantiation of *ConfigProvider* objects * via createStaticConfigProvider() and createXdsConfigProvider() is naturally thread safe. Care * must be taken with regards to destruction of these objects, since it must also happen on the main - * thread. + * thread _prior_ to destruction of the ConfigProviderManagerImplBase oject from which they were + * created. * * This class can not be instantiated directly; instead, it provides the foundation for * dynamic config provider implementations which derive from it. From 9cf7f27864cc1b940f8f8dbf0631a6e2013a505a Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 27 Dec 2018 15:38:54 -0500 Subject: [PATCH 22/25] clang-tidy cleanup. Signed-off-by: Andres Guedez --- test/common/config/config_provider_impl_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 287acd519e3e0..7e928963aed9f 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -21,7 +21,7 @@ class StaticDummyConfigProvider : public ImmutableConfigProviderImplBase { Server::Configuration::FactoryContext& factory_context, DummyConfigProviderManager& config_provider_manager); - ~StaticDummyConfigProvider() {} + ~StaticDummyConfigProvider() override = default; // Envoy::Config::ConfigProvider const Protobuf::Message* getConfigProto() const override { return &config_proto_; } @@ -45,7 +45,7 @@ class DummyConfigSubscription Server::Configuration::FactoryContext& factory_context, DummyConfigProviderManager& config_provider_manager); - ~DummyConfigSubscription() {} + ~DummyConfigSubscription() override = default; // Envoy::Config::ConfigSubscriptionInstanceBase void start() override {} @@ -92,7 +92,7 @@ class DummyDynamicConfigProvider : public MutableConfigProviderImplBase { initialize(initial_config); } - ~DummyDynamicConfigProvider() {} + ~DummyDynamicConfigProvider() override = default; DummyConfigSubscription& subscription() { return *subscription_; } @@ -124,7 +124,7 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase { DummyConfigProviderManager(Server::Admin& admin) : ConfigProviderManagerImplBase(admin, "dummy") {} - ~DummyConfigProviderManager() {} + ~DummyConfigProviderManager() override = default; // Envoy::Config::ConfigProviderManagerImplBase ProtobufTypes::MessagePtr dumpConfigs() const override { From 58ac876b69e890d1262449ffcec7a56ca606a20b Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 27 Dec 2018 23:09:33 -0500 Subject: [PATCH 23/25] Comments. Signed-off-by: Andres Guedez --- include/envoy/config/config_provider.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index e383fa6cbba8c..f63f01b706993 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -12,7 +12,17 @@ namespace Envoy { namespace Config { /** - * A provider for configuration obtained ether statically or dynamically via xDS APIs. + * A provider for configuration obtained statically (via static resources in the bootstrap config), + * inline with a higher level resource or dynamically via xDS APIs. + * + * The ConfigProvider is an abstraction layer which higher level components such as the + * HttpConnectionManager, Listener, etc can leverage to interface with Envoy's configuration + * mechanisms. Implementations of this interface build upon lower level abstractions such as + * Envoy::Config::Subscription and Envoy::Config::SubscriptionCallbacks. + * + * The interface exposed below allows xDS providers to share the underlying config protos and + * resulting config implementations (i.e., the ConfigProvider::Config); this enables linear memory + * scaling based on the size of the configuration set, regardless of the number of threads/workers. * * Use config() to obtain a shared_ptr to the implementation of the config, and configProtoInfo() to * obtain a reference to the underlying config proto and version (applicable only to dynamic config From bb990fab6e399790a44f3c44f0c0e5be04df8f3d Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 31 Dec 2018 10:01:12 -0500 Subject: [PATCH 24/25] Cleanup. - Removed ASSERT_IGNORE_POTENTIALLY_EVALUATED() and replaced with an ASSERT(lambda) that bypasses the issue. Signed-off-by: Andres Guedez --- source/common/common/assert.h | 22 -------------------- source/common/config/config_provider_impl.cc | 17 +++++++++------ 2 files changed, 11 insertions(+), 28 deletions(-) diff --git a/source/common/common/assert.h b/source/common/common/assert.h index fc2617a5beaae..d19270e27296e 100644 --- a/source/common/common/assert.h +++ b/source/common/common/assert.h @@ -48,28 +48,6 @@ namespace Envoy { } while (false) #endif -#ifndef NDEBUG -#if __clang__ -#define __CLANG_PRAGMA(X) _Pragma(X) -#else -#define __CLANG_PRAGMA(X) -#endif - -// clang-format off - -#define ASSERT_IGNORE_POTENTIALLY_EVALUATED(COND) \ - do { \ - __CLANG_PRAGMA("clang diagnostic push") \ - __CLANG_PRAGMA("clang diagnostic ignored \"-Wpotentially-evaluated-expression\"") \ - ASSERT(COND); \ - __CLANG_PRAGMA("clang diagnostic pop") \ - } while (0) -#else // NDEBUG -#define ASSERT_IGNORE_POTENTIALLY_EVALUATED(COND) -#endif - -// clang-format on - /** * Indicate a panic situation and exit. */ diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 5fc05d933ab60..541c767412aac 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -59,12 +59,17 @@ bool ConfigSubscriptionInstanceBase::checkAndApplyConfig(const Protobuf::Message void ConfigSubscriptionInstanceBase::bindConfigProvider(MutableConfigProviderImplBase* provider) { // All config providers bound to a ConfigSubscriptionInstanceBase must be of the same concrete - // type; this is assumed by checkAndApplyConfig() and is verified by the assertion below. NOTE: a - // regular ASSERT() triggers a potentially evaluated expression warning from clang due to the args - // passed to the second typeid() call. - ASSERT_IGNORE_POTENTIALLY_EVALUATED(mutable_config_providers_.empty() || - typeid(*provider) == - typeid(**mutable_config_providers_.begin())); + // type; this is assumed by checkAndApplyConfig() and is verified by the assertion below. + // NOTE: an inlined statement ASSERT() triggers a potentially evaluated expression warning from + // clang due to `typeid(**mutable_config_providers_.begin())`. To avoid this, we use a lambda to + // separate the first mutable provider dereference from the typeid() statement. + ASSERT([&]() { + if (!mutable_config_providers_.empty()) { + const auto& first_provider = **mutable_config_providers_.begin(); + return typeid(*provider) == typeid(first_provider); + } + return true; + }()); mutable_config_providers_.insert(provider); } From b49127f02e0fff4caa0383f80590a2c2f584b094 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Wed, 2 Jan 2019 09:37:18 -0500 Subject: [PATCH 25/25] Minor cleanup. Signed-off-by: Andres Guedez --- include/envoy/config/config_provider.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/config/config_provider.h b/include/envoy/config/config_provider.h index f63f01b706993..a42f512dea4e9 100644 --- a/include/envoy/config/config_provider.h +++ b/include/envoy/config/config_provider.h @@ -64,7 +64,7 @@ class ConfigProvider { const auto* config_proto = dynamic_cast(getConfigProto()); if (config_proto == nullptr) { - return {}; + return absl::nullopt; } return ConfigProtoInfo

{*config_proto, getConfigVersion()}; }