dynamic_modules: adds body read/write callbacks

[mathetake]

Commit Message: dynamic_modules: adds body reader/writer functionality
Additional Description:

This adds the following ABI functions:

• envoy_dynamic_module_callback_http_get_request_body_vector_size
• envoy_dynamic_module_callback_http_append_request_body
• envoy_dynamic_module_callback_http_drain_request_body
• envoy_dynamic_module_callback_http_get_response_body_vector
• envoy_dynamic_module_callback_http_get_response_body_vector_size
• envoy_dynamic_module_callback_http_append_response_body
• envoy_dynamic_module_callback_http_drain_response_body

which allows the modules to read and write HTTP body buffers, including
the ability to replace the entire body with a new one.

This completes the initial series of dynamic module feature patches
and after this, the documentation and integration tests will be worked on.

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

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38102 was opened by mathetake.

see: more, trace.

[mathetake]

cc @bplotnick

[isker]

This completes the initial series of dynamic module feature patches

Hi, sorry to ask here but I couldn't find any other place where this is being discussed: is it technically possible to expand the feature set of dynamic modules in the future, e.g. with the equivalent of Lua's httpCall/proxy-wasm's proxy_http_call, or the ability to define custom metrics? We have some wasm filters that do such things but that we would otherwise like to migrate to dynamic modules for the perf.

Thanks!

[mathetake]

Hi @isker! Nothing stops us from implementing that functionality and any other things available in other extension mechanism. Especially for the callout stuff it's of course within my radar and stay tuned! It's just the beginning!

[mathetake]

Metrics will be soon worked on by me or @bplotnick ;)

[isker]

That’s very exciting 🌞. Thanks for all your work on this.

[mattklein123]

At a high level, it's not clear to me that exposing io::Read/io::Write is the best abstraction for this. Why not just operate on lifetime bound EnvoyBuffers? This will allow no copy when the caller doesn't need to copy, and if they need to copy they can copy themselves?

/wait

[mathetake]

@mattklein123 that's indeed a good question and the answer is like i wish i could use EnvoyBuffer as-is. The reason is that in envoy request and response bodies are abstracted by the c++ buffer::Instance virtual class (i think you know well about it) and in reality it essentially models the vector of bytes slices (i guess each corresponds to tcp frame?), which in other words is not contiguous memory region. The current Rust's EnvoyBuffer is simply pointing to a contiguous memory span, so it cannot be used to express the buffer::Instance of C++. So i figured the best i could do was to provide the abstraction over buffer::Instance methods, especially for the common use cases of reading and writing. std::io::{Read, Write} is not that bad i think as the callsite doesn't need to copy everything, for example they can pass it to a serde_json::from_reader or serde_json::to_writer to directly make the Rust constructs work with Envoy buffer::Instance. But I am 100% open to better alternative...

(btw I think maybe we should rename Rust EnvoyBuffer to EnvoySlice to match the Envoy C++ underlying terminology)

[mattklein123]

Can you just return a vector of EnvoyBuffer slices with appropriate lifetimes? That seems fine.

[mathetake]

yeah that should work as long as the number of slices is not that huge (i assume so). let me rework the pr

/wait

[mathetake]

ok so for reading, the raw Vec<EnvoyBuffer<'a>> works well, but for writing it's a bit tricky. The underlying buffer::RawSlice is not resizable so without some wrapper on Vec<EnvoyBuffer<'a>> it's not possible to provider the "replace body" functionality, which is my primary use case.

However, even if i have the wrapper and allows the access to the raw buffer:Instance functionality, it still lacks the function to "shrink" the buffer in case of the new body is smaller than the original size. (to write the json, the callsite cannot know the size before hand and the serialization often writes a smaller bytes at time calling Write::write). So i will add the inverse version of buffer::Instance::drain

[mathetake]

sorry, let me think about it more

[mathetake]

@mattklein123 it took me a while but finally managed to make it in a simple state by simply using Vec<EnvoyBuffer<'a>> as you suggested. I would appreciate it if you could take a look again!

[mathetake]

/retest

[alyssawilk]

@mattklein123 PTAL!

[mattklein123]

In looking at this more it seems kind of confusing that EnvoyBuffer supports both mut and not mut access, as I don't think mutability is safe or works correctly in all cases? For example header map stuff? Is it safe in all cases even if there is a mutable borrow on the filter? Do we need to split the EnvoyBuffer into a read only and mutable type?

/wait-any

[mathetake]

Do we need to split the EnvoyBuffer into a read only and mutable type?

yeah agree this is confusing and I was thinking about the exact same thing about having two EnvoyBuffers kinds for mutable and non-mutable version. I think the headers should be returned with immutable version. I will follow up in the separate PR in a bit

[mattklein123]

OK sounds good you can fix in a follow up. Personally I I would call this one EnvoyMutBuffer and leave the other one as is.