http2: Change accounting of WINDOW_UPDATE frames (envoyproxy#14924)

Signed-off-by: Yan Avlasov <[email protected]>

api/envoy/config/core/v3/protocol.proto

@@ -301,9 +301,12 @@ message Http2ProtocolOptions {
   // of PRIORITY frames received over the lifetime of connection exceeds the value calculated
   // using this formula::
   //
-  //     max_inbound_priority_frames_per_stream * (1 + inbound_streams)
+  //   max_inbound_priority_frames_per_stream * (1 + opened_streams)
   //
-  // the connection is terminated. The ``http2.inbound_priority_frames_flood`` stat tracks
+  // the connection is terminated. For downstream connections the `opened_streams` is incremented when
+  // Envoy receives complete response headers from the upstream server. For upstream connection the
+  // `opened_streams` is incremented when Envoy send the HEADERS frame for a new stream. The
+  // ``http2.inbound_priority_frames_flood`` stat tracks
   // the number of connections terminated due to flood mitigation. The default limit is 100.
   // NOTE: flood and abuse mitigation for upstream connections is presently enabled by the
   // `envoy.reloadable_features.upstream_http2_flood_checks` flag.
@@ -313,11 +316,14 @@ message Http2ProtocolOptions {
   // of WINDOW_UPDATE frames received over the lifetime of connection exceeds the value calculated
   // using this formula::
   //
-  //     1 + 2 * (inbound_streams +
-  //              max_inbound_window_update_frames_per_data_frame_sent * outbound_data_frames)
+  //   5 + 2 * (opened_streams +
+  //            max_inbound_window_update_frames_per_data_frame_sent * outbound_data_frames)
   //
-  // the connection is terminated. The ``http2.inbound_priority_frames_flood`` stat tracks
-  // the number of connections terminated due to flood mitigation. The default limit is 10.
+  // the connection is terminated. For downstream connections the `opened_streams` is incremented when
+  // Envoy receives complete response headers from the upstream server. For upstream connections the
+  // `opened_streams` is incremented when Envoy sends the HEADERS frame for a new stream. The
+  // ``http2.inbound_priority_frames_flood`` stat tracks the number of connections terminated due to
+  // flood mitigation. The default max_inbound_window_update_frames_per_data_frame_sent value is 10.
   // Setting this to 1 should be enough to support HTTP/2 implementations with basic flow control,
   // but more complex implementations that try to estimate available bandwidth require at least 2.
   // NOTE: flood and abuse mitigation for upstream connections is presently enabled by the

docs/root/version_history/current.rst

@@ -33,6 +33,10 @@ Minor Behavior Changes
   instead of `//source/common/chromium_url`. The new path canonicalizer is enabled by default. To
   revert to the legacy path canonicalizer, enable the runtime flag
   `envoy.reloadable_features.remove_forked_chromium_url`.
+* http: increase the maximum allowed number of initial connection WINDOW_UPDATE frames sent by the peer from 1 to 5.
+* http: upstream flood and abuse checks increment the count of opened HTTP/2 streams when Envoy sends
+  initial HEADERS frame for the new stream. Before the counter was incrementred when Envoy received
+  response HEADERS frame with the END_HEADERS flag set from upstream server.
 * oauth filter: added the optional parameter :ref:`auth_scopes <envoy_v3_api_field_extensions.filters.http.oauth2.v3alpha.OAuth2Config.auth_scopes>` with default value of 'user' if not provided. Enables this value to be overridden in the Authorization request to the OAuth provider.
 * perf: allow reading more bytes per operation from raw sockets to improve performance.
 * router: extended custom date formatting to DOWNSTREAM_PEER_CERT_V_START and DOWNSTREAM_PEER_CERT_V_END when using :ref:`custom request/response header formats <config_http_conn_man_headers_custom_request_headers>`.

source/common/http/http2/codec_impl.cc

@@ -1412,6 +1412,7 @@ RequestEncoder& ClientConnectionImpl::newStream(ResponseDecoder& decoder) {
   }
   ClientStreamImpl& stream_ref = *stream;
   LinkedList::moveIntoList(std::move(stream), active_streams_);
+  protocol_constraints_.incrementOpenedStreamCount();
   return stream_ref;
 }
 
@@ -1513,6 +1514,7 @@ Status ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) {
   LinkedList::moveIntoList(std::move(stream), active_streams_);
   nghttp2_session_set_stream_user_data(session_, frame->hd.stream_id,
                                        active_streams_.front().get());
+  protocol_constraints_.incrementOpenedStreamCount();
   return okStatus();
 }
 

source/common/http/http2/protocol_constraints.cc

@@ -62,15 +62,6 @@ Status ProtocolConstraints::trackInboundFrames(const nghttp2_frame_hd* hd,
   switch (hd->type) {
   case NGHTTP2_HEADERS:
   case NGHTTP2_CONTINUATION:
-    // Track new streams.
-
-    // TODO(yanavlasov): The protocol constraint tracker for upstream connections considers the
-    // stream to be in the OPEN state after the server sends complete response headers. The
-    // correctness of this is debatable and needs to be revisited.
-    if (hd->flags & NGHTTP2_FLAG_END_HEADERS) {
-      inbound_streams_++;
-    }
-    FALLTHRU;
   case NGHTTP2_DATA:
     // Track frames with an empty payload and no end stream flag.
     if (hd->length - padding_length == 0 && !(hd->flags & NGHTTP2_FLAG_END_STREAM)) {
@@ -106,13 +97,13 @@ Status ProtocolConstraints::checkInboundFrameLimits() {
   }
 
   if (inbound_priority_frames_ >
-      static_cast<uint64_t>(max_inbound_priority_frames_per_stream_) * (1 + inbound_streams_)) {
+      static_cast<uint64_t>(max_inbound_priority_frames_per_stream_) * (1 + opened_streams_)) {
     stats_.inbound_priority_frames_flood_.inc();
     return bufferFloodError("Too many PRIORITY frames");
   }
 
   if (inbound_window_update_frames_ >
-      1 + 2 * (inbound_streams_ +
+      5 + 2 * (opened_streams_ +
                max_inbound_window_update_frames_per_data_frame_sent_ * outbound_data_frames_)) {
     stats_.inbound_window_update_frames_flood_.inc();
     return bufferFloodError("Too many WINDOW_UPDATE frames");

source/common/http/http2/protocol_constraints.h

@@ -50,6 +50,7 @@ class ProtocolConstraints {
   Status trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length);
   // Increment the number of DATA frames sent to the peer.
   void incrementOutboundDataFrameCount() { ++outbound_data_frames_; }
+  void incrementOpenedStreamCount() { ++opened_streams_; }
 
   Status checkOutboundFrameLimits();
 
@@ -87,8 +88,12 @@ class ProtocolConstraints {
   // a payload. Initialized from corresponding http2_protocol_options. Default value is 1.
   const uint32_t max_consecutive_inbound_frames_with_empty_payload_;
 
-  // This counter keeps track of the number of inbound streams.
-  uint32_t inbound_streams_ = 0;
+  // This counter keeps track of the number of opened streams.
+  // For downstream connection this is incremented when the first HEADERS frame with the new
+  // stream ID is received from the client.
+  // For upstream connections this is incremented when the first HEADERS frame with the new
+  // stream ID is sent to the upstream server.
+  uint32_t opened_streams_ = 0;
   // This counter keeps track of the number of inbound PRIORITY frames. If this counter exceeds
   // the value calculated using this formula:
   //

test/common/http/http2/codec_impl_test.cc

@@ -302,11 +302,11 @@ class Http2CodecImplTest : public ::testing::TestWithParam<Http2SettingsTestPara
     EXPECT_NO_THROW(response_encoder_->encodeData(data, false));
 
     // See the limit formula in the
-    // `Envoy::Http::Http2::ServerConnectionImpl::checkInboundFrameLimits()' method.
+    // `Envoy::Http::Http2::ProtocolConstraints::checkInboundFrameLimits()' method.
     constexpr uint32_t max_allowed =
-        1 + 2 * (CommonUtility::OptionsLimits::
-                     DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT +
-                 1);
+        5 + 2 * (1 + CommonUtility::OptionsLimits::
+                             DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT *
+                         1);
     for (uint32_t i = 0; i < max_allowed + 1; ++i) {
       EXPECT_EQ(0, nghttp2_submit_window_update(client_->session(), NGHTTP2_FLAG_NONE, 1, 1));
     }

test/common/http/http2/protocol_constraints_test.cc

@@ -156,12 +156,9 @@ TEST_F(ProtocolConstraintsTest, Priority) {
   options_.mutable_max_inbound_priority_frames_per_stream()->set_value(2);
   ProtocolConstraints constraints(http2CodecStats(), options_);
   // Create one stream
-  nghttp2_frame_hd frame;
-  frame.type = NGHTTP2_HEADERS;
-  frame.length = 1;
-  frame.flags = NGHTTP2_FLAG_END_HEADERS;
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
+  constraints.incrementOpenedStreamCount();
 
+  nghttp2_frame_hd frame;
   frame.type = NGHTTP2_PRIORITY;
   EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
   EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
@@ -174,26 +171,23 @@ TEST_F(ProtocolConstraintsTest, Priority) {
 }
 
 TEST_F(ProtocolConstraintsTest, WindowUpdate) {
-  options_.mutable_max_inbound_window_update_frames_per_data_frame_sent()->set_value(1);
+  options_.mutable_max_inbound_window_update_frames_per_data_frame_sent()->set_value(2);
   ProtocolConstraints constraints(http2CodecStats(), options_);
   // Create one stream
-  nghttp2_frame_hd frame;
-  frame.type = NGHTTP2_HEADERS;
-  frame.length = 1;
-  frame.flags = NGHTTP2_FLAG_END_HEADERS;
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
+  constraints.incrementOpenedStreamCount();
   // Send 2 DATA frames
   constraints.incrementOutboundDataFrameCount();
   constraints.incrementOutboundDataFrameCount();
 
+  // Per the `5 + 2 * (opened_streams_ +
+  // max_inbound_window_update_frames_per_data_frame_sent_ * outbound_data_frames_`
+  // formula 5 + 2 * (1 + 2 * 2) = 15 WINDOW_UPDATE frames should NOT fail constraint
+  // check, but 16th should.
+  nghttp2_frame_hd frame;
   frame.type = NGHTTP2_WINDOW_UPDATE;
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
-  EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
+  for (uint32_t i = 0; i < 15; ++i) {
+    EXPECT_TRUE(constraints.trackInboundFrames(&frame, 0).ok());
+  }
   EXPECT_TRUE(isBufferFloodError(constraints.trackInboundFrames(&frame, 0)));
   EXPECT_TRUE(isBufferFloodError(constraints.status()));
   EXPECT_EQ("Too many WINDOW_UPDATE frames", constraints.status().message());

test/integration/http2_flood_integration_test.cc

@@ -1117,10 +1117,14 @@ TEST_P(Http2FloodMitigationTest, WindowUpdate) {
   const auto request = Http2Frame::makeRequest(request_stream_id, "host", "/");
   sendFrame(request);
 
-  // Since we do not send any DATA frames, only 4 sequential WINDOW_UPDATE frames should
-  // trigger flood protection.
+  // See the limit formula in the
+  // `Envoy::Http::Http2::ProtocolConstraints::checkInboundFrameLimits()' method.
+  constexpr uint32_t max_allowed =
+      5 + 2 * (1 + Http2::Utility::OptionsLimits::
+                           DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT *
+                       0 /* 0 DATA frames */);
   floodServer(Http2Frame::makeWindowUpdateFrame(request_stream_id, 1),
-              "http2.inbound_window_update_frames_flood", 4);
+              "http2.inbound_window_update_frames_flood", max_allowed + 1);
 }
 
 // Verify that the HTTP/2 connection is terminated upon receiving invalid HEADERS frame.
@@ -1206,7 +1210,11 @@ TEST_P(Http2FloodMitigationTest, UpstreamWindowUpdate) {
     return;
   }
 
-  floodClient(Http2Frame::makeWindowUpdateFrame(0, 1), 4,
+  constexpr uint32_t max_allowed =
+      5 + 2 * (1 + Http2::Utility::OptionsLimits::
+                           DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT *
+                       0 /* 0 DATA frames */);
+  floodClient(Http2Frame::makeWindowUpdateFrame(0, 1), max_allowed + 1,
               "cluster.cluster_0.http2.inbound_window_update_frames_flood");
 }
 
@@ -1381,16 +1389,9 @@ TEST_P(Http2FloodMitigationTest, UpstreamPriorityNoOpenStreams) {
     return;
   }
 
-  // TODO(yanavlasov): The protocol constraint tracker for upstream connections considers the stream
-  // to be in the OPEN state after the server sends complete response headers. The correctness of
-  // this is debatable and needs to be revisited.
-
-  // The `floodClient` method sends request headers to open upstream connection, but upstream does
-  // not send any response. In this case the number of streams tracked by the upstream protocol
-  // constraints checker is still 0.
   floodClient(Http2Frame::makePriorityFrame(Http2Frame::makeClientStreamId(1),
                                             Http2Frame::makeClientStreamId(2)),
-              Http2::Utility::OptionsLimits::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM + 1,
+              Http2::Utility::OptionsLimits::DEFAULT_MAX_INBOUND_PRIORITY_FRAMES_PER_STREAM * 2 + 1,
               "cluster.cluster_0.http2.inbound_priority_frames_flood");
 }