ext_proc filter: do not send an empty trailer to external processing server if client does not send trailer

[yanjunxiang-google]

If you are reporting any crash or any potential security issue, do not
open an issue in this repo. Please report the issue via emailing
[email protected] where the issue will be triaged appropriately.

Title: One line description
ext_proc filter: do not send an empty trailer to external processing server if client does not send trailer

Description:

If ext_proc filter processing mode is configured with sending trailers, and client or upstream server is not sending trailer, when envoy receives data from client, the end_stream flag is true. In this case, ext_proc filter is creating and sending an empty trailer to external processing server:

envoy/source/extensions/filters/http/ext_proc/ext_proc.cc

Line 268 in a4dbd74

if (end_stream && state.sendTrailers()) {

envoy/source/extensions/filters/http/ext_proc/ext_proc.cc

Line 426 in a4dbd74

if (just_added_trailers) {

The idea of this logic is that even client is not sending trailer, it provides the external processing server the capability to mutate the trailer, and send a trailer back to Envoy and attached to the original message for future processing. However, this logic is not working, please check: #28592.

Since ext_proc filter returns stopIteration, not like other filters which call addTrailers() and return Continue, it's not easy to fix the issue for ext_proc filter.

The idea is to simply remove the logic in ext_proc filter, i.e, do not create and send an empty trailer to the external processing server if client does not send one.

Describe the issue.

[optional Relevant Links:]

Any extra documentation required to understand the issue.

[yanjunxiang-google]

/assign @tyxia @stevenzzzz @htuch @yanavlasov @gbrail

[repokitteh-read-only]

@gbrail cannot be assigned to this issue.

🐱

Caused by: a #28620 (comment) was created by @yanjunxiang-google.

see: more, trace.

[yanjunxiang-google]

I had a chat with @tyxia and agreed that this is a very questionable edge case, we can take it out for now.

If in the future somebody really needs it, we can add the support then after some careful design based on filter_manager behavior and ext_proc state machine.

[tyxia]

Thanks for digging into this issue @yanjunxiang-google . Also, I want to emphasize: We still support sending trailer to external server for mutation/addition when there are trailers from downStream client. The only case we will disable(for now) is sending trailer when there is no trailer from downstream but external server want to add.

[yanjunxiang-google]

That's right!

I think the existing design is debatable. Its principle seems to be sending whatever the external processing server asks, event Envoy ext_proc filter does not have it. I would argue ext_proc filter should only send if it has it.

Also, if the principle is " sending whatever the external processing server asks", then the ext_proc filter behavior should be consistent. However, it is not. For example, if ext_proc filter only sees header, but external processing is asking for trailer, ext_proc filter is not sending it.

IMHO, it's probably easier to switch to the principle that if external processing server is asking for something, send it only if Envoy sees it.

[pgeler]

agree with last point, filters have all capabilities, so in this case users can use lua scripting add a trailer and then process it through ext_proc mutation.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.