Configuration canary for the second or further plugins.

[ingwonsong]

Fixes: #175

Applying Configuration Canary for second or further plugin.

In #175, there are some discussion about 1) needs of optimization for multiple plugins, and 2) the initialization in the main thread.

But, before making such optimization, this PR proposes to solve the issue itself because it seems to be critical in terms of stability.

[ingwonsong]

@mathetake @PiotrSikora Friendly ping.

[PiotrSikora]

Can you add tests?

Also, the recent issue was a single plugin with multiple configurations (one valid, one invalid), but this is talking about multiple plugins, so it's unclear which case you are trying to address.

[ingwonsong]

Also, the recent issue was a single plugin with multiple configurations (one valid, one invalid), but this is talking about multiple plugins, so it's unclear which case you are trying to address.

I thought that the multiple filters with the same WasmVM was problematic: envoyproxy/envoy#20843 (comment)

In my understanding, Plugin is created per filter config (per configuration), while Wasm base handle (before cloning per thread) is created per vm_key. Isn't it right?

[PiotrSikora]

I thought that the multiple filters with the same WasmVM was problematic: envoyproxy/envoy#20843 (comment)

In my understanding, Plugin is created per filter config (per configuration), while Wasm base handle (before cloning per thread) is created per vm_key. Isn't it right?

That's the issue with different instances of the same Wasm plugin (i.e. when same was plugin is started with different plugin configuration).

However, #175 (which this PR supposedly resolves) is about multiple Wasm plugins linked in the same Wasm module, which I don't belive this fixes.

[ingwonsong]

Oh.. "multiple Wasm plugin" means a kind of multiple type? (like network, http, bootstrap)

According to #175 (comment), #175 seems to be about that "canary" is not applied to the second plugin instance when vm_key is the same.

[PiotrSikora]

@ingwonsong you need to add rule to build wasm_cc_binary targets in test_data step on the CI (.github/workflows/test.yml).

@chaoqin-li1123 @mpwarres could you do the first review pass? Thank you.

[PiotrSikora]

@ingwonsong could you update Proxy-Wasm C++ SDK to latest as part of this PR?

[ingwonsong]

@ingwonsong could you update Proxy-Wasm C++ SDK to latest as part of this PR?

Done.

[ingwonsong]

@ingwonsong you need to add rule to build wasm_cc_binary targets in test_data step on the CI (.github/workflows/test.yml).

Done, too.

[mpwarres]

LG, thanks!

[ingwonsong]

@PiotrSikora Wam reminder. :)

[ingwonsong]

@PiotrSikora Can you take a look once again? :)

[PiotrSikora]

LGTM, with a few small comments/nits.

[PiotrSikora]

All three YamlLoadFromFileWasmInvalidConfig tests in Envoy are failing because of this change, which is a bit unexpected. Could you take a look?

Ideally, you would open a draft PR in Envoy pointing to this unmerged branch.

[ingwonsong]

All three YamlLoadFromFileWasmInvalidConfig tests in Envoy are failing because of this change, which is a bit unexpected. Could you take a look?

Ideally, you would open a draft PR in Envoy pointing to this unmerged branch.

Sure. will take a look

[ingwonsong]

@PiotrSikora

Can you check out the behavioral difference in the fixed envoy test cases?
https://github.com/envoyproxy/envoy/pull/22469/files

TL;DR - now we always apply canary, so the creation of FilterConfig or AccessLog should raise the exception always when the invalid config is configured.

[PiotrSikora]

Thanks!