-
Notifications
You must be signed in to change notification settings - Fork 20
ext_proc: ignore send_body_without_waiting_for_header_response in streamed body modes #154
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @Aditya7880900936! It looks like this is your first PR to kmesh-net/orion 🎉 |
Summary of ChangesHello @Aditya7880900936, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an interaction issue within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses an issue where send_body_without_waiting_for_header_response was being incorrectly applied to streamed body modes, which could violate the ext_proc protocol. The change ensures this optimization is only active for buffered modes, aligning with the expected behavior. The logic is sound and the fix is well-targeted. I have one suggestion to improve the readability of the new conditional logic.
81b70a4 to
bf9c6b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a protocol flow issue where send_body_without_waiting_for_header_response was incorrectly applied to streamed body processing modes (STREAMED and FULL_DUPLEX_STREAMED), which could violate the expected ext_proc protocol flow.
Key Changes:
- Introduced a helper function to detect streamed body modes
- Modified the condition for enabling early body streaming to exclude streamed modes
- Preserved existing behavior for buffered body processing modes (
Buffered,BufferedPartial)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let can_send_body_early = self.send_body_without_waiting_for_header_response | ||
| && send_body_or_trailers | ||
| && !is_streamed_body_mode(&override_mode.body_mode::<Msg>()); | ||
|
|
||
| if M::OBSERVABILITY || can_send_body_early { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the existing tests cover send_body_without_waiting_for_header_response for buffered modes and streamed modes separately, there's no test coverage for the new behavior that ensures send_body_without_waiting_for_header_response is ignored when using streamed body modes (STREAMED or FULL_DUPLEX_STREAMED). Consider adding test cases that verify this interaction to ensure the fix works as intended and to prevent regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
orion-lib/src/listeners/http_connection_manager/ext_proc/processing.rs
Outdated
Show resolved
Hide resolved
dawid-nowak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment...
…reamed body modes Signed-off-by: Aditya7880900936 <adityasanskarsrivastav788@gmail.com>
f3a8d35 to
8fcbd0b
Compare
|
Hi @dawid-nowak , |
dawid-nowak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not sure if this is going to work as expected.. see my other comment
|
|
||
| if M::OBSERVABILITY || (self.send_body_without_waiting_for_header_response && send_body_or_trailers) { | ||
| if M::OBSERVABILITY | ||
| || (self.send_body_without_waiting_for_header_response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am still not sure if this is going to work as expected,
my understanding is streaming_body_enabled should be true unless self.send_body_without_waiting_for_header_response is false and OverridableBodyMode::Streamed
in every other case we assume that we will send the body without waiting for the response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dawid-nowak, thanks for the clarification — that makes sense.
You’re right, according to the Envoy documentation the
send_body_without_waiting_for_header_response flag is only meaningful
for STREAMED mode and ignored for other body modes.
Based on your comment, the correct behavior should be:
streaming_body_enableddefaults totrue- except when:
body_mode == OverridableBodyMode::Streamed- AND
send_body_without_waiting_for_header_response == false
In that specific case we should wait for the header response before
sending body frames.
I’ll update the logic accordingly and adjust the condition so that
non-streamed modes always send the body early, matching Envoy semantics.
Thanks for pointing this out — I’ll push an updated commit shortly.
Summary
This PR fixes the interaction between
send_body_without_waiting_for_header_responseand streamed body processing modes.
When the body processing mode is
STREAMEDorFULL_DUPLEX_STREAMED, sending bodyframes before receiving the header response is incorrect and can violate the expected
ext_proc protocol flow.
This change ensures that
send_body_without_waiting_for_header_responseis ignoredfor streamed body modes, while preserving existing behavior for buffered modes.
Changes
STREAMED,FULL_DUPLEX_STREAMED) via overridable body modesend_body_without_waiting_for_header_responseis setTesting
Fixes #149