From 04016dd8b29006eb4a8a223152b57312c49f0c74 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Sun, 15 Oct 2023 12:42:33 -0300 Subject: [PATCH 01/11] feat(proxy-wasm) host-managed property getters and setters 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 da820875ed7. 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 447ef130a9 also continue to work normally. --- lib/resty/wasmx.lua | 4 + lib/resty/wasmx/proxy_wasm.lua | 198 +++++- src/common/lua/ngx_wasm_lua_ffi.c | 58 ++ src/common/lua/ngx_wasm_lua_ffi.h | 6 + src/common/proxy_wasm/ngx_proxy_wasm.h | 9 +- .../proxy_wasm/ngx_proxy_wasm_properties.c | 132 +++- .../proxy_wasm/ngx_proxy_wasm_properties.h | 6 + .../ffi/304-proxy_wasm_set_property_getter.t | 597 +++++++++++++++++ .../ffi/305-proxy_wasm_set_property_setter.t | 616 ++++++++++++++++++ 9 files changed, 1586 insertions(+), 40 deletions(-) create mode 100644 t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t create mode 100644 t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t diff --git a/lib/resty/wasmx.lua b/lib/resty/wasmx.lua index 9f71f75f2..e61da77c0 100644 --- a/lib/resty/wasmx.lua +++ b/lib/resty/wasmx.lua @@ -15,10 +15,14 @@ ffi.cdef [[ ngx_str_t *config; } ngx_wasm_filter_t; + typedef intptr_t ngx_int_t; typedef unsigned char u_char; typedef struct ngx_wavm_t ngx_wasm_vm_t; typedef struct ngx_wasm_ops_plan_t ngx_wasm_plan_t; + typedef ngx_int_t (*ngx_wasm_host_prop_fn_t)(void *data, ngx_str_t *key, + ngx_str_t *value); + ngx_wasm_vm_t *ngx_wasm_ffi_main_vm(); ]] diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index e44e727bf..7934c4fd6 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -8,9 +8,11 @@ local C = ffi.C local ngx = ngx local error = error local type = type +local pcall = pcall local ffi_gc = ffi.gc local ffi_new = ffi.new local ffi_str = ffi.string +local tostring = tostring local subsystem = ngx.config.subsystem local get_request = wasmx.get_request local get_err_ptr = wasmx.get_err_ptr @@ -38,23 +40,32 @@ local _M = { if subsystem == "http" then ffi.cdef [[ - int ngx_http_wasm_ffi_plan_new(ngx_wasm_vm_t *vm, - ngx_wasm_filter_t *filters, - size_t n_filters, - ngx_wasm_plan_t **out, - u_char *err, size_t *errlen); - int ngx_http_wasm_ffi_plan_free(ngx_wasm_plan_t *plan); - int ngx_http_wasm_ffi_plan_load(ngx_wasm_plan_t *plan); - int ngx_http_wasm_ffi_plan_attach(ngx_http_request_t *r, - ngx_wasm_plan_t *plan, - unsigned int isolation); - int ngx_http_wasm_ffi_start(ngx_http_request_t *r); - int ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *value); - int ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *out); + ngx_int_t ngx_http_wasm_ffi_plan_new(ngx_wasm_vm_t *vm, + ngx_wasm_filter_t *filters, + size_t n_filters, + ngx_wasm_plan_t **out, + u_char *err, size_t *errlen); + ngx_int_t ngx_http_wasm_ffi_plan_free(ngx_wasm_plan_t *plan); + ngx_int_t ngx_http_wasm_ffi_plan_load(ngx_wasm_plan_t *plan); + ngx_int_t ngx_http_wasm_ffi_plan_attach(ngx_http_request_t *r, + ngx_wasm_plan_t *plan, + unsigned int isolation); + ngx_int_t ngx_http_wasm_ffi_start(ngx_http_request_t *r); + ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *value); + ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *out); + ngx_int_t ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *value, + unsigned is_const, + unsigned retrieve); + ngx_int_t ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, + ngx_wasm_host_prop_fn_t fn); + ngx_int_t ngx_http_wasm_ffi_set_property_getter(ngx_http_request_t *r, + ngx_wasm_host_prop_fn_t fn); ]] else @@ -67,6 +78,86 @@ local ffi_ops_type = ffi.typeof("ngx_wasm_plan_t *") local ffi_pops_type = ffi.typeof("ngx_wasm_plan_t *[1]") +local lua_prop_setter +local lua_prop_getter +local c_prop_setter +local c_prop_getter + + +do + -- LuaJIT C callbacks are a limited resource: + -- We can't create one of these on each request, + -- so we prepare these wrappers, which in turn call a Lua function. + + local function store_c_value(r, ckey, cvalue, lvalue, is_const) + if lvalue ~= nil then + local value = tostring(lvalue) + cvalue.data = value + cvalue.len = #value + + else + cvalue.data = nil + cvalue.len = 0 + end + + return C.ngx_http_wasm_ffi_set_host_property(r, ckey, cvalue, + is_const and 1 or 0, + 1) + end + + c_prop_setter = ffi.cast("ngx_wasm_host_prop_fn_t", function(r, ckey, + cvalue) + local lkey = ffi_str(ckey.data, ckey.len) + local lval + if cvalue.data ~= nil then + lval = ffi_str(cvalue.data, cvalue.len) + end + + local pok, ok, lvalue, is_const = pcall(lua_prop_setter, lkey, lval) + if not pok then + ngx.log(ngx.ERR, "error setting property from Lua: ", ok) + return FFI_ERROR + end + + if not ok then + local err = lvalue or "unknown error" + ngx.log(ngx.ERR, "error setting property: ", err) + return FFI_ERROR + end + + return store_c_value(r, ckey, cvalue, lvalue, is_const) + end) + + + c_prop_getter = ffi.cast("ngx_wasm_host_prop_fn_t", function(r, ckey, + cvalue) + local lkey = ffi_str(ckey.data, ckey.len) + + local pok, ok, lvalue, is_const = pcall(lua_prop_getter, lkey) + if not pok then + ngx.log(ngx.ERR, "error getting property from Lua: ", ok) + return FFI_ERROR + end + + if not ok then + if lvalue then + ngx.log(ngx.ERR, "error setting property: ", lvalue) + return FFI_ERROR + end + + return FFI_DECLINED + end + + local rc = store_c_value(r, ckey, cvalue, lvalue, is_const) + if rc == FFI_OK and lvalue == nil then + return FFI_DECLINED + end + + return rc + end) +end + + function _M.new(filters) if type(filters) ~= "table" then error("filters must be a table", 2) @@ -229,7 +320,7 @@ function _M.set_property(key, value) error("key must be a string", 2) end - if type(value) ~= "string" then + if value ~= nil and type(value) ~= "string" then error("value must be a string", 2) end @@ -239,7 +330,7 @@ function _M.set_property(key, value) end local ckey = ffi_new("ngx_str_t", { data = key, len = #key }) - local cvalue = ffi_new("ngx_str_t", { data = value, len = #value }) + local cvalue = ffi_new("ngx_str_t", { data = value, len = value and #value or 0 }) local rc = C.ngx_http_wasm_ffi_set_property(r, ckey, cvalue) if rc == FFI_ERROR then @@ -280,4 +371,73 @@ function _M.get_property(key) end +--- +-- Define a setter function for host-managed properties. +-- +-- @param setter The setter function is the handler for updating properties. +-- Its type signature is `function(string, string): boolean, string`, +-- where the inputs are the property key and value and the results are: +-- * `truthy` - success setting the property +-- * `truthy, value` - success, cache property value (useful if hosts +-- sets only a setter but not a getter) +-- * `truthy, value, truthy` - success, constant property value: further +-- requests to the same property during a request are cached by +-- ngx_wasm_module and do not invoke the Lua getter. +-- * `falsy, err` - failure, and an optional error message +-- @return true on success setting the getter, or nil and an error message. +function _M.set_property_setter(setter) + if type(setter) ~= "function" then + error("setter must be a function", 2) + end + + local r = get_request() + if not r then + error("no request found", 2) + end + + lua_prop_setter = setter + + local rc = C.ngx_http_wasm_ffi_set_property_setter(r, c_prop_setter) + if rc == FFI_OK then + return true + end + + return nil, "could not set property setter" +end + + +--- +-- Define a getter function for host-managed properties. +-- +-- @param getter The getter function is the handler for resolving properties. +-- Its type signature is `function(string): boolean, string, boolean`, +-- where the input is the property key and the results are: +-- * `truthy, value` - success, property value +-- * `truthy, value, truthy` - success, constant property value: further +-- requests to the same property during a request are cached by +-- ngx_wasm_module and do not invoke the Lua getter. +-- * `falsy` - property not found +-- * `falsy, err` - failure, error message +-- @return true on success setting the getter, or nil and an error message. +function _M.set_property_getter(getter) + if type(getter) ~= "function" then + error("getter must be a function", 2) + end + + local r = get_request() + if not r then + error("no request found", 2) + end + + lua_prop_getter = getter + + local rc = C.ngx_http_wasm_ffi_set_property_getter(r, c_prop_getter) + if rc == FFI_OK then + return true + end + + return nil, "could not set property getter" +end + + return _M diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index 0516a39b1..c875c748f 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -180,6 +180,21 @@ ngx_http_wasm_ffi_start(ngx_http_request_t *r) } +static ngx_inline ngx_proxy_wasm_ctx_t * +get_pwctx(ngx_http_request_t *r) +{ + ngx_http_wasm_req_ctx_t *rctx; + + if (ngx_http_wasm_rctx(r, &rctx) != NGX_OK) { + return NULL; + } + + return ngx_proxy_wasm_ctx(NULL, 0, + NGX_PROXY_WASM_ISOLATION_STREAM, + &ngx_http_proxy_wasm, rctx); +} + + ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, ngx_str_t *key, ngx_str_t *value) @@ -226,4 +241,47 @@ ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, return ngx_proxy_wasm_properties_get(pwctx, key, value); } + + +ngx_int_t +ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, + ngx_str_t *key, ngx_str_t *value, unsigned is_const, unsigned retrieve) +{ + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + + if (pwctx == NULL) { + return NGX_ERROR; + } + + return ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, + retrieve); +} + + +ngx_int_t +ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, + ngx_wasm_host_prop_fn_t fn) +{ + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + + if (pwctx == NULL) { + return NGX_ERROR; + } + + return ngx_proxy_wasm_properties_set_host_prop_setter(pwctx, fn, r); +} + + +ngx_int_t +ngx_http_wasm_ffi_set_property_getter(ngx_http_request_t *r, + ngx_wasm_host_prop_fn_t fn) +{ + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + + if (pwctx == NULL) { + return NGX_ERROR; + } + + return ngx_proxy_wasm_properties_set_host_prop_getter(pwctx, fn, r); +} #endif diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 0178058e1..1d87e22d1 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -33,6 +33,12 @@ ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, ngx_str_t *key, ngx_str_t *value); ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, ngx_str_t *key, ngx_str_t *value); +ngx_int_t ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, + ngx_str_t *key, ngx_str_t *value, unsigned is_const, unsigned retrieve); +ngx_int_t ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, + ngx_wasm_host_prop_fn_t fn); +ngx_int_t ngx_http_wasm_ffi_set_property_getter(ngx_http_request_t *r, + ngx_wasm_host_prop_fn_t fn); #endif diff --git a/src/common/proxy_wasm/ngx_proxy_wasm.h b/src/common/proxy_wasm/ngx_proxy_wasm.h index 772508676..4c2877244 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm.h +++ b/src/common/proxy_wasm/ngx_proxy_wasm.h @@ -166,6 +166,9 @@ typedef struct ngx_http_proxy_wasm_dispatch_s ngx_http_proxy_wasm_dispatch_t; typedef ngx_str_t ngx_proxy_wasm_marshalled_map_t; +typedef ngx_int_t (*ngx_wasm_host_prop_fn_t)(void *data, ngx_str_t *key, + ngx_str_t *value); + typedef struct { ngx_queue_t busy; @@ -245,10 +248,14 @@ struct ngx_proxy_wasm_ctx_s { ngx_uint_t call_code; ngx_uint_t response_code; - /* host properties rbtree */ + /* host properties */ ngx_rbtree_t host_props_tree; ngx_rbtree_node_t host_props_sentinel; + ngx_wasm_host_prop_fn_t host_prop_getter; + ngx_wasm_host_prop_fn_t host_prop_setter; + void *host_prop_getter_data; + void *host_prop_setter_data; /* flags */ diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_properties.c b/src/common/proxy_wasm/ngx_proxy_wasm_properties.c index 6af10b3e3..6f865ea59 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm_properties.c +++ b/src/common/proxy_wasm/ngx_proxy_wasm_properties.c @@ -53,6 +53,8 @@ static size_t host_prefix_len; typedef struct { ngx_str_node_t sn; ngx_str_t value; + unsigned is_const:1; + unsigned negative_cache:1; } host_props_node_t; @@ -877,24 +879,33 @@ ngx_proxy_wasm_properties_get_host(ngx_proxy_wasm_ctx_t *pwctx, hpn = (host_props_node_t *) ngx_str_rbtree_lookup(&pwctx->host_props_tree, path, hash); + if (!hpn) { + return NGX_DECLINED; + } - if (hpn) { - value->data = hpn->value.data; - value->len = hpn->value.len; + /* stored value is not const: need to call getter again */ + if (!hpn->is_const && pwctx->host_prop_getter) { + return NGX_DECLINED; + } - return NGX_OK; + value->data = hpn->value.data; + value->len = hpn->value.len; + + if (hpn->negative_cache) { + return NGX_BUSY; } - return NGX_DECLINED; + return NGX_OK; } -static ngx_int_t +ngx_int_t ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, - ngx_str_t *path, ngx_str_t *value) + ngx_str_t *path, ngx_str_t *value, unsigned is_const, unsigned retrieve) { host_props_node_t *hpn; uint32_t hash; + unsigned char *new_data; unsigned new_entry = 1; #ifdef NGX_WASM_HTTP ngx_http_wasm_req_ctx_t *rctx = pwctx->data; @@ -911,16 +922,19 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, hpn = (host_props_node_t *) ngx_str_rbtree_lookup(&pwctx->host_props_tree, path, hash); - if (value->data == NULL) { - if (hpn) { - ngx_rbtree_delete(&pwctx->host_props_tree, &hpn->sn.node); + if (hpn) { + if (hpn->is_const) { + return NGX_DECLINED; } - return NGX_OK; - } - - if (hpn) { ngx_pfree(pwctx->pool, hpn->value.data); + + if (value->data == NULL && !is_const) { + ngx_rbtree_delete(&pwctx->host_props_tree, &hpn->sn.node); + + return NGX_OK; + } + new_entry = 0; } else { @@ -935,16 +949,29 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, ngx_memcpy(hpn->sn.str.data, path->data, path->len); } - hpn->value.len = value->len; - hpn->value.data = ngx_pstrdup(pwctx->pool, value); - if (hpn->value.data == NULL) { - return NGX_ERROR; + if (value->data == NULL) { + new_data = NULL; + hpn->negative_cache = 1; + + } else { + new_data = ngx_pstrdup(pwctx->pool, value); + if (new_data == NULL) { + return NGX_ERROR; + } } + hpn->is_const = is_const; + hpn->value.len = value->len; + hpn->value.data = new_data; + if (new_entry) { ngx_rbtree_insert(&pwctx->host_props_tree, &hpn->sn.node); } + if (retrieve) { + value->data = hpn->value.data; + } + return NGX_OK; } @@ -970,6 +997,7 @@ ngx_proxy_wasm_properties_get(ngx_proxy_wasm_ctx_t *pwctx, ngx_str_t p = { path->len, NULL }; ngx_uint_t key; pwm2ngx_mapping_t *m; + ngx_int_t rc; p.data = replace_nulls_by_dots(path, dotted_path_buf); key = ngx_hash_key(p.data, p.len); @@ -997,7 +1025,22 @@ ngx_proxy_wasm_properties_get(ngx_proxy_wasm_ctx_t *pwctx, && ngx_memcmp(p.data, host_prefix, host_prefix_len) == 0) { /* host variable */ - return ngx_proxy_wasm_properties_get_host(pwctx, &p, value); + + /* even if there is a getter, try reading const value first */ + rc = ngx_proxy_wasm_properties_get_host(pwctx, &p, value); + if (rc == NGX_OK) { + return NGX_OK; + + } else if (rc == NGX_BUSY) { + return NGX_DECLINED; + } + + if (pwctx->host_prop_getter) { + return pwctx->host_prop_getter(pwctx->host_prop_getter_data, + &p, value); + } + + return rc; } return NGX_DECLINED; @@ -1033,8 +1076,57 @@ ngx_proxy_wasm_properties_set(ngx_proxy_wasm_ctx_t *pwctx, && ngx_memcmp(p.data, host_prefix, host_prefix_len) == 0) { /* host variable */ - return ngx_proxy_wasm_properties_set_host(pwctx, &p, value); + if (pwctx->host_prop_setter) { + return pwctx->host_prop_setter(pwctx->host_prop_setter_data, + &p, value); + } + + return ngx_proxy_wasm_properties_set_host(pwctx, &p, value, 0, 0); } return NGX_DECLINED; } + + +ngx_int_t +ngx_proxy_wasm_properties_set_host_prop_setter(ngx_proxy_wasm_ctx_t *pwctx, + ngx_wasm_host_prop_fn_t fn, void *data) +{ +#ifdef NGX_WASM_HTTP + ngx_http_wasm_req_ctx_t *rctx = pwctx->data; + + if (rctx == NULL || rctx->fake_request) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "cannot set host properties setter " + "outside of a request"); + return NGX_ERROR; + } +#endif + + pwctx->host_prop_setter = fn; + pwctx->host_prop_setter_data = data; + + return NGX_OK; +} + + +ngx_int_t +ngx_proxy_wasm_properties_set_host_prop_getter(ngx_proxy_wasm_ctx_t *pwctx, + ngx_wasm_host_prop_fn_t fn, void *data) +{ +#ifdef NGX_WASM_HTTP + ngx_http_wasm_req_ctx_t *rctx = pwctx->data; + + if (rctx == NULL || rctx->fake_request) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "cannot set host properties getter " + "outside of a request"); + return NGX_ERROR; + } +#endif + + pwctx->host_prop_getter = fn; + pwctx->host_prop_getter_data = data; + + return NGX_OK; +} diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_properties.h b/src/common/proxy_wasm/ngx_proxy_wasm_properties.h index e39438cfa..9843fd229 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm_properties.h +++ b/src/common/proxy_wasm/ngx_proxy_wasm_properties.h @@ -9,6 +9,12 @@ ngx_int_t ngx_proxy_wasm_properties_get(ngx_proxy_wasm_ctx_t *pwctx, ngx_str_t *path, ngx_str_t *value); ngx_int_t ngx_proxy_wasm_properties_set(ngx_proxy_wasm_ctx_t *pwctx, ngx_str_t *path, ngx_str_t *value); +ngx_int_t ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, + ngx_str_t *path, ngx_str_t *value, unsigned is_const, unsigned retrieve); +ngx_int_t ngx_proxy_wasm_properties_set_host_prop_setter( + ngx_proxy_wasm_ctx_t *pwctx, ngx_wasm_host_prop_fn_t fn, void *data); +ngx_int_t ngx_proxy_wasm_properties_set_host_prop_getter( + ngx_proxy_wasm_ctx_t *pwctx, ngx_wasm_host_prop_fn_t fn, void *data); #endif /* _NGX_PROXY_WASM_PROPERTIES_H_INCLUDED_ */ diff --git a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t new file mode 100644 index 000000000..e6c8f764f --- /dev/null +++ b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t @@ -0,0 +1,597 @@ +# vim:set ft= ts=4 sw=4 et fdm=marker: + +use strict; +use lib '.'; +use t::TestWasm::Lua; + +skip_no_openresty(); + +plan tests => repeat_each() * (blocks() * 5); + +run_tests(); + +__DATA__ + +=== TEST 1: set_property_getter() - setting host properties getter fails on init_worker_by_lua +--- wasm_modules: on_phases +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "on_phases" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + + local function getter(key) + if key == "my_key" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + } +--- config + location /t { + rewrite_by_lua_block { + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[error\] .*? cannot set host properties getter outside of a request/, + qr/\[error\] .*? init_worker_by_lua(\(nginx\.conf:\d+\))?:\d+: could not set property getter/, +] +--- no_error_log +[crit] + + + +=== TEST 2: set_property_getter() - setting host properties getter works on rewrite_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: my_value/, +] +--- no_error_log +[error] + + + +=== TEST 3: set_property_getter() - setting host properties getter works on access_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: my_value/, +] +--- no_error_log +[error] + + + +=== TEST 4: set_property_getter() - setting host properties getter works on header_filter_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + header_filter_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: my_value/, +] +--- no_error_log +[error] + + + +=== TEST 5: set_property_getter() - setting host properties getter works on body_filter_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + body_filter_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: my_value/, +] +--- no_error_log +[error] + + + +=== TEST 6: set_property_getter() - setting host properties getter works on log_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm_request_headers_in_access on; + + proxy_wasm hostcalls 'on=log \ + test=/t/log/property \ + name=wasmx.my_property'; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + log_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_log/, + qr/\[info\] .*? wasmx.my_property: my_value/, +] +--- no_error_log +[error] + + + +=== TEST 7: set_property_getter() - setting property getter at startup doesn't reset filter list +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls", config = "on=log " .. + "test=/t/log/property " .. + "name=wasmx.startup_property" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm_request_headers_in_access on; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + assert(proxy_wasm.attach(_G.c_plan)) + + local function getter(key) + if key == "wasmx.startup_property" then + return true, "foo" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_log/, + qr/\[info\] .*?hostcalls.*? wasmx.startup_property: foo/, +] +--- no_error_log +[error] + + + +=== TEST 8: set_property_getter() - getter returning false produces property not found +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.missing_property'; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + return false + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? property not found: wasmx.missing_property/, +] +--- no_error_log +[error] + + + +=== TEST 9: set_property_getter() - getter returning nil produces property not found +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.missing_property'; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local function getter(key) + if key == "wasmx.my_property" then + return true, "my_value" + end + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? property not found: wasmx.missing_property/, +] +--- no_error_log +[error] + + + +=== TEST 10: set_property_getter() - getter returning true on 3rd value caches result +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local counter = 0 + + local function getter(key) + counter = counter + 1 + + ngx.log(ngx.INFO, "getting ", key, ", counter: ", + tostring(counter)) + + if key == "wasmx.dyn_property" then + return true, tostring(counter) + + elseif key == "wasmx.const_property" then + return true, tostring(counter), true + + elseif key == "wasmx.nil_dyn_property" then + return true, nil + + elseif key == "wasmx.nil_const_property" then + return true, nil, true + + elseif key == "wasmx.empty_dyn_property" then + return true, "" + + elseif key == "wasmx.empty_const_property" then + return true, "", true + end + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + log_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local function log_property(k) + local v = proxy_wasm.get_property(k) + ngx.log(ngx.INFO, k, ": ", type(v), " \"", v, "\"") + end + + -- four get_property operations, + -- but it will hit the getter three times + -- because the second call to get wasmx.const_property + -- hits the cache + log_property("wasmx.const_property") -- returns 1 + log_property("wasmx.dyn_property") -- returns 2 + log_property("wasmx.const_property") -- returns 1 + log_property("wasmx.dyn_property") -- returns 3 + + log_property("wasmx.nil_const_property") -- returns nil + log_property("wasmx.nil_dyn_property") -- returns nil + log_property("wasmx.nil_const_property") -- returns nil + log_property("wasmx.nil_dyn_property") -- returns nil + + log_property("wasmx.empty_const_property") -- returns "" + log_property("wasmx.empty_dyn_property") -- returns "" + log_property("wasmx.empty_const_property") -- returns "" + log_property("wasmx.empty_dyn_property") -- returns "" + } + } +--- response_body +ok +--- grep_error_log eval: qr/.*wasmx.*property.*/ +--- grep_error_log_out eval +qr/^[^\n]*?\[info\] [^\n]*? getting wasmx.const_property, counter: 1[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.const_property: string "1"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.dyn_property, counter: 2[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.dyn_property: string "2"[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.const_property: string "1"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.dyn_property, counter: 3[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.dyn_property: string "3"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.nil_const_property, counter: 4[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_const_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.nil_dyn_property, counter: 5[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_dyn_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_const_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.nil_dyn_property, counter: 6[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_dyn_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.empty_const_property, counter: 7[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_const_property: string ""[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.empty_dyn_property, counter: 8[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_dyn_property: string ""[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_const_property: string ""[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.empty_dyn_property, counter: 9[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_dyn_property: string ""[^\n]*?$/ +--- no_error_log +[error] +[crit] + + + +=== TEST 11: set_property_getter() - errors in getter are caught by pcall in Lua library and don't crash the worker +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local function getter(key) + error("crash!") + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[error\] .*? error getting property from Lua: rewrite_by_lua\(nginx.conf:[0-9]+\):[0-9]+: crash\!/, +] +--- no_error_log +[emerg] diff --git a/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t b/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t new file mode 100644 index 000000000..471385d3d --- /dev/null +++ b/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t @@ -0,0 +1,616 @@ +# vim:set ft= ts=4 sw=4 et fdm=marker: + +use strict; +use lib '.'; +use t::TestWasm::Lua; + +skip_no_openresty(); + +plan tests => repeat_each() * (blocks() * 5); + +run_tests(); + +__DATA__ + +=== TEST 1: set_property_setter() - setting host properties setter fails on init_worker_by_lua +--- wasm_modules: on_phases +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "on_phases" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + + local tbl = {} + + local function setter(key, value) + tbl[key] = value + return true + end + + assert(proxy_wasm.set_property_setter(setter)) + } +--- config + location /t { + rewrite_by_lua_block { + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[error\] .*? cannot set host properties setter outside of a request/, + qr/\[error\] .*? init_worker_by_lua(\(nginx\.conf:\d+\))?:\d+: could not set property setter/, +] +--- no_error_log +[crit] + + + +=== TEST 2: set_property_setter() - setting host properties setter works on rewrite_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local tbl = {} + + local function setter(key, value) + local new_value = value:upper() + tbl[key] = new_value + return true, new_value + end + + assert(proxy_wasm.set_property_setter(setter)) + + assert(proxy_wasm.set_property("wasmx.my_property", "my_value")) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: MY_VALUE/, +] +--- no_error_log +[error] + + + +=== TEST 3: set_property_setter() - setting host properties setter works on access_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local tbl = {} + + local function setter(key, value) + local new_value = value:upper() + tbl[key] = new_value + return true, new_value + end + + assert(proxy_wasm.set_property_setter(setter)) + + assert(proxy_wasm.set_property("wasmx.my_property", "my_value")) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: MY_VALUE/, +] +--- no_error_log +[error] + + + +=== TEST 4: set_property_setter() - setting host properties setter works on header_filter_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + header_filter_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local tbl = {} + + local function setter(key, value) + local new_value = value:upper() + tbl[key] = new_value + return true, new_value + end + + assert(proxy_wasm.set_property_setter(setter)) + + assert(proxy_wasm.set_property("wasmx.my_property", "my_value")) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: MY_VALUE/, +] +--- no_error_log +[error] + + + +=== TEST 5: set_property_setter() - setting host properties setter works on body_filter_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + body_filter_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local tbl = {} + + local function setter(key, value) + local new_value = value:upper() + tbl[key] = new_value + return true, new_value + end + + assert(proxy_wasm.set_property_setter(setter)) + + assert(proxy_wasm.set_property("wasmx.my_property", "my_value")) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[info\] .*? wasmx.my_property: MY_VALUE/, +] +--- no_error_log +[error] + + + +=== TEST 6: set_property_setter() - setting host properties setter works on log_by_lua +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm_request_headers_in_access on; + + proxy_wasm hostcalls 'on=log \ + test=/t/log/property \ + name=wasmx.my_property'; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + log_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local tbl = {} + + local function setter(key, value) + local new_value = value:upper() + tbl[key] = new_value + return true, new_value + end + + assert(proxy_wasm.set_property_setter(setter)) + + assert(proxy_wasm.set_property("wasmx.my_property", "my_value")) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_log/, + qr/\[info\] .*? wasmx.my_property: MY_VALUE/, +] +--- no_error_log +[error] + + + +=== TEST 7: set_property_setter() - setting property setter at startup doesn't reset filter list +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls", config = "on=log " .. + "test=/t/log/property " .. + "name=wasmx.my_property" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm_request_headers_in_access on; + + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + assert(proxy_wasm.attach(_G.c_plan)) + + local function setter(key, value) + if key == "wasmx.my_property" then + return true, "foo" + end + return false + end + + assert(proxy_wasm.set_property_setter(setter)) + + assert(proxy_wasm.start()) + + ngx.say("ok") + } + + body_filter_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + assert(proxy_wasm.set_property("wasmx.my_property", "bar")) + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_log/, + qr/\[info\] .*?hostcalls.*? wasmx.my_property: foo/, +] +--- no_error_log +[error] + + + +=== TEST 8: set_property_setter() - setter returning falsy logs custom message or "unknown error" +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + proxy_wasm hostcalls 'on=log \ + test=/t/log/property \ + name=wasmx.fail1_property'; + + location /t { + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local function setter(key) + if key == "wasmx.fail1_property" then + return nil, "first wasmx property custom error" + + elseif key == "wasmx.fail2_property" then + return false, "second wasmx property custom error" + + elseif key == "wasmx.fail3_property" then + return nil + + elseif key == "wasmx.fail4_property" then + return false + end + end + + assert(proxy_wasm.set_property_setter(setter)) + + local msg = "result:" + for i = 1, 4 do + local key = "wasmx.fail" .. i .. "_property" + local ok, err = proxy_wasm.set_property(key, "wat") + if not ok then + msg = msg .. "\n" .. err + end + end + + assert(proxy_wasm.start()) + ngx.say(msg) + } + } +--- response_body +result: +unknown error +unknown error +unknown error +unknown error +--- grep_error_log eval: qr/.*property.*/ +--- grep_error_log_out eval +qr/^[^\n]*?\[error\] [^\n]*? error setting property: first wasmx property custom error[^\n]*? +[^\n]*?\[error\] [^\n]*? error setting property: second wasmx property custom error[^\n]*? +[^\n]*?\[error\] [^\n]*? error setting property: unknown error[^\n]*? +[^\n]*?\[error\] [^\n]*? error setting property: unknown error[^\n]*? +[^\n]*?\[info\] [^\n]*? property not found: wasmx.fail1_property[^\n]*?$/ +--- no_error_log +[crit] +[alert] + + + +=== TEST 9: set_property_setter() - setter returning true on 3rd value caches result +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + access_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local counter = 0 + + local tbl = {} + + local function setter(key, value) + counter = counter + 1 + + if value and #value > 0 then + value = value:upper() .. " " .. tostring(counter) + end + + tbl[key] = value + + ngx.log(ngx.INFO, "setting ", key, " to ", value, ", counter: " .. counter) + + return true, value, (key:match("const") and true or false) + end + + assert(proxy_wasm.set_property_setter(setter)) + + local function getter(key) + counter = counter + 1 + + ngx.log(ngx.INFO, "getting ", key, " via getter, counter: ", counter) + + local value = tbl[key] + + if value and #value > 0 then + value = value .. " " .. tostring(counter) + end + + return true, value + end + + assert(proxy_wasm.set_property_getter(getter)) + + assert(proxy_wasm.start()) + ngx.say("ok") + } + + log_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + + local function log_property(k) + local v = proxy_wasm.get_property(k) + ngx.log(ngx.INFO, k, ": ", type(v), " \"", v, "\"") + end + + -- two set_property operations, each will hit the setter. + -- four get_property operations, + -- but only wasmx.dyn_property will hit the getter + -- because the setter to wasmx.const_property cached the result. + proxy_wasm.set_property("wasmx.const_property", "hello") -- sets "HELLO 1" + log_property("wasmx.const_property") -- returns "HELLO 1" + proxy_wasm.set_property("wasmx.dyn_property", "hi") -- sets "HI 2" + log_property("wasmx.dyn_property") -- returns "HI 2 3" + log_property("wasmx.const_property") -- returns "HELLO 1" + log_property("wasmx.dyn_property") -- returns "HI 2 4" + + proxy_wasm.set_property("wasmx.nil_const_property", nil) -- sets nil + log_property("wasmx.nil_const_property") -- returns nil + proxy_wasm.set_property("wasmx.nil_dyn_property", nil) -- sets nil + log_property("wasmx.nil_dyn_property") -- returns nil + log_property("wasmx.nil_const_property") -- returns nil + log_property("wasmx.nil_dyn_property") -- returns nil + + proxy_wasm.set_property("wasmx.empty_const_property", "") -- sets "" + log_property("wasmx.empty_const_property") -- returns "" + proxy_wasm.set_property("wasmx.empty_dyn_property", "") -- sets "" + log_property("wasmx.empty_dyn_property") -- returns "" + log_property("wasmx.empty_const_property") -- returns "" + log_property("wasmx.empty_dyn_property") -- returns "" + } + } +--- response_body +ok +--- grep_error_log eval: qr/.*wasmx.*property.*/ +--- grep_error_log_out eval +qr/^[^\n]*?\[info\] [^\n]*? setting wasmx.const_property to HELLO 1, counter: 1[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.const_property: string "HELLO 1"[^\n]*? +[^\n]*?\[info\] [^\n]*? setting wasmx.dyn_property to HI 2, counter: 2[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.dyn_property via getter, counter: 3[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.dyn_property: string "HI 2 3"[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.const_property: string "HELLO 1"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.dyn_property via getter, counter: 4[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.dyn_property: string "HI 2 4"[^\n]*? +[^\n]*?\[info\] [^\n]*? setting wasmx.nil_const_property to nil, counter: 5[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_const_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? setting wasmx.nil_dyn_property to nil, counter: 6[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.nil_dyn_property via getter, counter: 7[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_dyn_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_const_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.nil_dyn_property via getter, counter: 8[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.nil_dyn_property: nil "nil"[^\n]*? +[^\n]*?\[info\] [^\n]*? setting wasmx.empty_const_property to , counter: 9[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_const_property: string ""[^\n]*? +[^\n]*?\[info\] [^\n]*? setting wasmx.empty_dyn_property to , counter: 10[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.empty_dyn_property via getter, counter: 11[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_dyn_property: string ""[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_const_property: string ""[^\n]*? +[^\n]*?\[info\] [^\n]*? getting wasmx.empty_dyn_property via getter, counter: 12[^\n]*? +[^\n]*?\[info\] [^\n]*? wasmx.empty_dyn_property: string ""[^\n]*?$/ +--- no_error_log +[error] +[crit] + + + +=== TEST 10: set_property_setter() - errors in setter are caught by pcall in Lua library and don't crash the worker +--- wasm_modules: hostcalls +--- http_config + init_worker_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + local filters = { + { name = "hostcalls" }, + } + + _G.c_plan = assert(proxy_wasm.new(filters)) + assert(proxy_wasm.load(_G.c_plan)) + } +--- config + location /t { + proxy_wasm hostcalls 'on=response_body \ + test=/t/log/property \ + name=wasmx.my_property'; + + rewrite_by_lua_block { + local proxy_wasm = require "resty.wasmx.proxy_wasm" + assert(proxy_wasm.attach(_G.c_plan)) + + local tbl = {} + + local function setter(key, value) + error("crash!") + end + + assert(proxy_wasm.set_property_setter(setter)) + + local ok, err = proxy_wasm.set_property("wasmx.my_property", "my_value") + assert((not ok) and err == "unknown error") + + assert(proxy_wasm.start()) + ngx.say("ok") + } + } +--- response_body +ok +--- error_log eval +[ + qr/\[info\] .*? \[hostcalls\] on_response_body/, + qr/\[error\] .*? error setting property from Lua: rewrite_by_lua\(nginx.conf:[0-9]+\):[0-9]+: crash\!/, +] +--- no_error_log +[emerg] From 26e91bdcd739161286529514a3f471dc922f60ed Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Tue, 24 Oct 2023 16:46:25 -0300 Subject: [PATCH 02/11] code review --- src/common/lua/ngx_wasm_lua_ffi.c | 4 +- .../proxy_wasm/ngx_proxy_wasm_properties.c | 55 +++++++++++++------ 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index c875c748f..aaa5b54af 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -190,8 +190,8 @@ get_pwctx(ngx_http_request_t *r) } return ngx_proxy_wasm_ctx(NULL, 0, - NGX_PROXY_WASM_ISOLATION_STREAM, - &ngx_http_proxy_wasm, rctx); + NGX_PROXY_WASM_ISOLATION_STREAM, + &ngx_http_proxy_wasm, rctx); } diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_properties.c b/src/common/proxy_wasm/ngx_proxy_wasm_properties.c index 6f865ea59..74467e06b 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm_properties.c +++ b/src/common/proxy_wasm/ngx_proxy_wasm_properties.c @@ -899,14 +899,38 @@ ngx_proxy_wasm_properties_get_host(ngx_proxy_wasm_ctx_t *pwctx, } +static ngx_inline ngx_int_t +set_hpn_value(host_props_node_t *hpn, ngx_str_t *value, unsigned is_const, + ngx_pool_t *pool) +{ + unsigned char *new_data; + + if (value->data == NULL) { + new_data = NULL; + hpn->negative_cache = 1; + + } else { + new_data = ngx_pstrdup(pool, value); + if (new_data == NULL) { + return NGX_ERROR; + } + } + + hpn->is_const = is_const; + hpn->value.len = value->len; + hpn->value.data = new_data; + + return NGX_OK; +} + + ngx_int_t ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, ngx_str_t *path, ngx_str_t *value, unsigned is_const, unsigned retrieve) { host_props_node_t *hpn; uint32_t hash; - unsigned char *new_data; - unsigned new_entry = 1; + ngx_int_t rc; #ifdef NGX_WASM_HTTP ngx_http_wasm_req_ctx_t *rctx = pwctx->data; @@ -935,7 +959,14 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, return NGX_OK; } - new_entry = 0; + rc = set_hpn_value(hpn, value, is_const, pwctx->pool); + if (rc != NGX_OK) { + return rc; + } + + } else if (value->data == NULL && !is_const) { + /* no need to store a non-const NULL */ + return NGX_OK; } else { hpn = ngx_pcalloc(pwctx->pool, sizeof(host_props_node_t) + path->len); @@ -947,24 +978,12 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, hpn->sn.str.len = path->len; hpn->sn.str.data = (u_char *) hpn + sizeof(host_props_node_t); ngx_memcpy(hpn->sn.str.data, path->data, path->len); - } - - if (value->data == NULL) { - new_data = NULL; - hpn->negative_cache = 1; - } else { - new_data = ngx_pstrdup(pwctx->pool, value); - if (new_data == NULL) { - return NGX_ERROR; + rc = set_hpn_value(hpn, value, is_const, pwctx->pool); + if (rc != NGX_OK) { + return rc; } - } - - hpn->is_const = is_const; - hpn->value.len = value->len; - hpn->value.data = new_data; - if (new_entry) { ngx_rbtree_insert(&pwctx->host_props_tree, &hpn->sn.node); } From 669084d1d042f778c4de217ee8335ffd4a8b0277 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Thu, 26 Oct 2023 13:40:37 -0300 Subject: [PATCH 03/11] fix(lua) avoid LuaJIT issue with FFI callback roundtrips The Kong Gateway CI started seeing intermittent crashes with "bad callback" errors: See: https://luajit.freelists.narkive.com/sdhSLJSr/how-to-make-bad-callback-more-deterministic ``` The bad callback error happens because some JIT-compiled Lua code calls a C function which in turn calls an FFI callback. ``` https://lua-l.lua.narkive.com/qXJrNlpP/luajit-ffi-windows-bad-callback-error-in-msgwaitformultipleobjects-proof-of-concept From Mike Pall: ``` The problem is that a FFI callback cannot safely be called from a C function which is itself called via the FFI from JIT-compiled code. In your case this is the call to MsgWaitForMultipleObjects. I've put in a lot of heuristics to detect this, and it usually succeeds in disabling compilation for such a function. However in your case the loop is compiled before the callback is ever called, so the detection fails. The straighforward solution is to put the message loop into an extra Lua function and use jit.off(func) ``` See --- lib/resty/wasmx/proxy_wasm.lua | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index 7934c4fd6..ebbf8037c 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -113,7 +113,9 @@ do lval = ffi_str(cvalue.data, cvalue.len) end + jit.off() local pok, ok, lvalue, is_const = pcall(lua_prop_setter, lkey, lval) + jit.on() if not pok then ngx.log(ngx.ERR, "error setting property from Lua: ", ok) return FFI_ERROR @@ -133,7 +135,9 @@ do cvalue) local lkey = ffi_str(ckey.data, ckey.len) + jit.off() local pok, ok, lvalue, is_const = pcall(lua_prop_getter, lkey) + jit.on() if not pok then ngx.log(ngx.ERR, "error getting property from Lua: ", ok) return FFI_ERROR From a59bb7e807603c81d0e08da19fd1036b26c2720e Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Thu, 26 Oct 2023 17:01:21 -0300 Subject: [PATCH 04/11] code review --- lib/resty/wasmx/proxy_wasm.lua | 290 +++++++++--------- .../proxy_wasm/ngx_proxy_wasm_properties.c | 45 +-- 2 files changed, 179 insertions(+), 156 deletions(-) diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index ebbf8037c..095607d4e 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -78,90 +78,6 @@ local ffi_ops_type = ffi.typeof("ngx_wasm_plan_t *") local ffi_pops_type = ffi.typeof("ngx_wasm_plan_t *[1]") -local lua_prop_setter -local lua_prop_getter -local c_prop_setter -local c_prop_getter - - -do - -- LuaJIT C callbacks are a limited resource: - -- We can't create one of these on each request, - -- so we prepare these wrappers, which in turn call a Lua function. - - local function store_c_value(r, ckey, cvalue, lvalue, is_const) - if lvalue ~= nil then - local value = tostring(lvalue) - cvalue.data = value - cvalue.len = #value - - else - cvalue.data = nil - cvalue.len = 0 - end - - return C.ngx_http_wasm_ffi_set_host_property(r, ckey, cvalue, - is_const and 1 or 0, - 1) - end - - c_prop_setter = ffi.cast("ngx_wasm_host_prop_fn_t", function(r, ckey, - cvalue) - local lkey = ffi_str(ckey.data, ckey.len) - local lval - if cvalue.data ~= nil then - lval = ffi_str(cvalue.data, cvalue.len) - end - - jit.off() - local pok, ok, lvalue, is_const = pcall(lua_prop_setter, lkey, lval) - jit.on() - if not pok then - ngx.log(ngx.ERR, "error setting property from Lua: ", ok) - return FFI_ERROR - end - - if not ok then - local err = lvalue or "unknown error" - ngx.log(ngx.ERR, "error setting property: ", err) - return FFI_ERROR - end - - return store_c_value(r, ckey, cvalue, lvalue, is_const) - end) - - - c_prop_getter = ffi.cast("ngx_wasm_host_prop_fn_t", function(r, ckey, - cvalue) - local lkey = ffi_str(ckey.data, ckey.len) - - jit.off() - local pok, ok, lvalue, is_const = pcall(lua_prop_getter, lkey) - jit.on() - if not pok then - ngx.log(ngx.ERR, "error getting property from Lua: ", ok) - return FFI_ERROR - end - - if not ok then - if lvalue then - ngx.log(ngx.ERR, "error setting property: ", lvalue) - return FFI_ERROR - end - - return FFI_DECLINED - end - - local rc = store_c_value(r, ckey, cvalue, lvalue, is_const) - if rc == FFI_OK and lvalue == nil then - return FFI_DECLINED - end - - return rc - end) -end - - function _M.new(filters) if type(filters) ~= "table" then error("filters must be a table", 2) @@ -333,8 +249,14 @@ function _M.set_property(key, value) error("no request found", 2) end - local ckey = ffi_new("ngx_str_t", { data = key, len = #key }) - local cvalue = ffi_new("ngx_str_t", { data = value, len = value and #value or 0 }) + local ckey = ffi_new("ngx_str_t", { + data = key, + len = #key + }) + local cvalue = ffi_new("ngx_str_t", { + data = value, + len = value and #value or 0 + }) local rc = C.ngx_http_wasm_ffi_set_property(r, ckey, cvalue) if rc == FFI_ERROR then @@ -375,72 +297,166 @@ function _M.get_property(key) end ---- --- Define a setter function for host-managed properties. --- --- @param setter The setter function is the handler for updating properties. --- Its type signature is `function(string, string): boolean, string`, --- where the inputs are the property key and value and the results are: --- * `truthy` - success setting the property --- * `truthy, value` - success, cache property value (useful if hosts --- sets only a setter but not a getter) --- * `truthy, value, truthy` - success, constant property value: further --- requests to the same property during a request are cached by --- ngx_wasm_module and do not invoke the Lua getter. --- * `falsy, err` - failure, and an optional error message --- @return true on success setting the getter, or nil and an error message. -function _M.set_property_setter(setter) - if type(setter) ~= "function" then - error("setter must be a function", 2) +do + -- LuaJIT C callbacks are a limited resource: + -- We can't create one of these on each request, + -- so we prepare these wrappers, which in turn call a Lua function. + local lua_prop_setter + local lua_prop_getter + local c_prop_setter + local c_prop_getter + + + local function update_cvalue(cvalue, lvalue) + if lvalue == nil then + cvalue.data = nil + cvalue.len = 0 + return + end + + local value = tostring(lvalue) + cvalue.data = value + cvalue.len = #value end - local r = get_request() - if not r then - error("no request found", 2) + + local function jit_wrap(fn) + return function(r, ckey, cvalue) + jit.off() + local rc = fn(r, ckey, cvalue) + jit.on() + return rc + end end - lua_prop_setter = setter - local rc = C.ngx_http_wasm_ffi_set_property_setter(r, c_prop_setter) - if rc == FFI_OK then + c_prop_setter = ffi.cast("ngx_wasm_host_prop_fn_t", + jit_wrap(function(r, ckey, cvalue) + local lkey = ffi_str(ckey.data, ckey.len) + local lval + if cvalue.data ~= nil then + lval = ffi_str(cvalue.data, cvalue.len) + end + + local pok, ok, lvalue, is_const = pcall(lua_prop_setter, lkey, lval) + if not pok then + ngx.log(ngx.ERR, "error setting property from Lua: ", ok) + return FFI_ERROR + end + + if not ok then + local err = lvalue or "unknown error" + ngx.log(ngx.ERR, "error setting property: ", err) + return FFI_ERROR + end + + update_cvalue(cvalue, lvalue) + + return C.ngx_http_wasm_ffi_set_host_property(r, ckey, cvalue, + is_const and 1 or 0, + 0) + end)) + + + c_prop_getter = ffi.cast("ngx_wasm_host_prop_fn_t", + jit_wrap(function(r, ckey, cvalue) + local lkey = ffi_str(ckey.data, ckey.len) + + local pok, ok, lvalue, is_const = pcall(lua_prop_getter, lkey) + if not pok then + ngx.log(ngx.ERR, "error getting property from Lua: ", ok) + return FFI_ERROR + end + + if not ok then + if lvalue then + ngx.log(ngx.ERR, "error setting property: ", lvalue) + return FFI_ERROR + end + + return FFI_DECLINED + end + + update_cvalue(cvalue, lvalue) + + local rc = C.ngx_http_wasm_ffi_set_host_property(r, ckey, cvalue, + is_const and 1 or 0, + 1) + if rc == FFI_OK and lvalue == nil then + return FFI_DECLINED + end + + return rc + end)) + + + --- + -- Define a setter function for host-managed properties. + -- + -- @param setter The setter function is the handler for updating properties. + -- Its type signature is `function(string, string): boolean, string`, + -- where the inputs are the property key and value and the results are: + -- * `truthy` - success setting the property + -- * `truthy, value` - success, cache property value (useful if hosts + -- sets only a setter but not a getter) + -- * `truthy, value, truthy` - success, constant property value: further + -- requests to the same property during a request are cached by + -- ngx_wasm_module and do not invoke the Lua getter. + -- * `falsy, err` - failure, and an optional error message + -- @return true on success setting the getter, or nil and an error message. + function _M.set_property_setter(setter) + if type(setter) ~= "function" then + error("setter must be a function", 2) + end + + local r = get_request() + if not r then + error("no request found", 2) + end + + lua_prop_setter = setter + + local rc = C.ngx_http_wasm_ffi_set_property_setter(r, c_prop_setter) + if rc ~= FFI_OK then + return nil, "could not set property setter" + end + return true end - return nil, "could not set property setter" -end + --- + -- Define a getter function for host-managed properties. + -- + -- @param getter The getter function is the handler for resolving properties. + -- Its type signature is `function(string): boolean, string, boolean`, + -- where the input is the property key and the results are: + -- * `truthy, value` - success, property value + -- * `truthy, value, truthy` - success, constant property value: further + -- requests to the same property during a request are cached by + -- ngx_wasm_module and do not invoke the Lua getter. + -- * `falsy` - property not found + -- * `falsy, err` - failure, error message + -- @return true on success setting the getter, or nil and an error message. + function _M.set_property_getter(getter) + if type(getter) ~= "function" then + error("getter must be a function", 2) + end ---- --- Define a getter function for host-managed properties. --- --- @param getter The getter function is the handler for resolving properties. --- Its type signature is `function(string): boolean, string, boolean`, --- where the input is the property key and the results are: --- * `truthy, value` - success, property value --- * `truthy, value, truthy` - success, constant property value: further --- requests to the same property during a request are cached by --- ngx_wasm_module and do not invoke the Lua getter. --- * `falsy` - property not found --- * `falsy, err` - failure, error message --- @return true on success setting the getter, or nil and an error message. -function _M.set_property_getter(getter) - if type(getter) ~= "function" then - error("getter must be a function", 2) - end + local r = get_request() + if not r then + error("no request found", 2) + end - local r = get_request() - if not r then - error("no request found", 2) - end + lua_prop_getter = getter - lua_prop_getter = getter + local rc = C.ngx_http_wasm_ffi_set_property_getter(r, c_prop_getter) + if rc ~= FFI_OK then + return nil, "could not set property getter" + end - local rc = C.ngx_http_wasm_ffi_set_property_getter(r, c_prop_getter) - if rc == FFI_OK then return true end - - return nil, "could not set property getter" end diff --git a/src/common/proxy_wasm/ngx_proxy_wasm_properties.c b/src/common/proxy_wasm/ngx_proxy_wasm_properties.c index 74467e06b..333688722 100644 --- a/src/common/proxy_wasm/ngx_proxy_wasm_properties.c +++ b/src/common/proxy_wasm/ngx_proxy_wasm_properties.c @@ -54,7 +54,7 @@ typedef struct { ngx_str_node_t sn; ngx_str_t value; unsigned is_const:1; - unsigned negative_cache:1; + unsigned const_null:1; } host_props_node_t; @@ -879,7 +879,7 @@ ngx_proxy_wasm_properties_get_host(ngx_proxy_wasm_ctx_t *pwctx, hpn = (host_props_node_t *) ngx_str_rbtree_lookup(&pwctx->host_props_tree, path, hash); - if (!hpn) { + if (hpn == NULL) { return NGX_DECLINED; } @@ -888,13 +888,13 @@ ngx_proxy_wasm_properties_get_host(ngx_proxy_wasm_ctx_t *pwctx, return NGX_DECLINED; } - value->data = hpn->value.data; - value->len = hpn->value.len; - - if (hpn->negative_cache) { + if (hpn->const_null) { return NGX_BUSY; } + value->data = hpn->value.data; + value->len = hpn->value.len; + return NGX_OK; } @@ -907,7 +907,7 @@ set_hpn_value(host_props_node_t *hpn, ngx_str_t *value, unsigned is_const, if (value->data == NULL) { new_data = NULL; - hpn->negative_cache = 1; + hpn->const_null = 1; } else { new_data = ngx_pstrdup(pool, value); @@ -946,29 +946,36 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx, hpn = (host_props_node_t *) ngx_str_rbtree_lookup(&pwctx->host_props_tree, path, hash); - if (hpn) { - if (hpn->is_const) { - return NGX_DECLINED; - } + if (hpn && hpn->is_const) { + /* if a const node exists, we can't overwrite it */ - ngx_pfree(pwctx->pool, hpn->value.data); + return NGX_DECLINED; + } - if (value->data == NULL && !is_const) { - ngx_rbtree_delete(&pwctx->host_props_tree, &hpn->sn.node); + if (value->data == NULL && !is_const) { + /* if setting non-const NULL, remove node if it exists */ - return NGX_OK; + if (hpn) { + ngx_pfree(pwctx->pool, hpn->value.data); + ngx_rbtree_delete(&pwctx->host_props_tree, &hpn->sn.node); } + return NGX_OK; + } + + if (hpn) { + /* if the node exists, replace the value */ + + ngx_pfree(pwctx->pool, hpn->value.data); rc = set_hpn_value(hpn, value, is_const, pwctx->pool); if (rc != NGX_OK) { return rc; } - } else if (value->data == NULL && !is_const) { - /* no need to store a non-const NULL */ - return NGX_OK; - } else { + /* if the node doesn't exist, + create it, set key, value, and store node in rbtree */ + hpn = ngx_pcalloc(pwctx->pool, sizeof(host_props_node_t) + path->len); if (hpn == NULL) { return NGX_ERROR; From 921035c19ab63db4d59161bb07d7328957119536 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Fri, 27 Oct 2023 15:05:43 -0300 Subject: [PATCH 05/11] fix(ffi) use Lua/C API for calling host property handlers --- lib/resty/wasmx/proxy_wasm.lua | 215 +++++------------- src/common/lua/ngx_wasm_lua_ffi.c | 137 +++++++++-- src/common/lua/ngx_wasm_lua_ffi.h | 7 + .../ffi/304-proxy_wasm_set_property_getter.t | 2 +- .../ffi/305-proxy_wasm_set_property_setter.t | 10 +- 5 files changed, 192 insertions(+), 179 deletions(-) diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index 095607d4e..aa2e77d28 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -8,11 +8,9 @@ local C = ffi.C local ngx = ngx local error = error local type = type -local pcall = pcall local ffi_gc = ffi.gc local ffi_new = ffi.new local ffi_str = ffi.string -local tostring = tostring local subsystem = ngx.config.subsystem local get_request = wasmx.get_request local get_err_ptr = wasmx.get_err_ptr @@ -57,15 +55,8 @@ if subsystem == "http" then ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, ngx_str_t *key, ngx_str_t *out); - ngx_int_t ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *value, - unsigned is_const, - unsigned retrieve); - ngx_int_t ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, - ngx_wasm_host_prop_fn_t fn); - ngx_int_t ngx_http_wasm_ffi_set_property_getter(ngx_http_request_t *r, - ngx_wasm_host_prop_fn_t fn); + ngx_int_t ngx_http_wasm_ffi_enable_property_setter(ngx_http_request_t *r); + ngx_int_t ngx_http_wasm_ffi_enable_property_getter(ngx_http_request_t *r); ]] else @@ -297,166 +288,72 @@ function _M.get_property(key) end -do - -- LuaJIT C callbacks are a limited resource: - -- We can't create one of these on each request, - -- so we prepare these wrappers, which in turn call a Lua function. - local lua_prop_setter - local lua_prop_getter - local c_prop_setter - local c_prop_getter - - - local function update_cvalue(cvalue, lvalue) - if lvalue == nil then - cvalue.data = nil - cvalue.len = 0 - return - end - - local value = tostring(lvalue) - cvalue.data = value - cvalue.len = #value +--- +-- Define a setter function for host-managed properties. +-- +-- @param setter The setter function is the handler for updating properties. +-- Its type signature is `function(string, string): boolean, string`, +-- where the inputs are the property key and value and the results are: +-- * `truthy` - success setting the property +-- * `truthy, value` - success, cache property value (useful if hosts +-- sets only a setter but not a getter) +-- * `truthy, value, truthy` - success, constant property value: further +-- requests to the same property during a request are cached by +-- ngx_wasm_module and do not invoke the Lua getter. +-- * `falsy, err` - failure, and an optional error message +-- @return true on success setting the getter, or nil and an error message. +function _M.set_property_setter(setter) + if type(setter) ~= "function" then + error("setter must be a function", 2) end - - local function jit_wrap(fn) - return function(r, ckey, cvalue) - jit.off() - local rc = fn(r, ckey, cvalue) - jit.on() - return rc - end + local r = get_request() + if not r then + error("no request found", 2) end + _M.property_setter = setter - c_prop_setter = ffi.cast("ngx_wasm_host_prop_fn_t", - jit_wrap(function(r, ckey, cvalue) - local lkey = ffi_str(ckey.data, ckey.len) - local lval - if cvalue.data ~= nil then - lval = ffi_str(cvalue.data, cvalue.len) - end - - local pok, ok, lvalue, is_const = pcall(lua_prop_setter, lkey, lval) - if not pok then - ngx.log(ngx.ERR, "error setting property from Lua: ", ok) - return FFI_ERROR - end - - if not ok then - local err = lvalue or "unknown error" - ngx.log(ngx.ERR, "error setting property: ", err) - return FFI_ERROR - end - - update_cvalue(cvalue, lvalue) - - return C.ngx_http_wasm_ffi_set_host_property(r, ckey, cvalue, - is_const and 1 or 0, - 0) - end)) - - - c_prop_getter = ffi.cast("ngx_wasm_host_prop_fn_t", - jit_wrap(function(r, ckey, cvalue) - local lkey = ffi_str(ckey.data, ckey.len) - - local pok, ok, lvalue, is_const = pcall(lua_prop_getter, lkey) - if not pok then - ngx.log(ngx.ERR, "error getting property from Lua: ", ok) - return FFI_ERROR - end - - if not ok then - if lvalue then - ngx.log(ngx.ERR, "error setting property: ", lvalue) - return FFI_ERROR - end - - return FFI_DECLINED - end - - update_cvalue(cvalue, lvalue) - - local rc = C.ngx_http_wasm_ffi_set_host_property(r, ckey, cvalue, - is_const and 1 or 0, - 1) - if rc == FFI_OK and lvalue == nil then - return FFI_DECLINED - end - - return rc - end)) - - - --- - -- Define a setter function for host-managed properties. - -- - -- @param setter The setter function is the handler for updating properties. - -- Its type signature is `function(string, string): boolean, string`, - -- where the inputs are the property key and value and the results are: - -- * `truthy` - success setting the property - -- * `truthy, value` - success, cache property value (useful if hosts - -- sets only a setter but not a getter) - -- * `truthy, value, truthy` - success, constant property value: further - -- requests to the same property during a request are cached by - -- ngx_wasm_module and do not invoke the Lua getter. - -- * `falsy, err` - failure, and an optional error message - -- @return true on success setting the getter, or nil and an error message. - function _M.set_property_setter(setter) - if type(setter) ~= "function" then - error("setter must be a function", 2) - end - - local r = get_request() - if not r then - error("no request found", 2) - end - - lua_prop_setter = setter + local rc = C.ngx_http_wasm_ffi_enable_property_setter(r) + if rc ~= FFI_OK then + return nil, "could not set property setter" + end - local rc = C.ngx_http_wasm_ffi_set_property_setter(r, c_prop_setter) - if rc ~= FFI_OK then - return nil, "could not set property setter" - end + return true +end - return true - end - - - --- - -- Define a getter function for host-managed properties. - -- - -- @param getter The getter function is the handler for resolving properties. - -- Its type signature is `function(string): boolean, string, boolean`, - -- where the input is the property key and the results are: - -- * `truthy, value` - success, property value - -- * `truthy, value, truthy` - success, constant property value: further - -- requests to the same property during a request are cached by - -- ngx_wasm_module and do not invoke the Lua getter. - -- * `falsy` - property not found - -- * `falsy, err` - failure, error message - -- @return true on success setting the getter, or nil and an error message. - function _M.set_property_getter(getter) - if type(getter) ~= "function" then - error("getter must be a function", 2) - end - local r = get_request() - if not r then - error("no request found", 2) - end +--- +-- Define a getter function for host-managed properties. +-- +-- @param getter The getter function is the handler for resolving properties. +-- Its type signature is `function(string): boolean, string, boolean`, +-- where the input is the property key and the results are: +-- * `truthy, value` - success, property value +-- * `truthy, value, truthy` - success, constant property value: further +-- requests to the same property during a request are cached by +-- ngx_wasm_module and do not invoke the Lua getter. +-- * `falsy` - property not found +-- * `falsy, err` - failure, error message +-- @return true on success setting the getter, or nil and an error message. +function _M.set_property_getter(getter) + if type(getter) ~= "function" then + error("getter must be a function", 2) + end - lua_prop_getter = getter + local r = get_request() + if not r then + error("no request found", 2) + end - local rc = C.ngx_http_wasm_ffi_set_property_getter(r, c_prop_getter) - if rc ~= FFI_OK then - return nil, "could not set property getter" - end + _M.property_getter = getter - return true + local rc = C.ngx_http_wasm_ffi_enable_property_getter(r) + if rc ~= FFI_OK then + return nil, "could not set property getter" end + + return true end diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index aaa5b54af..fac9d1df3 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -3,6 +3,7 @@ #endif #include "ddebug.h" +#include #include @@ -243,45 +244,153 @@ ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, } -ngx_int_t -ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value, unsigned is_const, unsigned retrieve) +static ngx_int_t +property_handler(void *data, ngx_str_t *key, ngx_str_t *value) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + ngx_wasm_ffi_host_property_ctx_t *hpctx = data; + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hpctx->r); + lua_State *L = hpctx->L; + ngx_int_t rc; + unsigned is_const; + int pok; + int top; + + top = lua_gettop(L); + + lua_getglobal(L, "package"); + lua_getfield(L, -1, "loaded"); + lua_getfield(L, -1, "resty.wasmx.proxy_wasm"); + if (lua_type(L, -1) != LUA_TTABLE) { + goto fail; + } + + lua_getfield(L, -1, + hpctx->is_getter ? "property_getter" : "property_setter"); + if (lua_type(L, -1) != LUA_TFUNCTION) { + goto fail; + } + + ngx_wasm_assert(key && key->data); + + lua_pushlstring(L, (char *) key->data, key->len); + + ngx_wasm_assert(value); + + if (value->data == NULL) { + lua_pushnil(L); + + } else { + if ((int) value->len == 0) { + lua_pushliteral(L, ""); + + } else { + lua_pushlstring(L, (char *) value->data, value->len); + } + } + + pok = lua_pcall(L, 2, 3, 0); + if (pok != 0) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "Lua error in property handler: %s", + lua_tostring(L, -1)); + goto fail; + } + + if (lua_toboolean(L, -3) == 0) { + if (lua_isstring(L, -2)) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "error in property handler: %s", + lua_tostring(L, -2)); + goto fail; + + } else if (!hpctx->is_getter) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "error in property handler: unknown error"); + goto fail; + + } + + return NGX_DECLINED; + } + + if (lua_isnil(L, -2)) { + value->data = NULL; + value->len = 0; + + } else { + value->data = (u_char *) lua_tolstring(L, -2, &value->len); + } + + is_const = lua_toboolean(L, -1); if (pwctx == NULL) { return NGX_ERROR; } - return ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, - retrieve); + rc = ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, + hpctx->is_getter); + + lua_settop(L, top); + + if (hpctx->is_getter && rc == NGX_OK && value->data == NULL) { + return NGX_DECLINED; + } + + return rc; + +fail: + + lua_settop(L, top); + return NGX_ERROR; } ngx_int_t -ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, - ngx_wasm_host_prop_fn_t fn) +ngx_http_wasm_ffi_enable_property_setter(ngx_http_request_t *r) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + ngx_wasm_ffi_host_property_ctx_t *hpctx; if (pwctx == NULL) { return NGX_ERROR; } - return ngx_proxy_wasm_properties_set_host_prop_setter(pwctx, fn, r); + hpctx = ngx_palloc(pwctx->pool, sizeof(ngx_wasm_ffi_host_property_ctx_t)); + if (hpctx == NULL) { + return NGX_ERROR; + } + + hpctx->r = r; + hpctx->L = ngx_http_lua_get_lua_vm(r, NULL); + hpctx->is_getter = 0; + + return ngx_proxy_wasm_properties_set_host_prop_setter(pwctx, + property_handler, + hpctx); } ngx_int_t -ngx_http_wasm_ffi_set_property_getter(ngx_http_request_t *r, - ngx_wasm_host_prop_fn_t fn) +ngx_http_wasm_ffi_enable_property_getter(ngx_http_request_t *r) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + ngx_wasm_ffi_host_property_ctx_t *hpctx; if (pwctx == NULL) { return NGX_ERROR; } - return ngx_proxy_wasm_properties_set_host_prop_getter(pwctx, fn, r); + hpctx = ngx_palloc(pwctx->pool, sizeof(ngx_wasm_ffi_host_property_ctx_t)); + if (hpctx == NULL) { + return NGX_ERROR; + } + + hpctx->r = r; + hpctx->L = ngx_http_lua_get_lua_vm(r, NULL); + hpctx->is_getter = 1; + + return ngx_proxy_wasm_properties_set_host_prop_getter(pwctx, + property_handler, + hpctx); } #endif diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 1d87e22d1..0dc78299d 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -21,6 +21,13 @@ typedef struct { } ngx_wasm_ffi_filter_t; +typedef struct { + ngx_http_request_t *r; + lua_State *L; + unsigned is_getter; +} ngx_wasm_ffi_host_property_ctx_t; + + ngx_int_t ngx_http_wasm_ffi_plan_new(ngx_wavm_t *vm, ngx_wasm_ffi_filter_t *filters, size_t n_filters, ngx_wasm_ops_plan_t **out, u_char *err, size_t *errlen); diff --git a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t index e6c8f764f..ac8f8786b 100644 --- a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t +++ b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t @@ -591,7 +591,7 @@ ok --- error_log eval [ qr/\[info\] .*? \[hostcalls\] on_response_body/, - qr/\[error\] .*? error getting property from Lua: rewrite_by_lua\(nginx.conf:[0-9]+\):[0-9]+: crash\!/, + qr/\[error\] .*? Lua error in property handler: rewrite_by_lua\(nginx.conf:[0-9]+\):[0-9]+: crash\!/, ] --- no_error_log [emerg] diff --git a/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t b/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t index 471385d3d..04f05af05 100644 --- a/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t +++ b/t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t @@ -431,10 +431,10 @@ unknown error unknown error --- grep_error_log eval: qr/.*property.*/ --- grep_error_log_out eval -qr/^[^\n]*?\[error\] [^\n]*? error setting property: first wasmx property custom error[^\n]*? -[^\n]*?\[error\] [^\n]*? error setting property: second wasmx property custom error[^\n]*? -[^\n]*?\[error\] [^\n]*? error setting property: unknown error[^\n]*? -[^\n]*?\[error\] [^\n]*? error setting property: unknown error[^\n]*? +qr/^[^\n]*?\[error\] [^\n]*? error in property handler: first wasmx property custom error[^\n]*? +[^\n]*?\[error\] [^\n]*? error in property handler: second wasmx property custom error[^\n]*? +[^\n]*?\[error\] [^\n]*? error in property handler: unknown error[^\n]*? +[^\n]*?\[error\] [^\n]*? error in property handler: unknown error[^\n]*? [^\n]*?\[info\] [^\n]*? property not found: wasmx.fail1_property[^\n]*?$/ --- no_error_log [crit] @@ -610,7 +610,7 @@ ok --- error_log eval [ qr/\[info\] .*? \[hostcalls\] on_response_body/, - qr/\[error\] .*? error setting property from Lua: rewrite_by_lua\(nginx.conf:[0-9]+\):[0-9]+: crash\!/, + qr/\[error\] .*? Lua error in property handler: rewrite_by_lua\(nginx.conf:[0-9]+\):[0-9]+: crash\!/, ] --- no_error_log [emerg] From e2f2449e6dbe9efdba3662a74056ede0874ed7a2 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Wed, 8 Nov 2023 18:03:44 -0300 Subject: [PATCH 06/11] fix?(lua) set request in Lua context to match the caller's As far as I can tell this shouldn't be necessary as the coroutine uses the same global state as its parent, but I was observing crashes in OpenResty code triggered by `ngx.log` at the log phase, where the `log` variable used by OpenResty's C code was set to one from already-freed data from a fake request created during `init_worker`. Forcing the Lua state's `r` to match the caller's here made the tests pass and Valgrind happy. --- src/common/lua/ngx_wasm_lua.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/common/lua/ngx_wasm_lua.c b/src/common/lua/ngx_wasm_lua.c index fc83c1ba7..482acc76a 100644 --- a/src/common/lua/ngx_wasm_lua.c +++ b/src/common/lua/ngx_wasm_lua.c @@ -155,6 +155,9 @@ ngx_wasm_lua_thread_new(const char *tag, const char *src, lctx->L = ngx_http_lua_get_lua_vm(r, NULL); lctx->co = ngx_http_lua_new_thread(r, lctx->L, &lctx->co_ref); + + ngx_http_lua_set_req(lctx->co, r); + break; } #endif @@ -166,6 +169,9 @@ ngx_wasm_lua_thread_new(const char *tag, const char *src, lctx->L = ngx_stream_lua_get_lua_vm(sr, NULL); lctx->co = ngx_stream_lua_new_thread(sr, lctx->L, lctx->co_ref); + + ngx_stream_lua_set_req(lctx->co, sr); + break; } #endif From be6e21cffbf08ee2e0da053a401ee22154ee20f2 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Mon, 30 Oct 2023 11:13:43 -0300 Subject: [PATCH 07/11] [wip] fix(*) use the Lua bridge --- lib/resty/wasmx/proxy_wasm.lua | 4 + src/common/lua/ngx_wasm_lua.c | 98 +++++++------ src/common/lua/ngx_wasm_lua_ffi.c | 225 ++++++++++++++++++++---------- src/common/lua/ngx_wasm_lua_ffi.h | 15 +- 4 files changed, 222 insertions(+), 120 deletions(-) diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index aa2e77d28..b3310cb04 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -284,6 +284,10 @@ function _M.get_property(key) return nil, "property \"" .. key .. "\" not found", NOT_FOUND end + if cvalue.data == nil then + return nil + end + return ffi_str(cvalue.data, cvalue.len) end diff --git a/src/common/lua/ngx_wasm_lua.c b/src/common/lua/ngx_wasm_lua.c index 482acc76a..11f8274c2 100644 --- a/src/common/lua/ngx_wasm_lua.c +++ b/src/common/lua/ngx_wasm_lua.c @@ -1,5 +1,5 @@ #ifndef DDEBUG -#define DDEBUG 0 +#define DDEBUG 1 #endif #include "ddebug.h" @@ -113,6 +113,56 @@ ngx_wasm_lua_thread_destroy(ngx_wasm_lua_ctx_t *lctx) } +static ngx_int_t +ngx_wasm_lua_load_code(ngx_wasm_lua_ctx_t *lctx, + const char *tag, const char *src, + ngx_wasm_subsys_env_t *env) +{ + ngx_int_t rc; + + lctx->code = src; + lctx->code_len = ngx_strlen(src); + lctx->cache_key = ngx_wasm_lua_thread_cache_key(lctx->pool, + tag, + (u_char *) lctx->code, + lctx->code_len); + if (lctx->cache_key == NULL) { + return NGX_ERROR; + } + + /* load code */ + + switch (env->subsys->kind) { +#if (NGX_WASM_HTTP) + case NGX_WASM_SUBSYS_HTTP: + rc = ngx_http_lua_cache_loadbuffer(lctx->log, + lctx->L, + (u_char *) lctx->code, + lctx->code_len, + &lctx->code_ref, + lctx->cache_key, + tag); + break; +#endif +#if (NGX_WASM_STREAM) + case NGX_WASM_SUBSYS_STREAM: + rc = ngx_stream_lua_cache_loadbuffer(lctx->log, + lctx->L, + (u_char *) lctx->code, + lctx->code_len, + lctx->cache_key, + tag); + break; +#endif + default: + ngx_wasm_assert(0); + return NGX_ERROR; + } + + return rc; +} + + ngx_wasm_lua_ctx_t * ngx_wasm_lua_thread_new(const char *tag, const char *src, ngx_wasm_subsys_env_t *env, ngx_log_t *log, void *data, @@ -189,47 +239,11 @@ ngx_wasm_lua_thread_new(const char *tag, const char *src, /* code */ - lctx->code = src; - lctx->code_len = ngx_strlen(src); - lctx->cache_key = ngx_wasm_lua_thread_cache_key(lctx->pool, - tag, - (u_char *) lctx->code, - lctx->code_len); - if (lctx->cache_key == NULL) { - goto error; - } - - /* load code */ - - switch (env->subsys->kind) { -#if (NGX_WASM_HTTP) - case NGX_WASM_SUBSYS_HTTP: - rc = ngx_http_lua_cache_loadbuffer(lctx->log, - lctx->L, - (u_char *) lctx->code, - lctx->code_len, - &lctx->code_ref, - lctx->cache_key, - tag); - break; -#endif -#if (NGX_WASM_STREAM) - case NGX_WASM_SUBSYS_STREAM: - rc = ngx_stream_lua_cache_loadbuffer(lctx->log, - lctx->L, - (u_char *) lctx->code, - lctx->code_len, - lctx->cache_key, - tag); - break; -#endif - default: - ngx_wasm_assert(0); - goto error; - } - - if (rc != NGX_OK) { - goto error; + if (src) { + rc = ngx_wasm_lua_load_code(lctx, tag, src, env); + if (rc != NGX_OK) { + goto error; + } } /* move code closure to new coroutine */ diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index fac9d1df3..604b1cef9 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -244,104 +244,187 @@ ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, } -static ngx_int_t -property_handler(void *data, ngx_str_t *key, ngx_str_t *value) +void +ngx_wasm_lua_ffi_host_prop_handler( + ngx_wasm_ffi_host_property_req_ctx_t *hprctx, + unsigned char ok, const char* new_value, size_t len, + unsigned char is_const) { - ngx_wasm_ffi_host_property_ctx_t *hpctx = data; - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hpctx->r); - lua_State *L = hpctx->L; - ngx_int_t rc; - unsigned is_const; - int pok; - int top; + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hprctx->r); + ngx_str_t *key = hprctx->key; + ngx_str_t *value = hprctx->value; + + if (!ok) { + if (new_value) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "error in property handler: %s", + new_value); + hprctx->rc = NGX_ERROR; + return; - top = lua_gettop(L); + } else if (!hprctx->is_getter) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "error in property handler: unknown error"); + hprctx->rc = NGX_ERROR; + return; + } - lua_getglobal(L, "package"); - lua_getfield(L, -1, "loaded"); - lua_getfield(L, -1, "resty.wasmx.proxy_wasm"); - if (lua_type(L, -1) != LUA_TTABLE) { - goto fail; + hprctx->rc = NGX_DECLINED; + return; } - lua_getfield(L, -1, - hpctx->is_getter ? "property_getter" : "property_setter"); - if (lua_type(L, -1) != LUA_TFUNCTION) { - goto fail; + if (new_value == NULL) { + value->data = NULL; + value->len = 0; + + } else { + value->data = (u_char *) new_value; + value->len = len; } - ngx_wasm_assert(key && key->data); + hprctx->rc = ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, + hprctx->is_getter); + + if (hprctx->is_getter && hprctx->rc == NGX_OK && value->data == NULL) { + hprctx->rc = NGX_DECLINED; + } + + hprctx->rc = NGX_OK; +} - lua_pushlstring(L, (char *) key->data, key->len); +static const char *HOST_PROPERTY_SCRIPT_NAME = "wasm_lua_host_property_chunk"; +static const char *HOST_PROPERTY_SCRIPT = "" + "local hprctx, is_getter, key, value = ... \n" + "local ffi = require('ffi') \n" + "local fmt = string.format \n" + " \n" + "ffi.cdef[[ \n" + " typedef unsigned char u_char; \n" + " typedef void ngx_wasm_ffi_host_property_req_ctx_t; \n" + " \n" + " void ngx_wasm_lua_ffi_host_prop_handler( \n" + " ngx_wasm_ffi_host_property_req_ctx_t *hprctx, \n" + " u_char ok, const char* value, size_t len, \n" + " u_char is_const); \n" + "]] \n" + " \n" + "local proxy_wasm = require('resty.wasmx.proxy_wasm') \n" + " \n" + "local fn_name = is_getter and 'property_getter' or 'property_setter' \n" + "local fn = proxy_wasm[fn_name] \n" + "if not fn then \n" + " error('property handler function not found') \n" + "end \n" + " \n" + "local pok, ok, new_value, is_const = pcall(fn, key, value) \n" + "if not pok then \n" + " ngx.log(ngx.ERR, 'Lua error in property handler: ' .. ok) \n" + " error('fail') \n" + "end \n" + " \n" + "local new_value_len = new_value and #new_value or 0 \n" + " \n" + "ffi.C.ngx_wasm_lua_ffi_host_prop_handler(hprctx, ok and 1 or 0, \n" + " new_value, new_value_len, \n" + " is_const and 1 or 0) \n"; +static ngx_int_t +property_handler(void *data, ngx_str_t *key, ngx_str_t *value) +{ + ngx_wasm_ffi_host_property_ctx_t *hpctx = data; + ngx_wasm_ffi_host_property_req_ctx_t hprctx; + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hpctx->r); + lua_State *co; + ngx_int_t rc = NGX_OK; + ngx_wasm_lua_ctx_t *lctx; + + // FIXME if this function is changed to return NGX_AGAIN, + // hprctx needs to be dynamically allocated: + + hprctx.r = hpctx->r; + hprctx.is_getter = hpctx->is_getter; + hprctx.key = key; + hprctx.value = value; + + /* create coroutine */ + + lctx = ngx_wasm_lua_thread_new(HOST_PROPERTY_SCRIPT_NAME, + HOST_PROPERTY_SCRIPT, + hpctx->env, pwctx->log, &hprctx, + NULL, NULL); + if (lctx == NULL) { + return NGX_ERROR; + } + + /* push key and value arguments */ + + co = lctx->co; + + ngx_wasm_assert(key && key->data); ngx_wasm_assert(value); - if (value->data == NULL) { - lua_pushnil(L); + lctx->nargs = 4; + + lua_pushlightuserdata(co, &hprctx); + lua_pushboolean(co, hpctx->is_getter); + lua_pushlstring(co, (char *) key->data, key->len); + + if (hpctx->is_getter || value->data == NULL) { + lua_pushnil(co); } else { if ((int) value->len == 0) { - lua_pushliteral(L, ""); + lua_pushliteral(co, ""); } else { - lua_pushlstring(L, (char *) value->data, value->len); + lua_pushlstring(co, (char *) value->data, value->len); } } - pok = lua_pcall(L, 2, 3, 0); - if (pok != 0) { - ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, - "Lua error in property handler: %s", - lua_tostring(L, -1)); - goto fail; - } + /* run handler */ - if (lua_toboolean(L, -3) == 0) { - if (lua_isstring(L, -2)) { - ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, - "error in property handler: %s", - lua_tostring(L, -2)); - goto fail; - - } else if (!hpctx->is_getter) { - ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, - "error in property handler: unknown error"); - goto fail; - - } + rc = ngx_wasm_lua_thread_run(lctx); + if (rc == NGX_AGAIN) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "cannot yield from host property handler"); - return NGX_DECLINED; + return NGX_ERROR; } - if (lua_isnil(L, -2)) { - value->data = NULL; - value->len = 0; + // ngx_wasm_assert(rc == NGX_DONE); - } else { - value->data = (u_char *) lua_tolstring(L, -2, &value->len); - } + return hprctx.rc; +} - is_const = lua_toboolean(L, -1); + +static ngx_wasm_ffi_host_property_ctx_t * +get_hpctx(ngx_http_request_t *r, unsigned is_getter) +{ + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); + ngx_wasm_ffi_host_property_ctx_t *hpctx; + ngx_http_wasm_req_ctx_t *rctx; + ngx_int_t rc; if (pwctx == NULL) { - return NGX_ERROR; + return NULL; } - rc = ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, - hpctx->is_getter); - - lua_settop(L, top); - - if (hpctx->is_getter && rc == NGX_OK && value->data == NULL) { - return NGX_DECLINED; + rc = ngx_http_wasm_rctx(r, &rctx); + ngx_wasm_assert(rc != NGX_DECLINED); + if (rc != NGX_OK) { + return NULL; } - return rc; + hpctx = ngx_palloc(pwctx->pool, sizeof(ngx_wasm_ffi_host_property_ctx_t)); + if (hpctx == NULL) { + return NULL; + } -fail: + hpctx->r = r; + hpctx->env = &rctx->env; + hpctx->is_getter = is_getter; - lua_settop(L, top); - return NGX_ERROR; + return hpctx; } @@ -355,15 +438,11 @@ ngx_http_wasm_ffi_enable_property_setter(ngx_http_request_t *r) return NGX_ERROR; } - hpctx = ngx_palloc(pwctx->pool, sizeof(ngx_wasm_ffi_host_property_ctx_t)); + hpctx = get_hpctx(r, 0); if (hpctx == NULL) { return NGX_ERROR; } - hpctx->r = r; - hpctx->L = ngx_http_lua_get_lua_vm(r, NULL); - hpctx->is_getter = 0; - return ngx_proxy_wasm_properties_set_host_prop_setter(pwctx, property_handler, hpctx); @@ -380,15 +459,11 @@ ngx_http_wasm_ffi_enable_property_getter(ngx_http_request_t *r) return NGX_ERROR; } - hpctx = ngx_palloc(pwctx->pool, sizeof(ngx_wasm_ffi_host_property_ctx_t)); + hpctx = get_hpctx(r, 1); if (hpctx == NULL) { return NGX_ERROR; } - hpctx->r = r; - hpctx->L = ngx_http_lua_get_lua_vm(r, NULL); - hpctx->is_getter = 1; - return ngx_proxy_wasm_properties_set_host_prop_getter(pwctx, property_handler, hpctx); diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 0dc78299d..889306263 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -22,12 +22,21 @@ typedef struct { typedef struct { - ngx_http_request_t *r; - lua_State *L; - unsigned is_getter; + ngx_http_request_t *r; + ngx_wasm_subsys_env_t *env; + unsigned is_getter; } ngx_wasm_ffi_host_property_ctx_t; +typedef struct { + ngx_http_request_t *r; + unsigned is_getter; + ngx_str_t *key; + ngx_str_t *value; + ngx_int_t rc; +} ngx_wasm_ffi_host_property_req_ctx_t; + + ngx_int_t ngx_http_wasm_ffi_plan_new(ngx_wavm_t *vm, ngx_wasm_ffi_filter_t *filters, size_t n_filters, ngx_wasm_ops_plan_t **out, u_char *err, size_t *errlen); From c7fa536b53770a63f04f46913c1df783640e7dd3 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Tue, 7 Nov 2023 18:59:26 -0300 Subject: [PATCH 08/11] [wip] make TEST 10 pass by avoiding FFI returns. TEST 9 still fails... --- lib/resty/wasmx/proxy_wasm.lua | 24 +++++++++++------ src/common/lua/ngx_wasm_lua_ffi.c | 27 ++++++++++++------- src/common/lua/ngx_wasm_lua_ffi.h | 8 +++--- .../ffi/304-proxy_wasm_set_property_getter.t | 1 + 4 files changed, 38 insertions(+), 22 deletions(-) diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index b3310cb04..32888ec29 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -49,12 +49,14 @@ if subsystem == "http" then ngx_wasm_plan_t *plan, unsigned int isolation); ngx_int_t ngx_http_wasm_ffi_start(ngx_http_request_t *r); - ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *value); - ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *out); + void ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *value, + ngx_int_t *rc); + void ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *out, + ngx_int_t *rc); ngx_int_t ngx_http_wasm_ffi_enable_property_setter(ngx_http_request_t *r); ngx_int_t ngx_http_wasm_ffi_enable_property_getter(ngx_http_request_t *r); ]] @@ -248,8 +250,11 @@ function _M.set_property(key, value) data = value, len = value and #value or 0 }) + local rcp = ffi_new("ngx_int_t[1]", { 0 }) - local rc = C.ngx_http_wasm_ffi_set_property(r, ckey, cvalue) + C.ngx_http_wasm_ffi_set_property(r, ckey, cvalue, rcp) + + local rc = rcp[0] if rc == FFI_ERROR then return nil, "unknown error" end @@ -274,8 +279,11 @@ function _M.get_property(key) local ckey = ffi_new("ngx_str_t", { data = key, len = #key }) local cvalue = ffi_new("ngx_str_t", { data = nil, len = 0 }) + local rcp = ffi_new("ngx_int_t[1]", { 0 }) + + C.ngx_http_wasm_ffi_get_property(r, ckey, cvalue, rcp) - local rc = C.ngx_http_wasm_ffi_get_property(r, ckey, cvalue) + local rc = tonumber(rcp[0]) if rc == FFI_ERROR then return nil, "unknown error", ERROR end diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index 604b1cef9..430037367 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -196,31 +196,34 @@ get_pwctx(ngx_http_request_t *r) } -ngx_int_t +void ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value) + ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc) { ngx_http_wasm_req_ctx_t *rctx; ngx_proxy_wasm_ctx_t *pwctx; if (ngx_http_wasm_rctx(r, &rctx) != NGX_OK) { - return NGX_ERROR; + *rc = NGX_ERROR; + return; } pwctx = ngx_proxy_wasm_ctx(NULL, 0, NGX_PROXY_WASM_ISOLATION_STREAM, &ngx_http_proxy_wasm, rctx); if (pwctx == NULL) { - return NGX_ERROR; + *rc = NGX_ERROR; + return; } - return ngx_proxy_wasm_properties_set(pwctx, key, value); + *rc = ngx_proxy_wasm_properties_set(pwctx, key, value); + return; } -ngx_int_t +void ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value) + ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc) { ngx_http_wasm_req_ctx_t *rctx; ngx_proxy_wasm_ctx_t *pwctx; @@ -230,17 +233,20 @@ ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, * TODO: return code signifying "no plan attached to request" and co. * return associated constant as err/code from Lua lib */ - return NGX_ERROR; + *rc = NGX_ERROR; + return; } pwctx = ngx_proxy_wasm_ctx(NULL, 0, NGX_PROXY_WASM_ISOLATION_STREAM, &ngx_http_proxy_wasm, rctx); if (pwctx == NULL) { - return NGX_ERROR; + *rc = NGX_ERROR; + return; } - return ngx_proxy_wasm_properties_get(pwctx, key, value); + *rc = ngx_proxy_wasm_properties_get(pwctx, key, value); + return; } @@ -324,6 +330,7 @@ static const char *HOST_PROPERTY_SCRIPT = "" " \n" "local new_value_len = new_value and #new_value or 0 \n" " \n" + " \n" "ffi.C.ngx_wasm_lua_ffi_host_prop_handler(hprctx, ok and 1 or 0, \n" " new_value, new_value_len, \n" " is_const and 1 or 0) \n"; diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 889306263..26b5d97f4 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -45,10 +45,10 @@ ngx_int_t ngx_http_wasm_ffi_plan_load(ngx_wasm_ops_plan_t *plan); ngx_int_t ngx_http_wasm_ffi_plan_attach(ngx_http_request_t *r, ngx_wasm_ops_plan_t *plan, ngx_uint_t isolation); ngx_int_t ngx_http_wasm_ffi_start(ngx_http_request_t *r); -ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value); -ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value); +void ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, + ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc); +void ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, + ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc); ngx_int_t ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, ngx_str_t *key, ngx_str_t *value, unsigned is_const, unsigned retrieve); ngx_int_t ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, diff --git a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t index ac8f8786b..fb053dce0 100644 --- a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t +++ b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t @@ -398,6 +398,7 @@ ok === TEST 9: set_property_getter() - getter returning nil produces property not found +--- ONLY --- wasm_modules: hostcalls --- http_config init_worker_by_lua_block { From d72f8de42c33e9865b76d02d6236aaff98c3fe84 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Tue, 7 Nov 2023 19:35:10 -0300 Subject: [PATCH 09/11] Revert "[wip] make TEST 10 pass by avoiding FFI returns. TEST 9 still fails..." This reverts commit efe1c314164ea69ff9dd9f66153a769c1468ef68. --- lib/resty/wasmx/proxy_wasm.lua | 24 ++++++----------- src/common/lua/ngx_wasm_lua_ffi.c | 26 +++++++------------ src/common/lua/ngx_wasm_lua_ffi.h | 8 +++--- .../ffi/304-proxy_wasm_set_property_getter.t | 1 - 4 files changed, 22 insertions(+), 37 deletions(-) diff --git a/lib/resty/wasmx/proxy_wasm.lua b/lib/resty/wasmx/proxy_wasm.lua index 32888ec29..b3310cb04 100644 --- a/lib/resty/wasmx/proxy_wasm.lua +++ b/lib/resty/wasmx/proxy_wasm.lua @@ -49,14 +49,12 @@ if subsystem == "http" then ngx_wasm_plan_t *plan, unsigned int isolation); ngx_int_t ngx_http_wasm_ffi_start(ngx_http_request_t *r); - void ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *value, - ngx_int_t *rc); - void ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, - ngx_str_t *out, - ngx_int_t *rc); + ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *value); + ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, + ngx_str_t *key, + ngx_str_t *out); ngx_int_t ngx_http_wasm_ffi_enable_property_setter(ngx_http_request_t *r); ngx_int_t ngx_http_wasm_ffi_enable_property_getter(ngx_http_request_t *r); ]] @@ -250,11 +248,8 @@ function _M.set_property(key, value) data = value, len = value and #value or 0 }) - local rcp = ffi_new("ngx_int_t[1]", { 0 }) - C.ngx_http_wasm_ffi_set_property(r, ckey, cvalue, rcp) - - local rc = rcp[0] + local rc = C.ngx_http_wasm_ffi_set_property(r, ckey, cvalue) if rc == FFI_ERROR then return nil, "unknown error" end @@ -279,11 +274,8 @@ function _M.get_property(key) local ckey = ffi_new("ngx_str_t", { data = key, len = #key }) local cvalue = ffi_new("ngx_str_t", { data = nil, len = 0 }) - local rcp = ffi_new("ngx_int_t[1]", { 0 }) - - C.ngx_http_wasm_ffi_get_property(r, ckey, cvalue, rcp) - local rc = tonumber(rcp[0]) + local rc = C.ngx_http_wasm_ffi_get_property(r, ckey, cvalue) if rc == FFI_ERROR then return nil, "unknown error", ERROR end diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index 430037367..8f022a4b4 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -196,34 +196,31 @@ get_pwctx(ngx_http_request_t *r) } -void +ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc) + ngx_str_t *key, ngx_str_t *value) { ngx_http_wasm_req_ctx_t *rctx; ngx_proxy_wasm_ctx_t *pwctx; if (ngx_http_wasm_rctx(r, &rctx) != NGX_OK) { - *rc = NGX_ERROR; - return; + return NGX_ERROR; } pwctx = ngx_proxy_wasm_ctx(NULL, 0, NGX_PROXY_WASM_ISOLATION_STREAM, &ngx_http_proxy_wasm, rctx); if (pwctx == NULL) { - *rc = NGX_ERROR; - return; + return NGX_ERROR; } - *rc = ngx_proxy_wasm_properties_set(pwctx, key, value); - return; + return ngx_proxy_wasm_properties_set(pwctx, key, value); } -void +ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc) + ngx_str_t *key, ngx_str_t *value) { ngx_http_wasm_req_ctx_t *rctx; ngx_proxy_wasm_ctx_t *pwctx; @@ -233,20 +230,17 @@ ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, * TODO: return code signifying "no plan attached to request" and co. * return associated constant as err/code from Lua lib */ - *rc = NGX_ERROR; - return; + return NGX_ERROR; } pwctx = ngx_proxy_wasm_ctx(NULL, 0, NGX_PROXY_WASM_ISOLATION_STREAM, &ngx_http_proxy_wasm, rctx); if (pwctx == NULL) { - *rc = NGX_ERROR; - return; + return NGX_ERROR; } - *rc = ngx_proxy_wasm_properties_get(pwctx, key, value); - return; + return ngx_proxy_wasm_properties_get(pwctx, key, value); } diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 26b5d97f4..889306263 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -45,10 +45,10 @@ ngx_int_t ngx_http_wasm_ffi_plan_load(ngx_wasm_ops_plan_t *plan); ngx_int_t ngx_http_wasm_ffi_plan_attach(ngx_http_request_t *r, ngx_wasm_ops_plan_t *plan, ngx_uint_t isolation); ngx_int_t ngx_http_wasm_ffi_start(ngx_http_request_t *r); -void ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc); -void ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, - ngx_str_t *key, ngx_str_t *value, ngx_int_t *rc); +ngx_int_t ngx_http_wasm_ffi_set_property(ngx_http_request_t *r, + ngx_str_t *key, ngx_str_t *value); +ngx_int_t ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, + ngx_str_t *key, ngx_str_t *value); ngx_int_t ngx_http_wasm_ffi_set_host_property(ngx_http_request_t *r, ngx_str_t *key, ngx_str_t *value, unsigned is_const, unsigned retrieve); ngx_int_t ngx_http_wasm_ffi_set_property_setter(ngx_http_request_t *r, diff --git a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t index fb053dce0..ac8f8786b 100644 --- a/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t +++ b/t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t @@ -398,7 +398,6 @@ ok === TEST 9: set_property_getter() - getter returning nil produces property not found ---- ONLY --- wasm_modules: hostcalls --- http_config init_worker_by_lua_block { From fb49309f53e2471dcd970f1c88b00c4730ab71e9 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Tue, 7 Nov 2023 19:34:47 -0300 Subject: [PATCH 10/11] [wip] use lua_resume directly! This makes other tests pass, but this is very WIP because it doesn't destroy the thread, etc. Some setup is going wrong when running this because this change produces segfaults with `r->connection->pool` being unset. --- src/common/lua/ngx_wasm_lua_ffi.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index 8f022a4b4..d3a6fe63e 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -336,8 +336,8 @@ property_handler(void *data, ngx_str_t *key, ngx_str_t *value) ngx_wasm_ffi_host_property_req_ctx_t hprctx; ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hpctx->r); lua_State *co; - ngx_int_t rc = NGX_OK; ngx_wasm_lua_ctx_t *lctx; + int rv; // FIXME if this function is changed to return NGX_AGAIN, // hprctx needs to be dynamically allocated: @@ -384,14 +384,21 @@ property_handler(void *data, ngx_str_t *key, ngx_str_t *value) /* run handler */ - rc = ngx_wasm_lua_thread_run(lctx); - if (rc == NGX_AGAIN) { + rv = lua_resume(lctx->co, lctx->nargs); + if (rv == LUA_YIELD) { ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, "cannot yield from host property handler"); return NGX_ERROR; } + if (rv != 0) { + ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, + "Lua resume failed with error: %d", rv); + + return NGX_ERROR; + } + // ngx_wasm_assert(rc == NGX_DONE); return hprctx.rc; From 7e803d9a026171743938ebcd64b05ae4f379d7e7 Mon Sep 17 00:00:00 2001 From: Hisham Muhammad Date: Wed, 8 Nov 2023 18:21:40 -0300 Subject: [PATCH 11/11] refactor(lua) simplify code --- src/common/lua/ngx_wasm_lua_ffi.c | 124 +++++++++++------------------- src/common/lua/ngx_wasm_lua_ffi.h | 17 ++-- 2 files changed, 51 insertions(+), 90 deletions(-) diff --git a/src/common/lua/ngx_wasm_lua_ffi.c b/src/common/lua/ngx_wasm_lua_ffi.c index d3a6fe63e..538edd2fc 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.c +++ b/src/common/lua/ngx_wasm_lua_ffi.c @@ -246,30 +246,30 @@ ngx_http_wasm_ffi_get_property(ngx_http_request_t *r, void ngx_wasm_lua_ffi_host_prop_handler( - ngx_wasm_ffi_host_property_req_ctx_t *hprctx, + ngx_wasm_ffi_host_property_ctx_t *hpctx, unsigned char ok, const char* new_value, size_t len, unsigned char is_const) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hprctx->r); - ngx_str_t *key = hprctx->key; - ngx_str_t *value = hprctx->value; + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hpctx->r); + ngx_str_t *key = hpctx->key; + ngx_str_t *value = hpctx->value; if (!ok) { if (new_value) { ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, "error in property handler: %s", new_value); - hprctx->rc = NGX_ERROR; + hpctx->rc = NGX_ERROR; return; - } else if (!hprctx->is_getter) { + } else if (!hpctx->is_getter) { ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, "error in property handler: unknown error"); - hprctx->rc = NGX_ERROR; + hpctx->rc = NGX_ERROR; return; } - hprctx->rc = NGX_DECLINED; + hpctx->rc = NGX_DECLINED; return; } @@ -282,28 +282,28 @@ ngx_wasm_lua_ffi_host_prop_handler( value->len = len; } - hprctx->rc = ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, - hprctx->is_getter); + hpctx->rc = ngx_proxy_wasm_properties_set_host(pwctx, key, value, is_const, + hpctx->is_getter); - if (hprctx->is_getter && hprctx->rc == NGX_OK && value->data == NULL) { - hprctx->rc = NGX_DECLINED; + if (hpctx->is_getter && hpctx->rc == NGX_OK && value->data == NULL) { + hpctx->rc = NGX_DECLINED; } - hprctx->rc = NGX_OK; + hpctx->rc = NGX_OK; } static const char *HOST_PROPERTY_SCRIPT_NAME = "wasm_lua_host_property_chunk"; static const char *HOST_PROPERTY_SCRIPT = "" - "local hprctx, is_getter, key, value = ... \n" + "local hpctx, is_getter, key, value = ... \n" "local ffi = require('ffi') \n" "local fmt = string.format \n" " \n" "ffi.cdef[[ \n" " typedef unsigned char u_char; \n" - " typedef void ngx_wasm_ffi_host_property_req_ctx_t; \n" + " typedef void ngx_wasm_ffi_host_property_ctx_t; \n" " \n" " void ngx_wasm_lua_ffi_host_prop_handler( \n" - " ngx_wasm_ffi_host_property_req_ctx_t *hprctx, \n" + " ngx_wasm_ffi_host_property_ctx_t *hpctx, \n" " u_char ok, const char* value, size_t len, \n" " u_char is_const); \n" "]] \n" @@ -324,34 +324,33 @@ static const char *HOST_PROPERTY_SCRIPT = "" " \n" "local new_value_len = new_value and #new_value or 0 \n" " \n" - " \n" - "ffi.C.ngx_wasm_lua_ffi_host_prop_handler(hprctx, ok and 1 or 0, \n" + "ffi.C.ngx_wasm_lua_ffi_host_prop_handler(hpctx, ok and 1 or 0, \n" " new_value, new_value_len, \n" " is_const and 1 or 0) \n"; static ngx_int_t -property_handler(void *data, ngx_str_t *key, ngx_str_t *value) +property_handler(ngx_proxy_wasm_ctx_t* pwctx, ngx_str_t *key, ngx_str_t *value, + unsigned is_getter) { - ngx_wasm_ffi_host_property_ctx_t *hpctx = data; - ngx_wasm_ffi_host_property_req_ctx_t hprctx; - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(hpctx->r); + ngx_http_wasm_req_ctx_t *rctx = pwctx->data; + ngx_wasm_ffi_host_property_ctx_t hpctx; lua_State *co; ngx_wasm_lua_ctx_t *lctx; int rv; // FIXME if this function is changed to return NGX_AGAIN, - // hprctx needs to be dynamically allocated: + // hpctx needs to be dynamically allocated: - hprctx.r = hpctx->r; - hprctx.is_getter = hpctx->is_getter; - hprctx.key = key; - hprctx.value = value; + hpctx.r = rctx->r; + hpctx.is_getter = is_getter; + hpctx.key = key; + hpctx.value = value; /* create coroutine */ lctx = ngx_wasm_lua_thread_new(HOST_PROPERTY_SCRIPT_NAME, HOST_PROPERTY_SCRIPT, - hpctx->env, pwctx->log, &hprctx, + &rctx->env, rctx->r->connection->log, &hpctx, NULL, NULL); if (lctx == NULL) { return NGX_ERROR; @@ -366,11 +365,11 @@ property_handler(void *data, ngx_str_t *key, ngx_str_t *value) lctx->nargs = 4; - lua_pushlightuserdata(co, &hprctx); - lua_pushboolean(co, hpctx->is_getter); + lua_pushlightuserdata(co, &hpctx); + lua_pushboolean(co, is_getter); lua_pushlstring(co, (char *) key->data, key->len); - if (hpctx->is_getter || value->data == NULL) { + if (is_getter || value->data == NULL) { lua_pushnil(co); } else { @@ -394,86 +393,55 @@ property_handler(void *data, ngx_str_t *key, ngx_str_t *value) if (rv != 0) { ngx_wavm_log_error(NGX_LOG_ERR, pwctx->log, NULL, - "Lua resume failed with error: %d", rv); + "Lua resume failed with error: code %d", rv); return NGX_ERROR; } - // ngx_wasm_assert(rc == NGX_DONE); - - return hprctx.rc; + return hpctx.rc; } -static ngx_wasm_ffi_host_property_ctx_t * -get_hpctx(ngx_http_request_t *r, unsigned is_getter) +static ngx_int_t +property_setter(void* pwctx, ngx_str_t *key, ngx_str_t *value) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); - ngx_wasm_ffi_host_property_ctx_t *hpctx; - ngx_http_wasm_req_ctx_t *rctx; - ngx_int_t rc; - - if (pwctx == NULL) { - return NULL; - } - - rc = ngx_http_wasm_rctx(r, &rctx); - ngx_wasm_assert(rc != NGX_DECLINED); - if (rc != NGX_OK) { - return NULL; - } - - hpctx = ngx_palloc(pwctx->pool, sizeof(ngx_wasm_ffi_host_property_ctx_t)); - if (hpctx == NULL) { - return NULL; - } + return property_handler((ngx_proxy_wasm_ctx_t *) pwctx, key, value, 0); +} - hpctx->r = r; - hpctx->env = &rctx->env; - hpctx->is_getter = is_getter; - return hpctx; +static ngx_int_t +property_getter(void* pwctx, ngx_str_t *key, ngx_str_t *value) +{ + return property_handler((ngx_proxy_wasm_ctx_t *) pwctx, key, value, 1); } ngx_int_t ngx_http_wasm_ffi_enable_property_setter(ngx_http_request_t *r) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); - ngx_wasm_ffi_host_property_ctx_t *hpctx; + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); if (pwctx == NULL) { return NGX_ERROR; } - hpctx = get_hpctx(r, 0); - if (hpctx == NULL) { - return NGX_ERROR; - } - return ngx_proxy_wasm_properties_set_host_prop_setter(pwctx, - property_handler, - hpctx); + property_setter, + pwctx); } ngx_int_t ngx_http_wasm_ffi_enable_property_getter(ngx_http_request_t *r) { - ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); - ngx_wasm_ffi_host_property_ctx_t *hpctx; + ngx_proxy_wasm_ctx_t *pwctx = get_pwctx(r); if (pwctx == NULL) { return NGX_ERROR; } - hpctx = get_hpctx(r, 1); - if (hpctx == NULL) { - return NGX_ERROR; - } - return ngx_proxy_wasm_properties_set_host_prop_getter(pwctx, - property_handler, - hpctx); + property_getter, + pwctx); } #endif diff --git a/src/common/lua/ngx_wasm_lua_ffi.h b/src/common/lua/ngx_wasm_lua_ffi.h index 889306263..9d9808da7 100644 --- a/src/common/lua/ngx_wasm_lua_ffi.h +++ b/src/common/lua/ngx_wasm_lua_ffi.h @@ -22,21 +22,14 @@ typedef struct { typedef struct { - ngx_http_request_t *r; - ngx_wasm_subsys_env_t *env; - unsigned is_getter; + ngx_http_request_t *r; + unsigned is_getter; + ngx_str_t *key; + ngx_str_t *value; + ngx_int_t rc; } ngx_wasm_ffi_host_property_ctx_t; -typedef struct { - ngx_http_request_t *r; - unsigned is_getter; - ngx_str_t *key; - ngx_str_t *value; - ngx_int_t rc; -} ngx_wasm_ffi_host_property_req_ctx_t; - - ngx_int_t ngx_http_wasm_ffi_plan_new(ngx_wavm_t *vm, ngx_wasm_ffi_filter_t *filters, size_t n_filters, ngx_wasm_ops_plan_t **out, u_char *err, size_t *errlen);