Skip to content

Commit

Permalink
fix(proxy-wasm): handle nil vs. empty-string in host-driven properties
Browse files Browse the repository at this point in the history
  • Loading branch information
@hishamhm
hishamhm committed Oct 23, 2023
1 parent ebb2203 commit 08e4a5c
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 54 deletions.
32 changes: 21 additions & 11 deletions lib/resty/wasmx/proxy_wasm.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,15 @@ do
-- so we prepare these wrappers, which in turn call a Lua function.

local function store_c_value(r, ckey, cvalue, lvalue, is_const)
local value = tostring(lvalue)
cvalue.data = value
cvalue.len = #value
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,
Expand All @@ -100,7 +106,10 @@ do
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 = ffi_str(cvalue.data, cvalue.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)

Expand All @@ -115,10 +124,6 @@ do
return FFI_ERROR
end

if lvalue == nil then
return FFI_OK
end

return store_c_value(r, ckey, cvalue, lvalue, is_const)
end)

Expand All @@ -143,7 +148,12 @@ do
return FFI_DECLINED
end

return store_c_value(r, ckey, cvalue, lvalue, is_const)
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

Expand Down Expand Up @@ -310,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

Expand All @@ -320,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
Expand Down
51 changes: 33 additions & 18 deletions src/common/proxy_wasm/ngx_proxy_wasm_properties.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ typedef struct {
ngx_str_node_t sn;
ngx_str_t value;
unsigned is_const:1;
unsigned negative_cache:1;
} host_props_node_t;


Expand Down Expand Up @@ -860,7 +861,7 @@ ngx_proxy_wasm_properties_set_ngx(ngx_proxy_wasm_ctx_t *pwctx,

static ngx_int_t
ngx_proxy_wasm_properties_get_host(ngx_proxy_wasm_ctx_t *pwctx,
ngx_str_t *path, ngx_str_t *value)
ngx_str_t *path, ngx_str_t *value, unsigned *negative_cache)
{
host_props_node_t *hpn;
uint32_t hash;
Expand All @@ -883,6 +884,10 @@ ngx_proxy_wasm_properties_get_host(ngx_proxy_wasm_ctx_t *pwctx,
value->data = hpn->value.data;
value->len = hpn->value.len;

if (hpn->negative_cache) {
*negative_cache = 1;
}

return NGX_OK;
}

Expand All @@ -896,6 +901,7 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx,
{
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;
Expand All @@ -912,24 +918,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) {
if (hpn->is_const) {
return NGX_DECLINED;
}

ngx_rbtree_delete(&pwctx->host_props_tree, &hpn->sn.node);
}

return NGX_OK;
}

if (hpn) {
if (hpn->is_const) {
return NGX_DECLINED;
}

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 {
Expand All @@ -944,12 +945,20 @@ ngx_proxy_wasm_properties_set_host(ngx_proxy_wasm_ctx_t *pwctx,
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;
}
}

hpn->is_const = is_const;
hpn->value.len = value->len;
hpn->value.data = ngx_pstrdup(pwctx->pool, value);
if (hpn->value.data == NULL) {
return NGX_ERROR;
}
hpn->value.data = new_data;

if (new_entry) {
ngx_rbtree_insert(&pwctx->host_props_tree, &hpn->sn.node);
Expand Down Expand Up @@ -985,6 +994,7 @@ ngx_proxy_wasm_properties_get(ngx_proxy_wasm_ctx_t *pwctx,
ngx_uint_t key;
pwm2ngx_mapping_t *m;
ngx_int_t rc;
unsigned negative_cache = 0;

p.data = replace_nulls_by_dots(path, dotted_path_buf);
key = ngx_hash_key(p.data, p.len);
Expand Down Expand Up @@ -1014,8 +1024,13 @@ ngx_proxy_wasm_properties_get(ngx_proxy_wasm_ctx_t *pwctx,
/* host variable */

/* even if there is a getter, try reading const value first */
rc = ngx_proxy_wasm_properties_get_host(pwctx, &p, value);
rc = ngx_proxy_wasm_properties_get_host(pwctx, &p, value,
&negative_cache);
if (rc == NGX_OK) {
if (negative_cache) {
return NGX_DECLINED;
}

return NGX_OK;
}

Expand Down
46 changes: 41 additions & 5 deletions t/04-openresty/ffi/304-proxy_wasm_set_property_getter.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,18 @@ ok
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
Expand All @@ -488,7 +500,7 @@ ok
local function log_property(k)
local v = proxy_wasm.get_property(k)
ngx.log(ngx.INFO, k, ": ", v)
ngx.log(ngx.INFO, k, ": ", type(v), " \"", v, "\"")
end
-- four get_property operations,
Expand All @@ -499,19 +511,43 @@ ok
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: 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: 2[^\n]*?
[^\n]*?\[info\] [^\n]*? wasmx.const_property: 1[^\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: 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]
Expand Down
72 changes: 52 additions & 20 deletions t/04-openresty/ffi/305-proxy_wasm_set_property_setter.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,29 +467,31 @@ qr/^[^\n]*?\[error\] [^\n]*? error setting property: first wasmx property custom
local function setter(key, value)
counter = counter + 1
local new_value = value:upper() .. " " .. tostring(counter)
tbl[key] = new_value
if value and #value > 0 then
value = value:upper() .. " " .. tostring(counter)
end
ngx.log(ngx.INFO, "setting ", key, " to ", new_value)
tbl[key] = value
if key == "wasmx.dyn_property" then
return true, new_value
ngx.log(ngx.INFO, "setting ", key, " to ", value, ", counter: " .. counter)
elseif key == "wasmx.const_property" then
return true, new_value, true
end
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")
ngx.log(ngx.INFO, "getting ", key, " via getter, counter: ", counter)
local value = tbl[key]
if value then
return true, value .. " " .. tostring(counter)
if value and #value > 0 then
value = value .. " " .. tostring(counter)
end
return true, value
end
assert(proxy_wasm.set_property_getter(getter))
Expand All @@ -503,7 +505,7 @@ qr/^[^\n]*?\[error\] [^\n]*? error setting property: first wasmx property custom
local function log_property(k)
local v = proxy_wasm.get_property(k)
ngx.log(ngx.INFO, k, ": ", v)
ngx.log(ngx.INFO, k, ": ", type(v), " \"", v, "\"")
end
-- two set_property operations, each will hit the setter.
Expand All @@ -516,20 +518,50 @@ qr/^[^\n]*?\[error\] [^\n]*? error setting property: first wasmx property custom
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[^\n]*?
[^\n]*?\[info\] [^\n]*? wasmx.const_property: HELLO 1[^\n]*?
[^\n]*?\[info\] [^\n]*? setting wasmx.dyn_property to HI 2[^\n]*?
[^\n]*?\[info\] [^\n]*? getting wasmx.dyn_property via getter[^\n]*?
[^\n]*?\[info\] [^\n]*? wasmx.dyn_property: HI 2 3[^\n]*?
[^\n]*?\[info\] [^\n]*? wasmx.const_property: HELLO 1[^\n]*?
[^\n]*?\[info\] [^\n]*? getting wasmx.dyn_property via getter[^\n]*?
[^\n]*?\[info\] [^\n]*? wasmx.dyn_property: HI 2 4[^\n]*?$/
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]
Expand Down

0 comments on commit 08e4a5c

Please sign in to comment.