From 6035d30d4d30821463b015ed7488cd346b608115 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 27 Jan 2020 13:57:15 -0500 Subject: [PATCH 1/3] http: fixing HTTP/1.0 response headers Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + source/common/http/http1/codec_impl.cc | 8 ++++++++ test/common/http/http1/codec_impl_test.cc | 3 ++- test/integration/autonomous_upstream.cc | 14 +++++++++++++- test/integration/autonomous_upstream.h | 12 ++++++++++-- test/integration/integration_test.cc | 22 +++++++++++++++++++++- 6 files changed, 55 insertions(+), 5 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 97eef6756228..e47b75a266d6 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,6 +4,7 @@ Version history 1.14.0 (Pending) ================ * config: use type URL to select an extension whenever the config type URL (or its previous versions) uniquely identify a typed extension, see :ref:`extension configuration `. +* http: fixing a bug in HTTP/1.0 responses where Connection: keep-alive was not appended for connections which were kept alive. * retry: added a retry predicate that :ref:`rejects hosts based on metadata. ` * upstream: changed load distribution algorithm when all priorities enter :ref:`panic mode`. diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 2bc32b6d73aa..f210604b3316 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -156,6 +156,7 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) // For 204s and 1xx where content length is disallowed, don't append the content length but // also don't chunk encode. if (is_content_length_allowed_) { + saw_content_length = true; encodeFormattedHeader(Headers::get().ContentLength.get(), "0"); } chunk_encoding_ = false; @@ -174,6 +175,13 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) chunk_encoding_ = !Utility::isUpgrade(headers) && !is_response_to_head_request_; } } + // For HTTP/1.0 If the HCM doesn't see a Connection: Keep-Alive in the request, it will add a + // Connection: Close to the response. If no Connection: Close is present and there's a content + // length header, add the Keep-Alive. + if (connection_.protocol() == Protocol::Http10 && saw_content_length && !headers.Connection()) { + encodeFormattedHeader(Headers::get().Connection.get(), + Headers::get().ConnectionValues.KeepAlive); + } connection_.reserveBuffer(2); connection_.addCharToBuffer('\r'); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index d1ad84afb984..cabe96fe26c7 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -385,6 +385,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10Absolute) { } TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { + codec_settings_.accept_http_10_ = true; initialize(); Http::MockStreamDecoder decoder; @@ -406,7 +407,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestHeaderMapImpl headers{{":status", "200"}}; response_encoder->encodeHeaders(headers, true); - EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); + EXPECT_EQ("HTTP/1.0 200 OK\r\ncontent-length: 0\r\nconnection: keep-alive\r\n\r\n", output); EXPECT_EQ(Protocol::Http10, codec_->protocol()); } diff --git a/test/integration/autonomous_upstream.cc b/test/integration/autonomous_upstream.cc index 997f28cad759..c96cb8d23e52 100644 --- a/test/integration/autonomous_upstream.cc +++ b/test/integration/autonomous_upstream.cc @@ -58,7 +58,7 @@ void AutonomousStream::sendResponse() { int32_t response_body_length = 10; HeaderToInt(RESPONSE_SIZE_BYTES, response_body_length, headers); - encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, false); + encodeHeaders(upstream_.responseHeaders(), false); encodeData(response_body_length, true); } @@ -109,4 +109,16 @@ std::unique_ptr AutonomousUpstream::lastRequestHeaders( return std::move(last_request_headers_); } +void AutonomousUpstream::setResponseHeaders( + std::unique_ptr&& response_headers) { + Thread::LockGuard lock(headers_lock_); + response_headers_ = std::move(response_headers); +} + +Http::TestHeaderMapImpl AutonomousUpstream::responseHeaders() { + Thread::LockGuard lock(headers_lock_); + Http::TestHeaderMapImpl return_headers = *response_headers_; + return return_headers; +} + } // namespace Envoy diff --git a/test/integration/autonomous_upstream.h b/test/integration/autonomous_upstream.h index 4cda07a7ad2e..89161dab65e3 100644 --- a/test/integration/autonomous_upstream.h +++ b/test/integration/autonomous_upstream.h @@ -54,12 +54,17 @@ class AutonomousUpstream : public FakeUpstream { FakeHttpConnection::Type type, Event::TestTimeSystem& time_system, bool allow_incomplete_streams) : FakeUpstream(address, type, time_system), - allow_incomplete_streams_(allow_incomplete_streams) {} + allow_incomplete_streams_(allow_incomplete_streams), + response_headers_(std::make_unique( + Http::TestHeaderMapImpl({{":status", "200"}}))) {} + AutonomousUpstream(Network::TransportSocketFactoryPtr&& transport_socket_factory, uint32_t port, FakeHttpConnection::Type type, Network::Address::IpVersion version, Event::TestTimeSystem& time_system, bool allow_incomplete_streams) : FakeUpstream(std::move(transport_socket_factory), port, type, version, time_system), - allow_incomplete_streams_(allow_incomplete_streams) {} + allow_incomplete_streams_(allow_incomplete_streams), + response_headers_(std::make_unique( + Http::TestHeaderMapImpl({{":status", "200"}}))) {} ~AutonomousUpstream() override; bool @@ -71,11 +76,14 @@ class AutonomousUpstream : public FakeUpstream { void setLastRequestHeaders(const Http::HeaderMap& headers); std::unique_ptr lastRequestHeaders(); + void setResponseHeaders(std::unique_ptr&& response_headers); + Http::TestHeaderMapImpl responseHeaders(); const bool allow_incomplete_streams_{false}; private: Thread::MutexBasicLockable headers_lock_; std::unique_ptr last_request_headers_; + std::unique_ptr response_headers_; std::vector http_connections_; std::vector shared_connections_; }; diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 0dbddb5ffcf1..8d7a9f147ffc 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -551,7 +551,7 @@ TEST_P(IntegrationTest, TestInlineHeaders) { // Also verify existing host headers are passed through for the HTTP/1.0 case. // This also regression tests proper handling of trailing whitespace after key // values, specifically the host header. -TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { +TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLwsNoContentLength) { autonomous_upstream_ = true; config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost); initialize(); @@ -561,6 +561,8 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { &response, true); EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); EXPECT_THAT(response, Not(HasSubstr("connection: close"))); + EXPECT_THAT(response, Not(HasSubstr("connection: keep-alive"))); + EXPECT_THAT(response, Not(HasSubstr("content-length:"))); EXPECT_THAT(response, Not(HasSubstr("transfer-encoding: chunked\r\n"))); std::unique_ptr upstream_headers = @@ -569,6 +571,24 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { EXPECT_EQ(upstream_headers->Host()->value(), "foo.com"); } +TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndContentLengthAndLws) { + autonomous_upstream_ = true; + config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost); + initialize(); + reinterpret_cast(fake_upstreams_.front().get()) + ->setResponseHeaders(std::make_unique( + Http::TestHeaderMapImpl({{":status", "200"}, {"content-length", "10"}}))); + std::string response; + sendRawHttpAndWaitForResponse(lookupPort("http"), + "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", + &response, true); + EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); + EXPECT_THAT(response, Not(HasSubstr("connection: close"))); + EXPECT_THAT(response, HasSubstr("connection: keep-alive")); + EXPECT_THAT(response, HasSubstr("content-length:")); + EXPECT_THAT(response, Not(HasSubstr("transfer-encoding: chunked\r\n"))); +} + TEST_P(IntegrationTest, Pipeline) { autonomous_upstream_ = true; initialize(); From 83940a6afeb80358bc7ef04a3f6e6822a25f73a2 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 30 Jan 2020 11:31:15 -0500 Subject: [PATCH 2/3] moving HTTP/1.0 logic Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 13 +++++++++++++ source/common/http/http1/codec_impl.cc | 8 -------- test/common/http/http1/codec_impl_test.cc | 3 +-- test/integration/integration_test.cc | 1 - 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 4f730bf73033..3c4956339e7f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1549,6 +1549,19 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte ENVOY_STREAM_LOG(debug, "drain closing connection", *this); } + if (connection_manager_.codec_->protocol() == Protocol::Http10) { + // As HTTP/1.0 and below can not do chunked encoding, if there is no content + // length the response will be framed by connection close. + if (!headers.ContentLength()) { + state_.saw_connection_close_ = true; + } + // If the request came with a keep-alive and no other factor resulted in a + // connection close header, send an explicit keep-alive header. + if (!state_.saw_connection_close_) { + headers.setConnection(Headers::get().ConnectionValues.KeepAlive); + } + } + if (connection_manager_.drain_state_ == DrainState::NotDraining && state_.saw_connection_close_) { ENVOY_STREAM_LOG(debug, "closing connection due to connection close header", *this); connection_manager_.drain_state_ = DrainState::Closing; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index f210604b3316..2bc32b6d73aa 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -156,7 +156,6 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) // For 204s and 1xx where content length is disallowed, don't append the content length but // also don't chunk encode. if (is_content_length_allowed_) { - saw_content_length = true; encodeFormattedHeader(Headers::get().ContentLength.get(), "0"); } chunk_encoding_ = false; @@ -175,13 +174,6 @@ void StreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) chunk_encoding_ = !Utility::isUpgrade(headers) && !is_response_to_head_request_; } } - // For HTTP/1.0 If the HCM doesn't see a Connection: Keep-Alive in the request, it will add a - // Connection: Close to the response. If no Connection: Close is present and there's a content - // length header, add the Keep-Alive. - if (connection_.protocol() == Protocol::Http10 && saw_content_length && !headers.Connection()) { - encodeFormattedHeader(Headers::get().Connection.get(), - Headers::get().ConnectionValues.KeepAlive); - } connection_.reserveBuffer(2); connection_.addCharToBuffer('\r'); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index cabe96fe26c7..d1ad84afb984 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -385,7 +385,6 @@ TEST_F(Http1ServerConnectionImplTest, Http10Absolute) { } TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { - codec_settings_.accept_http_10_ = true; initialize(); Http::MockStreamDecoder decoder; @@ -407,7 +406,7 @@ TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); TestHeaderMapImpl headers{{":status", "200"}}; response_encoder->encodeHeaders(headers, true); - EXPECT_EQ("HTTP/1.0 200 OK\r\ncontent-length: 0\r\nconnection: keep-alive\r\n\r\n", output); + EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); EXPECT_EQ(Protocol::Http10, codec_->protocol()); } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 8d7a9f147ffc..caab7e463380 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -560,7 +560,6 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLwsNoContentLength) { "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", &response, true); EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); - EXPECT_THAT(response, Not(HasSubstr("connection: close"))); EXPECT_THAT(response, Not(HasSubstr("connection: keep-alive"))); EXPECT_THAT(response, Not(HasSubstr("content-length:"))); EXPECT_THAT(response, Not(HasSubstr("transfer-encoding: chunked\r\n"))); From c54936877f1de8838750717720894f8461284f12 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 30 Jan 2020 14:38:24 -0500 Subject: [PATCH 3/3] test fix Signed-off-by: Alyssa Wilk --- test/integration/integration_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index caab7e463380..a1085f92af05 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -560,6 +560,7 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLwsNoContentLength) { "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", &response, true); EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); + EXPECT_THAT(response, HasSubstr("connection: close")); EXPECT_THAT(response, Not(HasSubstr("connection: keep-alive"))); EXPECT_THAT(response, Not(HasSubstr("content-length:"))); EXPECT_THAT(response, Not(HasSubstr("transfer-encoding: chunked\r\n")));