Fix H2 origin payload handling#13363
Open
bneradt wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes HTTP/2 origin-side edge cases in payload handling by preserving the outbound request method for response validation (e.g., HEAD with non-zero Content-Length but no body) and by capping outbound DATA frame payloads to the remaining write VIO byte count. It also extends the existing H2 origin replay gold tests to cover these regressions.
Changes:
- Track the sent outbound request method on an H2 stream and use it when deciding whether
Content-Lengthvs. received payload-length mismatches are valid for no-body responses. - Limit outbound H2 DATA frame payload length to the remaining write VIO bytes while still properly emitting
END_STREAMwhen the transaction body is complete. - Extend H2 origin replay coverage (HEAD no-body w/ non-zero
Content-Length, large PUT + response) and update expected metric outputs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/proxy/http2/Http2ConnectionState.cc |
Caps DATA frame payload by remaining write VIO bytes; records the outbound request method when sending request headers. |
include/proxy/http2/Http2Stream.h |
Adds write_vio_ntodo() accessor; uses stored sent request method for payload-length validation logic. |
tests/gold_tests/h2/replay_h2origin/h2-origin.yaml |
Adds new replay scenarios for HEAD (non-zero Content-Length, no body) and large PUT request/response. |
tests/gold_tests/h2/h2origin.test.py |
Updates squid log expectations for the new UUID ranges. |
tests/gold_tests/h2/gold/http-request-method-metrics.gold |
Updates the expected PUT request metric count to reflect the new PUT test. |
Comments suppressed due to low confidence (1)
include/proxy/http2/Http2Stream.h:429
- Typo in the Warning message key: "data_legnth" should be "data_length" to make the log output searchable/consistent.
if (content_length != 0 && !is_payload_precluded && content_length != data_length) {
Warning("Bad payload length content_length=%d data_legnth=%d session_id=%" PRId64, content_length,
static_cast<int>(data_length), _proxy_ssn->connection_id());
2521a9e to
c99484e
Compare
HTTP/2 origin responses can legally carry a non-zero Content-Length on responses that do not carry a payload, such as responses to HEAD requests. ATS was validating these responses without retaining the request method that had been sent on the outbound H2 stream, so it could mistake a valid no-body response for a payload length error. Outbound H2 DATA generation also needs to respect the transaction write VIO byte count when a buffer reader has more bytes available than should be sent on the stream. This caps the frame payload length to the remaining write VIO bytes and still marks END_STREAM when that payload finishes the transaction body. This extends the H2 origin replay coverage with a HEAD response that has a non-zero Content-Length and no body, plus a large PUT request and response over an H2 client and H2 origin path.
c99484e to
4d912fa
Compare
Contributor
Author
|
[approve ci] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This breaks out HTTP/2 work from this otherwise HTTP/3 focused PR:
#13213
HTTP/2 origin responses can legally carry a non-zero Content-Length on
responses that do not carry a payload, such as responses to HEAD
requests. ATS was validating these responses without retaining the
request method that had been sent on the outbound H2 stream, so it could
mistake a valid no-body response for a payload length error.
Outbound H2 DATA generation also needs to respect the transaction write
VIO byte count when a buffer reader has more bytes available than should
be sent on the stream. This caps the frame payload length to the
remaining write VIO bytes and still marks END_STREAM when that payload
finishes the transaction body.
This extends the H2 origin replay coverage with a HEAD response that has
a non-zero Content-Length and no body, plus a large PUT request and
response over an H2 client and H2 origin path.