http: fixing a bug with HTTP/1.0 keep-alive. (#9843)

Signed-off-by: Alyssa Wilk <[email protected]>

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 <config_overview_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. <envoy_api_field_route.RetryPolicy.retry_host_predicate>`
 * upstream: combined HTTP/1 and HTTP/2 connection pool code. This means that circuit breaker
   limits for both requests and connections apply to both pool types. Also, HTTP/2 now has

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;

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<Http::TestHeaderMapImpl> AutonomousUpstream::lastRequestHeaders(
   return std::move(last_request_headers_);
 }
 
+void AutonomousUpstream::setResponseHeaders(
+    std::unique_ptr<Http::TestHeaderMapImpl>&& 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

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>(
+            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>(
+            Http::TestHeaderMapImpl({{":status", "200"}}))) {}
 
   ~AutonomousUpstream() override;
   bool
@@ -71,11 +76,14 @@ class AutonomousUpstream : public FakeUpstream {
 
   void setLastRequestHeaders(const Http::HeaderMap& headers);
   std::unique_ptr<Http::TestHeaderMapImpl> lastRequestHeaders();
+  void setResponseHeaders(std::unique_ptr<Http::TestHeaderMapImpl>&& response_headers);
+  Http::TestHeaderMapImpl responseHeaders();
   const bool allow_incomplete_streams_{false};
 
 private:
   Thread::MutexBasicLockable headers_lock_;
   std::unique_ptr<Http::TestHeaderMapImpl> last_request_headers_;
+  std::unique_ptr<Http::TestHeaderMapImpl> response_headers_;
   std::vector<AutonomousHttpConnectionPtr> http_connections_;
   std::vector<SharedConnectionWrapperPtr> shared_connections_;
 };

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();
@@ -560,7 +560,9 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) {
                                 "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: 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<Http::TestHeaderMapImpl> 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<AutonomousUpstream*>(fake_upstreams_.front().get())
+      ->setResponseHeaders(std::make_unique<Http::TestHeaderMapImpl>(
+          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();