http/1.1: correctly handle connection header for for incoming requests

include/envoy/http/access_log.h

@@ -5,6 +5,7 @@
 #include "envoy/common/pure.h"
 #include "envoy/common/time.h"
 #include "envoy/http/header_map.h"
+#include "envoy/http/protocol.h"
 #include "envoy/upstream/upstream.h"
 
 namespace Http {
@@ -64,7 +65,12 @@ class RequestInfo {
   /**
    * @return the protocol of the request.
    */
-  virtual const std::string& protocol() const PURE;
+  virtual Protocol protocol() const PURE;
+
+  /**
+   * Set the request's protocol.
+   */
+  virtual void protocol(Protocol protocol) PURE;
 
   /**
    * @return the response code.

include/envoy/http/codec.h

@@ -3,6 +3,7 @@
 #include "envoy/buffer/buffer.h"
 #include "envoy/common/pure.h"
 #include "envoy/http/header_map.h"
+#include "envoy/http/protocol.h"
 
 namespace Http {
 
@@ -144,14 +145,6 @@ class ConnectionCallbacks {
   virtual void onGoAway() PURE;
 };
 
-/**
- * A list of features that a codec provides.
- */
-class CodecFeatures {
-public:
-  static const uint64_t Multiplexing = 0x1;
-};
-
 /**
  * A list of options that can be specified when creating a codec.
  */
@@ -173,20 +166,16 @@ class Connection {
    */
   virtual void dispatch(Buffer::Instance& data) PURE;
 
-  /**
-   * Get the features that a connection provides. Maps to entries in CodecFeatures.
-   */
-  virtual uint64_t features() PURE;
-
   /**
    * Indicate "go away" to the remote. No new streams can be created beyond this point.
    */
   virtual void goAway() PURE;
 
   /**
-   * @return const std::string& the human readable name of the protocol that this codec wraps.
+   * @return the protocol backing the connection. This can change if for example an HTTP/1.1
+   *         connection gets an HTTP/1.0 request on it.
    */
-  virtual const std::string& protocolString() PURE;
+  virtual Protocol protocol() PURE;
 
   /**
    * Indicate a "shutdown notice" to the remote. This is a hint that the remote should not send

include/envoy/http/header_map.h

@@ -190,7 +190,6 @@ class HeaderEntry {
   HEADER_FUNC(EnvoyInternalRequest)                                                                \
   HEADER_FUNC(EnvoyMaxRetries)                                                                     \
   HEADER_FUNC(EnvoyOriginalPath)                                                                   \
-  HEADER_FUNC(EnvoyProtocolVersion)                                                                \
   HEADER_FUNC(EnvoyRetryOn)                                                                        \
   HEADER_FUNC(EnvoyUpstreamAltStatName)                                                            \
   HEADER_FUNC(EnvoyUpstreamCanary)                                                                 \
@@ -214,8 +213,7 @@ class HeaderEntry {
   HEADER_FUNC(Status)                                                                              \
   HEADER_FUNC(TransferEncoding)                                                                    \
   HEADER_FUNC(Upgrade)                                                                             \
-  HEADER_FUNC(UserAgent)                                                                           \
-  HEADER_FUNC(Version)
+  HEADER_FUNC(UserAgent)
 
 /**
  * The following functions are defined for each inline header above. E.g., for ContentLength we

include/envoy/http/protocol.h

@@ -0,0 +1,10 @@
+#pragma once
+
+namespace Http {
+
+/**
+ * Possible HTTP connection/request protocols.
+ */
+enum class Protocol { Http10, Http11, Http2 };
+
+} // Http

source/common/grpc/http1_bridge_filter.cc

@@ -30,8 +30,7 @@ Http::FilterHeadersStatus Http1BridgeFilter::decodeHeaders(Http::HeaderMap& head
     setupStatTracking(headers);
   }
 
-  if (decoder_callbacks_->requestInfo().protocol() == Http::Http1::PROTOCOL_STRING &&
-      grpc_request) {
+  if (decoder_callbacks_->requestInfo().protocol() != Http::Protocol::Http2 && grpc_request) {
     do_bridging_ = true;
   }
 

source/common/http/access_log/access_log_formatter.cc

@@ -1,5 +1,6 @@
 #include "access_log_formatter.h"
 
+#include "common/common/assert.h"
 #include "common/common/utility.h"
 
 namespace Http {
@@ -54,6 +55,23 @@ FormatterPtr AccessLogFormatUtils::defaultAccessLogFormatter() {
   return FormatterPtr{new FormatterImpl(DEFAULT_FORMAT)};
 }
 
+static const std::string Http10String = "HTTP/1.0";
+static const std::string Http11String = "HTTP/1.1";
+static const std::string Http2String = "HTTP/2";
+
+const std::string& AccessLogFormatUtils::protocolToString(Protocol protocol) {
+  switch (protocol) {
+  case Protocol::Http10:
+    return Http10String;
+  case Protocol::Http11:
+    return Http11String;
+  case Protocol::Http2:
+    return Http2String;
+  }
+
+  NOT_IMPLEMENTED;
+}
+
 FormatterImpl::FormatterImpl(const std::string& format) {
   formatters_ = AccessLogFormatParser::parse(format);
 }
@@ -173,7 +191,9 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) {
       return std::to_string(request_info.bytesReceived());
     };
   } else if (field_name == "PROTOCOL") {
-    field_extractor_ = [](const RequestInfo& request_info) { return request_info.protocol(); };
+    field_extractor_ = [](const RequestInfo& request_info) {
+      return AccessLogFormatUtils::protocolToString(request_info.protocol());
+    };
   } else if (field_name == "RESPONSE_CODE") {
     field_extractor_ = [](const RequestInfo& request_info) {
       return request_info.responseCode().valid()

source/common/http/access_log/access_log_formatter.h

@@ -45,9 +45,10 @@ class AccessLogFormatParser {
 class AccessLogFormatUtils {
 public:
   static FormatterPtr defaultAccessLogFormatter();
+  static const std::string& protocolToString(Protocol protocol);
 
 private:
-  AccessLogFormatUtils(){};
+  AccessLogFormatUtils();
 
   static const std::string DEFAULT_FORMAT;
 };

source/common/http/access_log/request_info_impl.h

@@ -6,15 +6,16 @@ namespace Http {
 namespace AccessLog {
 
 struct RequestInfoImpl : public RequestInfo {
-  RequestInfoImpl(const std::string& protocol)
+  RequestInfoImpl(Protocol protocol)
       : protocol_(protocol), start_time_(std::chrono::system_clock::now()) {}
 
   // Http::AccessLog::RequestInfo
   SystemTime startTime() const override { return start_time_; }
 
   uint64_t bytesReceived() const override { return bytes_received_; }
 
-  const std::string& protocol() const override { return protocol_; }
+  Protocol protocol() const override { return protocol_; }
+  void protocol(Protocol protocol) override { protocol_ = protocol; }
 
   const Optional<uint32_t>& responseCode() const override { return response_code_; }
 
@@ -39,7 +40,7 @@ struct RequestInfoImpl : public RequestInfo {
 
   void healthCheck(bool is_hc) override { hc_request_ = is_hc; }
 
-  const std::string& protocol_;
+  Protocol protocol_;
   const SystemTime start_time_;
   uint64_t bytes_received_{};
   Optional<uint32_t> response_code_;

source/common/http/async_client_impl.cc

@@ -38,7 +38,7 @@ AsyncRequestImpl::AsyncRequestImpl(MessagePtr&& request, AsyncClientImpl& parent
                                    const Optional<std::chrono::milliseconds>& timeout)
     : request_(std::move(request)), parent_(parent), callbacks_(callbacks),
       stream_id_(parent.config_.random_.random()), router_(parent.config_),
-      request_info_(EMPTY_STRING), route_(parent_.cluster_.name(), timeout) {
+      request_info_(Protocol::Http11), route_(parent_.cluster_.name(), timeout) {
 
   router_.setDecoderFilterCallbacks(*this);
   request_->headers().insertEnvoyInternalRequest().value(
@@ -50,6 +50,7 @@ AsyncRequestImpl::AsyncRequestImpl(MessagePtr&& request, AsyncClientImpl& parent
   }
 
   // TODO: Support request trailers.
+  // TODO: Correctly set protocol in request info when we support access logging.
 }
 
 AsyncRequestImpl::~AsyncRequestImpl() { ASSERT(!reset_callback_); }

source/common/http/conn_manager_impl.cc

@@ -12,6 +12,7 @@
 #include "common/buffer/buffer_impl.h"
 #include "common/common/assert.h"
 #include "common/common/enum_to_int.h"
+#include "common/common/utility.h"
 #include "common/http/codes.h"
 #include "common/http/exception.h"
 #include "common/http/header_map_impl.h"
@@ -66,11 +67,10 @@ ConnectionManagerImpl::~ConnectionManagerImpl() {
   }
 
   if (codec_) {
-    if (codec_->protocolString() == Http1::PROTOCOL_STRING) {
-      stats_.named_.downstream_cx_http1_active_.dec();
-    } else {
-      ASSERT(codec_->protocolString() == Http2::PROTOCOL_STRING);
+    if (codec_->protocol() == Protocol::Http2) {
       stats_.named_.downstream_cx_http2_active_.dec();
+    } else {
+      stats_.named_.downstream_cx_http1_active_.dec();
     }
   }
 
@@ -104,14 +104,14 @@ void ConnectionManagerImpl::destroyStream(ActiveStream& stream) {
     read_callbacks_->connection().dispatcher().deferredDelete(stream.removeFromList(streams_));
   }
 
-  if (reset_stream && !(codec_->features() & CodecFeatures::Multiplexing)) {
+  if (reset_stream && codec_->protocol() != Protocol::Http2) {
     drain_state_ = DrainState::Closing;
   }
 
   checkForDeferredClose();
 
   // Reading may have been disabled for the non-multiplexing case, so enable it again.
-  if (drain_state_ != DrainState::Closing && !(codec_->features() & CodecFeatures::Multiplexing) &&
+  if (drain_state_ != DrainState::Closing && codec_->protocol() != Protocol::Http2 &&
       !read_callbacks_->connection().readEnabled()) {
     read_callbacks_->connection().readDisable(false);
   }
@@ -138,13 +138,12 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder)
 Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data) {
   if (!codec_) {
     codec_ = config_.createCodec(read_callbacks_->connection(), data, *this);
-    if (codec_->protocolString() == Http1::PROTOCOL_STRING) {
-      stats_.named_.downstream_cx_http1_total_.inc();
-      stats_.named_.downstream_cx_http1_active_.inc();
-    } else {
-      ASSERT(codec_->protocolString() == Http2::PROTOCOL_STRING);
+    if (codec_->protocol() == Protocol::Http2) {
       stats_.named_.downstream_cx_http2_total_.inc();
       stats_.named_.downstream_cx_http2_active_.inc();
+    } else {
+      stats_.named_.downstream_cx_http1_total_.inc();
+      stats_.named_.downstream_cx_http1_active_.inc();
     }
   }
 
@@ -172,10 +171,10 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data) {
     // Processing incoming data may release outbound data so check for closure here as well.
     checkForDeferredClose();
 
-    // The HTTP/1.1 codec will pause dispatch after a single message is complete. We want to
+    // The HTTP/1 codec will pause dispatch after a single message is complete. We want to
     // either redispatch if there are no streams and we have more data, or if we have a single
     // complete stream but have not responded yet we will pause socket reads to apply back pressure.
-    if (!(codec_->features() & CodecFeatures::Multiplexing)) {
+    if (codec_->protocol() != Protocol::Http2) {
       if (read_callbacks_->connection().state() == Network::Connection::State::Open &&
           data.length() > 0 && streams_.empty()) {
         redispatch = true;
@@ -268,14 +267,13 @@ std::atomic<uint64_t> ConnectionManagerImpl::ActiveStream::next_stream_id_(0);
 ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connection_manager)
     : connection_manager_(connection_manager), stream_id_(next_stream_id_++),
       request_timer_(connection_manager_.stats_.named_.downstream_rq_time_.allocateSpan()),
-      request_info_(connection_manager_.codec_->protocolString()) {
+      request_info_(connection_manager_.codec_->protocol()) {
   connection_manager_.stats_.named_.downstream_rq_total_.inc();
   connection_manager_.stats_.named_.downstream_rq_active_.inc();
-  if (connection_manager_.codec_->protocolString() == Http1::PROTOCOL_STRING) {
-    connection_manager_.stats_.named_.downstream_rq_http1_total_.inc();
-  } else {
-    ASSERT(connection_manager_.codec_->protocolString() == Http2::PROTOCOL_STRING);
+  if (connection_manager_.codec_->protocol() == Protocol::Http2) {
     connection_manager_.stats_.named_.downstream_rq_http2_total_.inc();
+  } else {
+    connection_manager_.stats_.named_.downstream_rq_http1_total_.inc();
   }
 }
 
@@ -349,8 +347,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
       *request_headers_, connection_manager_.stats_.prefix_, connection_manager_.stats_.store_);
 
   // Make sure we are getting a codec version we support.
-  const HeaderString& codec_version = request_headers_->Version()->value();
-  if (!(codec_version == "HTTP/1.1" || codec_version == "HTTP/2")) {
+  Protocol protocol = connection_manager_.codec_->protocol();
+  if (protocol == Protocol::Http10) {
+    // The protocol may have shifted in the HTTP/1.0 case so reset it.
+    request_info_.protocol(protocol);
     HeaderMapImpl headers{
         {Headers::get().Status, std::to_string(enumToInt(Code::UpgradeRequired))}};
     encodeHeaders(nullptr, headers, true);
@@ -391,6 +391,13 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
     return;
   }
 
+  if (protocol == Protocol::Http11 && request_headers_->Connection() &&
+      0 ==
+          StringUtil::caseInsensitiveCompare(request_headers_->Connection()->value().c_str(),
+                                             Http::Headers::get().ConnectionValues.Close.c_str())) {
+    state_.saw_connection_close_ = true;
+  }
+
   ConnectionManagerUtility::mutateRequestHeaders(
       *request_headers_, connection_manager_.read_callbacks_->connection(),
       connection_manager_.config_, connection_manager_.random_generator_,
@@ -528,7 +535,6 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
   // Base headers.
   connection_manager_.config_.dateProvider().setDateHeader(headers);
   headers.insertServer().value(connection_manager_.config_.serverName());
-  headers.insertEnvoyProtocolVersion().value(connection_manager_.codec_->protocolString());
   ConnectionManagerUtility::mutateResponseHeaders(headers, *request_headers_,
                                                   connection_manager_.config_);
 
@@ -545,19 +551,24 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
     stream_log_debug("drain closing connection", *this);
   }
 
+  if (connection_manager_.drain_state_ == DrainState::NotDraining && state_.saw_connection_close_) {
+    stream_log_debug("closing connection due to connection close header", *this);
+    connection_manager_.drain_state_ = DrainState::Closing;
+  }
+
   // If we are destroying a stream before remote is complete and the connection does not support
   // multiplexing, we should disconnect since we don't want to wait around for the request to
   // finish.
   if (!state_.remote_complete_) {
-    if (!(connection_manager_.codec_->features() & CodecFeatures::Multiplexing)) {
+    if (connection_manager_.codec_->protocol() != Protocol::Http2) {
       connection_manager_.drain_state_ = DrainState::Closing;
     }
 
     connection_manager_.stats_.named_.downstream_rq_response_before_rq_complete_.inc();
   }
 
   if (connection_manager_.drain_state_ == DrainState::Closing &&
-      !(connection_manager_.codec_->features() & CodecFeatures::Multiplexing)) {
+      connection_manager_.codec_->protocol() != Protocol::Http2) {
     headers.insertConnection().value(Headers::get().ConnectionValues.Close);
   }
 

source/common/http/conn_manager_impl.h

@@ -379,8 +379,11 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     // All state for the stream. Put here for readability. We could move this to a bit field
     // eventually if we want.
     struct State {
-      bool remote_complete_{};
-      bool local_complete_{};
+      State() : remote_complete_(false), local_complete_(false), saw_connection_close_(false) {}
+
+      bool remote_complete_ : 1;
+      bool local_complete_ : 1;
+      bool saw_connection_close_ : 1;
     };
 
     // NOTE: This is used for stable randomness. For performance reasons we use an incrementing

source/common/http/conn_manager_utility.cc

@@ -20,7 +20,6 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
   request_headers.removeProxyConnection();
   request_headers.removeTransferEncoding();
   request_headers.removeUpgrade();
-  request_headers.removeVersion();
 
   // If we are "using remote address" this means that we create/append to XFF with our immediate
   // peer. Cases where we don't "use remote address" include trusted double proxy where we expect
@@ -111,7 +110,6 @@ void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_h
                                                      ConnectionManagerConfig& config) {
   response_headers.removeConnection();
   response_headers.removeTransferEncoding();
-  response_headers.removeVersion();
 
   for (const Http::LowerCaseString& to_remove : config.routeConfig().responseHeadersToRemove()) {
     response_headers.remove(to_remove);