2 initial stress tests for the http wasm filters#72
2 initial stress tests for the http wasm filters#72duderino wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
| static Registry::RegisterFactory<WasmFilterConfig, | ||
| Server::Configuration::NamedHttpFilterConfigFactory> | ||
| register_; | ||
| REGISTER_FACTORY(WasmFilterConfig, Server::Configuration::NamedHttpFilterConfigFactory); |
There was a problem hiding this comment.
And all the other filters used this REGISTER_FACTORY macro so added that too
|
|
||
| StringView callout_url{data->toString()}; | ||
|
|
||
| logInfo("Forwarding to: " + std::string(callout_url)); |
There was a problem hiding this comment.
@jplevyak take note. After 20 or so iterations the callout_url will log something that looks corrupted
There was a problem hiding this comment.
Not sure if we can use fmt::format here
There was a problem hiding this comment.
It looks like this is disable for the exception issue.
| for (auto& upstream : fake_upstreams_) { | ||
| if (upstream->localAddress()->ip()) { | ||
| ports.push_back(upstream->localAddress()->ip()->port()); | ||
| ports_.push_back(upstream->localAddress()->ip()->port()); |
There was a problem hiding this comment.
the StressTest subclass needs to set these ports without creating FakeUpstreams
| * Test fixture for all integration tests. | ||
| */ | ||
| class BaseIntegrationTest : Logger::Loggable<Logger::Id::testing> { | ||
| class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> { |
There was a problem hiding this comment.
lets the StressTest subclass use ENVOY_LOG
duderino
force-pushed
the
jblatt_wasm_stress_test
branch
from
9683bdf to
097ccb9
Compare
| public: | ||
| MyGrpcCallHandler(ExampleContext *context) | ||
| : GrpcCallHandler<google::protobuf::Value>(context), context_(context) {} | ||
| void onCreateInitialMetadata() override {} |
There was a problem hiding this comment.
Should we convert these from pure virtuals to having a default implementation of {} so that we can avoid requiring handlers which are used?
There was a problem hiding this comment.
Sure, would you like me to do that in this PR?
There was a problem hiding this comment.
Nothing, just wondering if you thought that was a good idea.
There was a problem hiding this comment.
Oh, I'm pretty neutral on that. Most people will copy the boilerplate anyways and then hack it up.
lambdai
left a comment
lambdai
left a comment
There was a problem hiding this comment.
comments for the topmost files
| static Registry::RegisterFactory<WasmFilterConfig, | ||
| Server::Configuration::NamedHttpFilterConfigFactory> | ||
| register_; | ||
| REGISTER_FACTORY(WasmFilterConfig, Server::Configuration::NamedHttpFilterConfigFactory); |
| return FilterHeadersStatus::Continue; | ||
| } | ||
|
|
||
| StringView callout_url{data->toString()}; |
There was a problem hiding this comment.
always prefer data->view() since it's a string_view
|
|
||
| StringView callout_url{data->toString()}; | ||
|
|
||
| logInfo("Forwarding to: " + std::string(callout_url)); |
There was a problem hiding this comment.
Not sure if we can use fmt::format here
duderino
force-pushed
the
jblatt_wasm_stress_test
branch
from
f3c8023 to
7b20bbc
Compare
duderino
changed the title
|
It seems that exception support in emscripten is... poor. First, in order to get exception support we need to add "-s DISABLE_EXCEPTION_CATCHING=0" to the Makefile. After that, we would need a great deal of support, implementing a raft of invoke_XXX and dynCall_XXX functions in addition to __cxa calls. All this is likely to change as well when we move to the new upstream (LLVM) based backend for emscripten. The upshot is that I think we should drop exception support within the VM until this gets sorted out. Thoughts? |
duderino
force-pushed
the
jblatt_wasm_stress_test
branch
2 times, most recently
from
5b59d88 to
f6eb3a8
Compare
@jplevyak if we drop exception support, what happens if third party code, etc in a wasm bundle throws an exception? |
Signed-off-by: Joshua Blatt <jblatt@google.com> Signed-off-by: Joshua Blatt <jtblatt@gmail.com>
duderino
force-pushed
the
jblatt_wasm_stress_test
branch
from
f6eb3a8 to
9549c42
Compare
duderino
changed the title
| TestEnvironment::runfilesDirectory(), | ||
| "/test/extensions/filters/http/wasm/test_data/grpc_callout_", wasmLang(), ".wasm"); | ||
| // Must match cluster name in the wasm bundle: | ||
| const std::string callout_cluster_name{"callout_cluster"}; |
There was a problem hiding this comment.
@mandarjog Here is an example wasm test case. The link between this test case and the wasm bundle is this 'callout_cluster' name.
This test case creates two clusters: a 'callout_cluster' to simulate some random upstream that the wasm bundle can callout to and a 'origin_cluster' which the data plane request will be forwarded to. The grpc_callout_cpp.cc file passes the 'callout_cluster' name to the grpc library and Envoy does the rest
|
@duderino Could you split the PR? Even we exclude the generated code this PR is no doubt a huge one. Very hard to review |
The test framework was already reviewed by @jplevyak back in the istio/proxy repo, so I though we could just not review the framework part (everything under stress/)? Once this is merged and has a few additional test cases I'm planning on sending the framework to envoyproxy/envoy where it'll experience another batch of reviews |
4 participants
@jplevyak this should get you started and I'll keep adding additional test cases.
Test it like so:
there's a basic grpc callout test that mostly works... except the wasm filter thinks the grpc response is bad. It logs this for every grpc response it receives:
The http callout test throws an exception inside the wasm bundle. that one is currently skipped.