Skip to content

config: enforcing terminal filters are the final filter in their respective chains #7779

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
alyssawilk merged 9 commits into from
Aug 13, 2019
Merged

config: enforcing terminal filters are the final filter in their respective chains #7779

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6fd5120
hcm: enforcing the router being the final filter in a chain
alyssawilk Jul 31, 2019
2681f7a
reviewer comments
alyssawilk Jul 31, 2019
756e84b
slipped off the slope directly into yak shaving
alyssawilk Jul 31, 2019
da16f3e
Merge branch 'master' into router_is_last
alyssawilk Aug 12, 2019
aa6a58a
terminal iff last
alyssawilk Aug 12, 2019
a8c6957
and new unit tests
alyssawilk Aug 12, 2019
f7cb281
Merge branch 'refs/heads/master' into router_is_last
alyssawilk Aug 12, 2019
15c3a81
utility
alyssawilk Aug 12, 2019
4bbc8c3
doing the repo dance
alyssawilk Aug 12, 2019
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
1 change: 1 addition & 0 deletions source/extensions/filters/network/http_connection_manager/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ envoy_cc_library(
"//source/common/protobuf:utility_lib",
"//source/common/router:rds_lib",
"//source/common/router:scoped_rds_lib",
"//source/extensions/filters/http:well_known_names",
"//source/extensions/filters/network:well_known_names",
"//source/extensions/filters/network/common:factory_base_lib",
],
Expand Down
6 changes: 6 additions & 0 deletions source/extensions/filters/network/http_connection_manager/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include "common/router/rds_impl.h"
#include "common/router/scoped_rds.h"

#include "extensions/filters/http/well_known_names.h"

namespace Envoy {
namespace Extensions {
namespace NetworkFilters {
Expand Down Expand Up @@ -306,6 +308,10 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(

const auto& filters = config.http_filters();
for (int32_t i = 0; i < filters.size(); i++) {
if (filters[i].name() == HttpFilters::HttpFilterNames::get().Router &&
i != filters.size() - 1) {
throw EnvoyException(fmt::format("Error: envoy.router must be terminal filter."));
}
processFilter(filters[i], i, "http", filter_factories_);
}

Expand Down
33 changes: 33 additions & 0 deletions test/extensions/filters/network/http_connection_manager/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,36 @@ stat_prefix: router
EnvoyException, "Didn't find a registered implementation for name: 'foo'");
}

TEST_F(HttpConnectionManagerConfigTest, RouterInverted) {
const std::string yaml_string = R"EOF(
codec_type: http1
server_name: foo
stat_prefix: router
route_config:
virtual_hosts:
- name: service
domains:
- "*"
routes:
- match:
prefix: "/"
route:
cluster: cluster
http_filters:
- name: envoy.router
config: {}
- name: envoy.health_check
config:
pass_through_mode: false
)EOF";

EXPECT_THROW_WITH_MESSAGE(
HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
date_provider_, route_config_provider_manager_,
scoped_routes_config_provider_manager_),
EnvoyException, "Error: envoy.router must be terminal filter.");
}

TEST_F(HttpConnectionManagerConfigTest, MiscConfig) {
const std::string yaml_string = R"EOF(
codec_type: http1
Expand All @@ -166,6 +196,9 @@ stat_prefix: router
request_headers_for_tags:
- foo
http_filters:
- name: envoy.health_check

@mattklein123 mattklein123 Jul 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this change needed?

@alyssawilk alyssawilk Jul 31, 2019

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.

Not needed - I just wanted the comparable test to have 2 filters in the right order to verify that worked.
Can revert if you'd prefer - obviously we have enough integration tests of multiple filters

@mattklein123 mattklein123 Jul 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine either way. I guess I have a mild preference to revert but I don't feel strongly about it.

config:
pass_through_mode: false
- name: envoy.router
config: {}
)EOF";
Expand Down