diff --git a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto b/api/envoy/extensions/filters/http/compressor/v3/compressor.proto index bd53c30cec80c..d8cca485a6472 100644 --- a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto +++ b/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 `. // [#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 // ``.compressor...*``. 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; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 0c9998077cb4d..2e3d2e4ce7431 100644 --- a/changelogs/current.yaml +++ b/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`. - 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. diff --git a/source/extensions/filters/http/compressor/compressor_filter.cc b/source/extensions/filters/http/compressor/compressor_filter.cc index 1e9ff0a41a910..67938092d0e33 100644 --- a/source/extensions/filters/http/compressor/compressor_filter.cc +++ b/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& types) { @@ -322,7 +323,7 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const { } // Find all compressors enabled for the filter chain. - std::map allowed_compressors; + std::map 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,14 @@ 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(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)].choose_first_)) { + choice = pair; + } } } @@ -427,10 +432,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& a, - const std::pair& b) -> bool { return a.second < b.second; }); + auto first_registered = + std::min_element(allowed_compressors.begin(), allowed_compressors.end(), + [](const std::pair& a, + const std::pair& b) -> bool { + return a.second.registration_count_ < b.second.registration_count_; + }); return std::make_unique( first_registered->first, CompressorFilter::EncodingDecision::HeaderStat::Wildcard); } diff --git a/source/extensions/filters/http/compressor/compressor_filter.h b/source/extensions/filters/http/compressor/compressor_filter.h index 2290b653312b1..2a4f05a11404a 100644 --- a/source/extensions/filters/http/compressor/compressor_filter.h +++ b/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; @@ -200,6 +202,11 @@ class CompressorFilter : public Http::PassThroughFilter { const HeaderStat stat_; }; + struct CompressorInChain { + uint32_t registration_count_; + bool choose_first_; + }; + std::unique_ptr chooseEncoding(const Http::ResponseHeaderMap& headers) const; bool shouldCompress(const EncodingDecision& decision) const; diff --git a/test/extensions/filters/http/compressor/compressor_filter_test.cc b/test/extensions/filters/http/compressor/compressor_filter_test.cc index b5c837f2c3534..23ca35486a4fc 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_test.cc +++ b/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> { +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("test1"); + auto config1 = std::make_shared(compressor, "test1.", stats1_, runtime_, + std::move(compressor_factory1)); + filter1_ = std::make_unique(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("test2"); + auto config2 = std::make_shared(compressor, "test2.", stats2_, runtime_, + std::move(compressor_factory2)); + filter2_ = std::make_unique(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 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 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;