Revert "[gzip]: allow gzip to work w/ http2 backend w/o content-lengt… (

#14567) Revert "[gzip]: allow gzip to work w/ http2 backend w/o content-length (#14405)" This reverts commit 0c9e7bf. It breaks web sockets because it compresses payload on upgrade responses (when apparently it shouldn't). Risk: low (reinstating prior behavior) Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
envoyproxy · Jan 6, 2021 · 3d3b42e · 3d3b42e
1 parent b561a52
commit 3d3b42e
Show file tree

Hide file tree

Showing 5 changed files with 9 additions and 101 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -64,7 +64,6 @@ Removed Config or Runtime
 
 New Features
 ------------
-* compression: added new runtime guard `envoy.reloadable_features.enable_compression_without_chunked_header` (enabled by default) to control compression if no transfer-encoding=chunked header exists (now envoy would compress in such cases) to account for http/2 connections, where such transfer-encoding were removed.
 * compression: the :ref:`compressor <envoy_v3_api_msg_extensions.filters.http.compressor.v3.Compressor>` filter adds support for compressing request payloads. Its configuration is unified with the :ref:`decompressor <envoy_v3_api_msg_extensions.filters.http.decompressor.v3.Decompressor>` filter with two new fields for different directions - :ref:`requests <envoy_v3_api_field_extensions.filters.http.compressor.v3.Compressor.request_direction_config>` and :ref:`responses <envoy_v3_api_field_extensions.filters.http.compressor.v3.Compressor.response_direction_config>`. The latter deprecates the old response-specific fields and, if used, roots the response-specific stats in `<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.response.*` instead of `<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.*`.
 * config: added ability to flush stats when the admin's :ref:`/stats endpoint <operations_admin_interface_stats>` is hit instead of on a timer via :ref:`stats_flush_on_admin <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.stats_flush_on_admin>`.
 * config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features.

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -66,7 +66,6 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.disable_tls_inspector_injection",
     "envoy.reloadable_features.disallow_unbounded_access_logs",
     "envoy.reloadable_features.early_errors_via_hcm",
-    "envoy.reloadable_features.enable_compression_without_chunked_header",
     "envoy.reloadable_features.enable_dns_cache_circuit_breakers",
     "envoy.reloadable_features.fix_upgrade_response",
     "envoy.reloadable_features.fix_wildcard_matching",

diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc
@@ -495,10 +495,6 @@ bool CompressorFilter::isEtagAllowed(Http::ResponseHeaderMap& headers) const {
 
 bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength(
     const Http::RequestOrResponseHeaderMap& headers) const {
-  if (StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",",
-                                Http::Headers::get().TransferEncodingValues.Chunked)) {
-    return true;
-  }
   const Http::HeaderEntry* content_length = headers.ContentLength();
   if (content_length != nullptr) {
     uint64_t length;
@@ -510,12 +506,9 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength(
     }
     return is_minimum_content_length;
   }
-  if (Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.enable_compression_without_chunked_header")) {
-    // returning true to account for HTTP/2 where content-length is optional
-    return true;
-  }
-  return false;
+
+  return StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",",
+                                   Http::Headers::get().TransferEncodingValues.Chunked);
 }
 
 bool CompressorFilter::isTransferEncodingAllowed(Http::RequestOrResponseHeaderMap& headers) const {

diff --git a/test/extensions/filters/http/common/compressor/BUILD b/test/extensions/filters/http/common/compressor/BUILD
@@ -21,7 +21,6 @@ envoy_cc_test(
         "//test/mocks/http:http_mocks",
         "//test/mocks/protobuf:protobuf_mocks",
         "//test/mocks/runtime:runtime_mocks",
-        "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/extensions/filters/http/compressor/v3:pkg_cc_proto",
     ],

diff --git a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc
@@ -11,7 +11,6 @@
 #include "test/mocks/protobuf/mocks.h"
 #include "test/mocks/runtime/mocks.h"
 #include "test/mocks/stats/mocks.h"
-#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "gtest/gtest.h"
@@ -78,14 +77,8 @@ class CompressorFilterTest : public testing::Test {
   }
 
   void verifyCompressedData() {
-    EXPECT_EQ(
-        expected_str_.length(),
-        stats_.counter(fmt::format("test.test.{}total_uncompressed_bytes", response_stats_prefix_))
-            .value());
-    EXPECT_EQ(
-        data_.length(),
-        stats_.counter(fmt::format("test.test.{}total_compressed_bytes", response_stats_prefix_))
-            .value());
+    EXPECT_EQ(expected_str_.length(), stats_.counter("test.test.total_uncompressed_bytes").value());
+    EXPECT_EQ(data_.length(), stats_.counter("test.test.total_compressed_bytes").value());
   }
 
   void populateBuffer(uint64_t size) {
@@ -139,6 +132,9 @@ class CompressorFilterTest : public testing::Test {
                   bool with_trailers) {
     uint64_t buffer_content_size;
     if (!absl::SimpleAtoi(headers.get_("content-length"), &buffer_content_size)) {
+      ASSERT_TRUE(
+          StringUtil::CaseInsensitiveCompare()(headers.get_("transfer-encoding"), "chunked"));
+      // In case of chunked stream just feed the buffer with 1000 bytes.
       buffer_content_size = 1000;
     }
     populateBuffer(buffer_content_size);
@@ -160,8 +156,7 @@ class CompressorFilterTest : public testing::Test {
         EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(trailers));
       }
       verifyCompressedData();
-      EXPECT_EQ(
-          1, stats_.counter(fmt::format("test.test.{}compressed", response_stats_prefix_)).value());
+      EXPECT_EQ(1, stats_.counter("test.test.compressed").value());
     } else {
       EXPECT_EQ("", headers.get_("content-encoding"));
       EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(data_, false));
@@ -238,82 +233,6 @@ TEST_F(CompressorFilterTest, CompressRequest) {
   doResponseNoCompression(headers);
 }
 
-TEST_F(CompressorFilterTest, CompressRequestNoContentLength) {
-  setUpFilter(R"EOF(
-{
-  "request_direction_config": {},
-  "compressor_library": {
-     "name": "test",
-     "typed_config": {
-       "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
-     }
-  }
-}
-)EOF");
-  doRequestCompression({{":method", "post"}}, false);
-  Http::TestResponseHeaderMapImpl headers{{":status", "200"}};
-  doResponseNoCompression(headers);
-}
-
-TEST_F(CompressorFilterTest, CompressRequestNoContentLengthRuntimeDisabled) {
-  setUpFilter(R"EOF(
-{
-  "request_direction_config": {},
-  "compressor_library": {
-    "name": "test",
-    "typed_config": {
-      "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
-    }
-  }
-}
-)EOF");
-  TestScopedRuntime scoped_runtime;
-  Runtime::LoaderSingleton::getExisting()->mergeValues(
-      {{"envoy.reloadable_features.enable_compression_without_chunked_header", "false"}});
-  doRequestNoCompression({{":method", "post"}});
-  Http::TestResponseHeaderMapImpl headers{{":status", "200"}};
-  doResponseNoCompression(headers);
-}
-
-TEST_F(CompressorFilterTest, CompressResponseNoContentLength) {
-  setUpFilter(R"EOF(
-{
-  "response_direction_config": {},
-  "compressor_library": {
-     "name": "test",
-     "typed_config": {
-       "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
-     }
-  }
-}
-)EOF");
-  response_stats_prefix_ = "response.";
-  doRequestNoCompression({{":method", "get"}, {"accept-encoding", "deflate, test"}});
-  Http::TestResponseHeaderMapImpl headers{{":status", "200"}};
-  doResponseCompression(headers, false);
-}
-
-TEST_F(CompressorFilterTest, CompressResponseNoContentLengthRuntimeDisabled) {
-  setUpFilter(R"EOF(
-{
-  "response_direction_config": {},
-  "compressor_library": {
-    "name": "test",
-    "typed_config": {
-      "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
-    }
-  }
-}
-)EOF");
-  TestScopedRuntime scoped_runtime;
-  Runtime::LoaderSingleton::getExisting()->mergeValues(
-      {{"envoy.reloadable_features.enable_compression_without_chunked_header", "false"}});
-  response_stats_prefix_ = "response.";
-  doRequestNoCompression({{":method", "get"}, {"accept-encoding", "deflate, test"}});
-  Http::TestResponseHeaderMapImpl headers{{":status", "200"}};
-  doResponseNoCompression(headers);
-}
-
 TEST_F(CompressorFilterTest, CompressRequestWithTrailers) {
   setUpFilter(R"EOF(
 {
@@ -637,7 +556,6 @@ INSTANTIATE_TEST_SUITE_P(
                     std::make_tuple("transfer-encoding", "Chunked", "", true),
                     std::make_tuple("transfer-encoding", "chunked", "\"content_length\": 500,",
                                     true),
-                    std::make_tuple("", "", "\"content_length\": 500,", true),
                     std::make_tuple("content-length", "501", "\"content_length\": 500,", true),
                     std::make_tuple("content-length", "499", "\"content_length\": 500,", false)));