dynamic modules: new body processing ABI

[wbpcode]

Commit Message: dynamic modules: new body processing ABI
Additional Description:

This PR refactor the dynamic modules ABI of body processing to address problem that noticed in the #40918. (To close #40918)

With the new change, the body ABI are parted to two different parts:

1. received body, the body new received to trigger the decodeData/on_request_body callback.
2. buffered body, the body historical buffered body after return the StopIterationAndBuffer/StopIterationAndWatermark.

By this way, the modules could operate the received body or old buffered body clearly and correctly. With the new ABI, the dynamic modules now could provide body processing ability like the native cpp filter.

It's no doubt this brings some complexity because the developers now need to aware the different between buffered body and new received body.
But NOTE, it's inevitable complexity to develop correct body processing extensions.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.

[wbpcode]

I actually want to back port this change to 1.35 and 1.36 because without this change, it's hard to process stream body with dynamic modules.
But that means we will change the ABI version of dynamic module in the minor version of Envoy. WDYT? cc @mathetake @mattklein123

[wbpcode]

cc @anuraaga because I think you are paying attention on the dynamic modules recently.

[anuraaga]

Just a very selfish ask, if you do end up bumping the ABI version, it would be very awesome to also include #41485 :D Then terminal filters would likely be ready for production (minus the macOS only weird #41656).

For this PR, for sending response data we have three functions now, append_received, append_buffered, and send_ (last just calls encodeData). Just curious, is there any overlap among these? Or is it fair to say non-terminal filters should always use append, terminal should always use send? I wasn't sure about this so kept comments lightweight, but maybe we should be more clear about it if it is the case.

https://github.com/wbpcode/envoy/blob/1b5b7005bf16ddb66d203b8c2e23da0a02077a80/source/extensions/dynamic_modules/abi.h#L1351

[wbpcode]

For this PR, for sending response data we have three functions now, append_received, append_buffered, and send_ (last just calls encodeData). Just curious, is there any overlap among these? Or is it fair to say non-terminal filters should always use append, terminal should always use send? I wasn't sure about this so kept comments lightweight, but maybe we should be more clear about it if it is the case.

The append + drain is used to modify body from upstream or local response. The send_ is used to generate a local response for terminal filter.

Just a very selfish ask, if you do end up bumping the ABI version, it would be very awesome to also include #41485 :D Then terminal filters would likely be ready for production (minus the macOS only weird #41656).

Sounds good to me if we can finally get agreement on the backport. And I will check the macOS issue this weekend.

[anuraaga]

The append + drain is used to modify body from upstream or local response. The send_ is used to generate a local response for terminal filter.

Thanks for confirming - so maybe we can add this note to some of the doc comments.

[wbpcode]

The append + drain is used to modify body from upstream or local response. The send_ is used to generate a local response for terminal filter.

Thanks for confirming - so maybe we can add this note to some of the doc comments.

You can add a more clear comment to the send_ method on the sdk. Actually, IMO, it's clear enough 🤣

[mathetake]

I actually want to back port this change to 1.35 and 1.36 because without this change, it's hard to process stream body with dynamic modules.
But that means we will change the ABI version of dynamic module in the minor version of Envoy.

I think we could do that since it's not crashing the Envoy, but Envoy will simply reject the filter config. At the end of the day, users are usually supposed to "bundle" the Envoy binary with the build shared libraries, so yes we could as far as we communicate clearly. In other words, when users deploy a new patched Envoy, they will rebuild the shared library and it will be already part of their deploy flow for the most cases.

I am a bit against it though but i guess the blast radius is much more small because of the reason ^^.

[mathetake]

Sorry I am not following this.. are you saying that any subsequent filters should not access body buffer once this is used, even when they are reading, not modifying? Could you elaborate on why?

[wbpcode]

That's say, once the filters after this current filter X have accessed the body (no matter read or write), then the current filter X cannot use the method to modify the buffered body.
This limitation is inherit from the addEncoded and also the modifyDecodingBuffer, and is the key to ensure the HCM body proccessing correctly works.

This underlying logic is to meet the following rules:

• For specific filter X and given data piece, any change/modification from X should be saw by the filters after X.
• For specific filter X and given data piece, if the filters after X have saw the data piece, then filter X cannot modify it again.

And because the request/response buffer is shared by all filters. Finally result in the limit only the latest data processing filter in the filter chain could modify the buffered body. I will update the comment to try to make it more clear.

(I do want to do some the refactor to loose the limitation and also make the filter chain more robust, but it's another topic and still waiting my free bandwidth)

[mathetake]

overall the direction looks good to me!

[wbpcode]

I actually want to back port this change to 1.35 and 1.36 because without this change, it's hard to process stream body with dynamic modules.
But that means we will change the ABI version of dynamic module in the minor version of Envoy.

I think we could do that since it's not crashing the Envoy, but Envoy will simply reject the filter config. At the end of the day, users are usually supposed to "bundle" the Envoy binary with the build shared libraries, so yes we could as far as we communicate clearly. In other words, when users deploy a new patched Envoy, they will rebuild the shared library and it will be already part of their deploy flow for the most cases.

I am a bit against it though but i guess the blast radius is much more small because of the reason ^^.

Get it. Thanks for the input. The reason why I want to back port these ABIs is that we want use it at the Envoy 1.35 rather than to wait the 1.37.
I think we can declare it clearly in the change log document and also at the envoy-dev/envoy-users