Pass PluginHandle to stream contexts.

[mathetake]

VM and root contexts (i.e. plugins) must outlive stream contexts, so this commit passes PluginHandle to stream contexts.
Resolves istio/istio#33091

Signed-off-by: Takeshi Yoneda [email protected]

[mathetake]

note: previously ContextBase only holds raw pointers to WasmBase, and this is the root cause (since WasmBase is deleted in response to plugin deletion).

[PiotrSikora]

@mathetake I'm pretty sure that stream context cannot outlive Envoy filter and therefore the plugin itself.

Could you check if you can reproduce the same issue with Envoy v1.17.3 to make sure it's not a regression in different part of Envoy?

[mathetake]

I guess this is something to do with ECDS related lifecycle issue.. but year let me double check later. In anyway, with this and Envyo patches, the original issue has gone.

[PiotrSikora]

Could you check if you can reproduce the same issue with Envoy v1.17.3 to make sure it's not a regression in different part of Envoy?

Actually, nevermind, Istio v1.9.5 is based on Envoy v1.17.x, so the original report uses that version.

[mathetake]

You can actually verify this by inserting assert(wasm_ != nullptr) https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/master/src/context.cc#L316 and elsewhere on the stream context callbacks.

[mathetake]

and wasm_ is sometimes going nullptr after ecds updates.

[PiotrSikora]

I guess this is something to do with ECDS related lifecycle issue.. but year let me double check later. In anyway, with this and Envyo patches, the original issue has gone.

Yeah, that's kind of my point. If the root cause is that extension lifecycle is different between ECDS and LDS, then we're not helping anybody by adding a Wasm-specific workaround for the issue, without fixing the actual problem.

Could you check if you can reproduce this when using LDS instead of ECDS?

At the very least, we should verify with @kyessenov if streams outliving filters is the exepected behavior when using ECDS.

[mathetake]

Yeah, that's kind of my point. If the root cause is that extension lifecycle is different between ECDS and LDS, then we're not helping anybody by adding a Wasm-specific workaround for the issue, without fixing the actual problem.

OK agreed. but this fix itself is not bad to add since "plugins must outlive stream contexts" seems like an Envoy-specific assumption of this library to me.. wdyt?

[PiotrSikora]

OK agreed. but this fix itself is not bad to add since "plugins must outlive stream contexts" seems like an Envoy-specific assumption of this library to me.. wdyt?

That's a valid point. I definitely agree that the lifecycle logic should be fully cointained in this library, but I don't think that's the case right now, and if we want to stop relying on those Envoy assumptions, then we might need a much bigger rewrite.

[mathetake]

At the very least, we should verify with @kyessenov if streams outliving filters is the exepected behavior when using ECDS.

kindly ping @kyessenov

[kyessenov]

Yeah, streams should hold on to filter instances even if filter factories are removed by ECDS. That is how other filters operate and Wasm should be no different. I think we actually tested this exact scenario so not sure how we missed it.

[mathetake]

IIUC it is Wasm http/network filters' factory that holds PluginHandle

• https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/wasm/config.cc#L20-L21
• https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/wasm/wasm_filter.h#L49

so I see from what @kyessenov you mentioned that this is an expected behavior. And we should fix Wasm specificially..

[mathetake]

that is, if factories are deleted on ECDS updates, then Wasm Handles are deleted regardless of existence of outstanding streams(Wasm contexts).

[mathetake]

OK, added test cases for this bug, and passed in envoy envoyproxy/envoy#16795.

[mathetake]

kindly ping @PiotrSikora here and envoyproxy/envoy#16795.

[PiotrSikora]

Thanks, and sorry for the delay!