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

docs/root/intro/version_history.rst

@@ -6,6 +6,7 @@ Version history
 * access log: added :ref:`buffering <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_size_bytes>` and :ref:`periodical flushing <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_flush_interval>` support to gRPC access logger. Defaults to 16KB buffer and flushing every 1 second.
 * 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.
 * buffer filter: the buffer filter populates content-length header if not present, behavior can be disabled using the runtime feature `envoy.reloadable_features.buffer_filter_populate_content_length`.
 * config: added access log :ref:`extension filter<envoy_api_field_config.filter.accesslog.v2.AccessLogFilter.extension_filter>`.
 * config: async data access for local and remote data source.

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/common/config/utility.h

@@ -291,6 +291,26 @@ class Utility {
    * Return whether v1-style JSON filter config loading is allowed via 'deprecated_v1: true'.
    */
   static bool allowDeprecatedV1Config(Runtime::Loader& runtime, const Json::Object& config);
+
+  /**
+   * Verify any any filter designed to be terminal is configured to be terminal, and vice versa.
+   * @param name the name of the filter.
+   * @param name the type of filter.
+   * @param is_terminal_filter true if the filter is designed to be terminal.
+   * @param last_filter_in_current_config true if the filter is last in the configuration.
+   * @throws EnvoyException if there is a mismatch between design and configuration.
+   */
+  static void validateTerminalFilters(const std::string& name, const char* filter_type,
+                                      bool is_terminal_filter, bool last_filter_in_current_config) {
+    if (is_terminal_filter && !last_filter_in_current_config) {
+      throw EnvoyException(
+          fmt::format("Error: {} must be the terminal {} filter.", name, filter_type));
+    } else if (!is_terminal_filter && last_filter_in_current_config) {
+      throw EnvoyException(
+          fmt::format("Error: non-terminal filter {} is the last filter in a {} filter chain.",
+                      name, filter_type));
+    }
+  }
 };
 
 } // namespace Config

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,10 @@ 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);
+    Config::Utility::validateTerminalFilters(filters[i].name(), "http", is_terminal,
+                                             i == filters.size() - 1);
   }
 
   for (const auto& upgrade_config : config.upgrade_configs()) {
@@ -320,8 +323,12 @@ 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);
+      for (int32_t j = 0; j < upgrade_config.filters().size(); j++) {
+        bool is_terminal = false;
+        processFilter(upgrade_config.filters(j), j, name, *factories, is_terminal);
+        Config::Utility::validateTerminalFilters(upgrade_config.filters(j).name(), "http upgrade",
+                                                 is_terminal,
+                                                 j == upgrade_config.filters().size() - 1);
       }
       upgrade_filter_factories_.emplace(
           std::make_pair(name, FilterConfig{std::move(factories), enabled}));
@@ -335,7 +342,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 +366,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,10 @@ std::vector<Network::FilterFactoryCb> ProdListenerComponentFactory::createNetwor
     auto& factory =
         Config::Utility::getAndCheckFactory<Configuration::NamedNetworkFilterConfigFactory>(
             string_name);
+
+    Config::Utility::validateTerminalFilters(filters[i].name(), "network",
+                                             factory.isTerminalFilter(), i == filters.size() - 1);
+
     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 the terminal http 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 the terminal http upgrade filter.");
+}
+
 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",