Do not 503 on Upgrade: h2c instead remove the header and ignore.

docs/root/intro/version_history.rst

@@ -26,6 +26,7 @@ Version history
 * header to metadata: added :ref:`PROTOBUF_VALUE <envoy_api_enum_value_config.filter.http.header_to_metadata.v2.Config.ValueType.PROTOBUF_VALUE>` and :ref:`ValueEncode <envoy_api_enum_config.filter.http.header_to_metadata.v2.Config.ValueEncode>` to support protobuf Value and Base64 encoding.
 * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
 * http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.
+* http: ignore h2c upgrades for HTTP/1.1 requests which are not currently supported.
 * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out.
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.
 * redis: added :ref:`read_policy <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.ConnPoolSettings.read_policy>` to allow reading from redis replicas for Redis Cluster deployments.

source/common/http/conn_manager_impl.cc

@@ -629,7 +629,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
       Server::OverloadActionState::Active) {
     // In this one special case, do not create the filter chain. If there is a risk of memory
     // overload it is more important to avoid unnecessary allocation than to create the filters.
-    state_.created_filter_chain_ = true;
+    state_.overloaded_do_not_create_filter_chain_ = true;
     connection_manager_.stats_.named_.downstream_rq_overload_close_.inc();
     sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_),
                    Http::Code::ServiceUnavailable, "envoy overloaded", nullptr, is_head_request_,
@@ -758,13 +758,15 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
         cached_route_.value().get());
   }
 
-  const bool upgrade_rejected = createFilterChain() == false;
+  const auto create_result = createFilterChain();
 
   // TODO if there are no filters when starting a filter iteration, the connection manager
   // should return 404. The current returns no response if there is no router filter.
   if (protocol == Protocol::Http11 && hasCachedRoute()) {
-    if (upgrade_rejected) {
+    if (create_result == CreateFilterChainResult::UpgradeRejected ||
+        create_result == CreateFilterChainResult::Overloaded) {
       // Do not allow upgrades if the route does not support it.
+      // NB: includes OVERLOADED and failed h2c upgrades.
       connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc();
       sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "",
                      nullptr, is_head_request_, absl::nullopt,
@@ -1243,9 +1245,7 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply(
   ASSERT(response_headers_ == nullptr);
   // For early error handling, do a best-effort attempt to create a filter chain
   // to ensure access logging.
-  if (!state_.created_filter_chain_) {
-    createFilterChain();
-  }
+  createFilterChain();
   stream_info_.setResponseCodeDetails(details);
   Utility::sendLocalReply(
       is_grpc_request,
@@ -1724,11 +1724,15 @@ void ConnectionManagerImpl::ActiveStream::setBufferLimit(uint32_t new_limit) {
   }
 }
 
-bool ConnectionManagerImpl::ActiveStream::createFilterChain() {
+ConnectionManagerImpl::ActiveStream::CreateFilterChainResult
+ConnectionManagerImpl::ActiveStream::createFilterChain() {
   if (state_.created_filter_chain_) {
-    return false;
+    return CreateFilterChainResult::AlreadyCreated;
   }
-  bool upgrade_rejected = false;
+  if (state_.overloaded_do_not_create_filter_chain_) {
+    return CreateFilterChainResult::Overloaded;
+  }
+  auto result = CreateFilterChainResult::Created;
   auto upgrade = request_headers_ ? request_headers_->Upgrade() : nullptr;
   state_.created_filter_chain_ = true;
   if (upgrade != nullptr) {
@@ -1745,16 +1749,25 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() {
       state_.successful_upgrade_ = true;
       connection_manager_.stats_.named_.downstream_cx_upgrades_total_.inc();
       connection_manager_.stats_.named_.downstream_cx_upgrades_active_.inc();
-      return true;
+      return CreateFilterChainResult::Upgraded;
     } else {
-      upgrade_rejected = true;
-      // Fall through to the default filter chain. The function calling this
-      // will send a local reply indicating that the upgrade failed.
+      // Ignore h2c upgrade requests until we support them.
+      // See https://github.com/envoyproxy/envoy/issues/7161 for details.
+      if (absl::EqualsIgnoreCase(request_headers_->Upgrade()->value().getStringView(),
+                                 Http::Headers::get().UpgradeValues.H2c)) {
+        request_headers_->removeUpgrade();
+        result = CreateFilterChainResult::UpgradeIgnored;
+        // Fall through to the default filter chain.
+      } else {
+        result = CreateFilterChainResult::UpgradeRejected;
+        // Fall through to the default filter chain. The function calling this
+        // will send a local reply indicating that the upgrade failed.
+      }
     }
   }
 
   connection_manager_.config_.filterFactory().createFilterChain(*this);
-  return !upgrade_rejected;
+  return result;
 }
 
 void ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue() {

source/common/http/conn_manager_impl.h

@@ -530,7 +530,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
       State()
           : remote_complete_(false), local_complete_(false), codec_saw_local_complete_(false),
             saw_connection_close_(false), successful_upgrade_(false), created_filter_chain_(false),
-            is_internally_created_(false) {}
+            overloaded_do_not_create_filter_chain_(false), is_internally_created_(false) {}
 
       uint32_t filter_call_state_{0};
       // The following 3 members are booleans rather than part of the space-saving bitfield as they
@@ -548,6 +548,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
       bool saw_connection_close_ : 1;
       bool successful_upgrade_ : 1;
       bool created_filter_chain_ : 1;
+      bool overloaded_do_not_create_filter_chain_ : 1;
 
       // True if this stream is internally created. Currently only used for
       // internal redirects or other streams created via recreateStream().
@@ -561,7 +562,15 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     // Possibly increases buffer_limit_ to the value of limit.
     void setBufferLimit(uint32_t limit);
     // Set up the Encoder/Decoder filter chain.
-    bool createFilterChain();
+    enum class CreateFilterChainResult {
+      Overloaded,      // Filter chain not created.
+      AlreadyCreated,  // Filter chain not created (again).
+      UpgradeRejected, // Filter chain not created.
+      UpgradeIgnored,  // Filter chain created (e.g. http1 for Upgrade: h2c).
+      Created,         // Filter chain created, no Upgrade header.
+      Upgraded,        // Upgraded filter chain created.
+    };
+    CreateFilterChainResult createFilterChain();
     // Per-stream idle timeout callback.
     void onIdleTimeout();
     // Reset per-stream idle timer.

source/common/http/headers.h

@@ -158,6 +158,7 @@ class HeaderValues {
   } ConnectionValues;
 
   struct {
+    const std::string H2c{"h2c"};
     const std::string WebSocket{"websocket"};
   } UpgradeValues;
 

test/common/http/conn_manager_impl_test.cc

@@ -2108,6 +2108,33 @@ TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) {
   conn_manager_->onData(fake_input, false);
 }
 
+TEST_F(HttpConnectionManagerImplTest, IgnoreUpgradeH2c) {
+  setup(false, "");
+  StreamDecoder* decoder = nullptr;
+  HeaderMap* header_map = nullptr;
+
+  NiceMock<MockStreamEncoder> encoder;
+  EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void {
+    decoder = &conn_manager_->newStream(encoder);
+    HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"},
+                                               {":method", "GET"},
+                                               {":path", "/"},
+                                               {"connection", "Upgrade"},
+                                               {"upgrade", "h2c"}}};
+    header_map = headers.get();
+    decoder->decodeHeaders(std::move(headers), true);
+    data.drain(4);
+  }));
+
+  Buffer::OwnedImpl fake_input("1234");
+  conn_manager_->onData(fake_input, false);
+
+  // The h2c header should be removed.
+  EXPECT_TRUE(header_map->Upgrade() == nullptr);
+
+  EXPECT_EQ(0U, stats_.named_.downstream_rq_ws_on_non_ws_route_.value());
+}
+
 TEST_F(HttpConnectionManagerImplTest, DrainClose) {
   setup(true, "");