Configuration canary for the second or further plugins. #289

ingwonsong · 2022-06-12T17:16:43Z

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 · 2022-06-16T18:13:25Z

@mathetake @PiotrSikora Friendly ping.

PiotrSikora · 2022-06-16T19:57:06Z

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 · 2022-06-16T20:31:31Z

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 · 2022-06-16T22:00:58Z

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 · 2022-06-16T22:19:26Z

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 · 2022-07-13T07:33:38Z

@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?

src/wasm.cc

Signed-off-by: Ingwon Song <igsong@google.com>

src/wasm.cc

test/utility.h

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong · 2022-07-14T19:48:22Z

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

Done.

ingwonsong · 2022-07-14T19:48:47Z

@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.

Signed-off-by: Ingwon Song <igsong@google.com>

mpwarres

LG, thanks!

ingwonsong · 2022-07-21T00:02:37Z

@PiotrSikora Wam reminder. :)

src/wasm.cc

test/wasm_test.cc

test/test_data/configure_check.cc

test/wasm_test.cc

.github/workflows/test.yml

test/utility.h

Signed-off-by: Ingwon Song <igsong@google.com>

…tion Signed-off-by: Ingwon Song <igsong@google.com>

…t_data Signed-off-by: Ingwon Song <igsong@google.com>

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong · 2022-07-28T23:39:50Z

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

PiotrSikora

LGTM, with a few small comments/nits.

test/utility.h

test/wasm_test.cc

test/test_data/BUILD

test/test_data/configure_check.cc

test/wasm_test.cc

Signed-off-by: Ingwon Song <igsong@google.com>

test/utility.cc

Signed-off-by: Ingwon Song <igsong@google.com>

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 · 2022-07-29T17:55:46Z

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

.github/workflows/test.yml

ingwonsong · 2022-07-29T19:29:43Z

@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.

Signed-off-by: Ingwon Song <igsong@google.com>

PiotrSikora

Thanks!

Fixes proxy-wasm#175. Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong requested review from PiotrSikora and mathetake as code owners June 12, 2022 17:16

ingwonsong force-pushed the canary-second branch 2 times, most recently from a68b9ac to 0da9fd0 Compare June 14, 2022 18:42

ingwonsong force-pushed the canary-second branch 3 times, most recently from da5fba7 to 73caefd Compare July 1, 2022 21:52

PiotrSikora reviewed Jul 13, 2022

View reviewed changes

chaoqin-li1123 reviewed Jul 13, 2022

View reviewed changes

src/wasm.cc Outdated Show resolved Hide resolved

ingwonsong added 5 commits July 14, 2022 00:05

Canary for the second plugins

1ddd4a4

Signed-off-by: Ingwon Song <igsong@google.com>

Fix lint errors

44e4019

Signed-off-by: Ingwon Song <igsong@google.com>

Add tests for secondary canary test

6a8becc

Signed-off-by: Ingwon Song <igsong@google.com>

Add log checks for checking the root_id is working properly

0527d4b

Signed-off-by: Ingwon Song <igsong@google.com>

Iterate all the combinations of the plugin parameters in tests

c2f3676

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch from b01440d to 0b7141a Compare July 14, 2022 00:06

Change the canary function to be the member function of WasmHandleBase

b361a81

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch from 0b7141a to b361a81 Compare July 14, 2022 00:09

mpwarres reviewed Jul 14, 2022

View reviewed changes

src/wasm.cc Show resolved Hide resolved

src/wasm.cc Show resolved Hide resolved

test/utility.h Outdated Show resolved Hide resolved

Bump up the version of proxy-wasm-cpp-sdk, and fix the CI script

f131a26

Signed-off-by: Ingwon Song <igsong@google.com>

Reduce the constructors and fix lint errors

7089760

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch 2 times, most recently from 2575f73 to a28ee34 Compare July 14, 2022 22:22

mpwarres approved these changes Jul 18, 2022

View reviewed changes

PiotrSikora reviewed Jul 22, 2022

View reviewed changes

Reflect the comments

ff3d16a

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch from 70506c9 to ff3d16a Compare July 22, 2022 21:44

Defend the case that WasmHandleBase::wasm() is null for canary func…

f011993

…tion Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch from 65fea82 to f011993 Compare July 22, 2022 21:53

ingwonsong added 3 commits July 22, 2022 22:15

Add the comments for the reason why we deletes the directories in tes…

7e3af54

…t_data Signed-off-by: Ingwon Song <igsong@google.com>

Add timeout to Bazel, not github script

9006f57

Signed-off-by: Ingwon Song <igsong@google.com>

Keep the wasm handler reference to reuse the wasm vm

d10641e

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch from 7ba877e to d10641e Compare July 23, 2022 23:44

ingwonsong requested a review from PiotrSikora July 24, 2022 03:23

PiotrSikora reviewed Jul 29, 2022

View reviewed changes

ingwonsong added 2 commits July 29, 2022 16:30

Change the prefix "Test" to CanaryCheck

25f6f58

Signed-off-by: Ingwon Song <igsong@google.com>

Reflect the comments

35a0685

Signed-off-by: Ingwon Song <igsong@google.com>

PiotrSikora reviewed Jul 29, 2022

View reviewed changes

test/utility.cc Show resolved Hide resolved

Add the omitted empty line between external and local includes

d7abc4f

Signed-off-by: Ingwon Song <igsong@google.com>

PiotrSikora reviewed Jul 29, 2022

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

Remove trailing white spaces

0bd8418

Signed-off-by: Ingwon Song <igsong@google.com>

ingwonsong force-pushed the canary-second branch from 47d0cab to 0bd8418 Compare July 29, 2022 19:34

ingwonsong mentioned this pull request Jul 29, 2022

wasm: update proxy-wasm to apply canary always envoyproxy/envoy#22469

Merged

PiotrSikora approved these changes Jul 31, 2022

View reviewed changes

PiotrSikora changed the title ~~Configuration Canary for the second or further plugins~~ Configuration canary for the second or further plugins. Jul 31, 2022

PiotrSikora merged commit 4ddbed3 into proxy-wasm:master Jul 31, 2022

ingwonsong deleted the canary-second branch July 31, 2022 15:17

knm3000 pushed a commit to knm3000/proxy-wasm-cpp-host that referenced this pull request Aug 17, 2022

Configuration canary for the second or further plugins. (proxy-wasm#289)

ee63c1b

Fixes proxy-wasm#175. Signed-off-by: Ingwon Song <igsong@google.com>

Configuration canary for the second or further plugins. #289

Configuration canary for the second or further plugins. #289

Uh oh!

Conversation

ingwonsong commented Jun 12, 2022

Uh oh!

ingwonsong commented Jun 16, 2022

Uh oh!

PiotrSikora commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingwonsong commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrSikora commented Jun 16, 2022

Uh oh!

ingwonsong commented Jun 16, 2022

Uh oh!

PiotrSikora commented Jul 13, 2022

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ingwonsong commented Jul 14, 2022

Uh oh!

ingwonsong commented Jul 14, 2022

Uh oh!

mpwarres left a comment

Choose a reason for hiding this comment

Uh oh!

ingwonsong commented Jul 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ingwonsong commented Jul 28, 2022

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

ingwonsong commented Jul 29, 2022

Uh oh!

Uh oh!

Uh oh!

ingwonsong commented Jul 29, 2022

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PiotrSikora commented Jun 16, 2022 •

edited

Loading

ingwonsong commented Jun 16, 2022 •

edited

Loading