Do not send empty data chunk with end_of_stream FALSE to the ext_proc server

[yanjunxiang-google]

Title:
Do not send empty data chunk with end_of_stream FALSE to the ext_proc server

Description:

1. Currently, Envoy ext_proc filter is sending whatever data receives from the filter chain, and create a gRPC message and send that data to the ext_proc server. This means even a data chunk's size is 0, and end_of_stream = false, it will be sent to the ext_proc server.

2. Such zero size data chunk with end_of_stream = false does not carry any useful information, thus such gRPC messages to the ext_proc server are a totally waste to Envoy and the ext_proc server.

3. In normal situations, it's rare for Envoy codec to send an empty data chunk with end_of_stream = false to the HTTP filter chain. Thus the above problem is not a big deal. Note, zero sized data chunk with end_of_stream = true is meaningful and not in the scope of this discussion.

4. However, with Envoy ext_proc STREAMED mode, the data is moved to a local buffer, and an empty chunk with end_of_stream=false is sent down the filter chain (Seeking proposals for how to implement STREAMING body mode in ext_proc #16760. Such behavior is leveraged to the ext_proc FULL_DUPLEX_STREAMED mode.

5. With 4), also if there are two ext_proc filters configured in the HTTP filter chain, it is pretty routine to see empty data chunks with end_of_stream=false are observed by the 2nd ext_proc filter, as the 1st ext_proc filter created such data chunks.

6. There is no functional issue observed with this issue. However, we should optimize to avoid such kind of wasted gRPC messages.

Solution:

Option 1: For STREAMED mode, after move the data into ext_proc local buffer, it probably can just stop iteration(return StopIterationNoBuffer:

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

Line 706 in a19b275

return Http::FilterDataStatus::StopIterationNoBuffer;

), which will avoid empty data sent down the filter chain. This is recommended in this comment: #16760 (comment), and can avoid the problem altogether. Not sure why it is not implemented like this way.

This option is cleaner, but it has some ext_proc state machine change, so a little bit risky. Also, needs to figure out why existing behavior is sending an empty data chunk down the filter chain.

Option 2: Envoy ext_proc can add a check : if chunk data size is zero, and end_of_stream == false, do not send it to the ext_proc server. This does not avoid zero chunk data + eos_false to the remain filter chain, but avoided it being sent to the ext_proc server.

This option 2 is simple and almost no risk at all.

[yanjunxiang-google]

@yanavlasov @tyxia @gbrail @stevenzzzz

[stevenzzzz]

I think in the original disucssion, "option 3." was eventually implemented because that's most straightforward and would avoid any complications around status bookkeeping esp around resumption. but it seems didn't think of this special case you mentioned: excessive round trips for other ext_proc impls (or other callout alike filters) down the chain.

I think it's probably time to weighing in more on the "cleaner solution": we are already "pausing" the body processing in someway in STREAMED mode (and in full-duplex even more so): per-chunk body response injects data into stream buffer and totaly depends on ext_proc filter status bookkeeping in ext_proc to push the data downward.

I agree with you that the 2d option would be more "safe" in that it doesn't introduce deltas to the activestream side status bookkeeping, but only "safely ignore" no-op body messages to ext_proc server.

I'd let @gbrail or @yanavlasov @tyxia to chime in on this as well, but seems pretty doable with option 1. if no objections heard from them. It does look like a much cleaner option.

[wbpcode]

STREAM mode of ext proc is super complex. Orz.

I guess dym or wasm would be better for stream extension support. 🤣

But thanks for all these works to make it better. 🙇

[yanjunxiang-google]

Thanks @stevenzzzz for your comments!

#44110 implements option 2.

BTW, option 1 and option 2 are not mutual exclusive. Empty chunks with eos=false going through the Envoy filter chain is not a big deal. But sending it through the network interface is a waste. Let's optimize that first. We can assess whether option 1 is necessary down the road.