Skip to content
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

Wrong buffer size when using expect_set_buffer_bytes #24

Open
vojty opened this issue Aug 5, 2024 · 2 comments
Open

Wrong buffer size when using expect_set_buffer_bytes #24

vojty opened this issue Aug 5, 2024 · 2 comments

Comments

@vojty
Copy link

vojty commented Aug 5, 2024

Hello,
I'm writing an envoy plugin that replaces certain HTTP responses and I'm confused about using set_http_response_body(start, size, value); (in the plugin implementation) and expect_set_buffer_bytes (in the integration test)

A simple example code that replaces HTTP responses:

fn on_http_response_body(&mut self, body_size: usize, end_of_stream: bool) -> Action {
  ...
  if some_condition {
    let bytes = vec![....];
    let size = body_size; // <---------------- THIS
    self.set_http_response_body(0, size, bytes.as_slice());
                                // ^^^^--------- AND THIS
  }
  ...
}

From my testing and debugging with an actual running Envoy instance, it looks like the size parameter has to be set EXACTLY to body_size argument, otherwise, there would be leftovers from the previous response (if the original response is larger than the replacement).

If I'm not mistaken, the data flow goes like this:

  1. https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/main/src/exports.cc#L508
  2. https://github.com/envoyproxy/envoy/blob/b508cb4d78bea275a9cebc43c9000b2b41cc9e56/source/extensions/common/wasm/context.cc#L141-L146
  3. https://github.com/envoyproxy/envoy/blob/b508cb4d78bea275a9cebc43c9000b2b41cc9e56/envoy/buffer/buffer.h#L225-L229
    so the buffer is drained up to size argument.

However, passing the original body size doesn't work with this tool. .expect_set_buffer_bytes(Some(BufferType::HttpResponseBody), Some(expected_body)) computes the size internally from expected_body and cannot be changed.
I did a dummy workaround in my fork (vojty@a0af2e9) that makes it work.

Is this a bug or am I missing something?

@PiotrSikora
Copy link
Member

From my testing and debugging with an actual running Envoy instance, it looks like the size parameter has to be set EXACTLY to body_size argument, otherwise, there would be leftovers from the previous response (if the original response is larger than the replacement).

That's correct, and this is done to allow users to perform prepend/append/inject/replace without excessive copies and multitude of APIs, but perhaps it's not a very intuitive interface. Feel free to file an issue in the Rust SDK.

However, passing the original body size doesn't work with this tool. .expect_set_buffer_bytes(Some(BufferType::HttpResponseBody), Some(expected_body)) computes the size internally from expected_body and cannot be changed. I did a dummy workaround in my fork (vojty@a0af2e9) that makes it work.

Is this a bug or am I missing something?

cc @alexsnaps @mpwarres

@vojty
Copy link
Author

vojty commented Aug 7, 2024

That's correct, and this is done to allow users to perform prepend/append/inject/replace without excessive copies and multitude of APIs, but perhaps it's not a very intuitive interface. Feel free to file an issue in the Rust SDK.

Thanks for clarifying this. It makes sense but it's not clear. Maybe extending examples or adding short documentation in the SDK repository would be enough. I'll file an issue there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants