From ef3e73140dbd58edd0c85c379419d2049ead0a7b Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Wed, 6 Jan 2021 17:21:06 +0000 Subject: [PATCH 01/13] [gzip]: allow gzip to work w/ http backend w/o content-lengt and/or transfer encoding Signed-off-by: Nikita V. Shirokov --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 1 + .../compression/gzip/compressor/BUILD | 3 + .../filters/http/common/compressor/BUILD | 1 + .../http/common/compressor/compressor.cc | 25 +++-- .../extensions/filters/http/compressor/BUILD | 1 + .../filters/http/common/compressor/BUILD | 1 + .../compressor/compressor_filter_test.cc | 94 +++++++++++++++++-- test/integration/BUILD | 2 + .../integration/websocket_integration_test.cc | 9 ++ 10 files changed, 124 insertions(+), 14 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index cf656be3370f..2932be7bba2c 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -64,6 +64,7 @@ Removed Config or Runtime New Features ------------ +* compression: added new runtime guard `envoy.reloadable_features.enable_compression_without_chunked_header` (enabled by default) to control compression if no transfer-encoding=chunked header exists (now envoy would compress in such cases) to account for http/2 connections, where such transfer-encoding were removed. * compression: the :ref:`compressor ` filter adds support for compressing request payloads. Its configuration is unified with the :ref:`decompressor ` filter with two new fields for different directions - :ref:`requests ` and :ref:`responses `. The latter deprecates the old response-specific fields and, if used, roots the response-specific stats in `.compressor...response.*` instead of `.compressor...*`. * config: added ability to flush stats when the admin's :ref:`/stats endpoint ` is hit instead of on a timer via :ref:`stats_flush_on_admin `. * config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 34ba8d3a013e..4600455f3e46 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -66,6 +66,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.disallow_unbounded_access_logs", "envoy.reloadable_features.early_errors_via_hcm", + "envoy.reloadable_features.enable_compression_without_chunked_header", "envoy.reloadable_features.enable_dns_cache_circuit_breakers", "envoy.reloadable_features.fix_upgrade_response", "envoy.reloadable_features.fix_wildcard_matching", diff --git a/source/extensions/compression/gzip/compressor/BUILD b/source/extensions/compression/gzip/compressor/BUILD index e8918d1fcbc8..0581f3df1b8b 100644 --- a/source/extensions/compression/gzip/compressor/BUILD +++ b/source/extensions/compression/gzip/compressor/BUILD @@ -26,6 +26,9 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], + extra_visibility = [ + "//test:__subpackages__", + ], security_posture = "robust_to_untrusted_downstream", deps = [ ":compressor_lib", diff --git a/source/extensions/filters/http/common/compressor/BUILD b/source/extensions/filters/http/common/compressor/BUILD index a1c67b984a5e..ec0648006b5c 100644 --- a/source/extensions/filters/http/common/compressor/BUILD +++ b/source/extensions/filters/http/common/compressor/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "//include/envoy/stream_info:filter_state_interface", "//source/common/buffer:buffer_lib", "//source/common/http:header_map_lib", + "//source/common/http:utility_lib", "//source/common/protobuf", "//source/common/runtime:runtime_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index c14c42ed6eca..05145ad59a6e 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -2,6 +2,7 @@ #include "common/buffer/buffer_impl.h" #include "common/http/header_map_impl.h" +#include "common/http/utility.h" namespace Envoy { namespace Extensions { @@ -156,7 +157,8 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap } const auto& request_config = config_->requestDirectionConfig(); - if (!end_stream && request_config.compressionEnabled() && + if (!end_stream && request_config.compressionEnabled() && !Http::Utility::isUpgrade(headers) && + !Http::Utility::isH2UpgradeRequest(headers) && request_config.isMinimumContentLength(headers) && request_config.isContentTypeAllowed(headers) && !headers.getInline(request_content_encoding_handle.handle()) && @@ -221,10 +223,10 @@ Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMa const auto& config = config_->responseDirectionConfig(); const bool isEnabledAndContentLengthBigEnough = config.compressionEnabled() && config.isMinimumContentLength(headers); - const bool isCompressible = isEnabledAndContentLengthBigEnough && - config.isContentTypeAllowed(headers) && - !hasCacheControlNoTransform(headers) && isEtagAllowed(headers) && - !headers.getInline(response_content_encoding_handle.handle()); + const bool isCompressible = + isEnabledAndContentLengthBigEnough && !Http::Utility::isUpgrade(headers) && + config.isContentTypeAllowed(headers) && !hasCacheControlNoTransform(headers) && + isEtagAllowed(headers) && !headers.getInline(response_content_encoding_handle.handle()); if (!end_stream && isEnabledAndContentLengthBigEnough && isAcceptEncodingAllowed(headers) && isCompressible && isTransferEncodingAllowed(headers)) { sanitizeEtagHeader(headers); @@ -495,6 +497,10 @@ bool CompressorFilter::isEtagAllowed(Http::ResponseHeaderMap& headers) const { bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( const Http::RequestOrResponseHeaderMap& headers) const { + if (StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",", + Http::Headers::get().TransferEncodingValues.Chunked)) { + return true; + } const Http::HeaderEntry* content_length = headers.ContentLength(); if (content_length != nullptr) { uint64_t length; @@ -506,9 +512,12 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( } return is_minimum_content_length; } - - return StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",", - Http::Headers::get().TransferEncodingValues.Chunked); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_compression_without_chunked_header")) { + // returning true to account for HTTP/2 where content-length is optional + return true; + } + return false; } bool CompressorFilter::isTransferEncodingAllowed(Http::RequestOrResponseHeaderMap& headers) const { diff --git a/source/extensions/filters/http/compressor/BUILD b/source/extensions/filters/http/compressor/BUILD index 01855f8eb64a..072732920e77 100644 --- a/source/extensions/filters/http/compressor/BUILD +++ b/source/extensions/filters/http/compressor/BUILD @@ -27,6 +27,7 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], + extra_visibility = ["//test:__subpackages__"], security_posture = "robust_to_untrusted_downstream", deps = [ ":compressor_filter_lib", diff --git a/test/extensions/filters/http/common/compressor/BUILD b/test/extensions/filters/http/common/compressor/BUILD index a6b214dd6b50..7cb62eb6f4c3 100644 --- a/test/extensions/filters/http/common/compressor/BUILD +++ b/test/extensions/filters/http/common/compressor/BUILD @@ -21,6 +21,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/protobuf:protobuf_mocks", "//test/mocks/runtime:runtime_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/extensions/filters/http/compressor/v3:pkg_cc_proto", ], diff --git a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc index c8b4b79093e7..d35e570360eb 100644 --- a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc @@ -11,6 +11,7 @@ #include "test/mocks/protobuf/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/stats/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -77,8 +78,14 @@ class CompressorFilterTest : public testing::Test { } void verifyCompressedData() { - EXPECT_EQ(expected_str_.length(), stats_.counter("test.test.total_uncompressed_bytes").value()); - EXPECT_EQ(data_.length(), stats_.counter("test.test.total_compressed_bytes").value()); + EXPECT_EQ( + expected_str_.length(), + stats_.counter(fmt::format("test.test.{}total_uncompressed_bytes", response_stats_prefix_)) + .value()); + EXPECT_EQ( + data_.length(), + stats_.counter(fmt::format("test.test.{}total_compressed_bytes", response_stats_prefix_)) + .value()); } void populateBuffer(uint64_t size) { @@ -132,9 +139,6 @@ class CompressorFilterTest : public testing::Test { bool with_trailers) { uint64_t buffer_content_size; if (!absl::SimpleAtoi(headers.get_("content-length"), &buffer_content_size)) { - ASSERT_TRUE( - StringUtil::CaseInsensitiveCompare()(headers.get_("transfer-encoding"), "chunked")); - // In case of chunked stream just feed the buffer with 1000 bytes. buffer_content_size = 1000; } populateBuffer(buffer_content_size); @@ -156,7 +160,8 @@ class CompressorFilterTest : public testing::Test { EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(trailers)); } verifyCompressedData(); - EXPECT_EQ(1, stats_.counter("test.test.compressed").value()); + EXPECT_EQ( + 1, stats_.counter(fmt::format("test.test.{}compressed", response_stats_prefix_)).value()); } else { EXPECT_EQ("", headers.get_("content-encoding")); EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(data_, false)); @@ -233,6 +238,82 @@ TEST_F(CompressorFilterTest, CompressRequest) { doResponseNoCompression(headers); } +TEST_F(CompressorFilterTest, CompressRequestNoContentLength) { + setUpFilter(R"EOF( +{ + "request_direction_config": {}, + "compressor_library": { + "name": "test", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" + } + } +} +)EOF"); + doRequestCompression({{":method", "post"}}, false); + Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; + doResponseNoCompression(headers); +} + +TEST_F(CompressorFilterTest, CompressRequestNoContentLengthRuntimeDisabled) { + setUpFilter(R"EOF( +{ + "request_direction_config": {}, + "compressor_library": { + "name": "test", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" + } + } +} +)EOF"); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_compression_without_chunked_header", "false"}}); + doRequestNoCompression({{":method", "post"}}); + Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; + doResponseNoCompression(headers); +} + +TEST_F(CompressorFilterTest, CompressResponseNoContentLength) { + setUpFilter(R"EOF( +{ + "response_direction_config": {}, + "compressor_library": { + "name": "test", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" + } + } +} +)EOF"); + response_stats_prefix_ = "response."; + doRequestNoCompression({{":method", "get"}, {"accept-encoding", "deflate, test"}}); + Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; + doResponseCompression(headers, false); +} + +TEST_F(CompressorFilterTest, CompressResponseNoContentLengthRuntimeDisabled) { + setUpFilter(R"EOF( +{ + "response_direction_config": {}, + "compressor_library": { + "name": "test", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" + } + } +} +)EOF"); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_compression_without_chunked_header", "false"}}); + response_stats_prefix_ = "response."; + doRequestNoCompression({{":method", "get"}, {"accept-encoding", "deflate, test"}}); + Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; + doResponseNoCompression(headers); +} + TEST_F(CompressorFilterTest, CompressRequestWithTrailers) { setUpFilter(R"EOF( { @@ -556,6 +637,7 @@ INSTANTIATE_TEST_SUITE_P( std::make_tuple("transfer-encoding", "Chunked", "", true), std::make_tuple("transfer-encoding", "chunked", "\"content_length\": 500,", true), + std::make_tuple("", "", "\"content_length\": 500,", true), std::make_tuple("content-length", "501", "\"content_length\": 500,", true), std::make_tuple("content-length", "499", "\"content_length\": 500,", false))); diff --git a/test/integration/BUILD b/test/integration/BUILD index dc921e3a137e..50e6c35753e1 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -917,7 +917,9 @@ envoy_cc_test( ":http_protocol_integration_lib", "//source/common/http:header_map_lib", "//source/extensions/access_loggers/file:config", + "//source/extensions/compression/gzip/compressor:config", "//source/extensions/filters/http/buffer:config", + "//source/extensions/filters/http/compressor:config", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index d07a4aa76bfd..92288b326e9b 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -133,6 +133,15 @@ void WebsocketIntegrationTest::initialize() { [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); } + config_helper_.addFilter(R"EOF( +name: envoy.filters.http.compressor +typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor + compressor_library: + name: test + typed_config: + "@type": type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip +)EOF"); HttpProtocolIntegrationTest::initialize(); } From 8e5f8c5c7803857517ccc7571b767e32c784167f Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Thu, 7 Jan 2021 20:58:53 +0000 Subject: [PATCH 02/13] addressing comments 1. isUpgrade logic now runtime guarded as well 2. renaming runtime guard to enable_compression_without_content_length_header as it is now has nothing to do w/ chunked encoding 3. removed transfer-encoding=chunked logic (we stops to rely on it) 4. copy pasted websocket integration tests to compressor (only related to upgrade) Signed-off-by: Nikita V. Shirokov --- docs/root/version_history/current.rst | 2 +- source/common/runtime/runtime_features.cc | 2 +- .../compression/gzip/compressor/BUILD | 2 +- .../http/common/compressor/compressor.cc | 31 ++- .../extensions/filters/http/compressor/BUILD | 2 +- .../filters/http/common/compressor/BUILD | 19 ++ .../compressor/compressor_filter_test.cc | 36 +-- ...socket_with_compressor_integration_test.cc | 248 ++++++++++++++++++ ...bsocket_with_compressor_integration_test.h | 43 +++ test/integration/BUILD | 2 - .../integration/websocket_integration_test.cc | 9 - 11 files changed, 336 insertions(+), 60 deletions(-) create mode 100644 test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc create mode 100644 test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2932be7bba2c..49b0dc927996 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -64,7 +64,7 @@ Removed Config or Runtime New Features ------------ -* compression: added new runtime guard `envoy.reloadable_features.enable_compression_without_chunked_header` (enabled by default) to control compression if no transfer-encoding=chunked header exists (now envoy would compress in such cases) to account for http/2 connections, where such transfer-encoding were removed. +* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. * compression: the :ref:`compressor ` filter adds support for compressing request payloads. Its configuration is unified with the :ref:`decompressor ` filter with two new fields for different directions - :ref:`requests ` and :ref:`responses `. The latter deprecates the old response-specific fields and, if used, roots the response-specific stats in `.compressor...response.*` instead of `.compressor...*`. * config: added ability to flush stats when the admin's :ref:`/stats endpoint ` is hit instead of on a timer via :ref:`stats_flush_on_admin `. * config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 4600455f3e46..adfc14eb7170 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -66,7 +66,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.disallow_unbounded_access_logs", "envoy.reloadable_features.early_errors_via_hcm", - "envoy.reloadable_features.enable_compression_without_chunked_header", + "envoy.reloadable_features.enable_compression_without_content_length_header", "envoy.reloadable_features.enable_dns_cache_circuit_breakers", "envoy.reloadable_features.fix_upgrade_response", "envoy.reloadable_features.fix_wildcard_matching", diff --git a/source/extensions/compression/gzip/compressor/BUILD b/source/extensions/compression/gzip/compressor/BUILD index 0581f3df1b8b..632ef292ae40 100644 --- a/source/extensions/compression/gzip/compressor/BUILD +++ b/source/extensions/compression/gzip/compressor/BUILD @@ -27,7 +27,7 @@ envoy_cc_extension( srcs = ["config.cc"], hdrs = ["config.h"], extra_visibility = [ - "//test:__subpackages__", + "//test/extensions:__subpackages__", ], security_posture = "robust_to_untrusted_downstream", deps = [ diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index 05145ad59a6e..a7250638474e 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -157,8 +157,14 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap } const auto& request_config = config_->requestDirectionConfig(); - if (!end_stream && request_config.compressionEnabled() && !Http::Utility::isUpgrade(headers) && - !Http::Utility::isH2UpgradeRequest(headers) && + // temp variable to preserve old behavior w/ http upgrade. should be removed when + // enable_compression_without_content_length_header runtime variable would be removed + const bool allowUpgrade = + (!Http::Utility::isUpgrade(headers) && !Http::Utility::isH2UpgradeRequest(headers)) || + !Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_compression_without_content_length_header"); + + if (!end_stream && request_config.compressionEnabled() && allowUpgrade && request_config.isMinimumContentLength(headers) && request_config.isContentTypeAllowed(headers) && !headers.getInline(request_content_encoding_handle.handle()) && @@ -223,10 +229,17 @@ Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMa const auto& config = config_->responseDirectionConfig(); const bool isEnabledAndContentLengthBigEnough = config.compressionEnabled() && config.isMinimumContentLength(headers); - const bool isCompressible = - isEnabledAndContentLengthBigEnough && !Http::Utility::isUpgrade(headers) && - config.isContentTypeAllowed(headers) && !hasCacheControlNoTransform(headers) && - isEtagAllowed(headers) && !headers.getInline(response_content_encoding_handle.handle()); + // temp variable to preserve old behavior w/ http upgrade. should be removed when + // enable_compression_without_content_length_header runtime variable would be removed + const bool allowUpgrade = + !Http::Utility::isUpgrade(headers) || + !Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_compression_without_content_length_header"); + + const bool isCompressible = isEnabledAndContentLengthBigEnough && allowUpgrade && + config.isContentTypeAllowed(headers) && + !hasCacheControlNoTransform(headers) && isEtagAllowed(headers) && + !headers.getInline(response_content_encoding_handle.handle()); if (!end_stream && isEnabledAndContentLengthBigEnough && isAcceptEncodingAllowed(headers) && isCompressible && isTransferEncodingAllowed(headers)) { sanitizeEtagHeader(headers); @@ -497,10 +510,6 @@ bool CompressorFilter::isEtagAllowed(Http::ResponseHeaderMap& headers) const { bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( const Http::RequestOrResponseHeaderMap& headers) const { - if (StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",", - Http::Headers::get().TransferEncodingValues.Chunked)) { - return true; - } const Http::HeaderEntry* content_length = headers.ContentLength(); if (content_length != nullptr) { uint64_t length; @@ -513,7 +522,7 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( return is_minimum_content_length; } if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.enable_compression_without_chunked_header")) { + "envoy.reloadable_features.enable_compression_without_content_length_header")) { // returning true to account for HTTP/2 where content-length is optional return true; } diff --git a/source/extensions/filters/http/compressor/BUILD b/source/extensions/filters/http/compressor/BUILD index 072732920e77..dc56e00b81d1 100644 --- a/source/extensions/filters/http/compressor/BUILD +++ b/source/extensions/filters/http/compressor/BUILD @@ -27,7 +27,7 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], - extra_visibility = ["//test:__subpackages__"], + extra_visibility = ["//test/extensions:__subpackages__"], security_posture = "robust_to_untrusted_downstream", deps = [ ":compressor_filter_lib", diff --git a/test/extensions/filters/http/common/compressor/BUILD b/test/extensions/filters/http/common/compressor/BUILD index 7cb62eb6f4c3..25a0147a6ddd 100644 --- a/test/extensions/filters/http/common/compressor/BUILD +++ b/test/extensions/filters/http/common/compressor/BUILD @@ -47,6 +47,25 @@ envoy_cc_benchmark_binary( ], ) +envoy_cc_test( + name = "websocket_with_compressor_integration_test", + srcs = [ + "websocket_with_compressor_integration_test.cc", + "websocket_with_compressor_integration_test.h", + ], + deps = [ + "//source/common/http:header_map_lib", + "//source/extensions/access_loggers/file:config", + "//source/extensions/compression/gzip/compressor:config", + "//source/extensions/filters/http/buffer:config", + "//source/extensions/filters/http/compressor:config", + "//test/integration:http_protocol_integration_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + ], +) + envoy_benchmark_test( name = "compressor_filter_speed_test_benchmark_test", benchmark_binary = "compressor_filter_speed_test", diff --git a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc index d35e570360eb..6fecdfab884d 100644 --- a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc @@ -269,7 +269,7 @@ TEST_F(CompressorFilterTest, CompressRequestNoContentLengthRuntimeDisabled) { )EOF"); TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.enable_compression_without_chunked_header", "false"}}); + {{"envoy.reloadable_features.enable_compression_without_content_length_header", "false"}}); doRequestNoCompression({{":method", "post"}}); Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; doResponseNoCompression(headers); @@ -307,7 +307,7 @@ TEST_F(CompressorFilterTest, CompressResponseNoContentLengthRuntimeDisabled) { )EOF"); TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.enable_compression_without_chunked_header", "false"}}); + {{"envoy.reloadable_features.enable_compression_without_content_length_header", "false"}}); response_stats_prefix_ = "response."; doRequestNoCompression({{":method", "get"}, {"accept-encoding", "deflate, test"}}); Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; @@ -633,10 +633,6 @@ INSTANTIATE_TEST_SUITE_P( IsMinimumContentLengthTestSuite, IsMinimumContentLengthTest, testing::Values(std::make_tuple("content-length", "31", "", true), std::make_tuple("content-length", "29", "", false), - std::make_tuple("transfer-encoding", "chunked", "", true), - std::make_tuple("transfer-encoding", "Chunked", "", true), - std::make_tuple("transfer-encoding", "chunked", "\"content_length\": 500,", - true), std::make_tuple("", "", "\"content_length\": 500,", true), std::make_tuple("content-length", "501", "\"content_length\": 500,", true), std::make_tuple("content-length", "499", "\"content_length\": 500,", false))); @@ -666,34 +662,6 @@ TEST_P(IsMinimumContentLengthTest, Validate) { EXPECT_EQ(is_compression_expected, headers.has("vary")); } -class IsTransferEncodingAllowedTest - : public CompressorFilterTest, - public testing::WithParamInterface> {}; - -INSTANTIATE_TEST_SUITE_P( - IsTransferEncodingAllowedSuite, IsTransferEncodingAllowedTest, - testing::Values(std::make_tuple("transfer-encoding", "chunked", true), - std::make_tuple("transfer-encoding", "Chunked", true), - std::make_tuple("transfer-encoding", "deflate", false), - std::make_tuple("transfer-encoding", "Deflate", false), - std::make_tuple("transfer-encoding", "test", false), - std::make_tuple("transfer-encoding", "chunked, test", false), - std::make_tuple("transfer-encoding", "test, chunked", false), - std::make_tuple("transfer-encoding", "test\t, chunked\t", false), - std::make_tuple("x-garbage", "no_value", true))); - -TEST_P(IsTransferEncodingAllowedTest, Validate) { - const std::string& header_name = std::get<0>(GetParam()); - const std::string& header_value = std::get<1>(GetParam()); - const bool is_compression_expected = std::get<2>(GetParam()); - - doRequestNoCompression({{":method", "get"}, {"accept-encoding", "test"}}); - Http::TestResponseHeaderMapImpl headers{ - {":method", "get"}, {"content-length", "256"}, {header_name, header_value}}; - doResponse(headers, is_compression_expected, false); - EXPECT_EQ("Accept-Encoding", headers.get_("vary")); -} - class InsertVaryHeaderTest : public CompressorFilterTest, public testing::WithParamInterface> {}; diff --git a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc new file mode 100644 index 000000000000..28a493f28bac --- /dev/null +++ b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc @@ -0,0 +1,248 @@ +#include "test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h" + +#include + +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" + +#include "common/http/header_map_impl.h" +#include "common/protobuf/utility.h" + +#include "test/integration/utility.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/printers.h" +#include "test/test_common/utility.h" + +#include "absl/strings/str_cat.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +Http::TestRequestHeaderMapImpl upgradeRequestHeaders(const char* upgrade_type = "websocket", + uint32_t content_length = 0) { + return Http::TestRequestHeaderMapImpl{{":authority", "host"}, + {"content-length", fmt::format("{}", content_length)}, + {":path", "/websocket/test"}, + {":method", "GET"}, + {":scheme", "http"}, + {"upgrade", upgrade_type}, + {"connection", "keep-alive, upgrade"}}; +} + +Http::TestResponseHeaderMapImpl upgradeResponseHeaders(const char* upgrade_type = "websocket") { + return Http::TestResponseHeaderMapImpl{ + {":status", "101"}, {"connection", "upgrade"}, {"upgrade", upgrade_type}}; +} + +template +void commonValidate(ProxiedHeaders& proxied_headers, const OriginalHeaders& original_headers) { + // If no content length is specified, the HTTP1 codec will add a chunked encoding header. + if (original_headers.ContentLength() == nullptr && + proxied_headers.TransferEncoding() != nullptr) { + ASSERT_EQ(proxied_headers.getTransferEncodingValue(), "chunked"); + proxied_headers.removeTransferEncoding(); + } + if (proxied_headers.Connection() != nullptr && + proxied_headers.Connection()->value() == "upgrade" && + original_headers.Connection() != nullptr && + original_headers.Connection()->value() == "keep-alive, upgrade") { + // The keep-alive is implicit for HTTP/1.1, so Envoy only sets the upgrade + // header when converting from HTTP/1.1 to H2 + proxied_headers.setConnection("keep-alive, upgrade"); + } +} + +} // namespace + +void WebsocketWithCompressorIntegrationTest::validateUpgradeRequestHeaders( + const Http::RequestHeaderMap& original_proxied_request_headers, + const Http::RequestHeaderMap& original_request_headers) { + Http::TestRequestHeaderMapImpl proxied_request_headers(original_proxied_request_headers); + if (proxied_request_headers.ForwardedProto()) { + ASSERT_EQ(proxied_request_headers.getForwardedProtoValue(), "http"); + proxied_request_headers.removeForwardedProto(); + } + + // Check for and remove headers added by default for HTTP requests. + ASSERT_TRUE(proxied_request_headers.RequestId() != nullptr); + ASSERT_TRUE(proxied_request_headers.EnvoyExpectedRequestTimeoutMs() != nullptr); + proxied_request_headers.removeEnvoyExpectedRequestTimeoutMs(); + + if (proxied_request_headers.Scheme()) { + ASSERT_EQ(proxied_request_headers.getSchemeValue(), "http"); + } else { + proxied_request_headers.setScheme("http"); + } + + // 0 byte content lengths may be stripped on the H2 path - ignore that as a difference by adding + // it back to the proxied headers. + if (original_request_headers.ContentLength() && + proxied_request_headers.ContentLength() == nullptr) { + proxied_request_headers.setContentLength(size_t(0)); + } + + commonValidate(proxied_request_headers, original_request_headers); + proxied_request_headers.removeRequestId(); + + EXPECT_THAT(&proxied_request_headers, HeaderMapEqualIgnoreOrder(&original_request_headers)); +} + +void WebsocketWithCompressorIntegrationTest::validateUpgradeResponseHeaders( + const Http::ResponseHeaderMap& original_proxied_response_headers, + const Http::ResponseHeaderMap& original_response_headers) { + Http::TestResponseHeaderMapImpl proxied_response_headers(original_proxied_response_headers); + + // Check for and remove headers added by default for HTTP responses. + ASSERT_TRUE(proxied_response_headers.Date() != nullptr); + ASSERT_TRUE(proxied_response_headers.Server() != nullptr); + ASSERT_EQ(proxied_response_headers.getServerValue(), "envoy"); + proxied_response_headers.removeDate(); + proxied_response_headers.removeServer(); + + ASSERT_TRUE(proxied_response_headers.TransferEncoding() == nullptr); + + commonValidate(proxied_response_headers, original_response_headers); + + EXPECT_THAT(&proxied_response_headers, HeaderMapEqualIgnoreOrder(&original_response_headers)); +} + +INSTANTIATE_TEST_SUITE_P(Protocols, WebsocketWithCompressorIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +ConfigHelper::HttpModifierFunction setRouteUsingWebsocket() { + return [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { hcm.add_upgrade_configs()->set_upgrade_type("websocket"); }; +} + +void WebsocketWithCompressorIntegrationTest::initialize() { + if (upstreamProtocol() != FakeHttpConnection::Type::HTTP1) { + config_helper_.addConfigModifier( + [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + ConfigHelper::HttpProtocolOptions protocol_options; + protocol_options.mutable_explicit_http_config() + ->mutable_http2_protocol_options() + ->set_allow_connect(true); + ConfigHelper::setProtocolOptions( + *bootstrap.mutable_static_resources()->mutable_clusters(0), protocol_options); + }); + } + if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); + } + config_helper_.addFilter(R"EOF( +name: envoy.filters.http.compressor +typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor + compressor_library: + name: test + typed_config: + "@type": type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip +)EOF"); + HttpProtocolIntegrationTest::initialize(); +} + +void WebsocketWithCompressorIntegrationTest::performUpgrade( + const Http::TestRequestHeaderMapImpl& upgrade_request_headers, + const Http::TestResponseHeaderMapImpl& upgrade_response_headers) { + // Establish the initial connection. + codec_client_ = makeHttpConnection(lookupPort("http")); + + // Send websocket upgrade request + auto encoder_decoder = codec_client_->startRequest(upgrade_request_headers); + request_encoder_ = &encoder_decoder.first; + response_ = std::move(encoder_decoder.second); + test_server_->waitForCounterGe("http.config_test.downstream_cx_upgrades_total", 1); + test_server_->waitForGaugeGe("http.config_test.downstream_cx_upgrades_active", 1); + + // Verify the upgrade was received upstream. + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + validateUpgradeRequestHeaders(upstream_request_->headers(), upgrade_request_headers); + + // Send the upgrade response + upstream_request_->encodeHeaders(upgrade_response_headers, false); + + // Verify the upgrade response was received downstream. + response_->waitForHeaders(); + validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); +} + +void WebsocketWithCompressorIntegrationTest::sendBidirectionalData() { + // Verify that the client can still send data upstream, and that upstream + // receives it. + codec_client_->sendData(*request_encoder_, "hello", false); + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hello")); + + // Verify the upstream can send data to the client and that the client + // receives it. + upstream_request_->encodeData("world", false); + response_->waitForBodyData(5); + EXPECT_EQ("world", response_->body()); +} + +// Technically not a websocket tests, but verifies normal upgrades have parity +// with websocket upgrades +TEST_P(WebsocketWithCompressorIntegrationTest, NonWebsocketUpgrade) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + auto* foo_upgrade = hcm.add_upgrade_configs(); + foo_upgrade->set_upgrade_type("foo"); + }); + + config_helper_.addConfigModifier(setRouteUsingWebsocket()); + initialize(); + + performUpgrade(upgradeRequestHeaders("foo", 0), upgradeResponseHeaders("foo")); + sendBidirectionalData(); + codec_client_->sendData(*request_encoder_, "bye!", false); + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + codec_client_->close(); + } else { + codec_client_->sendReset(*request_encoder_); + } + + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hellobye!")); + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); + + auto upgrade_response_headers(upgradeResponseHeaders("foo")); + validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); + codec_client_->close(); +} + +TEST_P(WebsocketWithCompressorIntegrationTest, RouteSpecificUpgrade) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + auto* foo_upgrade = hcm.add_upgrade_configs(); + foo_upgrade->set_upgrade_type("foo"); + foo_upgrade->mutable_enabled()->set_value(false); + }); + auto host = config_helper_.createVirtualHost("host", "/websocket/test"); + host.mutable_routes(0)->mutable_route()->add_upgrade_configs()->set_upgrade_type("foo"); + config_helper_.addVirtualHost(host); + initialize(); + + performUpgrade(upgradeRequestHeaders("foo", 0), upgradeResponseHeaders("foo")); + sendBidirectionalData(); + codec_client_->sendData(*request_encoder_, "bye!", false); + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + codec_client_->close(); + } else { + codec_client_->sendReset(*request_encoder_); + } + + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hellobye!")); + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); + + auto upgrade_response_headers(upgradeResponseHeaders("foo")); + validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); + codec_client_->close(); +} + +} // namespace Envoy diff --git a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h new file mode 100644 index 000000000000..b84f700b0bb2 --- /dev/null +++ b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h @@ -0,0 +1,43 @@ +#pragma once + +#include "envoy/http/codec.h" + +#include "test/integration/http_protocol_integration.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +struct WebsocketWithCompressorProtocolTestParams { + Network::Address::IpVersion version; + Http::CodecClient::Type downstream_protocol; + FakeHttpConnection::Type upstream_protocol; +}; + +class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTest { +public: + void initialize() override; + +protected: + void performUpgrade(const Http::TestRequestHeaderMapImpl& upgrade_request_headers, + const Http::TestResponseHeaderMapImpl& upgrade_response_headers); + void sendBidirectionalData(); + + void validateUpgradeRequestHeaders(const Http::RequestHeaderMap& proxied_request_headers, + const Http::RequestHeaderMap& original_request_headers); + void validateUpgradeResponseHeaders(const Http::ResponseHeaderMap& proxied_response_headers, + const Http::ResponseHeaderMap& original_response_headers); + + ABSL_MUST_USE_RESULT + testing::AssertionResult waitForUpstreamDisconnectOrReset() { + if (upstreamProtocol() != FakeHttpConnection::Type::HTTP1) { + return upstream_request_->waitForReset(); + } else { + return fake_upstream_connection_->waitForDisconnect(); + } + } + + IntegrationStreamDecoderPtr response_; +}; + +} // namespace Envoy diff --git a/test/integration/BUILD b/test/integration/BUILD index 50e6c35753e1..dc921e3a137e 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -917,9 +917,7 @@ envoy_cc_test( ":http_protocol_integration_lib", "//source/common/http:header_map_lib", "//source/extensions/access_loggers/file:config", - "//source/extensions/compression/gzip/compressor:config", "//source/extensions/filters/http/buffer:config", - "//source/extensions/filters/http/compressor:config", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 92288b326e9b..d07a4aa76bfd 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -133,15 +133,6 @@ void WebsocketIntegrationTest::initialize() { [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); } - config_helper_.addFilter(R"EOF( -name: envoy.filters.http.compressor -typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor - compressor_library: - name: test - typed_config: - "@type": type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip -)EOF"); HttpProtocolIntegrationTest::initialize(); } From 3793491a0d3d2cf132918d4a960a1ed6d7ea8422 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Thu, 7 Jan 2021 23:15:32 +0000 Subject: [PATCH 03/13] removing unused test params struct Signed-off-by: Nikita V. Shirokov --- .../compressor/websocket_with_compressor_integration_test.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h index b84f700b0bb2..1139b8954629 100644 --- a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h +++ b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h @@ -8,12 +8,6 @@ namespace Envoy { -struct WebsocketWithCompressorProtocolTestParams { - Network::Address::IpVersion version; - Http::CodecClient::Type downstream_protocol; - FakeHttpConnection::Type upstream_protocol; -}; - class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTest { public: void initialize() override; From 459a21659e4c905ee226870eb43dfb7fe06a82d4 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Thu, 14 Jan 2021 21:47:15 +0000 Subject: [PATCH 04/13] fixing merge issue Signed-off-by: Nikita V. Shirokov --- docs/root/version_history/current.rst | 40 +-------------------------- 1 file changed, 1 insertion(+), 39 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 77a458708f98..62fde1d03955 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -34,46 +34,8 @@ Removed Config or Runtime New Features ------------ -* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. -* compression: the :ref:`compressor ` filter adds support for compressing request payloads. Its configuration is unified with the :ref:`decompressor ` filter with two new fields for different directions - :ref:`requests ` and :ref:`responses `. The latter deprecates the old response-specific fields and, if used, roots the response-specific stats in `.compressor...response.*` instead of `.compressor...*`. -* config: added ability to flush stats when the admin's :ref:`/stats endpoint ` is hit instead of on a timer via :ref:`stats_flush_on_admin `. -* config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features. -* formatter: added new :ref:`text_format_source ` field to support format strings both inline and from a file. -* grpc: implemented header value syntax support when defining :ref:`initial metadata ` for gRPC-based `ext_authz` :ref:`HTTP ` and :ref:`network ` filters, and :ref:`ratelimit ` filters. -* grpc-json: added support for configuring :ref:`unescaping behavior ` for path components. -* hds: added support for delta updates in the :ref:`HealthCheckSpecifier `, making only the Endpoints and Health Checkers that changed be reconstructed on receiving a new message, rather than the entire HDS. -* health_check: added option to use :ref:`no_traffic_healthy_interval ` which allows a different no traffic interval when the host is healthy. -* http: added HCM :ref:`timeout config field ` to control how long a downstream has to finish sending headers before the stream is cancelled. -* http: added frame flood and abuse checks to the upstream HTTP/2 codec. This check is off by default and can be enabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to true. -* http: added :ref:`stripping any port from host header ` support. -* http: clusters now support selecting HTTP/1 or HTTP/2 based on ALPN, configurable via :ref:`alpn_config ` in the :ref:`http_protocol_options ` message. -* jwt_authn: added support for :ref:`per-route config `. -* jwt_authn: changed config field :ref:`issuer ` to be optional to comply with JWT `RFC `_ requirements. -* kill_request: added new :ref:`HTTP kill request filter `. -* listener: added an optional :ref:`default filter chain `. If this field is supplied, and none of the :ref:`filter_chains ` matches, this default filter chain is used to serve the connection. -* listener: added back the :ref:`use_original_dst field `. -* log: added a new custom flag ``%_`` to the log pattern to print the actual message to log, but with escaped newlines. -* lua: added `downstreamDirectRemoteAddress()` and `downstreamLocalAddress()` APIs to :ref:`streamInfo() `. -* mongo_proxy: the list of commands to produce metrics for is now :ref:`configurable `. -* network: added a :ref:`timeout ` for incoming connections completing transport-level negotiation, including TLS and ALTS hanshakes. -* overload: add :ref:`envoy.overload_actions.reduce_timeouts ` overload action to enable scaling timeouts down with load. Scaling support :ref:`is limited ` to the HTTP connection and stream idle timeouts. -* ratelimit: added support for use of various :ref:`metadata ` as a ratelimit action. -* ratelimit: added :ref:`disable_x_envoy_ratelimited_header ` option to disable `X-Envoy-RateLimited` header. -* ratelimit: added :ref:`body ` field to support custom response bodies for non-OK responses from the external ratelimit service. -* router: added support for regex rewrites during HTTP redirects using :ref:`regex_rewrite `. -* sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for - :ref:`TlsCertificate ` and - :ref:`CertificateValidationContext `. -* signal: added an extension point for custom actions to run on the thread that has encountered a fatal error. Actions are configurable via :ref:`fatal_actions `. -* start_tls: :ref:`transport socket` which starts in clear-text but may programatically be converted to use tls. -* tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. -* thrift_proxy: added a new :ref: `payload_passthrough ` option to skip decoding body in the Thrift message. -* tls: added support for RSA certificates with 4096-bit keys in FIPS mode. -* tracing: added SkyWalking tracer. -* tracing: added support for setting the hostname used when sending spans to a Zipkin collector using the :ref:`collector_hostname ` field. -* xds: added support for resource TTLs. A TTL is specified on the :ref:`Resource `. For SotW, a :ref:`Resource ` can be embedded - in the list of resources to specify the TTL. * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). +* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. * http: added support for :ref:`:ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. * tcp_proxy: add support for converting raw TCP streams into HTTP/1.1 CONNECT requests. See :ref:`upgrade documentation ` for details. From 03976a45ab74202f2f9a3a2402de9ab26aa92283 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Fri, 15 Jan 2021 23:08:17 +0000 Subject: [PATCH 05/13] moving back transfer-encoding tests for non chunked case Signed-off-by: Nikita V. Shirokov --- .../compressor/compressor_filter_test.cc | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc index 6fecdfab884d..611a519d5f41 100644 --- a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc @@ -662,6 +662,32 @@ TEST_P(IsMinimumContentLengthTest, Validate) { EXPECT_EQ(is_compression_expected, headers.has("vary")); } +class IsTransferEncodingAllowedTest + : public CompressorFilterTest, + public testing::WithParamInterface> {}; + +INSTANTIATE_TEST_SUITE_P( + IsTransferEncodingAllowedSuite, IsTransferEncodingAllowedTest, + testing::Values(std::make_tuple("transfer-encoding", "deflate", false), + std::make_tuple("transfer-encoding", "Deflate", false), + std::make_tuple("transfer-encoding", "test", false), + std::make_tuple("transfer-encoding", "chunked, test", false), + std::make_tuple("transfer-encoding", "test, chunked", false), + std::make_tuple("transfer-encoding", "test\t, chunked\t", false), + std::make_tuple("x-garbage", "no_value", true))); + +TEST_P(IsTransferEncodingAllowedTest, Validate) { + const std::string& header_name = std::get<0>(GetParam()); + const std::string& header_value = std::get<1>(GetParam()); + const bool is_compression_expected = std::get<2>(GetParam()); + + doRequestNoCompression({{":method", "get"}, {"accept-encoding", "test"}}); + Http::TestResponseHeaderMapImpl headers{ + {":method", "get"}, {"content-length", "256"}, {header_name, header_value}}; + doResponse(headers, is_compression_expected, false); + EXPECT_EQ("Accept-Encoding", headers.get_("vary")); +} + class InsertVaryHeaderTest : public CompressorFilterTest, public testing::WithParamInterface> {}; From 3c10ef8e903a135cec3a6dd7b9e982b6a58393d8 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Wed, 20 Jan 2021 21:09:36 +0000 Subject: [PATCH 06/13] addressing comments 1. moving back TE:chunked logic (to make sure that w/ disabled runtime there is no change) 2. colapsing UTs for req/response into 1 3. removing route specific update from integration tests 4. removed extra_visibility for gzip Signed-off-by: Nikita V. Shirokov --- .../compression/gzip/compressor/BUILD | 3 -- .../http/common/compressor/compressor.cc | 12 +++--- .../compressor/compressor_filter_test.cc | 43 ++----------------- ...socket_with_compressor_integration_test.cc | 30 ------------- 4 files changed, 10 insertions(+), 78 deletions(-) diff --git a/source/extensions/compression/gzip/compressor/BUILD b/source/extensions/compression/gzip/compressor/BUILD index 632ef292ae40..e8918d1fcbc8 100644 --- a/source/extensions/compression/gzip/compressor/BUILD +++ b/source/extensions/compression/gzip/compressor/BUILD @@ -26,9 +26,6 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], - extra_visibility = [ - "//test/extensions:__subpackages__", - ], security_posture = "robust_to_untrusted_downstream", deps = [ ":compressor_lib", diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index a7250638474e..c9aed458b3df 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -159,12 +159,12 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap const auto& request_config = config_->requestDirectionConfig(); // temp variable to preserve old behavior w/ http upgrade. should be removed when // enable_compression_without_content_length_header runtime variable would be removed - const bool allowUpgrade = + const bool is_upgrade_allowed = (!Http::Utility::isUpgrade(headers) && !Http::Utility::isH2UpgradeRequest(headers)) || !Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header"); - if (!end_stream && request_config.compressionEnabled() && allowUpgrade && + if (!end_stream && request_config.compressionEnabled() && is_upgrade_allowed && request_config.isMinimumContentLength(headers) && request_config.isContentTypeAllowed(headers) && !headers.getInline(request_content_encoding_handle.handle()) && @@ -223,7 +223,6 @@ void CompressorFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallba StreamInfo::FilterState::StateType::Mutable); } } - Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { const auto& config = config_->responseDirectionConfig(); @@ -231,12 +230,12 @@ Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMa config.compressionEnabled() && config.isMinimumContentLength(headers); // temp variable to preserve old behavior w/ http upgrade. should be removed when // enable_compression_without_content_length_header runtime variable would be removed - const bool allowUpgrade = + const bool is_upgrade_allowed = !Http::Utility::isUpgrade(headers) || !Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header"); - const bool isCompressible = isEnabledAndContentLengthBigEnough && allowUpgrade && + const bool isCompressible = isEnabledAndContentLengthBigEnough && is_upgrade_allowed && config.isContentTypeAllowed(headers) && !hasCacheControlNoTransform(headers) && isEtagAllowed(headers) && !headers.getInline(response_content_encoding_handle.handle()); @@ -526,7 +525,8 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( // returning true to account for HTTP/2 where content-length is optional return true; } - return false; + return StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",", + Http::Headers::get().TransferEncodingValues.Chunked); } bool CompressorFilter::isTransferEncodingAllowed(Http::RequestOrResponseHeaderMap& headers) const { diff --git a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc index 611a519d5f41..37513d45a968 100644 --- a/test/extensions/filters/http/common/compressor/compressor_filter_test.cc +++ b/test/extensions/filters/http/common/compressor/compressor_filter_test.cc @@ -238,46 +238,10 @@ TEST_F(CompressorFilterTest, CompressRequest) { doResponseNoCompression(headers); } -TEST_F(CompressorFilterTest, CompressRequestNoContentLength) { +TEST_F(CompressorFilterTest, CompressRequestAndResponseNoContentLength) { setUpFilter(R"EOF( { "request_direction_config": {}, - "compressor_library": { - "name": "test", - "typed_config": { - "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" - } - } -} -)EOF"); - doRequestCompression({{":method", "post"}}, false); - Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; - doResponseNoCompression(headers); -} - -TEST_F(CompressorFilterTest, CompressRequestNoContentLengthRuntimeDisabled) { - setUpFilter(R"EOF( -{ - "request_direction_config": {}, - "compressor_library": { - "name": "test", - "typed_config": { - "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip" - } - } -} -)EOF"); - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.enable_compression_without_content_length_header", "false"}}); - doRequestNoCompression({{":method", "post"}}); - Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; - doResponseNoCompression(headers); -} - -TEST_F(CompressorFilterTest, CompressResponseNoContentLength) { - setUpFilter(R"EOF( -{ "response_direction_config": {}, "compressor_library": { "name": "test", @@ -288,14 +252,15 @@ TEST_F(CompressorFilterTest, CompressResponseNoContentLength) { } )EOF"); response_stats_prefix_ = "response."; - doRequestNoCompression({{":method", "get"}, {"accept-encoding", "deflate, test"}}); + doRequestCompression({{":method", "post"}, {"accept-encoding", "deflate, test"}}, false); Http::TestResponseHeaderMapImpl headers{{":status", "200"}}; doResponseCompression(headers, false); } -TEST_F(CompressorFilterTest, CompressResponseNoContentLengthRuntimeDisabled) { +TEST_F(CompressorFilterTest, CompressRequestAndResponseNoContentLengthRuntimeDisabled) { setUpFilter(R"EOF( { + "request_direction_config": {}, "response_direction_config": {}, "compressor_library": { "name": "test", diff --git a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc index 28a493f28bac..b06125cfd392 100644 --- a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc +++ b/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc @@ -215,34 +215,4 @@ TEST_P(WebsocketWithCompressorIntegrationTest, NonWebsocketUpgrade) { codec_client_->close(); } -TEST_P(WebsocketWithCompressorIntegrationTest, RouteSpecificUpgrade) { - config_helper_.addConfigModifier( - [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& - hcm) -> void { - auto* foo_upgrade = hcm.add_upgrade_configs(); - foo_upgrade->set_upgrade_type("foo"); - foo_upgrade->mutable_enabled()->set_value(false); - }); - auto host = config_helper_.createVirtualHost("host", "/websocket/test"); - host.mutable_routes(0)->mutable_route()->add_upgrade_configs()->set_upgrade_type("foo"); - config_helper_.addVirtualHost(host); - initialize(); - - performUpgrade(upgradeRequestHeaders("foo", 0), upgradeResponseHeaders("foo")); - sendBidirectionalData(); - codec_client_->sendData(*request_encoder_, "bye!", false); - if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { - codec_client_->close(); - } else { - codec_client_->sendReset(*request_encoder_); - } - - ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hellobye!")); - ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); - - auto upgrade_response_headers(upgradeResponseHeaders("foo")); - validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); - codec_client_->close(); -} - } // namespace Envoy From 1adea2c8340eea692944d2bfdef80730da933d5d Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Wed, 27 Jan 2021 20:24:34 +0000 Subject: [PATCH 07/13] adding integration test for connect proxy Signed-off-by: Nikita V. Shirokov --- docs/root/version_history/current.rst | 2 +- .../filters/http/common/compressor/BUILD | 6 +- ...est.cc => compressor_integration_tests.cc} | 76 ++++++++++++++++--- ..._test.h => compressor_integration_tests.h} | 11 +++ 4 files changed, 81 insertions(+), 14 deletions(-) rename test/extensions/filters/http/common/compressor/{websocket_with_compressor_integration_test.cc => compressor_integration_tests.cc} (78%) rename test/extensions/filters/http/common/compressor/{websocket_with_compressor_integration_test.h => compressor_integration_tests.h} (69%) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 5377ba22bc44..a9024eb57ab8 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -41,8 +41,8 @@ Removed Config or Runtime New Features ------------ * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). -* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES% and %RESPONSE_TRAILERS_BYTES%. +* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash. * http: added support for :ref:`:ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. diff --git a/test/extensions/filters/http/common/compressor/BUILD b/test/extensions/filters/http/common/compressor/BUILD index 25a0147a6ddd..6d984e2245ed 100644 --- a/test/extensions/filters/http/common/compressor/BUILD +++ b/test/extensions/filters/http/common/compressor/BUILD @@ -48,10 +48,10 @@ envoy_cc_benchmark_binary( ) envoy_cc_test( - name = "websocket_with_compressor_integration_test", + name = "compressor_integration_tests", srcs = [ - "websocket_with_compressor_integration_test.cc", - "websocket_with_compressor_integration_test.h", + "compressor_integration_tests.cc", + "compressor_integration_tests.h", ], deps = [ "//source/common/http:header_map_lib", diff --git a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc similarity index 78% rename from test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc rename to test/extensions/filters/http/common/compressor/compressor_integration_tests.cc index b06125cfd392..c3b805ef6f54 100644 --- a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.cc +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc @@ -1,4 +1,4 @@ -#include "test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h" +#include "test/extensions/filters/http/common/compressor/compressor_integration_tests.h" #include @@ -35,6 +35,16 @@ Http::TestResponseHeaderMapImpl upgradeResponseHeaders(const char* upgrade_type {":status", "101"}, {"connection", "upgrade"}, {"upgrade", upgrade_type}}; } +const std::string compressorFilterConfig = R"EOF( +name: envoy.filters.http.compressor +typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor + compressor_library: + name: test + typed_config: + "@type": type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip +)EOF"; + template void commonValidate(ProxiedHeaders& proxied_headers, const OriginalHeaders& original_headers) { // If no content length is specified, the HTTP1 codec will add a chunked encoding header. @@ -133,15 +143,7 @@ void WebsocketWithCompressorIntegrationTest::initialize() { [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); } - config_helper_.addFilter(R"EOF( -name: envoy.filters.http.compressor -typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor - compressor_library: - name: test - typed_config: - "@type": type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip -)EOF"); + config_helper_.addFilter(compressorFilterConfig); HttpProtocolIntegrationTest::initialize(); } @@ -215,4 +217,58 @@ TEST_P(WebsocketWithCompressorIntegrationTest, NonWebsocketUpgrade) { codec_client_->close(); } +INSTANTIATE_TEST_SUITE_P(Protocols, CompressorProxyingConnectIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +void CompressorProxyingConnectIntegrationTest::initialize() { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); + config_helper_.addFilter(compressorFilterConfig); + HttpProtocolIntegrationTest::initialize(); +} + +TEST_P(CompressorProxyingConnectIntegrationTest, ProxyConnect) { + initialize(); + + // Send request headers. + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(connect_headers_); + request_encoder_ = &encoder_decoder.first; + response_ = std::move(encoder_decoder.second); + + // Wait for them to arrive upstream. + AssertionResult result = + fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_); + RELEASE_ASSERT(result, result.message()); + result = fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_); + RELEASE_ASSERT(result, result.message()); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Method)[0]->value(), "CONNECT"); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + EXPECT_TRUE(upstream_request_->headers().get(Http::Headers::get().Protocol).empty()); + } else { + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Protocol)[0]->value(), + "bytestream"); + } + + // Send response headers + upstream_request_->encodeHeaders(default_response_headers_, false); + + // Wait for them to arrive downstream. + response_->waitForHeaders(); + EXPECT_EQ("200", response_->headers().getStatusValue()); + + // Make sure that even once the response has started, that data can continue to go upstream. + codec_client_->sendData(*request_encoder_, "hello", false); + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, 5)); + + // Also test upstream to downstream data. + upstream_request_->encodeData(12, false); + response_->waitForBodyData(12); + + cleanupUpstreamAndDownstream(); +} + } // namespace Envoy diff --git a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h b/test/extensions/filters/http/common/compressor/compressor_integration_tests.h similarity index 69% rename from test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h rename to test/extensions/filters/http/common/compressor/compressor_integration_tests.h index 1139b8954629..4e9436aaf980 100644 --- a/test/extensions/filters/http/common/compressor/websocket_with_compressor_integration_test.h +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.h @@ -34,4 +34,15 @@ class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTes IntegrationStreamDecoderPtr response_; }; +class CompressorProxyingConnectIntegrationTest : public HttpProtocolIntegrationTest { +public: + void initialize() override; + Http::TestRequestHeaderMapImpl connect_headers_{{":method", "CONNECT"}, + {":path", "/"}, + {":protocol", "bytestream"}, + {":scheme", "https"}, + {":authority", "host:80"}}; + IntegrationStreamDecoderPtr response_; +}; + } // namespace Envoy From 9bbbd80d92f50eb2b17b968bb66d9b1192374804 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Thu, 28 Jan 2021 18:21:13 +0000 Subject: [PATCH 08/13] addressing comments 1. s/is_upgrade_allowed/is_not_upgade/g 2. removed extra_visibility from BUILD Signed-off-by: Nikita V. Shirokov --- .../filters/http/common/compressor/compressor.cc | 8 ++++---- source/extensions/filters/http/compressor/BUILD | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index c9aed458b3df..cf2766207e67 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -159,12 +159,12 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap const auto& request_config = config_->requestDirectionConfig(); // temp variable to preserve old behavior w/ http upgrade. should be removed when // enable_compression_without_content_length_header runtime variable would be removed - const bool is_upgrade_allowed = + const bool is_not_upgrade = (!Http::Utility::isUpgrade(headers) && !Http::Utility::isH2UpgradeRequest(headers)) || !Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header"); - if (!end_stream && request_config.compressionEnabled() && is_upgrade_allowed && + if (!end_stream && request_config.compressionEnabled() && is_not_upgrade && request_config.isMinimumContentLength(headers) && request_config.isContentTypeAllowed(headers) && !headers.getInline(request_content_encoding_handle.handle()) && @@ -230,12 +230,12 @@ Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMa config.compressionEnabled() && config.isMinimumContentLength(headers); // temp variable to preserve old behavior w/ http upgrade. should be removed when // enable_compression_without_content_length_header runtime variable would be removed - const bool is_upgrade_allowed = + const bool is_not_upgrade = !Http::Utility::isUpgrade(headers) || !Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header"); - const bool isCompressible = isEnabledAndContentLengthBigEnough && is_upgrade_allowed && + const bool isCompressible = isEnabledAndContentLengthBigEnough && is_not_upgrade && config.isContentTypeAllowed(headers) && !hasCacheControlNoTransform(headers) && isEtagAllowed(headers) && !headers.getInline(response_content_encoding_handle.handle()); diff --git a/source/extensions/filters/http/compressor/BUILD b/source/extensions/filters/http/compressor/BUILD index dc56e00b81d1..01855f8eb64a 100644 --- a/source/extensions/filters/http/compressor/BUILD +++ b/source/extensions/filters/http/compressor/BUILD @@ -27,7 +27,6 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], - extra_visibility = ["//test/extensions:__subpackages__"], security_posture = "robust_to_untrusted_downstream", deps = [ ":compressor_filter_lib", From 775c5912d1115dc12019e43424983b0071627a43 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Thu, 28 Jan 2021 21:35:16 +0000 Subject: [PATCH 09/13] addressing comments 1. removed check in request path for http2 upgrade. as h2 codec translates it to http1 2. added compressed/not_compressed counters check in integration tests Signed-off-by: Nikita V. Shirokov --- .../http/common/compressor/compressor.cc | 4 ++-- .../compressor_integration_tests.cc | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index cf2766207e67..b4a69e47de95 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -160,7 +160,7 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap // temp variable to preserve old behavior w/ http upgrade. should be removed when // enable_compression_without_content_length_header runtime variable would be removed const bool is_not_upgrade = - (!Http::Utility::isUpgrade(headers) && !Http::Utility::isH2UpgradeRequest(headers)) || + !Http::Utility::isUpgrade(headers) || !Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header"); @@ -522,7 +522,7 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( } if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header")) { - // returning true to account for HTTP/2 where content-length is optional + // return true to ignore the minimum length configuration if no content-length header presents return true; } return StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",", diff --git a/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc index c3b805ef6f54..132cbd37e2c7 100644 --- a/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc @@ -39,6 +39,8 @@ const std::string compressorFilterConfig = R"EOF( name: envoy.filters.http.compressor typed_config: "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor + reque_direction_config: + response_direction_config: compressor_library: name: test typed_config: @@ -215,6 +217,26 @@ TEST_P(WebsocketWithCompressorIntegrationTest, NonWebsocketUpgrade) { auto upgrade_response_headers(upgradeResponseHeaders("foo")); validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); codec_client_->close(); + + auto request_compressed_counter = + test_server_->counter("http.config_test.compressor.test.gzip.request.compressed"); + ASSERT_NE(request_compressed_counter, nullptr); + ASSERT_EQ(1, request_compressed_counter->value()); + + auto request_uncompressed_counter = + test_server_->counter("http.config_test.compressor.test.gzip.request.not_compressed"); + ASSERT_NE(request_uncompressed_counter, nullptr); + ASSERT_EQ(1, request_uncompressed_counter->value()); + + auto response_compressed_counter = + test_server_->counter("http.config_test.compressor.test.gzip.compressed"); + ASSERT_NE(response_compressed_counter, nullptr); + ASSERT_EQ(0, response_compressed_counter->value()); + + auto response_uncompressed_counter = + test_server_->counter("http.config_test.compressor.test.gzip.not_compressed"); + ASSERT_NE(response_uncompressed_counter, nullptr); + ASSERT_EQ(1, response_uncompressed_counter->value()); } INSTANTIATE_TEST_SUITE_P(Protocols, CompressorProxyingConnectIntegrationTest, From 8641490c77c8f909ab3a382b1ebf914ceb5a371a Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Wed, 3 Feb 2021 00:24:01 +0000 Subject: [PATCH 10/13] addressing comments Signed-off-by: Nikita V. Shirokov --- .../filters/http/common/compressor/compressor.cc | 7 ++----- .../common/compressor/compressor_integration_tests.cc | 11 ++++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc index b4a69e47de95..7f9f5fe8e0a0 100644 --- a/source/extensions/filters/http/common/compressor/compressor.cc +++ b/source/extensions/filters/http/common/compressor/compressor.cc @@ -157,8 +157,6 @@ Http::FilterHeadersStatus CompressorFilter::decodeHeaders(Http::RequestHeaderMap } const auto& request_config = config_->requestDirectionConfig(); - // temp variable to preserve old behavior w/ http upgrade. should be removed when - // enable_compression_without_content_length_header runtime variable would be removed const bool is_not_upgrade = !Http::Utility::isUpgrade(headers) || !Runtime::runtimeFeatureEnabled( @@ -223,13 +221,12 @@ void CompressorFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallba StreamInfo::FilterState::StateType::Mutable); } } + Http::FilterHeadersStatus CompressorFilter::encodeHeaders(Http::ResponseHeaderMap& headers, bool end_stream) { const auto& config = config_->responseDirectionConfig(); const bool isEnabledAndContentLengthBigEnough = config.compressionEnabled() && config.isMinimumContentLength(headers); - // temp variable to preserve old behavior w/ http upgrade. should be removed when - // enable_compression_without_content_length_header runtime variable would be removed const bool is_not_upgrade = !Http::Utility::isUpgrade(headers) || !Runtime::runtimeFeatureEnabled( @@ -522,7 +519,7 @@ bool CompressorFilterConfig::DirectionConfig::isMinimumContentLength( } if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.enable_compression_without_content_length_header")) { - // return true to ignore the minimum length configuration if no content-length header presents + // return true to ignore the minimum length configuration if no content-length header is present return true; } return StringUtil::caseFindToken(headers.getTransferEncodingValue(), ",", diff --git a/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc index 132cbd37e2c7..69e4bce05806 100644 --- a/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc @@ -39,7 +39,7 @@ const std::string compressorFilterConfig = R"EOF( name: envoy.filters.http.compressor typed_config: "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor - reque_direction_config: + request_direction_config: response_direction_config: compressor_library: name: test @@ -221,7 +221,7 @@ TEST_P(WebsocketWithCompressorIntegrationTest, NonWebsocketUpgrade) { auto request_compressed_counter = test_server_->counter("http.config_test.compressor.test.gzip.request.compressed"); ASSERT_NE(request_compressed_counter, nullptr); - ASSERT_EQ(1, request_compressed_counter->value()); + ASSERT_EQ(0, request_compressed_counter->value()); auto request_uncompressed_counter = test_server_->counter("http.config_test.compressor.test.gzip.request.not_compressed"); @@ -284,11 +284,12 @@ TEST_P(CompressorProxyingConnectIntegrationTest, ProxyConnect) { // Make sure that even once the response has started, that data can continue to go upstream. codec_client_->sendData(*request_encoder_, "hello", false); - ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, 5)); + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hello")); // Also test upstream to downstream data. - upstream_request_->encodeData(12, false); - response_->waitForBodyData(12); + upstream_request_->encodeData("world", false); + response_->waitForBodyData(5); + EXPECT_EQ("world", response_->body()); cleanupUpstreamAndDownstream(); } From 8a66e8df177c53fdbcfcb204a658ab2954bbf0f6 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Wed, 3 Feb 2021 00:30:08 +0000 Subject: [PATCH 11/13] fixing merge Signed-off-by: Nikita V. Shirokov --- docs/root/version_history/current.rst | 3 +-- .../http/common/compressor/compressor_integration_tests.cc | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 9e3f48be51f9..2940e3b10e10 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -70,9 +70,8 @@ Removed Config or Runtime New Features ------------ * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). -* access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES% and %RESPONSE_TRAILERS_BYTES%. -* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES%, and %RESPONSE_TRAILERS_BYTES%. +* compression: extended the compression allow compressing when the content length header is not present. This behavior may be temporarily reverted by setting `envoy.reloadable_features.enable_compression_without_content_length_header` to false. * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash. * grpc_json_transcoder: added option :ref:`strict_http_request_validation ` to reject invalid requests early. * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route. diff --git a/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc index 69e4bce05806..1ee14acf3cd4 100644 --- a/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.cc @@ -246,7 +246,7 @@ INSTANTIATE_TEST_SUITE_P(Protocols, CompressorProxyingConnectIntegrationTest, void CompressorProxyingConnectIntegrationTest::initialize() { config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& - hcm) -> void { ConfigHelper::setConnectConfig(hcm, false); }); + hcm) -> void { ConfigHelper::setConnectConfig(hcm, false, false); }); config_helper_.addFilter(compressorFilterConfig); HttpProtocolIntegrationTest::initialize(); } From cc40eed24b5db752b3410fb7fe3b6f2852320396 Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Fri, 5 Mar 2021 21:00:53 +0000 Subject: [PATCH 12/13] adding comments about compression integration tests in .h Signed-off-by: Nikita V. Shirokov --- .../http/common/compressor/compressor_integration_tests.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/extensions/filters/http/common/compressor/compressor_integration_tests.h b/test/extensions/filters/http/common/compressor/compressor_integration_tests.h index 4e9436aaf980..766c7f011238 100644 --- a/test/extensions/filters/http/common/compressor/compressor_integration_tests.h +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.h @@ -8,6 +8,10 @@ namespace Envoy { +/* + Integration test to make sure that new logic, which does not care about content-length header +does not break websocket's upgrade method (by not adding any unexpected by remote side headers) +*/ class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTest { public: void initialize() override; @@ -34,6 +38,10 @@ class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTes IntegrationStreamDecoderPtr response_; }; +/* + Integration test to make sure that new logic, which does not care about content-length header +does not break proxying of connect method. +*/ class CompressorProxyingConnectIntegrationTest : public HttpProtocolIntegrationTest { public: void initialize() override; From b35654a18f23a05463f831643904b28fc06b67dc Mon Sep 17 00:00:00 2001 From: "Nikita V. Shirokov" Date: Thu, 11 Mar 2021 00:08:43 +0000 Subject: [PATCH 13/13] fixing comments to itests Signed-off-by: Nikita V. Shirokov --- .../http/common/compressor/compressor_integration_tests.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/http/common/compressor/compressor_integration_tests.h b/test/extensions/filters/http/common/compressor/compressor_integration_tests.h index 766c7f011238..43bd52e50d74 100644 --- a/test/extensions/filters/http/common/compressor/compressor_integration_tests.h +++ b/test/extensions/filters/http/common/compressor/compressor_integration_tests.h @@ -9,8 +9,8 @@ namespace Envoy { /* - Integration test to make sure that new logic, which does not care about content-length header -does not break websocket's upgrade method (by not adding any unexpected by remote side headers) +Test verifying that we don't add any unexpecting headers when we are trying to upgrade connection +to a websocket connection */ class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTest { public: @@ -39,8 +39,7 @@ class WebsocketWithCompressorIntegrationTest : public HttpProtocolIntegrationTes }; /* - Integration test to make sure that new logic, which does not care about content-length header -does not break proxying of connect method. +Test verifying that we don't break proxying of CONNECT method when compressor filter is enabled */ class CompressorProxyingConnectIntegrationTest : public HttpProtocolIntegrationTest { public: