feat(proxy-wasm) host-managed property getters and setters

[hishamhm]

This feature adds support for host-managed property getter and setter functions.

These feature is implemented in three stages:

• ngx_proxy_wasm_properties.c: in our implementation for proxy-wasm properties, we now allow the host to specify a function which gets called whenever proxy-wasm receives a set_property or get_property call matching the host-defined namespace. Those getter and setter functions are then responsible to return and store the values, and are able to produce any desired side-effects in the host.

• ngx_wasm_lua_ffi.c: We then expose this functionality in our Lua FFI interface. This is a lightweight wrapper, which accepts a C function pointer directly, and associates this function to the request context object.

• lib/resty/wasmx/proxy_wasm.lua: Finally, we expose a Lua API for setting a Lua functions as setters and getters. The interface is such that the host Lua program should set one setter function and/or getter function, and then within that function properly dispatch the behavior based on the received key. Internally, these functions use the fact that LuaJIT is able to convert a Lua function into a C function pointer. Due to lifetime management, those generated C callbacks are a limited resource in the LuaJIT VM, so we produce a single pair of these and then have them call the user-defined Lua function, which remains as a normal Lua closure. (See http://luajit.org/ext_ffi_semantics.html for more details on LuaJIT FFI C callback managment.)

To ensure that the lifetime of strings produced by the host continue valid by the time the getter callback finished running and the Wasm engine reads the properties, getter/setter-produced values continue to be stored in the rbtree for host properties that we already had for plain get_property/set_property, introduced in da82087.

Since we need to store this data on the C side anyway, we also add an extra feature as an optimization: the concept of "const" host-properties. If the Lua callback returns a truthy third-argument, that signals that that value should be flagged as constant. Further calls to get_property during the same request will get the property value directly from the ngx_wasm_module rbtree "cache" without hitting the getter callback again. Test cases in the test suite demonstrate this feature.

In this PR, the host namespace continues to be statically defined at compile time via the NGX_WASM_HOST_PROPERTY_NAMESPACE define. The regular FFI functions for set_property and get_property introduced in 447ef13 also continue to work normally.

[codecov]

Codecov Report

Merging #431 (31262a7) into main (ecd7896) will decrease coverage by 0.00958%.
Report is 3 commits behind head on main.
The diff coverage is 90.24390%.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main        #431         +/-   ##
===================================================
- Coverage   90.18238%   90.17280%   -0.00958%     
===================================================
  Files             46          46                 
  Lines           8444        8507         +63     
===================================================
+ Hits            7615        7671         +56     
- Misses           829         836          +7     

Files	Coverage Δ
src/common/proxy_wasm/ngx_proxy_wasm.h	92.30769% <ø> (ø)
src/common/lua/ngx_wasm_lua_ffi.c	88.17204% <78.94737%> (-2.36850%)	⬇️
src/common/proxy_wasm/ngx_proxy_wasm_properties.c	93.49593% <93.65079%> (-0.04253%)	⬇️

[thibaultcha]

Not able to make a full review at this time but at merge time, for sure the 2nd commit should be squashed, at least.

[hishamhm]

Not able to make a full review at this time but at merge time, for sure the 2nd commit should be squashed, at least.

Yeah, for sure, I only kept it separate for readability during review to help scope what that extra code is for.

[thibaultcha]

Quick review + have not looked at the tests either.

[thibaultcha]

Not easy to review post eye-surgery but I gave it another round

[hishamhm]

@thibaultcha That LuaJIT FFI callback issue is proving to be way more annoying than anticipated. :( I guess I celebrated victory too soon; we had one lucky full run on the DB-less tests, but Postgres-based integration tests keep failing (and in a not fully consistent way, as is typical of code that works or not depending on FFI heuristics): Kong/kong#11856

I'm afraid we'll have to rethink the communication strategy away from the slick-but-unreliable FFI callbacks and into the boring-but-faithful Lua/C API approach used by the Lua bridge. Do we have access to the lua_State running in the OpenResty worker? (That is, if we use the Lua bridge from WasmX, will we have access to Kong Gateway's global state?)

[thibaultcha]

@hishamhm Yes, the WasmX Lua bridge provides access to the OpenResty lua_State with lctx->L, which we get from ngx_http_lua_get_lua_vm.

[hishamhm]

@thibaultcha Reverted back to the original implementation, with the original implementation's reviews squashed into the commit. Should be ready to go! (or another review round!)

[hishamhm]

@thibaultcha I still think we should keep the ngx_int_t changes because they fix the observable sizeof of the return values of these FFI functions to match what's really happening in C... even if most of them are not giving us trouble right now, that might bite us in the future is we keep the discrepancy around.

(As for code aesthetics, I'd be down with moving that string to an unindented top-level thing like

local ffi_http_cdefs = [[
ngx_int_t ngx_http_wasm_ffi_plan_new(ngx_wasm_vm_t *vm,
...
]]

-- ...


if subsystem == "http" then
    ffi.cdef(ffi_http_cdefs)

), possibly matching the look of these definitions from their .h files.

[thibaultcha]

So what I encounter because of this int to ngx_int_t change is that return values (rc) from FFI calls such as C.ngx_http_wasm_ffi_set_host_property are returned as cdata types instead of number, which is quite the undesired side-effect imho, especially given int should already cover all necessary return codes, just like in the core OpenResty FFI APIs. I was just trying to follow some tests by adding print statements to the Lua lib and ended up going down this rabbit hole after a persisting, obscure error in error.log:

fatal runtime error: Rust cannot catch foreign exceptions

[hishamhm]

So what I encounter because of this int to ngx_int_t change is that return values (rc) from FFI calls such as C.ngx_http_wasm_ffi_set_host_property are returned as cdata types instead of number, which is quite the undesired side-effect imho, especially given int should already cover all necessary return codes, just like in the core OpenResty FFI APIs

Ah, I see. I suspect using int here might be working "by accident", just because we're running on little-endian systems.

If we're keeping int in the function definitions because it's harmless in practice, I think we need ngx_int_t only for the definition of the ngx_wasm_host_prop_fn_t function pointer typedef, because that flows the data in the opposite direction (from Lua to C), so LuaJIT compiles that into a C function stub which needs to be 64-bit aware in order to fill the return data with the correct sizeof to be consumed by C code (because otherwise I did see garbage being returned by the callback).

[thibaultcha]

It is not clear to me what this test is supposed to test; the startup seems to be the same as other tests, in fact this test seems functionally identical to TEST 3 - sanity in access_by_lua, except for request_headers_in_access flag which should not make any difference whatsoever? I don't see why this flag would be worth testing with host properties, but maybe am missing something. In my merge PR I marked these tests for getter/setter as SKIP? for now, given I'd rather get rid of them if they are redundant.

[thibaultcha]

@hishamhm I think this is my last item before merging; was there something I missed in this test that isn't present in any other test?

[hishamhm]

You're right, tests 3 and 7 in this file are currently redundant, but the reason why this one originally existed is relevant and possibly should be captured in the test suite, if not with a test of its own, at least with a comment:

I based that test off test 4 of proxy_wasm_set_property_host.t, which is a regression test added when fixing a bug reported by the Gateway, which was an issue when setting properties between attach and start. So I replicated that test to ensure that configuring the property getter/setter also works when done in between attach and start. However, test 3 which is checking the access_by_lua phase is also doing that, which rendered the test case redundant.

So in short, we should test that the various phases, and we should test that setting the getter/setter works before and after proxy_wasm.start. So I'd say we have two options:

• test 3 should be changed so that the getter is set after proxy_wasm.start in the access phase, and then test 7 is renamed to "setting property getter before proxy_wasm.start works"
• test 7 is removed, we assume that if setting the getter works in later phases then it also works on access after proxy_wasm.start, we keep test 3 as is, and then I'd recommend adding an explicit comment in the Lua block saying something like -- ensuring that setting the getter in between attach and start works

[thibaultcha]

Great thanks! Ok so I removed the test since the header_filter_by_lua test case does test this (set_prop after attach/start) while all other tests do attach/set_prop/start). I think this will be merged today.

[thibaultcha]

Merged.

[hishamhm]

@thibaultcha Sweet! I will retarget the .dependencies of the Gateway PR to target main!