search

proxy-wasm / proxy-wasm-cpp-host

WebAssembly for Proxies (C++ host implementation)
Apache License 2.0
84 stars 69 forks source link

wasmtime: update to v9.0.3. #355

Closed phlax closed 1 year ago

phlax commented 1 year ago

@PiotrSikora this seems to be passing ci here, but when i test it in envoy ci im seeing errors like ...

[2023-05-31 10:21:05.114][18][error][wasm] [external/envoy/source/extensions/common/wasm/wasm_vm.cc:38] Failed to load Wasm module due to a missing import: env.proxy_get_header_map_valuen
[2023-05-31 10:21:05.114][18][error][wasm] [external/envoy/source/extensions/common/wasm/wasm.cc:109] Wasm VM failed Failed to initialize Wasm code
unknown file: Failure
C++ exception with description "Unable to create Wasm HTTP filter " thrown in the test body.
[  FAILED  ] Runtimes/WasmFilterConfigTest.YamlLoadFromFileWasmFailOpenOk/wamr_cpp, where GetParam() = ("wamr", "cpp") (831 ms)
[----------] 1 test from Runtimes/WasmFilterConfigTest (831 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (831 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] Runtimes/WasmFilterConfigTest.YamlLoadFromFileWasmFailOpenOk/wamr_cpp, where GetParam() = ("wamr", "cpp")

 1 FAILED TEST

https://dev.azure.com/cncf/envoy/_build/results?buildId=138916&view=logs&j=e969334a-0e55-5c18-ac96-8b546753391e&t=32392586-69f5-5768-4caa-e00e8d4cc47e&l=407

PiotrSikora commented 1 year ago 
[2023-05-31 10:21:05.114][18][error][wasm] [external/envoy/source/extensions/common/wasm/wasm_vm.cc:38] Failed to load Wasm module due to a missing import: env�.proxy_get_header_map_valuen

This is because WAMR was updated here, but apparently not in Envoy, and they made a breaking changes in the code.

You need to update WAMR (and presumably LLVM) in Envoy first, unfortunately.

phlax commented 1 year ago

You need to update WAMR (and presumably LLVM) in Envoy first, unfortunately.

im trying to knock out a set of patch releases (hopefully tomorrow) to address any flagged cves, so trying to push this through

ill raise a pr to update wamr now ...

phlax commented 1 year ago

versions updated here - this really needs a better solution for updating

phlax commented 1 year ago

https://github.com/envoyproxy/envoy/pull/27737

phlax commented 1 year ago

confirming that this PR at current HEAD + wamr update passes envoy ci

PiotrSikora commented 1 year ago

versions updated here - this really needs a better solution for updating

IMHO, the best way would be if Envoy used proxy_wasm_cpp_host_repositories() and proxy_wasm_cpp_host_dependencies() to pull required deps instead of redeclaring them in Envoy.

But I think there was a supply chain related reason why you want to control them there?

phlax commented 1 year ago

... the best way would be if Envoy used ...

how would that be different if one of us still has to come here and do this to update?

the only difference i think would be that we would probably not actually see the CVE notices

phlax commented 1 year ago

updated to 9.0.3

phlax commented 1 year ago

updated to 9.0.3

somewhat involuntarily - it changed all the versions running generate-lock - ie didnt respect the various places the versions are (manually) set

PiotrSikora commented 1 year ago

how would that be different if one of us still has to come here and do this to update?

the only difference i think would be that we would probably not actually see the CVE notices

You'd only need to do a single "atomic" bump of proxy-wasm-cpp-host in Envoy, without risk of having other dependencies out-of-sync, like with WAMR now (I thought that was your main complaint?).

phlax commented 1 year ago

You'd only need to do a single "atomic" bump of proxy-wasm-cpp-host in Envoy, without risk of having other dependencies out-of-sync, like with WAMR now

i guess there is some benefit to delegating the dep setup - it would mess up our cve tracking, which tbh, i think this repo depends on as we are the ones updating it

(I thought that was your main complaint?).

the main complaint is the complexity of doing this - everyone dreads it and as we track the cves it needs to be updated fairly frequently

ideally we reduce what is required to a single command - ie ./ci/update_wasmtime.sh kinda thing - then it would be no big deal, and potentially could be automated with ci here - the current situation is a nightmare

PiotrSikora commented 1 year ago

the main complaint is the complexity of doing this - everyone dreads it and as we track the cves it needs to be updated fairly frequently

ideally we reduce what is required to a single command - ie ./ci/update_wasmtime.sh kinda thing - then it would be no big deal, and potentially could be automated with ci here - the current situation is a nightmare

cc @mpwarres @martijneken