From 5632fb7946aca5f50ac47af44ad5b1a569943922 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Wed, 15 Jan 2020 01:30:17 -0500 Subject: [PATCH 01/16] Accumulate H1 request and response bodies in a buffer during parse, and do a single dispatch call for the buffered body either after the http_parser_execute call completes or after the last byte of the body is processed. Signed-off-by: Antonio Vicente --- source/common/http/http1/codec_impl.cc | 66 +++++++++---- source/common/http/http1/codec_impl.h | 14 ++- test/common/http/http1/codec_impl_test.cc | 115 +++++++++++++++++++--- 3 files changed, 160 insertions(+), 35 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 309cdab5e70fb..9eaba30184267 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -388,15 +388,19 @@ http_parser_settings ConnectionImpl::settings_{ return static_cast(parser->data)->onHeadersCompleteBase(); }, [](http_parser* parser, const char* at, size_t length) -> int { - static_cast(parser->data)->onBody(at, length); + static_cast(parser->data)->bufferBody(at, length); return 0; }, [](http_parser* parser) -> int { static_cast(parser->data)->onMessageCompleteBase(); return 0; }, - nullptr, // on_chunk_header - nullptr // on_chunk_complete + [](http_parser* parser) -> int { + const bool is_final_chunk = (parser->content_length == 0); + static_cast(parser->data)->onChunkHeader(is_final_chunk); + return 0; + }, + nullptr // on_chunk_complete }; ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, @@ -450,21 +454,16 @@ bool ConnectionImpl::maybeDirectDispatch(Buffer::Instance& data) { return false; } - ssize_t total_parsed = 0; - uint64_t num_slices = data.getRawSlices(nullptr, 0); - absl::FixedArray slices(num_slices); - data.getRawSlices(slices.begin(), num_slices); - for (const Buffer::RawSlice& slice : slices) { - total_parsed += slice.len_; - onBody(static_cast(slice.mem_), slice.len_); - } - ENVOY_CONN_LOG(trace, "direct-dispatched {} bytes", connection_, total_parsed); - data.drain(total_parsed); + ENVOY_CONN_LOG(trace, "direct-dispatched {} bytes", connection_, data.length()); + onBody(data); + data.drain(data.length()); + return true; } void ConnectionImpl::dispatch(Buffer::Instance& data) { ENVOY_CONN_LOG(trace, "parsing {} bytes", connection_, data.length()); + ASSERT(pending_body_buffer_.length() == 0); if (maybeDirectDispatch(data)) { return; @@ -480,10 +479,15 @@ void ConnectionImpl::dispatch(Buffer::Instance& data) { data.getRawSlices(slices.begin(), num_slices); for (const Buffer::RawSlice& slice : slices) { total_parsed += dispatchSlice(static_cast(slice.mem_), slice.len_); + if (HTTP_PARSER_ERRNO(&parser_) != HPE_OK) { + break; + } } + dispatchBody(); } else { dispatchSlice(nullptr, 0); } + ASSERT(pending_body_buffer_.length() == 0); ENVOY_CONN_LOG(trace, "parsed {} bytes", connection_, total_parsed); data.drain(total_parsed); @@ -624,9 +628,32 @@ int ConnectionImpl::onHeadersCompleteBase() { return handling_upgrade_ ? 2 : rc; } +void ConnectionImpl::bufferBody(const char* data, size_t length) { + pending_body_buffer_.add(data, length); +} + +void ConnectionImpl::dispatchBody() { + ASSERT(HTTP_PARSER_ERRNO(&parser_) == HPE_OK || HTTP_PARSER_ERRNO(&parser_) == HPE_PAUSED); + if (pending_body_buffer_.length() > 0) { + onBody(pending_body_buffer_); + // Clear the buffer. + pending_body_buffer_.drain(pending_body_buffer_.length()); + } +} + +void ConnectionImpl::onChunkHeader(bool is_final_chunk) { + if (is_final_chunk) { + // Dispatch body before parsing trailers, so body ends up dispatched even if an error is found + // while processing trailers. + dispatchBody(); + } +} + void ConnectionImpl::onMessageCompleteBase() { ENVOY_CONN_LOG(trace, "message complete", connection_); + dispatchBody(); + if (handling_upgrade_) { // If this is an upgrade request, swallow the onMessageComplete. The // upgrade payload will be treated as stream body. @@ -815,12 +842,11 @@ void ServerConnectionImpl::onUrl(const char* data, size_t length) { } } -void ServerConnectionImpl::onBody(const char* data, size_t length) { +void ServerConnectionImpl::onBody(Buffer::Instance& data) { ASSERT(!deferred_end_stream_headers_); if (active_request_.has_value()) { - ENVOY_CONN_LOG(trace, "body size={}", connection_, length); - Buffer::OwnedImpl buffer(data, length); - active_request_.value().request_decoder_->decodeData(buffer, false); + ENVOY_CONN_LOG(trace, "body size={}", connection_, data.length()); + active_request_.value().request_decoder_->decodeData(data, false); } } @@ -953,12 +979,10 @@ int ClientConnectionImpl::onHeadersComplete() { return cannotHaveBody() ? 1 : 0; } -void ClientConnectionImpl::onBody(const char* data, size_t length) { +void ClientConnectionImpl::onBody(Buffer::Instance& data) { ASSERT(!deferred_end_stream_headers_); if (pending_response_.has_value()) { - Buffer::OwnedImpl buffer; - buffer.add(data, length); - pending_response_.value().decoder_->decodeData(buffer, false); + pending_response_.value().decoder_->decodeData(data, false); } } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index c613a8221ca10..170336362e25c 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -285,7 +285,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable decoder; EXPECT_CALL(callbacks_, newStream(_, _)) .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); EXPECT_CALL(decoder, decodeHeaders_(_, false)); if (enable_trailers) { - EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, false)); EXPECT_CALL(decoder, decodeTrailers_); } else { - EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, false)); EXPECT_CALL(decoder, decodeData(_, true)); } - Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello " - "World\r\n0\r\nhello: world\r\nsecond: header\r\n\r\n"); + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\nhello: world\r\nsecond: header\r\n\r\n"); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); } @@ -191,9 +201,9 @@ void Http1ServerConnectionImplTest::testTrailersExceedLimit(std::string trailer_ if (enable_trailers) { EXPECT_CALL(decoder, decodeHeaders_(_, false)); - EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, false)).Times(1); } else { - EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, false)).Times(1); EXPECT_CALL(decoder, decodeData(_, true)); } @@ -310,8 +320,68 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBody) { EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); EXPECT_CALL(decoder, decodeData(_, true)).Times(1); - Buffer::OwnedImpl buffer( - "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"); + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); +} + +TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplit) { + initialize(); + + InSequence sequence; + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "POST"}, + {"transfer-encoding", "chunked"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + Buffer::OwnedImpl expected_data1("Hello Worl"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + Buffer::OwnedImpl expected_data2("d"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)).Times(1); + EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorl"); + Buffer::OwnedImpl buffer2("d\r\n" + "0\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + codec_->dispatch(buffer2); + EXPECT_EQ(0U, buffer2.length()); +} + +TEST_F(Http1ServerConnectionImplTest, ChunkedBodyFragmentedBuffer) { + initialize(); + + InSequence sequence; + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "POST"}, + {"transfer-encoding", "chunked"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + Buffer::OwnedImpl expected_data("Hello World"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + + Buffer::OwnedImpl buffer = + createBufferWithOneByteSlices("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n"); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); } @@ -506,7 +576,7 @@ TEST_F(Http1ServerConnectionImplTest, Http11InvalidTrailerPost) { .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); EXPECT_CALL(decoder, decodeHeaders_(_, false)); - EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, false)).Times(1); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\n" "Host: host\r\n" @@ -899,6 +969,29 @@ TEST_F(Http1ServerConnectionImplTest, PostWithContentLength) { EXPECT_EQ(0U, buffer.length()); } +TEST_F(Http1ServerConnectionImplTest, PostWithContentLengthFragmentedBuffer) { + initialize(); + + InSequence sequence; + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{{"content-length", "5"}, {":path", "/"}, {":method", "POST"}}; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + + Buffer::OwnedImpl expected_data1("12345"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + + Buffer::OwnedImpl expected_data2; + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), true)).Times(1); + + Buffer::OwnedImpl buffer = + createBufferWithOneByteSlices("POST / HTTP/1.1\r\ncontent-length: 5\r\n\r\n12345"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); +} + TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponse) { initialize(); From f8e5d34f422ad892ca1c515b636ba82128825c5c Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 16 Mar 2020 15:18:43 -0400 Subject: [PATCH 02/16] add test for invalid chunk header processing add missing comments rename buffered_body_ and associated methods Signed-off-by: Antonio Vicente --- source/common/http/http1/codec_impl.cc | 21 ++++++++++---------- source/common/http/http1/codec_impl.h | 21 ++++++++++++++------ test/common/http/http1/codec_impl_test.cc | 24 +++++++++++++++++++++++ 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 9eaba30184267..6013c4b1c9a38 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -463,7 +463,7 @@ bool ConnectionImpl::maybeDirectDispatch(Buffer::Instance& data) { void ConnectionImpl::dispatch(Buffer::Instance& data) { ENVOY_CONN_LOG(trace, "parsing {} bytes", connection_, data.length()); - ASSERT(pending_body_buffer_.length() == 0); + ASSERT(buffered_body_.length() == 0); if (maybeDirectDispatch(data)) { return; @@ -483,11 +483,11 @@ void ConnectionImpl::dispatch(Buffer::Instance& data) { break; } } - dispatchBody(); + dispatchBufferedBody(); } else { dispatchSlice(nullptr, 0); } - ASSERT(pending_body_buffer_.length() == 0); + ASSERT(buffered_body_.length() == 0); ENVOY_CONN_LOG(trace, "parsed {} bytes", connection_, total_parsed); data.drain(total_parsed); @@ -629,15 +629,14 @@ int ConnectionImpl::onHeadersCompleteBase() { } void ConnectionImpl::bufferBody(const char* data, size_t length) { - pending_body_buffer_.add(data, length); + buffered_body_.add(data, length); } -void ConnectionImpl::dispatchBody() { +void ConnectionImpl::dispatchBufferedBody() { ASSERT(HTTP_PARSER_ERRNO(&parser_) == HPE_OK || HTTP_PARSER_ERRNO(&parser_) == HPE_PAUSED); - if (pending_body_buffer_.length() > 0) { - onBody(pending_body_buffer_); - // Clear the buffer. - pending_body_buffer_.drain(pending_body_buffer_.length()); + if (buffered_body_.length() > 0) { + onBody(buffered_body_); + buffered_body_.drain(buffered_body_.length()); } } @@ -645,14 +644,14 @@ void ConnectionImpl::onChunkHeader(bool is_final_chunk) { if (is_final_chunk) { // Dispatch body before parsing trailers, so body ends up dispatched even if an error is found // while processing trailers. - dispatchBody(); + dispatchBufferedBody(); } } void ConnectionImpl::onMessageCompleteBase() { ENVOY_CONN_LOG(trace, "message complete", connection_); - dispatchBody(); + dispatchBufferedBody(); if (handling_upgrade_) { // If this is an upgrade request, swallow the onMessageComplete. The diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 170336362e25c..5ee61ef417a82 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -243,6 +243,18 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggabledispatch(buffer), CodecProtocolException, + "http/1.1 protocol error: HPE_INVALID_CHUNK_SIZE"); +} + // Currently http_parser does not support chained transfer encodings. TEST_F(Http1ServerConnectionImplTest, IdentityAndChunkedBody) { initialize(); From 44e59f6d4f98e29f2036b5e97ed108bba78d7057 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 16 Mar 2020 19:15:59 -0400 Subject: [PATCH 03/16] Change argument name to hack around name conflict issue with namespace envoy::data in bazel compile_time_options CI build. Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 32c8a0bd3225a..6b6307d3bbed6 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -43,9 +43,9 @@ std::string createHeaderFragment(int num_headers) { return headers; } -Buffer::OwnedImpl createBufferWithOneByteSlices(std::string_view data) { +Buffer::OwnedImpl createBufferWithOneByteSlices(std::string_view input) { Buffer::OwnedImpl buffer; - for (const char& c : data) { + for (const char& c : input) { buffer.appendSliceForTest(&c, 1); } return buffer; From e7096f4aa15ba7734c845103eb5306b547e4cf66 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Tue, 17 Mar 2020 00:52:47 -0400 Subject: [PATCH 04/16] Add missing header to fix bazel compile_time_options CI Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 6b6307d3bbed6..da78e80307c9d 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1,5 +1,6 @@ #include #include +#include #include "envoy/buffer/buffer.h" #include "envoy/event/dispatcher.h" From 9de08cc780fdd5de1ddce1bf0669bb1a932a2250 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Tue, 17 Mar 2020 02:22:58 -0400 Subject: [PATCH 05/16] use absl::string_view Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index da78e80307c9d..ac04a4b4113ef 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1,6 +1,5 @@ #include #include -#include #include "envoy/buffer/buffer.h" #include "envoy/event/dispatcher.h" @@ -19,6 +18,7 @@ #include "test/test_common/printers.h" #include "test/test_common/test_runtime.h" +#include "absl/strings/string_view.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -44,7 +44,7 @@ std::string createHeaderFragment(int num_headers) { return headers; } -Buffer::OwnedImpl createBufferWithOneByteSlices(std::string_view input) { +Buffer::OwnedImpl createBufferWithOneByteSlices(absl::string_view input) { Buffer::OwnedImpl buffer; for (const char& c : input) { buffer.appendSliceForTest(&c, 1); From 26340aca14c686f9c67228f435da4fc16f304cc2 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 23 Mar 2020 16:49:29 -0400 Subject: [PATCH 06/16] improve e2e coverage for larger request/responses over H1 and H2 e2e HTTP benchmark Signed-off-by: Antonio Vicente --- test/benchmark/BUILD | 9 +- test/benchmark/main.cc | 3 + test/integration/BUILD | 19 +++ test/integration/http2_integration_test.cc | 12 +- test/integration/http_benchmark.cc | 136 +++++++++++++++++++++ test/integration/integration_test.cc | 11 +- 6 files changed, 184 insertions(+), 6 deletions(-) create mode 100644 test/integration/http_benchmark.cc diff --git a/test/benchmark/BUILD b/test/benchmark/BUILD index 7bfc766727b5f..062017b0f7dde 100644 --- a/test/benchmark/BUILD +++ b/test/benchmark/BUILD @@ -15,8 +15,9 @@ envoy_cc_test_library( "abseil_symbolize", "benchmark", ], - deps = select({ - "//bazel:disable_signal_trace": [], - "//conditions:default": ["//source/common/signal:sigaction_lib"], - }), + deps = ["//test/test_common:environment_lib"] + + select({ + "//bazel:disable_signal_trace": [], + "//conditions:default": ["//source/common/signal:sigaction_lib"], + }), ) diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index ae39333d72a82..5866156ed516d 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -7,6 +7,8 @@ #include "common/signal/signal_action.h" #endif +#include "test/test_common/environment.h" + #include "absl/debugging/symbolize.h" // Boilerplate main(), which discovers benchmarks and runs them. @@ -23,5 +25,6 @@ int main(int argc, char** argv) { if (benchmark::ReportUnrecognizedArguments(argc, argv)) { return 1; } + Envoy::TestEnvironment::initializeOptions(argc, argv); benchmark::RunSpecifiedBenchmarks(); } diff --git a/test/integration/BUILD b/test/integration/BUILD index 595bf58235b35..eec00936d1aa6 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -2,6 +2,8 @@ licenses(["notice"]) # Apache 2 load( "//bazel:envoy_build_system.bzl", + "envoy_benchmark_test", + "envoy_cc_benchmark_binary", "envoy_cc_fuzz_test", "envoy_cc_test", "envoy_cc_test_library", @@ -286,6 +288,23 @@ envoy_cc_test( ], ) +envoy_cc_benchmark_binary( + name = "http_benchmark", + srcs = ["http_benchmark.cc"], + external_deps = [ + "benchmark", + ], + deps = [ + ":http_integration_lib", + "//source/exe:process_wide_lib", + ], +) + +envoy_benchmark_test( + name = "http_benchmark_test", + benchmark_binary = "http_benchmark", +) + envoy_cc_test( name = "http_subset_lb_integration_test", srcs = [ diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 4a1340755b244..29420b0d21ab5 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -31,9 +31,19 @@ TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { testRouterRequestAndResponseWithBody(1024, 512, false); } +TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithGiantBodyNoBuffer) { + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); +} + TEST_P(Http2IntegrationTest, FlowControlOnAndGiantBody) { config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithBody(1024 * 1024, 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); +} + +TEST_P(Http2IntegrationTest, LargeFlowControlOnAndGiantBody) { + config_helper_.setBufferLimits(128 * 1024, + 128 * 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); } TEST_P(Http2IntegrationTest, RouterHeaderOnlyRequestAndResponseNoBuffer) { diff --git a/test/integration/http_benchmark.cc b/test/integration/http_benchmark.cc new file mode 100644 index 0000000000000..7074a16c3f581 --- /dev/null +++ b/test/integration/http_benchmark.cc @@ -0,0 +1,136 @@ +#include + +#include "exe/process_wide.h" + +#include "test/integration/http_integration.h" +#include "test/mocks/access_log/mocks.h" + +#include "absl/strings/str_cat.h" +#include "benchmark/benchmark.h" +#include "gmock/gmock.h" + +namespace Envoy { + +namespace { + +// Get a debug string from an object by calling operator<< +template std::string toString(const T& obj) { + std::stringstream ss; + ss << obj; + return ss.str(); +} + +std::string versionString(Http::CodecClient::Type type) { + switch (type) { + case Http::CodecClient::Type::HTTP1: + return "h1"; + case Http::CodecClient::Type::HTTP2: + return "h2"; + case Http::CodecClient::Type::HTTP3: + return "h3"; + } +} + +std::string versionString(FakeHttpConnection::Type type) { + switch (type) { + case FakeHttpConnection::Type::HTTP1: + return "h1"; + case FakeHttpConnection::Type::HTTP2: + return "h2"; + } +} + +} // namespace + +class HttpBenchmarkImpl : public HttpIntegrationTest { +public: + static Network::Address::IpVersion getSupportedIpVersion() { + return TestEnvironment::getIpVersionsForTest().front(); + } + + HttpBenchmarkImpl(Http::CodecClient::Type downstream_protocol) + : HttpIntegrationTest(downstream_protocol, getSupportedIpVersion()) {} + + void initialize() override { + if (!Envoy::Event::Libevent::Global::initialized()) { + Envoy::Event::Libevent::Global::initialize(); + } + HttpIntegrationTest::initialize(); + } + + void doBenchmark(benchmark::State& state, uint64_t request_body_size, uint64_t response_body_size, + bool set_content_length) { + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "POST"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + if (set_content_length) { + request_headers.setContentLength(request_body_size); + response_headers.setContentLength(response_body_size); + } + + autonomous_upstream_ = false; + initialize(); + + for (auto _ : state) { + codec_client_ = makeHttpConnection(lookupPort("http")); + RELEASE_ASSERT(codec_client_->connected(), + std::string(codec_client_->connection()->transportFailureReason())); + + auto response_decoder = sendRequestAndWaitForResponse( + request_headers, request_body_size, default_response_headers_, response_body_size); + RELEASE_ASSERT(response_decoder->complete(), "Expected complete response"); + RELEASE_ASSERT(response_decoder->headers().Status()->value().getStringView() == "200", + toString(response_decoder->headers())); + codec_client_->close(); + codec_client_.reset(); + } + } + + Envoy::Thread::MutexBasicLockable lock_; + Envoy::Logger::Context logging_state_{Envoy::TestEnvironment::getOptions().logLevel(), + Envoy::TestEnvironment::getOptions().logFormat(), lock_, + false}; +}; + +void BM_http(benchmark::State& state) { + Http::CodecClient::Type client_type = static_cast(state.range(0)); + FakeHttpConnection::Type upstream_type = static_cast(state.range(1)); + const uint64_t request_body_size = state.range(2); + const uint64_t response_body_size = state.range(3); + const bool set_content_length = state.range(4); + + if (!Envoy::Event::Libevent::Global::initialized()) { + Envoy::Event::Libevent::Global::initialize(); + } + + state.SetLabel(absl::StrCat("request: ", versionString(client_type), " ", request_body_size, + ", response: ", versionString(upstream_type), " ", response_body_size, + ", ", set_content_length ? "content-length" : "chunked")); + HttpBenchmarkImpl http_benchmark(client_type); + http_benchmark.setUpstreamProtocol(upstream_type); + http_benchmark.doBenchmark(state, request_body_size, response_body_size, set_content_length); +} + +static void BenchmarkArguments(benchmark::internal::Benchmark* b) { + for (bool large_post : {false, true}) { + for (Http::CodecClient::Type client_type : + {Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}) { + for (FakeHttpConnection::Type upstream_type : + {FakeHttpConnection::Type::HTTP1, FakeHttpConnection::Type::HTTP2}) { + for (bool set_content_length : {false, true}) { + int64_t request_body_size = large_post ? 10 * 1024 * 1024 : 0; + int64_t response_body_size = large_post ? 0 : 10 * 1024 * 1024; + b->Args({static_cast(client_type), static_cast(upstream_type), + request_body_size, response_body_size, set_content_length}); + } + } + } + } +} + +BENCHMARK(BM_http) + ->Apply(BenchmarkArguments) + ->MeasureProcessCPUTime() + ->Unit(benchmark::kMillisecond); + +} // namespace Envoy diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 9efffe810c983..9c6f73e2cfcb0 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -160,9 +160,18 @@ TEST_P(IntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { testRouterRequestAndResponseWithBody(1024, 512, false); } +TEST_P(IntegrationTest, RouterRequestAndResponseWithGiantBodyNoBuffer) { + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); +} + TEST_P(IntegrationTest, FlowControlOnAndGiantBody) { config_helper_.setBufferLimits(1024, 1024); - testRouterRequestAndResponseWithBody(1024 * 1024, 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); +} + +TEST_P(IntegrationTest, LargeFlowControlOnAndGiantBody) { + config_helper_.setBufferLimits(128 * 1024, 128 * 1024); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); } TEST_P(IntegrationTest, RouterRequestAndResponseLargeHeaderNoBuffer) { From 45086970e2bba833501d80a087ddf34dde5ba1f8 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 23 Mar 2020 17:18:27 -0400 Subject: [PATCH 07/16] cover large transfers with content-length Signed-off-by: Antonio Vicente --- .../alts/alts_integration_test.cc | 4 +-- .../tls/integration/ssl_integration_test.cc | 28 +++++++++---------- test/integration/http2_integration_test.cc | 27 +++++++++++++++--- test/integration/http_integration.cc | 11 ++++++-- test/integration/http_integration.h | 2 +- .../idle_timeout_integration_test.cc | 4 +-- test/integration/integration_test.cc | 26 ++++++++++++++--- .../proxy_proto_integration_test.cc | 18 ++++++------ .../sds_static_integration_test.cc | 4 +-- test/integration/uds_integration_test.cc | 2 +- 10 files changed, 84 insertions(+), 42 deletions(-) diff --git a/test/extensions/transport_sockets/alts/alts_integration_test.cc b/test/extensions/transport_sockets/alts/alts_integration_test.cc index 39189c8170168..587fd3b8e490b 100644 --- a/test/extensions/transport_sockets/alts/alts_integration_test.cc +++ b/test/extensions/transport_sockets/alts/alts_integration_test.cc @@ -165,7 +165,7 @@ TEST_P(AltsIntegrationTestValidPeer, RouterRequestAndResponseWithBodyNoBuffer) { ConnectionCreationFunction creator = [this]() -> Network::ClientConnectionPtr { return makeAltsConnection(); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } class AltsIntegrationTestEmptyPeer : public AltsIntegrationTestBase { @@ -186,7 +186,7 @@ TEST_P(AltsIntegrationTestEmptyPeer, RouterRequestAndResponseWithBodyNoBuffer) { ConnectionCreationFunction creator = [this]() -> Network::ClientConnectionPtr { return makeAltsConnection(); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } class AltsIntegrationTestClientInvalidPeer : public AltsIntegrationTestBase { diff --git a/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc b/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc index bf55f47d034e8..bd755736ef457 100644 --- a/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc +++ b/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc @@ -89,7 +89,7 @@ TEST_P(SslIntegrationTest, RouterRequestAndResponseWithGiantBodyBuffer) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); }; - testRouterRequestAndResponseWithBody(16 * 1024 * 1024, 16 * 1024 * 1024, false, &creator); + testRouterRequestAndResponseWithBody(16 * 1024 * 1024, 16 * 1024 * 1024, false, false, &creator); checkStats(); } @@ -97,7 +97,7 @@ TEST_P(SslIntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -108,7 +108,7 @@ TEST_P(SslIntegrationTest, RouterRequestAndResponseWithBodyNoBufferHttp2) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(ClientSslTransportOptions().setAlpn(true)); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -116,7 +116,7 @@ TEST_P(SslIntegrationTest, RouterRequestAndResponseWithBodyNoBufferVerifySAN) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(ClientSslTransportOptions().setSan(true)); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -125,7 +125,7 @@ TEST_P(SslIntegrationTest, RouterRequestAndResponseWithBodyNoBufferHttp2VerifySA ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(ClientSslTransportOptions().setAlpn(true).setSan(true)); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -244,7 +244,7 @@ TEST_P(SslCertficateIntegrationTest, ServerRsa) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -255,7 +255,7 @@ TEST_P(SslCertficateIntegrationTest, ServerEcdsa) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -266,7 +266,7 @@ TEST_P(SslCertficateIntegrationTest, ServerRsaEcdsa) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -277,7 +277,7 @@ TEST_P(SslCertficateIntegrationTest, ClientRsaOnly) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(rsaOnlyClientOptions()); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -302,7 +302,7 @@ TEST_P(SslCertficateIntegrationTest, ServerRsaEcdsaClientRsaOnly) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(rsaOnlyClientOptions()); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -329,7 +329,7 @@ TEST_P(SslCertficateIntegrationTest, ServerEcdsaClientEcdsaOnly) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(ecdsaOnlyClientOptions()); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -341,7 +341,7 @@ TEST_P(SslCertficateIntegrationTest, ServerRsaEcdsaClientEcdsaOnly) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(ecdsaOnlyClientOptions()); }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); } @@ -550,7 +550,7 @@ TEST_P(SslTapIntegrationTest, RequestWithTextProto) { return makeSslClientConnection({}); }; const uint64_t id = Network::ConnectionImpl::nextGlobalIdForTest() + 1; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); checkStats(); codec_client_->close(); test_server_->waitForCounterGe("http.config_test.downstream_cx_destroy", 1); @@ -576,7 +576,7 @@ TEST_P(SslTapIntegrationTest, RequestWithJsonBodyAsStringUpstreamTap) { return makeSslClientConnection({}); }; const uint64_t id = Network::ConnectionImpl::nextGlobalIdForTest() + 2; - testRouterRequestAndResponseWithBody(512, 1024, false, &creator); + testRouterRequestAndResponseWithBody(512, 1024, false, false, &creator); checkStats(); codec_client_->close(); test_server_->waitForCounterGe("http.config_test.downstream_cx_destroy", 1); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 29420b0d21ab5..8630c161563f1 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -28,22 +28,41 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, Http2IntegrationTest, TestUtility::ipTestParamsToString); TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { - testRouterRequestAndResponseWithBody(1024, 512, false); + testRouterRequestAndResponseWithBody(1024, 512, false, false); } TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithGiantBodyNoBuffer) { - testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false); } TEST_P(Http2IntegrationTest, FlowControlOnAndGiantBody) { config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false); } TEST_P(Http2IntegrationTest, LargeFlowControlOnAndGiantBody) { config_helper_.setBufferLimits(128 * 1024, 128 * 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false); +} + +TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithBodyAndContentLengthNoBuffer) { + testRouterRequestAndResponseWithBody(1024, 512, false, true); +} + +TEST_P(Http2IntegrationTest, RouterRequestAndResponseWithGiantBodyAndContentLengthNoBuffer) { + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true); +} + +TEST_P(Http2IntegrationTest, FlowControlOnAndGiantBodyWithContentLength) { + config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true); +} + +TEST_P(Http2IntegrationTest, LargeFlowControlOnAndGiantBodyWithContentLength) { + config_helper_.setBufferLimits(128 * 1024, + 128 * 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true); } TEST_P(Http2IntegrationTest, RouterHeaderOnlyRequestAndResponseNoBuffer) { diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 790b0b7140f91..19dcd72b210c0 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -411,7 +411,7 @@ void HttpIntegrationTest::checkSimpleRequestSuccess(uint64_t expected_request_si } void HttpIntegrationTest::testRouterRequestAndResponseWithBody( - uint64_t request_size, uint64_t response_size, bool big_header, + uint64_t request_size, uint64_t response_size, bool big_header, bool set_content_length_header, ConnectionCreationFunction* create_connection) { initialize(); codec_client_ = makeHttpConnection( @@ -419,11 +419,16 @@ void HttpIntegrationTest::testRouterRequestAndResponseWithBody( Http::TestRequestHeaderMapImpl request_headers{ {":method", "POST"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}, {"x-lyft-user-id", "123"}, {"x-forwarded-for", "10.0.0.1"}}; + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + if (set_content_length_header) { + request_headers.setContentLength(request_size); + response_headers.setContentLength(response_size); + } if (big_header) { request_headers.addCopy("big", std::string(4096, 'a')); } - auto response = sendRequestAndWaitForResponse(request_headers, request_size, - default_response_headers_, response_size); + auto response = + sendRequestAndWaitForResponse(request_headers, request_size, response_headers, response_size); checkSimpleRequestSuccess(request_size, response_size, response.get()); } diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 263bb1bd7272e..14c615a60dddc 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -175,7 +175,7 @@ class HttpIntegrationTest : public BaseIntegrationTest { void testRouterNotFoundWithBody(); void testRouterRequestAndResponseWithBody(uint64_t request_size, uint64_t response_size, - bool big_header, + bool big_header, bool set_content_length_header = false, ConnectionCreationFunction* creator = nullptr); void testRouterHeaderOnlyRequestAndResponse(ConnectionCreationFunction* creator = nullptr, int upstream_index = 0, diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index d4827b0d7148f..299d875247bee 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -282,7 +282,7 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutAfterBidiData) { // Successful request/response when per-stream idle timeout is configured. TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutRequestAndResponse) { enable_per_stream_idle_timeout_ = true; - testRouterRequestAndResponseWithBody(1024, 1024, false, nullptr); + testRouterRequestAndResponseWithBody(1024, 1024, false); } TEST_P(IdleTimeoutIntegrationTest, RequestTimeoutConfiguredRequestResponse) { @@ -292,7 +292,7 @@ TEST_P(IdleTimeoutIntegrationTest, RequestTimeoutConfiguredRequestResponse) { TEST_P(IdleTimeoutIntegrationTest, RequestTimeoutConfiguredRequestResponseWithBody) { enable_request_timeout_ = true; - testRouterRequestAndResponseWithBody(1024, 1024, false, nullptr); + testRouterRequestAndResponseWithBody(1024, 1024, false); } TEST_P(IdleTimeoutIntegrationTest, RequestTimeoutTriggersOnBodilessPost) { diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 9c6f73e2cfcb0..de3d38ae197c2 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -157,21 +157,39 @@ TEST_P(IntegrationTest, ConnectionClose) { } TEST_P(IntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { - testRouterRequestAndResponseWithBody(1024, 512, false); + testRouterRequestAndResponseWithBody(1024, 512, false, false); } TEST_P(IntegrationTest, RouterRequestAndResponseWithGiantBodyNoBuffer) { - testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false); } TEST_P(IntegrationTest, FlowControlOnAndGiantBody) { config_helper_.setBufferLimits(1024, 1024); - testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false); } TEST_P(IntegrationTest, LargeFlowControlOnAndGiantBody) { config_helper_.setBufferLimits(128 * 1024, 128 * 1024); - testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false); +} + +TEST_P(IntegrationTest, RouterRequestAndResponseWithBodyAndContentLengthNoBuffer) { + testRouterRequestAndResponseWithBody(1024, 512, false, true); +} + +TEST_P(IntegrationTest, RouterRequestAndResponseWithGiantBodyAndContentLengthNoBuffer) { + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true); +} + +TEST_P(IntegrationTest, FlowControlOnAndGiantBodyWithContentLength) { + config_helper_.setBufferLimits(1024, 1024); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true); +} + +TEST_P(IntegrationTest, LargeFlowControlOnAndGiantBodyWithContentLength) { + config_helper_.setBufferLimits(128 * 1024, 128 * 1024); + testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, true); } TEST_P(IntegrationTest, RouterRequestAndResponseLargeHeaderNoBuffer) { diff --git a/test/integration/proxy_proto_integration_test.cc b/test/integration/proxy_proto_integration_test.cc index c10323e75737c..ec62a8991a6ad 100644 --- a/test/integration/proxy_proto_integration_test.cc +++ b/test/integration/proxy_proto_integration_test.cc @@ -26,7 +26,7 @@ TEST_P(ProxyProtoIntegrationTest, V1RouterRequestAndResponseWithBodyNoBuffer) { return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(ProxyProtoIntegrationTest, V2RouterRequestAndResponseWithBodyNoBuffer) { @@ -40,7 +40,7 @@ TEST_P(ProxyProtoIntegrationTest, V2RouterRequestAndResponseWithBodyNoBuffer) { return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(ProxyProtoIntegrationTest, V1RouterRequestAndResponseWithBodyNoBufferV6) { @@ -51,7 +51,7 @@ TEST_P(ProxyProtoIntegrationTest, V1RouterRequestAndResponseWithBodyNoBufferV6) return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(ProxyProtoIntegrationTest, V2RouterRequestAndResponseWithBodyNoBufferV6) { @@ -67,7 +67,7 @@ TEST_P(ProxyProtoIntegrationTest, V2RouterRequestAndResponseWithBodyNoBufferV6) return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownRequestAndResponseWithBodyNoBuffer) { @@ -78,7 +78,7 @@ TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownRequestAndResponseWithBodyNo return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownLongRequestAndResponseWithBodyNoBuffer) { @@ -89,7 +89,7 @@ TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownLongRequestAndResponseWithBo return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } // Test that %DOWNSTREAM_DIRECT_REMOTE_ADDRESS%/%DOWNSTREAM_DIRECT_REMOTE_ADDRESS_WITHOUT_PORT% @@ -110,7 +110,7 @@ TEST_P(ProxyProtoIntegrationTest, AccessLog) { return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); const std::string log_line = waitForAccessLog(access_log_name_); const std::vector tokens = StringUtil::splitToken(log_line, " "); @@ -147,7 +147,7 @@ TEST_P(ProxyProtoIntegrationTest, DEPRECATED_FEATURE_TEST(OriginalDst)) { return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(ProxyProtoIntegrationTest, ClusterProvided) { @@ -177,7 +177,7 @@ TEST_P(ProxyProtoIntegrationTest, ClusterProvided) { return conn; }; - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } } // namespace Envoy diff --git a/test/integration/sds_static_integration_test.cc b/test/integration/sds_static_integration_test.cc index 8b488b0f58e19..f0a92b8e19c74 100644 --- a/test/integration/sds_static_integration_test.cc +++ b/test/integration/sds_static_integration_test.cc @@ -104,7 +104,7 @@ TEST_P(SdsStaticDownstreamIntegrationTest, RouterRequestAndResponseWithGiantBody ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection(); }; - testRouterRequestAndResponseWithBody(16 * 1024 * 1024, 16 * 1024 * 1024, false, &creator); + testRouterRequestAndResponseWithBody(16 * 1024 * 1024, 16 * 1024 * 1024, false, false, &creator); } class SdsStaticUpstreamIntegrationTest : public testing::TestWithParam, @@ -160,7 +160,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, SdsStaticUpstreamIntegrationTest, TestUtility::ipTestParamsToString); TEST_P(SdsStaticUpstreamIntegrationTest, RouterRequestAndResponseWithGiantBodyBuffer) { - testRouterRequestAndResponseWithBody(16 * 1024 * 1024, 16 * 1024 * 1024, false, nullptr); + testRouterRequestAndResponseWithBody(16 * 1024 * 1024, 16 * 1024 * 1024, false, false, nullptr); } } // namespace Ssl diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index ddb9abc68ad6d..e1e63b8fbfb55 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -111,7 +111,7 @@ TEST_P(UdsListenerIntegrationTest, TestPeerCredentials) { TEST_P(UdsListenerIntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { ConnectionCreationFunction creator = createConnectionFn(); - testRouterRequestAndResponseWithBody(1024, 512, false, &creator); + testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); } TEST_P(UdsListenerIntegrationTest, RouterHeaderOnlyRequestAndResponse) { From ae92586ba881be6980b007c3cb5f1aa2c8105ae0 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Tue, 24 Mar 2020 11:59:36 -0400 Subject: [PATCH 08/16] address review comments Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 107 ++++++++++++---------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 00b4e506e918f..0d7afe972a936 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -146,7 +146,7 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs MockRequestDecoder decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); @@ -202,9 +202,9 @@ void Http1ServerConnectionImplTest::testTrailersExceedLimit(std::string trailer_ if (enable_trailers) { EXPECT_CALL(decoder, decodeHeaders_(_, false)); - EXPECT_CALL(decoder, decodeData(_, false)).Times(1); + EXPECT_CALL(decoder, decodeData(_, false)); } else { - EXPECT_CALL(decoder, decodeData(_, false)).Times(1); + EXPECT_CALL(decoder, decodeData(_, false)); EXPECT_CALL(decoder, decodeData(_, true)); } @@ -277,7 +277,7 @@ TEST_F(Http1ServerConnectionImplTest, EmptyHeader) { {":path", "/"}, {":method", "GET"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\nTest:\r\nHello: World\r\n\r\n"); codec_->dispatch(buffer); @@ -297,12 +297,13 @@ TEST_F(Http1ServerConnectionImplTest, IdentityEncoding) { {":method", "GET"}, {"transfer-encoding", "identity"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\ntransfer-encoding: identity\r\n\r\n"); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); } +// Verify that data in the two body chunks is merged before the upcall to decodeData. TEST_F(Http1ServerConnectionImplTest, ChunkedBody) { initialize(); @@ -316,10 +317,12 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBody) { {":method", "POST"}, {"transfer-encoding", "chunked"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data("Hello World"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); - EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + // Call to decodeData("", true) happens after. + Buffer::OwnedImpl empty(""); + EXPECT_CALL(decoder, decodeData(&empty, true)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" "6\r\nHello \r\n" @@ -329,7 +332,9 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBody) { EXPECT_EQ(0U, buffer.length()); } -TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplit) { +// Verify dispatch behavior when dispatching an incomplete chunk, and resumption of the parse via a +// second dispatch. +TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplitOverTwoDispatches) { initialize(); InSequence sequence; @@ -342,12 +347,12 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplit) { {":method", "POST"}, {"transfer-encoding", "chunked"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data1("Hello Worl"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)); Buffer::OwnedImpl expected_data2("d"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)).Times(1); - EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)); + EXPECT_CALL(decoder, decodeData(_, true)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" "6\r\nHello \r\n" @@ -360,6 +365,8 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplit) { EXPECT_EQ(0U, buffer2.length()); } +// Verify that headers and chunked body are processed correctly and data is merged before the +// decodeData upcall even if delivered in a buffer that holds 1 byte per slice. TEST_F(Http1ServerConnectionImplTest, ChunkedBodyFragmentedBuffer) { initialize(); @@ -373,10 +380,10 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodyFragmentedBuffer) { {":method", "POST"}, {"transfer-encoding", "chunked"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data("Hello World"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); - EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(decoder, decodeData(_, true)); Buffer::OwnedImpl buffer = createBufferWithOneByteSlices("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" @@ -400,10 +407,10 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodyCase) { {":method", "POST"}, {"transfer-encoding", "Chunked"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data("Hello World"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); - EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(decoder, decodeData(_, true)); Buffer::OwnedImpl buffer( "POST / HTTP/1.1\r\ntransfer-encoding: Chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"); @@ -411,6 +418,7 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodyCase) { EXPECT_EQ(0U, buffer.length()); } +// No decodeData upcall for chunk data handled by the same dispatch call that detects a parse error. TEST_F(Http1ServerConnectionImplTest, InvalidChunkHeader) { initialize(); @@ -418,13 +426,14 @@ TEST_F(Http1ServerConnectionImplTest, InvalidChunkHeader) { MockRequestDecoder decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + EXPECT_CALL(decoder, decodeData(_, _)).Times(0); TestHeaderMapImpl expected_headers{ {":path", "/"}, {":method", "POST"}, {"transfer-encoding", "chunked"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); EXPECT_CALL(decoder, decodeData(_, _)).Times(0); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" @@ -476,7 +485,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); TestHeaderMapImpl expected_headers{{":path", "/"}, {":method", "GET"}}; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer("GET / HTTP/1.0\r\n\r\n"); codec_->dispatch(buffer); @@ -516,7 +525,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { return decoder; })); - EXPECT_CALL(decoder, decodeHeaders_(_, true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(_, true)); codec_->dispatch(buffer); std::string output; @@ -539,7 +548,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { response_encoder = &encoder; return decoder; })); - EXPECT_CALL(decoder, decodeHeaders_(_, true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(_, true)); codec_->dispatch(buffer); EXPECT_EQ(Protocol::Http11, codec_->protocol()); } @@ -601,7 +610,7 @@ TEST_F(Http1ServerConnectionImplTest, Http11InvalidTrailerPost) { .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); EXPECT_CALL(decoder, decodeHeaders_(_, false)); - EXPECT_CALL(decoder, decodeData(_, false)).Times(1); + EXPECT_CALL(decoder, decodeData(_, false)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\n" "Host: host\r\n" @@ -671,7 +680,7 @@ TEST_F(Http1ServerConnectionImplTest, SimpleGet) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); TestHeaderMapImpl expected_headers{{":path", "/"}, {":method", "GET"}}; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); codec_->dispatch(buffer); @@ -744,7 +753,7 @@ TEST_F(Http1ServerConnectionImplTest, FloodProtection) { // In most tests the write output is serialized to a buffer here it is // ignored to build up queued "end connection" sentinels. EXPECT_CALL(connection_, write(_, _)) - .Times(1) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { // Move the response out of data while preserving the buffer fragment sentinels. local_buffer.move(data); @@ -797,7 +806,7 @@ TEST_F(Http1ServerConnectionImplTest, FloodProtectionOff) { // In most tests the write output is serialized to a buffer here it is // ignored to build up queued "end connection" sentinels. EXPECT_CALL(connection_, write(_, _)) - .Times(1) + .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> void { // Move the response out of data while preserving the buffer fragment sentinels. local_buffer.move(data); @@ -817,7 +826,7 @@ TEST_F(Http1ServerConnectionImplTest, HostHeaderTranslation) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); TestHeaderMapImpl expected_headers{{":authority", "hello"}, {":path", "/"}, {":method", "GET"}}; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\nHOST: hello\r\n\r\n"); codec_->dispatch(buffer); @@ -981,19 +990,21 @@ TEST_F(Http1ServerConnectionImplTest, PostWithContentLength) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); TestHeaderMapImpl expected_headers{{"content-length", "5"}, {":path", "/"}, {":method", "POST"}}; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data1("12345"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)); Buffer::OwnedImpl expected_data2; - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), true)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), true)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ncontent-length: 5\r\n\r\n12345"); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); } +// Verify that headers and body with content length are processed correctly and data is merged +// before the decodeData upcall even if delivered in a buffer that holds 1 byte per slice. TEST_F(Http1ServerConnectionImplTest, PostWithContentLengthFragmentedBuffer) { initialize(); @@ -1003,13 +1014,13 @@ TEST_F(Http1ServerConnectionImplTest, PostWithContentLengthFragmentedBuffer) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); TestHeaderMapImpl expected_headers{{"content-length", "5"}, {":path", "/"}, {":method", "POST"}}; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data1("12345"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)); Buffer::OwnedImpl expected_data2; - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), true)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), true)); Buffer::OwnedImpl buffer = createBufferWithOneByteSlices("POST / HTTP/1.1\r\ncontent-length: 5\r\n\r\n12345"); @@ -1380,19 +1391,19 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequest) { NiceMock decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); Buffer::OwnedImpl buffer( "POST / HTTP/1.1\r\nConnection: upgrade\r\nUpgrade: foo\r\ncontent-length:5\r\n\r\n"); codec_->dispatch(buffer); Buffer::OwnedImpl expected_data1("12345"); Buffer::OwnedImpl body("12345"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)); codec_->dispatch(body); Buffer::OwnedImpl expected_data2("abcd"); Buffer::OwnedImpl websocket_payload("abcd"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)); codec_->dispatch(websocket_payload); } @@ -1404,8 +1415,8 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestWithEarlyData) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); Buffer::OwnedImpl expected_data("12345abcd"); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\nConnection: upgrade\r\nUpgrade: " "foo\r\ncontent-length:5\r\n\r\n12345abcd"); codec_->dispatch(buffer); @@ -1421,8 +1432,8 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestWithTEChunked) { // Even with T-E chunked, the data should neither be inspected for (the not // present in this unit test) chunks, but simply passed through. Buffer::OwnedImpl expected_data("12345abcd"); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\nConnection: upgrade\r\nUpgrade: " "foo\r\ntransfer-encoding: chunked\r\n\r\n12345abcd"); codec_->dispatch(buffer); @@ -1438,15 +1449,15 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestWithNoBody) { // Make sure we avoid the deferred_end_stream_headers_ optimization for // requests-with-no-body. Buffer::OwnedImpl expected_data("abcd"); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); Buffer::OwnedImpl buffer( "GET / HTTP/1.1\r\nConnection: upgrade\r\nUpgrade: foo\r\ncontent-length: 0\r\n\r\nabcd"); codec_->dispatch(buffer); } TEST_F(Http1ServerConnectionImplTest, WatermarkTest) { - EXPECT_CALL(connection_, bufferLimit()).Times(1).WillOnce(Return(10)); + EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); NiceMock decoder; @@ -1742,13 +1753,13 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponse) { // Send body payload Buffer::OwnedImpl expected_data1("12345"); Buffer::OwnedImpl body("12345"); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data1), false)); codec_->dispatch(body); // Send websocket payload Buffer::OwnedImpl expected_data2("abcd"); Buffer::OwnedImpl websocket_payload("abcd"); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data2), false)).Times(1); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data2), false)); codec_->dispatch(websocket_payload); } @@ -1767,14 +1778,14 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponseWithEarlyData) { // Send upgrade headers EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); Buffer::OwnedImpl expected_data("12345abcd"); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)); Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: " "upgrade\r\nUpgrade: websocket\r\n\r\n12345abcd"); codec_->dispatch(response); } TEST_F(Http1ClientConnectionImplTest, WatermarkTest) { - EXPECT_CALL(connection_, bufferLimit()).Times(1).WillOnce(Return(10)); + EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); InSequence s; From 6739dfcb08d13f3c6d291bb048068f2a7f02fad1 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Tue, 24 Mar 2020 12:01:36 -0400 Subject: [PATCH 09/16] Remove http_benchmark Signed-off-by: Antonio Vicente --- test/benchmark/BUILD | 9 +- test/benchmark/main.cc | 3 - test/integration/BUILD | 19 ---- test/integration/http_benchmark.cc | 136 ----------------------------- 4 files changed, 4 insertions(+), 163 deletions(-) delete mode 100644 test/integration/http_benchmark.cc diff --git a/test/benchmark/BUILD b/test/benchmark/BUILD index 062017b0f7dde..7bfc766727b5f 100644 --- a/test/benchmark/BUILD +++ b/test/benchmark/BUILD @@ -15,9 +15,8 @@ envoy_cc_test_library( "abseil_symbolize", "benchmark", ], - deps = ["//test/test_common:environment_lib"] + - select({ - "//bazel:disable_signal_trace": [], - "//conditions:default": ["//source/common/signal:sigaction_lib"], - }), + deps = select({ + "//bazel:disable_signal_trace": [], + "//conditions:default": ["//source/common/signal:sigaction_lib"], + }), ) diff --git a/test/benchmark/main.cc b/test/benchmark/main.cc index 5866156ed516d..ae39333d72a82 100644 --- a/test/benchmark/main.cc +++ b/test/benchmark/main.cc @@ -7,8 +7,6 @@ #include "common/signal/signal_action.h" #endif -#include "test/test_common/environment.h" - #include "absl/debugging/symbolize.h" // Boilerplate main(), which discovers benchmarks and runs them. @@ -25,6 +23,5 @@ int main(int argc, char** argv) { if (benchmark::ReportUnrecognizedArguments(argc, argv)) { return 1; } - Envoy::TestEnvironment::initializeOptions(argc, argv); benchmark::RunSpecifiedBenchmarks(); } diff --git a/test/integration/BUILD b/test/integration/BUILD index eec00936d1aa6..595bf58235b35 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -2,8 +2,6 @@ licenses(["notice"]) # Apache 2 load( "//bazel:envoy_build_system.bzl", - "envoy_benchmark_test", - "envoy_cc_benchmark_binary", "envoy_cc_fuzz_test", "envoy_cc_test", "envoy_cc_test_library", @@ -288,23 +286,6 @@ envoy_cc_test( ], ) -envoy_cc_benchmark_binary( - name = "http_benchmark", - srcs = ["http_benchmark.cc"], - external_deps = [ - "benchmark", - ], - deps = [ - ":http_integration_lib", - "//source/exe:process_wide_lib", - ], -) - -envoy_benchmark_test( - name = "http_benchmark_test", - benchmark_binary = "http_benchmark", -) - envoy_cc_test( name = "http_subset_lb_integration_test", srcs = [ diff --git a/test/integration/http_benchmark.cc b/test/integration/http_benchmark.cc deleted file mode 100644 index 7074a16c3f581..0000000000000 --- a/test/integration/http_benchmark.cc +++ /dev/null @@ -1,136 +0,0 @@ -#include - -#include "exe/process_wide.h" - -#include "test/integration/http_integration.h" -#include "test/mocks/access_log/mocks.h" - -#include "absl/strings/str_cat.h" -#include "benchmark/benchmark.h" -#include "gmock/gmock.h" - -namespace Envoy { - -namespace { - -// Get a debug string from an object by calling operator<< -template std::string toString(const T& obj) { - std::stringstream ss; - ss << obj; - return ss.str(); -} - -std::string versionString(Http::CodecClient::Type type) { - switch (type) { - case Http::CodecClient::Type::HTTP1: - return "h1"; - case Http::CodecClient::Type::HTTP2: - return "h2"; - case Http::CodecClient::Type::HTTP3: - return "h3"; - } -} - -std::string versionString(FakeHttpConnection::Type type) { - switch (type) { - case FakeHttpConnection::Type::HTTP1: - return "h1"; - case FakeHttpConnection::Type::HTTP2: - return "h2"; - } -} - -} // namespace - -class HttpBenchmarkImpl : public HttpIntegrationTest { -public: - static Network::Address::IpVersion getSupportedIpVersion() { - return TestEnvironment::getIpVersionsForTest().front(); - } - - HttpBenchmarkImpl(Http::CodecClient::Type downstream_protocol) - : HttpIntegrationTest(downstream_protocol, getSupportedIpVersion()) {} - - void initialize() override { - if (!Envoy::Event::Libevent::Global::initialized()) { - Envoy::Event::Libevent::Global::initialize(); - } - HttpIntegrationTest::initialize(); - } - - void doBenchmark(benchmark::State& state, uint64_t request_body_size, uint64_t response_body_size, - bool set_content_length) { - Http::TestRequestHeaderMapImpl request_headers{ - {":method", "POST"}, {":path", "/"}, {":scheme", "http"}, {":authority", "host"}}; - Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; - if (set_content_length) { - request_headers.setContentLength(request_body_size); - response_headers.setContentLength(response_body_size); - } - - autonomous_upstream_ = false; - initialize(); - - for (auto _ : state) { - codec_client_ = makeHttpConnection(lookupPort("http")); - RELEASE_ASSERT(codec_client_->connected(), - std::string(codec_client_->connection()->transportFailureReason())); - - auto response_decoder = sendRequestAndWaitForResponse( - request_headers, request_body_size, default_response_headers_, response_body_size); - RELEASE_ASSERT(response_decoder->complete(), "Expected complete response"); - RELEASE_ASSERT(response_decoder->headers().Status()->value().getStringView() == "200", - toString(response_decoder->headers())); - codec_client_->close(); - codec_client_.reset(); - } - } - - Envoy::Thread::MutexBasicLockable lock_; - Envoy::Logger::Context logging_state_{Envoy::TestEnvironment::getOptions().logLevel(), - Envoy::TestEnvironment::getOptions().logFormat(), lock_, - false}; -}; - -void BM_http(benchmark::State& state) { - Http::CodecClient::Type client_type = static_cast(state.range(0)); - FakeHttpConnection::Type upstream_type = static_cast(state.range(1)); - const uint64_t request_body_size = state.range(2); - const uint64_t response_body_size = state.range(3); - const bool set_content_length = state.range(4); - - if (!Envoy::Event::Libevent::Global::initialized()) { - Envoy::Event::Libevent::Global::initialize(); - } - - state.SetLabel(absl::StrCat("request: ", versionString(client_type), " ", request_body_size, - ", response: ", versionString(upstream_type), " ", response_body_size, - ", ", set_content_length ? "content-length" : "chunked")); - HttpBenchmarkImpl http_benchmark(client_type); - http_benchmark.setUpstreamProtocol(upstream_type); - http_benchmark.doBenchmark(state, request_body_size, response_body_size, set_content_length); -} - -static void BenchmarkArguments(benchmark::internal::Benchmark* b) { - for (bool large_post : {false, true}) { - for (Http::CodecClient::Type client_type : - {Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}) { - for (FakeHttpConnection::Type upstream_type : - {FakeHttpConnection::Type::HTTP1, FakeHttpConnection::Type::HTTP2}) { - for (bool set_content_length : {false, true}) { - int64_t request_body_size = large_post ? 10 * 1024 * 1024 : 0; - int64_t response_body_size = large_post ? 0 : 10 * 1024 * 1024; - b->Args({static_cast(client_type), static_cast(upstream_type), - request_body_size, response_body_size, set_content_length}); - } - } - } - } -} - -BENCHMARK(BM_http) - ->Apply(BenchmarkArguments) - ->MeasureProcessCPUTime() - ->Unit(benchmark::kMillisecond); - -} // namespace Envoy From 334847fe6d5aedeae1a9a0ae12a7d8a194c725ca Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Tue, 24 Mar 2020 12:03:00 -0400 Subject: [PATCH 10/16] make spellchecker happy Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 0d7afe972a936..9aed66107a1b8 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -303,7 +303,7 @@ TEST_F(Http1ServerConnectionImplTest, IdentityEncoding) { EXPECT_EQ(0U, buffer.length()); } -// Verify that data in the two body chunks is merged before the upcall to decodeData. +// Verify that data in the two body chunks is merged before the call to decodeData. TEST_F(Http1ServerConnectionImplTest, ChunkedBody) { initialize(); @@ -366,7 +366,7 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplitOverTwoDispatches) { } // Verify that headers and chunked body are processed correctly and data is merged before the -// decodeData upcall even if delivered in a buffer that holds 1 byte per slice. +// decodeData call even if delivered in a buffer that holds 1 byte per slice. TEST_F(Http1ServerConnectionImplTest, ChunkedBodyFragmentedBuffer) { initialize(); @@ -418,7 +418,7 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodyCase) { EXPECT_EQ(0U, buffer.length()); } -// No decodeData upcall for chunk data handled by the same dispatch call that detects a parse error. +// No decodeData call for chunk data handled by the same dispatch call that detects a parse error. TEST_F(Http1ServerConnectionImplTest, InvalidChunkHeader) { initialize(); @@ -1004,7 +1004,7 @@ TEST_F(Http1ServerConnectionImplTest, PostWithContentLength) { } // Verify that headers and body with content length are processed correctly and data is merged -// before the decodeData upcall even if delivered in a buffer that holds 1 byte per slice. +// before the decodeData call even if delivered in a buffer that holds 1 byte per slice. TEST_F(Http1ServerConnectionImplTest, PostWithContentLengthFragmentedBuffer) { initialize(); From f419bf1065524403fb1ec82fec282cf2850d9f2b Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Wed, 25 Mar 2020 18:55:33 -0400 Subject: [PATCH 11/16] add invalid chunk test Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 24 +++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index efda3e3916163..266eb6d5d5e0c 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -427,6 +427,30 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodyCase) { EXPECT_EQ(0U, buffer.length()); } +TEST_F(Http1ServerConnectionImplTest, InvalidChunkHeader) { + initialize(); + + InSequence sequence; + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "POST"}, + {"transfer-encoding", "chunked"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(decoder, decodeData(_, _)).Times(0); + + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" + "6\r\nHello \r\n" + "invalid\r\nWorl"); + + EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), CodecProtocolException, + "http/1.1 protocol error: HPE_INVALID_CHUNK_SIZE"); +} + TEST_F(Http1ServerConnectionImplTest, IdentityAndChunkedBody) { initialize(); From ddc5a011fe678b552915d8be7c309ce6cd9ecfea Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Wed, 25 Mar 2020 19:27:38 -0400 Subject: [PATCH 12/16] Address review comments: - Add some comments to codec_impl and test - Move test expectations down to improve clarity - Tighten chunked body with trailers test Signed-off-by: Antonio Vicente --- source/common/http/http1/codec_impl.cc | 7 +++++++ test/common/http/http1/codec_impl_test.cc | 22 ++++++++++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 810146ee6a89c..4ff1d350f02ad 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -396,6 +396,10 @@ http_parser_settings ConnectionImpl::settings_{ return 0; }, [](http_parser* parser) -> int { + // A 0-byte chunk header is used to signal the end of the chunked body. + // When this function is called, http-parser holds the size of the chunk in + // parser->content_length. See + // https://github.com/nodejs/http-parser/blob/v2.9.3/http_parser.h#L336 const bool is_final_chunk = (parser->content_length == 0); static_cast(parser->data)->onChunkHeader(is_final_chunk); return 0; @@ -476,6 +480,9 @@ void ConnectionImpl::dispatch(Buffer::Instance& data) { for (const Buffer::RawSlice& slice : data.getRawSlices()) { total_parsed += dispatchSlice(static_cast(slice.mem_), slice.len_); if (HTTP_PARSER_ERRNO(&parser_) != HPE_OK) { + // Parse errors trigger an exception in dispatchSlice so we are guaranteed to be paused at + // this point. + ASSERT(HTTP_PARSER_ERRNO(&parser_) == HPE_PAUSED); break; } } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 266eb6d5d5e0c..f55507ea7f3fa 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -171,14 +171,15 @@ void Http1ServerConnectionImplTest::expectTrailersTest(bool enable_trailers) { EXPECT_CALL(decoder, decodeHeaders_(_, false)); + Buffer::OwnedImpl expected_data("Hello World"); if (enable_trailers) { - EXPECT_CALL(decoder, decodeData(_, false)); + // Verify that body data is delivered before trailers. + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); EXPECT_CALL(decoder, decodeTrailers_); } else { - EXPECT_CALL(decoder, decodeData(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); EXPECT_CALL(decoder, decodeData(_, true)); } - Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" "6\r\nHello \r\n" "5\r\nWorld\r\n" @@ -359,17 +360,20 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodySplitOverTwoDispatches) { EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl expected_data1("Hello Worl"); EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data1), false)); - Buffer::OwnedImpl expected_data2("d"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)); - EXPECT_CALL(decoder, decodeData(_, true)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n" "6\r\nHello \r\n" "5\r\nWorl"); - Buffer::OwnedImpl buffer2("d\r\n" - "0\r\n\r\n"); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); + + // Process the rest of the body and final chunk. + Buffer::OwnedImpl expected_data2("d"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data2), false)); + EXPECT_CALL(decoder, decodeData(_, true)); + + Buffer::OwnedImpl buffer2("d\r\n" + "0\r\n\r\n"); codec_->dispatch(buffer2); EXPECT_EQ(0U, buffer2.length()); } @@ -427,6 +431,8 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedBodyCase) { EXPECT_EQ(0U, buffer.length()); } +// Verify that body dispatch does not happen after detecting a parse error processing a chunk +// header. TEST_F(Http1ServerConnectionImplTest, InvalidChunkHeader) { initialize(); From 010152175d591223725d96c3efbcb2e69963405c Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Wed, 25 Mar 2020 19:45:58 -0400 Subject: [PATCH 13/16] Tighten test that verifies that chunked body is delivered even if an error is found while processing trailers. Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index f55507ea7f3fa..1beeb4d80e642 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -622,7 +622,10 @@ TEST_F(Http1ServerConnectionImplTest, Http11InvalidTrailerPost) { .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); EXPECT_CALL(decoder, decodeHeaders_(_, false)); - EXPECT_CALL(decoder, decodeData(_, false)); + // Verify that body is delivered as soon as the final chunk marker is found, even if an error is + // found while processing trailers. + Buffer::OwnedImpl expected_data("body"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\n" "Host: host\r\n" From 631ea09b0f25508be7eba99cb5f5c718d66e2b40 Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Thu, 26 Mar 2020 11:58:12 -0400 Subject: [PATCH 14/16] expand comments Signed-off-by: Antonio Vicente --- source/common/http/http1/codec_impl.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 5ee61ef417a82..99f4c95283351 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -293,7 +293,13 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Thu, 26 Mar 2020 13:14:16 -0400 Subject: [PATCH 15/16] fix clang-tidy Signed-off-by: Antonio Vicente --- test/common/http/http1/codec_impl_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 1beeb4d80e642..4ba3d14eb046e 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -25,7 +25,6 @@ using testing::_; using testing::InSequence; using testing::Invoke; -using testing::InvokeWithoutArgs; using testing::NiceMock; using testing::Return; using testing::ReturnRef; From 91ff8d6e9980a8f630883667640c651b1f4be13a Mon Sep 17 00:00:00 2001 From: Antonio Vicente Date: Mon, 30 Mar 2020 11:45:40 -0400 Subject: [PATCH 16/16] Kick CI Signed-off-by: Antonio Vicente