-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
ext_proc filter ASSERT at canIterate() when client not sending trailer #28592
ext_proc filter ASSERT at canIterate() when client not sending trailer #28592
Conversation
…opIteration. Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
Configuration to trigger this:
|
The traceback: [2023-07-24 16:49:33.519][30][trace][http] [source/common/http/filter_manager.cc:550] [Tags: "ConnectionId":"3","StreamId":"14728416450513760437"] decode headers called: filter=envoy.filters.http.router status=1 |
The related logs shows the problem: Tags: "ConnectionId":"3","StreamId":"14728416450513760437"] request end stream ... [2023-07-24 16:49:33.519][30][trace][http] [source/common/http/filter_manager.cc:550] [Tags: "ConnectionId":"3","StreamId":"14728416450513760437"] decode headers called: filter=envoy.filters.http.router status=1 |
There is an issue raised for this: The solution is to follow what's proposed in the issue to just remove the ext_proc filter logic which construct and send empty trailer if client is not sending trailer at same time ext_proc filter is configure to send trailers to the external processing server. |
Signed-off-by: Yanjun Xiang <[email protected]>
…er from client or server Signed-off-by: Yanjun Xiang <[email protected]>
please hold off pushing CI for the next few hours while we push through release CI |
The issue can be simply reproduced by the added ext_proc integration test case in this PR, which test this in the request_body path. It can also be reproduced at the response_body path by below change. diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc verifyDownstreamResponse(*response, 200); Below are the logs, which you can see the spurious message received since ext_proc filter is sending trailer to ext_proc server right after it sends the body to the ext_proc server. This causes the ext_proc state machine broken since it is now waiting for trailer response, but ext_proc server may sends back the body response first. ext_proc state machine requires sequentially message sending. [2023-07-25 18:24:56.541][3802][trace][ext_proc] [source/extensions/filters/http/ext_proc/ext_proc.cc:557] encodeData(100): end_stream = true [2023-07-25 18:24:56.542][16][debug][testing] [./test/integration/fake_upstream.h:170] Sent gRPC message: response_body { [2023-07-25 18:24:56.542][16][debug][testing] [./test/integration/fake_upstream.h:189] Waiting for gRPC message... <<<<<<<<<<<<<< ******* This is wrong. Sending trailers right after sending body in onData() causing ext_prco filter state machine issue: it transitioned into wait for trailer response state when the body response came back. [2023-07-25 18:24:56.543][3802][debug][ext_proc] [source/extensions/filters/http/ext_proc/ext_proc.cc:188] Calling close on stream [2023-07-25 18:24:56.543][16][debug][testing] [./test/integration/fake_upstream.h:170] Sent gRPC message: response_trailers { |
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
/assign @tyxia @yanavlasov @stevenzzzz |
/retest |
/assign @mattklein123 |
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.
Overall approach make sense to me. This edge case (that is not well supported) requires careful evaluation and handling of interaction between two state machine (filter chain and ext_proc).
I am wondering do we need to change ext_proc's doc/ api proto related to trailer mode to mention this behavior change?
Thanks @yanavlasov for approval. @tyxia That's a good question. I searched ext_proc API proto and all the related docs, one place which mentioned this is below: " This doc states we should only deliver if trailers are present and trailer mode is SEND, which matches what's done in this PR. I didn't find any doc or API stating envoy need to send an empty trailer if trailer is not present and ext_proc filter processing mode is configured to SEND trailers. |
@yanjunxiang-google No worries, it is fine if this disabled behavior was not documented before. |
ext_proc filter ASSERT at canIterate() when downstream client is not sending trailer at same time the filter is configured to send trailer to the external processing server.
The problem is that In such case, during decodeData(), ext_proc filter creates an empty trailer and adds it to the trailer map:
envoy/source/extensions/filters/http/ext_proc/ext_proc.cc
Line 274 in 030a5ec
, then returns stopIterationAndWatermark:
envoy/source/extensions/filters/http/ext_proc/ext_proc.cc
Line 288 in 030a5ec
The returned stopIteration cause filter_manager break from the loop:
envoy/source/common/http/filter_manager.cc
Line 692 in 030a5ec
Later on it call decodeTrailer() from next filter, which is router filter:
envoy/source/common/http/filter_manager.cc
Line 699 in 030a5ec
This caused router filter calls decodeTrailer() before call decodeHeaders(). And later on when it call decodeHeaders(), it ASSERT at canIterate().
There is an issue raised for this:
#28620
Commit Message:
Additional Description:
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:]