http2: send graceful goaways on close #41284

kanurag94 · 2025-09-30T10:28:09Z

Description

With this change envoy will send one GOAWAY frame with a last_stream_id set to 2^31-1, as described in RFC 9113. After a reasonable interval (defaults to 1s), envoy will send a followup GOAWAY frame with last_stream_id set to the highest received stream ID at that point in time.
This interval is configurable using graceful_goaway_timeout field.

Commit Message: http2: send graceful GoAway on close
Additional Description: According to RFC 9113, envoy should send a goaway with last_stream_id set to 2^31-1 before sending a final goaway with the highest received stream ID.
Risk Level: Low
Testing: Tests added. Manually verified.
Docs Changes: NA
Release Notes: Changelog added
Platform Specific Features:
Runtime guard: envoy.reloadable_features.http2_graceful_goaway
Fixes #39876
[Optional API Considerations:]

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only · 2025-09-30T10:28:15Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #41284 was opened by kanurag94.

see: more, trace.

repokitteh-read-only · 2025-09-30T10:28:21Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #41284 was opened by kanurag94.

see: more, trace.

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only · 2025-09-30T17:46:16Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #41284 was synchronize by kanurag94.

see: more, trace.

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 · 2025-10-01T04:10:21Z

/retest

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 · 2025-10-01T13:39:33Z

/retest

Signed-off-by: Anurag Aggarwal <[email protected]>

antoniovleonti · 2025-10-01T14:32:07Z

My understanding is that a graceful goaway is sent at the beginning of the drain sequence via Connection::shutdownNotice() (check code here) That eventually calls this adapter function which in both nghttp2 and oghttp2 sends the goaway frame with the max stream ID as you have done here.

Can you explain why this is not sufficient?

kanurag94 · 2025-10-01T14:59:52Z

My understanding is that a graceful goaway is sent at the beginning of the drain sequence via Connection::shutdownNotice() (check code here) That eventually calls this adapter function which in both nghttp2 and oghttp2 sends the goaway frame with the max stream ID as you have done here.

Can you explain why this is not sufficient?

On connection drain the shutdown notice in both nghttp2/oghttp2 adapter sends goaways correctly. If you look at the original issue for this PR #39876, an important part is filters triggering GoAways immediately. This PR solves that part.
For nghttp2/oghttp2 connection drain and subsequent shutdown, this goaway will not be called AFAICS.

antoniovleonti

I'm sorry! I missed the issue in the description. Thank you for the explanation.

antoniovleonti · 2025-10-01T15:10:06Z

test/common/http/http2/codec_impl_test.cc

+  EXPECT_EQ(1, server_stats_store_.counter("http2.goaway_sent").value());
+}
+
+TEST_P(Http2CodecImplTest, GracefulGoAwayCausesOutboundFlood) {


I think it would be prudent to add some tests showing how this new feature and the existing http connection manager drain phase interact.

Could you also add a couple integration tests?

Thanks!, I added some integration tests. Please let me know if they are okay.

TY for the added testing!

I think what worries me the most is if it's possible for the drain timer to fire after the graceful shutdown timer or vice versa.

Will that crash the envoy? (e.g. the second timer fires and tries calling a function on a now null object).

Could that potentially cause us to send two final goaways (with the second having a higher stream_id-- which is not allowed according to the spec)?

As far as I understand the code (which is little to be honest) -- the destructor on ConnectionImpl should destroy the timers -> cancelling any pending callbacks -> preventing null object access.

Actually the behavior hasn't changed much. I reverted the change on calling gracefulGoAway on onDrainTimeout. Now only filter or load shedding invoked goaways are changed to graceful. Both goaway paths are separate and non overlapping now. It just changes the manual goaway to be more RFC compliant.

However, I will keep attempting adding test.

1. only call custom gracefulGoAway when triggered from LoadSheddding or Filter 2. Add UTs 3. Add integration tests showing interaction of connection drain and filter invoked Signed-off-by: Anurag Aggarwal <[email protected]>

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 · 2025-10-02T06:35:25Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540.
Please correct me if my understanding is wrong.

cc @birenroy, @antoniovleonti, @yanavlasov

Signed-off-by: Anurag Aggarwal <[email protected]>

antoniovleonti · 2025-10-02T20:16:18Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540. Please correct me if my understanding is wrong.

cc @birenroy, @antoniovleonti, @yanavlasov

https://github.com/envoyproxy/envoy/blob/main/envoy/http/codec.h#L599 This comment seems to imply this is working as intended, but I agree the RFC seems to say this shouldn't be allowed.

kanurag94 · 2025-10-03T05:03:07Z

While trying to fix loadShedding integration tests, I see that envoy allows new streams to be created when they shouldn't be during the GOAWAY period. See ServerConnectionImpl::onBeginHeaders(int32_t stream_id). This violates RFC 7540. Please correct me if my understanding is wrong.
cc @birenroy, @antoniovleonti, @yanavlasov

https://github.com/envoyproxy/envoy/blob/main/envoy/http/codec.h#L599 This comment seems to imply this is working as intended, but I agree the RFC seems to say this shouldn't be allowed.

Thanks for the reference. I see the behavior with nghttp2 vs oghttp2 different.

nghttp2 resets any new streams as soon as it goaway.
oghttp2 seems to accept new streams.

I think this makes the behaviour already inconsistent irrespective what we expect according to the comment, orz.

http2: send graceful goaways on close

5cfd1fd

Signed-off-by: Anurag Aggarwal <[email protected]>

repokitteh-read-only bot added the api label Sep 30, 2025

repokitteh-read-only bot assigned markdroth Sep 30, 2025

run format; add changelog; update tests

2b15c51

Signed-off-by: Anurag Aggarwal <[email protected]>

correct alphabetical ordering of runtime guard

284c938

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 marked this pull request as ready for review September 30, 2025 18:23

correct alphabetical ordering of runtime guard - 2

fe44aad

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 added 5 commits October 1, 2025 10:55

Merge remote-tracking branch 'upstream/main' into graceful-http2-goaway

7006b22

add missing flood test

7d6ea68

Signed-off-by: Anurag Aggarwal <[email protected]>

add runtime guard disabled test

32a19ca

Signed-off-by: Anurag Aggarwal <[email protected]>

run format

5c85dd4

Signed-off-by: Anurag Aggarwal <[email protected]>

Merge remote-tracking branch 'upstream/main' into graceful-http2-goaway

8b65795

kick ci

3641061

Signed-off-by: Anurag Aggarwal <[email protected]>

antoniovleonti suggested changes Oct 1, 2025

View reviewed changes

address review comments:

10c24ff

1. only call custom gracefulGoAway when triggered from LoadSheddding or Filter 2. Add UTs 3. Add integration tests showing interaction of connection drain and filter invoked Signed-off-by: Anurag Aggarwal <[email protected]>

KBaichoo assigned birenroy Oct 1, 2025

kanurag94 added 2 commits October 2, 2025 00:17

try fixing failint uts

5a8404b

Signed-off-by: Anurag Aggarwal <[email protected]>

fix test and point out issue

d26710a

Signed-off-by: Anurag Aggarwal <[email protected]>

kanurag94 added 2 commits October 2, 2025 20:05

fix comments

5a24ddc

Signed-off-by: Anurag Aggarwal <[email protected]>

fix spelling

e39a5d5

Signed-off-by: Anurag Aggarwal <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

http2: send graceful goaways on close #41284

http2: send graceful goaways on close #41284

kanurag94 commented Sep 30, 2025 •

edited

Loading

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

antoniovleonti commented Oct 1, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

antoniovleonti left a comment

Uh oh!

antoniovleonti Oct 1, 2025

Uh oh!

kanurag94 Oct 1, 2025 •

edited

Loading

Uh oh!

antoniovleonti Oct 1, 2025

Uh oh!

kanurag94 Oct 1, 2025

Uh oh!

kanurag94 commented Oct 2, 2025

Uh oh!

antoniovleonti commented Oct 2, 2025

Uh oh!

kanurag94 commented Oct 3, 2025

Uh oh!

Uh oh!

http2: send graceful goaways on close #41284

Are you sure you want to change the base?

http2: send graceful goaways on close #41284

Conversation

kanurag94 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

repokitteh-read-only bot commented Sep 30, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

antoniovleonti commented Oct 1, 2025

Uh oh!

kanurag94 commented Oct 1, 2025

Uh oh!

antoniovleonti left a comment

Choose a reason for hiding this comment

Uh oh!

antoniovleonti Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kanurag94 Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovleonti Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kanurag94 Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kanurag94 commented Oct 2, 2025

Uh oh!

antoniovleonti commented Oct 2, 2025

Uh oh!

kanurag94 commented Oct 3, 2025

Uh oh!

Uh oh!

kanurag94 commented Sep 30, 2025 •

edited

Loading

kanurag94 Oct 1, 2025 •

edited

Loading