gzip: allow gzip to work w/ http backend w/o content-length (envoypro…

…xy#14584)

This is an attempt to address issue in envoyproxy#14121

tl;dr is that some http/2 backends does not send "content-length" header in replies, as http/2 spec do have the same info as a part of the frame (unfortunately it seems like there is no way to pass this value from the frame to the filter) and transfer-encoding=chunked (before this diff having that header/encoding was a prerequisite, if content-length is not defined) was removed from http/2 spec.

As discussed in the issue itself, instead, if there is no content lengths header - we would try to gzip it by default.

This new behavior is controlled by runtime guarded feature envoy.reloadable_features.enable_compression_without_chunked_header

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>
Signed-off-by: Auni Ahsan <auni@google.com>

docs/root/version_history/current.rst

@@ -110,6 +110,7 @@ New Features
 * access log: support command operator: %FILTER_CHAIN_NAME% for the downstream tcp and http request.
 * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES%, and %RESPONSE_TRAILERS_BYTES%.
 * compression: add brotli :ref:`compressor <envoy_v3_api_msg_extensions.compression.brotli.compressor.v3.Brotli>` and :ref:`decompressor <envoy_v3_api_msg_extensions.compression.brotli.decompressor.v3.Brotli>`.
+* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false.
 * config: add `envoy.features.fail_on_any_deprecated_feature` runtime key, which matches the behaviour of compile-time flag `ENVOY_DISABLE_DEPRECATED_FEATURES`, i.e. use of deprecated fields will cause a crash.
 * config: the ``Node`` :ref:`dynamic context parameters <envoy_v3_api_field_config.core.v3.Node.dynamic_parameters>` are populated in discovery requests when set on the server instance.
 * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash.

source/common/runtime/runtime_features.cc

@@ -66,6 +66,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.check_ocsp_policy",
     "envoy.reloadable_features.disable_tls_inspector_injection",
     "envoy.reloadable_features.dont_add_content_length_for_bodiless_requests",
+    "envoy.reloadable_features.enable_compression_without_content_length_header",
     "envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling",
     "envoy.reloadable_features.hcm_stream_error_on_invalid_message",
     "envoy.reloadable_features.health_check.graceful_goaway_handling",

source/extensions/filters/http/common/compressor/BUILD

@@ -19,6 +19,7 @@ envoy_cc_library(
         "//include/envoy/stream_info:filter_state_interface",
         "//source/common/buffer:buffer_lib",
         "//source/common/http:header_map_lib",
+        "//source/common/http:utility_lib",
         "//source/common/protobuf",
         "//source/common/runtime:runtime_lib",
         "//source/extensions/filters/http/common:pass_through_filter_lib",

source/extensions/filters/http/common/compressor/compressor.cc

@@ -2,6 +2,7 @@
 
 #include "common/buffer/buffer_impl.h"
 #include "common/http/header_map_impl.h"
+#include "common/http/utility.h"
 
 namespace Envoy {
 namespace Extensions {
@@ -156,7 +157,12 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap
   }
 
   const auto& request_config = config_->requestDirectionConfig();
-  if (!end_stream && request_config.compressionEnabled() &&
+  const bool is_not_upgrade =
+      !Http::Utility::isUpgrade(headers) ||
+      !Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.enable_compression_without_content_length_header");
+
+  if (!end_stream && request_config.compressionEnabled() && is_not_upgrade &&
       request_config.isMinimumContentLength(headers) &&
       request_config.isContentTypeAllowed(headers) &&
       !headers.getInline(request_content_encoding_handle.handle()) &&
@@ -221,7 +227,12 @@ Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMa
   const auto& config = config_->responseDirectionConfig();
   const bool isEnabledAndContentLengthBigEnough =
       config.compressionEnabled() && config.isMinimumContentLength(headers);
-  const bool isCompressible = isEnabledAndContentLengthBigEnough &&
+  const bool is_not_upgrade =
+      !Http::Utility::isUpgrade(headers) ||
+      !Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.enable_compression_without_content_length_header");
+
+  const bool isCompressible = isEnabledAndContentLengthBigEnough && is_not_upgrade &&
                               config.isContentTypeAllowed(headers) &&
                               !hasCacheControlNoTransform(headers) && isEtagAllowed(headers) &&
                               !headers.getInline(response_content_encoding_handle.handle());
@@ -506,7 +517,11 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength(
     }
     return is_minimum_content_length;
   }
-
+  if (Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.enable_compression_without_content_length_header")) {
+    // return true to ignore the minimum length configuration if no content-length header is present
+    return true;
+  }
   return StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",",
                                    Http::Headers::get().TransferEncodingValues.Chunked);
 }

test/extensions/filters/http/common/compressor/BUILD

@@ -21,6 +21,7 @@ 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",
     ],
@@ -46,6 +47,25 @@ envoy_cc_benchmark_binary(
     ],
 )
 
+envoy_cc_test(
+    name = "compressor_integration_tests",
+    srcs = [
+        "compressor_integration_tests.cc",
+        "compressor_integration_tests.h",
+    ],
+    deps = [
+        "//source/common/http:header_map_lib",
+        "//source/extensions/access_loggers/file:config",
+        "//source/extensions/compression/gzip/compressor:config",
+        "//source/extensions/filters/http/buffer:config",
+        "//source/extensions/filters/http/compressor:config",
+        "//test/integration:http_protocol_integration_lib",
+        "//test/test_common:utility_lib",
+        "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
+        "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
+    ],
+)
+
 envoy_benchmark_test(
     name = "compressor_filter_speed_test_benchmark_test",
     benchmark_binary = "compressor_filter_speed_test",

test/extensions/filters/http/common/compressor/compressor_filter_test.cc

@@ -11,6 +11,7 @@
 #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"
@@ -77,8 +78,14 @@ class CompressorFilterTest : public testing::Test {
   }
 
   void verifyCompressedData() {
-    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());
+    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());
   }
 
   void populateBuffer(uint64_t size) {
@@ -132,9 +139,6 @@ 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);
@@ -156,7 +160,8 @@ class CompressorFilterTest : public testing::Test {
         EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(trailers));
       }
       verifyCompressedData();
-      EXPECT_EQ(1, stats_.counter("test.test.compressed").value());
+      EXPECT_EQ(
+          1, stats_.counter(fmt::format("test.test.{}compressed", response_stats_prefix_)).value());
     } else {
       EXPECT_EQ("", headers.get_("content-encoding"));
       EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(data_, false));
@@ -233,6 +238,47 @@ TEST_F(CompressorFilterTest, CompressRequest) {
   doResponseNoCompression(headers);
 }
 
+TEST_F(CompressorFilterTest, CompressRequestAndResponseNoContentLength) {
+  setUpFilter(R"EOF(
+{
+  "request_direction_config": {},
+  "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.";
+  doRequestCompression({{":method", "post"}, {"accept-encoding", "deflate, test"}}, false);
+  Http::TestResponseHeaderMapImpl headers{{":status", "200"}};
+  doResponseCompression(headers, false);
+}
+
+TEST_F(CompressorFilterTest, CompressRequestAndResponseNoContentLengthRuntimeDisabled) {
+  setUpFilter(R"EOF(
+{
+  "request_direction_config": {},
+  "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_content_length_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(
 {
@@ -552,10 +598,7 @@ INSTANTIATE_TEST_SUITE_P(
     IsMinimumContentLengthTestSuite, IsMinimumContentLengthTest,
     testing::Values(std::make_tuple("content-length", "31", "", true),
                     std::make_tuple("content-length", "29", "", false),
-                    std::make_tuple("transfer-encoding", "chunked", "", true),
-                    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)));
 
@@ -590,9 +633,7 @@ class IsTransferEncodingAllowedTest
 
 INSTANTIATE_TEST_SUITE_P(
     IsTransferEncodingAllowedSuite, IsTransferEncodingAllowedTest,
-    testing::Values(std::make_tuple("transfer-encoding", "chunked", true),
-                    std::make_tuple("transfer-encoding", "Chunked", true),
-                    std::make_tuple("transfer-encoding", "deflate", false),
+    testing::Values(std::make_tuple("transfer-encoding", "deflate", false),
                     std::make_tuple("transfer-encoding", "Deflate", false),
                     std::make_tuple("transfer-encoding", "test", false),
                     std::make_tuple("transfer-encoding", "chunked, test", false),