Skip to content

HTTP2: change the default reset code from no error to internal error #42269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
wbpcode merged 9 commits into from
Dec 4, 2025
Merged

HTTP2: change the default reset code from no error to internal error #42269

wbpcode merged 9 commits into from
Dec 4, 2025

Conversation

@wbpcode
Copy link
Member

@wbpcode wbpcode commented Nov 26, 2025

Commit Message: HTTP2: change the default reset code from no error to internal error
Additional Description:

This PR makes Envoy generate less NO_ERROR from the codec when there is a stream reset to close #36200.

https://httpwg.org/specs/rfc9113.html#RST_STREAM
https://httpwg.org/specs/rfc9113.html#ErrorCodes

Thanks so much for the great work from @botengyao in the #40729

Risk Level: mid.
Testing: unit + integration
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Nov 26, 2025

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

🐱

Caused by: #42269 was opened by wbpcode.

see: more, trace.

@wbpcode wbpcode requested a review from yanavlasov November 26, 2025 08:15
wbpcode
wbpcode commented Nov 26, 2025
View reviewed changes
source/common/http/http2/codec_impl.cc
Comment on lines -897 to +924
codec_callbacks_->onCodecLowLevelReset();
// TODO(wbpcode): this ensure that onCodecLowLevelReset is only called once. But
// we should replace this with a better design later.
// See https://github.com/envoyproxy/envoy/issues/42264 for why we need this.
if (!codec_low_level_reset_is_called_) {
codec_low_level_reset_is_called_ = true;
codec_callbacks_->onCodecLowLevelReset();
}
Copy link
Member Author

@wbpcode wbpcode Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changed the default stream reset code and will enlarger the problem in the #42264. So, I made a quick work around to resolve problem here.

@wbpcode
Copy link
Member Author

wbpcode commented Nov 26, 2025 •
edited
Loading

Compare to the change in the #40729, this PR removed all RemoteResetNoError related things. This is because:

  1. error code from remote is untrustable anyway. The server or client may have similar problem of Envoy and abuse NO_ERROR.
  2. upstream remote reset no error not means that we can reset the downstream stream with NO_ERROR because the retrying, L7 filter chain processing and so on.
  3. In the Envoy, the resetStream() should only be called when there is an error. And any other normal response or completion, the end stream flag should be used in the frame.

@wbpcode
Copy link
Member Author

wbpcode commented Nov 27, 2025

/retest

@mattklein123
Copy link
Member

mattklein123 commented Nov 27, 2025

At a high level this makes sense to me: send NO_ERROR only if the response is complete. I don't have the time to analyze the details on this so will defer to others on that.

botengyao
botengyao reviewed Nov 27, 2025
View reviewed changes
Copy link
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wbpcode for driving this! Here is a quick pass, and I will take an additional look this weekend, and the CI is legit for ogHTTP2 + ngHTTP2 + UHV.

/wait

@wbpcode
fix test
c81fe0e 
Signed-off-by: wbpcode <[email protected]>
botengyao
botengyao reviewed Dec 2, 2025
View reviewed changes
Copy link
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! lgtm overall module one concern.

/wait

source/common/http/conn_manager_impl.cc Outdated
Comment on lines 274 to 278
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reset_with_error");
// We should not propagate UpstreamProtocolError to downstream as that indicates
// an error on the upstream connection and may have nothing to do with the downstream.
// But we do need to handle DownstreamProtocolError which indicates a protocol error
// on the downstream connection.
Copy link
Member

@botengyao botengyao Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emm, I understand the use of resetStream() is to downstream, but still think we need to propagate the ProtocolError if there are either DownstreamProtocolError or UpstreamProtocolError, and keep consistent as before. If you are strong with this, would an additional PR with a new runtime guard, saying that UpstreamProtocolError is rest with LocalRest be better?

Copy link
Member Author

@wbpcode wbpcode Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted it and left a TODO there.

@botengyao
Copy link
Member

botengyao commented Dec 2, 2025

/retest

@wbpcode
address comment
f1371e1 
Signed-off-by: wbpcode <[email protected]>
@wbpcode
Copy link
Member Author

wbpcode commented Dec 3, 2025

/retest

botengyao
botengyao previously approved these changes Dec 3, 2025
View reviewed changes
Copy link
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving this behavior @wbpcode.

RyanTheOptimist
RyanTheOptimist previously approved these changes Dec 3, 2025
View reviewed changes
@wbpcode wbpcode dismissed stale reviews from RyanTheOptimist and botengyao via 44990ec December 4, 2025 02:27
@wbpcode wbpcode enabled auto-merge (squash) December 4, 2025 02:28
@wbpcode
Copy link
Member Author

wbpcode commented Dec 4, 2025

/retest

@wbpcode wbpcode merged commit 8f0bc15 into envoyproxy:main Dec 4, 2025
24 of 25 checks passed
@wbpcode wbpcode deleted the dev-fix-http2-error branch December 4, 2025 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@botengyao botengyao botengyao approved these changes

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

@yanavlasov yanavlasov Awaiting requested review from yanavlasov

Assignees

@RyanTheOptimist RyanTheOptimist

@botengyao botengyao

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HTTP2 locally-originated stream resets use the NO_ERROR code

4 participants

@wbpcode @mattklein123 @botengyao @RyanTheOptimist