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

enable Wasm filter enable per route #26440

Open
ramaraochavali opened this issue Mar 29, 2023 · 20 comments
Open

enable Wasm filter enable per route #26440

ramaraochavali opened this issue Mar 29, 2023 · 20 comments
Labels
area/wasm enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@ramaraochavali
Copy link
Contributor

We have use cases for enabling WASM filters per route. Are there any technical challenges for not supporting this or is it that it has not been implemented because there was not a use case for it?

If it is latter, Can we add support for it?

@ramaraochavali ramaraochavali added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Mar 29, 2023
@lizan lizan added help wanted Needs help! area/wasm and removed triage Issue requires triage labels Mar 29, 2023
@kyessenov
Copy link
Contributor

Can you elaborate? You can use the composite filter, or do it in wasm with route_name.

@ramaraochavali
Copy link
Contributor Author

You can use the composite filter, or do it in wasm with route_name.

Composite filter does not work with WASM + ECDS (with OCI) in Istio as Istio expects the top level filter to be WASM Http Filter during conversion.

do it in wasm with route_name.

It is a possibility. We can check with route_name. But imagine if we want to deploy a WASM at Ingress for few services we ideally do not want to check in WASM but only add this filter for matching virtual hosts.

@kyessenov
Copy link
Contributor

You can use the composite filter, or do it in wasm with route_name.

Composite filter does not work with WASM + ECDS (with OCI) in Istio as Istio expects the top level filter to be WASM Http Filter during conversion.

That can be fixed - Istio hasn't started using composite filter at all.

do it in wasm with route_name.

It is a possibility. We can check with route_name. But imagine if we want to deploy a WASM at Ingress for few services we ideally do not want to check in WASM but only add this filter for matching virtual hosts.

Filters apply per HCM normally, not per host. You could still implement the filtering in Wasm for hosts using resource metadata as a filter.

Are you looking for typed_per_filter_config specifically?

@ramaraochavali
Copy link
Contributor Author

Are you looking for typed_per_filter_config specifically?

Yes

@kyessenov
Copy link
Contributor

kyessenov commented Mar 30, 2023

@ramaraochavali My understanding is that filters need to query for the "most specific" config right now. That doesn't combine well with Wasm model. Each new config spawns a brand new isolate, so they are quite expensive to keep around actually. Isolates with the same config are shared across routes / HCMs. Basically, you need an isolate to run some code to get "the most specific config" but that isolate already has a config.

@ramaraochavali
Copy link
Contributor Author

@kyessenov correct. But if we assume WASM is disabled at the HCM level and only have "typed_per_filter_config" i.e. disabled by default but only enabled on specific routes - then would it help? But still I see the complication of what if there are different configs across routes? Then do we have to spin up that many VMs etc? Is that what your concern is?

@kyessenov
Copy link
Contributor

@ramaraochavali The concern about the number of VMs is about the different "configs" - since a VM has a unique config, you have to have as many VMs as there are configs. I think we could build some generic Wasm HTTP filter way to enable/disable it on routes. Maybe one of the new "filter_config:disabled" options could work.

@ramaraochavali
Copy link
Contributor Author

May be we can do similar to what Lua does already

map<string, config.core.v3.DataSource> source_codes = 2;
?

@ramaraochavali
Copy link
Contributor Author

Another better option might be similar to what we do for stateful session filter. If stateful session filter has empty state it is treated as disabled and at listener level we can have empty filter and per route we can enable with correct config

I think that would work better for WASM also

@kyessenov
Copy link
Contributor

@ramaraochavali The problem is that loading a config is a slow operation (may take seconds fully blocked). That means all per-route configs must be pre-loaded on main, and the Lua version is probably better.

On the other hand, it can be accomplished simpler by chaining multiple Wasm filters and just having the "disabled" bit via #25927.

@ramaraochavali
Copy link
Contributor Author

On the other hand, it can be accomplished simpler by chaining multiple Wasm filters and just having the "disabled" bit via #25927.

Yeah. Typically we need it "enabled" for selected instead of "disabled" for many.

Any case, seems we have options here.

@StarryVae
Copy link
Contributor

Is anyone working on this? and should the route config be just the route plugin configuration? for example:

message RoutePluginConfig {
  google.protobuf.Any configuration = 1;
}

the configuration in RoutePluginConfig is just the route configuration for wasm filter written by user. cc @kyessenov @ramaraochavali

@kyessenov
Copy link
Contributor

This is a question for Wasm ABI. The envoy per-route override semantics with the most specific route config does not map well to proxy-wasm. Unless it's somehow done in ABI, the best we can do is to chain multiple Wasm filters with some predicate in front, which can be accomplished in various ways (e.g. composite filter).

@kyessenov
Copy link
Contributor

cc @mpwarres @PiotrSikora

@StarryVae
Copy link
Contributor

get_configuration ABI is now exported in proxy-wasm, and it is implemented in Envoy

std::string_view Context::getConfiguration() {

could we add the most specific route config here? so we do not have to add a new ABI?

@PiotrSikora
Copy link
Contributor

Each new config spawns a brand new isolate, so they are quite expensive to keep around actually. Isolates with the same config are shared across routes / HCMs. Basically, you need an isolate to run some code to get "the most specific config" but that isolate already has a config.

That's not the case. By default, each plugin spawns a separate WasmVM (assuming the same VM config). Multiple instances of the same plugin with different plugin configs can be executed in the same WasmVM (see: proxy-wasm/proxy-wasm-cpp-host#92).

This is a question for Wasm ABI. The envoy per-route override semantics with the most specific route config does not map well to proxy-wasm. Unless it's somehow done in ABI, the best we can do is to chain multiple Wasm filters with some predicate in front, which can be accomplished in various ways (e.g. composite filter).

I don't follow this logic. Any solution using Proxy-Wasm ABI would need to call into WasmVM to make the ABI call, at which point we might as well perform the check using if proxy_get_header_map_value(':path'), since the context switch has already happened.

IMHO, this should be handled on the proxy side to prevent the context switch to WasmVM in the first place.

At the time when we were integrating Proxy-Wasm in Envoy, the consensus was that per-route filter configs should be done via some generic mechanism in Envoy, and if I recall correctly, composite filters were supposed to be the magic solution for that.

@StarryVae
Copy link
Contributor

In my understanding, maybe it must use Proxy-Wasm ABI to get the route plugin config just like the global plugin config? For different routes, there may be different plugin configs, check using if proxy_get_header_map_value(':path') may not be enough, we still have to expose a ABI to get route plugin config? @PiotrSikora and cc @wbpcode any suggestions?

@PiotrSikora
Copy link
Contributor

Ah, I see what you mean. But that's leaking Envoy-ism into Proxy-Wasm.

IMHO, each route with a custom config should be a separate plugin instance with it's own "global" plugin configuration.

@kyessenov
Copy link
Contributor

IMHO, each route with a custom config should be a separate plugin instance with it's own "global" plugin configuration.

That sounds like what I said: if you want two routes with two different plugin configs, the best to do is to chain them into two instances. Anything better than that would require an ABI change (e.g. you could add a Wasm ABi that calls "use_config(std::string config_key)" based on most specific RDS config prior to all other HTTP callbacks).

@wbpcode
Copy link
Member

wbpcode commented Oct 10, 2023

If only route level flags or supplement config are necessary, may be we can use the route level metadata directly to store the route level filter config and use it in the wasm code.

The main difference of route metadata and typed_per_filter_config (except the new disabled flag) is that the config in the typed_per_filter_config will be parsed and loaded by the createRouteSpecificFilterConfig.

But for the wasm plugin, it's impossible to call createRouteSpecificFilterConfig because the plugin may be updated dynamically. So, metadata is almost same with the typed_per_filter_config for the wasm scenario.

Yeah. Typically we need it "enabled" for selected instead of "disabled" for many.
Any case, seems we have options here.

@ramaraochavali I think you could disable a wasm plugin globally by create a RouteConfigration level disable flag. And enable it in some specific routes.
I think another possible choice would be create a disabled option in the HttpFilter of HCM. But I am not sure the actual value of this option. If you think it's deserved, may be you can create a issue and we can do some more discussion in that new issue.

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

6 participants