wasm: fix multiple bug of vm key and plugin key calculation - #46893
wasm: fix multiple bug of vm key and plugin key calculation#46893wbpcode wants to merge 3 commits into
Conversation
Signed-off-by: wbpcode <wbphub@gmail.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: wbpcode <wbphub@gmail.com>
Signed-off-by: wbpcode <wbphub@gmail.com>
| @@ -0,0 +1,7 @@ | |||
| The identity of a Wasm plugin (which plugin configurations share a single root context and | |||
| thread-local plugin instance inside a Wasm VM) is now derived from the whole | |||
| :ref:`plugin configuration <envoy_v3_api_msg_extensions.wasm.v3.PluginConfig>` instead of from the | |||
There was a problem hiding this comment.
I think instance sharing was intended because the same VM could receive multiple calls for on_configure. Are you sure this is safe to merge?
There was a problem hiding this comment.
Yeah, it's safe. Note, we still shared plugin instance and VM instance if they have exactly same configuration. In our previous implementation, there actually a bug that result in unexpected instance be shared and finally result in unexpected behavior.
For example, in the previous implementation, two plugin configurations with different allow_on_headers_stop_iteration will hit same cache and only the first one's flag will take effects which is wrong.
#46906) Commit Message: istio stats: remove the dependency to the direction of factory context Additional Description: Now, the the istio stats won't depends on the factory context's trafficDirection(), like what we did at the #46893 After all these filters be updated, we could remove the direction() from the FactoryContext and the these filters also could works correctly at cross-listeners shared filter chain (FCDS). Risk Level: low. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode <wbphub@gmail.com>
3 participants
Commit Message: wasm: fix multiple bug of vm key and plugin key calculation
Additional Description:
This PR fixed tow bugs of wasm vm cache key and wasm plugin cache key:
allow_on_headers_stop_iterationmay hit same cached plugin handle and make unexpected behavior.capability_restriction_configandruntimewill be used to calculate the vm cache key. Before this change, the plugin VMs with differentcapability_restriction_configandruntimemay hit same cache and result in unexpected behavior.Now, we also removed the traffic direction from the plugin cache key. It's safe because it's unnecessary now.
Long time ago, before the #31365, we cannot extract the listener info from the connection info, so the WASM will get the traffic direction and listener metadata from the FactoryContext and store the reference at the cached plugin. Then when
get_propertyABI is used, the plugin could serve the related attributes.And to avoid the wasm plugins that be configured in different listeners (with different direction/metadata) hit same plugin cache, we added the listener metadata and traffic direction in to plugin cache key calculation at #17243.
But at the #36619 , we have did a great refactoring and removed the dependencies to the traffic direction and listener metadata from the FactoryContext. The
get_propertyABI will get attributes from the stream info and stream level context. So, after that, it's unnecessary to use the listener metadata and traffic direction as cache key. And at the #36941, we removed the traffic direction and listener metadata from the cached plugin completely. After that, the traffic direction/metadata make nonsense in the cache key.Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.