From f2e2cdd582882aa7ab9151bff1b8aa515e5ab957 Mon Sep 17 00:00:00 2001 From: giantcroc Date: Fri, 2 Sep 2022 15:26:20 +0800 Subject: [PATCH 1/5] add multi_compressor_test Signed-off-by: giantcroc --- .../http/compressor/v3/compressor.proto | 5 +- .../http/compressor/compressor_filter.cc | 18 +-- .../http/compressor/compressor_filter.h | 2 + .../http/compressor/compressor_filter_test.cc | 130 ++++++++++++++++++ 4 files changed, 146 insertions(+), 9 deletions(-) diff --git a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto b/api/envoy/extensions/filters/http/compressor/v3/compressor.proto index bd53c30cec80c..5a03173ac25fd 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,7 @@ message Compressor { // instead of // ``.compressor...*``. ResponseDirectionConfig response_direction_config = 8; + + // If true, chooses this compressor first to do compression when the q-values are same. + bool choose_first = 9; } diff --git a/source/extensions/filters/http/compressor/compressor_filter.cc b/source/extensions/filters/http/compressor/compressor_filter.cc index 1e9ff0a41a910..52e8127628237 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,7 @@ 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 +404,12 @@ 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)) || + if (allowed_compressors.count(std::string(pair.first)) || pair.first == Http::CustomHeaders::get().AcceptEncodingValues.Identity || - pair.first == Http::CustomHeaders::get().AcceptEncodingValues.Wildcard)) { + 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; + } } } @@ -429,8 +431,8 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const { 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; }); + [](const std::pair>& a, + const std::pair>& b) -> bool { return a.second.first < b.second.first; }); 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..6b6394c31073e 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; diff --git a/test/extensions/filters/http/compressor/compressor_filter_test.cc b/test/extensions/filters/http/compressor/compressor_filter_test.cc index b5c837f2c3534..1a198fd01aae7 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/compressor/compressor_filter_test.cc @@ -812,6 +812,136 @@ TEST_F(MultipleFiltersTest, UseFirstRegisteredFilterWhenWildcard) { EXPECT_EQ(1, stats2_.counter("test2.compressor.test2.test.header_wildcard").value()); } +class ChooseFirstTest + : public MultipleFiltersTest, + public testing::WithParamInterface> { +protected: + void SetUp() override { + envoy::extensions::filters::http::compressor::v3::Compressor compressor; + TestUtility::loadFromJson(R"EOF( +{ + "compressor_library": { + "name": "test1", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" + } + } +} +)EOF", + compressor); + auto compressor_factory1 = std::make_unique("test1"); + // compressor_factory1->setExpectedCompressCalls(1); + auto config1 = std::make_shared(compressor, "test1.", stats1_, runtime_, + std::move(compressor_factory1)); + filter1_ = std::make_unique(config1); + + TestUtility::loadFromJson(R"EOF( +{ + "compressor_library": { + "name": "test2", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" + } + } +} +)EOF", + compressor); + auto compressor_factory2 = std::make_unique("test2"); + // compressor_factory2->setExpectedCompressCalls(0); + 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) { + 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("test1", 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("", 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("test1", true, "test1"))); + +TEST_P(ChooseFirstTest, Validate) { + const std::string& accept_encoding = std::get<0>(GetParam()); + const bool is_compression_expected = std::get<1>(GetParam()); + // const std::string& content_encoding = std::get<2>(GetParam()); + + 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); + +} + TEST(CompressorFilterConfigTests, MakeCompressorTest) { const envoy::extensions::filters::http::compressor::v3::Compressor compressor_cfg; NiceMock runtime; From 640ab87b31933bd353c0e3077e55deb12dc0cc0d Mon Sep 17 00:00:00 2001 From: giantcroc Date: Wed, 7 Sep 2022 14:01:39 +0800 Subject: [PATCH 2/5] add doc and format Signed-off-by: giantcroc --- .../http/compressor/v3/compressor.proto | 2 +- changelogs/current.yaml | 3 ++ .../http/compressor/compressor_filter.cc | 24 ++++++++------ .../http/compressor/compressor_filter.h | 2 +- .../http/compressor/compressor_filter_test.cc | 33 +++++++++---------- 5 files changed, 35 insertions(+), 29 deletions(-) diff --git a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto b/api/envoy/extensions/filters/http/compressor/v3/compressor.proto index 5a03173ac25fd..51906ae03934b 100644 --- a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto +++ b/api/envoy/extensions/filters/http/compressor/v3/compressor.proto @@ -124,6 +124,6 @@ message Compressor { // ``.compressor...*``. ResponseDirectionConfig response_direction_config = 8; - // If true, chooses this compressor first to do compression when the q-values are same. + // If true, chooses this compressor first to do compression when the q-values are same. 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 52e8127628237..d043e016ecf1e 100644 --- a/source/extensions/filters/http/compressor/compressor_filter.cc +++ b/source/extensions/filters/http/compressor/compressor_filter.cc @@ -323,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 = @@ -357,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,filter_config->chooseFirst()}}); + allowed_compressors.insert( + {filter_config->contentEncoding(), {registration_count, filter_config->chooseFirst()}}); ++registration_count; } } @@ -405,10 +406,11 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const { EncPair choice{Http::CustomHeaders::get().AcceptEncodingValues.Identity, static_cast(0)}; for (const auto& pair : pairs) { 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; + 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; } } } @@ -429,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>& a, - const std::pair>& b) -> bool { return a.second.first < b.second.first; }); + auto first_registered = + std::min_element(allowed_compressors.begin(), allowed_compressors.end(), + [](const std::pair>& a, + const std::pair>& b) -> bool { + return a.second.first < b.second.first; + }); 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 6b6394c31073e..5d01b2d2e66cf 100644 --- a/source/extensions/filters/http/compressor/compressor_filter.h +++ b/source/extensions/filters/http/compressor/compressor_filter.h @@ -145,7 +145,7 @@ class CompressorFilterConfig { Envoy::Compression::Compressor::CompressorPtr makeCompressor(); const std::string contentEncoding() const { return content_encoding_; }; - bool chooseFirst() const {return choose_first_; }; + bool chooseFirst() const { return choose_first_; }; const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; } const ResponseDirectionConfig& responseDirectionConfig() { return response_direction_config_; } diff --git a/test/extensions/filters/http/compressor/compressor_filter_test.cc b/test/extensions/filters/http/compressor/compressor_filter_test.cc index 1a198fd01aae7..852748bbbb4dc 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/compressor/compressor_filter_test.cc @@ -820,6 +820,7 @@ class ChooseFirstTest envoy::extensions::filters::http::compressor::v3::Compressor compressor; TestUtility::loadFromJson(R"EOF( { + "choose_first": "true", "compressor_library": { "name": "test1", "typed_config": { @@ -830,7 +831,6 @@ class ChooseFirstTest )EOF", compressor); auto compressor_factory1 = std::make_unique("test1"); - // compressor_factory1->setExpectedCompressCalls(1); auto config1 = std::make_shared(compressor, "test1.", stats1_, runtime_, std::move(compressor_factory1)); filter1_ = std::make_unique(config1); @@ -847,7 +847,6 @@ class ChooseFirstTest )EOF", compressor); auto compressor_factory2 = std::make_unique("test2"); - // compressor_factory2->setExpectedCompressCalls(0); auto config2 = std::make_shared(compressor, "test2.", stats2_, runtime_, std::move(compressor_factory2)); filter2_ = std::make_unique(config2); @@ -863,22 +862,22 @@ class ChooseFirstTest EXPECT_EQ(expected_str_.length(), stats1_ .counter(fmt::format("test1.compressor.test1.test.{}total_uncompressed_bytes", - response_stats_prefix_)) + response_stats_prefix_)) .value()); EXPECT_EQ(data_.length(), stats1_ .counter(fmt::format("test1.compressor.test1.test.{}total_compressed_bytes", - response_stats_prefix_)) + 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)); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter1_->decodeData(data_, false)); } void doResponse(Http::TestResponseHeaderMapImpl& headers, bool with_compression, - bool with_trailers) { + 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; @@ -892,7 +891,7 @@ class ChooseFirstTest if (with_compression) { EXPECT_EQ("", headers.get_("content-length")); - EXPECT_EQ("test1", headers.get_("content-encoding")); + 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)) @@ -906,7 +905,7 @@ class ChooseFirstTest response_stats_prefix_)) .value()); } else { - EXPECT_EQ("", headers.get_("content-encoding")); + 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", @@ -919,16 +918,18 @@ class ChooseFirstTest std::string expected_str_; std::string response_stats_prefix_{}; NiceMock encoder_callbacks_; - }; +}; -INSTANTIATE_TEST_SUITE_P( - ChooseFirstTestSuite, ChooseFirstTest, - testing::Values(std::make_tuple("test1", true, "test1"))); +INSTANTIATE_TEST_SUITE_P(ChooseFirstTestSuite, ChooseFirstTest, + testing::Values(std::make_tuple("test1", true, "test1"), + std::make_tuple("test2, test1", true, "test1"), + std::make_tuple("test2;q=1, test1;q=1", true, "test1"), + std::make_tuple("test2;q=1, test1;q=0.5", false, ""))); TEST_P(ChooseFirstTest, Validate) { const std::string& accept_encoding = std::get<0>(GetParam()); const bool is_compression_expected = std::get<1>(GetParam()); - // const std::string& content_encoding = std::get<2>(GetParam()); + const std::string& content_encoding = std::get<2>(GetParam()); NiceMock decoder_callbacks; filter1_->setDecoderFilterCallbacks(decoder_callbacks); @@ -936,10 +937,8 @@ TEST_P(ChooseFirstTest, Validate) { 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); - + Http::TestResponseHeaderMapImpl headers{{":method", "get"}, {"content-length", "256"}}; + doResponse(headers, is_compression_expected, false, content_encoding); } TEST(CompressorFilterConfigTests, MakeCompressorTest) { From bb8417026d2a458a57cb194fcc97914265c8d0d2 Mon Sep 17 00:00:00 2001 From: giantcroc Date: Fri, 9 Sep 2022 17:29:16 +0800 Subject: [PATCH 3/5] add comments in proto Signed-off-by: giantcroc --- .../extensions/filters/http/compressor/v3/compressor.proto | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto b/api/envoy/extensions/filters/http/compressor/v3/compressor.proto index 51906ae03934b..d8cca485a6472 100644 --- a/api/envoy/extensions/filters/http/compressor/v3/compressor.proto +++ b/api/envoy/extensions/filters/http/compressor/v3/compressor.proto @@ -124,6 +124,7 @@ message Compressor { // ``.compressor...*``. ResponseDirectionConfig response_direction_config = 8; - // If true, chooses this compressor first to do compression when the q-values are same. + // 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; } From 8dac6b29c62c5c3444d065889872ca2abfc8158e Mon Sep 17 00:00:00 2001 From: giantcroc Date: Wed, 14 Sep 2022 20:04:31 +0800 Subject: [PATCH 4/5] add more test case Signed-off-by: giantcroc --- .../http/compressor/compressor_filter_test.cc | 66 +++++++++++-------- 1 file changed, 39 insertions(+), 27 deletions(-) diff --git a/test/extensions/filters/http/compressor/compressor_filter_test.cc b/test/extensions/filters/http/compressor/compressor_filter_test.cc index 852748bbbb4dc..23ca35486a4fc 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/compressor/compressor_filter_test.cc @@ -812,39 +812,45 @@ TEST_F(MultipleFiltersTest, UseFirstRegisteredFilterWhenWildcard) { EXPECT_EQ(1, stats2_.counter("test2.compressor.test2.test.header_wildcard").value()); } -class ChooseFirstTest - : public MultipleFiltersTest, - public testing::WithParamInterface> { +// TODO(giantcroc): Refactor the code of MultipleFiltersTest and CompressorFilterTest due to many +// duplicate methods +class ChooseFirstTest : public MultipleFiltersTest, + public testing::WithParamInterface< + std::tuple> { protected: - void SetUp() override { + // ChooseFirstTest Helpers + void setUpFilter(const std::string& choose_first1, const std::string& choose_first2) { envoy::extensions::filters::http::compressor::v3::Compressor compressor; - TestUtility::loadFromJson(R"EOF( -{ - "choose_first": "true", - "compressor_library": { + TestUtility::loadFromJson(fmt::format(R"EOF( +{{ + "choose_first": {}, + "compressor_library": {{ "name": "test1", - "typed_config": { + "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(R"EOF( -{ - "compressor_library": { + TestUtility::loadFromJson(fmt::format(R"EOF( +{{ + "choose_first": {}, + "compressor_library": {{ "name": "test2", - "typed_config": { + "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_, @@ -920,17 +926,23 @@ class ChooseFirstTest NiceMock encoder_callbacks_; }; -INSTANTIATE_TEST_SUITE_P(ChooseFirstTestSuite, ChooseFirstTest, - testing::Values(std::make_tuple("test1", true, "test1"), - std::make_tuple("test2, test1", true, "test1"), - std::make_tuple("test2;q=1, test1;q=1", true, "test1"), - std::make_tuple("test2;q=1, test1;q=0.5", false, ""))); +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& accept_encoding = std::get<0>(GetParam()); - const bool is_compression_expected = std::get<1>(GetParam()); - const std::string& content_encoding = std::get<2>(GetParam()); + 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_); From 5f9f37f677c2ced6b0f50457df74bacc953420b3 Mon Sep 17 00:00:00 2001 From: giantcroc Date: Thu, 15 Sep 2022 12:21:24 +0800 Subject: [PATCH 5/5] add CompressorInChain Signed-off-by: giantcroc --- .../filters/http/compressor/compressor_filter.cc | 11 ++++++----- .../filters/http/compressor/compressor_filter.h | 5 +++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/compressor/compressor_filter.cc b/source/extensions/filters/http/compressor/compressor_filter.cc index d043e016ecf1e..67938092d0e33 100644 --- a/source/extensions/filters/http/compressor/compressor_filter.cc +++ b/source/extensions/filters/http/compressor/compressor_filter.cc @@ -323,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 = @@ -409,7 +409,8 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const { 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)) { + (pair.second == choice.second && + allowed_compressors[std::string(pair.first)].choose_first_)) { choice = pair; } } @@ -433,9 +434,9 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const { 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.first < b.second.first; + [](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 5d01b2d2e66cf..2a4f05a11404a 100644 --- a/source/extensions/filters/http/compressor/compressor_filter.h +++ b/source/extensions/filters/http/compressor/compressor_filter.h @@ -202,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;