feat(proxy-wasm) foreign function support #626

casimiro · 2024-11-18T20:43:08Z

Foreign Function Support

The Proxy-Wasm spec defines the function proxy_call_foreign_function and the callback proxy_on_foreign_function that filter developers can use to invoke host specific functions, a.k.a. foreign functions, and receive callbacks.

A foreign function invoked with proxy_call_foreign_function may return its value immediately -- as part of the returned value of the proxy_call_foreign_function call; or it can return it later, writing it to the FOREIGN_FUNCTION_ARGUMENTS buffer, and invoking proxy_on_foreign_function with an id identifying the function initially called.

This PR adds support for the mechanism described above; and although the spec doesn't restrict when foreign functions can be called, in ngx_wasm_module they cannot be invoked from proxy_on_configure or proxy_on_vm_start.

DNS resolution

This PR also exposes the Lua DNS resolver through the resolve_lua foreign function.

This function expects the name to be resolved as its single argument. If the name can be resolved without performing any IO, the resolved address is put in the returned value of proxy_call_foreign_function.

If the resolver needs to forward the resolution request to a DNS server, the resolved address and the name itself are written to the FOREIGN_FUNCTION_ARGUMENTS buffer and the proxy_on_foreign_function callback is invoked with function_id 0, as soon as the address is returned from the server.

TODO

on_tick support
general review
update docs

codecov · 2024-11-19T22:03:15Z

Codecov Report

Attention: Patch coverage is 82.97872% with 48 lines in your changes missing coverage. Please review.

Project coverage is 90.73090%. Comparing base (d834bb0) to head (d217d1a).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/common/proxy_wasm/ngx_proxy_wasm_foreign_call.c	76.00000%	36 Missing ⚠️
src/common/proxy_wasm/ngx_proxy_wasm_host.c	87.50000%	3 Missing ⚠️
src/http/ngx_http_wasm_util.c	80.00000%	3 Missing ⚠️
src/http/proxy_wasm/ngx_http_proxy_wasm.c	84.21053%	3 Missing ⚠️
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c	93.75000%	2 Missing ⚠️
src/common/proxy_wasm/ngx_proxy_wasm.c	96.15385%	1 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main        #626         +/-   ##
===================================================
- Coverage   90.84288%   90.73090%   -0.11198%     
===================================================
  Files             52          53          +1     
  Lines          11259       11479        +220     
===================================================
+ Hits           10228       10415        +187     
- Misses          1031        1064         +33

Files with missing lines	Coverage Δ
src/common/proxy_wasm/ngx_proxy_wasm.h	`92.30769% <ø> (ø)`
src/common/proxy_wasm/ngx_proxy_wasm_maps.c	`94.01042% <100.00000%> (ø)`
src/common/proxy_wasm/ngx_proxy_wasm_util.c	`94.25287% <100.00000%> (ø)`
src/common/proxy_wasm/ngx_proxy_wasm.c	`92.66451% <96.15385%> (-0.01309%)`	⬇️
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c	`91.74528% <93.75000%> (+0.31671%)`	⬆️
src/common/proxy_wasm/ngx_proxy_wasm_host.c	`93.61233% <87.50000%> (-0.18699%)`	⬇️
src/http/ngx_http_wasm_util.c	`86.95652% <80.00000%> (-0.27752%)`	⬇️
src/http/proxy_wasm/ngx_http_proxy_wasm.c	`92.99065% <84.21053%> (-0.88690%)`	⬇️
...rc/common/proxy_wasm/ngx_proxy_wasm_foreign_call.c	`76.00000% <76.00000%> (ø)`

... and 2 files with indirect coverage changes

Flag	Coverage Δ
unit	`90.67342% <90.36145%> (+0.06802%)`	⬆️
valgrind	`81.06350% <29.41176%> (-1.38648%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

src/common/proxy_wasm/ngx_proxy_wasm.h

src/common/proxy_wasm/ngx_proxy_wasm_foreign_callback.c

src/common/proxy_wasm/ngx_proxy_wasm_host.c

src/http/ngx_http_wasm_util.c

src/common/proxy_wasm/ngx_proxy_wasm_foreign_callback.h

src/common/proxy_wasm/ngx_proxy_wasm_host.c

src/common/proxy_wasm/ngx_proxy_wasm_foreign_call.c

Making struct member names explict to distinguish them from the upcoming Proxy-Wasm foreign call support.

src/common/proxy_wasm/ngx_proxy_wasm_foreign_call.c

casimiro · 2024-11-27T17:43:12Z

Codecov is playing games with me. It's reporting lines that are definitely covered by tests as uncovered.

util/morestyle.pl

config

src/common/proxy_wasm/ngx_proxy_wasm.h

src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c

t/03-proxy_wasm/hfuncs/contexts/140-proxy_foreign_function_resolve_lua.t

thibaultcha · 2024-12-04T02:02:21Z

t/03-proxy_wasm/hfuncs/contexts/140-proxy_foreign_function_resolve_lua.t

+[error]
+can only call resolve_lua during "on_request_headers", "on_request_body", "on_dispatch_response", "on_tick", "on_foreign_function"
+--- no_error_log
+[crit]


Technically, we would have to also add a new test case to all context checks for the new on_foreign_call step... I was going to say we can factorize both those steps together (since they both are "background calls"), but we may want some functions to be disabled in one but not the other...

This makes me think of some cases that we ought to test, for example making a dispatch call during on_foreign_call and vice-versa... This test is also relevant as we factorize both call queues. Maybe we ought to add the new step to the context tests...

Yeah, as I was implementing support for calling lua_resolver from on_http_call_response I noticed that the code also allowed for calling dispatch_http_call from on_foreign_call and that on_foreign_call would be an additional check in the context tests for the hostcalls.

There's a test asserting that filter developers can call the lua_resolver from the on_http_call_response callback; but tests not strictly related to the lua_resolver function (e.g. calling http_call from on_foreign_call and the new on_foreign_call check in the context checks) were left to be addressed in a separate PR as this one is already rather large.

I can include them in this PR. though; if you prefer that way.

I agree that there's value in keeping them separate.

tests not strictly related to the lua_resolver function (e.g. calling http_call from on_foreign_call and the new on_foreign_call check in the context checks) were left to be addressed in a separate PR as this one is already rather large.

Let's add them in this PR, they belong to this feature and that way it will be done. The PR is not very large to me, it's alright.

thibaultcha · 2024-12-04T02:03:36Z

t/lib/proxy-wasm-tests/hostcalls/Cargo.toml

@@ -8,7 +8,8 @@ edition = "2018"
 crate-type = ["cdylib"]

 [dependencies]
-proxy-wasm = "0.2"
+#proxy-wasm = "0.2"
+proxy-wasm = { git = "https://github.com/casimiro/proxy-wasm-rust-sdk.git", branch = "foreign-function-callback" }


Hmmmm, this may be a blocker until it is merged upstream... Actually, getting the feature merged upstream may be a requirement for merging this PR, don't you think?

Yeah, I was hoping the changes in the upstream would be merged by the end of this PR.

On one hand, this workaround seems very subpar and makes me want to consider it a blocker.

On the other, that separate branch contains a single commit that's already been approved by the maintainers and should be part of the next release; and using this separate branch would allow us to ship foreign function support in time for the 3.9 release of the gateway.

src/common/proxy_wasm/ngx_proxy_wasm.h

…aration Handles the case when the expression defining a variable being declared is a function call receiving arguments that don't fit in the same line, e.g.: ngx_proxy_wasm_exec_t *pwexec = ngx_proxy_wasm_instance2pwexec( instance);

The trap message describing the lua_resolver being called in unsupported context exceeds the current limit.

casimiro added the pr/work-in-progress PR: Work in progress label Nov 18, 2024

casimiro force-pushed the feat/wasm-foreign-function branch 5 times, most recently from d4b4267 to 99b8e75 Compare November 19, 2024 21:58

casimiro force-pushed the feat/wasm-foreign-function branch from 99b8e75 to 5bb49f5 Compare November 20, 2024 12:01

casimiro changed the title ~~feat(proxy-wasm) foreign function~~ feat(proxy-wasm) foreign function support Nov 20, 2024

casimiro force-pushed the feat/wasm-foreign-function branch 5 times, most recently from 08ec198 to 2ecb389 Compare November 21, 2024 13:28