diff --git a/include/envoy/init/BUILD b/include/envoy/init/BUILD index cfa069239b96f..2229d7c7a12e4 100644 --- a/include/envoy/init/BUILD +++ b/include/envoy/init/BUILD @@ -9,6 +9,23 @@ load( envoy_package() envoy_cc_library( - name = "init_interface", - hdrs = ["init.h"], + name = "watcher_interface", + hdrs = ["watcher.h"], +) + +envoy_cc_library( + name = "target_interface", + hdrs = ["target.h"], + deps = [ + ":watcher_interface", + ], +) + +envoy_cc_library( + name = "manager_interface", + hdrs = ["manager.h"], + deps = [ + ":target_interface", + ":watcher_interface", + ], ) diff --git a/include/envoy/init/init.h b/include/envoy/init/init.h deleted file mode 100644 index 338511c3545b8..0000000000000 --- a/include/envoy/init/init.h +++ /dev/null @@ -1,66 +0,0 @@ -#pragma once - -#include - -#include "envoy/common/pure.h" - -#include "absl/strings/string_view.h" - -namespace Envoy { -namespace Init { - -/** - * A single initialization target. Deprecated, use SafeInit::Target instead. - * TODO(mergeconflict): convert all Init::Target implementations to SafeInit::TargetImpl. - */ -class Target { -public: - virtual ~Target() {} - - /** - * Called when the target should begin its own initialization. - * @param callback supplies the callback to invoke when the target has completed its - * initialization. - */ - virtual void initialize(std::function callback) PURE; -}; - -/** - * A manager that initializes multiple targets. Deprecated, use SafeInit::Manager instead. - * TODO(mergeconflict): convert all Init::Manager uses to SafeInit::Manager. - */ -class Manager { -public: - virtual ~Manager() {} - - /** - * Register a target to be initialized in the future. The manager will call initialize() on each - * target at some point in the future. It is an error to register the same target more than once. - * @param target the Target to initialize. - * @param description a human-readable description of target used for logging and debugging. - */ - virtual void registerTarget(Target& target, absl::string_view description) PURE; - - enum class State { - /** - * Targets have not been initialized. - */ - NotInitialized, - /** - * Targets are currently being initialized. - */ - Initializing, - /** - * All targets have been initialized. - */ - Initialized - }; - - /** - * Returns the current state of the init manager. - */ - virtual State state() const PURE; -}; - -} // namespace Init -} // namespace Envoy diff --git a/include/envoy/safe_init/manager.h b/include/envoy/init/manager.h similarity index 89% rename from include/envoy/safe_init/manager.h rename to include/envoy/init/manager.h index a94718fbd2869..94cf0dbb25e1a 100644 --- a/include/envoy/safe_init/manager.h +++ b/include/envoy/init/manager.h @@ -1,14 +1,14 @@ #pragma once #include "envoy/common/pure.h" -#include "envoy/safe_init/target.h" -#include "envoy/safe_init/watcher.h" +#include "envoy/init/target.h" +#include "envoy/init/watcher.h" namespace Envoy { -namespace SafeInit { +namespace Init { /** - * SafeInit::Manager coordinates initialization of one or more "targets." A typical flow would be: + * Init::Manager coordinates initialization of one or more "targets." A typical flow would be: * * - One or more initialization targets are registered with a manager using `add`. * - The manager is told to `initialize` all its targets, given a Watcher to notify when all @@ -21,14 +21,14 @@ namespace SafeInit { * Since there are several entities involved in this flow -- the owner of the manager, the targets * registered with the manager, and the manager itself -- it may be difficult or impossible in some * cases to guarantee that their lifetimes line up correctly to avoid use-after-free errors. The - * interface design here in SafeInit allows implementations to avoid the issue: + * interface design here in Init allows implementations to avoid the issue: * * - A Target can only be initialized via a TargetHandle, which acts as a weak reference. * Attempting to initialize a destroyed Target via its handle has no ill effects. * - Likewise, a Watcher can only be notified that initialization was complete via a * WatcherHandle, which acts as a weak reference as well. * - * See target.h and watcher.h, as well as implementation in source/common/safe_init for details. + * See target.h and watcher.h, as well as implementation in source/common/init for details. */ struct Manager { virtual ~Manager() = default; @@ -75,5 +75,5 @@ struct Manager { virtual void initialize(const Watcher& watcher) PURE; }; -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/include/envoy/safe_init/target.h b/include/envoy/init/target.h similarity index 82% rename from include/envoy/safe_init/target.h rename to include/envoy/init/target.h index 25dd958d3a646..9ab46d38aff48 100644 --- a/include/envoy/safe_init/target.h +++ b/include/envoy/init/target.h @@ -3,17 +3,17 @@ #include #include "envoy/common/pure.h" -#include "envoy/safe_init/watcher.h" +#include "envoy/init/watcher.h" #include "absl/strings/string_view.h" namespace Envoy { -namespace SafeInit { +namespace Init { /** * A TargetHandle functions as a weak reference to a Target. It is how an implementation of - * SafeInit::Manager would safely tell a target to `initialize` with no guarantees about the - * target's lifetime. Typical usage (outside of SafeInit::ManagerImpl) does not require touching + * Init::Manager would safely tell a target to `initialize` with no guarantees about the + * target's lifetime. Typical usage (outside of Init::ManagerImpl) does not require touching * TargetHandles at all. */ struct TargetHandle { @@ -48,5 +48,5 @@ struct Target { virtual TargetHandlePtr createHandle(absl::string_view name) const PURE; }; -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/include/envoy/safe_init/watcher.h b/include/envoy/init/watcher.h similarity index 80% rename from include/envoy/safe_init/watcher.h rename to include/envoy/init/watcher.h index b9eb0cf08959e..ccf17adfcbafc 100644 --- a/include/envoy/safe_init/watcher.h +++ b/include/envoy/init/watcher.h @@ -7,14 +7,14 @@ #include "absl/strings/string_view.h" namespace Envoy { -namespace SafeInit { +namespace Init { /** * A WatcherHandle functions as a weak reference to a Watcher. It is how an implementation of - * SafeInit::Target would safely notify a Manager that it has initialized, and likewise it's how - * an implementation of SafeInit::Manager would safely tell its client that all registered targets + * Init::Target would safely notify a Manager that it has initialized, and likewise it's how + * an implementation of Init::Manager would safely tell its client that all registered targets * have initialized, with no guarantees about the lifetimes of the manager or client. Typical usage - * (outside of SafeInit::TargetImpl and ManagerImpl) does not require touching WatcherHandles at + * (outside of Init::TargetImpl and ManagerImpl) does not require touching WatcherHandles at * all. */ struct WatcherHandle { @@ -49,5 +49,5 @@ struct Watcher { virtual WatcherHandlePtr createHandle(absl::string_view name) const PURE; }; -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/include/envoy/router/BUILD b/include/envoy/router/BUILD index f47a6db92ec04..1952414e09d19 100644 --- a/include/envoy/router/BUILD +++ b/include/envoy/router/BUILD @@ -20,7 +20,6 @@ envoy_cc_library( deps = [ ":rds_interface", "//include/envoy/event:dispatcher_interface", - "//include/envoy/init:init_interface", "//include/envoy/json:json_object_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/runtime:runtime_interface", diff --git a/include/envoy/router/route_config_provider_manager.h b/include/envoy/router/route_config_provider_manager.h index 3070a8dc407d9..9bf02066a6009 100644 --- a/include/envoy/router/route_config_provider_manager.h +++ b/include/envoy/router/route_config_provider_manager.h @@ -4,7 +4,6 @@ #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" #include "envoy/event/dispatcher.h" -#include "envoy/init/init.h" #include "envoy/json/json_object.h" #include "envoy/local_info/local_info.h" #include "envoy/router/rds.h" diff --git a/include/envoy/safe_init/BUILD b/include/envoy/safe_init/BUILD deleted file mode 100644 index 2229d7c7a12e4..0000000000000 --- a/include/envoy/safe_init/BUILD +++ /dev/null @@ -1,31 +0,0 @@ -licenses(["notice"]) # Apache 2 - -load( - "//bazel:envoy_build_system.bzl", - "envoy_cc_library", - "envoy_package", -) - -envoy_package() - -envoy_cc_library( - name = "watcher_interface", - hdrs = ["watcher.h"], -) - -envoy_cc_library( - name = "target_interface", - hdrs = ["target.h"], - deps = [ - ":watcher_interface", - ], -) - -envoy_cc_library( - name = "manager_interface", - hdrs = ["manager.h"], - deps = [ - ":target_interface", - ":watcher_interface", - ], -) diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 536710d325617..c81fcccfb656e 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -99,7 +99,7 @@ envoy_cc_library( "//include/envoy/event:timer_interface", "//include/envoy/http:context_interface", "//include/envoy/http:query_params_interface", - "//include/envoy/init:init_interface", + "//include/envoy/init:manager_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/secret:secret_manager_interface", @@ -151,7 +151,7 @@ envoy_cc_library( "//include/envoy/http:codes_interface", "//include/envoy/http:context_interface", "//include/envoy/http:filter_interface", - "//include/envoy/init:init_interface", + "//include/envoy/init:manager_interface", "//include/envoy/json:json_object_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/network:drain_decision_interface", @@ -196,7 +196,7 @@ envoy_cc_library( hdrs = ["transport_socket_config.h"], deps = [ "//include/envoy/event:dispatcher_interface", - "//include/envoy/init:init_interface", + "//include/envoy/init:manager_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/network:transport_socket_interface", "//include/envoy/runtime:runtime_interface", diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index 0f9234addd7d8..2f433b1e6b656 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -7,7 +7,7 @@ #include "envoy/http/codes.h" #include "envoy/http/context.h" #include "envoy/http/filter.h" -#include "envoy/init/init.h" +#include "envoy/init/manager.h" #include "envoy/json/json_object.h" #include "envoy/network/drain_decision.h" #include "envoy/network/filter.h" diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index a9be0d78ec56d..324960b35ffa0 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -9,7 +9,7 @@ #include "envoy/common/mutex_tracer.h" #include "envoy/event/timer.h" #include "envoy/http/context.h" -#include "envoy/init/init.h" +#include "envoy/init/manager.h" #include "envoy/local_info/local_info.h" #include "envoy/network/listen_socket.h" #include "envoy/runtime/runtime.h" diff --git a/include/envoy/server/transport_socket_config.h b/include/envoy/server/transport_socket_config.h index 15236bd5700ec..939846e08b23a 100644 --- a/include/envoy/server/transport_socket_config.h +++ b/include/envoy/server/transport_socket_config.h @@ -3,7 +3,7 @@ #include #include "envoy/event/dispatcher.h" -#include "envoy/init/init.h" +#include "envoy/init/manager.h" #include "envoy/local_info/local_info.h" #include "envoy/network/transport_socket.h" #include "envoy/runtime/runtime.h" diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 8071c1855276b..0d41c3e672e7a 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -391,11 +391,12 @@ envoy_cc_library( ":utility_lib", "//include/envoy/config:config_provider_interface", "//include/envoy/config:config_provider_manager_interface", - "//include/envoy/init:init_interface", + "//include/envoy/init:manager_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/init:target_lib", "//source/common/protobuf", ], ) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 541c767412aac..da3d65043a96d 100644 --- a/source/common/config/config_provider_impl.cc +++ b/source/common/config/config_provider_impl.cc @@ -16,17 +16,10 @@ ImmutableConfigProviderImplBase::~ImmutableConfigProviderImplBase() { } ConfigSubscriptionInstanceBase::~ConfigSubscriptionInstanceBase() { - runInitializeCallbackIfAny(); + init_target_.ready(); config_provider_manager_.unbindSubscription(manager_identifier_); } -void ConfigSubscriptionInstanceBase::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } -} - bool ConfigSubscriptionInstanceBase::checkAndApplyConfig(const Protobuf::Message& config_proto, const std::string& config_name, const std::string& version_info) { diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index b865165cd4f44..50e916d1d0348 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -4,7 +4,7 @@ #include "envoy/config/config_provider.h" #include "envoy/config/config_provider_manager.h" -#include "envoy/init/init.h" +#include "envoy/init/manager.h" #include "envoy/server/admin.h" #include "envoy/server/config_tracker.h" #include "envoy/singleton/instance.h" @@ -13,6 +13,7 @@ #include "common/common/thread.h" #include "common/common/utility.h" #include "common/config/utility.h" +#include "common/init/target_impl.h" #include "common/protobuf/protobuf.h" namespace Envoy { @@ -133,21 +134,14 @@ class MutableConfigProviderImplBase; * This class can not be instantiated directly; instead, it provides the foundation for * config subscription implementations which derive from it. */ -class ConfigSubscriptionInstanceBase : public Init::Target, - protected Logger::Loggable { +class ConfigSubscriptionInstanceBase : protected Logger::Loggable { public: struct LastConfigInfo { uint64_t last_config_hash_; std::string last_config_version_; }; - ~ConfigSubscriptionInstanceBase() override; - - // Init::Target - void initialize(std::function callback) override { - initialize_callback_ = callback; - start(); - } + virtual ~ConfigSubscriptionInstanceBase(); /** * Starts the subscription corresponding to a config source. @@ -166,14 +160,14 @@ class ConfigSubscriptionInstanceBase : public Init::Target, */ void onConfigUpdate() { setLastUpdated(); - runInitializeCallbackIfAny(); + init_target_.ready(); } /** * Must be called by derived classes when the onConfigUpdateFailed() callback associated with the * underlying subscription is issued. */ - void onConfigUpdateFailed() { runInitializeCallbackIfAny(); } + void onConfigUpdateFailed() { init_target_.ready(); } /** * Determines whether a configuration proto is a new update, and if so, propagates it to all @@ -200,21 +194,16 @@ class ConfigSubscriptionInstanceBase : public Init::Target, 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) { + : name_(name), init_target_(fmt::format("ConfigSubscriptionInstanceBase {}", name_), + [this]() { start(); }), + 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, fmt::format("ConfigSubscriptionInstanceBase {}", name_)); - } - void bindConfigProvider(MutableConfigProviderImplBase* provider); void unbindConfigProvider(MutableConfigProviderImplBase* provider) { @@ -222,7 +211,7 @@ class ConfigSubscriptionInstanceBase : public Init::Target, } const std::string name_; - std::function initialize_callback_; + Init::TargetImpl init_target_; std::unordered_set mutable_config_providers_; const uint64_t manager_identifier_; ConfigProviderManagerImplBase& config_provider_manager_; @@ -387,8 +376,7 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl // 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); + init_manager.add(subscription->init_target_); bindSubscription(manager_identifier, subscription); } else { diff --git a/source/common/safe_init/BUILD b/source/common/init/BUILD similarity index 80% rename from source/common/safe_init/BUILD rename to source/common/init/BUILD index 269cd9fbaace6..6fef3006865be 100644 --- a/source/common/safe_init/BUILD +++ b/source/common/init/BUILD @@ -13,7 +13,7 @@ envoy_cc_library( srcs = ["watcher_impl.cc"], hdrs = ["watcher_impl.h"], deps = [ - "//include/envoy/safe_init:watcher_interface", + "//include/envoy/init:watcher_interface", "//source/common/common:logger_lib", ], ) @@ -23,7 +23,7 @@ envoy_cc_library( srcs = ["target_impl.cc"], hdrs = ["target_impl.h"], deps = [ - "//include/envoy/safe_init:target_interface", + "//include/envoy/init:target_interface", "//source/common/common:logger_lib", ], ) @@ -34,7 +34,7 @@ envoy_cc_library( hdrs = ["manager_impl.h"], deps = [ ":watcher_lib", - "//include/envoy/safe_init:manager_interface", + "//include/envoy/init:manager_interface", "//source/common/common:logger_lib", ], ) diff --git a/source/common/safe_init/manager_impl.cc b/source/common/init/manager_impl.cc similarity index 97% rename from source/common/safe_init/manager_impl.cc rename to source/common/init/manager_impl.cc index a21827c67f2f0..f60ddc64a9e90 100644 --- a/source/common/safe_init/manager_impl.cc +++ b/source/common/init/manager_impl.cc @@ -1,9 +1,9 @@ -#include "common/safe_init/manager_impl.h" +#include "common/init/manager_impl.h" #include "common/common/assert.h" namespace Envoy { -namespace SafeInit { +namespace Init { ManagerImpl::ManagerImpl(absl::string_view name) : name_(fmt::format("init manager {}", name)), state_(State::Uninitialized), count_(0), @@ -75,5 +75,5 @@ void ManagerImpl::ready() { watcher_handle_->ready(); } -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/source/common/safe_init/manager_impl.h b/source/common/init/manager_impl.h similarity index 82% rename from source/common/safe_init/manager_impl.h rename to source/common/init/manager_impl.h index 7a88572422ad7..b92ac102fd729 100644 --- a/source/common/safe_init/manager_impl.h +++ b/source/common/init/manager_impl.h @@ -2,17 +2,17 @@ #include -#include "envoy/safe_init/manager.h" +#include "envoy/init/manager.h" #include "common/common/logger.h" -#include "common/safe_init/watcher_impl.h" +#include "common/init/watcher_impl.h" namespace Envoy { -namespace SafeInit { +namespace Init { /** - * SafeInit::ManagerImpl coordinates initialization of one or more "targets." See comments in - * include/envoy/safe_init/manager.h for an overview. + * Init::ManagerImpl coordinates initialization of one or more "targets." See comments in + * include/envoy/init/manager.h for an overview. * * When the logging level is set to "debug" or "trace," the log will contain entries for all * significant events in the initialization flow: @@ -30,7 +30,7 @@ class ManagerImpl : public Manager, Logger::Loggable { */ ManagerImpl(absl::string_view name); - // SafeInit::Manager + // Init::Manager State state() const override; void add(const Target& target) override; void initialize(const Watcher& watcher) override; @@ -58,5 +58,5 @@ class ManagerImpl : public Manager, Logger::Loggable { std::list target_handles_; }; -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/source/common/safe_init/target_impl.cc b/source/common/init/target_impl.cc similarity index 95% rename from source/common/safe_init/target_impl.cc rename to source/common/init/target_impl.cc index bdc839018e38e..5bf0288b82980 100644 --- a/source/common/safe_init/target_impl.cc +++ b/source/common/init/target_impl.cc @@ -1,7 +1,7 @@ -#include "common/safe_init/target_impl.h" +#include "common/init/target_impl.h" namespace Envoy { -namespace SafeInit { +namespace Init { TargetHandleImpl::TargetHandleImpl(absl::string_view handle_name, absl::string_view name, std::weak_ptr fn) @@ -50,5 +50,5 @@ bool TargetImpl::ready() { return false; } -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/source/common/safe_init/target_impl.h b/source/common/init/target_impl.h similarity index 95% rename from source/common/safe_init/target_impl.h rename to source/common/init/target_impl.h index 675cfceb91eec..ad2757433f0de 100644 --- a/source/common/safe_init/target_impl.h +++ b/source/common/init/target_impl.h @@ -2,12 +2,12 @@ #include -#include "envoy/safe_init/target.h" +#include "envoy/init/target.h" #include "common/common/logger.h" namespace Envoy { -namespace SafeInit { +namespace Init { /** * A target is just a glorified callback function, called by the manager it was registered with. @@ -33,7 +33,7 @@ class TargetHandleImpl : public TargetHandle, Logger::Loggable std::weak_ptr fn); public: - // SafeInit::TargetHandle + // Init::TargetHandle bool initialize(const Watcher& watcher) const override; private: @@ -62,7 +62,7 @@ class TargetImpl : public Target, Logger::Loggable { TargetImpl(absl::string_view name, InitializeFn fn); ~TargetImpl() override; - // SafeInit::Target + // Init::Target absl::string_view name() const override; TargetHandlePtr createHandle(absl::string_view handle_name) const override; @@ -85,5 +85,5 @@ class TargetImpl : public Target, Logger::Loggable { const std::shared_ptr fn_; }; -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/source/common/safe_init/watcher_impl.cc b/source/common/init/watcher_impl.cc similarity index 93% rename from source/common/safe_init/watcher_impl.cc rename to source/common/init/watcher_impl.cc index ee7899f55637f..b69fe3e7cf846 100644 --- a/source/common/safe_init/watcher_impl.cc +++ b/source/common/init/watcher_impl.cc @@ -1,7 +1,7 @@ -#include "common/safe_init/watcher_impl.h" +#include "common/init/watcher_impl.h" namespace Envoy { -namespace SafeInit { +namespace Init { WatcherHandleImpl::WatcherHandleImpl(absl::string_view handle_name, absl::string_view name, std::weak_ptr fn) @@ -34,5 +34,5 @@ WatcherHandlePtr WatcherImpl::createHandle(absl::string_view handle_name) const new WatcherHandleImpl(handle_name, name_, std::weak_ptr(fn_))); } -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/source/common/safe_init/watcher_impl.h b/source/common/init/watcher_impl.h similarity index 94% rename from source/common/safe_init/watcher_impl.h rename to source/common/init/watcher_impl.h index 582fd64910816..816a37c860eb2 100644 --- a/source/common/safe_init/watcher_impl.h +++ b/source/common/init/watcher_impl.h @@ -2,12 +2,12 @@ #include -#include "envoy/safe_init/watcher.h" +#include "envoy/init/watcher.h" #include "common/common/logger.h" namespace Envoy { -namespace SafeInit { +namespace Init { /** * A watcher is just a glorified callback function, called by a target or a manager when @@ -28,7 +28,7 @@ class WatcherHandleImpl : public WatcherHandle, Logger::Loggable fn); public: - // SafeInit::WatcherHandle + // Init::WatcherHandle bool ready() const override; private: @@ -57,7 +57,7 @@ class WatcherImpl : public Watcher, Logger::Loggable { WatcherImpl(absl::string_view name, ReadyFn fn); ~WatcherImpl() override; - // SafeInit::Watcher + // Init::Watcher absl::string_view name() const override; WatcherHandlePtr createHandle(absl::string_view handle_name) const override; @@ -69,5 +69,5 @@ class WatcherImpl : public Watcher, Logger::Loggable { const std::shared_ptr fn_; }; -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/source/common/router/BUILD b/source/common/router/BUILD index b861e6eeb3037..498babb17239e 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -74,7 +74,6 @@ envoy_cc_library( ":config_lib", "//include/envoy/config:subscription_interface", "//include/envoy/http:codes_interface", - "//include/envoy/init:init_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/router:rds_interface", "//include/envoy/router:route_config_provider_manager_interface", @@ -86,6 +85,7 @@ envoy_cc_library( "//source/common/config:rds_json_lib", "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", + "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", "@envoy_api//envoy/admin/v2alpha:config_dump_cc", "@envoy_api//envoy/api/v2:rds_cc", diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 3908fa799cd96..9393b7190f77e 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -60,6 +60,8 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( const std::string& stat_prefix, Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager) : route_config_name_(rds.route_config_name()), + init_target_(fmt::format("RdsRouteConfigSubscription {}", route_config_name_), + [this]() { subscription_->start({route_config_name_}, *this); }), scope_(factory_context.scope().createScope(stat_prefix + "rds." + route_config_name_ + ".")), stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}), route_config_provider_manager_(route_config_provider_manager), @@ -77,7 +79,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( RdsRouteConfigSubscription::~RdsRouteConfigSubscription() { // If we get destroyed during initialization, make sure we signal that we "initialized". - runInitializeCallbackIfAny(); + init_target_.ready(); // The ownership of RdsRouteConfigProviderImpl is shared among all HttpConnectionManagers that // hold a shared_ptr to it. The RouteConfigProviderManager holds weak_ptrs to the @@ -93,7 +95,7 @@ void RdsRouteConfigSubscription::onConfigUpdate(const ResourceVector& resources, if (resources.empty()) { ENVOY_LOG(debug, "Missing RouteConfiguration for {} in onConfigUpdate()", route_config_name_); stats_.update_empty_.inc(); - runInitializeCallbackIfAny(); + init_target_.ready(); return; } if (resources.size() != 1) { @@ -119,25 +121,13 @@ void RdsRouteConfigSubscription::onConfigUpdate(const ResourceVector& resources, } } - runInitializeCallbackIfAny(); + init_target_.ready(); } void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { // We need to allow server startup to continue, even if we have a bad // config. - runInitializeCallbackIfAny(); -} - -void RdsRouteConfigSubscription::registerInitTarget(Init::Manager& init_manager) { - init_manager.registerTarget(*this, - fmt::format("RdsRouteConfigSubscription {}", route_config_name_)); -} - -void RdsRouteConfigSubscription::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } + init_target_.ready(); } RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( @@ -207,7 +197,7 @@ Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteCon subscription.reset(new RdsRouteConfigSubscription(rds, manager_identifier, factory_context, stat_prefix, *this)); - subscription->registerInitTarget(factory_context.initManager()); + factory_context.initManager().add(subscription->init_target_); route_config_subscriptions_.insert({manager_identifier, subscription}); } else { diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 67e086258e21b..d665a3aed8303 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -12,7 +12,6 @@ #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" #include "envoy/config/subscription.h" #include "envoy/http/codes.h" -#include "envoy/init/init.h" #include "envoy/local_info/local_info.h" #include "envoy/router/rds.h" #include "envoy/router/route_config_provider_manager.h" @@ -23,6 +22,7 @@ #include "envoy/thread_local/thread_local.h" #include "common/common/logger.h" +#include "common/init/target_impl.h" #include "common/protobuf/utility.h" namespace Envoy { @@ -94,18 +94,11 @@ class RdsRouteConfigProviderImpl; * RDS config providers. */ class RdsRouteConfigSubscription - : public Init::Target, - Envoy::Config::SubscriptionCallbacks, + : Envoy::Config::SubscriptionCallbacks, Logger::Loggable { public: ~RdsRouteConfigSubscription(); - // Init::Target - void initialize(std::function callback) override { - initialize_callback_ = callback; - subscription_->start({route_config_name_}, *this); - } - // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; @@ -130,12 +123,9 @@ class RdsRouteConfigSubscription const std::string& stat_prefix, RouteConfigProviderManagerImpl& route_config_provider_manager); - void registerInitTarget(Init::Manager& init_manager); - void runInitializeCallbackIfAny(); - - std::unique_ptr> subscription_; - std::function initialize_callback_; const std::string route_config_name_; + Init::TargetImpl init_target_; + std::unique_ptr> subscription_; Stats::ScopePtr scope_; RdsStats stats_; RouteConfigProviderManagerImpl& route_config_provider_manager_; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 5a9f0f94ec7ce..3248ffb331ab8 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -42,7 +42,7 @@ envoy_cc_library( deps = [ "//include/envoy/config:subscription_interface", "//include/envoy/event:dispatcher_interface", - "//include/envoy/init:init_interface", + "//include/envoy/init:manager_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/secret:secret_provider_interface", @@ -52,6 +52,7 @@ envoy_cc_library( "//source/common/common:cleanup_lib", "//source/common/config:resources_lib", "//source/common/config:subscription_factory_lib", + "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", "//source/common/ssl:tls_certificate_config_impl_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 2471ad98657f6..10f04d5ef6281 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -17,7 +17,8 @@ SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispat const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, std::function destructor_cb, Api::Api& api) - : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), + : init_target_(fmt::format("SdsApi {}", sds_config_name), [this] { initialize(); }), + local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), secret_hash_(0), clean_up_(destructor_cb), api_(api) { Config::Utility::checkLocalInfo("sds", local_info_); @@ -25,19 +26,7 @@ SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispat // can be chained together to behave as one init_manager. In that way, we let // two listeners which share same SdsApi to register at separate init managers, and // each init manager has a chance to initialize its targets. - init_manager.registerTarget(*this, fmt::format("SdsApi {}", sds_config_name)); -} - -void SdsApi::initialize(std::function callback) { - initialize_callback_ = callback; - - subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< - envoy::api::v2::auth::Secret>( - sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, - "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", - "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets", api_); - - subscription_->start({sds_config_name_}, *this); + init_manager.add(init_target_); } void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) { @@ -66,19 +55,21 @@ void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) update_callback_manager_.runCallbacks(); } - runInitializeCallbackIfAny(); + init_target_.ready(); } void SdsApi::onConfigUpdateFailed(const EnvoyException*) { // We need to allow server startup to continue, even if we have a bad config. - runInitializeCallbackIfAny(); + init_target_.ready(); } -void SdsApi::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } +void SdsApi::initialize() { + subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< + envoy::api::v2::auth::Secret>( + sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, + "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", + "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets", api_); + subscription_->start({sds_config_name_}, *this); } } // namespace Secret diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 6123159b372fe..bb6132febd181 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -7,7 +7,7 @@ #include "envoy/api/v2/core/config_source.pb.h" #include "envoy/config/subscription.h" #include "envoy/event/dispatcher.h" -#include "envoy/init/init.h" +#include "envoy/init/manager.h" #include "envoy/local_info/local_info.h" #include "envoy/runtime/runtime.h" #include "envoy/secret/secret_callbacks.h" @@ -18,6 +18,7 @@ #include "common/common/callback_impl.h" #include "common/common/cleanup.h" +#include "common/init/target_impl.h" #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" @@ -27,8 +28,7 @@ namespace Secret { /** * SDS API implementation that fetches secrets from SDS server via Subscription. */ -class SdsApi : public Init::Target, - public Config::SubscriptionCallbacks { +class SdsApi : public Config::SubscriptionCallbacks { public: SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, Stats::Store& stats, @@ -36,9 +36,6 @@ class SdsApi : public Init::Target, const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, std::function destructor_cb, Api::Api& api); - // Init::Target - void initialize(std::function callback) override; - // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; @@ -58,8 +55,8 @@ class SdsApi : public Init::Target, Common::CallbackManager<> update_callback_manager_; private: - void runInitializeCallbackIfAny(); - + void initialize(); + Init::TargetImpl init_target_; const LocalInfo::LocalInfo& local_info_; Event::Dispatcher& dispatcher_; Runtime::RandomGenerator& random_; @@ -68,7 +65,6 @@ class SdsApi : public Init::Target, const envoy::api::v2::core::ConfigSource sds_config_; std::unique_ptr> subscription_; - std::function initialize_callback_; const std::string sds_config_name_; uint64_t secret_hash_; diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index bd09d3f9329b1..77bab1075574f 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -384,9 +384,9 @@ envoy_cc_library( "//source/common/common:minimal_logger_lib", "//source/common/config:metadata_lib", "//source/common/config:well_known_names", + "//source/common/init:manager_lib", "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", - "//source/server:init_manager_lib", "//source/server:transport_socket_config_lib", "@envoy_api//envoy/api/v2/core:base_cc", "@envoy_api//envoy/api/v2/endpoint:endpoint_cc", @@ -454,7 +454,6 @@ envoy_cc_library( "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", "//source/extensions/clusters:well_known_names", - "//source/server:init_manager_lib", "//source/server:transport_socket_config_lib", "@envoy_api//envoy/api/v2/core:base_cc", "@envoy_api//envoy/api/v2/endpoint:endpoint_cc", diff --git a/source/common/upstream/cluster_factory_impl.h b/source/common/upstream/cluster_factory_impl.h index 6d4c8b95c62e5..ea8b1cdf7ac23 100644 --- a/source/common/upstream/cluster_factory_impl.h +++ b/source/common/upstream/cluster_factory_impl.h @@ -44,8 +44,6 @@ #include "common/upstream/resource_manager_impl.h" #include "common/upstream/upstream_impl.h" -#include "server/init_manager_impl.h" - #include "extensions/clusters/well_known_names.h" namespace Envoy { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index ef436d8a084c7..0b05f65b74791 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -644,7 +644,8 @@ ClusterImplBase::ClusterImplBase( const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Server::Configuration::TransportSocketFactoryContext& factory_context, Stats::ScopePtr&& stats_scope, bool added_via_api) - : runtime_(runtime), init_manager_(fmt::format("Cluster {}", cluster.name())) { + : init_manager_(fmt::format("Cluster {}", cluster.name())), + init_watcher_("ClusterImplBase", [this]() { onInitDone(); }), runtime_(runtime) { factory_context.setInitManager(init_manager_); auto socket_factory = createTransportSocketFactory(cluster, factory_context); info_ = std::make_unique(cluster, factory_context.clusterManager().bindConfig(), @@ -714,7 +715,7 @@ void ClusterImplBase::onPreInitComplete() { initialization_started_ = true; ENVOY_LOG(debug, "initializing secondary cluster {} completed", info()->name()); - init_manager_.initialize([this]() { onInitDone(); }); + init_manager_.initialize(init_watcher_); } void ClusterImplBase::onInitDone() { diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 8a9096a2e89d6..f2ce1a7e4c3f9 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -34,14 +34,13 @@ #include "common/common/logger.h" #include "common/config/metadata.h" #include "common/config/well_known_names.h" +#include "common/init/manager_impl.h" #include "common/network/utility.h" #include "common/stats/isolated_store_impl.h" #include "common/upstream/load_balancer_impl.h" #include "common/upstream/outlier_detection_impl.h" #include "common/upstream/resource_manager_impl.h" -#include "server/init_manager_impl.h" - #include "absl/synchronization/mutex.h" namespace Envoy { @@ -673,8 +672,16 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable void { init_manager_.initialize([]() -> void {}); }); + clusterManager().setInitializedCb([this]() -> void { init_manager_.initialize(init_watcher_); }); } void ValidationInstance::shutdown() { diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 3e5d039d9b930..3d6493302faf4 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -145,7 +145,8 @@ class ValidationInstance : Logger::Loggable, // init_manager_ must come before any member that participates in initialization, and destructed // only after referencing members are gone, since initialization continuation can potentially // occur at any point during member lifetime. - InitManagerImpl init_manager_{"Validation server"}; + Init::ManagerImpl init_manager_{"Validation server"}; + Init::WatcherImpl init_watcher_{"(no-op)", []() {}}; // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed // only after these members can no longer reference it, since: // - There may be active filter chains referencing it in listener_manager_. diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 9c7da83e0e358..09b2b646fbed1 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -639,7 +639,7 @@ Http::Code AdminImpl::handlerServerInfo(absl::string_view, Http::HeaderMap& head server_info.set_version(VersionInfo::version()); switch (server_.initManager().state()) { - case Init::Manager::State::NotInitialized: + case Init::Manager::State::Uninitialized: server_info.set_state(envoy::admin::v2alpha::ServerInfo::PRE_INITIALIZING); break; case Init::Manager::State::Initializing: diff --git a/source/server/init_manager_impl.cc b/source/server/init_manager_impl.cc deleted file mode 100644 index 650d284217c1c..0000000000000 --- a/source/server/init_manager_impl.cc +++ /dev/null @@ -1,66 +0,0 @@ -#include "server/init_manager_impl.h" - -#include - -#include "common/common/assert.h" - -#define TRACE_INIT_MANAGER(fmt, ...) \ - ENVOY_LOG(debug, "InitManagerImpl({}): " fmt, description_, ##__VA_ARGS__) - -namespace Envoy { -namespace Server { - -InitManagerImpl::InitManagerImpl(absl::string_view description) : description_(description) { - TRACE_INIT_MANAGER("constructor"); -} - -InitManagerImpl::~InitManagerImpl() { TRACE_INIT_MANAGER("destructor"); } - -void InitManagerImpl::initialize(std::function callback) { - ASSERT(state_ == State::NotInitialized); - if (targets_.empty()) { - TRACE_INIT_MANAGER("empty targets, initialized"); - callback(); - state_ = State::Initialized; - } else { - TRACE_INIT_MANAGER("initializing"); - callback_ = callback; - state_ = State::Initializing; - // Target::initialize(...) method can modify the list to remove the item currently - // being initialized, so we increment the iterator before calling initialize. - for (auto iter = targets_.begin(); iter != targets_.end();) { - TargetWithDescription& target = *iter; - ++iter; - initializeTarget(target); - } - } -} - -void InitManagerImpl::initializeTarget(TargetWithDescription& target) { - TRACE_INIT_MANAGER("invoking initializeTarget {}", target.second); - target.first->initialize([this, &target]() -> void { - TRACE_INIT_MANAGER("completed initializeTarget {}", target.second); - ASSERT(std::find(targets_.begin(), targets_.end(), target) != targets_.end()); - targets_.remove(target); - if (targets_.empty()) { - TRACE_INIT_MANAGER("initialized"); - state_ = State::Initialized; - callback_(); - } - }); -} - -void InitManagerImpl::registerTarget(Init::Target& target, absl::string_view description) { - TRACE_INIT_MANAGER("registerTarget {}", description); - ASSERT(state_ != State::Initialized); - ASSERT(std::find(targets_.begin(), targets_.end(), - TargetWithDescription{&target, std::string(description)}) == targets_.end(), - "Registered duplicate Init::Target"); - targets_.emplace_back(&target, std::string(description)); - if (state_ == State::Initializing) { - initializeTarget(targets_.back()); - } -} - -} // namespace Server -} // namespace Envoy diff --git a/source/server/init_manager_impl.h b/source/server/init_manager_impl.h deleted file mode 100644 index e84ec4fbd32d7..0000000000000 --- a/source/server/init_manager_impl.h +++ /dev/null @@ -1,40 +0,0 @@ -#pragma once - -#include - -#include "envoy/init/init.h" - -#include "common/common/logger.h" - -namespace Envoy { -namespace Server { - -/** - * Implementation of Init::Manager for use during post cluster manager init / pre listening. - * Deprecated, use SafeInit::ManagerImpl instead. - * TODO(mergeconflict): convert all Init::ManagerImpl uses to SafeInit::ManagerImpl. - */ -class InitManagerImpl : public Init::Manager, Logger::Loggable { -public: - InitManagerImpl(absl::string_view description); - ~InitManagerImpl() override; - - void initialize(std::function callback); - - // Init::Manager - void registerTarget(Init::Target& target, absl::string_view description) override; - State state() const override { return state_; } - -private: - using TargetWithDescription = std::pair; - - void initializeTarget(TargetWithDescription& target); - - std::list targets_; - State state_{State::NotInitialized}; - std::function callback_; - std::string description_; // For debug tracing. -}; - -} // namespace Server -} // namespace Envoy diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 2b5c18629b92d..10aa4b611d121 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -20,19 +20,15 @@ LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, Runtime::RandomGenerator& random, Init::Manager& init_manager, const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, ListenerManager& lm, Api::Api& api) - : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm) { + : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm), + init_target_("LDS", [this]() { subscription_->start({}, *this); }) { subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( lds_config, local_info, dispatcher, cm, random, *scope_, "envoy.api.v2.ListenerDiscoveryService.FetchListeners", "envoy.api.v2.ListenerDiscoveryService.StreamListeners", api); Config::Utility::checkLocalInfo("lds", local_info); - init_manager.registerTarget(*this, "LDS"); -} - -void LdsApiImpl::initialize(std::function callback) { - initialize_callback_ = callback; - subscription_->start({}, *this); + init_manager.add(init_target_); } void LdsApiImpl::onConfigUpdate(const ResourceVector& resources, const std::string& version_info) { @@ -81,7 +77,7 @@ void LdsApiImpl::onConfigUpdate(const ResourceVector& resources, const std::stri } version_info_ = version_info; - runInitializeCallbackIfAny(); + init_target_.ready(); if (!exception_msgs.empty()) { throw EnvoyException(fmt::format("Error adding/updating listener(s) {}", StringUtil::join(exception_msgs, ", "))); @@ -91,14 +87,7 @@ void LdsApiImpl::onConfigUpdate(const ResourceVector& resources, const std::stri void LdsApiImpl::onConfigUpdateFailed(const EnvoyException*) { // We need to allow server startup to continue, even if we have a bad // config. - runInitializeCallbackIfAny(); -} - -void LdsApiImpl::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } + init_target_.ready(); } } // namespace Server diff --git a/source/server/lds_api.h b/source/server/lds_api.h index 713ead3f118a6..647e5664ea240 100644 --- a/source/server/lds_api.h +++ b/source/server/lds_api.h @@ -5,11 +5,12 @@ #include "envoy/api/api.h" #include "envoy/api/v2/lds.pb.h" #include "envoy/config/subscription.h" -#include "envoy/init/init.h" +#include "envoy/init/manager.h" #include "envoy/server/listener_manager.h" #include "envoy/stats/scope.h" #include "common/common/logger.h" +#include "common/init/target_impl.h" namespace Envoy { namespace Server { @@ -18,7 +19,6 @@ namespace Server { * LDS API implementation that fetches via Subscription. */ class LdsApiImpl : public LdsApi, - public Init::Target, Config::SubscriptionCallbacks, Logger::Loggable { public: @@ -30,9 +30,6 @@ class LdsApiImpl : public LdsApi, // Server::LdsApi std::string versionInfo() const override { return version_info_; } - // Init::Target - void initialize(std::function callback) override; - // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; @@ -46,14 +43,12 @@ class LdsApiImpl : public LdsApi, } private: - void runInitializeCallbackIfAny(); - std::unique_ptr> subscription_; std::string version_info_; ListenerManager& listener_manager_; Stats::ScopePtr scope_; Upstream::ClusterManager& cm_; - std::function initialize_callback_; + Init::TargetImpl init_target_; }; } // namespace Server diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index ac8bd1a6c2f21..8598f2779d3dd 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -168,6 +168,8 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st listener_tag_(parent_.factory_.nextListenerTag()), name_(name), modifiable_(modifiable), workers_started_(workers_started), hash_(hash), dynamic_init_manager_(fmt::format("Listener {}", name)), + init_watcher_(std::make_unique( + "ListenerImpl", [this] { parent_.onListenerWarmed(*this); })), local_drain_manager_(parent.factory_.createDrainManager(config.drain_type())), config_(config), version_info_(version_info), listener_filters_timeout_( @@ -317,9 +319,9 @@ ListenerImpl::~ListenerImpl() { // The filter factories may have pending initialize actions (like in the case of RDS). Those // actions will fire in the destructor to avoid blocking initial server startup. If we are using // a local init manager we should block the notification from trying to move us from warming to - // active. This is done here explicitly by setting a boolean and then clearing the factory + // active. This is done here explicitly by resetting the watcher and then clearing the factory // vector for clarity. - initialize_canceled_ = true; + init_watcher_.reset(); destination_ports_map_.clear(); } @@ -629,13 +631,9 @@ void ListenerImpl::initialize() { last_updated_ = timeSource().systemTime(); // If workers have already started, we shift from using the global init manager to using a local // per listener init manager. See ~ListenerImpl() for why we gate the onListenerWarmed() call - // with initialize_canceled_. + // by resetting the watcher. if (workers_started_) { - dynamic_init_manager_.initialize([this]() -> void { - if (!initialize_canceled_) { - parent_.onListenerWarmed(*this); - } - }); + dynamic_init_manager_.initialize(*init_watcher_); } } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 647edfa5a89c9..41da1135d4443 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -12,10 +12,10 @@ #include "envoy/stats/scope.h" #include "common/common/logger.h" +#include "common/init/manager_impl.h" #include "common/network/cidr_range.h" #include "common/network/lc_trie.h" -#include "server/init_manager_impl.h" #include "server/lds_api.h" namespace Envoy { @@ -403,8 +403,14 @@ class ListenerImpl : public Network::ListenerConfig, const bool modifiable_; const bool workers_started_; const uint64_t hash_; - InitManagerImpl dynamic_init_manager_; - bool initialize_canceled_{}; + + // This init manager is populated with targets from the filter chain factories, namely + // RdsRouteConfigSubscription::init_target_, so the listener can wait for route configs. + Init::ManagerImpl dynamic_init_manager_; + + // This init watcher, if available, notifies the "parent" listener manager when listener + // initialization is complete. It may be reset to cancel interest. + std::unique_ptr init_watcher_; std::vector listener_filter_factories_; DrainManagerPtr local_drain_manager_; bool saw_listener_create_failure_{}; diff --git a/source/server/server.cc b/source/server/server.cc index c15c4c29ccd57..a75d0d291ba47 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -412,9 +412,13 @@ uint64_t InstanceImpl::numConnections() { return listener_manager_->numConnectio RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, - InitManagerImpl& init_manager, OverloadManager& overload_manager, - std::function workers_start_cb) { - + Init::Manager& init_manager, OverloadManager& overload_manager, + std::function workers_start_cb) + : init_watcher_("RunHelper", [&instance, workers_start_cb]() { + if (!instance.isShutdown()) { + workers_start_cb(); + } + }) { // Setup signals. if (options.signalHandlingEnabled()) { sigterm_ = dispatcher.listenForSignal(SIGTERM, [&instance]() { @@ -445,7 +449,7 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch // this can fire immediately if all clusters have already initialized. Also note that we need // to guard against shutdown at two different levels since SIGTERM can come in once the run loop // starts. - cm.setInitializedCb([&instance, &init_manager, &cm, workers_start_cb]() { + cm.setInitializedCb([&instance, &init_manager, &cm, this]() { if (instance.isShutdown()) { return; } @@ -456,16 +460,7 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch cm.adsMux().pause(Config::TypeUrl::get().RouteConfiguration); ENVOY_LOG(info, "all clusters initialized. initializing init manager"); - - // Note: the lambda below should not capture "this" since the RunHelper object may - // have been destructed by the time it gets executed. - init_manager.initialize([&instance, workers_start_cb]() { - if (instance.isShutdown()) { - return; - } - - workers_start_cb(); - }); + init_manager.initialize(init_watcher_); // Now that we're execute all the init callbacks we can resume RDS // as we've subscribed to all the statically defined RDS resources. @@ -474,11 +469,10 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch } void InstanceImpl::run() { - // We need the RunHelper to be available to call from InstanceImpl::shutdown() below, so - // we save it as a member variable. - run_helper_ = std::make_unique(*this, options_, *dispatcher_, clusterManager(), - access_log_manager_, init_manager_, overloadManager(), - [this]() -> void { startWorkers(); }); + // RunHelper exists primarily to facilitate testing of how we respond to early shutdown during + // startup (see RunHelperTest in server_test.cc). + auto run_helper = RunHelper(*this, options_, *dispatcher_, clusterManager(), access_log_manager_, + init_manager_, overloadManager(), [this] { startWorkers(); }); // Run the main dispatch loop waiting to exit. ENVOY_LOG(info, "starting main dispatch loop"); @@ -491,7 +485,6 @@ void InstanceImpl::run() { watchdog.reset(); terminate(); - run_helper_.reset(); } void InstanceImpl::terminate() { diff --git a/source/server/server.h b/source/server/server.h index b897918cf8d7f..c09479dfb610d 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -21,6 +21,7 @@ #include "common/common/logger_delegates.h" #include "common/grpc/async_client_manager_impl.h" #include "common/http/context_impl.h" +#include "common/init/manager_impl.h" #include "common/memory/heap_shrinker.h" #include "common/runtime/runtime_impl.h" #include "common/secret/secret_manager_impl.h" @@ -28,7 +29,6 @@ #include "server/configuration_impl.h" #include "server/http/admin.h" -#include "server/init_manager_impl.h" #include "server/listener_manager_impl.h" #include "server/overload_manager_impl.h" #include "server/test_hooks.h" @@ -122,10 +122,11 @@ class RunHelper : Logger::Loggable { public: RunHelper(Instance& instance, const Options& options, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, - InitManagerImpl& init_manager, OverloadManager& overload_manager, + Init::Manager& init_manager, OverloadManager& overload_manager, std::function workers_start_cb); private: + Init::WatcherImpl init_watcher_; Event::SignalEventPtr sigterm_; Event::SignalEventPtr sigint_; Event::SignalEventPtr sig_usr_1_; @@ -209,8 +210,8 @@ class InstanceImpl : Logger::Loggable, // init_manager_ must come before any member that participates in initialization, and destructed // only after referencing members are gone, since initialization continuation can potentially - // occur at any point during member lifetime. - InitManagerImpl init_manager_{"Server"}; + // occur at any point during member lifetime. This init manager is populated with LdsApi targets. + Init::ManagerImpl init_manager_{"Server"}; // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed // only after these members can no longer reference it, since: // - There may be active filter chains referencing it in listener_manager_. @@ -255,7 +256,6 @@ class InstanceImpl : Logger::Loggable, Upstream::ProdClusterInfoFactory info_factory_; Upstream::HdsDelegatePtr hds_delegate_; std::unique_ptr overload_manager_; - std::unique_ptr run_helper_; Envoy::MutexTracer* mutex_tracer_; Http::ContextImpl http_context_; std::unique_ptr heap_shrinker_; diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index f25f77a0b1dd8..56d6ab008304f 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -235,7 +235,8 @@ test::common::config::DummyConfig parseDummyConfigFromYaml(const std::string& ya // 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(); + Init::ExpectableWatcherImpl watcher; + factory_context_.init_manager_.initialize(watcher); envoy::api::v2::core::ApiConfigSource config_source_proto; config_source_proto.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); diff --git a/test/common/safe_init/BUILD b/test/common/init/BUILD similarity index 69% rename from test/common/safe_init/BUILD rename to test/common/init/BUILD index 35dd33cd09b7c..894e7493aa722 100644 --- a/test/common/safe_init/BUILD +++ b/test/common/init/BUILD @@ -12,7 +12,7 @@ envoy_cc_test( name = "watcher_impl_test", srcs = ["watcher_impl_test.cc"], deps = [ - "//test/mocks/safe_init:safe_init_mocks", + "//test/mocks/init:init_mocks", ], ) @@ -20,7 +20,7 @@ envoy_cc_test( name = "target_impl_test", srcs = ["target_impl_test.cc"], deps = [ - "//test/mocks/safe_init:safe_init_mocks", + "//test/mocks/init:init_mocks", ], ) @@ -28,7 +28,7 @@ envoy_cc_test( name = "manager_impl_test", srcs = ["manager_impl_test.cc"], deps = [ - "//source/common/safe_init:manager_lib", - "//test/mocks/safe_init:safe_init_mocks", + "//source/common/init:manager_lib", + "//test/mocks/init:init_mocks", ], ) diff --git a/test/common/safe_init/manager_impl_test.cc b/test/common/init/manager_impl_test.cc similarity index 86% rename from test/common/safe_init/manager_impl_test.cc rename to test/common/init/manager_impl_test.cc index 88e6cc97c7d34..8a479b0c1977a 100644 --- a/test/common/safe_init/manager_impl_test.cc +++ b/test/common/init/manager_impl_test.cc @@ -1,6 +1,6 @@ -#include "common/safe_init/manager_impl.h" +#include "common/init/manager_impl.h" -#include "test/mocks/safe_init/mocks.h" +#include "test/mocks/init/mocks.h" #include "gtest/gtest.h" @@ -8,14 +8,14 @@ using ::testing::InSequence; using ::testing::InvokeWithoutArgs; namespace Envoy { -namespace SafeInit { +namespace Init { namespace { void expectUninitialized(const Manager& m) { EXPECT_EQ(Manager::State::Uninitialized, m.state()); } void expectInitializing(const Manager& m) { EXPECT_EQ(Manager::State::Initializing, m.state()); } void expectInitialized(const Manager& m) { EXPECT_EQ(Manager::State::Initialized, m.state()); } -TEST(SafeInitManagerImplTest, AddImmediateTargetsWhenUninitialized) { +TEST(InitManagerImplTest, AddImmediateTargetsWhenUninitialized) { InSequence s; ManagerImpl m("test"); @@ -37,7 +37,7 @@ TEST(SafeInitManagerImplTest, AddImmediateTargetsWhenUninitialized) { expectInitialized(m); } -TEST(SafeInitManagerImplTest, AddAsyncTargetsWhenUninitialized) { +TEST(InitManagerImplTest, AddAsyncTargetsWhenUninitialized) { InSequence s; ManagerImpl m("test"); @@ -67,7 +67,7 @@ TEST(SafeInitManagerImplTest, AddAsyncTargetsWhenUninitialized) { expectInitialized(m); } -TEST(SafeInitManagerImplTest, AddMixedTargetsWhenUninitialized) { +TEST(InitManagerImplTest, AddMixedTargetsWhenUninitialized) { InSequence s; ManagerImpl m("test"); @@ -93,7 +93,7 @@ TEST(SafeInitManagerImplTest, AddMixedTargetsWhenUninitialized) { expectInitialized(m); } -TEST(SafeInitManagerImplTest, AddImmediateTargetWhenInitializing) { +TEST(InitManagerImplTest, AddImmediateTargetWhenInitializing) { InSequence s; ManagerImpl m("test"); @@ -121,7 +121,7 @@ TEST(SafeInitManagerImplTest, AddImmediateTargetWhenInitializing) { expectInitialized(m); } -TEST(SafeInitManagerImplTest, UnavailableTarget) { +TEST(InitManagerImplTest, UnavailableTarget) { InSequence s; ManagerImpl m("test"); @@ -142,7 +142,7 @@ TEST(SafeInitManagerImplTest, UnavailableTarget) { expectInitialized(m); } -TEST(SafeInitManagerImplTest, UnavailableManager) { +TEST(InitManagerImplTest, UnavailableManager) { InSequence s; ExpectableTargetImpl t("t"); @@ -165,7 +165,7 @@ TEST(SafeInitManagerImplTest, UnavailableManager) { t.ready(); } -TEST(SafeInitManagerImplTest, UnavailableWatcher) { +TEST(InitManagerImplTest, UnavailableWatcher) { InSequence s; ManagerImpl m("test"); @@ -190,5 +190,5 @@ TEST(SafeInitManagerImplTest, UnavailableWatcher) { } } // namespace -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/test/common/safe_init/target_impl_test.cc b/test/common/init/target_impl_test.cc similarity index 82% rename from test/common/safe_init/target_impl_test.cc rename to test/common/init/target_impl_test.cc index df0c41fad2f1e..7cebbb371d9e9 100644 --- a/test/common/safe_init/target_impl_test.cc +++ b/test/common/init/target_impl_test.cc @@ -1,19 +1,19 @@ -#include "test/mocks/safe_init/mocks.h" +#include "test/mocks/init/mocks.h" #include "gtest/gtest.h" using ::testing::InSequence; namespace Envoy { -namespace SafeInit { +namespace Init { namespace { -TEST(SafeInitTargetImplTest, Name) { +TEST(InitTargetImplTest, Name) { ExpectableTargetImpl target; EXPECT_EQ("target test", target.name()); } -TEST(SafeInitTargetImplTest, InitializeWhenAvailable) { +TEST(InitTargetImplTest, InitializeWhenAvailable) { InSequence s; ExpectableTargetImpl target; @@ -32,7 +32,7 @@ TEST(SafeInitTargetImplTest, InitializeWhenAvailable) { EXPECT_FALSE(target.ready()); } -TEST(SafeInitTargetImplTest, InitializeWhenUnavailable) { +TEST(InitTargetImplTest, InitializeWhenUnavailable) { ExpectableWatcherImpl watcher; TargetHandlePtr handle; { @@ -45,7 +45,7 @@ TEST(SafeInitTargetImplTest, InitializeWhenUnavailable) { EXPECT_FALSE(handle->initialize(watcher)); } -TEST(SafeInitTargetImplTest, ReadyWhenWatcherUnavailable) { +TEST(InitTargetImplTest, ReadyWhenWatcherUnavailable) { ExpectableTargetImpl target; { ExpectableWatcherImpl watcher; @@ -61,5 +61,5 @@ TEST(SafeInitTargetImplTest, ReadyWhenWatcherUnavailable) { } } // namespace -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/test/common/safe_init/watcher_impl_test.cc b/test/common/init/watcher_impl_test.cc similarity index 72% rename from test/common/safe_init/watcher_impl_test.cc rename to test/common/init/watcher_impl_test.cc index 39abccbf40f93..020c5467bbc74 100644 --- a/test/common/safe_init/watcher_impl_test.cc +++ b/test/common/init/watcher_impl_test.cc @@ -1,17 +1,17 @@ -#include "test/mocks/safe_init/mocks.h" +#include "test/mocks/init/mocks.h" #include "gtest/gtest.h" namespace Envoy { -namespace SafeInit { +namespace Init { namespace { -TEST(SafeInitWatcherImplTest, Name) { +TEST(InitWatcherImplTest, Name) { ExpectableWatcherImpl watcher; EXPECT_EQ("test", watcher.name()); } -TEST(SafeInitWatcherImplTest, ReadyWhenAvailable) { +TEST(InitWatcherImplTest, ReadyWhenAvailable) { ExpectableWatcherImpl watcher; // notifying the watcher through its handle should invoke ready(). @@ -19,7 +19,7 @@ TEST(SafeInitWatcherImplTest, ReadyWhenAvailable) { EXPECT_TRUE(watcher.createHandle("test")->ready()); } -TEST(SafeInitWatcherImplTest, ReadyWhenUnavailable) { +TEST(InitWatcherImplTest, ReadyWhenUnavailable) { WatcherHandlePtr handle; { ExpectableWatcherImpl watcher; @@ -32,5 +32,5 @@ TEST(SafeInitWatcherImplTest, ReadyWhenUnavailable) { } } // namespace -} // namespace SafeInit +} // namespace Init } // namespace Envoy diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 89e87cda4ea0e..797f6c1dc0479 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -62,7 +62,6 @@ envoy_cc_test( "//source/common/json:json_loader_lib", "//source/common/router:rds_lib", "//source/server/http:admin_lib", - "//test/mocks/init:init_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/server:server_mocks", "//test/mocks/thread_local:thread_local_mocks", diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 266793ce209ad..5aa1ac1c96e69 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -49,7 +49,15 @@ parseHttpConnectionManagerFromJson(const std::string& json_string, const Stats:: class RdsTestBase : public testing::Test { public: - RdsTestBase() : request_(&factory_context_.cluster_manager_.async_client_) {} + RdsTestBase() : request_(&factory_context_.cluster_manager_.async_client_) { + ON_CALL(factory_context_.init_manager_, add(_)) + .WillByDefault(Invoke([this](const Init::Target& target) { + init_target_handle_ = target.createHandle("test"); + })); + ON_CALL(factory_context_.init_manager_, initialize(_)) + .WillByDefault(Invoke( + [this](const Init::Watcher& watcher) { init_target_handle_->initialize(watcher); })); + } void expectRequest() { EXPECT_CALL(factory_context_.cluster_manager_, httpAsyncClientForCluster("foo_cluster")); @@ -72,6 +80,8 @@ class RdsTestBase : public testing::Test { Event::SimulatedTimeSystem time_system_; NiceMock factory_context_; + Init::ExpectableWatcherImpl init_watcher_; + Init::TargetHandlePtr init_target_handle_; Http::MockAsyncClientRequest request_; Http::AsyncClient::Callbacks* callbacks_{}; Event::MockTimer* interval_timer_{}; @@ -112,12 +122,12 @@ class RdsImplTest : public RdsTestBase { EXPECT_CALL(cluster, info()); EXPECT_CALL(*cluster.info_, type()); interval_timer_ = new Event::MockTimer(&factory_context_.dispatcher_); - EXPECT_CALL(factory_context_.init_manager_, registerTarget(_, _)); + EXPECT_CALL(factory_context_.init_manager_, add(_)); rds_ = RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json, scope_), factory_context_, "foo.", *route_config_provider_manager_); expectRequest(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(init_watcher_); } NiceMock scope_; @@ -198,7 +208,7 @@ TEST_F(RdsImplTest, DestroyDuringInitialize) { InSequence s; setup(); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(request_, cancel()); rds_.reset(); } @@ -232,7 +242,7 @@ TEST_F(RdsImplTest, Basic) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response1_json); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); EXPECT_EQ(nullptr, rds_->config()->route(Http::TestHeaderMapImpl{{":authority", "foo"}}, 0)); @@ -342,7 +352,7 @@ TEST_F(RdsImplTest, Failure) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response_json); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -465,7 +475,7 @@ name: foo // Static + dynamic. setup(); expectRequest(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(init_watcher_); const std::string response1_json = R"EOF( { @@ -483,7 +493,7 @@ name: foo Http::MessagePtr message(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response1_json); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); @@ -519,8 +529,6 @@ name: foo TEST_F(RouteConfigProviderManagerImplTest, Basic) { Buffer::OwnedImpl data; - factory_context_.init_manager_.initialize(); - // Get a RouteConfigProvider. This one should create an entry in the RouteConfigProviderManager. setup(); @@ -616,9 +624,9 @@ TEST_F(RouteConfigProviderManagerImplTest, ValidateFail) { TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateEmpty) { setup(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(init_watcher_); auto& provider_impl = dynamic_cast(*provider_.get()); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); provider_impl.subscription().onConfigUpdate({}, ""); EXPECT_EQ( 1UL, factory_context_.scope_.counter("foo_prefix.rds.foo_route_config.update_empty").value()); @@ -626,12 +634,12 @@ TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateEmpty) { TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateWrongSize) { setup(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(init_watcher_); auto& provider_impl = dynamic_cast(*provider_.get()); Protobuf::RepeatedPtrField route_configs; route_configs.Add(); route_configs.Add(); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_THROW_WITH_MESSAGE(provider_impl.subscription().onConfigUpdate(route_configs, ""), EnvoyException, "Unexpected RDS resource length: 2"); } diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 87f6aacb16819..91108c6c07c0f 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -39,7 +39,12 @@ TEST_F(SdsApiTest, BasicTest) { const envoy::service::discovery::v2::SdsDummy dummy; NiceMock server; NiceMock init_manager; - EXPECT_CALL(init_manager, registerTarget(_, _)); + NiceMock init_watcher; + Init::TargetHandlePtr init_target_handle; + EXPECT_CALL(init_manager, add(_)) + .WillOnce(Invoke([&init_target_handle](const Init::Target& target) { + init_target_handle = target.createHandle("test"); + })); envoy::api::v2::core::ConfigSource config_source; config_source.mutable_api_config_source()->set_api_type( @@ -61,8 +66,8 @@ TEST_F(SdsApiTest, BasicTest) { EXPECT_CALL(*factory, create()).WillOnce(Invoke([grpc_client] { return Grpc::AsyncClientPtr{grpc_client}; })); - EXPECT_CALL(init_manager.initialized_, ready()); - init_manager.initialize(); + EXPECT_CALL(init_watcher, ready()); + init_target_handle->initialize(init_watcher); } // Validate that TlsCertificateSdsApi updates secrets successfully if a good secret diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 736a3dd783811..1e1e67bf565ee 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -675,6 +675,85 @@ TEST_P(AdsIntegrationTest, RdsAfterLdsWithRdsChange) { makeSingleRequest(); } +// Regression test for the use-after-free crash when a listener awaiting an RDS update is destroyed +// (#6116). +TEST_P(AdsIntegrationTest, RdsAfterLdsInvalidated) { + + initialize(); + + // STEP 1: Initial setup + // --------------------- + + // Initial request for any cluster, respond with cluster_0 version 1 + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {})); + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, + {buildCluster("cluster_0")}, "1"); + + // Initial request for load assignment for cluster_0, respond with version 1 + EXPECT_TRUE( + compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "", {"cluster_0"})); + sendDiscoveryResponse( + Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, "1"); + + // Request for updates to cluster_0 version 1, no response + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "1", {})); + + // Initial request for any listener, respond with listener_0 version 1 + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "", {})); + sendDiscoveryResponse( + Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_0")}, "1"); + + // Request for updates to load assignment version 1, no response + EXPECT_TRUE( + compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1", {"cluster_0"})); + + // Initial request for route_config_0 (referenced by listener_0), respond with version 1 + EXPECT_TRUE( + compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "", {"route_config_0"})); + sendDiscoveryResponse( + Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, + "1"); + + // Wait for initial listener to be created successfully. Any subsequent listeners will then use + // the dynamic InitManager (see ListenerImpl::initManager). + test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); + + // STEP 2: Listener with dynamic InitManager + // ----------------------------------------- + + // Request for updates to listener_0 version 1, respond with version 2. Under the hood, this + // registers RdsRouteConfigSubscription's init target with the new ListenerImpl instance. + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "1", {})); + sendDiscoveryResponse( + Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_1")}, "2"); + + // Request for updates to route_config_0 version 1, and initial request for route_config_1 + // (referenced by listener_0), don't respond yet! + EXPECT_TRUE( + compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "1", {"route_config_0"})); + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "1", + {"route_config_1", "route_config_0"})); + + // STEP 3: "New listener, who dis?" + // -------------------------------- + + // Request for updates to listener_0 version 2, respond with version 3 (updated stats prefix). + // This should blow away the previous ListenerImpl instance, which is still waiting for + // route_config_1... + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "2", {})); + sendDiscoveryResponse( + Config::TypeUrl::get().Listener, {buildListener("listener_0", "route_config_1", "omg")}, "3"); + + // Respond to prior request for route_config_1. Under the hood, this invokes + // RdsRouteConfigSubscription::runInitializeCallbackIfAny, which references the defunct + // ListenerImpl instance. We should not crash in this event! + sendDiscoveryResponse( + Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_1", "cluster_0")}, + "1"); + + test_server_->waitForCounterGe("listener_manager.listener_create_success", 2); +} + class AdsFailIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public HttpIntegrationTest { public: diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index d7508b3339477..a8cc23cafaa08 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -16,7 +16,6 @@ #include "test/integration/http_integration.h" #include "test/integration/server.h" #include "test/integration/ssl_utility.h" -#include "test/mocks/init/mocks.h" #include "test/mocks/secret/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/network_utility.h" diff --git a/test/integration/sds_static_integration_test.cc b/test/integration/sds_static_integration_test.cc index dcd1d0814d1ce..453942faf0c06 100644 --- a/test/integration/sds_static_integration_test.cc +++ b/test/integration/sds_static_integration_test.cc @@ -14,7 +14,6 @@ #include "test/integration/http_integration.h" #include "test/integration/server.h" #include "test/integration/ssl_utility.h" -#include "test/mocks/init/mocks.h" #include "test/mocks/secret/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/network_utility.h" diff --git a/test/mocks/init/BUILD b/test/mocks/init/BUILD index 682c862e1a4e7..5aa9f74bacd3c 100644 --- a/test/mocks/init/BUILD +++ b/test/mocks/init/BUILD @@ -13,7 +13,8 @@ envoy_cc_mock( srcs = ["mocks.cc"], hdrs = ["mocks.h"], deps = [ - "//include/envoy/init:init_interface", - "//test/mocks:common_lib", + "//include/envoy/init:manager_interface", + "//source/common/init:target_lib", + "//source/common/init:watcher_lib", ], ) diff --git a/test/mocks/init/mocks.cc b/test/mocks/init/mocks.cc index f968ad7c290bc..9e28923af3c0c 100644 --- a/test/mocks/init/mocks.cc +++ b/test/mocks/init/mocks.cc @@ -1,33 +1,25 @@ -#include "mocks.h" - -#include - -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -using testing::_; -using testing::Invoke; +#include "test/mocks/init/mocks.h" namespace Envoy { namespace Init { -MockTarget::MockTarget() { - ON_CALL(*this, initialize(_)) - .WillByDefault(Invoke([this](std::function callback) -> void { - EXPECT_EQ(nullptr, callback_); - callback_ = callback; - })); -} +using ::testing::Invoke; -MockTarget::~MockTarget() {} - -MockManager::MockManager() { - ON_CALL(*this, registerTarget(_, _)) - .WillByDefault(Invoke( - [this](Target& target, absl::string_view) -> void { targets_.push_back(&target); })); +ExpectableWatcherImpl::ExpectableWatcherImpl(absl::string_view name) + : WatcherImpl(name, {[this]() { ready(); }}) {} +::testing::internal::TypedExpectation& ExpectableWatcherImpl::expectReady() const { + return EXPECT_CALL(*this, ready()); } -MockManager::~MockManager() {} +ExpectableTargetImpl::ExpectableTargetImpl(absl::string_view name) + : TargetImpl(name, {[this]() { initialize(); }}) {} +::testing::internal::TypedExpectation& ExpectableTargetImpl::expectInitialize() { + return EXPECT_CALL(*this, initialize()); +} +::testing::internal::TypedExpectation& +ExpectableTargetImpl::expectInitializeWillCallReady() { + return expectInitialize().WillOnce(Invoke([this]() { ready(); })); +} } // namespace Init } // namespace Envoy diff --git a/test/mocks/init/mocks.h b/test/mocks/init/mocks.h index e8f6d093a8270..44189ac091447 100644 --- a/test/mocks/init/mocks.h +++ b/test/mocks/init/mocks.h @@ -1,44 +1,65 @@ #pragma once -#include -#include +#include "envoy/init/manager.h" -#include "envoy/init/init.h" - -#include "test/mocks/common.h" +#include "common/init/target_impl.h" +#include "common/init/watcher_impl.h" #include "gmock/gmock.h" namespace Envoy { namespace Init { -class MockTarget : public Target { +/** + * ExpectableWatcherImpl is a real WatcherImpl, subclassed to add a mock `ready` method that you can + * set expectations on in tests. Tests should never want a watcher with different behavior than the + * real implementation. + */ +class ExpectableWatcherImpl : public WatcherImpl { public: - MockTarget(); - ~MockTarget(); - - MOCK_METHOD1(initialize, void(std::function callback)); + ExpectableWatcherImpl(absl::string_view name = "test"); + MOCK_CONST_METHOD0(ready, void()); - std::function callback_; + /** + * Convenience method to provide a shorthand for EXPECT_CALL(watcher, ready()). Can be chained, + * for example: watcher.expectReady().Times(0); + */ + ::testing::internal::TypedExpectation& expectReady() const; }; -class MockManager : public Manager { +/** + * ExpectableTargetImpl is a real TargetImpl, subclassed to add a mock `initialize` method that you + * can set expectations on in tests. Tests should never want a target with a different behavior than + * the real implementation. + */ +class ExpectableTargetImpl : public TargetImpl { public: - MockManager(); - ~MockManager(); + ExpectableTargetImpl(absl::string_view name = "test"); + MOCK_METHOD0(initialize, void()); - void initialize() { - for (auto target : targets_) { - target->initialize([this]() -> void { initialized_.ready(); }); - } - } + /** + * Convenience method to provide a shorthand for EXPECT_CALL(target, initialize()). Can be + * chained, for example: target.expectInitialize().Times(0); + */ + ::testing::internal::TypedExpectation& expectInitialize(); - // Init::Manager - MOCK_METHOD2(registerTarget, void(Target& target, absl::string_view description)); - MOCK_CONST_METHOD0(state, State()); + /** + * Convenience method to provide a shorthand for expectInitialize() with mocked behavior of + * calling `ready` immediately. + */ + ::testing::internal::TypedExpectation& expectInitializeWillCallReady(); +}; - std::list targets_; - ReadyWatcher initialized_; +/** + * MockManager is a typical mock. In many cases, it won't be necessary to mock any of its methods. + * In cases where its `add` and `initialize` methods are actually called in a test, it's usually + * sufficient to mock `add` by saving the target argument locally, and to mock `initialize` by + * invoking the saved target with the watcher argument. + */ +struct MockManager : Manager { + MOCK_CONST_METHOD0(state, Manager::State()); + MOCK_METHOD1(add, void(const Target&)); + MOCK_METHOD1(initialize, void(const Watcher&)); }; } // namespace Init diff --git a/test/mocks/router/BUILD b/test/mocks/router/BUILD index 75e4a07c6f393..76e2c77be171b 100644 --- a/test/mocks/router/BUILD +++ b/test/mocks/router/BUILD @@ -14,7 +14,6 @@ envoy_cc_mock( hdrs = ["mocks.h"], deps = [ "//include/envoy/event:dispatcher_interface", - "//include/envoy/init:init_interface", "//include/envoy/json:json_object_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/router:route_config_provider_manager_interface", diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 8a1e7d30653d8..ba40815aaab49 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -10,7 +10,6 @@ #include "envoy/config/typed_metadata.h" #include "envoy/event/dispatcher.h" -#include "envoy/init/init.h" #include "envoy/json/json_object.h" #include "envoy/local_info/local_info.h" #include "envoy/router/rds.h" diff --git a/test/mocks/safe_init/BUILD b/test/mocks/safe_init/BUILD deleted file mode 100644 index fbb24c52d1861..0000000000000 --- a/test/mocks/safe_init/BUILD +++ /dev/null @@ -1,20 +0,0 @@ -licenses(["notice"]) # Apache 2 - -load( - "//bazel:envoy_build_system.bzl", - "envoy_cc_mock", - "envoy_package", -) - -envoy_package() - -envoy_cc_mock( - name = "safe_init_mocks", - srcs = ["mocks.cc"], - hdrs = ["mocks.h"], - deps = [ - "//include/envoy/safe_init:manager_interface", - "//source/common/safe_init:target_lib", - "//source/common/safe_init:watcher_lib", - ], -) diff --git a/test/mocks/safe_init/mocks.cc b/test/mocks/safe_init/mocks.cc deleted file mode 100644 index 1ef93da2f3074..0000000000000 --- a/test/mocks/safe_init/mocks.cc +++ /dev/null @@ -1,25 +0,0 @@ -#include "test/mocks/safe_init/mocks.h" - -namespace Envoy { -namespace SafeInit { - -using ::testing::Invoke; - -ExpectableWatcherImpl::ExpectableWatcherImpl(absl::string_view name) - : WatcherImpl(name, {[this]() { ready(); }}) {} -::testing::internal::TypedExpectation& ExpectableWatcherImpl::expectReady() const { - return EXPECT_CALL(*this, ready()); -} - -ExpectableTargetImpl::ExpectableTargetImpl(absl::string_view name) - : TargetImpl(name, {[this]() { initialize(); }}) {} -::testing::internal::TypedExpectation& ExpectableTargetImpl::expectInitialize() { - return EXPECT_CALL(*this, initialize()); -} -::testing::internal::TypedExpectation& -ExpectableTargetImpl::expectInitializeWillCallReady() { - return expectInitialize().WillOnce(Invoke([this]() { ready(); })); -} - -} // namespace SafeInit -} // namespace Envoy diff --git a/test/mocks/safe_init/mocks.h b/test/mocks/safe_init/mocks.h deleted file mode 100644 index 92a41cf4d7389..0000000000000 --- a/test/mocks/safe_init/mocks.h +++ /dev/null @@ -1,66 +0,0 @@ -#pragma once - -#include "envoy/safe_init/manager.h" - -#include "common/safe_init/target_impl.h" -#include "common/safe_init/watcher_impl.h" - -#include "gmock/gmock.h" - -namespace Envoy { -namespace SafeInit { - -/** - * ExpectableWatcherImpl is a real WatcherImpl, subclassed to add a mock `ready` method that you can - * set expectations on in tests. Tests should never want a watcher with different behavior than the - * real implementation. - */ -class ExpectableWatcherImpl : public WatcherImpl { -public: - ExpectableWatcherImpl(absl::string_view name = "test"); - MOCK_CONST_METHOD0(ready, void()); - - /** - * Convenience method to provide a shorthand for EXPECT_CALL(watcher, ready()). Can be chained, - * for example: watcher.expectReady().Times(0); - */ - ::testing::internal::TypedExpectation& expectReady() const; -}; - -/** - * ExpectableTargetImpl is a real TargetImpl, subclassed to add a mock `initialize` method that you - * can set expectations on in tests. Tests should never want a target with a different behavior than - * the real implementation. - */ -class ExpectableTargetImpl : public TargetImpl { -public: - ExpectableTargetImpl(absl::string_view name = "test"); - MOCK_METHOD0(initialize, void()); - - /** - * Convenience method to provide a shorthand for EXPECT_CALL(target, initialize()). Can be - * chained, for example: target.expectInitialize().Times(0); - */ - ::testing::internal::TypedExpectation& expectInitialize(); - - /** - * Convenience method to provide a shorthand for expectInitialize() with mocked behavior of - * calling `ready` immediately. - */ - ::testing::internal::TypedExpectation& expectInitializeWillCallReady(); -}; - -/** - * MockManager is a typical mock. In many cases, it won't be necessary to mock any of its methods. - * In cases where its `add` and `initialize` methods are actually called in a test, it's usually - * sufficient to mock `add` by saving the target argument locally, and to mock `initialize` by - * invoking the saved target with the watcher argument. - */ -struct MockManager : Manager { - MOCK_CONST_METHOD0(state, Manager::State()); - MOCK_METHOD1(add, void(const Target&)); - MOCK_METHOD1(initialize, void(const Watcher&)); -}; - -} // namespace SafeInit -} // namespace Envoy diff --git a/test/server/BUILD b/test/server/BUILD index 1c64006680419..8cae70848f762 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -80,16 +80,6 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "init_manager_impl_test", - srcs = ["init_manager_impl_test.cc"], - deps = [ - "//source/server:init_manager_lib", - "//test/mocks:common_lib", - "//test/mocks/init:init_mocks", - ], -) - envoy_cc_test( name = "guarddog_impl_test", srcs = ["guarddog_impl_test.cc"], diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index bae835f4f65af..bffaf0c9380f6 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1191,7 +1191,7 @@ TEST_P(AdminInstanceTest, GetRequest) { Http::HeaderMapImpl response_headers; std::string body; - ON_CALL(initManager, state()).WillByDefault(Return(Init::Manager::State::NotInitialized)); + ON_CALL(initManager, state()).WillByDefault(Return(Init::Manager::State::Uninitialized)); EXPECT_EQ(Http::Code::OK, admin_.request("/server_info", "GET", response_headers, body)); envoy::admin::v2alpha::ServerInfo server_info_proto; EXPECT_THAT(std::string(response_headers.ContentType()->value().getStringView()), diff --git a/test/server/init_manager_impl_test.cc b/test/server/init_manager_impl_test.cc deleted file mode 100644 index 964db18551670..0000000000000 --- a/test/server/init_manager_impl_test.cc +++ /dev/null @@ -1,69 +0,0 @@ -#include "server/init_manager_impl.h" - -#include "test/mocks/common.h" -#include "test/mocks/init/mocks.h" - -#include "gmock/gmock.h" - -using testing::_; -using testing::InSequence; -using testing::Invoke; - -namespace Envoy { -namespace Server { -namespace { - -class InitManagerImplTest : public testing::Test { -public: - InitManagerImpl manager_{"test"}; - ReadyWatcher initialized_; -}; - -TEST_F(InitManagerImplTest, NoTargets) { - EXPECT_CALL(initialized_, ready()); - manager_.initialize([&]() -> void { initialized_.ready(); }); -} - -TEST_F(InitManagerImplTest, Targets) { - InSequence s; - Init::MockTarget target; - - manager_.registerTarget(target, ""); - EXPECT_CALL(target, initialize(_)); - manager_.initialize([&]() -> void { initialized_.ready(); }); - EXPECT_CALL(initialized_, ready()); - target.callback_(); -} - -TEST_F(InitManagerImplTest, TargetRemoveWhileInitializing) { - InSequence s; - Init::MockTarget target; - - manager_.registerTarget(target, ""); - EXPECT_CALL(target, initialize(_)).WillOnce(Invoke([](std::function callback) -> void { - callback(); - })); - EXPECT_CALL(initialized_, ready()); - manager_.initialize([&]() -> void { initialized_.ready(); }); -} - -TEST_F(InitManagerImplTest, TargetAfterInitializing) { - InSequence s; - Init::MockTarget target1; - Init::MockTarget target2; - - manager_.registerTarget(target1, ""); - EXPECT_CALL(target1, initialize(_)); - manager_.initialize([&]() -> void { initialized_.ready(); }); - - EXPECT_CALL(target2, initialize(_)); - manager_.registerTarget(target2, ""); - - target2.callback_(); - EXPECT_CALL(initialized_, ready()); - target1.callback_(); -} - -} // namespace -} // namespace Server -} // namespace Envoy diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 7c73374a1d986..87e5ff11c921f 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -25,7 +25,11 @@ namespace { class LdsApiTest : public testing::Test { public: - LdsApiTest() : request_(&cluster_manager_.async_client_), api_(Api::createApiForTest(store_)) {} + LdsApiTest() : request_(&cluster_manager_.async_client_), api_(Api::createApiForTest(store_)) { + ON_CALL(init_manager_, add(_)).WillByDefault(Invoke([this](const Init::Target& target) { + init_target_handle_ = target.createHandle("test"); + })); + } void setup() { const std::string config_json = R"EOF( @@ -50,12 +54,13 @@ class LdsApiTest : public testing::Test { EXPECT_CALL(cluster, info()); EXPECT_CALL(*cluster.info_, type()); interval_timer_ = new Event::MockTimer(&dispatcher_); - EXPECT_CALL(init_, registerTarget(_, _)); - lds_ = std::make_unique(lds_config, cluster_manager_, dispatcher_, random_, init_, - local_info_, store_, listener_manager_, *api_); + EXPECT_CALL(init_manager_, add(_)); + lds_ = + std::make_unique(lds_config, cluster_manager_, dispatcher_, random_, + init_manager_, local_info_, store_, listener_manager_, *api_); expectRequest(); - init_.initialize(); + init_target_handle_->initialize(init_watcher_); } void expectAdd(const std::string& listener_name, absl::optional version, @@ -121,7 +126,9 @@ class LdsApiTest : public testing::Test { NiceMock cluster_manager_; Event::MockDispatcher dispatcher_; NiceMock random_; - Init::MockManager init_; + Init::MockManager init_manager_; + Init::ExpectableWatcherImpl init_watcher_; + Init::TargetHandlePtr init_target_handle_; NiceMock local_info_; Stats::IsolatedStoreImpl store_; MockListenerManager listener_manager_; @@ -163,8 +170,8 @@ TEST_F(LdsApiTest, UnknownCluster) { Upstream::ClusterManager::ClusterInfoMap cluster_map; EXPECT_CALL(cluster_manager_, clusters()).WillOnce(Return(cluster_map)); EXPECT_THROW_WITH_MESSAGE( - LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_, local_info_, store_, - listener_manager_, *api_), + LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_manager_, local_info_, + store_, listener_manager_, *api_), EnvoyException, "envoy::api::v2::core::ConfigSource must have a statically defined non-EDS " "cluster: 'foo_cluster' does not exist, was added via api, or is an " @@ -191,7 +198,7 @@ TEST_F(LdsApiTest, MisconfiguredListenerNameIsPresentInException) { EXPECT_CALL(listener_manager_, addOrUpdateListener(_, _, true)) .WillOnce(Throw(EnvoyException("something is wrong"))); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_THROW_WITH_MESSAGE( lds_->onConfigUpdate(listeners, ""), EnvoyException, @@ -209,7 +216,7 @@ TEST_F(LdsApiTest, EmptyListenersUpdate) { EXPECT_CALL(listener_manager_, listeners()).WillOnce(Return(existing_listeners)); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(request_, cancel()); lds_->onConfigUpdate(listeners, ""); @@ -237,7 +244,7 @@ TEST_F(LdsApiTest, ListenerCreationContinuesEvenAfterException) { .WillOnce(Return(true)) .WillOnce(Throw(EnvoyException("something else is wrong"))); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_THROW_WITH_MESSAGE(lds_->onConfigUpdate(listeners, ""), EnvoyException, "Error adding/updating listener(s) invalid-listener-1: something is " @@ -285,8 +292,8 @@ TEST_F(LdsApiTest, BadLocalInfo) { EXPECT_CALL(*cluster.info_, type()); ON_CALL(local_info_, clusterName()).WillByDefault(Return(std::string())); EXPECT_THROW_WITH_MESSAGE( - LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_, local_info_, store_, - listener_manager_, *api_), + LdsApiImpl(lds_config, cluster_manager_, dispatcher_, random_, init_manager_, local_info_, + store_, listener_manager_, *api_), EnvoyException, "lds: node 'id' and 'cluster' are required. Set it either in 'node' config or via " "--service-node and --service-cluster options."); @@ -324,7 +331,7 @@ TEST_F(LdsApiTest, Basic) { makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); expectAdd("listener2", "0", true); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -397,7 +404,7 @@ TEST_F(LdsApiTest, TlsConfigWithoutCaCert) { makeListenersAndExpectCall({"listener0"}); expectAdd("listener0", {}, true); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -474,7 +481,7 @@ TEST_F(LdsApiTest, Failure) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response_json); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -524,7 +531,7 @@ TEST_F(LdsApiTest, ReplacingListenerWithSameAddress) { makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); expectAdd("listener2", "0", true); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(init_watcher_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 5c0cf8bc1ee6e..53f9e086ba7ee 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -51,7 +51,7 @@ class ListenerHandle { MOCK_METHOD0(onDestroy, void()); - Init::MockTarget target_; + Init::ExpectableTargetImpl target_; MockDrainManager* drain_manager_ = new MockDrainManager(); Configuration::FactoryContext* context_{}; }; @@ -85,7 +85,7 @@ class ListenerManagerImplTest : public testing::Test { std::shared_ptr notifier(raw_listener); raw_listener->context_ = &context; if (need_init) { - context.initManager().registerTarget(notifier->target_, ""); + context.initManager().add(notifier->target_); } return {[notifier](Network::FilterManager&) -> void {}}; })); @@ -866,7 +866,7 @@ filter_chains: {} ListenerHandle* listener_baz = expectListenerCreate(true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); - EXPECT_CALL(listener_baz->target_, initialize(_)); + EXPECT_CALL(listener_baz->target_, initialize()); EXPECT_TRUE( manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_baz_yaml), "version5", true)); EXPECT_EQ(2UL, manager_->listeners().size()); @@ -931,9 +931,9 @@ version_info: version5 ListenerHandle* listener_baz_update1 = expectListenerCreate(true); EXPECT_CALL(*listener_baz, onDestroy()).WillOnce(Invoke([listener_baz]() -> void { // Call the initialize callback during destruction like RDS will. - listener_baz->target_.callback_(); + listener_baz->target_.ready(); })); - EXPECT_CALL(listener_baz_update1->target_, initialize(_)); + EXPECT_CALL(listener_baz_update1->target_, initialize()); EXPECT_TRUE( manager_->addOrUpdateListener(parseListenerFromJson(listener_baz_update1_json), "", true)); EXPECT_EQ(2UL, manager_->listeners().size()); @@ -941,7 +941,7 @@ version_info: version5 // Finish initialization for baz which should make it active. EXPECT_CALL(*worker_, addListener(_, _)); - listener_baz_update1->target_.callback_(); + listener_baz_update1->target_.ready(); EXPECT_EQ(3UL, manager_->listeners().size()); worker_->callAddCompletion(true); checkStats(3, 3, 0, 0, 3, 0); @@ -1090,7 +1090,7 @@ TEST_F(ListenerManagerImplTest, RemoveListener) { ListenerHandle* listener_foo = expectListenerCreate(true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); - EXPECT_CALL(listener_foo->target_, initialize(_)); + EXPECT_CALL(listener_foo->target_, initialize()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromJson(listener_foo_json), "", true)); EXPECT_EQ(0UL, manager_->listeners().size()); checkStats(1, 0, 0, 1, 0, 0); @@ -1104,11 +1104,11 @@ TEST_F(ListenerManagerImplTest, RemoveListener) { // Add foo again and initialize it. listener_foo = expectListenerCreate(true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); - EXPECT_CALL(listener_foo->target_, initialize(_)); + EXPECT_CALL(listener_foo->target_, initialize()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromJson(listener_foo_json), "", true)); checkStats(2, 0, 1, 1, 0, 0); EXPECT_CALL(*worker_, addListener(_, _)); - listener_foo->target_.callback_(); + listener_foo->target_.ready(); worker_->callAddCompletion(true); EXPECT_EQ(1UL, manager_->listeners().size()); checkStats(2, 0, 1, 0, 1, 0); @@ -1125,7 +1125,7 @@ TEST_F(ListenerManagerImplTest, RemoveListener) { )EOF"; ListenerHandle* listener_foo_update1 = expectListenerCreate(true); - EXPECT_CALL(listener_foo_update1->target_, initialize(_)); + EXPECT_CALL(listener_foo_update1->target_, initialize()); EXPECT_TRUE( manager_->addOrUpdateListener(parseListenerFromJson(listener_foo_update1_json), "", true)); EXPECT_EQ(1UL, manager_->listeners().size()); @@ -1212,7 +1212,7 @@ TEST_F(ListenerManagerImplTest, DuplicateAddressDontBind) { ListenerHandle* listener_foo = expectListenerCreate(true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, false)); - EXPECT_CALL(listener_foo->target_, initialize(_)); + EXPECT_CALL(listener_foo->target_, initialize()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromJson(listener_foo_json), "", true)); // Add bar with same non-binding address. Should fail. @@ -1234,7 +1234,7 @@ TEST_F(ListenerManagerImplTest, DuplicateAddressDontBind) { // Move foo to active and then try to add again. This should still fail. EXPECT_CALL(*worker_, addListener(_, _)); - listener_foo->target_.callback_(); + listener_foo->target_.ready(); worker_->callAddCompletion(true); listener_bar = expectListenerCreate(true); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 6206b8282e4b6..c961b393e2f09 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -80,7 +80,7 @@ class RunHelperTest : public testing::Test { NiceMock cm_; NiceMock access_log_manager_; NiceMock overload_manager_; - InitManagerImpl init_manager_{""}; + Init::ManagerImpl init_manager_{""}; ReadyWatcher start_workers_; std::unique_ptr helper_; std::function cm_init_callback_; @@ -105,13 +105,13 @@ TEST_F(RunHelperTest, ShutdownBeforeCmInitialize) { TEST_F(RunHelperTest, ShutdownBeforeInitManagerInit) { EXPECT_CALL(start_workers_, ready()).Times(0); - Init::MockTarget target; - init_manager_.registerTarget(target, ""); - EXPECT_CALL(target, initialize(_)); + Init::ExpectableTargetImpl target; + init_manager_.add(target); + EXPECT_CALL(target, initialize()); cm_init_callback_(); sigterm_->callback_(); EXPECT_CALL(server_, isShutdown()).WillOnce(Return(shutdown_)); - target.callback_(); + target.ready(); } // Class creates minimally viable server instance for testing.