config: enforcing terminal filters are the final filter in their resp…

…ective chains (envoyproxy#7779) An (no longer annoyingly one-off) solution to the common problem folks run into with Envoy configs where they add their filters behind the router filter and don't get why things aren't working. Ditto for HCM, tcp_proxy etc for L4 Risk Level: Low (except for folks with broken config) Testing: new UT Docs Changes: n/a Release Notes: n/a Fixes envoyproxy#7767 Signed-off-by: Alyssa Wilk <[email protected]>
LK4D4 · Aug 13, 2019 · 3f5580f · 3f5580f
1 parent f2129cb
commit 3f5580f
Show file tree

Hide file tree

Showing 23 changed files with 217 additions and 13 deletions.
diff --git a/docs/root/intro/version_history.rst b/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.

diff --git a/include/envoy/server/filter_config.h b/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

diff --git a/source/common/config/utility.h b/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

diff --git a/source/extensions/filters/http/router/config.h b/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,

diff --git a/source/extensions/filters/network/common/factory_base.h b/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

diff --git a/source/extensions/filters/network/dubbo_proxy/config.h b/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(

diff --git a/source/extensions/filters/network/echo/config.cc b/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; }
 };
 
 /**

diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/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);
 }
 

diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/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_;

diff --git a/source/extensions/filters/network/redis_proxy/config.h b/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

diff --git a/source/extensions/filters/network/tcp_proxy/config.h b/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

diff --git a/source/extensions/filters/network/thrift_proxy/config.h b/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(

diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc
@@ -64,6 +64,13 @@ std::vector<Network::FilterFactoryCb> ProdListenerComponentFactory::createNetwor
     auto& factory =
         Config::Utility::getAndCheckFactory<Configuration::NamedNetworkFilterConfigFactory>(
             string_name);
+
+    // TODO(alyssar) replace this block and reinstate TerminalNotLast test once echo2 is updated
+    if (factory.isTerminalFilter() && i != filters.size() - 1) {
+      throw EnvoyException(
+          fmt::format("Error: {} must be the terminal network filter.", filters[i].name()));
+    }
+
     Network::FilterFactoryCb callback;
     if (Config::Utility::allowDeprecatedV1Config(context.runtime(), *filter_config)) {
       callback = factory.createFilterFactory(*filter_config->getObject("value", true), context);

diff --git a/test/extensions/filters/network/dubbo_proxy/config_test.cc b/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);

diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/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");

diff --git a/test/extensions/filters/network/rbac/BUILD b/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",

diff --git a/test/extensions/filters/network/rbac/integration_test.cc b/test/extensions/filters/network/rbac/integration_test.cc
@@ -37,6 +37,8 @@ class RoleBasedAccessControlNetworkFilterIntegrationTest
                   principals:
                     - not_id:
                         any: true
+       -  name: envoy.echo
+          config:
 )EOF";
   }
 

diff --git a/test/extensions/filters/network/redis_proxy/config_test.cc b/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);

diff --git a/test/extensions/filters/network/tcp_proxy/config_test.cc b/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(_));

diff --git a/test/extensions/filters/network/thrift_proxy/config_test.cc b/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(_));

diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/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
-Original file line number
+Diff line change
@@ Expand Up @@
       }
       std::string name() override { return NetworkFilterNames::get().Echo; }
+      bool isTerminalFilter() override { return true; }
     };
     /**
@@ Expand Down @@