Skip to content

ecds: refactor FilterConfigProviderManagerImpl into base and derived classes #17852

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
snowp merged 3 commits into from
Aug 31, 2021
Merged

ecds: refactor FilterConfigProviderManagerImpl into base and derived classes #17852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34a8c9d
ecds: refactor FilterConfigProviderManagerImpl into base and derived …
tbarrella Aug 25, 2021
d8a05b8
move type url validation
tbarrella Aug 25, 2021
7796988
remove unneeded declaration
tbarrella Aug 25, 2021
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
112 changes: 62 additions & 50 deletions source/common/filter/config_discovery_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ void DynamicFilterConfigProviderImplBase::validateTerminalFilter(const std::stri
FilterConfigSubscription::FilterConfigSubscription(
const envoy::config::core::v3::ConfigSource& config_source,
const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix, FilterConfigProviderManagerImpl& filter_config_provider_manager,
const std::string& stat_prefix,
FilterConfigProviderManagerImplBase& filter_config_provider_manager,
const std::string& subscription_id)
: Config::SubscriptionBase<envoy::config::core::v3::TypedExtensionConfig>(
factory_context.messageValidationContext().dynamicValidationVisitor(), "name"),
Expand Down Expand Up @@ -190,7 +191,7 @@ FilterConfigSubscription::~FilterConfigSubscription() {

void FilterConfigSubscription::incrementConflictCounter() { stats_.config_conflict_.inc(); }

std::shared_ptr<FilterConfigSubscription> FilterConfigProviderManagerImpl::getSubscription(
std::shared_ptr<FilterConfigSubscription> FilterConfigProviderManagerImplBase::getSubscription(
const envoy::config::core::v3::ConfigSource& config_source, const std::string& name,
Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) {
// FilterConfigSubscriptions are unique based on their config source and filter config name
Expand All @@ -212,6 +213,39 @@ std::shared_ptr<FilterConfigSubscription> FilterConfigProviderManagerImpl::getSu
}
}

void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig(
std::shared_ptr<FilterConfigSubscription>& subscription,
DynamicFilterConfigProviderImplBase& provider, const std::string& filter_config_name) {
// If the subscription already received a config, attempt to apply it.
// It is possible that the received extension config fails to satisfy the listener
// type URL constraints. This may happen if ECDS and LDS updates are racing, and the LDS
// update arrives first. In this case, use the default config, increment a metric,
// and the applied config eventually converges once ECDS update arrives.
bool last_config_valid = false;
if (subscription->lastConfig().has_value()) {
TRY_ASSERT_MAIN_THREAD {
provider.validateTypeUrl(subscription->lastTypeUrl());
provider.validateTerminalFilter(filter_config_name, subscription->lastFilterName(),
subscription->isLastFilterTerminal());
last_config_valid = true;
}
END_TRY catch (const EnvoyException& e) {
ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.",
filter_config_name, e.what());
subscription->incrementConflictCounter();
}
if (last_config_valid) {
provider.onConfigUpdate(subscription->lastConfig().value(), subscription->lastVersionInfo(),
nullptr);
}
}

// Apply the default config if none has been applied.
if (!last_config_valid) {
provider.applyDefaultConfiguration();
}
}

DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConfigProvider(
const envoy::config::core::v3::ExtensionConfigSource& config_source,
const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context,
Expand All @@ -233,26 +267,9 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil

Envoy::Http::FilterFactoryCb default_config = nullptr;
if (config_source.has_default_config()) {
auto* default_factory =
Config::Utility::getFactoryByType<Server::Configuration::NamedHttpFilterConfigFactory>(
config_source.default_config());
if (default_factory == nullptr) {
throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter "
"configuration with type URL {}.",
filter_config_name,
config_source.default_config().type_url()));
}
validateTypeUrlHelper(Config::Utility::getFactoryType(config_source.default_config()),
require_type_urls);
ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig(
config_source.default_config(), factory_context.messageValidationVisitor(),
*default_factory);
Config::Utility::validateTerminalFilters(
filter_config_name, default_factory->name(), filter_chain_type,
default_factory->isTerminalFilterByProto(*message, factory_context),
last_filter_in_filter_config);
default_config =
default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context);
default_config = getDefaultConfig(config_source.default_config(), filter_config_name,
factory_context, stat_prefix, last_filter_in_filter_config,
filter_chain_type, require_type_urls);
}

auto provider = std::make_unique<DynamicFilterConfigProviderImpl>(
Expand All @@ -263,36 +280,31 @@ DynamicFilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFil
if (config_source.apply_default_config_without_warming()) {
factory_context.initManager().add(provider->initTarget());
}
applyLastOrDefaultConfig(subscription, *provider, filter_config_name);
return provider;
}

// If the subscription already received a config, attempt to apply it.
// It is possible that the received extension config fails to satisfy the listener
// type URL constraints. This may happen if ECDS and LDS updates are racing, and the LDS
// update arrives first. In this case, use the default config, increment a metric,
// and the applied config eventually converges once ECDS update arrives.
bool last_config_valid = false;
if (subscription->lastConfig().has_value()) {
TRY_ASSERT_MAIN_THREAD {
provider->validateTypeUrl(subscription->lastTypeUrl());
provider->validateTerminalFilter(filter_config_name, subscription->lastFilterName(),
subscription->isLastFilterTerminal());
last_config_valid = true;
}
END_TRY catch (const EnvoyException& e) {
ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.",
filter_config_name, e.what());
subscription->incrementConflictCounter();
}
if (last_config_valid) {
provider->onConfigUpdate(subscription->lastConfig().value(), subscription->lastVersionInfo(),
nullptr);
}
}

// Apply the default config if none has been applied.
if (!last_config_valid) {
provider->applyDefaultConfiguration();
Http::FilterFactoryCb HttpFilterConfigProviderManagerImpl::getDefaultConfig(
const ProtobufWkt::Any& proto_config, const std::string& filter_config_name,
Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix,
bool last_filter_in_filter_config, const std::string& filter_chain_type,
const absl::flat_hash_set<std::string> require_type_urls) const {
auto* default_factory =
Config::Utility::getFactoryByType<Server::Configuration::NamedHttpFilterConfigFactory>(
proto_config);
if (default_factory == nullptr) {
throw EnvoyException(fmt::format("Error: cannot find filter factory {} for default filter "
"configuration with type URL {}.",
filter_config_name, proto_config.type_url()));
}
return provider;
validateTypeUrlHelper(Config::Utility::getFactoryType(proto_config), require_type_urls);
ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig(
proto_config, factory_context.messageValidationVisitor(), *default_factory);
Config::Utility::validateTerminalFilters(
filter_config_name, default_factory->name(), filter_chain_type,
default_factory->isTerminalFilterByProto(*message, factory_context),
last_filter_in_filter_config);
return default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context);
}

} // namespace Filter
Expand Down
58 changes: 42 additions & 16 deletions source/common/filter/config_discovery_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
namespace Envoy {
namespace Filter {

class FilterConfigProviderManagerImpl;
class FilterConfigProviderManagerImplBase;
class FilterConfigSubscription;

using FilterConfigSubscriptionSharedPtr = std::shared_ptr<FilterConfigSubscription>;
Expand Down Expand Up @@ -161,7 +161,7 @@ class FilterConfigSubscription
const std::string& filter_config_name,
Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix,
FilterConfigProviderManagerImpl& filter_config_provider_manager,
FilterConfigProviderManagerImplBase& filter_config_provider_manager,
const std::string& subscription_id);

~FilterConfigSubscription() override;
Expand Down Expand Up @@ -204,8 +204,8 @@ class FilterConfigSubscription
const std::string stat_prefix_;
ExtensionConfigDiscoveryStats stats_;

// FilterConfigProviderManagerImpl maintains active subscriptions in a map.
FilterConfigProviderManagerImpl& filter_config_provider_manager_;
// FilterConfigProviderManagerImplBase maintains active subscriptions in a map.
FilterConfigProviderManagerImplBase& filter_config_provider_manager_;
const std::string subscription_id_;
absl::flat_hash_set<DynamicFilterConfigProviderImplBase*> filter_config_providers_;
friend class DynamicFilterConfigProviderImplBase;
Expand Down Expand Up @@ -233,17 +233,35 @@ class StaticFilterConfigProviderImpl : public FilterConfigProvider {
const std::string filter_config_name_;
};

/**
* Base class for a FilterConfigProviderManager.
*/
class FilterConfigProviderManagerImplBase : Logger::Loggable<Logger::Id::filter> {
protected:
std::shared_ptr<FilterConfigSubscription>
getSubscription(const envoy::config::core::v3::ConfigSource& config_source,
const std::string& name, Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix);
void applyLastOrDefaultConfig(std::shared_ptr<FilterConfigSubscription>& subscription,
DynamicFilterConfigProviderImplBase& provider,
const std::string& filter_config_name);

private:
absl::flat_hash_map<std::string, std::weak_ptr<FilterConfigSubscription>> subscriptions_;
friend class FilterConfigSubscription;
};

/**
* An implementation of FilterConfigProviderManager.
*/
class FilterConfigProviderManagerImpl : public FilterConfigProviderManager,
public Singleton::Instance,
Logger::Loggable<Logger::Id::filter> {
class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBase,
public FilterConfigProviderManager,
public Singleton::Instance {
public:
DynamicFilterConfigProviderPtr createDynamicFilterConfigProvider(
const envoy::config::core::v3::ExtensionConfigSource& config_source,
const std::string& filter_config_name, Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix, bool last_filter_in_filter_chain,
const std::string& stat_prefix, bool last_filter_in_filter_config,
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov Aug 30, 2021

Choose a reason for hiding this comment

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

I think we should keep the name last_filter_in_filter_chain. The condition is that only terminal filters are last in chains.

Copy link
Copy Markdown
Contributor Author

@tbarrella tbarrella Aug 30, 2021

Choose a reason for hiding this comment

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

Ah ok. I did it due to this since that code is going to be moved into the header. When I do that I can rename config -> chain from the code's body and elsewhere in the file as well

Copy link
Copy Markdown
Contributor

@snowp snowp Aug 31, 2021

Choose a reason for hiding this comment

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

So the plan is to handle this in a follow up or in this PR?

Copy link
Copy Markdown
Contributor Author

@tbarrella tbarrella Aug 31, 2021

Choose a reason for hiding this comment

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

A follow up. config is used in multiple other places too, not just here

Copy link
Copy Markdown
Contributor

@snowp snowp Aug 31, 2021

Choose a reason for hiding this comment

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

SGTM, let's merge this one then

const std::string& filter_chain_type) override;

FilterConfigProviderPtr
Expand All @@ -252,16 +270,24 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManager,
return std::make_unique<StaticFilterConfigProviderImpl>(config, filter_config_name);
}

private:
std::shared_ptr<FilterConfigSubscription>
getSubscription(const envoy::config::core::v3::ConfigSource& config_source,
const std::string& name, Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix);
absl::flat_hash_map<std::string, std::weak_ptr<FilterConfigSubscription>> subscriptions_;
friend class FilterConfigSubscription;
protected:
virtual Http::FilterFactoryCb
getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name,
Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix, bool last_filter_in_filter_config,
const std::string& filter_chain_type,
const absl::flat_hash_set<std::string> require_type_urls) const PURE;
};

class HttpFilterConfigProviderManagerImpl : public FilterConfigProviderManagerImpl {};
class HttpFilterConfigProviderManagerImpl : public FilterConfigProviderManagerImpl {
protected:
Http::FilterFactoryCb
getDefaultConfig(const ProtobufWkt::Any& proto_config, const std::string& filter_config_name,
Server::Configuration::FactoryContext& factory_context,
const std::string& stat_prefix, bool last_filter_in_filter_config,
const std::string& filter_chain_type,
const absl::flat_hash_set<std::string> require_type_urls) const override;
};

} // namespace Filter
} // namespace Envoy