proxy-wasm/proxy-wasm-cpp-host

Configuration canary is only created for the first plugin of VMs

mathetake opened this issue · 3 comments

mathetake commented

Currently configuration canary is only created for the first plugin because we early return createWasm if we find WasmHandle for the same vm_key: https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/master/src/wasm.cc#L485-L486

PiotrSikora commented

Good catch!

There are 2 questions that we need to answer (which perhaps deserve their own issues):

  1. Do we really need to support multiple plugins in a single Wasm module? IMHO, this was a premature optimization requested by Istio Telemetry's team, but they've never used it, and I doubt anyone else is. We have quite a few layers in Proxy-Wasm already (multiple plugins, multiple instances of the same plugin, and multiple requests being handled all in the single Wasm instance), and this is the easiest one to get rid of.

  2. Could we find a way to remove canary and instead clone and instantiate Wasm module on the main thread before spawning it on worker threads, effectively eliminating the need for the canary? I think it should be possible, but I didn't play around with the code to verify it.

mathetake commented

IMHO, this was a premature optimization requested by Istio Telemetry's team, but they've never used it, and I doubt anyone else is.

That's really good to know.. Actually I give +100 to removing the support for multiple plugins in one module style because I think that is one of the most confusing parts. Now I'm working on a comprehensive doc in Go SDK repo (https://github.com/tetratelabs/proxy-wasm-go-sdk/blob/docdocdoc/doc/OVERVIEW.md) and over the course of writing I realized how hard it is to explain and easy to image how difficult it is for newbies to understand this part. Go SDK is already able to support multiple plugins without using "root_id" stuff, but I would be happy to remove that.

  1. let me investigate that later when I have more cycles (in a few weeks), or feel free to beat me to it:)
mathetake commented

As for 2, I just quick researched the code, and the patch would change depending on wether we remove multiple plugins support or not.