Proactively disconnect connections flooded by keepalive PING frames (#…

…13495)

Signed-off-by: Yan Avlasov <yavlasov@google.com>

source/common/http/http2/codec_impl.cc

@@ -604,6 +604,7 @@ void ConnectionImpl::sendKeepalive() {
   auto status = sendPendingFrames();
   // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT.
   RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context");
+  checkProtocolConstraintViolation();
 
   keepalive_timeout_timer_->enableTimer(keepalive_timeout_);
 }

source/common/http/http2/codec_impl_legacy.cc

@@ -580,6 +580,7 @@ void ConnectionImpl::sendKeepalive() {
   int rc = nghttp2_submit_ping(session_, 0 /*flags*/, reinterpret_cast<uint8_t*>(&ms_since_epoch));
   ASSERT(rc == 0);
   sendPendingFrames();
+  checkProtocolConstraintViolation();
 
   keepalive_timeout_timer_->enableTimer(keepalive_timeout_);
 }

test/common/http/http2/codec_impl_test.cc

@@ -2547,6 +2547,62 @@ TEST_P(Http2CodecImplTest, ShudowNoticeCausesOutboundFlood) {
   EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value());
 }
 
+// Verify that codec detects flood of outbound PING frames caused by the keep alive timer
+TEST_P(Http2CodecImplTest, KeepAliveCausesOutboundFlood) {
+  // set-up server to send PING frames
+  constexpr uint32_t interval_ms = 100;
+  constexpr uint32_t timeout_ms = 200;
+  server_http2_options_.mutable_connection_keepalive()->mutable_interval()->set_nanos(interval_ms *
+                                                                                      1000 * 1000);
+  server_http2_options_.mutable_connection_keepalive()->mutable_timeout()->set_nanos(timeout_ms *
+                                                                                     1000 * 1000);
+  server_http2_options_.mutable_connection_keepalive()->mutable_interval_jitter()->set_value(0);
+  auto timeout_timer = new Event::MockTimer(&server_connection_.dispatcher_); /* */
+  auto send_timer = new Event::MockTimer(&server_connection_.dispatcher_);
+  EXPECT_CALL(*timeout_timer, disableTimer());
+  EXPECT_CALL(*send_timer, enableTimer(std::chrono::milliseconds(interval_ms), _));
+
+  initialize();
+
+  TestRequestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, false));
+  request_encoder_->encodeHeaders(request_headers, false);
+
+  int frame_count = 0;
+  Buffer::OwnedImpl buffer;
+  ON_CALL(server_connection_, write(_, _))
+      .WillByDefault(Invoke([&buffer, &frame_count](Buffer::Instance& frame, bool) {
+        ++frame_count;
+        buffer.move(frame);
+      }));
+
+  auto* violation_callback =
+      new NiceMock<Event::MockSchedulableCallback>(&server_connection_.dispatcher_);
+
+  TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+  response_encoder_->encodeHeaders(response_headers, false);
+  // Pre-fill outbound frame queue 1 away from overflow (account for the single HEADERS frame above)
+  for (uint32_t i = 0; i < CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES - 1; ++i) {
+    Buffer::OwnedImpl data("0");
+    EXPECT_NO_THROW(response_encoder_->encodeData(data, false));
+  }
+
+  EXPECT_FALSE(violation_callback->enabled_);
+
+  // Trigger sending a PING, which should overflow the outbound frame queue and cause
+  // client to be disconnected
+  EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(timeout_ms), _));
+  send_timer->callback_();
+
+  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());
+}
+
 // CONNECT without upgrade type gets tagged with "bytestream"
 TEST_P(Http2CodecImplTest, ConnectTest) {
   client_http2_options_.set_allow_connect(true);

test/integration/http2_integration_test.cc

@@ -2295,6 +2295,24 @@ name: send_local_reply_filter
   EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_cx_drain_close")->value());
 }
 
+// Verify detection of overflowing outbound frame queue with the PING frames sent by the keep alive
+// timer. The test verifies protocol constraint violation handling in the
+// Http2::ConnectionImpl::sendKeepalive() method.
+TEST_P(Http2FloodMitigationTest, KeepAliveTimeeTriggersFloodProtection) {
+  config_helper_.addConfigModifier(
+      [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+             hcm) {
+        auto* keep_alive = hcm.mutable_http2_protocol_options()->mutable_connection_keepalive();
+        keep_alive->mutable_interval()->set_nanos(500 * 1000 * 1000);
+        keep_alive->mutable_timeout()->set_seconds(1);
+      });
+
+  prefillOutboundDownstreamQueue(AllFrameFloodLimit - 1);
+  tcp_client_->waitForDisconnect();
+
+  EXPECT_EQ(1, test_server_->counter("http2.outbound_flood")->value());
+}
+
 // Verify that the server stop reading downstream connection on protocol error.
 TEST_P(Http2FloodMitigationTest, TooManyStreams) {
   config_helper_.addConfigModifier(