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

docs/root/intro/version_history.rst

@@ -5,6 +5,7 @@ Version history
 ================
 * admin: added ability to configure listener :ref:`socket options <envoy_api_field_config.bootstrap.v2.Admin.socket_options>`.
 * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump <envoy_api_msg_admin.v2alpha.SecretsConfigDump>`.
+* config: enforcing that terminal filters (e.g. HttpConnectionManager for L4, router for L7) be the last in their respective filter chains.
 * config: async data access for local and remote data source.
 * config: changed the default value of :ref:`initial_fetch_timeout <envoy_api_field_core.ConfigSource.initial_fetch_timeout>` from 0s to 15s. This is a change in behaviour in the sense that Envoy will move to the next initialization phase, even if the first config is not delivered in 15s. Refer to :ref:`initialization process <arch_overview_initialization>` for more details.
 * fault: added overrides for default runtime keys in :ref:`HTTPFault <envoy_api_msg_config.filter.http.fault.v2.HTTPFault>` filter.

include/envoy/server/filter_config.h

@@ -328,6 +328,11 @@ class NamedNetworkFilterConfigFactory : public ProtocolOptionsFactory {
    * produced by the factory.
    */
   virtual std::string name() PURE;
+
+  /**
+   * @return bool true if this filter must be the last filter in a filter chain, false otherwise.
+   */
+  virtual bool isTerminalFilter() { return false; }
 };
 
 /**
@@ -428,6 +433,11 @@ class NamedHttpFilterConfigFactory : public ProtocolOptionsFactory {
    * produced by the factory.
    */
   virtual std::string name() PURE;
+
+  /**
+   * @return bool true if this filter must be the last filter in a filter chain, false otherwise.
+   */
+  virtual bool isTerminalFilter() { return false; }
 };
 
 } // namespace Configuration

source/extensions/filters/http/router/config.h

@@ -26,6 +26,8 @@ class RouterFilterConfig
   createFilterFactory(const Json::Object& json_config, const std::string& stat_prefix,
                       Server::Configuration::FactoryContext& context) override;
 
+  bool isTerminalFilter() override { return true; }
+
 private:
   Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
       const envoy::config::filter::http::router::v2::Router& proto_config,

source/extensions/filters/network/common/factory_base.h

@@ -45,8 +45,11 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor
 
   std::string name() override { return name_; }
 
+  bool isTerminalFilter() override { return is_terminal_filter_; }
+
 protected:
-  FactoryBase(const std::string& name) : name_(name) {}
+  FactoryBase(const std::string& name, bool is_terminal = false)
+      : name_(name), is_terminal_filter_(is_terminal) {}
 
 private:
   virtual Network::FilterFactoryCb
@@ -59,6 +62,7 @@ class FactoryBase : public Server::Configuration::NamedNetworkFilterConfigFactor
   }
 
   const std::string name_;
+  const bool is_terminal_filter_;
 };
 
 } // namespace Common

source/extensions/filters/network/dubbo_proxy/config.h

@@ -24,7 +24,7 @@ class DubboProxyFilterConfigFactory
     : public Common::FactoryBase<
           envoy::config::filter::network::dubbo_proxy::v2alpha1::DubboProxy> {
 public:
-  DubboProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().DubboProxy) {}
+  DubboProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().DubboProxy, true) {}
 
 private:
   Network::FilterFactoryCb createFilterFactoryFromProtoTyped(

source/extensions/filters/network/echo/config.cc

@@ -35,6 +35,7 @@ class EchoConfigFactory : public Server::Configuration::NamedNetworkFilterConfig
   }
 
   std::string name() override { return NetworkFilterNames::get().Echo; }
+  bool isTerminalFilter() override { return true; }
 };
 
 /**

source/extensions/filters/network/http_connection_manager/config.cc

@@ -307,7 +307,15 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
 
   const auto& filters = config.http_filters();
   for (int32_t i = 0; i < filters.size(); i++) {
-    processFilter(filters[i], i, "http", filter_factories_);
+    bool is_terminal = false;
+    processFilter(filters[i], i, "http", filter_factories_, is_terminal);
+    if (is_terminal && i != filters.size() - 1) {
+      throw EnvoyException(fmt::format("Error: {} must be terminal filter.", filters[i].name()));
+    } else if (!is_terminal && i == filters.size() - 1) {
+      throw EnvoyException(
+          fmt::format("Error: non-terminal filter {} is the last filter in a http filter chain.",
+                      filters[i].name()));
+    }
   }
 
   for (const auto& upgrade_config : config.upgrade_configs()) {
@@ -321,7 +329,13 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
     if (!upgrade_config.filters().empty()) {
       std::unique_ptr<FilterFactoriesList> factories = std::make_unique<FilterFactoriesList>();
       for (int32_t i = 0; i < upgrade_config.filters().size(); i++) {
-        processFilter(upgrade_config.filters(i), i, name, *factories);
+        bool is_terminal = false;
+        processFilter(upgrade_config.filters(i), i, name, *factories, is_terminal);
+        if (is_terminal && i != upgrade_config.filters().size() - 1) {
+          throw EnvoyException(
+              fmt::format("Error: {} must be terminal filter in {} upgrade filter chain.",
+                          upgrade_config.filters(i).name(), name));
+        }
       }
       upgrade_filter_factories_.emplace(
           std::make_pair(name, FilterConfig{std::move(factories), enabled}));
@@ -335,7 +349,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
 
 void HttpConnectionManagerConfig::processFilter(
     const envoy::config::filter::network::http_connection_manager::v2::HttpFilter& proto_config,
-    int i, absl::string_view prefix, std::list<Http::FilterFactoryCb>& filter_factories) {
+    int i, absl::string_view prefix, std::list<Http::FilterFactoryCb>& filter_factories,
+    bool& is_terminal) {
   const std::string& string_name = proto_config.name();
 
   ENVOY_LOG(debug, "    {} filter #{}", prefix, i);
@@ -358,6 +373,7 @@ void HttpConnectionManagerConfig::processFilter(
         proto_config, context_.messageValidationVisitor(), factory);
     callback = factory.createFilterFactoryFromProto(*message, stats_prefix_, context_);
   }
+  is_terminal = factory.isTerminalFilter();
   filter_factories.push_back(callback);
 }
 

source/extensions/filters/network/http_connection_manager/config.h

@@ -33,7 +33,7 @@ class HttpConnectionManagerFilterConfigFactory
           envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager> {
 public:
   HttpConnectionManagerFilterConfigFactory()
-      : FactoryBase(NetworkFilterNames::get().HttpConnectionManager) {}
+      : FactoryBase(NetworkFilterNames::get().HttpConnectionManager, true) {}
 
   // NamedNetworkFilterConfigFactory
   Network::FilterFactoryCb
@@ -145,7 +145,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   enum class CodecType { HTTP1, HTTP2, AUTO };
   void processFilter(
       const envoy::config::filter::network::http_connection_manager::v2::HttpFilter& proto_config,
-      int i, absl::string_view prefix, FilterFactoriesList& filter_factories);
+      int i, absl::string_view prefix, FilterFactoriesList& filter_factories, bool& is_terminal);
 
   Server::Configuration::FactoryContext& context_;
   FilterFactoriesList filter_factories_;

source/extensions/filters/network/redis_proxy/config.h

@@ -41,7 +41,7 @@ class RedisProxyFilterConfigFactory
           envoy::config::filter::network::redis_proxy::v2::RedisProxy,
           envoy::config::filter::network::redis_proxy::v2::RedisProtocolOptions> {
 public:
-  RedisProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().RedisProxy) {}
+  RedisProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().RedisProxy, true) {}
 
   // NamedNetworkFilterConfigFactory
   Network::FilterFactoryCb

source/extensions/filters/network/tcp_proxy/config.h

@@ -16,7 +16,7 @@ namespace TcpProxy {
 class ConfigFactory
     : public Common::FactoryBase<envoy::config::filter::network::tcp_proxy::v2::TcpProxy> {
 public:
-  ConfigFactory() : FactoryBase(NetworkFilterNames::get().TcpProxy) {}
+  ConfigFactory() : FactoryBase(NetworkFilterNames::get().TcpProxy, true) {}
 
   // NamedNetworkFilterConfigFactory
   Network::FilterFactoryCb

source/extensions/filters/network/thrift_proxy/config.h

@@ -43,7 +43,7 @@ class ThriftProxyFilterConfigFactory
           envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy,
           envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProtocolOptions> {
 public:
-  ThriftProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().ThriftProxy) {}
+  ThriftProxyFilterConfigFactory() : FactoryBase(NetworkFilterNames::get().ThriftProxy, true) {}
 
 private:
   Network::FilterFactoryCb createFilterFactoryFromProtoTyped(

source/server/listener_manager_impl.cc

@@ -64,6 +64,16 @@ std::vector<Network::FilterFactoryCb> ProdListenerComponentFactory::createNetwor
     auto& factory =
         Config::Utility::getAndCheckFactory<Configuration::NamedNetworkFilterConfigFactory>(
             string_name);
+
+    if (factory.isTerminalFilter() && i != filters.size() - 1) {
+      throw EnvoyException(
+          fmt::format("Error: {} must be the terminal network filter.", filters[i].name()));
+    } else if (!factory.isTerminalFilter() && i == filters.size() - 1) {
+      throw EnvoyException(
+          fmt::format("Error: non-terminal filter {} is the last filter in a network filter chain.",
+                      filters[i].name()));
+    }
+
     Network::FilterFactoryCb callback;
     if (Config::Utility::allowDeprecatedV1Config(context.runtime(), *filter_config)) {
       callback = factory.createFilterFactory(*filter_config->getObject("value", true), context);

test/extensions/filters/network/dubbo_proxy/config_test.cc

@@ -62,6 +62,7 @@ TEST_F(DubboFilterConfigTest, ValidProtoConfiguration) {
   NiceMock<Server::Configuration::MockFactoryContext> context;
   DubboProxyFilterConfigFactory factory;
   Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context);
+  EXPECT_TRUE(factory.isTerminalFilter());
   Network::MockConnection connection;
   EXPECT_CALL(connection, addReadFilter(_));
   cb(connection);

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -146,6 +146,65 @@ 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, NonTerminalFilter) {
+  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.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: non-terminal filter envoy.health_check is the last filter in a http filter chain.");
+}
+
 TEST_F(HttpConnectionManagerConfigTest, MiscConfig) {
   const std::string yaml_string = R"EOF(
 codec_type: http1
@@ -526,7 +585,7 @@ stat_prefix: router
 http_filters:
 - name: envoy.http_dynamo_filter
   config: {}
-
+- name: envoy.router
   )EOF";
 
   auto proto_config = parseHttpConnectionManagerFromV2Yaml(yaml_string);
@@ -535,6 +594,7 @@ stat_prefix: router
   EXPECT_CALL(context_.thread_local_, allocateSlot());
   Network::FilterFactoryCb cb1 = factory.createFilterFactoryFromProto(proto_config, context_);
   Network::FilterFactoryCb cb2 = factory.createFilterFactoryFromProto(proto_config, context_);
+  EXPECT_TRUE(factory.isTerminalFilter());
 }
 
 TEST_F(HttpConnectionManagerConfigTest, BadHttpConnectionMangerConfig) {
@@ -785,13 +845,13 @@ TEST_F(FilterChainTest, createCustomUpgradeFilterChain) {
 
   auto foo_config = hcm_config.add_upgrade_configs();
   foo_config->set_upgrade_type("foo");
-  foo_config->add_filters()->ParseFromString("\n\fenvoy.router");
   foo_config->add_filters()->ParseFromString("\n"
                                              "\x18"
                                              "envoy.http_dynamo_filter");
   foo_config->add_filters()->ParseFromString("\n"
                                              "\x18"
                                              "envoy.http_dynamo_filter");
+  foo_config->add_filters()->ParseFromString("\n\fenvoy.router");
 
   HttpConnectionManagerConfig config(hcm_config, context_, date_provider_,
                                      route_config_provider_manager_,
@@ -818,6 +878,27 @@ TEST_F(FilterChainTest, createCustomUpgradeFilterChain) {
   }
 }
 
+TEST_F(FilterChainTest, createCustomUpgradeFilterChainWithRouterNotLast) {
+  auto hcm_config = parseHttpConnectionManagerFromV2Yaml(basic_config_);
+  auto websocket_config = hcm_config.add_upgrade_configs();
+  websocket_config->set_upgrade_type("websocket");
+
+  ASSERT_TRUE(websocket_config->add_filters()->ParseFromString("\n\fenvoy.router"));
+
+  auto foo_config = hcm_config.add_upgrade_configs();
+  foo_config->set_upgrade_type("foo");
+  foo_config->add_filters()->ParseFromString("\n\fenvoy.router");
+  foo_config->add_filters()->ParseFromString("\n"
+                                             "\x18"
+                                             "envoy.http_dynamo_filter");
+
+  EXPECT_THROW_WITH_MESSAGE(
+      HttpConnectionManagerConfig(hcm_config, context_, date_provider_,
+                                  route_config_provider_manager_,
+                                  scoped_routes_config_provider_manager_),
+      EnvoyException, "Error: envoy.router must be terminal filter in foo upgrade filter chain.");
+}
+
 TEST_F(FilterChainTest, invalidConfig) {
   auto hcm_config = parseHttpConnectionManagerFromV2Yaml(basic_config_);
   hcm_config.add_upgrade_configs()->set_upgrade_type("WEBSOCKET");

test/extensions/filters/network/rbac/BUILD

@@ -38,6 +38,7 @@ envoy_extension_cc_test(
     srcs = ["integration_test.cc"],
     extension_name = "envoy.filters.network.rbac",
     deps = [
+        "//source/extensions/filters/network/echo:config",
         "//source/extensions/filters/network/rbac:config",
         "//test/integration:integration_lib",
         "//test/test_common:environment_lib",

test/extensions/filters/network/rbac/integration_test.cc

@@ -37,6 +37,8 @@ class RoleBasedAccessControlNetworkFilterIntegrationTest
                   principals:
                     - not_id:
                         any: true
+       -  name: envoy.echo
+          config:
 )EOF";
   }
 

test/extensions/filters/network/redis_proxy/config_test.cc

@@ -74,6 +74,7 @@ stat_prefix: foo
   NiceMock<Server::Configuration::MockFactoryContext> context;
   RedisProxyFilterConfigFactory factory;
   Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context);
+  EXPECT_TRUE(factory.isTerminalFilter());
   Network::MockConnection connection;
   EXPECT_CALL(connection, addReadFilter(_));
   cb(connection);

test/extensions/filters/network/tcp_proxy/config_test.cc

@@ -81,6 +81,7 @@ TEST(ConfigTest, ConfigTest) {
   config.set_stat_prefix("prefix");
   config.set_cluster("cluster");
 
+  EXPECT_TRUE(factory.isTerminalFilter());
   Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context);
   Network::MockConnection connection;
   EXPECT_CALL(connection, addReadFilter(_));

test/extensions/filters/network/thrift_proxy/config_test.cc

@@ -56,6 +56,7 @@ class ThriftFilterConfigTestBase {
   void testConfig(envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy& config) {
     Network::FilterFactoryCb cb;
     EXPECT_NO_THROW({ cb = factory_.createFilterFactoryFromProto(config, context_); });
+    EXPECT_TRUE(factory_.isTerminalFilter());
 
     Network::MockConnection connection;
     EXPECT_CALL(connection, addReadFilter(_));

test/integration/tcp_conn_pool_integration_test.cc

@@ -107,6 +107,7 @@ class TestFilterConfigFactory : public Server::Configuration::NamedNetworkFilter
   }
 
   std::string name() override { CONSTRUCT_ON_FIRST_USE(std::string, "envoy.test.router"); }
+  bool isTerminalFilter() override { return true; }
 };
 
 } // namespace

test/server/BUILD

@@ -176,6 +176,7 @@ envoy_cc_test(
         "//source/extensions/filters/listener/original_dst:config",
         "//source/extensions/filters/listener/tls_inspector:config",
         "//source/extensions/filters/network/http_connection_manager:config",
+        "//source/extensions/filters/network/tcp_proxy:config",
         "//source/extensions/transport_sockets/raw_buffer:config",
         "//source/extensions/transport_sockets/tls:config",
         "//source/extensions/transport_sockets/tls:ssl_socket_lib",