test-framework icon indicating copy to clipboard operation
test-framework copied to clipboard

Wrong buffer size when using `expect_set_buffer_bytes`

Open vojty opened this issue 1 year ago • 2 comments

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 (https://github.com/vojty/test-framework/commit/a0af2e984cd3b391fb97c3edac4ffa502cb143ff) that makes it work.

Is this a bug or am I missing something?

vojty avatar Aug 05 '24 14:08 vojty

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

PiotrSikora avatar Aug 06 '24 05:08 PiotrSikora

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

vojty avatar Aug 07 '24 07:08 vojty