From 5b4033e2f4af7f8a6c9ee81f5bca14105872c0e0 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 24 Jun 2020 10:59:34 -0700 Subject: [PATCH 01/20] wip Signed-off-by: Jose Nino --- .../common/decompressor/factory_base.h | 2 +- .../compression/gzip/decompressor/BUILD | 2 ++ .../decompressor/zlib_decompressor_impl.cc | 11 ++++---- .../decompressor/zlib_decompressor_impl.h | 25 +++++++++++++++++-- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/source/extensions/compression/common/decompressor/factory_base.h b/source/extensions/compression/common/decompressor/factory_base.h index 98144e02e1b62..81adb23633c0e 100644 --- a/source/extensions/compression/common/decompressor/factory_base.h +++ b/source/extensions/compression/common/decompressor/factory_base.h @@ -31,7 +31,7 @@ class DecompressorLibraryFactoryBase private: virtual Envoy::Compression::Decompressor::DecompressorFactoryPtr - createDecompressorFactoryFromProtoTyped(const ConfigProto&) PURE; + createDecompressorFactoryFromProtoTyped(const ConfigProto&, 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 f31199e80811b..0aa3282b096e4 100644 --- a/source/extensions/compression/gzip/decompressor/BUILD +++ b/source/extensions/compression/gzip/decompressor/BUILD @@ -20,6 +20,8 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "//source/extensions/compression/gzip/common:zlib_base_lib", + "//include/envoy/stats:stats_macros", + "//include/envoy/stats:stats_interface", ], ) diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc index 4a1ca62510980..3fed76e8d79ec 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc @@ -14,13 +14,13 @@ namespace Compression { namespace Gzip { namespace Decompressor { -ZlibDecompressorImpl::ZlibDecompressorImpl() : ZlibDecompressorImpl(4096) {} +ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope) : ZlibDecompressorImpl(scope, 4096) {} -ZlibDecompressorImpl::ZlibDecompressorImpl(uint64_t chunk_size) +ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, uint64_t chunk_size) : Zlib::Base(chunk_size, [](z_stream* z) { inflateEnd(z); delete z; - }) { + }), stats_(generateStats("zlib_decompressor", scope)) { zstream_ptr_->zalloc = Z_NULL; zstream_ptr_->zfree = Z_NULL; zstream_ptr_->opaque = Z_NULL; @@ -69,8 +69,9 @@ bool ZlibDecompressorImpl::inflateNext() { decompression_error_ = result; ENVOY_LOG( trace, - "zlib decompression error: {}. Error codes are defined in https://www.zlib.net/manual.html", - result); + "zlib decompression error: {}, msg: {}. Error codes are defined in https://www.zlib.net/manual.html", + result, zstream_ptr_->msg); + stats_.decompression_error_.inc(); return false; } diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h index a4f27adb56583..6447729f3f6b5 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/stats_macros.h" +#include "envoy/stats/scope.h" #include "common/common/logger.h" @@ -14,6 +16,19 @@ namespace Compression { namespace Gzip { namespace Decompressor { +/** + * All zlib decompressor stats. @see stats_macros.h + */ +#define ALL_ZLIB_DECOMPRESSOR_STATS(COUNTER) \ + COUNTER(decompression_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 +36,7 @@ class ZlibDecompressorImpl : public Zlib::Base, public Envoy::Compression::Decompressor::Decompressor, public Logger::Loggable { public: - ZlibDecompressorImpl(); + ZlibDecompressorImpl(Stats::Scope& scope); /** * Constructor that allows setting the size of decompressor's output buffer. It @@ -31,7 +46,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, uint64_t chunk_size); /** * Init must be called in order to initialize the decompressor. Once decompressor is initialized, @@ -49,7 +64,13 @@ class ZlibDecompressorImpl : public Zlib::Base, int decompression_error_{0}; private: + static ZlibDecompressorStats generateStats(const std::string& prefix, Stats::Scope& scope) { + return ZlibDecompressorStats{ALL_ZLIB_DECOMPRESSOR_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; + } + bool inflateNext(); + + const ZlibDecompressorStats stats_; }; } // namespace Decompressor From 15baceb2ab798c5c89d165a42772357c3af6400a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 25 Jun 2020 12:55:32 -0700 Subject: [PATCH 02/20] wired up Signed-off-by: Jose Nino --- .../compression/common/decompressor/factory_base.h | 4 ++-- .../extensions/compression/gzip/decompressor/config.cc | 10 +++++----- .../extensions/compression/gzip/decompressor/config.h | 5 +++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/source/extensions/compression/common/decompressor/factory_base.h b/source/extensions/compression/common/decompressor/factory_base.h index 81adb23633c0e..655d8da216a9a 100644 --- a/source/extensions/compression/common/decompressor/factory_base.h +++ b/source/extensions/compression/common/decompressor/factory_base.h @@ -17,7 +17,7 @@ class DecompressorLibraryFactoryBase Server::Configuration::FactoryContext& context) override { return createDecompressorFactoryFromProtoTyped( MessageUtil::downcastAndValidate(proto_config, - context.messageValidationVisitor())); + context.messageValidationVisitor()), context); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -31,7 +31,7 @@ class DecompressorLibraryFactoryBase private: virtual Envoy::Compression::Decompressor::DecompressorFactoryPtr - createDecompressorFactoryFromProtoTyped(const ConfigProto&, Server::Configuration::FactoryContext& context) PURE; + createDecompressorFactoryFromProtoTyped(const ConfigProto& proto_config, Server::Configuration::FactoryContext& context) PURE; const std::string name_; }; diff --git a/source/extensions/compression/gzip/decompressor/config.cc b/source/extensions/compression/gzip/decompressor/config.cc index bf73e3eba6976..6c8f29bc3436d 100644 --- a/source/extensions/compression/gzip/decompressor/config.cc +++ b/source/extensions/compression/gzip/decompressor/config.cc @@ -15,21 +15,21 @@ 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_); + auto decompressor = std::make_unique(scope_, 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 9a99398b23a4e..f4dec959cf163 100644 --- a/source/extensions/compression/gzip/decompressor/config.h +++ b/source/extensions/compression/gzip/decompressor/config.h @@ -25,7 +25,7 @@ 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; @@ -35,6 +35,7 @@ class GzipDecompressorFactory : public Envoy::Compression::Decompressor::Decompr } private: + Stats::Scope& scope_; const int32_t window_bits_; const uint32_t chunk_size_; }; @@ -47,7 +48,7 @@ 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); From 98c7fba374f861c55f7f7b414a6409dc4ef16b0a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 26 Jun 2020 16:12:03 -0700 Subject: [PATCH 03/20] green 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 | 4 +- .../compression/gzip/decompressor/config.cc | 11 +- .../compression/gzip/decompressor/config.h | 9 +- .../decompressor/zlib_decompressor_impl.cc | 24 ++-- .../decompressor/zlib_decompressor_impl.h | 15 +-- .../http/decompressor/decompressor_filter.h | 2 +- .../compression/gzip/decompressor/BUILD | 1 + .../zlib_decompressor_impl_test.cc | 27 ++-- .../decompressor_filter_integration_test.cc | 122 +++++++++++++++++- .../decompressor/decompressor_filter_test.cc | 20 +-- 13 files changed, 191 insertions(+), 55 deletions(-) diff --git a/docs/root/configuration/http/http_filters/decompressor_filter.rst b/docs/root/configuration/http/http_filters/decompressor_filter.rst index 898f9d1f0fd65..eb71d1c2df666 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 e0a38713b42fc..8e3692f56ede5 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 655d8da216a9a..7bf3c1571f7f6 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); + context.messageValidationVisitor()), + context); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -31,7 +32,8 @@ class DecompressorLibraryFactoryBase private: virtual Envoy::Compression::Decompressor::DecompressorFactoryPtr - createDecompressorFactoryFromProtoTyped(const ConfigProto& proto_config, Server::Configuration::FactoryContext& context) 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 0aa3282b096e4..9c86b64ef61b4 100644 --- a/source/extensions/compression/gzip/decompressor/BUILD +++ b/source/extensions/compression/gzip/decompressor/BUILD @@ -16,12 +16,12 @@ 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", "//source/extensions/compression/gzip/common:zlib_base_lib", - "//include/envoy/stats:stats_macros", - "//include/envoy/stats:stats_interface", ], ) diff --git a/source/extensions/compression/gzip/decompressor/config.cc b/source/extensions/compression/gzip/decompressor/config.cc index 6c8f29bc3436d..f2b845de14334 100644 --- a/source/extensions/compression/gzip/decompressor/config.cc +++ b/source/extensions/compression/gzip/decompressor/config.cc @@ -16,19 +16,22 @@ const uint32_t GzipHeaderValue = 16; GzipDecompressorFactory::GzipDecompressorFactory( 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) | + : 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(scope_, 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, Server::Configuration::FactoryContext& context) { + 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 f4dec959cf163..676a5ed21219e 100644 --- a/source/extensions/compression/gzip/decompressor/config.h +++ b/source/extensions/compression/gzip/decompressor/config.h @@ -25,10 +25,12 @@ const std::string& gzipExtensionName() { class GzipDecompressorFactory : public Envoy::Compression::Decompressor::DecompressorFactory { public: - GzipDecompressorFactory(const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip, Stats::Scope& scope); + 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::Headers::get().ContentEncodingValues.Gzip; @@ -48,7 +50,8 @@ class GzipDecompressorLibraryFactory private: Envoy::Compression::Decompressor::DecompressorFactoryPtr createDecompressorFactoryFromProtoTyped( - const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config, Server::Configuration::FactoryContext& context) 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 3fed76e8d79ec..16cbfbc1016d9 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc @@ -14,13 +14,17 @@ namespace Compression { namespace Gzip { namespace Decompressor { -ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope) : ZlibDecompressorImpl(scope, 4096) {} +ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix) + : ZlibDecompressorImpl(scope, stats_prefix, 4096) {} -ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, uint64_t chunk_size) - : Zlib::Base(chunk_size, [](z_stream* z) { - inflateEnd(z); - delete z; - }), stats_(generateStats("zlib_decompressor", scope)) { +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,10 +71,10 @@ bool ZlibDecompressorImpl::inflateNext() { if (result < 0) { decompression_error_ = result; - ENVOY_LOG( - trace, - "zlib decompression error: {}, msg: {}. Error codes are defined in https://www.zlib.net/manual.html", - result, zstream_ptr_->msg); + ENVOY_LOG(trace, + "zlib decompression error: {}, msg: {}. Error codes are defined in " + "https://www.zlib.net/manual.html", + result, zstream_ptr_->msg); stats_.decompression_error_.inc(); return false; } diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h index 6447729f3f6b5..7e0c121128ee9 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h @@ -1,8 +1,8 @@ #pragma once #include "envoy/compression/decompressor/decompressor.h" -#include "envoy/stats/stats_macros.h" #include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" #include "common/common/logger.h" @@ -19,8 +19,7 @@ namespace Decompressor { /** * All zlib decompressor stats. @see stats_macros.h */ -#define ALL_ZLIB_DECOMPRESSOR_STATS(COUNTER) \ - COUNTER(decompression_error) +#define ALL_ZLIB_DECOMPRESSOR_STATS(COUNTER) COUNTER(decompression_error) /** * Struct definition for zlib decompressor stats. @see stats_macros.h @@ -36,7 +35,7 @@ class ZlibDecompressorImpl : public Zlib::Base, public Envoy::Compression::Decompressor::Decompressor, public Logger::Loggable { public: - ZlibDecompressorImpl(Stats::Scope& scope); + ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix); /** * Constructor that allows setting the size of decompressor's output buffer. It @@ -46,7 +45,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(Stats::Scope& scope, 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, @@ -64,9 +63,9 @@ class ZlibDecompressorImpl : public Zlib::Base, int decompression_error_{0}; private: - static ZlibDecompressorStats generateStats(const std::string& prefix, Stats::Scope& scope) { - return ZlibDecompressorStats{ALL_ZLIB_DECOMPRESSOR_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; - } + static ZlibDecompressorStats generateStats(const std::string& prefix, Stats::Scope& scope) { + return ZlibDecompressorStats{ALL_ZLIB_DECOMPRESSOR_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; + } bool inflateNext(); diff --git a/source/extensions/filters/http/decompressor/decompressor_filter.h b/source/extensions/filters/http/decompressor/decompressor_filter.h index d7017e3857712..839165e1ec507 100644 --- a/source/extensions/filters/http/decompressor/decompressor_filter.h +++ b/source/extensions/filters/http/decompressor/decompressor_filter.h @@ -94,7 +94,7 @@ class DecompressorFilterConfig { Compression::Decompressor::DecompressorFactoryPtr decompressor_factory); Compression::Decompressor::DecompressorPtr makeDecompressor() { - return decompressor_factory_->createDecompressor(); + return decompressor_factory_->createDecompressor(stats_prefix_ + "decompressor_library"); } const std::string& contentEncoding() { return decompressor_factory_->contentEncoding(); } const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; } diff --git a/test/extensions/compression/gzip/decompressor/BUILD b/test/extensions/compression/gzip/decompressor/BUILD index 19520c24b5453..bc732fc1a7c37 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 87cd0ecf5b1ae..d1f2f9985a781 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" @@ -46,7 +47,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 +68,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.decompression_error").value(), 1); } }; @@ -105,7 +110,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 +156,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 +187,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.decompression_error").value(), 17); } // Exercises decompression with a very small output buffer. @@ -218,7 +227,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 +294,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); diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index efb8885695433..7849c1827708f 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -70,14 +70,16 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { auto response = std::move(encoder_decoder.second); // Send first data chunk upstream. - Buffer::OwnedImpl request_data1(std::string(8192, 'a')); + Buffer::OwnedImpl request_data1; + TestUtility::feedBufferWithRandomCharacters(request_data1, 8192); auto uncompressed_request_length = request_data1.length(); 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(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(); @@ -95,8 +97,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { Http::TestRequestHeaderMapImpl{{":method", "POST"}, {":scheme", "http"}, {":path", "/test/long/url"}, - {":authority", "host"}, - {"accept-encoding", "wroong"}}, + {":authority", "host"}}, upstream_request_->headers()); EXPECT_EQ(uncompressed_request_length, upstream_request_->bodyLength()); @@ -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(); @@ -153,4 +156,111 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { uncompressed_response_length); } +/** + * Exercises gzip decompression bidirectionally with default configuration. + */ +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); + auto uncompressed_request_length = request_data1.length(); + 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); + uncompressed_request_length += request_data2.length(); + 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()); + TestUtility::headerMapEqualIgnoreOrder( + Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/test/long/url"}, + {":authority", "host"}, + {"accept-encoding", "wroong"}}, + upstream_request_->headers()); + + // 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.decompression_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); + 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; + 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(); + upstream_request_->encodeData(response_data2, true); + + // Wait for frames to arrive downstream. + response->waitForEndStream(); + + EXPECT_TRUE(response->complete()); + TestUtility::headerMapEqualIgnoreOrder(Http::TestRequestHeaderMapImpl{{":status", "200"}}, + response->headers()); + + // 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_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.decompressor_library.decompression_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 aa7b43435fb88..3a04f52cb3670 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 = @@ -231,7 +231,7 @@ 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::TestHeaderMapImpl headers_before_filter{{"content-encoding", "mock"}, {"content-length", "256"}}; std::unique_ptr headers_after_filter = @@ -257,7 +257,7 @@ 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); @@ -289,7 +289,7 @@ 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); @@ -298,7 +298,7 @@ TEST_P(DecompressorFilterTest, ResponseDecompressionDisabled) { } TEST_P(DecompressorFilterTest, NoDecompressionHeadersOnly) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestHeaderMapImpl headers_before_filter; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, true /* end_stream */); @@ -306,7 +306,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionHeadersOnly) { } TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingAbsent) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestHeaderMapImpl headers_before_filter{{"content-length", "256"}}; std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); @@ -316,7 +316,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingAbsent) { } TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingDoesNotMatch) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestHeaderMapImpl headers_before_filter{{"content-encoding", "not-matching"}, {"content-length", "256"}}; std::unique_ptr headers_after_filter = @@ -327,7 +327,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingDoesNotMatch) { } 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::TestHeaderMapImpl headers_before_filter{{"content-encoding", "gzip,mock"}, @@ -340,7 +340,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingNotCurrent) { } TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresent) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestHeaderMapImpl headers_before_filter{ {"cache-control", Http::Headers::get().CacheControlValues.NoTransform}, {"content-encoding", "mock"}, @@ -353,7 +353,7 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresent) { } TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresentInList) { - EXPECT_CALL(*decompressor_factory_, createDecompressor()).Times(0); + EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); Http::TestHeaderMapImpl headers_before_filter{ {"cache-control", fmt::format("{}, {}", Http::Headers::get().CacheControlValues.NoCache, Http::Headers::get().CacheControlValues.NoTransform)}, From bd363927e0c84a03af6f9dc2e25bed8a0efd07f6 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 26 Jun 2020 16:20:00 -0700 Subject: [PATCH 04/20] fmt Signed-off-by: Jose Nino --- .../decompressor_filter_integration_test.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 7849c1827708f..83aa79aebb155 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -93,12 +93,11 @@ 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"}}, - upstream_request_->headers()); + TestUtility::headerMapEqualIgnoreOrder(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":scheme", "http"}, + {":path", "/test/long/url"}, + {":authority", "host"}}, + upstream_request_->headers()); EXPECT_EQ(uncompressed_request_length, upstream_request_->bodyLength()); // Verify stats @@ -160,7 +159,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { * Exercises gzip decompression bidirectionally with default configuration. */ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { - const std::string bad_config{R"EOF( + const std::string bad_config{R"EOF( name: default_decompressor typed_config: "@type": type.googleapis.com/envoy.extensions.filters.http.decompressor.v3.Decompressor From fbf4bc24b7017b7db1f078e5163565829d1056c8 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 26 Jun 2020 16:39:14 -0700 Subject: [PATCH 05/20] green Signed-off-by: Jose Nino --- test/extensions/compression/gzip/compressor_fuzz_test.cc | 5 ++++- .../http/compressor/compressor_filter_integration_test.cc | 3 ++- test/mocks/compression/decompressor/mocks.h | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/test/extensions/compression/gzip/compressor_fuzz_test.cc b/test/extensions/compression/gzip/compressor_fuzz_test.cc index da76007c89890..bdaa5283e53af 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/filters/http/compressor/compressor_filter_integration_test.cc b/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc index 33b976857c55d..c7802476e2801 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc +++ b/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc @@ -94,7 +94,8 @@ class CompressorIntegrationTest : public testing::TestWithParam Date: Fri, 26 Jun 2020 17:18:20 -0700 Subject: [PATCH 06/20] fmt Signed-off-by: Jose Nino --- .../http/compressor/compressor_filter_integration_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 274ac0eb778fb..d4bdcc2f11a82 100644 --- a/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc +++ b/test/extensions/filters/http/compressor/compressor_filter_integration_test.cc @@ -95,7 +95,8 @@ class CompressorIntegrationTest : public testing::TestWithParam Date: Sun, 28 Jun 2020 19:31:46 -0700 Subject: [PATCH 07/20] fix Signed-off-by: Jose Nino --- .../filters/http/gzip/gzip_filter_integration_test.cc | 4 +++- test/extensions/filters/http/gzip/gzip_filter_test.cc | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) 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 409fe39e5effb..5c4ea14363140 100644 --- a/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc +++ b/test/extensions/filters/http/gzip/gzip_filter_integration_test.cc @@ -100,7 +100,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_; From da5338a6fb5779aed4c819e950261772ffa0b1b1 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 10:56:03 -0700 Subject: [PATCH 08/20] clang-tidy Signed-off-by: Jose Nino --- .../http/decompressor/decompressor_filter_integration_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 83aa79aebb155..69b7969bab7d6 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -185,7 +185,6 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { // Send first data chunk upstream. Buffer::OwnedImpl request_data1; TestUtility::feedBufferWithRandomCharacters(request_data1, 8192); - auto uncompressed_request_length = request_data1.length(); 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); @@ -193,7 +192,6 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { // Send second data chunk upstream and finish the request stream. 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(); codec_client_->sendData(*request_encoder, request_data2, true); @@ -230,7 +228,6 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { // Send first data chunk downstream. 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); @@ -238,7 +235,6 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { // Send second data chunk downstream and finish the response stream. 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(); upstream_request_->encodeData(response_data2, true); From 3558b963de9233a323d93e10f7ec907731c6c0c3 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 15:44:54 -0700 Subject: [PATCH 09/20] comments Signed-off-by: Jose Nino --- .../decompressor/zlib_decompressor_impl.cc | 23 ++++++++++++++++++- .../decompressor/zlib_decompressor_impl.h | 8 ++++++- .../http/decompressor/decompressor_filter.cc | 1 + .../http/decompressor/decompressor_filter.h | 3 ++- .../zlib_decompressor_impl_test.cc | 4 ++-- .../decompressor_filter_integration_test.cc | 4 ++-- 6 files changed, 36 insertions(+), 7 deletions(-) diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc index 16cbfbc1016d9..ad52352b0f525 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" @@ -75,7 +77,26 @@ bool ZlibDecompressorImpl::inflateNext() { "zlib decompression error: {}, msg: {}. Error codes are defined in " "https://www.zlib.net/manual.html", result, zstream_ptr_->msg); - stats_.decompression_error_.inc(); + 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; + } return false; } diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h index 7e0c121128ee9..78ef24f512027 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h @@ -19,7 +19,13 @@ namespace Decompressor { /** * All zlib decompressor stats. @see stats_macros.h */ -#define ALL_ZLIB_DECOMPRESSOR_STATS(COUNTER) COUNTER(decompression_error) +#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 diff --git a/source/extensions/filters/http/decompressor/decompressor_filter.cc b/source/extensions/filters/http/decompressor/decompressor_filter.cc index 284b14fe841de..e45067f3ba490 100644 --- a/source/extensions/filters/http/decompressor/decompressor_filter.cc +++ b/source/extensions/filters/http/decompressor/decompressor_filter.cc @@ -17,6 +17,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 839165e1ec507..d307f29571399 100644 --- a/source/extensions/filters/http/decompressor/decompressor_filter.h +++ b/source/extensions/filters/http/decompressor/decompressor_filter.h @@ -94,7 +94,7 @@ class DecompressorFilterConfig { Compression::Decompressor::DecompressorFactoryPtr decompressor_factory); Compression::Decompressor::DecompressorPtr makeDecompressor() { - return decompressor_factory_->createDecompressor(stats_prefix_ + "decompressor_library"); + return decompressor_factory_->createDecompressor(decompressor_stats_prefix_); } const std::string& contentEncoding() { return decompressor_factory_->contentEncoding(); } const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; } @@ -102,6 +102,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/decompressor/zlib_decompressor_impl_test.cc b/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc index d1f2f9985a781..c78aa600e9805 100644 --- a/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc +++ b/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc @@ -81,7 +81,7 @@ class ZlibDecompressorImplFailureTest : public ZlibDecompressorImplTest { TestUtility::feedBufferWithRandomCharacters(input_buffer, 100); decompressor.decompress(input_buffer, output_buffer); ASSERT_TRUE(decompressor.decompression_error_ < 0); - ASSERT_EQ(stats_store.counterFromString("test.decompression_error").value(), 1); + ASSERT_EQ(stats_store.counterFromString("test.zlib_stream_error").value(), 1); } }; @@ -194,7 +194,7 @@ TEST_F(ZlibDecompressorImplTest, FailedDecompression) { decompressor.decompress(accumulation_buffer, buffer); ASSERT_TRUE(decompressor.decompression_error_ < 0); - ASSERT_EQ(stats_store.counterFromString("test.decompression_error").value(), 17); + ASSERT_EQ(stats_store.counterFromString("test.zlib_data_error").value(), 17); } // Exercises decompression with a very small output buffer. diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 69b7969bab7d6..3bbb0c02dd867 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -219,7 +219,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { "http.config_test.decompressor.testlib.gzip.request.total_compressed_bytes", compressed_request_length); test_server_->waitForCounterEq( - "http.config_test.decompressor.testlib.gzip.decompressor_library.decompression_error", 2); + "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( @@ -255,7 +255,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", compressed_response_length); test_server_->waitForCounterEq( - "http.config_test.decompressor.testlib.gzip.decompressor_library.decompression_error", 3); + "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); } } // namespace Envoy From b9f7925928658f6d645f61412ae9f4ee4da734d4 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 16:41:50 -0700 Subject: [PATCH 10/20] test ci Signed-off-by: Jose Nino --- .../decompressor/decompressor_filter_integration_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 3bbb0c02dd867..1e3c7bb877379 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -218,8 +218,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { 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); +// 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( @@ -254,8 +254,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { test_server_->waitForCounterEq( "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", compressed_response_length); - test_server_->waitForCounterEq( - "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); +// test_server_->waitForCounterEq( +// "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); } } // namespace Envoy From 3461fca7acb8472faf7c444581b0b0ad2ac6da60 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 17:03:21 -0700 Subject: [PATCH 11/20] fmt Signed-off-by: Jose Nino --- .../decompressor/decompressor_filter_integration_test.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 1e3c7bb877379..b9360231b701b 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -218,8 +218,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { 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); + // 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( @@ -254,8 +254,9 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { test_server_->waitForCounterEq( "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", compressed_response_length); -// test_server_->waitForCounterEq( -// "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); + // test_server_->waitForCounterEq( + // "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); + ASSERT_TRUE(false); } } // namespace Envoy From 02c34fd64ee2d7cb8c3da814cb523416884c97ec Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 17:17:49 -0700 Subject: [PATCH 12/20] add back Signed-off-by: Jose Nino --- .../decompressor/decompressor_filter_integration_test.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index b9360231b701b..3bbb0c02dd867 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -218,8 +218,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { 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); + 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( @@ -254,9 +254,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { test_server_->waitForCounterEq( "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", compressed_response_length); - // test_server_->waitForCounterEq( - // "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); - ASSERT_TRUE(false); + test_server_->waitForCounterEq( + "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); } } // namespace Envoy From bc382b02a3033ae8c22c06b48990deaa2068b99f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 17:21:25 -0700 Subject: [PATCH 13/20] why is this test hanging in CI but not locally? Signed-off-by: Jose Nino --- test/integration/server.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/integration/server.h b/test/integration/server.h index d584dd61631b8..067cd887db3ca 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -198,11 +198,13 @@ class NotifyingAllocatorImpl : public Stats::AllocatorImpl { virtual void waitForCounterFromStringEq(const std::string& name, uint64_t value) { absl::MutexLock l(&mutex_); - ENVOY_LOG_MISC(trace, "waiting for {} to be {}", name, value); + ENVOY_LOG_MISC(error, "waiting for {} to be {}", name, value); while (getCounterLockHeld(name) == nullptr || getCounterLockHeld(name)->value() != value) { + ENVOY_LOG_MISC(error, "but {} is currently {}", name, + getCounterLockHeld(name) == nullptr ? -1 : getCounterLockHeld(name)->value()); condvar_.Wait(&mutex_); } - ENVOY_LOG_MISC(trace, "done waiting for {} to be {}", name, value); + ENVOY_LOG_MISC(error, "done waiting for {} to be {}", name, value); } virtual void waitForCounterFromStringGe(const std::string& name, uint64_t value) { From 6c071411ca27bbddbef24547c7ac80cab3b74673 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 18:45:03 -0700 Subject: [PATCH 14/20] Revert "why is this test hanging in CI but not locally?" This reverts commit bc382b02a3033ae8c22c06b48990deaa2068b99f. Signed-off-by: Jose Nino --- test/integration/server.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/integration/server.h b/test/integration/server.h index 067cd887db3ca..d584dd61631b8 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -198,13 +198,11 @@ class NotifyingAllocatorImpl : public Stats::AllocatorImpl { virtual void waitForCounterFromStringEq(const std::string& name, uint64_t value) { absl::MutexLock l(&mutex_); - ENVOY_LOG_MISC(error, "waiting for {} to be {}", name, value); + ENVOY_LOG_MISC(trace, "waiting for {} to be {}", name, value); while (getCounterLockHeld(name) == nullptr || getCounterLockHeld(name)->value() != value) { - ENVOY_LOG_MISC(error, "but {} is currently {}", name, - getCounterLockHeld(name) == nullptr ? -1 : getCounterLockHeld(name)->value()); condvar_.Wait(&mutex_); } - ENVOY_LOG_MISC(error, "done waiting for {} to be {}", name, value); + ENVOY_LOG_MISC(trace, "done waiting for {} to be {}", name, value); } virtual void waitForCounterFromStringGe(const std::string& name, uint64_t value) { From cf0f7485ad63c9629edabd9e47c5383fe1188d18 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 18:45:52 -0700 Subject: [PATCH 15/20] facepalm Signed-off-by: Jose Nino --- .../http/decompressor/decompressor_filter_integration_test.cc | 2 +- test/test_common/utility.cc | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 3bbb0c02dd867..e1bd2c8e5b670 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -255,7 +255,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", compressed_response_length); test_server_->waitForCounterEq( - "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); + "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 4); } } // namespace Envoy diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 36e266db8e7ce..6cccd00e08ed2 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -149,6 +149,10 @@ Stats::TextReadoutSharedPtr TestUtility::findTextReadout(Stats::Store& store, void TestUtility::waitForCounterEq(Stats::Store& store, const std::string& name, uint64_t value, Event::TestTimeSystem& time_system) { + ENVOY_LOG_MISC(error, "here"); + for (auto counter : store.counters()) { + std::cout << counter->name() << " " << counter->value() << std::endl; + } while (findCounter(store, name) == nullptr || findCounter(store, name)->value() != value) { time_system.advanceTimeWait(std::chrono::milliseconds(10)); } From 4ce4063b6c10f0b88329350dd71c7156815fe7e1 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 29 Jun 2020 18:47:44 -0700 Subject: [PATCH 16/20] crud Signed-off-by: Jose Nino --- test/test_common/utility.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 6cccd00e08ed2..36e266db8e7ce 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -149,10 +149,6 @@ Stats::TextReadoutSharedPtr TestUtility::findTextReadout(Stats::Store& store, void TestUtility::waitForCounterEq(Stats::Store& store, const std::string& name, uint64_t value, Event::TestTimeSystem& time_system) { - ENVOY_LOG_MISC(error, "here"); - for (auto counter : store.counters()) { - std::cout << counter->name() << " " << counter->value() << std::endl; - } while (findCounter(store, name) == nullptr || findCounter(store, name)->value() != value) { time_system.advanceTimeWait(std::chrono::milliseconds(10)); } From b7e7458ffd50d7f5f2eaeb0580efbbf74ff9acfa Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 6 Jul 2020 17:36:53 -0700 Subject: [PATCH 17/20] comments Signed-off-by: Jose Nino --- .../decompressor/zlib_decompressor_impl.cc | 44 ++++++++++--------- .../decompressor/zlib_decompressor_impl.h | 6 +++ .../zlib_decompressor_impl_test.cc | 27 +++++++++++- .../decompressor_filter_integration_test.cc | 33 +++++++------- .../decompressor/decompressor_filter_test.cc | 14 +++--- 5 files changed, 76 insertions(+), 48 deletions(-) diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc index ad52352b0f525..9066af8f0426f 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc @@ -77,32 +77,36 @@ bool ZlibDecompressorImpl::inflateNext() { "zlib decompression error: {}, msg: {}. Error codes are defined in " "https://www.zlib.net/manual.html", result, zstream_ptr_->msg); - 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; - } + 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 78ef24f512027..ec20b8c8dbcaa 100644 --- a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h +++ b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h @@ -69,11 +69,17 @@ 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_; }; 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 c78aa600e9805..782fd9af090e8 100644 --- a/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc +++ b/test/extensions/compression/gzip/decompressor/zlib_decompressor_impl_test.cc @@ -14,7 +14,6 @@ namespace Extensions { namespace Compression { namespace Gzip { namespace Decompressor { -namespace { class ZlibDecompressorImplTest : public testing::Test { protected: @@ -309,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/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index e1bd2c8e5b670..6c698440641f8 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -93,11 +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"}}, - 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 @@ -138,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 @@ -156,7 +156,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompression) { } /** - * Exercises gzip decompression bidirectionally with default configuration. + * 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( @@ -202,13 +203,12 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); 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"))); // Verify stats. While the stream was decompressed, there should be a decompression failure. test_server_->waitForCounterEq("http.config_test.decompressor.testlib.gzip.request.decompressed", @@ -243,8 +243,7 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); - TestUtility::headerMapEqualIgnoreOrder(Http::TestRequestHeaderMapImpl{{":status", "200"}}, - response->headers()); + 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", diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc index 00267387731bd..3195b92316c0c 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc @@ -238,7 +238,7 @@ TEST_P(DecompressorFilterTest, DecompressionDisabled) { {"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(); } @@ -262,7 +262,6 @@ TEST_P(DecompressorFilterTest, RequestDecompressionDisabled) { 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); expectNoDecompression(); } else { decompressionActive(headers_before_filter, absl::nullopt /* expected_content_encoding*/, @@ -294,7 +293,9 @@ TEST_P(DecompressorFilterTest, ResponseDecompressionDisabled) { 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(); } } @@ -304,7 +305,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionHeadersOnly) { 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) { @@ -312,7 +313,6 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingAbsent) { 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); expectNoDecompression(); } @@ -323,7 +323,6 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingDoesNotMatch) { {"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(); } @@ -336,7 +335,6 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingNotCurrent) { {"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(); } @@ -349,7 +347,6 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresent) { {"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(); } @@ -363,7 +360,6 @@ 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); expectNoDecompression(); } From d2b753f0998e307be5f10eb479e515078d4986cc Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 7 Jul 2020 10:18:17 -0700 Subject: [PATCH 18/20] make less flaky Signed-off-by: Jose Nino --- .../http/decompressor/decompressor_filter_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc index 6c698440641f8..29600b3180180 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc @@ -253,8 +253,8 @@ TEST_P(DecompressorIntegrationTest, BidirectionalDecompressionError) { test_server_->waitForCounterEq( "http.config_test.decompressor.testlib.gzip.response.total_compressed_bytes", compressed_response_length); - test_server_->waitForCounterEq( - "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 4); + test_server_->waitForCounterGe( + "http.config_test.decompressor.testlib.gzip.decompressor_library.zlib_data_error", 3); } } // namespace Envoy From 469e75f756b701fa4affa90be73c2358794cb89e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jul 2020 16:14:47 -0700 Subject: [PATCH 19/20] comment Signed-off-by: Jose Nino --- .../decompressor/decompressor_filter_test.cc | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc index 3195b92316c0c..86a1acda72bd2 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc @@ -262,6 +262,12 @@ TEST_P(DecompressorFilterTest, RequestDecompressionDisabled) { EXPECT_CALL(*decompressor_factory_, createDecompressor(_)).Times(0); std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); + + // The request direction adds Accept-Enconding 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*/, @@ -314,6 +320,17 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingAbsent) { std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Enconding 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(); } @@ -336,6 +353,17 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingNotCurrent) { std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Enconding 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(); } @@ -348,6 +376,17 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresent) { std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Enconding 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(); } @@ -361,6 +400,17 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresentInList) std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); + if (isRequestDirection()) { + ASSERT_EQ(headers_after_filter->get(Http::LowerCaseString("accept-encoding")) + ->value() + .getStringView(), + "mock"); + // The request direction adds Accept-Enconding 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(); } From 3e7be62575bd0ed1287acd453eab901241f29ad4 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jul 2020 16:15:36 -0700 Subject: [PATCH 20/20] comment Signed-off-by: Jose Nino --- .../http/decompressor/decompressor_filter_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc index 86a1acda72bd2..be4b313ce0d0b 100644 --- a/test/extensions/filters/http/decompressor/decompressor_filter_test.cc +++ b/test/extensions/filters/http/decompressor/decompressor_filter_test.cc @@ -263,7 +263,7 @@ TEST_P(DecompressorFilterTest, RequestDecompressionDisabled) { std::unique_ptr headers_after_filter = doHeaders(headers_before_filter, false /* end_stream */); - // The request direction adds Accept-Enconding by default. Other than this header, the rest of + // 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)); @@ -325,7 +325,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingAbsent) { ->value() .getStringView(), "mock"); - // The request direction adds Accept-Enconding by default. Other than this header, the rest of + // 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")); } @@ -358,7 +358,7 @@ TEST_P(DecompressorFilterTest, NoDecompressionContentEncodingNotCurrent) { ->value() .getStringView(), "mock"); - // The request direction adds Accept-Enconding by default. Other than this header, the rest of + // 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")); } @@ -381,7 +381,7 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresent) { ->value() .getStringView(), "mock"); - // The request direction adds Accept-Enconding by default. Other than this header, the rest of + // 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")); } @@ -405,7 +405,7 @@ TEST_P(DecompressorFilterTest, NoResponseDecompressionNoTransformPresentInList) ->value() .getStringView(), "mock"); - // The request direction adds Accept-Enconding by default. Other than this header, the rest of + // 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")); }