http2: Change accounting of WINDOW_UPDATE frames by yanavlasov · Pull Request #14924 · envoyproxy/envoy

yanavlasov · 2021-02-03T04:30:31Z

Commit Message:
Allow more WINDOW_UPDATE frames to be received for connections with no open streams or for streams without any DATA frames. This is to make Envoy work with upstream H/2 servers that send multiple connection WINDOW_UPDATE frames before any streams were opened, or for new streams before any DATA was sent.

Additionally fix the issue where upstream flood checker was incrementing opened stream counter when receiving last HEADERS frame from the upstream server. It now correctly incremented when Envoy opens a new stream to the upstream server.

Risk Level: Low (WINDOW_UPDATE flood budged is slightly increased)
Testing: Unit Tests
Docs Changes: proto doc
Release Notes: Yes
Platform Specific Features: N/A

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

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

repokitteh-read-only · 2021-02-03T04:30:37Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14924 was opened by yanavlasov.

see: more, trace.

antoniovicente

Just some naming nits in comments. Thanks for fixing this.

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

test/common/http/http2/protocol_constraints_test.cc

test/integration/http2_flood_integration_test.cc

PiotrSikora · 2021-02-03T05:41:58Z

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

  //
-  //     1 + 2 * (inbound_streams +
-  //              max_inbound_window_update_frames_per_data_frame_sent * outbound_data_frames)
+  //     10 * (1 + inbound_streams) +


Where is 10 coming from? It's a big multiplier, without any justification stated here.

Consider that the default value of max_inbound_window_update_frames_per_data_frame_sent is 10 and it comes with a bonus factor of 2.

I think that one possible flaw in this algorithm is that once you have sent a few outbound data frames, the remote endpoint is able to send you a lot of window updates without triggering the limit.

I've changed the formula once again. Basically it adds 1 DATA frame for each new stream. This will accommodate the case of server preemptively sending WINDOW_UPDATE.
Also dropped 2 multiplier. I do not think it makes sense in the new formula.

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

antoniovicente · 2021-02-03T06:25:18Z

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

  //
-  //     1 + 2 * (inbound_streams +
-  //              max_inbound_window_update_frames_per_data_frame_sent * outbound_data_frames)
+  //     10 * (1 + inbound_streams) +


Consider that the default value of max_inbound_window_update_frames_per_data_frame_sent is 10 and it comes with a bonus factor of 2.

I think that one possible flaw in this algorithm is that once you have sent a few outbound data frames, the remote endpoint is able to send you a lot of window updates without triggering the limit.

antoniovicente · 2021-02-03T17:23:14Z

source/common/http/http2/protocol_constraints.cc

-      1 + 2 * (inbound_streams_ +
-               max_inbound_window_update_frames_per_data_frame_sent_ * outbound_data_frames_)) {
+      (10 * (1 + opened_streams_) +
+       (2 * max_inbound_window_update_frames_per_data_frame_sent_ * outbound_data_frames_))) {


Could you change this to:

(2 * max_inbound_window_update_frames_per_data_frame_sent_ * (outbound_data_frames_ + 1))) {

An issue I see in the current formula is that setting max_inbound_window_update_frames_per_data_frame_sent_ to uint32 max does not effectively disable this flood check. It would be good to have a mechanism to mitigate any future issue in this logic by effectively disabling the check.

Ok, I think this will work too.

I've massaged it a bit more to accommodate cases where DATA frames have not yet been sent.

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

lizan · 2021-02-05T19:54:30Z

/lgtm api

antoniovicente

Thanks for the fix.

PiotrSikora · 2021-02-09T17:43:49Z

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

  //
-  //     1 + 2 * (inbound_streams +
-  //              max_inbound_window_update_frames_per_data_frame_sent * outbound_data_frames)
+  //   max_inbound_window_update_frames_per_data_frame_sent * (outbound_data_frames + opened_streams + 1)


I believe that the 2x multiplier was added to account for the same WINDOW_UPDATE increment to be received on a connection (stream == 0) and an individual stream (stream != 0) levels. The new formula doesn't account for that.

I believe the old formula is:

1 - allow initial WINDOW_UPDATE frame on the connection level,

2 * inbound_streams - allow initial WINDOW_UPDATE frame for each inbound stream (2x for connection and stream levels),

2 * max_inbound_window_update_frames_per_data_frame_sent * outbound_data_frames - tunable multiplier of allowed WINDOW_UPDATE frames received in response to each sent DATA frame (2x for connection and stream levels).

The new formula seems to apply the old tunable to non-DATA frames as well. It might very well work, since the limit is going to be higher than the existing one, but the reasoning behind the formula is much less clear.

The reasoning is to make tunable allowed number of WINDOW_UPDATE frames before any DATA was sent. This makes cases like facebook's or cases where server may preemptively open stream window knowing that it has large body work better.
I think agree that 2x multiplier needs to be put back.
Note that in the more restrictive case where max_inbound_window_update_frames_per_data_frame_sent == 1 the new formula (with 2x multiplier put back) does not increase the limit compared to the old formula.

The reasoning is to make tunable allowed number of WINDOW_UPDATE frames before any DATA was sent. This makes cases like facebook's or cases where server may preemptively open stream window knowing that it has large body work better.

My point is that if there is a specific scenario that we want to address (e.g. upstream servers sending 2 WINDOW_UPDATE frames for each new stream), then it should be part of the formula, and we could either hardcode the value we deem reasonable (e.g. 2 * inbound_streams) or introduce a new tunable. This PR should improve the formula, and not simply increase the limit.

Also, using max_inbound_window_update_frames_per_data_frame_sent to control number of WINDOW_UPDATE frames allowed before receiving any DATA frames seems quite confusing.

I'm going to just add a fixed constant to solve the issue we have in prod. We can discuss the formula later and do the deprecation dance if we need to.

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

test/integration/http2_flood_integration_test.cc

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

PiotrSikora

Thanks!

antoniovicente · 2021-02-10T18:13:26Z

docs/root/version_history/current.rst

  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


nit: Long line with line-break. Either merge the next line into this one, or apply line breaks around 100 columns instead of 185.

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

antoniovicente · 2021-02-10T18:23:46Z

@zuercher or @envoyproxy/senior-maintainers: Could you do a review pass?

zuercher

Looks good to me.

asraa · 2021-02-11T19:34:50Z

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

  //
-  // 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


@yanavlasov Is this comment right? I'm having a hard time seeing where that's happening. It looks to me like it's happening when receiving the first header frame.

Change accounting of WINDOW_UPDATE frames

ef61544

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

repokitteh-read-only bot added the api label Feb 3, 2021

repokitteh-read-only bot assigned lizan Feb 3, 2021

yanavlasov assigned PiotrSikora, antoniovicente and alyssawilk and unassigned lizan Feb 3, 2021

antoniovicente previously approved these changes Feb 3, 2021

View reviewed changes

api/envoy/config/core/v3/protocol.proto Outdated Show resolved Hide resolved

test/common/http/http2/protocol_constraints_test.cc Outdated Show resolved Hide resolved

test/integration/http2_flood_integration_test.cc Outdated Show resolved Hide resolved

PiotrSikora reviewed Feb 3, 2021

View reviewed changes

antoniovicente reviewed Feb 3, 2021

View reviewed changes

Merge branch 'main' into window-update-flood

62b742b

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

antoniovicente added the waiting label Feb 5, 2021

repokitteh-read-only bot removed the waiting label Feb 5, 2021

Address comments

2af9965

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

yanavlasov dismissed antoniovicente’s stale review via 2af9965 February 5, 2021 17:07

Missing file

3f84ee0

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

repokitteh-read-only bot removed the api label Feb 5, 2021

antoniovicente previously approved these changes Feb 6, 2021

View reviewed changes

PiotrSikora reviewed Feb 9, 2021

View reviewed changes

yanavlasov added 2 commits February 9, 2021 21:02

Merge branch 'main' into window-update-flood

9f006da

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

Put back 2x multiplier

8d13e0c

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

yanavlasov dismissed antoniovicente’s stale review via 8d13e0c February 10, 2021 02:38

repokitteh-read-only bot added the api label Feb 10, 2021

yanavlasov added 2 commits February 9, 2021 23:10

Address comments

9dc6aa6

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

Fix typo

c90eaef

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

PiotrSikora previously approved these changes Feb 10, 2021

View reviewed changes

test/integration/http2_flood_integration_test.cc Outdated Show resolved Hide resolved

Make math more explicit

63beb05

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

yanavlasov dismissed PiotrSikora’s stale review via 63beb05 February 10, 2021 16:49

PiotrSikora previously approved these changes Feb 10, 2021

View reviewed changes

antoniovicente previously approved these changes Feb 10, 2021

View reviewed changes

Fix spelling

c0298e3

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

yanavlasov dismissed stale reviews from antoniovicente and PiotrSikora via c0298e3 February 10, 2021 18:18

Address comments

96e3949

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

antoniovicente approved these changes Feb 10, 2021

View reviewed changes

antoniovicente assigned zuercher Feb 10, 2021

zuercher approved these changes Feb 11, 2021

View reviewed changes

PiotrSikora approved these changes Feb 11, 2021

View reviewed changes

yanavlasov merged commit 5bdcdd6 into envoyproxy:main Feb 11, 2021

asraa reviewed Feb 11, 2021

View reviewed changes

KBaichoo mentioned this pull request Feb 12, 2021

HTTP: Associate Response crashes with the corresponding Request. #15029

Merged

yanavlasov deleted the window-update-flood branch December 16, 2022 17:30

Conversation

yanavlasov commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 3, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Feb 5, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente commented Feb 10, 2021

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yanavlasov commented Feb 3, 2021 •

edited

Loading