Skip to content

ext_proc: buffered mode is not sending local reply with large request #39345

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

Merged
merged 4 commits into from
May 13, 2025

Conversation

yanjunxiang-google
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google commented May 5, 2025

When Envoy is waiting for header response, and new client data arrives Envoy, Envoy should just buffer the data without raising watermarks. Raising watermarks is causing Envoy not sending local reply when the buffer size over the per-connection-limit.

Fixes: #39365

Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
@yanjunxiang-google
Copy link
Contributor Author

/retest

@yanjunxiang-google
Copy link
Contributor Author

/assign @tyxia @stevenzzzz @yanavlasov

@@ -743,6 +743,9 @@ FilterDataStatus Filter::onData(ProcessorState& state, Buffer::Instance& data, b
// StopIterationAndWatermark as well to stop the ActiveStream from returning error when the
// last chunk added to stream buffer exceeds the buffer limit.
state.setPaused(true);
if (state.bodyMode() == ProcessingMode::BUFFERED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some comment here why this special treatment. I think it makes sense in most scenarios, that in happy path, we will buffer the body anyway, so no need to wait for header-response.

But in the rare case that header-response terminates the request already: error, body overrides, we may want to do some clarification here that this branch buffers unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For BUFFERED mode, the API definition is "
// Buffer the message body in memory and send the entire body at once.
// If the body exceeds the configured buffer limit, then the
// downstream system will receive an error.
"

This is regardless how the ext_proc server response will be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comments.

@arminabf
Copy link
Contributor

arminabf commented May 8, 2025

I tested the fix and could no longer reproduce the behavior 👍

Signed-off-by: Yanjun Xiang <[email protected]>
Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

Nice root-cause

@tyxia tyxia merged commit 672ecc4 into envoyproxy:main May 13, 2025
24 checks passed
fishpan1209 pushed a commit to fishpan1209/envoy that referenced this pull request May 22, 2025
…envoyproxy#39345)

When Envoy is waiting for header response, and new client data arrives
Envoy, Envoy should just buffer the data without raising watermarks.
Raising watermarks is causing Envoy not sending local reply when the
buffer size over the per-connection-limit.

Fixes: envoyproxy#39365

---------

Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Ting Pan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misbehaviour with External Processing Filter in Buffered mode since v1.32.0
5 participants