Skip to content
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

listener: allow setting only a default filter chain #14025

Merged
merged 6 commits into from
Nov 19, 2020
Merged

listener: allow setting only a default filter chain #14025

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
26a28f1
listener: allow setting only a default filter chain
tbarrella Nov 14, 2020
f611de6
Merge remote-tracking branch 'origin/master' into filter-chain
tbarrella Nov 16, 2020
7d3ed36
Address comments
tbarrella Nov 16, 2020
74ed282
Merge remote-tracking branch 'origin/master' into filter-chain
tbarrella Nov 17, 2020
e36ab02
Merge remote-tracking branch 'origin/master' into filter-chain
tbarrella Nov 17, 2020
dda71bb
Adjust condition and add comment
tbarrella Nov 19, 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
55 changes: 35 additions & 20 deletions source/server/listener_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,24 @@ namespace Envoy {
namespace Server {

namespace {
bool anyFilterChain(
const envoy::config::listener::v3::Listener& config,
std::function<bool(const envoy::config::listener::v3::FilterChain&)> predicate) {
if (config.has_default_filter_chain() && predicate(config.default_filter_chain())) {
return true;
}
return std::any_of(config.filter_chains().begin(), config.filter_chains().end(), predicate);
tbarrella marked this conversation as resolved.
Show resolved Hide resolved
}

bool needTlsInspector(const envoy::config::listener::v3::Listener& config) {
return std::any_of(config.filter_chains().begin(), config.filter_chains().end(),
[](const auto& filter_chain) {
const auto& matcher = filter_chain.filter_chain_match();
return matcher.transport_protocol() == "tls" ||
(matcher.transport_protocol().empty() &&
(!matcher.server_names().empty() ||
!matcher.application_protocols().empty()));
}) &&
return anyFilterChain(config,
[](const auto& filter_chain) {
const auto& matcher = filter_chain.filter_chain_match();
return matcher.transport_protocol() == "tls" ||
(matcher.transport_protocol().empty() &&
(!matcher.server_names().empty() ||
!matcher.application_protocols().empty()));
}) &&
!std::any_of(
config.listener_filters().begin(), config.listener_filters().end(),
[](const auto& filter) {
Expand All @@ -55,6 +64,12 @@ bool needTlsInspector(const envoy::config::listener::v3::Listener& config) {
filter.name() == "envoy.listener.tls_inspector";
});
}

bool usesProxyProto(const envoy::config::listener::v3::Listener& config) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to look like needTlsInspector with a search through all filter chains? I don't understand how you can only check the default or the first chain? Was this a bug that already existed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems this is the only file that checks use_proxy_proto, so maybe it was a bug/undocumented expectation. Not sure if the previous behavior should be modified in this change. I just realized it might be more conservative to change the condition from config.has_default_filter_chain() to config.filter_chains().empty() though

Copy link
Member

Choose a reason for hiding this comment

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

To me this is a bug as I don't think it makes any sense. If we don't want to fix this now I would make no changes here and then fix it in a follow up?

If we auto inject proxy proto I think all filter chains should probably be asking for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I'm reluctant to change two behaviors in one commit; opened #14085 for now

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattklein123 It's an invalid config if partial of the filter chain set use proxy proto while other filter chain don't. We check the first filter chain here for back compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no enforcement at this moment. And don't add enforcement in this PR.
However, could you add the warning message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add a printed warning since deprecation should serve that purpose, but I added a comment referencing #14085

Copy link
Member

Choose a reason for hiding this comment

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

Given that none of this make any sense, do you want to just revert this part of the PR entirely? Also do we want a warning in a PR or do you want to do this as a follow up? Otherwise this change LGTM and thanks for the discussion here.

/wait-any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still needed to avoid an indexing error when only a default filter chain is provided. I'd rather leave user-facing warnings etc. as a follow-up (part of the other issue) since this is consistent with existing behavior/isn't a regression or anything

Copy link
Member

Choose a reason for hiding this comment

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

OK fair enough. If you can fix all of ^ as a follow up that would be appreciated.

return PROTOBUF_GET_WRAPPED_OR_DEFAULT(
config.has_default_filter_chain() ? config.default_filter_chain() : config.filter_chains()[0],
use_proxy_proto, false);
}
} // namespace

ListenSocketFactoryImpl::ListenSocketFactoryImpl(ListenerComponentFactory& factory,
Expand Down Expand Up @@ -458,21 +473,22 @@ void ListenerImpl::createListenerFilterFactories(Network::Socket::Type socket_ty
}

void ListenerImpl::validateFilterChains(Network::Socket::Type socket_type) {
if (config_.filter_chains().empty() && (socket_type == Network::Socket::Type::Stream ||
!udp_listener_factory_->isTransportConnectionless())) {
if (config_.filter_chains().empty() && !config_.has_default_filter_chain() &&
(socket_type == Network::Socket::Type::Stream ||
!udp_listener_factory_->isTransportConnectionless())) {
// If we got here, this is a tcp listener or connection-oriented udp listener, so ensure there
// is a filter chain specified
throw EnvoyException(fmt::format("error adding listener '{}': no filter chains specified",
address_->asString()));
} else if (udp_listener_factory_ != nullptr &&
!udp_listener_factory_->isTransportConnectionless()) {
for (auto& filter_chain : config_.filter_chains()) {
// Early fail if any filter chain doesn't have transport socket configured.
if (!filter_chain.has_transport_socket()) {
throw EnvoyException(fmt::format("error adding listener '{}': no transport socket "
"specified for connection oriented UDP listener",
address_->asString()));
}
// Early fail if any filter chain doesn't have transport socket configured.
if (anyFilterChain(config_, [](const auto& filter_chain) {
return !filter_chain.has_transport_socket();
})) {
throw EnvoyException(fmt::format("error adding listener '{}': no transport socket "
"specified for connection oriented UDP listener",
address_->asString()));
}
}
}
Expand Down Expand Up @@ -528,7 +544,7 @@ void ListenerImpl::buildProxyProtocolListenerFilter() {
// TODO(jrajahalme): This is the last listener filter on purpose. When filter chain matching
// is implemented, this needs to be run after the filter chain has been
// selected.
if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config_.filter_chains()[0], use_proxy_proto, false)) {
if (usesProxyProto(config_)) {
auto& factory =
Config::Utility::getAndCheckFactoryByName<Configuration::NamedListenerFilterConfigFactory>(
Extensions::ListenerFilters::ListenerFilterNames::get().ProxyProtocol);
Expand Down Expand Up @@ -714,8 +730,7 @@ bool ListenerImpl::supportUpdateFilterChain(const envoy::config::listener::v3::L

// See buildProxyProtocolListenerFilter(). Full listener update guarantees at least 1 filter chain
// at tcp listener.
tbarrella marked this conversation as resolved.
Show resolved Hide resolved
if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config_.filter_chains()[0], use_proxy_proto, false) ^
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.filter_chains()[0], use_proxy_proto, false)) {
if (usesProxyProto(config_) ^ usesProxyProto(config)) {
return false;
}

Expand Down
14 changes: 14 additions & 0 deletions test/server/listener_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,20 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, UdpAddress) {
EXPECT_EQ(1u, manager_->listeners().size());
}

TEST_F(ListenerManagerImplWithRealFiltersTest, AllowOnlyDefaultFilterChain) {
const std::string yaml = R"EOF(
address:
socket_address:
address: 127.0.0.1
port_value: 1234
default_filter_chain:
filters: []
)EOF";

manager_->addOrUpdateListener(parseListenerFromV3Yaml(yaml), "", true);
EXPECT_EQ(1, manager_->listeners().size());
}

TEST_F(ListenerManagerImplWithRealFiltersTest, BadListenerConfig) {
const std::string yaml = R"EOF(
address:
Expand Down