Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wasm doesn't support setting filter state #28673

Closed
gitanuj opened this issue Jul 28, 2023 · 7 comments
Closed

Wasm doesn't support setting filter state #28673

gitanuj opened this issue Jul 28, 2023 · 7 comments
Labels
area/wasm enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@gitanuj
Copy link
Contributor

gitanuj commented Jul 28, 2023

Description:
Currently setProperty method in Wasm prefixes all the property keys with wasm.. This makes it impossible to set filter state like envoy.tcp_proxy.cluster which is used by tcp_proxy network filter to dynamically select upstream cluster.

Background:
I'm writing a custom Wasm filter which reads dynamic metadata (emitted by proxy_protocol listener based on TLV headers) and then selects a custom upstream cluster in tcp_proxy by setting filter state. However setting filter state is not possible with Wasm.

Is there any workaround for this?

@gitanuj
Copy link
Contributor Author

gitanuj commented Jul 28, 2023

There's a similar issue for adding support for setting dynamic metadata here

@gitanuj
Copy link
Contributor Author

gitanuj commented Jul 28, 2023

It seems strange that getProperty works for all attributes mentioned here but setProperty doesn't. Is this expected?
@kyessenov I see that you changed the behavior here. Any reason why we always add wasm. prefix whenever using setProperty?

@htuch
Copy link
Member

htuch commented Aug 1, 2023

@mpwarres @lizan

@kyessenov
Copy link
Contributor

There are two reasons for using wasm. prefix.

  1. To protect the internal filter states with untrusted wasm modules. The capability to set a filter state is too broad, and it's potentially dangerous to modify the internal filter state from wasm.

  2. Lack of a generic factory for filter state objects. Each filter state is supposed to use a custom C++ type, but there's no way to instantiate this type given the filter state key alone. There are a couple of issues where we could do better, maybe with some factory registration.

@gitanuj
Copy link
Contributor Author

gitanuj commented Aug 7, 2023

Sounds like there's no generic way to set filter state. Would it make sense to have very specific wasm foreign function to set tcp_proxy filter state? (like set_per_connection_cluster)

This would enable writing wasm filters which just contain some business logic to select upstream cluster while reusing tcp_proxy filter for managing connections. Tcp_proxy filter already supports this dynamic upstream cluster selection but the only way to use it is write a native filter and recompile envoy.

@kyessenov
Copy link
Contributor

I think the missing piece of functionality is #24013. I don't like the idea of hard-coding a specific function for tcp_proxy. It would be more useful for tcp_proxy to register a dynamic handler to create and reflect on its filter state that could be used from any dynamic extension - go, Lua, or Wasm.

@gitanuj
Copy link
Contributor Author

gitanuj commented Aug 9, 2023

That makes sense. However I don't fully understand #24013 (since I'm new to the codebase). Are there any pointers/examples on how to get the dynamic handler working? I'll be happy to contribute this to envoy since we really need this piece of extensibility in wasm.

kyessenov pushed a commit that referenced this issue Sep 6, 2023
Proxy wasm supports setProperty method. However that method always prefixes all property keys with `wasm.`. So currently there's no way to set envoy filter state from wasm which prevents from using it for some scenarios (like chaining a custom network wasm filter with tcp proxy filter and setting `envoy.tcp_proxy.cluster` filter state to select upstream cluster).

This change adds a new wasm foreign function `set_envoy_filter_state` and uses the [object factory to create filter state objects](#29030).

Related Issue: #28673

Commit Message: Add set_envoy_filter_state proxy wasm foreign function
Additional Description: Adds a proxy wasm foreign function to set envoy filter state
Risk Level: low
Testing: done

Signed-off-by: Tanuj Mittal <tmittal@singlestore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/wasm enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

3 participants