wasm: plugin configuration is racey

[kyessenov]

Context: istio/istio#28017

Plugin configuration is applied every time xDS is loaded that references the same root_id. This is inherently racey if the same root_id is referenced in multiple places. It is also applied if the xDS is rejected, corrupting the existing plugin configuration state.

[PiotrSikora]

I'm working on this.

[kyessenov]

@PiotrSikora Can you share how you want to plan to fix this? I proposed a few options and not sure which one is the preference. I'm curious how we can make it work in the upcoming istio release.

[PiotrSikora]

Second option from istio/istio#28017 (comment), i.e. create a new root context for each plugin configuration.

See those PRs: proxy-wasm/proxy-wasm-cpp-host#78 and #13753. Could you give them a try?

Note that those PRs are still WIP, and plugin instances are not garbage collected, but other than that it works fine in WasmVMs (I didn't do any testing with NullVMs, other that the fact that it's passing existing tests).

[PiotrSikora]

Thinking about this now, the existing PRs have one-to-many relationship between in-VM RootContext and Plugins/FilterConfigs, where plugin_root_id and plugin_configuration are used as the sharing key.

But perhaps it makes sense to make this relationship one-to-one, to simplify lifecycle and make the reasoning about what's configured in Envoy and what's available within WasmVM a bit more sane? What do you think?

[kyessenov]

I think that would have too much effect on existing plugins. The root context is used as a per-worker singleton already, so changing the semantics would change that. It's not easy to tell the performance cost of having many roots.

[kyessenov]

Confirmed this is not happening on master (istio/proxy and envoy). Extensions usually assumed eternal root, so they need to be fixed to clean up properly on termination. For example, telemetry should be pushed or it gets dropped.