websocket: fixing websocket to consistently not send connection: clos…

…e when draining (#3952)

Using the isUpgrade utility for consistent handling of upgrade strings w.r.t. case sensitivity.

Risk Level: Low (should only affect WebSocket, only when draining)
Testing: new regression unit test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/common/http/conn_manager_impl.cc

@@ -1006,7 +1006,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
     // If the connection manager is draining send "Connection: Close" on HTTP/1.1 connections.
     // Do not do this for H2 (which drains via GOAWA) or Upgrade (as the upgrade
     // payload is no longer HTTP/1.1)
-    if (headers.Connection() == nullptr || headers.Connection()->value() != "Upgrade") {
+    if (!Utility::isUpgrade(headers)) {
       headers.insertConnection().value().setReference(Headers::get().ConnectionValues.Close);
     }
   }

test/common/http/conn_manager_impl_test.cc

@@ -1856,6 +1856,65 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketEarlyEndStream) {
   conn_manager_.reset();
 }
 
+// Make sure for upgrades, we do not append Connection: Close when draining.
+TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) {
+  setup(false, "envoy-custom-server", false);
+
+  // Store the basic request encoder during filter chain setup.
+  MockStreamFilter* filter = new MockStreamFilter();
+  EXPECT_CALL(drain_close_, drainClose()).WillOnce(Return(true));
+
+  EXPECT_CALL(*filter, decodeHeaders(_, false))
+      .WillRepeatedly(Invoke([&](HeaderMap&, bool) -> FilterHeadersStatus {
+        return FilterHeadersStatus::StopIteration;
+      }));
+
+  EXPECT_CALL(*filter, encodeHeaders(_, false))
+      .WillRepeatedly(Invoke(
+          [&](HeaderMap&, bool) -> FilterHeadersStatus { return FilterHeadersStatus::Continue; }));
+
+  NiceMock<MockStreamEncoder> encoder;
+  EXPECT_CALL(encoder, encodeHeaders(_, false))
+      .WillOnce(Invoke([&](const HeaderMap& headers, bool) -> void {
+        EXPECT_NE(nullptr, headers.Connection());
+        EXPECT_STREQ("upgrade", headers.Connection()->value().c_str());
+      }));
+
+  EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)).Times(1);
+  EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)).Times(1);
+
+  EXPECT_CALL(filter_factory_, createUpgradeFilterChain(_, _))
+      .WillRepeatedly(
+          Invoke([&](absl::string_view, FilterChainFactoryCallbacks& callbacks) -> bool {
+            callbacks.addStreamFilter(StreamFilterSharedPtr{filter});
+            return true;
+          }));
+
+  // When dispatch is called on the codec, we pretend to get a new stream and then fire a headers
+  // only request into it. Then we respond into the filter.
+  StreamDecoder* decoder = nullptr;
+  EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void {
+    decoder = &conn_manager_->newStream(encoder);
+
+    HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"},
+                                               {":method", "GET"},
+                                               {":path", "/"},
+                                               {"connection", "Upgrade"},
+                                               {"upgrade", "foo"}}};
+    decoder->decodeHeaders(std::move(headers), false);
+
+    HeaderMapPtr response_headers{
+        new TestHeaderMapImpl{{":status", "101"}, {"Connection", "upgrade"}, {"upgrade", "foo"}}};
+    filter->decoder_callbacks_->encodeHeaders(std::move(response_headers), false);
+
+    data.drain(4);
+  }));
+
+  // Kick off the incoming data. Use extra data which should cause a redispatch.
+  Buffer::OwnedImpl fake_input("1234");
+  conn_manager_->onData(fake_input, false);
+}
+
 TEST_F(HttpConnectionManagerImplTest, DrainClose) {
   setup(true, "");