Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a33db77
imporve doc
qinggniq Mar 30, 2021
e93cfb4
Update README.md
qinggniq Mar 30, 2021
418554b
fix typo
qinggniq Mar 31, 2021
ed05ce5
Merge branch 'main' of https://github.com/envoyproxy/envoy into main
qinggniq Apr 1, 2021
bcf50b2
change signature of isTermianlFilter
qinggniq Apr 1, 2021
e89113f
remove code
qinggniq Apr 1, 2021
3bebde5
cover more cases to validate config
qinggniq Apr 1, 2021
18bf6ed
fix test flakes
qinggniq Apr 1, 2021
3d81b83
fix build flakes
qinggniq Apr 2, 2021
55dba70
fix typo
qinggniq Apr 2, 2021
af0ecf2
fix build flakes
qinggniq Apr 2, 2021
7b2214d
fix: prevent Werror=overloaded-virtual warnig
qinggniq Apr 2, 2021
f5c5371
merge from master
qinggniq Apr 6, 2021
a6dee8f
fix test flakes
qinggniq Apr 6, 2021
0e56490
refactor the signature of isTerminalFilter
qinggniq Apr 6, 2021
e7075fa
fix extension_discovery test
qinggniq Apr 9, 2021
c98dd0e
Merge branch 'main' of https://github.com/envoyproxy/envoy into main
qinggniq Apr 9, 2021
3b3884e
fix extension_discovery_integration_test flakes
qinggniq Apr 9, 2021
123ac59
kick ci
qinggniq Apr 9, 2021
244ebb4
fix doc format
qinggniq Apr 9, 2021
b9dfe5c
Merge branch 'main' of https://github.com/envoyproxy/envoy into main
qinggniq Apr 13, 2021
d72f95f
fix review comment
qinggniq Apr 14, 2021
f3dde43
fix: envoy-filter-example sha
qinggniq Apr 15, 2021
dbff93b
fix: change envoy-filter-exmaple url
qinggniq Apr 15, 2021
953f9b2
Merge branch 'main' of https://github.com/envoyproxy/envoy into main
qinggniq Apr 15, 2021
ff8a04b
merge from upstream
qinggniq Apr 16, 2021
3f9afe6
fix: change sha to latest
qinggniq Apr 16, 2021
23aade7
fix: build flakes
qinggniq Apr 16, 2021
af1a9b2
fix: change envoy-filter-example sha to latest commit
qinggniq Apr 19, 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
2 changes: 1 addition & 1 deletion docs/root/intro/arch_overview/http/http_filters.rst
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ decoder/encoder filters):
- A
- B
# The last configured filter has to be a terminal filter, as determined by the
# NamedHttpFilterConfigFactory::isTerminalFilter() function. This is most likely the router
# NamedHttpFilterConfigFactory::isTerminalFilter(config, context) function. This is most likely the router
# filter.
- C

Expand Down
5 changes: 4 additions & 1 deletion include/envoy/filter/http/filter_config_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,14 @@ class FilterConfigProviderManager {
* @param filter_config_name the filter config resource name.
* @param factory_context is the context to use for the filter config provider.
* @param stat_prefix supplies the stat_prefix to use for the provider stats.
* @param last_filter_in_current_config indicate the filter position at filter chain

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe something like indicates whether this filter is the last filter in the configured chain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

* @param filter_chain_type is the filter chain type
*/
virtual FilterConfigProviderPtr 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) PURE;
const std::string& stat_prefix, bool last_filter_in_current_config,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe just last_filter? or last_filter_in_filter_chain? not sure what "current" is referring to

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

last_filter_in_filter_chain better.

const std::string& filter_chain_type) PURE;

/**
* Get an FilterConfigProviderPtr for a statically inlined filter config.
Expand Down
4 changes: 2 additions & 2 deletions include/envoy/server/filter_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class NamedNetworkFilterConfigFactory : public ProtocolOptionsFactory {
/**
* @return bool true if this filter must be the last filter in a filter chain, false otherwise.
*/
virtual bool isTerminalFilter() { return false; }
virtual bool isTerminalFilter(const Protobuf::Message&, FactoryContext&) { return false; }
};

/**
Expand Down Expand Up @@ -204,7 +204,7 @@ class NamedHttpFilterConfigFactory : public ProtocolOptionsFactory {
/**
* @return bool true if this filter must be the last filter in a filter chain, false otherwise.
*/
virtual bool isTerminalFilter() { return false; }
virtual bool isTerminalFilter(const Protobuf::Message&, FactoryContext&) { return false; }

/**
* @return FilterDependenciesPtr specification of dependencies required or
Expand Down
2 changes: 1 addition & 1 deletion source/common/config/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ class Utility {
* @throws EnvoyException if there is a mismatch between design and configuration.
*/
static void validateTerminalFilters(const std::string& name, const std::string& filter_type,
const char* filter_chain_type, bool is_terminal_filter,
const std::string& filter_chain_type, bool is_terminal_filter,
bool last_filter_in_current_config) {
if (is_terminal_filter && !last_filter_in_current_config) {
ExceptionUtil::throwEnvoyException(
Expand Down
55 changes: 42 additions & 13 deletions source/common/filter/http/filter_config_discovery_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,21 @@ namespace Http {
DynamicFilterConfigProviderImpl::DynamicFilterConfigProviderImpl(
FilterConfigSubscriptionSharedPtr& subscription,
const absl::flat_hash_set<std::string>& require_type_urls,
Server::Configuration::FactoryContext& factory_context)
Server::Configuration::FactoryContext& factory_context, bool last_filter_in_current_config,
const std::string& filter_chain_type)
: subscription_(subscription), require_type_urls_(require_type_urls),
tls_(factory_context.threadLocal()),
init_target_("DynamicFilterConfigProviderImpl", [this]() {
subscription_->start();
// This init target is used to activate the subscription but not wait
// for a response. It is used whenever a default config is provided to be
// used while waiting for a response.
init_target_.ready();
}) {
tls_(factory_context.threadLocal()), init_target_("DynamicFilterConfigProviderImpl",
[this]() {
subscription_->start();
// This init target is used to activate
// the subscription but not wait for a
// response. It is used whenever a default
// config is provided to be used while
// waiting for a response.
init_target_.ready();
}),
last_filter_in_current_config_(last_filter_in_current_config),
filter_chain_type_(filter_chain_type) {
subscription_->filter_config_providers_.insert(this);
tls_.set([](Event::Dispatcher&) { return std::make_shared<ThreadLocalConfig>(); });
}
Expand Down Expand Up @@ -64,6 +69,12 @@ void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryC
this->current_config_ = config;
});
}
void DynamicFilterConfigProviderImpl::validateTermianlFilter(const std::string& name,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo in name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

const std::string& filter_type,
bool is_terminal_filter) {
Config::Utility::validateTerminalFilters(name, filter_type, filter_chain_type_,
is_terminal_filter, last_filter_in_current_config_);
}

FilterConfigSubscription::FilterConfigSubscription(
const envoy::config::core::v3::ConfigSource& config_source,
Expand Down Expand Up @@ -106,7 +117,7 @@ void FilterConfigSubscription::onConfigUpdate(
throw EnvoyException(fmt::format(
"Unexpected number of resources in ExtensionConfigDS response: {}", resources.size()));
}
const auto& filter_config = dynamic_cast<const envoy::config::core::v3::TypedExtensionConfig&>(
auto& filter_config = dynamic_cast<const envoy::config::core::v3::TypedExtensionConfig&>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why remove the const here? we cast to a const ref

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sorry, did not notice.

resources[0].get().resource());
if (filter_config.name() != filter_config_name_) {
throw EnvoyException(fmt::format("Unexpected resource name in ExtensionConfigDS response: {}",
Expand All @@ -129,6 +140,11 @@ void FilterConfigSubscription::onConfigUpdate(
}
ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig(
filter_config.typed_config(), validator_, factory);

for (auto* provider : filter_config_providers_) {
provider->validateTermianlFilter(filter_config_name_, factory.name(),
factory.isTerminalFilter(*message, factory_context_));
}
Envoy::Http::FilterFactoryCb factory_callback =
factory.createFilterFactoryFromProto(*message, stat_prefix_, factory_context_);
ENVOY_LOG(debug, "Updating filter config {}", filter_config_name_);
Expand All @@ -145,6 +161,7 @@ void FilterConfigSubscription::onConfigUpdate(
last_config_ = factory_callback;
last_type_url_ = type_url;
last_version_info_ = version_info;
last_filter_config_ = filter_config;
}

void FilterConfigSubscription::onConfigUpdate(
Expand Down Expand Up @@ -203,7 +220,8 @@ std::shared_ptr<FilterConfigSubscription> FilterConfigProviderManagerImpl::getSu
FilterConfigProviderPtr FilterConfigProviderManagerImpl::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) {
const std::string& stat_prefix, bool last_filter_in_current_config,
const std::string& filter_chain_type) {
auto subscription = getSubscription(config_source.config_source(), filter_config_name,
factory_context, stat_prefix);
// For warming, wait until the subscription receives the first response to indicate readiness.
Expand All @@ -217,8 +235,9 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf
auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url);
require_type_urls.emplace(factory_type_url);
}
auto provider = std::make_unique<DynamicFilterConfigProviderImpl>(subscription, require_type_urls,
factory_context);
auto provider = std::make_unique<DynamicFilterConfigProviderImpl>(
subscription, require_type_urls, factory_context, last_filter_in_current_config,
filter_chain_type);
// Ensure the subscription starts if it has not already.
if (config_source.apply_default_config_without_warming()) {
factory_context.initManager().add(provider->init_target_);
Expand All @@ -233,6 +252,14 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf
if (subscription->lastConfig().has_value()) {
TRY_ASSERT_MAIN_THREAD {
provider->validateTypeUrl(subscription->lastTypeUrl());
auto& factory =
Config::Utility::getAndCheckFactory<Server::Configuration::NamedHttpFilterConfigFactory>(
subscription->lastFilterConfig());
auto message = Config::Utility::translateAnyToFactoryConfig(
subscription->lastFilterConfig().typed_config(),
factory_context.messageValidationVisitor(), factory);
provider->validateTermianlFilter(filter_config_name, factory.name(),
factory.isTerminalFilter(*message, factory_context));
last_config_valid = true;
}
END_TRY catch (const EnvoyException& e) {
Expand Down Expand Up @@ -261,6 +288,8 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf
ProtobufTypes::MessagePtr message = Config::Utility::translateAnyToFactoryConfig(
config_source.default_config(), factory_context.messageValidationVisitor(),
*default_factory);
provider->validateTermianlFilter(filter_config_name, default_factory->name(),
default_factory->isTerminalFilter(*message, factory_context));
Envoy::Http::FilterFactoryCb default_config =
default_factory->createFilterFactoryFromProto(*message, stat_prefix, factory_context);
provider->onConfigUpdate(default_config, "", nullptr);
Expand Down
16 changes: 13 additions & 3 deletions source/common/filter/http/filter_config_discovery_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,14 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider {
public:
DynamicFilterConfigProviderImpl(FilterConfigSubscriptionSharedPtr& subscription,
const absl::flat_hash_set<std::string>& require_type_urls,
Server::Configuration::FactoryContext& factory_context);
Server::Configuration::FactoryContext& factory_context,
bool last_filter_in_current_config,
const std::string& filter_chain_type);
~DynamicFilterConfigProviderImpl() override;

void validateTypeUrl(const std::string& type_url) const;

void validateTermianlFilter(const std::string& name, const std::string& filter_type,
bool is_terminal_filter);
// Config::ExtensionConfigProvider
const std::string& name() override;
absl::optional<Envoy::Http::FilterFactoryCb> config() override;
Expand All @@ -61,6 +64,8 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider {
// case no warming is requested by any other filter config provider.
Init::TargetImpl init_target_;

const bool last_filter_in_current_config_;
const std::string filter_chain_type_;
friend class FilterConfigProviderManagerImpl;
};

Expand Down Expand Up @@ -102,6 +107,9 @@ class FilterConfigSubscription
const absl::optional<Envoy::Http::FilterFactoryCb>& lastConfig() { return last_config_; }
const std::string& lastTypeUrl() { return last_type_url_; }
const std::string& lastVersionInfo() { return last_version_info_; }
const envoy::config::core::v3::TypedExtensionConfig& lastFilterConfig() {
return last_filter_config_;
}
void incrementConflictCounter();

private:
Expand All @@ -121,6 +129,7 @@ class FilterConfigSubscription
absl::optional<Envoy::Http::FilterFactoryCb> last_config_{absl::nullopt};
std::string last_type_url_;
std::string last_version_info_;
envoy::config::core::v3::TypedExtensionConfig last_filter_config_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we intentionally avoid storing the entire filter config, can we just track whether the last filter passed the terminal filter check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, we only need it_terminal_filter

Server::Configuration::FactoryContext& factory_context_;
ProtobufMessage::ValidationVisitor& validator_;

Expand Down Expand Up @@ -174,7 +183,8 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManager,
FilterConfigProviderPtr 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) override;
const std::string& stat_prefix, bool last_filter_in_current_config,
const std::string& filter_chain_type) override;

FilterConfigProviderPtr
createStaticFilterConfigProvider(const Envoy::Http::FilterFactoryCb& config,
Expand Down
10 changes: 10 additions & 0 deletions source/extensions/filters/http/common/factory_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,20 @@ class FactoryBase : public Server::Configuration::NamedHttpFilterConfigFactory {

std::string name() const override { return name_; }

bool isTerminalFilter(const Protobuf::Message& proto_config,
Server::Configuration::FactoryContext& context) override {
return isTerminalFilter(MessageUtil::downcastAndValidate<const ConfigProto&>(
proto_config, context.messageValidationVisitor()),
context);
}

protected:
FactoryBase(const std::string& name) : name_(name) {}

private:
virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) {
return false;
}
virtual Http::FilterFactoryCb
createFilterFactoryFromProtoTyped(const ConfigProto& proto_config,
const std::string& stats_prefix,
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/filters/http/router/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ class RouterFilterConfig
public:
RouterFilterConfig() : FactoryBase(HttpFilterNames::get().Router) {}

bool isTerminalFilter() override { return true; }
bool isTerminalFilter(const Protobuf::Message&, Server::Configuration::FactoryContext&) override {
return true;
}

private:
Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
Expand Down
11 changes: 10 additions & 1 deletion source/extensions/filters/network/common/factory_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,16 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor

std::string name() const override { return name_; }

bool isTerminalFilter() override { return is_terminal_filter_; }
bool isTerminalFilter(const Protobuf::Message& proto_config,
Server::Configuration::FactoryContext& context) override {
return isTerminalFilter(MessageUtil::downcastAndValidate<const ConfigProto&>(
proto_config, context.messageValidationVisitor()),
context);
}

virtual bool isTerminalFilter(const ConfigProto&, Server::Configuration::FactoryContext&) {
return is_terminal_filter_;
}

protected:
FactoryBase(const std::string& name, bool is_terminal = false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ class DirectResponseConfigFactory
};
}

bool isTerminalFilter() override { return true; }
bool isTerminalFilter(const envoy::extensions::filters::network::direct_response::v3::Config&,
Server::Configuration::FactoryContext&) override {
return true;
}
};

/**
Expand Down
5 changes: 4 additions & 1 deletion source/extensions/filters/network/echo/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ class EchoConfigFactory
};
}

bool isTerminalFilter() override { return true; }
bool isTerminalFilter(const envoy::extensions::filters::network::echo::v3::Echo&,
Server::Configuration::FactoryContext&) override {
return true;
}
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,8 +502,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
void HttpConnectionManagerConfig::processFilter(
const envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter&
proto_config,
int i, absl::string_view prefix, FilterFactoriesList& filter_factories,
const char* filter_chain_type, bool last_filter_in_current_config) {
int i, const std::string& prefix, FilterFactoriesList& filter_factories,
const std::string& filter_chain_type, bool last_filter_in_current_config) {
ENVOY_LOG(debug, " {} filter #{}", prefix, i);
if (proto_config.config_type_case() ==
envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter::ConfigTypeCase::
Expand All @@ -521,7 +521,7 @@ void HttpConnectionManagerConfig::processFilter(
proto_config, context_.messageValidationVisitor(), factory);
Http::FilterFactoryCb callback =
factory.createFilterFactoryFromProto(*message, stats_prefix_, context_);
bool is_terminal = factory.isTerminalFilter();
bool is_terminal = factory.isTerminalFilter(*message, context_);
Config::Utility::validateTerminalFilters(proto_config.name(), factory.name(), filter_chain_type,
is_terminal, last_filter_in_current_config);
auto filter_config_provider = filter_config_provider_manager_.createStaticFilterConfigProvider(
Expand All @@ -539,7 +539,7 @@ void HttpConnectionManagerConfig::processFilter(

void HttpConnectionManagerConfig::processDynamicFilterConfig(
const std::string& name, const envoy::config::core::v3::ExtensionConfigSource& config_discovery,
FilterFactoriesList& filter_factories, const char* filter_chain_type,
FilterFactoriesList& filter_factories, const std::string& filter_chain_type,
bool last_filter_in_current_config) {
ENVOY_LOG(debug, " dynamic filter name: {}", name);
if (config_discovery.apply_default_config_without_warming() &&
Expand All @@ -555,12 +555,10 @@ void HttpConnectionManagerConfig::processDynamicFilterConfig(
throw EnvoyException(
fmt::format("Error: no factory found for a required type URL {}.", factory_type_url));
}
Config::Utility::validateTerminalFilters(name, factory->name(), filter_chain_type,
factory->isTerminalFilter(),
last_filter_in_current_config);
}
auto filter_config_provider = filter_config_provider_manager_.createDynamicFilterConfigProvider(
config_discovery, name, context_, stats_prefix_);
config_discovery, name, context_, stats_prefix_, last_filter_in_current_config,
filter_chain_type);
filter_factories.push_back(std::move(filter_config_provider));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,13 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
void
processFilter(const envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter&
proto_config,
int i, absl::string_view prefix, FilterFactoriesList& filter_factories,
const char* filter_chain_type, bool last_filter_in_current_config);
int i, const std::string& prefix, FilterFactoriesList& filter_factories,
const std::string& filter_chain_type, bool last_filter_in_current_config);
void
processDynamicFilterConfig(const std::string& name,
const envoy::config::core::v3::ExtensionConfigSource& config_discovery,
FilterFactoriesList& filter_factories, const char* filter_chain_type,
FilterFactoriesList& filter_factories,
const std::string& filter_chain_type,
bool last_filter_in_current_config);
void createFilterChainForFactories(Http::FilterChainFactoryCallbacks& callbacks,
const FilterFactoriesList& filter_factories);
Expand Down
Loading