From 84affaad1173a1e48bbee5b3049c305a555eb3b2 Mon Sep 17 00:00:00 2001 From: Jose Ulises Nino Rivera Date: Sat, 11 Jul 2020 18:50:31 -0700 Subject: [PATCH] decompressor library: Add stats to zlib library (#11782) This adds decompressor error stats to the zlib decompressor library. This introduces a lot of boilerplate but will make it easier to continue to add stats in the future. Additionally, the compressor library can use the same pattern. Risk Level: Low. Testing: Updated. Docs Changes: Added. Signed-off-by: Jose Nino --- .../http/http_filters/decompressor_filter.rst | 3 + .../envoy/compression/decompressor/factory.h | 2 +- .../common/decompressor/factory_base.h | 6 +- .../compression/gzip/decompressor/BUILD | 2 + .../compression/gzip/decompressor/config.cc | 15 +- .../compression/gzip/decompressor/config.h | 10 +- .../decompressor/zlib_decompressor_impl.cc | 50 +++++-- .../decompressor/zlib_decompressor_impl.h | 36 ++++- .../http/decompressor/decompressor_filter.cc | 1 + .../http/decompressor/decompressor_filter.h | 3 +- .../compression/gzip/compressor_fuzz_test.cc | 5 +- .../compression/gzip/decompressor/BUILD | 1 + .../zlib_decompressor_impl_test.cc | 54 ++++++-- .../compressor_filter_integration_test.cc | 4 +- .../decompressor_filter_integration_test.cc | 130 ++++++++++++++++-- .../decompressor/decompressor_filter_test.cc | 84 ++++++++--- .../http/gzip/gzip_filter_integration_test.cc | 4 +- .../filters/http/gzip/gzip_filter_test.cc | 3 +- test/mocks/compression/decompressor/mocks.h | 2 +- 19 files changed, 343 insertions(+), 72 deletions(-) diff --git a/docs/root/configuration/http/http_filters/decompressor_filter.rst b/docs/root/configuration/http/http_filters/decompressor_filter.rst index 898f9d1f0fd6..eb71d1c2df66 100644 --- a/docs/root/configuration/http/http_filters/decompressor_filter.rst +++ b/docs/root/configuration/http/http_filters/decompressor_filter.rst @@ -110,3 +110,6 @@ with the following: not_decompressed, Counter, Number of request/responses not compressed. total_uncompressed_bytes, Counter, The total uncompressed bytes of all the request/responses that were marked for decompression. total_compressed_bytes, Counter, The total compressed bytes of all the request/responses that were marked for decompression. + +Additional stats for the decompressor library are rooted at +.decompressor...decompressor_library. diff --git a/include/envoy/compression/decompressor/factory.h b/include/envoy/compression/decompressor/factory.h index e0a38713b42f..8e3692f56ede 100644 --- a/include/envoy/compression/decompressor/factory.h +++ b/include/envoy/compression/decompressor/factory.h @@ -10,7 +10,7 @@ class DecompressorFactory { public: virtual ~DecompressorFactory() = default; - virtual DecompressorPtr createDecompressor() PURE; + virtual DecompressorPtr createDecompressor(const std::string& stats_prefix) PURE; virtual const std::string& statsPrefix() const PURE; // TODO(junr03): this method assumes that decompressors are used on http messages. // A more generic method might be `hint()` which gives the user of the decompressor a hint about diff --git a/source/extensions/compression/common/decompressor/factory_base.h b/source/extensions/compression/common/decompressor/factory_base.h index 98144e02e1b6..7bf3c1571f7f 100644 --- a/source/extensions/compression/common/decompressor/factory_base.h +++ b/source/extensions/compression/common/decompressor/factory_base.h @@ -17,7 +17,8 @@ class DecompressorLibraryFactoryBase Server::Configuration::FactoryContext& context) override { return createDecompressorFactoryFromProtoTyped( MessageUtil::downcastAndValidate(proto_config, - context.messageValidationVisitor())); + context.messageValidationVisitor()), + context); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -31,7 +32,8 @@ class DecompressorLibraryFactoryBase private: virtual Envoy::Compression::Decompressor::DecompressorFactoryPtr - createDecompressorFactoryFromProtoTyped(const ConfigProto&) PURE; + createDecompressorFactoryFromProtoTyped(const ConfigProto& proto_config, + Server::Configuration::FactoryContext& context) PURE; const std::string name_; }; diff --git a/source/extensions/compression/gzip/decompressor/BUILD b/source/extensions/compression/gzip/decompressor/BUILD index f31199e80811..9c86b64ef61b 100644 --- a/source/extensions/compression/gzip/decompressor/BUILD +++ b/source/extensions/compression/gzip/decompressor/BUILD @@ -16,6 +16,8 @@ envoy_cc_library( external_deps = ["zlib"], deps = [ "//include/envoy/compression/decompressor:decompressor_interface", + "//include/envoy/stats:stats_interface", + "//include/envoy/stats:stats_macros", "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", diff --git a/source/extensions/compression/gzip/decompressor/config.cc b/source/extensions/compression/gzip/decompressor/config.cc index bf73e3eba697..f2b845de1433 100644 --- a/source/extensions/compression/gzip/decompressor/config.cc +++ b/source/extensions/compression/gzip/decompressor/config.cc @@ -15,21 +15,24 @@ const uint32_t GzipHeaderValue = 16; } // namespace GzipDecompressorFactory::GzipDecompressorFactory( - const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip) - : window_bits_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(gzip, window_bits, DefaultWindowBits) | + const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip, Stats::Scope& scope) + : scope_(scope), + window_bits_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(gzip, window_bits, DefaultWindowBits) | GzipHeaderValue), chunk_size_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(gzip, chunk_size, DefaultChunkSize)) {} -Envoy::Compression::Decompressor::DecompressorPtr GzipDecompressorFactory::createDecompressor() { - auto decompressor = std::make_unique(chunk_size_); +Envoy::Compression::Decompressor::DecompressorPtr +GzipDecompressorFactory::createDecompressor(const std::string& stats_prefix) { + auto decompressor = std::make_unique(scope_, stats_prefix, chunk_size_); decompressor->init(window_bits_); return decompressor; } Envoy::Compression::Decompressor::DecompressorFactoryPtr GzipDecompressorLibraryFactory::createDecompressorFactoryFromProtoTyped( - const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config) { - return std::make_unique(proto_config); + const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config, + Server::Configuration::FactoryContext& context) { + return std::make_unique(proto_config, context.scope()); } /** diff --git a/source/extensions/compression/gzip/decompressor/config.h b/source/extensions/compression/gzip/decompressor/config.h index c2b8ded22562..34c9ca11bf57 100644 --- a/source/extensions/compression/gzip/decompressor/config.h +++ b/source/extensions/compression/gzip/decompressor/config.h @@ -25,16 +25,19 @@ const std::string& gzipExtensionName() { class GzipDecompressorFactory : public Envoy::Compression::Decompressor::DecompressorFactory { public: - GzipDecompressorFactory(const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip); + GzipDecompressorFactory(const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip, + Stats::Scope& scope); // Envoy::Compression::Decompressor::DecompressorFactory - Envoy::Compression::Decompressor::DecompressorPtr createDecompressor() override; + Envoy::Compression::Decompressor::DecompressorPtr + createDecompressor(const std::string& stats_prefix) override; const std::string& statsPrefix() const override { return gzipStatsPrefix(); } const std::string& contentEncoding() const override { return Http::CustomHeaders::get().ContentEncodingValues.Gzip; } private: + Stats::Scope& scope_; const int32_t window_bits_; const uint32_t chunk_size_; }; @@ -47,7 +50,8 @@ class GzipDecompressorLibraryFactory private: Envoy::Compression::Decompressor::DecompressorFactoryPtr createDecompressorFactoryFromProtoTyped( - const envoy::extensions::compression::gzip::decompressor::v3::Gzip& config) override; + const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config, + Server::Configuration::FactoryContext& context) override; }; DECLARE_FACTORY(GzipDecompressorLibraryFactory); diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc index 4a1ca6251098..9066af8f0426 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc @@ -1,5 +1,7 @@ #include "extensions/compression/gzip/decompressor/zlib_decompressor_impl.h" +#include + #include #include "envoy/common/exception.h" @@ -14,13 +16,17 @@ namespace Compression { namespace Gzip { namespace Decompressor { -ZlibDecompressorImpl::ZlibDecompressorImpl() : ZlibDecompressorImpl(4096) {} +ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix) + : ZlibDecompressorImpl(scope, stats_prefix, 4096) {} -ZlibDecompressorImpl::ZlibDecompressorImpl(uint64_t chunk_size) - : Zlib::Base(chunk_size, [](z_stream* z) { - inflateEnd(z); - delete z; - }) { +ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix, + uint64_t chunk_size) + : Zlib::Base(chunk_size, + [](z_stream* z) { + inflateEnd(z); + delete z; + }), + stats_(generateStats(stats_prefix, scope)) { zstream_ptr_->zalloc = Z_NULL; zstream_ptr_->zfree = Z_NULL; zstream_ptr_->opaque = Z_NULL; @@ -67,16 +73,40 @@ bool ZlibDecompressorImpl::inflateNext() { if (result < 0) { decompression_error_ = result; - ENVOY_LOG( - trace, - "zlib decompression error: {}. Error codes are defined in https://www.zlib.net/manual.html", - result); + ENVOY_LOG(trace, + "zlib decompression error: {}, msg: {}. Error codes are defined in " + "https://www.zlib.net/manual.html", + result, zstream_ptr_->msg); + chargeErrorStats(result); return false; } return true; } +void ZlibDecompressorImpl::chargeErrorStats(const int result) { + switch (result) { + case Z_ERRNO: + stats_.zlib_errno_.inc(); + break; + case Z_STREAM_ERROR: + stats_.zlib_stream_error_.inc(); + break; + case Z_DATA_ERROR: + stats_.zlib_data_error_.inc(); + break; + case Z_MEM_ERROR: + stats_.zlib_mem_error_.inc(); + break; + case Z_BUF_ERROR: + stats_.zlib_buf_error_.inc(); + break; + case Z_VERSION_ERROR: + stats_.zlib_version_error_.inc(); + break; + } +} + } // namespace Decompressor } // namespace Gzip } // namespace Compression diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h index a4f27adb5658..ec20b8c8dbca 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h @@ -1,6 +1,8 @@ #pragma once #include "envoy/compression/decompressor/decompressor.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" #include "common/common/logger.h" @@ -14,6 +16,24 @@ namespace Compression { namespace Gzip { namespace Decompressor { +/** + * All zlib decompressor stats. @see stats_macros.h + */ +#define ALL_ZLIB_DECOMPRESSOR_STATS(COUNTER) \ + COUNTER(zlib_errno) \ + COUNTER(zlib_stream_error) \ + COUNTER(zlib_data_error) \ + COUNTER(zlib_mem_error) \ + COUNTER(zlib_buf_error) \ + COUNTER(zlib_version_error) + +/** + * Struct definition for zlib decompressor stats. @see stats_macros.h + */ +struct ZlibDecompressorStats { + ALL_ZLIB_DECOMPRESSOR_STATS(GENERATE_COUNTER_STRUCT) +}; + /** * Implementation of decompressor's interface. */ @@ -21,7 +41,7 @@ class ZlibDecompressorImpl : public Zlib::Base, public Envoy::Compression::Decompressor::Decompressor, public Logger::Loggable { public: - ZlibDecompressorImpl(); + ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix); /** * Constructor that allows setting the size of decompressor's output buffer. It @@ -31,7 +51,7 @@ class ZlibDecompressorImpl : public Zlib::Base, * 256K bytes. @see http://zlib.net/zlib_how.html * @param chunk_size amount of memory reserved for the decompressor output. */ - ZlibDecompressorImpl(uint64_t chunk_size); + ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix, uint64_t chunk_size); /** * Init must be called in order to initialize the decompressor. Once decompressor is initialized, @@ -49,7 +69,19 @@ class ZlibDecompressorImpl : public Zlib::Base, int decompression_error_{0}; private: + // TODO: clean up friend class. This is here to allow coverage of chargeErrorStats as it isn't + // completely straightforward + // to cause zlib's inflate function to return all the error codes necessary to hit all the cases + // in the switch statement. + friend class ZlibDecompressorStatsTest; + static ZlibDecompressorStats generateStats(const std::string& prefix, Stats::Scope& scope) { + return ZlibDecompressorStats{ALL_ZLIB_DECOMPRESSOR_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; + } + bool inflateNext(); + void chargeErrorStats(const int result); + + const ZlibDecompressorStats stats_; }; } // namespace Decompressor diff --git a/source/extensions/filters/http/decompressor/decompressor_filter.cc b/source/extensions/filters/http/decompressor/decompressor_filter.cc index ae36a25457cd..1c5efe245dac 100644 --- a/source/extensions/filters/http/decompressor/decompressor_filter.cc +++ b/source/extensions/filters/http/decompressor/decompressor_filter.cc @@ -27,6 +27,7 @@ DecompressorFilterConfig::DecompressorFilterConfig( : stats_prefix_(fmt::format("{}decompressor.{}.{}", stats_prefix, proto_config.decompressor_library().name(), decompressor_factory->statsPrefix())), + decompressor_stats_prefix_(stats_prefix_ + "decompressor_library"), decompressor_factory_(std::move(decompressor_factory)), request_direction_config_(proto_config.request_direction_config(), stats_prefix_, scope, runtime), diff --git a/source/extensions/filters/http/decompressor/decompressor_filter.h b/source/extensions/filters/http/decompressor/decompressor_filter.h index ec6df0b35539..9dabae66f0aa 100644 --- a/source/extensions/filters/http/decompressor/decompressor_filter.h +++ b/source/extensions/filters/http/decompressor/decompressor_filter.h @@ -95,7 +95,7 @@ class DecompressorFilterConfig { Compression::Decompressor::DecompressorFactoryPtr decompressor_factory); Compression::Decompressor::DecompressorPtr makeDecompressor() { - return decompressor_factory_->createDecompressor(); + return decompressor_factory_->createDecompressor(decompressor_stats_prefix_); } const std::string& contentEncoding() { return decompressor_factory_->contentEncoding(); } const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; } @@ -103,6 +103,7 @@ class DecompressorFilterConfig { private: const std::string stats_prefix_; + const std::string decompressor_stats_prefix_; const Compression::Decompressor::DecompressorFactoryPtr decompressor_factory_; const RequestDirectionConfig request_direction_config_; const ResponseDirectionConfig response_direction_config_; diff --git a/test/extensions/compression/gzip/compressor_fuzz_test.cc b/test/extensions/compression/gzip/compressor_fuzz_test.cc index da76007c8989..bdaa5283e53a 100644 --- a/test/extensions/compression/gzip/compressor_fuzz_test.cc +++ b/test/extensions/compression/gzip/compressor_fuzz_test.cc @@ -1,5 +1,6 @@ #include "common/buffer/buffer_impl.h" #include "common/common/assert.h" +#include "common/stats/isolated_store_impl.h" #include "extensions/compression/gzip/compressor/zlib_compressor_impl.h" #include "extensions/compression/gzip/decompressor/zlib_decompressor_impl.h" @@ -19,9 +20,11 @@ namespace Fuzz { // trip compress-decompress pair; the decompressor itself is not fuzzed beyond // whatever the compressor emits, as it exists only as a test utility today. DEFINE_FUZZER(const uint8_t* buf, size_t len) { + FuzzedDataProvider provider(buf, len); ZlibCompressorImpl compressor; - Decompressor::ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store; + Decompressor::ZlibDecompressorImpl decompressor{stats_store, "test"}; // Select target compression level. We can't use ConsumeEnum() since the range // is non-contiguous. diff --git a/test/extensions/compression/gzip/decompressor/BUILD b/test/extensions/compression/gzip/decompressor/BUILD index 19520c24b545..bc732fc1a7c3 100644 --- a/test/extensions/compression/gzip/decompressor/BUILD +++ b/test/extensions/compression/gzip/decompressor/BUILD @@ -18,6 +18,7 @@ envoy_extension_cc_test( deps = [ "//source/common/common:assert_lib", "//source/common/common:hex_lib", + "//source/common/stats:isolated_store_lib", "//source/extensions/compression/gzip/compressor:compressor_lib", "//source/extensions/compression/gzip/decompressor:zlib_decompressor_impl_lib", "//test/test_common:utility_lib", diff --git a/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc b/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc index 87cd0ecf5b1a..782fd9af090e 100644 --- a/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc +++ b/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc @@ -1,5 +1,6 @@ #include "common/buffer/buffer_impl.h" #include "common/common/hex.h" +#include "common/stats/isolated_store_impl.h" #include "extensions/compression/gzip/compressor/zlib_compressor_impl.h" #include "extensions/compression/gzip/decompressor/zlib_decompressor_impl.h" @@ -13,7 +14,6 @@ namespace Extensions { namespace Compression { namespace Gzip { namespace Decompressor { -namespace { class ZlibDecompressorImplTest : public testing::Test { protected: @@ -46,7 +46,8 @@ class ZlibDecompressorImplTest : public testing::Test { drainBuffer(buffer); ASSERT_EQ(0, buffer.length()); - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; decompressor.init(window_bits); decompressor.decompress(accumulation_buffer, buffer); @@ -66,17 +67,20 @@ class ZlibDecompressorImplTest : public testing::Test { class ZlibDecompressorImplFailureTest : public ZlibDecompressorImplTest { protected: static void decompressorBadInitTestHelper(int64_t window_bits) { - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; decompressor.init(window_bits); } static void uninitializedDecompressorTestHelper() { Buffer::OwnedImpl input_buffer; Buffer::OwnedImpl output_buffer; - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; TestUtility::feedBufferWithRandomCharacters(input_buffer, 100); decompressor.decompress(input_buffer, output_buffer); ASSERT_TRUE(decompressor.decompression_error_ < 0); + ASSERT_EQ(stats_store.counterFromString("test.zlib_stream_error").value(), 1); } }; @@ -105,7 +109,8 @@ TEST_F(ZlibDecompressorImplTest, CallingChecksum) { compressor.compress(compressor_buffer, Envoy::Compression::Compressor::State::Flush); ASSERT_TRUE(compressor.checksum() > 0); - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; decompressor.init(gzip_window_bits); EXPECT_EQ(0, decompressor.checksum()); @@ -150,7 +155,8 @@ TEST_F(ZlibDecompressorImplTest, CompressAndDecompress) { drainBuffer(buffer); ASSERT_EQ(0, buffer.length()); - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; decompressor.init(gzip_window_bits); decompressor.decompress(accumulation_buffer, buffer); @@ -180,12 +186,14 @@ TEST_F(ZlibDecompressorImplTest, FailedDecompression) { accumulation_buffer.add(buffer); drainBuffer(buffer); } - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; decompressor.init(gzip_window_bits); decompressor.decompress(accumulation_buffer, buffer); ASSERT_TRUE(decompressor.decompression_error_ < 0); + ASSERT_EQ(stats_store.counterFromString("test.zlib_data_error").value(), 17); } // Exercises decompression with a very small output buffer. @@ -218,7 +226,8 @@ TEST_F(ZlibDecompressorImplTest, DecompressWithSmallOutputBuffer) { drainBuffer(buffer); ASSERT_EQ(0, buffer.length()); - ZlibDecompressorImpl decompressor(16); + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test.", 16}; decompressor.init(gzip_window_bits); decompressor.decompress(accumulation_buffer, buffer); @@ -284,7 +293,8 @@ TEST_F(ZlibDecompressorImplTest, CompressDecompressOfMultipleSlices) { compressor.compress(buffer, Envoy::Compression::Compressor::State::Flush); accumulation_buffer.add(buffer); - ZlibDecompressorImpl decompressor; + Stats::IsolatedStoreImpl stats_store{}; + ZlibDecompressorImpl decompressor{stats_store, "test."}; decompressor.init(gzip_window_bits); drainBuffer(buffer); @@ -298,7 +308,31 @@ TEST_F(ZlibDecompressorImplTest, CompressDecompressOfMultipleSlices) { EXPECT_EQ(original_text, decompressed_text); } -} // namespace +class ZlibDecompressorStatsTest : public testing::Test { +protected: + void chargeErrorStats(const int result) { decompressor_.chargeErrorStats(result); } + + Stats::IsolatedStoreImpl stats_store_{}; + ZlibDecompressorImpl decompressor_{stats_store_, "test."}; +}; + +TEST_F(ZlibDecompressorStatsTest, ChargeErrorStats) { + decompressor_.init(31); + + chargeErrorStats(Z_ERRNO); + ASSERT_EQ(stats_store_.counterFromString("test.zlib_errno").value(), 1); + chargeErrorStats(Z_STREAM_ERROR); + ASSERT_EQ(stats_store_.counterFromString("test.zlib_stream_error").value(), 1); + chargeErrorStats(Z_DATA_ERROR); + ASSERT_EQ(stats_store_.counterFromString("test.zlib_data_error").value(), 1); + chargeErrorStats(Z_MEM_ERROR); + ASSERT_EQ(stats_store_.counterFromString("test.zlib_mem_error").value(), 1); + chargeErrorStats(Z_BUF_ERROR); + ASSERT_EQ(stats_store_.counterFromString("test.zlib_buf_error").value(), 1); + chargeErrorStats(Z_VERSION_ERROR); + ASSERT_EQ(stats_store_.counterFromString("test.zlib_version_error").value(), 1); +} + } // namespace Decompressor } // namespace Gzip } // namespace Compression diff --git a/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc b/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc index 2c112f07eb6a..0ded467631a2 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc +++ b/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc @@ -97,7 +97,9 @@ class CompressorIntegrationTest : public testing::TestWithParamcompress(request_data1, Envoy::Compression::Compressor::State::Flush); auto compressed_request_length = request_data1.length(); codec_client_->sendData(*request_encoder, request_data1, false); // Send second data chunk upstream and finish the request stream. - Buffer::OwnedImpl request_data2(std::string(16384, 'a')); + Buffer::OwnedImpl request_data2; + TestUtility::feedBufferWithRandomCharacters(request_data2, 16384); uncompressed_request_length += request_data2.length(); request_compressor_->compress(request_data2, Envoy::Compression::Compressor::State::Finish); compressed_request_length += request_data2.length(); @@ -91,13 +93,12 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { // Assert that the total bytes received upstream equal the sum of the uncompressed byte buffers // sent. EXPECT_TRUE(upstream_request_->complete()); - TestUtility::headerMapEqualIgnoreOrder( - Http::TestRequestHeaderMapImpl{{":method", "POST"}, - {":scheme", "http"}, - {":path", "/test/long/url"}, - {":authority", "host"}, - {"accept-encoding", "wroong"}}, - upstream_request_->headers()); + EXPECT_EQ("chunked", upstream_request_->headers().TransferEncoding()->value().getStringView()); + EXPECT_EQ("gzip", upstream_request_->headers() + .get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView()); + EXPECT_EQ(nullptr, upstream_request_->headers().get(Http::LowerCaseString("content-encoding"))); EXPECT_EQ(uncompressed_request_length, upstream_request_->bodyLength()); // Verify stats @@ -117,14 +118,16 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-encoding", "gzip"}}, false); // Send first data chunk downstream. - Buffer::OwnedImpl response_data1(std::string(4096, 'a')); + Buffer::OwnedImpl response_data1; + TestUtility::feedBufferWithRandomCharacters(response_data1, 4096); auto uncompressed_response_length = response_data1.length(); response_compressor_->compress(response_data1, Envoy::Compression::Compressor::State::Flush); auto compressed_response_length = response_data1.length(); upstream_request_->encodeData(response_data1, false); // Send second data chunk downstream and finish the response stream. - Buffer::OwnedImpl response_data2(std::string(8192, 'a')); + Buffer::OwnedImpl response_data2; + TestUtility::feedBufferWithRandomCharacters(response_data2, 8192); uncompressed_response_length += response_data2.length(); response_compressor_->compress(response_data2, Envoy::Compression::Compressor::State::Flush); compressed_response_length += response_data2.length(); @@ -136,8 +139,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { // Assert that the total bytes received downstream equal the sum of the uncompressed byte buffers // sent. EXPECT_TRUE(response->complete()); - TestUtility::headerMapEqualIgnoreOrder(Http::TestRequestHeaderMapImpl{{":status", "200"}}, - response->headers()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ(uncompressed_response_length, response->body().length()); // Verify stats @@ -153,4 +155,106 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { uncompressed_response_length); } +/** + * Exercises gzip decompression bidirectionally with configuration using incompatible window bits + * resulting in an error. + */ +TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { + const std::string bad_config{R"EOF( + name: default_decompressor + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.decompressor.v3.Decompressor + decompressor_library: + name: testlib + typed_config: + "@type": type.googleapis.com/envoy.extensions.compression.gzip.decompressor.v3.Gzip + window_bits: 10 + )EOF"}; + // Use gzip for decompression. + initializeFilter(bad_config); + + // Enable request decompression by setting the Content-Encoding header to gzip. + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/test/long/url"}, + {":authority", "host"}, + {"content-encoding", "gzip"}}); + auto request_encoder = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + // Send first data chunk upstream. + Buffer::OwnedImpl request_data1; + TestUtility::feedBufferWithRandomCharacters(request_data1, 8192); + request_compressor_->compress(request_data1, Envoy::Compression::Compressor::State::Flush); + auto compressed_request_length = request_data1.length(); + codec_client_->sendData(*request_encoder, request_data1, false); + + // Send second data chunk upstream and finish the request stream. + Buffer::OwnedImpl request_data2; + TestUtility::feedBufferWithRandomCharacters(request_data2, 16384); + request_compressor_->compress(request_data2, Envoy::Compression::Compressor::State::Finish); + compressed_request_length += request_data2.length(); + codec_client_->sendData(*request_encoder, request_data2, true); + + // Wait for frames to arrive upstream. + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ("chunked", upstream_request_->headers().TransferEncoding()->value().getStringView()); + EXPECT_EQ("gzip", upstream_request_->headers() + .get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView()); + EXPECT_EQ(nullptr, upstream_request_->headers().get(Http::LowerCaseString("content-encoding"))); + + // Verify stats. While the stream was decompressed, there should be a decompression failure. + test_server_->waitForCounterEq("http.config_test.decompressor.testlib.gzip.request.decompressed", + 1); + test_server_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.request.not_decompressed", 0); + test_server_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.request.total_compressed_bytes", + compressed_request_length); + test_server_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 2); + + // Enable response decompression by setting the Content-Encoding header to gzip. + upstream_request_->encodeHeaders( + Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-encoding", "gzip"}}, false); + + // Send first data chunk downstream. + Buffer::OwnedImpl response_data1; + TestUtility::feedBufferWithRandomCharacters(response_data1, 4096); + response_compressor_->compress(response_data1, Envoy::Compression::Compressor::State::Flush); + auto compressed_response_length = response_data1.length(); + upstream_request_->encodeData(response_data1, false); + + // Send second data chunk downstream and finish the response stream. + Buffer::OwnedImpl response_data2; + TestUtility::feedBufferWithRandomCharacters(response_data2, 8192); + response_compressor_->compress(response_data2, Envoy::Compression::Compressor::State::Flush); + compressed_response_length += response_data2.length(); + upstream_request_->encodeData(response_data2, true); + + // Wait for frames to arrive downstream. + response->waitForEndStream(); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + + // Verify stats. While the stream was decompressed, there should be a decompression failure. + test_server_->waitForCounterEq("http.config_test.decompressor.testlib.gzip.response.decompressed", + 1); + test_server_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.response.not_decompressed", 0); + test_server_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", + compressed_response_length); + test_server_->waitForCounterGe( + "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); +} + } // namespace Envoy diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc index eb1f42426bbd..b903a9e8b7ee 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc @@ -102,7 +102,7 @@ class DecompressorFilterTest : public testing::TestWithParam { // Keep the decompressor to set expectations about it auto decompressor = std::make_unique(); auto* decompressor_ptr = decompressor.get(); - EXPECT_CALL(*decompressor_factory_, createDecompressor()) + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)) .WillOnce(Return(ByMove(std::move(decompressor)))); std::unique_ptr headers_after_filter = @@ -233,12 +233,12 @@ TEST_P(DecompressorFilterTest, DecompressionDisabled) { runtime_key: does_not_exist )EOF"); - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestRequestHeaderMapImpl headers_before_filter{{"content-encoding", "mock"}, {"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); expectNoDecompression(); } @@ -259,10 +259,15 @@ TEST_P(DecompressorFilterTest, RequestDecompressionDisabled) { {"content-length", "256"}}; if (isRequestDirection()) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + + // The request direction adds Accept-Encoding by default. Other than this header, the rest of + // the headers should be the same before and after the filter. + headers_after_filter->remove(Http::LowerCaseString("accept-encoding")); + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); + expectNoDecompression(); } else { decompressionActive(headers_before_filter, absl::nullopt /* expected_content_encoding*/, @@ -291,71 +296,102 @@ TEST_P(DecompressorFilterTest, ResponseDecompressionDisabled) { decompressionActive(headers_before_filter, absl::nullopt /* expected_content_encoding*/, absl::nullopt /* expected_accept_encoding */); } else { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); + expectNoDecompression(); } } TEST_P(DecompressorFilterTest, NoDecompressionHeadersOnly) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestRequestHeaderMapImpl headers_before_filter; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, true /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); } TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingAbsent) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestRequestHeaderMapImpl headers_before_filter{{"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Encoding by default. Other than this header, the rest of + // the headers should be the same before and after the filter. + headers_after_filter->remove(Http::LowerCaseString("accept-encoding")); + } + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); expectNoDecompression(); } TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingDoesNotMatch) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestRequestHeaderMapImpl headers_before_filter{{"content-encoding", "not-matching"}, {"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); expectNoDecompression(); } TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingNotCurrent) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); // The decompressor's content scheme is not the first value in the comma-delimited list in the // Content-Encoding header. Therefore, compression will not occur. Http::TestRequestHeaderMapImpl headers_before_filter{{"content-encoding", "gzip,mock"}, {"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Encoding by default. Other than this header, the rest of + // the headers should be the same before and after the filter. + headers_after_filter->remove(Http::LowerCaseString("accept-encoding")); + } + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); expectNoDecompression(); } TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresent) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestRequestHeaderMapImpl headers_before_filter{ {"cache-control", Http::CustomHeaders::get().CacheControlValues.NoTransform}, {"content-encoding", "mock"}, {"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Encoding by default. Other than this header, the rest of + // the headers should be the same before and after the filter. + headers_after_filter->remove(Http::LowerCaseString("accept-encoding")); + } + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); expectNoDecompression(); } TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresentInList) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestRequestHeaderMapImpl headers_before_filter{ {"cache-control", fmt::format("{}, {}", Http::CustomHeaders::get().CacheControlValues.NoCache, Http::CustomHeaders::get().CacheControlValues.NoTransform)}, @@ -363,7 +399,17 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresentInList) {"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter); + + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Encoding by default. Other than this header, the rest of + // the headers should be the same before and after the filter. + headers_after_filter->remove(Http::LowerCaseString("accept-encoding")); + } + EXPECT_THAT(headers_after_filter, HeaderMapEqualIgnoreOrder(&headers_before_filter)); expectNoDecompression(); } diff --git a/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc b/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc index d2b874fde742..8996e2aa0684 100644 --- a/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc +++ b/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc @@ -103,7 +103,9 @@ class GzipIntegrationTest : public testing::TestWithParam config_; std::unique_ptr filter_; Buffer::OwnedImpl data_; - Compression::Gzip::Decompressor::ZlibDecompressorImpl decompressor_; + Stats::IsolatedStoreImpl stats_store_; + Compression::Gzip::Decompressor::ZlibDecompressorImpl decompressor_{stats_store_, "test"}; Buffer::OwnedImpl decompressed_data_; std::string expected_str_; Stats::TestUtil::TestStore stats_; diff --git a/test/mocks/compression/decompressor/mocks.h b/test/mocks/compression/decompressor/mocks.h index 07ce0f6fa701..5910ab9336a8 100644 --- a/test/mocks/compression/decompressor/mocks.h +++ b/test/mocks/compression/decompressor/mocks.h @@ -25,7 +25,7 @@ class MockDecompressorFactory : public DecompressorFactory { ~MockDecompressorFactory() override; // Decompressor::DecompressorFactory - MOCK_METHOD(DecompressorPtr, createDecompressor, ()); + MOCK_METHOD(DecompressorPtr, createDecompressor, (const std::string&)); MOCK_METHOD(const std::string&, statsPrefix, (), (const)); MOCK_METHOD(const std::string&, contentEncoding, (), (const));