Skip to content

listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 23 commits into from
Mar 4, 2020
Merged

listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7b3bfc9
replace FactoryContext with ServerFactoryContext in router/... so tha…
stevenzzzz Jan 22, 2020
e8de6c1
fix feedbacks
stevenzzzz Jan 27, 2020
64757d1
if only we can turn up spelling check at local
stevenzzzz Jan 27, 2020
17570bc
merge with master
stevenzzzz Jan 28, 2020
282a4c9
fix typo introduced during merging w/master
stevenzzzz Jan 28, 2020
71842d5
move local-init-manager to config-provider framework
stevenzzzz Jan 31, 2020
2c4b342
merge upstream master
stevenzzzz Feb 3, 2020
86799d4
fixes per review feedbacks
stevenzzzz Feb 7, 2020
7fd6fbe
add back the missing listenerimpl destructor, of course I have 4 back…
stevenzzzz Feb 7, 2020
2ba3f96
Merge branch 'master' into final-look-longlive-srds
stevenzzzz Feb 11, 2020
f01dc42
add a test for listener_impl destructor notifying parent initManager
stevenzzzz Feb 13, 2020
81c9fee
fix wrong parameter comment
stevenzzzz Feb 13, 2020
e13189e
fix accidental typo
stevenzzzz Feb 14, 2020
e7c567a
Merge branch 'master' into final-look-longlive-srds
stevenzzzz Feb 14, 2020
3a39741
fix one typo at a time
stevenzzzz Feb 14, 2020
78cb57d
add a integration test for listener + LDS
stevenzzzz Feb 28, 2020
2dadf40
merge from master
stevenzzzz Feb 28, 2020
7f7786f
some comment update on integration test
stevenzzzz Feb 28, 2020
ebd5d3d
fix format and typo
stevenzzzz Feb 28, 2020
65eb728
fix typo, one at a time
stevenzzzz Mar 2, 2020
8145447
Merge branch 'master' of https://github.com/envoyproxy/envoy into fin…
stevenzzzz Mar 2, 2020
d1204f1
make type more specific
stevenzzzz Mar 2, 2020
b08bded
fix compilation error
stevenzzzz Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions include/envoy/config/config_provider_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class ConfigProviderManager {
* and Config are shared amongst all providers relying on the same config source.
* @param config_source_proto supplies the proto containing the xDS API configuration.
* @param factory_context is the context to use for the provider.
* @param init_manager is the Init::Manager to use for the provider.
* @param stat_prefix supplies the prefix to use for statistics.
* @param optarg supplies an optional argument with data specific to the concrete class.
* @return ConfigProviderPtr a newly allocated dynamic config provider which shares underlying
Expand All @@ -52,8 +53,9 @@ class ConfigProviderManager {
*/
virtual ConfigProviderPtr
createXdsConfigProvider(const Protobuf::Message& config_source_proto,
Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix, const OptionalArg& optarg) PURE;
Server::Configuration::ServerFactoryContext& factory_context,
Init::Manager& init_manager, const std::string& stat_prefix,
const OptionalArg& optarg) PURE;

/**
* Returns a ConfigProvider associated with a statically specified configuration.
Expand All @@ -64,7 +66,7 @@ class ConfigProviderManager {
*/
virtual ConfigProviderPtr
createStaticConfigProvider(const Protobuf::Message& config_proto,
Server::Configuration::FactoryContext& factory_context,
Server::Configuration::ServerFactoryContext& factory_context,
const OptionalArg& optarg) {
UNREFERENCED_PARAMETER(config_proto);
UNREFERENCED_PARAMETER(factory_context);
Expand All @@ -82,7 +84,7 @@ class ConfigProviderManager {
*/
virtual ConfigProviderPtr
createStaticConfigProvider(ProtobufTypes::ConstMessagePtrVector&& config_protos,
Server::Configuration::FactoryContext& factory_context,
Server::Configuration::ServerFactoryContext& factory_context,
const OptionalArg& optarg) {
UNREFERENCED_PARAMETER(config_protos);
UNREFERENCED_PARAMETER(factory_context);
Expand Down
9 changes: 5 additions & 4 deletions include/envoy/router/route_config_provider_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,20 @@ class RouteConfigProviderManager {
*/
virtual RouteConfigProviderSharedPtr createRdsRouteConfigProvider(
const envoy::extensions::filters::network::http_connection_manager::v3::Rds& rds,
Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
Server::Configuration::ServerFactoryContext& factory_context, const std::string& stat_prefix,
Init::Manager& init_manager) PURE;

/**
* Get a RouteConfigSharedPtr for a statically defined route. Ownership is as described for
* getRdsRouteConfigProvider above. This method always create a new RouteConfigProvider.
* @param route_config supplies the RouteConfiguration for this route
* @param runtime supplies the runtime loader.
* @param cm supplies the ClusterManager.
* @param factory_context is the context to use for the route config provider.
* @param validator is the message validator for route config.
*/
virtual RouteConfigProviderPtr
createStaticRouteConfigProvider(const envoy::config::route::v3::RouteConfiguration& route_config,
Server::Configuration::FactoryContext& factory_context) PURE;
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator) PURE;
Copy link
Member

@junr03 junr03 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to read the rest of the PR, but given the change below which add messageValidationVisitor() to the CommonFactoryContext, why is this function also taking a validator as a parameter?

Copy link
Member

@junr03 junr03 Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevenzzzz I am still wondering about this

Copy link
Contributor Author

@stevenzzzz stevenzzzz Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, missed this comment. I think you mean "add a validation visitor context"?
It's because we can't tell from the current ServerFactoryContext if the static validator or the dynamic validator should be used.
For example, a static Route config provider could be put into a dynamic(LDS) listener's config, in which case we should use the dynamic validator instead (per Harvey).
For that, we will have to pass validator alongside the Serverfatorycontext.

Copy link
Member

@junr03 junr03 Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so only the code that is building the config provider knows if it should be using the static or dynamic visitor, but the constructor itself does not. Hence why the caller of createStaticRouteConfigProvider needs to pass the intended visitor.

Not to bike shed, but it seems that we have gone in one direction here, and the opposite in the ListernerImpl constructor where we used to pass the validator reference but now we decide inside the constructor. Should we pass an added_via_api boolean here and have the constructor decide?

Copy link
Contributor Author

@stevenzzzz stevenzzzz Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, I'd like the validator choice made as close to the consumer entity(in this case, the listenerImpl) as possible, which provides better modularity.

The listener could make the decision by itself as there is a somewhat hacky paramter add_via_api in its constructor, the added_via_api information could be deducted by any RDS-config-provider or any subscription-initiated config-provider,
but not in static config provider: its context doesn't provide enough information for making the choice.

Copy link
Member

@junr03 junr03 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we pass an added_via_api boolean here and have the constructor decide?

Right, that is why I was suggesting if we should hint the static config provider as well (albeit I have not looked if we have access to the same hint when we create this config provider, but my intuition tells me we do). I am not sure what the right answer here is. My point is that we seem to have gone in opposite directions where we could have the same setup.

Perhaps the next reviewer can state their preference.

Copy link
Contributor Author

@stevenzzzz stevenzzzz Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed this w/ Harvey before, passing the correct validator is preferred over hinting (passing the added_via_api signal). Agreed it's a preference thing as to if we want to keep all the places have the same interface.

};

} // namespace Router
Expand Down
6 changes: 6 additions & 0 deletions include/envoy/server/filter_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ class CommonFactoryContext {
*/
virtual const LocalInfo::LocalInfo& localInfo() const PURE;

/**
* @return ProtobufMessage::ValidationContext& validation visitor for xDS and static configuration
* messages.
*/
virtual ProtobufMessage::ValidationContext& messageValidationContext() PURE;

/**
* @return RandomGenerator& the random generator for the server.
*/
Expand Down
2 changes: 2 additions & 0 deletions source/common/config/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ envoy_cc_library(
"//include/envoy/server:config_tracker_interface",
"//include/envoy/singleton:instance_interface",
"//include/envoy/thread_local:thread_local_interface",
"//source/common/init:manager_lib",
"//source/common/init:target_lib",
"//source/common/init:watcher_lib",
"//source/common/protobuf",
],
)
Expand Down
4 changes: 2 additions & 2 deletions source/common/config/config_provider_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ namespace Envoy {
namespace Config {

ImmutableConfigProviderBase::ImmutableConfigProviderBase(
Server::Configuration::FactoryContext& factory_context,
Server::Configuration::ServerFactoryContext& factory_context,
ConfigProviderManagerImplBase& config_provider_manager,
ConfigProviderInstanceType instance_type, ApiType api_type)
: last_updated_(factory_context.timeSource().systemTime()),
Expand All @@ -20,7 +20,7 @@ ImmutableConfigProviderBase::~ImmutableConfigProviderBase() {
}

ConfigSubscriptionCommonBase::~ConfigSubscriptionCommonBase() {
init_target_.ready();
local_init_target_.ready();
config_provider_manager_.unbindSubscription(manager_identifier_);
}

Expand Down
44 changes: 29 additions & 15 deletions source/common/config/config_provider_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
#include "common/common/thread.h"
#include "common/common/utility.h"
#include "common/config/utility.h"
#include "common/init/manager_impl.h"
#include "common/init/target_impl.h"
#include "common/init/watcher_impl.h"
#include "common/protobuf/protobuf.h"

namespace Envoy {
Expand Down Expand Up @@ -115,7 +117,7 @@ class ImmutableConfigProviderBase : public ConfigProvider {
ConfigProviderInstanceType instanceType() const { return instance_type_; }

protected:
ImmutableConfigProviderBase(Server::Configuration::FactoryContext& factory_context,
ImmutableConfigProviderBase(Server::Configuration::ServerFactoryContext& factory_context,
ConfigProviderManagerImplBase& config_provider_manager,
ConfigProviderInstanceType instance_type, ApiType api_type);

Expand All @@ -137,12 +139,6 @@ class MutableConfigProviderCommonBase;
* A subscription is intended to be co-owned by config providers with the same config source, it's
* designed to be created/destructed on admin thread only.
*
* xDS config providers and subscriptions are split to avoid lifetime issues with arguments
* required by the config providers. An example is the Server::Configuration::FactoryContext, which
* is owned by listeners and therefore may be destroyed while an associated config provider is still
* in use (see #3960). This split enables single ownership of the config providers, while enabling
* shared ownership of the underlying subscription.
*
*/
class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::config> {
public:
Expand Down Expand Up @@ -180,7 +176,7 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf
*/
void onConfigUpdate() {
setLastUpdated();
init_target_.ready();
local_init_target_.ready();
}

/**
Expand All @@ -189,7 +185,7 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf
*/
void onConfigUpdateFailed() {
setLastUpdated();
init_target_.ready();
local_init_target_.ready();
}

protected:
Expand All @@ -202,13 +198,22 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf

ConfigSubscriptionCommonBase(const std::string& name, const uint64_t manager_identifier,
ConfigProviderManagerImplBase& config_provider_manager,
Server::Configuration::FactoryContext& factory_context)
Server::Configuration::ServerFactoryContext& factory_context)
: name_(name), tls_(factory_context.threadLocal().allocateSlot()),
init_target_(absl::StrCat("ConfigSubscriptionCommonBase ", name_), [this]() { start(); }),
local_init_target_(
fmt::format("ConfigSubscriptionCommonBase local init target '{}'", name_),
[this]() { start(); }),
parent_init_target_(fmt::format("ConfigSubscriptionCommonBase init target '{}'", name_),
[this]() { local_init_manager_.initialize(local_init_watcher_); }),
local_init_watcher_(fmt::format("ConfigSubscriptionCommonBase local watcher '{}'", name_),
[this]() { parent_init_target_.ready(); }),
local_init_manager_(
fmt::format("ConfigSubscriptionCommonBase local init manager '{}'", name_)),
manager_identifier_(manager_identifier), config_provider_manager_(config_provider_manager),
time_source_(factory_context.timeSource()),
last_updated_(factory_context.timeSource().systemTime()) {
Envoy::Config::Utility::checkLocalInfo(name, factory_context.localInfo());
local_init_manager_.add(local_init_target_);
}

/**
Expand All @@ -220,7 +225,7 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf
void applyConfigUpdate(const ConfigUpdateCb& update_fn);

void setLastUpdated() { last_updated_ = time_source_.systemTime(); }

Init::Manager& localInitManager() { return local_init_manager_; }
void setLastConfigInfo(absl::optional<LastConfigInfo>&& config_info) {
config_info_ = std::move(config_info);
}
Expand All @@ -232,7 +237,16 @@ class ConfigSubscriptionCommonBase : protected Logger::Loggable<Logger::Id::conf
ThreadLocal::SlotPtr tls_;

private:
Init::TargetImpl init_target_;
// Local init target which signals first RPC interaction with management server.
Init::TargetImpl local_init_target_;
// Target added to factory context's initManager.
Init::TargetImpl parent_init_target_;
// Watcher that marks parent_init_target_ ready when the local init manager is ready.
Init::WatcherImpl local_init_watcher_;
// Local manager that tracks the subscription initialization, it is also used for sub-resource
// initialization if the sub-resource is not initialized.
Init::ManagerImpl local_init_manager_;

const uint64_t manager_identifier_;
ConfigProviderManagerImplBase& config_provider_manager_;
TimeSource& time_source_;
Expand All @@ -259,7 +273,7 @@ class ConfigSubscriptionInstance : public ConfigSubscriptionCommonBase {
public:
ConfigSubscriptionInstance(const std::string& name, const uint64_t manager_identifier,
ConfigProviderManagerImplBase& config_provider_manager,
Server::Configuration::FactoryContext& factory_context)
Server::Configuration::ServerFactoryContext& factory_context)
: ConfigSubscriptionCommonBase(name, manager_identifier, config_provider_manager,
factory_context) {}

Expand Down Expand Up @@ -419,7 +433,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);
init_manager.add(subscription->init_target_);
init_manager.add(subscription->parent_init_target_);

bindSubscription(manager_identifier, subscription);
} else {
Expand Down
Loading