compression: support choosing compressor first when the q-values are same

api/envoy/extensions/filters/http/compressor/v3/compressor.proto

@@ -22,7 +22,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // Compressor :ref:`configuration overview <config_http_filters_compressor>`.
 // [#extension: envoy.filters.http.compressor]
 
-// [#next-free-field: 9]
+// [#next-free-field: 10]
 message Compressor {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.filter.http.compressor.v2.Compressor";
@@ -123,4 +123,8 @@ message Compressor {
   //    instead of
   //    ``<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.*``.
   ResponseDirectionConfig response_direction_config = 8;
+
+  // If true, chooses this compressor first to do compression when the q-values in `Accept-Encoding` are same.
+  // The last compressor which enables choose_first will be chosen if multiple compressor filters in the chain have choose_first as true.
+  bool choose_first = 9;
 }

changelogs/current.yaml

@@ -91,6 +91,9 @@ new_features:
 - area: thrift
   change: |
     added stats for downstream connection close to detect SR drop.
+- area: compression
+  change: |
+    added support for :ref:`choose_first<envoy_v3_api_msg_extensions.filters.http.compressor.v3.Compressor>`.
 - area: cors
   change: |
     added support for cors PNA. This behavioral change can be temporarily reverted by setting runtime guard ``envoy_reloadable_features_cors_private_network_access`` to false. More details refer to https://developer.chrome.com/blog/private-network-access-preflight.

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

@@ -91,7 +91,8 @@ CompressorFilterConfig::CompressorFilterConfig(
       request_direction_config_(proto_config, common_stats_prefix_, scope, runtime),
       response_direction_config_(proto_config, common_stats_prefix_, scope, runtime),
       content_encoding_(compressor_factory->contentEncoding()),
-      compressor_factory_(std::move(compressor_factory)) {}
+      compressor_factory_(std::move(compressor_factory)),
+      choose_first_(proto_config.choose_first()) {}
 
 StringUtil::CaseUnorderedSet CompressorFilterConfig::DirectionConfig::contentTypeSet(
     const Protobuf::RepeatedPtrField<std::string>& types) {
@@ -322,7 +323,7 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {
   }
 
   // Find all compressors enabled for the filter chain.
-  std::map<std::string, uint32_t> allowed_compressors;
+  std::map<std::string, std::pair<uint32_t, bool>> allowed_compressors;
   uint32_t registration_count{0};
 
   auto typed_state =
@@ -356,7 +357,8 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {
     // registered last.
     auto enc = allowed_compressors.find(filter_config->contentEncoding());
     if (enc == allowed_compressors.end()) {
-      allowed_compressors.insert({filter_config->contentEncoding(), registration_count});
+      allowed_compressors.insert(
+          {filter_config->contentEncoding(), {registration_count, filter_config->chooseFirst()}});
       ++registration_count;
     }
   }
@@ -403,11 +405,13 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {
   // by the allowed compressors and choose the one with the highest q-value.
   EncPair choice{Http::CustomHeaders::get().AcceptEncodingValues.Identity, static_cast<float>(0)};
   for (const auto& pair : pairs) {
-    if ((pair.second > choice.second) &&
-        (allowed_compressors.count(std::string(pair.first)) ||
-         pair.first == Http::CustomHeaders::get().AcceptEncodingValues.Identity ||
-         pair.first == Http::CustomHeaders::get().AcceptEncodingValues.Wildcard)) {
-      choice = pair;
+    if (allowed_compressors.count(std::string(pair.first)) ||
+        pair.first == Http::CustomHeaders::get().AcceptEncodingValues.Identity ||
+        pair.first == Http::CustomHeaders::get().AcceptEncodingValues.Wildcard) {
+      if ((pair.second > choice.second) ||
+          (pair.second == choice.second && allowed_compressors[std::string(pair.first)].second)) {
+        choice = pair;
+      }
     }
   }
 
@@ -427,10 +431,12 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {
 
   // If wildcard is given then use which ever compressor is registered first.
   if (choice.first == Http::CustomHeaders::get().AcceptEncodingValues.Wildcard) {
-    auto first_registered = std::min_element(
-        allowed_compressors.begin(), allowed_compressors.end(),
-        [](const std::pair<std::string, uint32_t>& a,
-           const std::pair<std::string, uint32_t>& b) -> bool { return a.second < b.second; });
+    auto first_registered =
+        std::min_element(allowed_compressors.begin(), allowed_compressors.end(),
+                         [](const std::pair<std::string, std::pair<uint32_t, bool>>& a,
+                            const std::pair<std::string, std::pair<uint32_t, bool>>& b) -> bool {
+                           return a.second.first < b.second.first;
+                         });
     return std::make_unique<CompressorFilter::EncodingDecision>(
         first_registered->first, CompressorFilter::EncodingDecision::HeaderStat::Wildcard);
   }

source/extensions/filters/http/compressor/compressor_filter.h

@@ -145,6 +145,7 @@ class CompressorFilterConfig {
   Envoy::Compression::Compressor::CompressorPtr makeCompressor();
 
   const std::string contentEncoding() const { return content_encoding_; };
+  bool chooseFirst() const { return choose_first_; };
   const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; }
   const ResponseDirectionConfig& responseDirectionConfig() { return response_direction_config_; }
 
@@ -155,6 +156,7 @@ class CompressorFilterConfig {
 
   const std::string content_encoding_;
   const Envoy::Compression::Compressor::CompressorFactoryPtr compressor_factory_;
+  const bool choose_first_;
 };
 using CompressorFilterConfigSharedPtr = std::shared_ptr<CompressorFilterConfig>;
 

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

@@ -812,6 +812,147 @@ TEST_F(MultipleFiltersTest, UseFirstRegisteredFilterWhenWildcard) {
   EXPECT_EQ(1, stats2_.counter("test2.compressor.test2.test.header_wildcard").value());
 }
 
+// TODO(giantcroc): Refactor the code of MultipleFiltersTest and CompressorFilterTest due to many
+// duplicate methods
+class ChooseFirstTest : public MultipleFiltersTest,
+                        public testing::WithParamInterface<
+                            std::tuple<std::string, std::string, std::string, bool, std::string>> {
+protected:
+  // ChooseFirstTest Helpers
+  void setUpFilter(const std::string& choose_first1, const std::string& choose_first2) {
+    envoy::extensions::filters::http::compressor::v3::Compressor compressor;
+    TestUtility::loadFromJson(fmt::format(R"EOF(
+{{
+  "choose_first": {},
+  "compressor_library": {{
+     "name": "test1",
+     "typed_config": {{
+       "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
+     }}
+  }}
+}}
+)EOF",
+                                          choose_first1),
+                              compressor);
+    auto compressor_factory1 = std::make_unique<TestCompressorFactory>("test1");
+    auto config1 = std::make_shared<CompressorFilterConfig>(compressor, "test1.", stats1_, runtime_,
+                                                            std::move(compressor_factory1));
+    filter1_ = std::make_unique<CompressorFilter>(config1);
+
+    TestUtility::loadFromJson(fmt::format(R"EOF(
+{{
+  "choose_first": {},
+  "compressor_library": {{
+     "name": "test2",
+     "typed_config": {{
+       "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
+     }}
+  }}
+}}
+)EOF",
+                                          choose_first2),
+                              compressor);
+    auto compressor_factory2 = std::make_unique<TestCompressorFactory>("test2");
+    auto config2 = std::make_shared<CompressorFilterConfig>(compressor, "test2.", stats2_, runtime_,
+                                                            std::move(compressor_factory2));
+    filter2_ = std::make_unique<CompressorFilter>(config2);
+  }
+
+  void populateBuffer(uint64_t size) {
+    data_.drain(data_.length());
+    TestUtility::feedBufferWithRandomCharacters(data_, size);
+    expected_str_ = data_.toString();
+  }
+
+  void verifyCompressedData() {
+    EXPECT_EQ(expected_str_.length(),
+              stats1_
+                  .counter(fmt::format("test1.compressor.test1.test.{}total_uncompressed_bytes",
+                                       response_stats_prefix_))
+                  .value());
+    EXPECT_EQ(data_.length(),
+              stats1_
+                  .counter(fmt::format("test1.compressor.test1.test.{}total_compressed_bytes",
+                                       response_stats_prefix_))
+                  .value());
+  }
+
+  void doRequest(Http::TestRequestHeaderMapImpl&& headers) {
+    EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter1_->decodeHeaders(headers, false));
+    EXPECT_EQ(Http::FilterDataStatus::Continue, filter1_->decodeData(data_, false));
+  }
+
+  void doResponse(Http::TestResponseHeaderMapImpl& headers, bool with_compression,
+                  bool with_trailers, const std::string& content_encoding) {
+    uint64_t buffer_content_size;
+    if (!absl::SimpleAtoi(headers.get_("content-length"), &buffer_content_size)) {
+      buffer_content_size = 1000;
+    }
+    populateBuffer(buffer_content_size);
+    Http::TestResponseHeaderMapImpl continue_headers;
+    EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter1_->encode1xxHeaders(continue_headers));
+    Http::MetadataMap metadata_map{{"metadata", "metadata"}};
+    EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter1_->encodeMetadata(metadata_map));
+    EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter1_->encodeHeaders(headers, false));
+
+    if (with_compression) {
+      EXPECT_EQ("", headers.get_("content-length"));
+      EXPECT_EQ(content_encoding, headers.get_("content-encoding"));
+      EXPECT_EQ(Http::FilterDataStatus::Continue, filter1_->encodeData(data_, !with_trailers));
+      if (with_trailers) {
+        EXPECT_CALL(encoder_callbacks_, addEncodedData(_, true))
+            .WillOnce(Invoke([&](Buffer::Instance& data, bool) { data_.move(data); }));
+        Http::TestResponseTrailerMapImpl trailers;
+        EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter1_->encodeTrailers(trailers));
+      }
+      verifyCompressedData();
+      EXPECT_EQ(1, stats1_
+                       .counter(fmt::format("test1.compressor.test1.test.{}compressed",
+                                            response_stats_prefix_))
+                       .value());
+    } else {
+      EXPECT_EQ(content_encoding, headers.get_("content-encoding"));
+      EXPECT_EQ(Http::FilterDataStatus::Continue, filter1_->encodeData(data_, false));
+      EXPECT_EQ(1, stats1_
+                       .counter(fmt::format("test1.compressor.test1.test.{}not_compressed",
+                                            response_stats_prefix_))
+                       .value());
+    }
+  }
+
+  Buffer::OwnedImpl data_;
+  std::string expected_str_;
+  std::string response_stats_prefix_{};
+  NiceMock<Http::MockStreamEncoderFilterCallbacks> encoder_callbacks_;
+};
+
+INSTANTIATE_TEST_SUITE_P(
+    ChooseFirstTestSuite, ChooseFirstTest,
+    testing::Values(std::make_tuple("true", "false", "test1", true, "test1"),
+                    std::make_tuple("true", "false", "test2, test1", true, "test1"),
+                    std::make_tuple("true", "false", "test2;q=1, test1;q=1", true, "test1"),
+                    std::make_tuple("true", "false", "test2;q=1, test1;q=0.5", false, ""),
+                    std::make_tuple("true", "true", "test2, test1", true, "test1"),
+                    std::make_tuple("true", "true", "test2;q=1, test1;q=0.5", false, "")));
+
+TEST_P(ChooseFirstTest, Validate) {
+  const std::string& choose_first1 = std::get<0>(GetParam());
+  const std::string& choose_first2 = std::get<1>(GetParam());
+  const std::string& accept_encoding = std::get<2>(GetParam());
+  const bool is_compression_expected = std::get<3>(GetParam());
+  const std::string& content_encoding = std::get<4>(GetParam());
+
+  setUpFilter(choose_first1, choose_first2);
+  NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks;
+  filter1_->setDecoderFilterCallbacks(decoder_callbacks);
+  filter1_->setEncoderFilterCallbacks(encoder_callbacks_);
+  filter2_->setDecoderFilterCallbacks(decoder_callbacks);
+
+  doRequest({{":method", "get"}, {"accept-encoding", accept_encoding}});
+  Http::TestResponseHeaderMapImpl headers{{":method", "get"}, {"content-length", "256"}};
+  doResponse(headers, is_compression_expected, false, content_encoding);
+}
+
 TEST(CompressorFilterConfigTests, MakeCompressorTest) {
   const envoy::extensions::filters::http::compressor::v3::Compressor compressor_cfg;
   NiceMock<Runtime::MockLoader> runtime;