Skip to content

filters/on_demand: fix first request dropped when body is present #42158

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
botengyao merged 2 commits into from
Nov 23, 2025
Merged

filters/on_demand: fix first request dropped when body is present #42158

botengyao merged 2 commits into from
Nov 23, 2025

Conversation

@wdauchy
Copy link
Contributor

@wdauchy wdauchy commented Nov 20, 2025 •
edited
Loading

Commit Message:

Track downstream_end_stream_ to support stream recreation with fully read request bodies. Previously, the filter rejected all requests with bodies by checking !callbacks_->decodingBuffer(), even when the body was complete.

Now properly distinguishes incomplete vs complete requests by tracking end_stream state through decoder methods, following the same pattern as the router filter's internal redirect support.

Additional Description:
Risk Level:
Testing: updated tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]

Fixes #17891

[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Nov 20, 2025

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: #42158 was opened by wdauchy.

see: more, trace.

@wdauchy wdauchy force-pushed the vhds branch 2 times, most recently from 5590f59 to bcd1197 Compare November 20, 2025 17:08
@wdauchy wdauchy marked this pull request as ready for review November 20, 2025 18:12
botengyao
botengyao reviewed Nov 22, 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, the fix lgtm, and could you also add a change log here?

/wait

@wdauchy
filters/on_demand: fix first request dropped when body is present
1126a18 
Track downstream_end_stream_ to support stream recreation with fully read
request bodies. Previously, the filter rejected all requests with bodies by
checking !callbacks_->decodingBuffer(), even when the body was complete.

Now properly distinguishes incomplete vs complete requests by tracking
end_stream state through decoder methods, following the same pattern as the
router filter's internal redirect support.

Fixes envoyproxy#17891

Signed-off-by: William Dauchy <william.dauchy@datadoghq.com>

Signed-off-by: William Dauchy <william.dauchy@datadoghq.com>
@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Nov 23, 2025

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

🐱

Caused by: #42158 was synchronize by wdauchy.

see: more, trace.

@wdauchy wdauchy force-pushed the vhds branch 3 times, most recently from d870f25 to b7de6c8 Compare November 23, 2025 15:28
@wdauchy
filers/ondemand: protect body behaviour change
8bcb1f5 
runtime flag to protect the behaviour change

Signed-off-by: William Dauchy <william.dauchy@datadoghq.com>
botengyao
botengyao reviewed Nov 23, 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.

One small improvement otherwise lgtm

/wait

botengyao
botengyao approved these changes Nov 23, 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!

@botengyao botengyao merged commit d39afea into envoyproxy:main Nov 23, 2025
25 checks passed
@adisuissa
Copy link
Contributor

adisuissa commented Nov 24, 2025

Does this addresses the comment mentioned #17891 (comment) for both H/1 and H/2?

@wdauchy
Copy link
Contributor Author

wdauchy commented Nov 24, 2025

Does this addresses the comment mentioned #17891 (comment) for both H/1 and H/2?

I believe the protocol level end_stream flag remains accurate regardless of buffering. The check !callbacks_->decodingBuffer() would fail for HTTP/2, since HTTP/2 may still hold a buffer even when the entire body has already been read.

I will double check this in practice.

@adisuissa
Copy link
Contributor

adisuissa commented Nov 24, 2025

I believe the protocol level end_stream flag remains accurate regardless of buffering. The check !callbacks_->decodingBuffer() would fail for HTTP/2, since HTTP/2 may still hold a buffer even when the entire body has already been read.

I will double check this in practice.

Will it make sense to add integration tests?

@wdauchy
Copy link
Contributor Author

wdauchy commented Nov 24, 2025

I believe the protocol level end_stream flag remains accurate regardless of buffering. The check !callbacks_->decodingBuffer() would fail for HTTP/2, since HTTP/2 may still hold a buffer even when the entire body has already been read.
I will double check this in practice.

Will it make sense to add integration tests?

good point, I will see if I can test this scenario

@wdauchy wdauchy mentioned this pull request Nov 25, 2025
Merged
@wdauchy
Copy link
Contributor Author

wdauchy commented Nov 27, 2025

I believe the protocol level end_stream flag remains accurate regardless of buffering. The check !callbacks_->decodingBuffer() would fail for HTTP/2, since HTTP/2 may still hold a buffer even when the entire body has already been read.
I will double check this in practice.

Will it make sense to add integration tests?

I tried something in #42248
let me know what you think

botengyao pushed a commit that referenced this pull request Dec 3, 2025
@wdauchy
test: add integration tests for on_demand VHDS with request bodies (#…
c352e1c 
…42248)

Commit Message:
Add integration tests verifying VHDS on-demand updates work correctly
with request bodies for both HTTP/1.1 and HTTP/2. The tests use
HttpProtocolIntegrationTest parameterization to automatically run for
both downstream protocols (HTTP/1.1 and HTTP/2), ensuring the fix for
#17891 works correctly across different HTTP protocol versions.

Additional Description:

Tests verify that:
- Request bodies are preserved when triggering on-demand VHDS updates
- Stream recreation works correctly after VHDS updates complete
- Both HTTP/1.1 and HTTP/2 protocols handle bodies correctly

The test instantiation uses getProtocolTestParamsWithoutHTTP3() which
returns parameter combinations for both HTTP/1.1 and HTTP/2 downstream
protocols, so each test runs multiple times (once per protocol
combination).

Complements the unit tests in d39afea
and provides end-to-end verification for #17891.

followup of #42158
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: William Dauchy <william.dauchy@datadoghq.com>
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

@yanavlasov yanavlasov Awaiting requested review from yanavlasov yanavlasov is a code owner

@kyessenov kyessenov Awaiting requested review from kyessenov kyessenov is a code owner

Assignees

@botengyao botengyao

Labels

waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OnDemand VHDS makes the first request returns 404.

3 participants

@wdauchy @adisuissa @botengyao