wasm: Add data buffering for chunks #36411

juanmolle · 2024-10-01T20:06:06Z

Commit Message: wasm: Add data buffering for chunks

Additional Description:
In HTTP/2 connections, the last chunk, which contains the end_of_stream flag, is not used to call the Wasm callback. This fix addresses the issue by dumping the data into the buffer before calling the Wasm callback, ensuring that the data is now present.

Risk Level: Low
Testing: yes
Docs Changes: n/a
Release Notes: yes
Platform Specific Features: n/a

Fixes #35884

Signed-off-by: Juan Manuel Ollé <[email protected]>

juanmolle · 2024-10-01T21:00:41Z

/retest

mpwarres · 2024-10-02T15:20:47Z

Ack. I am a bit overloaded at the moment but will review this evening.

alyssawilk · 2024-10-07T13:12:44Z

@mpwarres ping?

yanavlasov · 2024-10-09T20:16:45Z

This looks good to me. Will wait for @mpwarres review and then commit

/wait-any

wbpcode · 2024-10-10T07:44:00Z

source/extensions/common/wasm/context.cc

+  if (buffering_request_body_) {
+    decoder_callbacks_->addDecodedData(data, false);
+  }


I think this will make the final output contains repeated data piece?

/wait

I wasn't familiar with that function. and when try to understand where it was used and for what, I found this comment
https://github.com/envoyproxy/envoy/blob/main/source/server/admin/admin_filter.cc#L25
in our case streamming is a supported scenario and that is the reason it is only dumped in buffering scenario

I think I have that scenario covered with the test I have added. the one add at the end of the body '.0'
if other. scenario is not covered let me know I could add a test for that.
I made the suite for http and http/2 because this issue is not happening in http1, it seams the library is always sending the last chunk containing end_of_stream with 0 bytes.

the addDecodedData use the move semantics. So, yeah, then this should be safe. I will take a more detailed look this night.

wbpcode · 2024-10-10T13:57:29Z

test/extensions/filters/http/wasm/wasm_filter_integration_test.cc

+  auto request_body = std::vector<std::string>{{"request_"}, {"body"}};
+  auto upstream_response_body = std::vector<std::string>{{"upstream_"}, {"body"}};


could we add a test with 3 (or more) pieces?

For the first piece, the addDecodedData won't be called before it haven't enter the buffering mode.
For the second piece, the addDecodeData will be called but it's not the ending, so, the filter chain will still be stoped to handle the empty piece (the piece has been moved by the addDecodedData).
For the third piece, the addDecodedData will be called and it the ending, so the filter chain will continue.

wbpcode · 2024-10-10T13:58:05Z

One comment to the test. Other things are fine. Thanks for this great fix. 🌷

Signed-off-by: Juan Manuel Ollé <[email protected]>

wbpcode · 2024-10-11T01:48:29Z

ping for @mpwarres for final review.

mpwarres

LGTM, just minor/cosmetic comments. Thanks!

mpwarres · 2024-10-14T04:44:51Z

source/extensions/common/wasm/context.cc

@@ -1801,7 +1807,7 @@ Http::FilterDataStatus Context::encodeData(::Envoy::Buffer::Instance& data, bool
  buffering_response_body_ = false;
  switch (result) {
  case Http::FilterDataStatus::Continue:
-    request_body_buffer_ = nullptr;
+    response_body_buffer_ = nullptr;


Thanks for catching this as well.

mpwarres · 2024-10-14T04:47:13Z

test/extensions/filters/http/wasm/test_data/test_body_cpp.cc

+    logBody(type);
+    if (end_of_stream) {
+      getBufferStatus(type, &size, &flags);
+      setBuffer(type, size, 0, ".0");


(optional) nit: for consistency with other operation handling in this plugin, where descriptive text (e.g. "partial.replace") is used as the data that is inserted, you might consider using ".end" or ".appended" instead of ".0".

mpwarres · 2024-10-14T05:12:30Z

test/extensions/filters/http/wasm/wasm_filter_integration_test.cc

@@ -12,13 +12,21 @@ namespace {

 class WasmFilterIntegrationTest
    : public HttpIntegrationTest,
-      public testing::TestWithParam<std::tuple<std::string, std::string, bool>> {
+      public testing::TestWithParam<
+          std::tuple<std::tuple<std::string, std::string, bool>, Http::CodecType>> {


optional: you could define a helper function similar to wasmDualFilterTestMatrix to allow the test to use a flat tuple<std::string, std::string, bool, Http::CodecType> rather than nesting tuples, which gets a little awkward with the nested std::get<>s.

mpwarres · 2024-10-14T05:13:56Z

test/extensions/filters/http/wasm/wasm_filter_integration_test.cc

+  static std::string
+  testParamsToString(const ::testing::TestParamInfo<
+                     std::tuple<std::tuple<std::string, std::string, bool>, Http::CodecType>>& p) {
+    return fmt::format("{}_{}_{}_{}", std::get<2>(std::get<0>(p.param)) ? "downstream" : "upstream",


optional: if you decide to stick with nested tuples for test params, structured bindings might make this a little easier to read, e.g.

auto [wasm_test_params, codec] = p.param; auto [runtime, language, direction] = wasm_test_params; return fmt::format("{}_{}_{}_{}", direction ? "downstream" : "upstream", runtime, language, ...);

Signed-off-by: Juan Manuel Ollé <[email protected]>

wbpcode · 2024-10-14T15:04:33Z

test/extensions/common/wasm/wasm_runtime.cc

+wasmDualFilterWithCodecsTestMatrix(bool include_nullvm, bool cpp_only,
+                                   std::vector<Http::CodecType> codecs_type) {
+  std::vector<std::tuple<std::string, std::string, bool, Http::CodecType>> values;
+  for (const auto& codec_type : codecs_type) {


explicit type is better than auto except the type name is complex.
And you should use value semantics here rather than reference semantics for enum.

By the way, I think you could also use structured bindings in this helper function?

Signed-off-by: Juan Manuel Ollé <[email protected]>

mpwarres

Thanks, LGTM!

wbpcode · 2024-10-15T02:04:51Z

This is a bug fix of wasm and won't make thing worse anyway. So, will let it enter the 1.32 at the last minute.

Commit Message: wasm: Add data buffering for chunks Additional Description: In HTTP/2 connections, the last chunk, which contains the end_of_stream flag, is not used to call the Wasm callback. This fix addresses the issue by dumping the data into the buffer before calling the Wasm callback, ensuring that the data is now present. Risk Level: Low Testing: yes Docs Changes: n/a Release Notes: yes Platform Specific Features: n/a Fixes envoyproxy#35884 --------- Signed-off-by: Juan Manuel Ollé <[email protected]> Signed-off-by: Gustavo <[email protected]>

wasm: Add data buffering for chunks

655c260

Signed-off-by: Juan Manuel Ollé <[email protected]>

kyessenov assigned mpwarres Oct 1, 2024

juanmolle mentioned this pull request Oct 2, 2024

wasm: response body buffering with http2 #36227

Closed

alyssawilk assigned yanavlasov Oct 7, 2024

yanavlasov previously approved these changes Oct 9, 2024

View reviewed changes

repokitteh-read-only bot added the waiting:any label Oct 9, 2024

wbpcode reviewed Oct 10, 2024

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting:any labels Oct 10, 2024

juanmolle requested a review from wbpcode October 10, 2024 11:47

wbpcode reviewed Oct 10, 2024

View reviewed changes

Add extra test

04b008a

Signed-off-by: Juan Manuel Ollé <[email protected]>

juanmolle dismissed yanavlasov’s stale review via 04b008a October 10, 2024 15:07

repokitteh-read-only bot removed the waiting label Oct 10, 2024

mpwarres approved these changes Oct 14, 2024

View reviewed changes

some rework

13cbc23

Signed-off-by: Juan Manuel Ollé <[email protected]>

juanmolle requested a review from mpwarres October 14, 2024 14:29

juanmolle added 2 commits October 14, 2024 11:33

Merge branch 'main' into fix_wasm_body_buffering

b2087b3

Signed-off-by: Juan Manuel Ollé <[email protected]>

use correct path

d20b21e

Signed-off-by: Juan Manuel Ollé <[email protected]>

wbpcode reviewed Oct 14, 2024

View reviewed changes

some rework

dee70d1

Signed-off-by: Juan Manuel Ollé <[email protected]>

juanmolle requested a review from wbpcode October 14, 2024 18:48

mpwarres approved these changes Oct 14, 2024

View reviewed changes

wbpcode approved these changes Oct 15, 2024

View reviewed changes

wbpcode merged commit 2e4ee89 into envoyproxy:main Oct 15, 2024
20 checks passed

juanmolle deleted the fix_wasm_body_buffering branch October 15, 2024 11:33

wbpcode mentioned this pull request Oct 20, 2024

wasm: removed automatical route refreshment and add a foreign function to clear the route cache #36671

Merged

wbpcode mentioned this pull request Nov 5, 2024

large request body result in wasm crash #36989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasm: Add data buffering for chunks #36411

wasm: Add data buffering for chunks #36411

juanmolle commented Oct 1, 2024

juanmolle commented Oct 1, 2024

mpwarres commented Oct 2, 2024

alyssawilk commented Oct 7, 2024

yanavlasov commented Oct 9, 2024

wbpcode Oct 10, 2024

juanmolle Oct 10, 2024 •

edited

Loading

wbpcode Oct 10, 2024

wbpcode Oct 10, 2024

wbpcode commented Oct 10, 2024

wbpcode commented Oct 11, 2024

mpwarres left a comment

mpwarres Oct 14, 2024

mpwarres Oct 14, 2024

mpwarres Oct 14, 2024

mpwarres Oct 14, 2024

wbpcode Oct 14, 2024

mpwarres left a comment

wbpcode commented Oct 15, 2024

		auto request_body = std::vector<std::string>{{"request_"}, {"body"}};
		auto upstream_response_body = std::vector<std::string>{{"upstream_"}, {"body"}};

wasm: Add data buffering for chunks #36411

wasm: Add data buffering for chunks #36411

Conversation

juanmolle commented Oct 1, 2024

juanmolle commented Oct 1, 2024

mpwarres commented Oct 2, 2024

alyssawilk commented Oct 7, 2024

yanavlasov commented Oct 9, 2024

Choose a reason for hiding this comment

juanmolle Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Oct 10, 2024

wbpcode commented Oct 11, 2024

mpwarres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpwarres left a comment

Choose a reason for hiding this comment

wbpcode commented Oct 15, 2024

juanmolle Oct 10, 2024 •

edited

Loading