common: added a field in HCM proto to be able to reverse the order of…

… HTTP encoder filters. (#4721) Added a field in HCM proto to be able to reverse the order of HTTP encoder filters. The field is set false by default, indicating HTTP encoder filters have the same order as configured in the filter chain. If true, their order will be reversed. Risk Level: low Testing: bazel test //test/... Part of #4599 Signed-off-by: Qi (Anna) Wang <[email protected]>
envoyproxy · Oct 18, 2018 · 0ccc70a · 0ccc70a
1 parent de03926
commit 0ccc70a
Show file tree

Hide file tree

Showing 10 changed files with 213 additions and 53 deletions.
diff --git a/DEPRECATED.md b/DEPRECATED.md
@@ -8,6 +8,8 @@ A logged warning is expected for each deprecated item that is in deprecation win
 
 ## Version 1.9.0 (pending)
 
+* Order of execution of the encoder filter chain has been reversed. Prior to this release cycle it was incorrect, see [#4599](https://github.com/envoyproxy/envoy/issues/4599). In the 1.9.0 release cycle we introduced `bugfix_reverse_encode_order` in [http_connection_manager.proto] (https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto) to temporarily support both old and new behaviors. Note this boolean field is deprecated.
+
 ## Version 1.8.0 (Oct 4, 2018)
 
 * Use of the v1 API (including `*.deprecated_v1` fields in the v2 API) is deprecated.

diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
@@ -19,7 +19,7 @@ import "gogoproto/gogo.proto";
 // [#protodoc-title: HTTP connection manager]
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 
-// [#comment:next free field: 27]
+// [#comment:next free field: 28]
 message HttpConnectionManager {
   enum CodecType {
     option (gogoproto.goproto_enum_prefix) = false;
@@ -348,6 +348,15 @@ message HttpConnectionManager {
     repeated HttpFilter filters = 2;
   };
   repeated UpgradeConfig upgrade_configs = 23;
+
+  // If true, the order of encoder filters will be reversed to that of filters
+  // configured in the HTTP filter chain. Otherwise, it will keep the existing
+  // order.
+  // Note: this is a bug fix for Envoy, which is designed to have the reversed
+  // order of encode filters to that of decode ones, (see
+  // https://github.com/envoyproxy/envoy/issues/4599 for details). When we remove this field, envoy
+  // will have the same behavior when it sets true.
+  google.protobuf.BoolValue bugfix_reverse_encode_order = 27 [deprecated = true];
 }
 
 message Rds {

diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h
@@ -209,6 +209,12 @@ class ConnectionManagerConfig {
    */
   virtual FilterChainFactory& filterFactory() PURE;
 
+  /**
+   * @return whether the connection manager will reverse the order of encoder
+   * filters in the HTTP filter chain.
+   */
+  virtual bool reverseEncodeOrder() PURE;
+
   /**
    * @return whether the connection manager will generate a fresh x-request-id if the request does
    *         not have one.

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -458,7 +458,11 @@ void ConnectionManagerImpl::ActiveStream::addStreamEncoderFilterWorker(
     StreamEncoderFilterSharedPtr filter, bool dual_filter) {
   ActiveStreamEncoderFilterPtr wrapper(new ActiveStreamEncoderFilter(*this, filter, dual_filter));
   filter->setEncoderFilterCallbacks(*wrapper);
-  wrapper->moveIntoListBack(std::move(wrapper), encoder_filters_);
+  if (connection_manager_.config_.reverseEncodeOrder()) {
+    wrapper->moveIntoList(std::move(wrapper), encoder_filters_);
+  } else {
+    wrapper->moveIntoListBack(std::move(wrapper), encoder_filters_);
+  }
 }
 
 void ConnectionManagerImpl::ActiveStream::addAccessLogHandler(

diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc
@@ -134,7 +134,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
         config,
     Server::Configuration::FactoryContext& context, Http::DateProvider& date_provider,
     Router::RouteConfigProviderManager& route_config_provider_manager)
-    : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())),
+    : context_(context), reverse_encode_order_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(
+                             config, bugfix_reverse_encode_order, false)),
+      stats_prefix_(fmt::format("http.{}.", config.stat_prefix())),
       stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())),
       tracing_stats_(
           Http::ConnectionManagerImpl::generateTracingStats(stats_prefix_, context_.scope())),

diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h
@@ -107,6 +107,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   Http::DateProvider& dateProvider() override { return date_provider_; }
   std::chrono::milliseconds drainTimeout() override { return drain_timeout_; }
   FilterChainFactory& filterFactory() override { return *this; }
+  bool reverseEncodeOrder() override { return reverse_encode_order_; }
   bool generateRequestId() override { return generate_request_id_; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
@@ -145,6 +146,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   Server::Configuration::FactoryContext& context_;
   FilterFactoriesList filter_factories_;
   std::map<std::string, std::unique_ptr<FilterFactoriesList>> upgrade_filter_factories_;
+  const bool reverse_encode_order_{};
   std::list<AccessLog::InstanceSharedPtr> access_logs_;
   const std::string stats_prefix_;
   Http::ConnectionManagerStats stats_;

diff --git a/source/server/http/admin.h b/source/server/http/admin.h
@@ -96,6 +96,7 @@ class AdminImpl : public Admin,
   Http::DateProvider& dateProvider() override { return date_provider_; }
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   Http::FilterChainFactory& filterFactory() override { return *this; }
+  bool reverseEncodeOrder() override { return false; }
   bool generateRequestId() override { return false; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return {}; }

diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc
@@ -88,6 +88,7 @@ class FuzzConfig : public ConnectionManagerConfig {
   DateProvider& dateProvider() override { return date_provider_; }
   std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); }
   FilterChainFactory& filterFactory() override { return filter_factory_; }
+  bool reverseEncodeOrder() override { return true; }
   bool generateRequestId() override { return true; }
   absl::optional<std::chrono::milliseconds> idleTimeout() const override { return idle_timeout_; }
   std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }