http2: Proactively disconnect connections flooded when encoding headers

source/common/http/http2/codec_impl.cc

@@ -176,6 +176,7 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector<nghttp2_nv>
   // process termination from unhandled exception with the RELEASE_ASSERT.
   // Further work will replace this RELEASE_ASSERT with proper error handling.
   RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
+  parent_.checkProtocolConstraintViolation();
 }
 
 void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
@@ -488,7 +489,7 @@ void ConnectionImpl::StreamImpl::encodeDataHelper(Buffer::Instance& data, bool e
   auto status = parent_.sendPendingFrames();
   // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
   RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
-  parent_.checkProtocolConstrainViolation();
+  parent_.checkProtocolConstraintViolation();
 
   if (local_end_stream_ && pending_send_data_.length() > 0) {
     createPendingFlushTimer();
@@ -1506,7 +1507,7 @@ ServerConnectionImpl::trackOutboundFrames(bool is_outbound_flood_monitored_contr
   return releasor;
 }
 
-void ServerConnectionImpl::checkProtocolConstrainViolation() {
+void ServerConnectionImpl::checkProtocolConstraintViolation() {
   if (!protocol_constraints_.checkOutboundFrameLimits().ok()) {
     scheduleProtocolConstraintViolationCallback();
   }

source/common/http/http2/codec_impl.h

@@ -462,7 +462,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
    * implementation in the ServerConnectionImpl schedules callback to terminate connection if the
    * protocol constraint was violated.
    */
-  virtual void checkProtocolConstrainViolation() PURE;
+  virtual void checkProtocolConstraintViolation() PURE;
 
   /**
    * Callback for terminating connection when protocol constrain has been violated
@@ -578,7 +578,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
     return ProtocolConstraints::ReleasorProc([]() {});
   }
   Status trackInboundFrames(const nghttp2_frame_hd*, uint32_t) override { return okStatus(); }
-  void checkProtocolConstrainViolation() override {}
+  void checkProtocolConstraintViolation() override {}
 
   Http::ConnectionCallbacks& callbacks_;
 };
@@ -610,7 +610,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
    * Check protocol constraint violations outside of the dispatching context.
    * This method ASSERTs if it is called in the dispatching context.
    */
-  void checkProtocolConstrainViolation() override;
+  void checkProtocolConstraintViolation() override;
 
   // Http::Connection
   // The reason for overriding the dispatch method is to do flood mitigation only when

source/common/http/http2/codec_impl_legacy.cc

@@ -169,6 +169,7 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector<nghttp2_nv>
   local_end_stream_ = end_stream;
   submitHeaders(final_headers, end_stream ? nullptr : &provider);
   parent_.sendPendingFrames();
+  parent_.checkProtocolConstraintViolation();
 }
 
 void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers,
@@ -470,7 +471,7 @@ void ConnectionImpl::StreamImpl::encodeDataHelper(Buffer::Instance& data, bool e
   }
 
   parent_.sendPendingFrames();
-  parent_.checkProtocolConstrainViolation();
+  parent_.checkProtocolConstraintViolation();
 
   if (local_end_stream_ && pending_send_data_.length() > 0) {
     createPendingFlushTimer();
@@ -1457,7 +1458,7 @@ ServerConnectionImpl::trackOutboundFrames(bool is_outbound_flood_monitored_contr
   return releasor;
 }
 
-void ServerConnectionImpl::checkProtocolConstrainViolation() {
+void ServerConnectionImpl::checkProtocolConstraintViolation() {
   if (!protocol_constraints_.checkOutboundFrameLimits().ok()) {
     scheduleProtocolConstraintViolationCallback();
   }

source/common/http/http2/codec_impl_legacy.h

@@ -445,7 +445,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
    * The implementation in the ServerConnectionImpl schedules callback to terminate connection if
    * the protocol constraint was violated.
    */
-  virtual void checkProtocolConstrainViolation() PURE;
+  virtual void checkProtocolConstraintViolation() PURE;
 
   /**
    * Callback for terminating connection when protocol constrain has been violated
@@ -556,7 +556,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
     return Envoy::Http::Http2::ProtocolConstraints::ReleasorProc([]() {});
   }
   bool trackInboundFrames(const nghttp2_frame_hd*, uint32_t) override { return true; }
-  void checkProtocolConstrainViolation() override {}
+  void checkProtocolConstraintViolation() override {}
 
   Http::ConnectionCallbacks& callbacks_;
 };
@@ -588,7 +588,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
    * Check protocol constraint violations outside of the dispatching context.
    * This method ASSERTs if it is called in the dispatching context.
    */
-  void checkProtocolConstrainViolation() override;
+  void checkProtocolConstraintViolation() override;
 
   // Http::Connection
   // The reason for overriding the dispatch method is to do flood mitigation only when

test/common/http/http2/codec_impl_test.cc

@@ -2041,15 +2041,16 @@ TEST_P(Http2CodecImplTest, ResponseHeadersFlood) {
         buffer.move(frame);
       }));
 
+  auto* violation_callback =
+      new NiceMock<Event::MockSchedulableCallback>(&server_connection_.dispatcher_);
   TestResponseHeaderMapImpl response_headers{{":status", "200"}};
   for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1; ++i) {
     EXPECT_NO_THROW(response_encoder_->encodeHeaders(response_headers, false));
   }
-  // Presently flood mitigation is done only when processing downstream data
-  // So we need to send stream from downstream client to trigger mitigation
-  EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr));
-  EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError,
-                            "Too many frames in the outbound queue.");
+
+  EXPECT_TRUE(violation_callback->enabled_);
+  EXPECT_CALL(server_connection_, close(Envoy::Network::ConnectionCloseType::NoFlush));
+  violation_callback->invokeCallback();
 
   EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1);
   EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value());

test/integration/autonomous_upstream.cc

@@ -21,6 +21,7 @@ const char AutonomousStream::RESPONSE_SIZE_BYTES[] = "response_size_bytes";
 const char AutonomousStream::RESPONSE_DATA_BLOCKS[] = "response_data_blocks";
 const char AutonomousStream::EXPECT_REQUEST_SIZE_BYTES[] = "expect_request_size_bytes";
 const char AutonomousStream::RESET_AFTER_REQUEST[] = "reset_after_request";
+const char AutonomousStream::NO_TRAILERS[] = "no_trailers";
 
 AutonomousStream::AutonomousStream(FakeHttpConnection& parent, Http::ResponseEncoder& encoder,
                                    AutonomousUpstream& upstream, bool allow_incomplete_streams)
@@ -63,11 +64,17 @@ void AutonomousStream::sendResponse() {
   int32_t response_data_blocks = 1;
   HeaderToInt(RESPONSE_DATA_BLOCKS, response_data_blocks, headers);
 
-  encodeHeaders(upstream_.responseHeaders(), false);
-  for (int32_t i = 0; i < response_data_blocks; ++i) {
-    encodeData(response_body_length, false);
+  const bool send_trailers = headers.get_(NO_TRAILERS).empty();
+  const bool headers_only_response = !send_trailers && response_data_blocks == 0;
+  encodeHeaders(upstream_.responseHeaders(), headers_only_response);
+  if (!headers_only_response) {
+    for (int32_t i = 0; i < response_data_blocks; ++i) {
+      encodeData(response_body_length, i == (response_data_blocks - 1) && !send_trailers);
+    }
+    if (send_trailers) {
+      encodeTrailers(upstream_.responseTrailers());
+    }
   }
-  encodeTrailers(upstream_.responseTrailers());
 }
 
 AutonomousHttpConnection::AutonomousHttpConnection(AutonomousUpstream& autonomous_upstream,

test/integration/autonomous_upstream.h

@@ -21,6 +21,8 @@ class AutonomousStream : public FakeStream {
   // If set, the stream will reset when the request is complete, rather than
   // sending a response.
   static const char RESET_AFTER_REQUEST[];
+  // Prevents upstream from sending trailers.
+  static const char NO_TRAILERS[];
 
   AutonomousStream(FakeHttpConnection& parent, Http::ResponseEncoder& encoder,
                    AutonomousUpstream& upstream, bool allow_incomplete_streams);

test/integration/http2_integration_test.cc

@@ -1725,6 +1725,34 @@ void Http2FloodMitigationTest::floodServer(absl::string_view host, absl::string_
             test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value());
 }
 
+void Http2FloodMitigationTest::prefillOutboundDownstreamQueue(uint32_t data_frame_count) {
+  // Set large buffer limits so the test is not affected by the flow control.
+  config_helper_.setBufferLimits(1024 * 1024 * 1024, 1024 * 1024 * 1024);
+  autonomous_upstream_ = true;
+  autonomous_allow_incomplete_streams_ = true;
+  beginSession();
+
+  // Do not read from the socket and send request that causes autonomous upstream to respond
+  // with the specified number of DATA frames. This pre-fills downstream outbound frame queue
+  // such the the next response triggers flood protection.
+  // Simulate TCP push back on the Envoy's downstream network socket, so that outbound frames
+  // start to accumulate in the transport socket buffer.
+  writev_matcher_->setWritevReturnsEgain();
+
+  const auto request = Http2Frame::makeRequest(
+      Http2Frame::makeClientStreamId(0), "host", "/test/long/url",
+      {Http2Frame::Header("response_data_blocks", absl::StrCat(data_frame_count)),
+       Http2Frame::Header("no_trailers", "0")});
+  sendFrame(request);
+
+  // Wait for some data to arrive and then wait for the upstream_rq_active to flip to 0 to indicate
+  // that the first request has completed.
+  test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_rx_bytes_total", 10000);
+  test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_active", 0);
+  // Verify that pre-fill did not trigger flood protection
+  EXPECT_EQ(0, test_server_->counter("http2.outbound_flood")->value());
+}
+
 INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FloodMitigationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);
@@ -1773,7 +1801,8 @@ TEST_P(Http2FloodMitigationTest, Data) {
   writev_matcher_->setWritevReturnsEgain();
 
   const auto request = Http2Frame::makeRequest(
-      1, "host", "/test/long/url", {Http2Frame::Header("response_data_blocks", "1000")});
+      1, "host", "/test/long/url",
+      {Http2Frame::Header("response_data_blocks", "1000"), Http2Frame::Header("no_trailers", "0")});
   sendFrame(request);
 
   // Wait for connection to be flooded with outbound DATA frames and disconnected.
@@ -1793,8 +1822,6 @@ TEST_P(Http2FloodMitigationTest, Data) {
 // This test also verifies that RELEASE_ASSERT in the ConnectionImpl::StreamImpl::encodeDataHelper()
 // is not fired when it is called by the sendLocalReply() in the dispatching context.
 TEST_P(Http2FloodMitigationTest, DataOverflowFromDecoderFilterSendLocalReply) {
-  // Set large buffer limits so the test is not affected by the flow control.
-  config_helper_.setBufferLimits(1024 * 1024 * 1024, 1024 * 1024 * 1024);
   config_helper_.addConfigModifier(
       [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
               hcm) -> void {
@@ -1812,27 +1839,8 @@ name: send_local_reply_filter
         hcm.mutable_http_filters()->SwapElements(size - 2, size - 1);
       });
 
-  autonomous_upstream_ = true;
-  autonomous_allow_incomplete_streams_ = true;
-  beginSession();
-
-  // Do not read from the socket and send request that causes autonomous upstream
-  // to respond with 1000 DATA frames. The Http2FloodMitigationTest::beginSession()
-  // sets 1000 flood limit for all frame types. Including 1 HEADERS response frame
-  // 997 DATA frames should make just 2 frames under the flood limit.
-  // Simulate TCP push back on the Envoy's downstream network socket, so that outbound frames start
-  // to accumulate in the transport socket buffer.
-  writev_matcher_->setWritevReturnsEgain();
-
-  auto request =
-      Http2Frame::makeRequest(Http2Frame::makeClientStreamId(0), "host", "/test/long/url",
-                              {Http2Frame::Header("response_data_blocks", "997")});
-  sendFrame(request);
-
-  // Wait for some data to arrive and then wait for the upstream_rq_active to flip to 0 to indicate
-  // that the first request has completed.
-  test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_rx_bytes_total", 1000);
-  test_server_->waitForGaugeEq("cluster.cluster_0.upstream_rq_active", 0);
+  // pre-fill 2 away from overflow
+  prefillOutboundDownstreamQueue(AllFrameFloodLimit - 2);
 
   // At this point the outbound downstream frame queue should be 2 away from overflowing.
   // Make the SetResponseCodeFilterConfig decoder filter call sendLocalReply with body.
@@ -1852,6 +1860,100 @@ name: send_local_reply_filter
   EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value());
 }
 
+// Verify that the server can detect flood of response HEADERS frames
+TEST_P(Http2FloodMitigationTest, Headers) {
+  // pre-fill one away from overflow
+  prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1);
+
+  // Send second request which should trigger headers only response.
+  // Verify that connection was disconnected and appropriate counters were set.
+  auto request2 = Http2Frame::makeRequest(
+      Http2Frame::makeClientStreamId(1), "host", "/test/long/url",
+      {Http2Frame::Header("response_data_blocks", "0"), Http2Frame::Header("no_trailers", "0")});
+  sendFrame(request2);
+
+  // Wait for connection to be flooded with outbound HEADERS frame and disconnected.
+  tcp_client_->waitForDisconnect();
+
+  // If the server codec had incorrectly thrown an exception on flood detection it would cause
+  // the entire upstream to be disconnected. Verify it is still active, and there are no destroyed
+  // connections.
+  ASSERT_EQ(1, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value());
+  ASSERT_EQ(0, test_server_->counter("cluster.cluster_0.upstream_cx_destroy")->value());
+  // Verify that the flood check was triggered
+  EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value());
+}
+
+// Verify that the server can detect overflow by 100 continue response sent by Envoy itself
+TEST_P(Http2FloodMitigationTest, Envoy100ContinueHeaders) {
+  // pre-fill one away from overflow
+  prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1);
+
+  // Send second request which should trigger Envoy to respond with 100 continue.
+  // Verify that connection was disconnected and appropriate counters were set.
+  auto request2 = Http2Frame::makeRequest(
+      Http2Frame::makeClientStreamId(1), "host", "/test/long/url",
+      {Http2Frame::Header("response_data_blocks", "0"), Http2Frame::Header("no_trailers", "0"),
+       Http2Frame::Header("expect", "100-continue")});
+  sendFrame(request2);
+
+  // Wait for connection to be flooded with outbound HEADERS frame and disconnected.
+  tcp_client_->waitForDisconnect();
+
+  // If the server codec had incorrectly thrown an exception on flood detection it would cause
+  // the entire upstream to be disconnected. Verify it is still active, and there are no destroyed
+  // connections.
+  ASSERT_EQ(1, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value());
+  ASSERT_EQ(0, test_server_->counter("cluster.cluster_0.upstream_cx_destroy")->value());
+  // The second upstream request should be reset since it is disconnected when sending 100 continue
+  // response
+  EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_rq_tx_reset")->value());
+  // Verify that the flood check was triggered
+  EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value());
+}
+
+// Verify that the server can detect flood triggered by a HEADERS frame from a decoder filter call
+// to sendLocalReply().
+// This test also verifies that RELEASE_ASSERT in the
+// ConnectionImpl::StreamImpl::encodeHeadersBase() is not fired when it is called by the
+// sendLocalReply() in the dispatching context.
+TEST_P(Http2FloodMitigationTest, HeadersOverflowFromDecoderFilterSendLocalReply) {
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) -> void {
+        const std::string yaml_string = R"EOF(
+name: send_local_reply_filter
+typed_config:
+  "@type": type.googleapis.com/test.integration.filters.SetResponseCodeFilterConfig
+  prefix: "/call_send_local_reply"
+  code: 404
+  )EOF";
+        TestUtility::loadFromYaml(yaml_string, *hcm.add_http_filters());
+        // keep router the last
+        auto size = hcm.http_filters_size();
+        hcm.mutable_http_filters()->SwapElements(size - 2, size - 1);
+      });
+
+  // pre-fill one away from overflow
+  prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1);
+
+  // At this point the outbound downstream frame queue should be 1 away from overflowing.
+  // Make the SetResponseCodeFilterConfig decoder filter call sendLocalReply without body.
+  // Verify that connection was disconnected and appropriate counters were set.
+  auto request2 =
+      Http2Frame::makeRequest(Http2Frame::makeClientStreamId(1), "host", "/call_send_local_reply");
+  sendFrame(request2);
+
+  // Wait for connection to be flooded with outbound HEADERS frame and disconnected.
+  tcp_client_->waitForDisconnect();
+
+  // Verify that the upstream connection is still alive.
+  ASSERT_EQ(1, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value());
+  ASSERT_EQ(0, test_server_->counter("cluster.cluster_0.upstream_cx_destroy")->value());
+  // Verify that the flood check was triggered
+  EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value());
+}
+
 // TODO(yanavlasov): add the same tests as above for the encoder filters.
 // This is currently blocked by the https://github.com/envoyproxy/envoy/pull/13256
 

test/integration/http2_integration_test.h

@@ -138,5 +138,6 @@ class Http2FloodMitigationTest : public SocketInterfaceSwap, public Http2FrameIn
 
   void setNetworkConnectionBufferSize();
   void beginSession() override;
+  void prefillOutboundDownstreamQueue(uint32_t data_frame_count);
 };
 } // namespace Envoy