From 7694bdb7c2370a5c048c85ef9cdc1b763a002c98 Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Sun, 3 May 2020 23:41:38 +0200 Subject: [PATCH] fix(pdk) ensure 'kong.ctx.plugin' is light-thread safe * Store namespace keys in `ngx.ctx` to ensure all dynamically generated namespaces are isolated on a per-request basis. * Introduce a new global API (which is a private API for use by core only) to explicitly delete a namespace. A note on the benefits of this new implementation: * By using a table to keep track of namespaces in `ngx.ctx`, we ensure that users of `ngx.ctx` cannot access the table namespaces, thus properly isolating it and avoiding polluting `ngx.ctx` for core and/or plugins. We can think of it as a double-pointer reference to the namespace. Each request gets a pre-allocated table for 4 namespaces (of course not limited to 4), with the assumption that most instances do not execute more than 4 plugins using `kong.ctx.plugin` at a time. * We ensure that namespace key references cannot be `nil` to ensure a safe usage from the core. * All namespace key references are still weak when assigned to a namespace, thus tying the lifetime of the namespace to that of its key reference. Similarly to the previous implementation, this is done to ensure we avoid potential memory leaks. That said, the `kong.ctx.plugin` namespace does use the plugin's conf for that purpose anymore (See 40dc14690240f62137cc360579940e926be9cd66), which alleviates such concerns in the current usage of this API. * All tables allocated for namespace key references and namespaces keys themselves will be released when `ngx.ctx` will be GC'ed. * We also ensure than `kong.ctx.*` returns `nil` when `ngx.ctx` is not supported ("init" phase). Co-Authored-By: tdelaune Fix #4379 Fix #5853 See #5851 See #5459 --- kong/global.lua | 27 ++++++++++- kong/pdk/ctx.lua | 61 +++++++++++++++++++++---- t/01-pdk/05-client/09-load-consumer.t | 1 - t/02-global/02-set-named-ctx.t | 65 ++++++++++++++++++++++++++- 4 files changed, 142 insertions(+), 12 deletions(-) diff --git a/kong/global.lua b/kong/global.lua index b3d98cccc1e8..0cb42c006406 100644 --- a/kong/global.lua +++ b/kong/global.lua @@ -51,11 +51,36 @@ function _GLOBAL.set_named_ctx(self, name, key) error("name cannot be an empty string", 2) end + if key == nil then + error("key cannot be nil", 2) + end + + if not self.ctx then + error("ctx PDK module not initialized", 2) + end + + self.ctx.__set_namespace(name, key) +end + + +function _GLOBAL.del_named_ctx(self, name) + if not self then + error("arg #1 cannot be nil", 2) + end + + if type(name) ~= "string" then + error("name must be a string", 2) + end + + if #name == 0 then + error("name cannot be an empty string", 2) + end + if not self.ctx then error("ctx PDK module not initialized", 2) end - self.ctx.keys[name] = key + self.ctx.__del_namespace(name) end diff --git a/kong/pdk/ctx.lua b/kong/pdk/ctx.lua index 353164ace197..cbc9918cbb35 100644 --- a/kong/pdk/ctx.lua +++ b/kong/pdk/ctx.lua @@ -4,6 +4,7 @@ local ngx = ngx +local ngx_get_phase = ngx.get_phase -- shared between all global instances @@ -11,6 +12,10 @@ local _CTX_SHARED_KEY = {} local _CTX_CORE_KEY = {} +-- dynamic namespaces, also shared between global instances +local _CTX_NAMESPACES_KEY = {} + + --- -- A table that has the lifetime of the current request and is shared between -- all plugins. It can be used to share data between several plugins in a given @@ -86,19 +91,58 @@ local _CTX_CORE_KEY = {} -- -- kong.log(value) -- "hello world" -- end -local function new(self) - local _CTX = { - -- those would be visible on the *.ctx namespace for now - -- TODO: hide them in a private table shared between this - -- module and the global.lua one - keys = setmetatable({}, { __mode = "v" }), - } +local function new(self) + local _CTX = {} local _ctx_mt = {} + local _ns_mt = { __mode = "v" } + + + local function get_namespaces(nctx) + local namespaces = nctx[_CTX_NAMESPACES_KEY] + if not namespaces then + -- 4 namespaces for request, i.e. ~4 plugins + namespaces = self.table.new(0, 4) + nctx[_CTX_NAMESPACES_KEY] = setmetatable(namespaces, _ns_mt) + end + + return namespaces + end + + + local function set_namespace(namespace, namespace_key) + local nctx = ngx.ctx + local namespaces = get_namespaces(nctx) + + local ns = namespaces[namespace] + if ns and ns == namespace_key then + return + end + + namespaces[namespace] = namespace_key + end + + + local function del_namespace(namespace) + local nctx = ngx.ctx + local namespaces = get_namespaces(nctx) + namespaces[namespace] = nil + end function _ctx_mt.__index(t, k) + if k == "__set_namespace" then + return set_namespace + + elseif k == "__del_namespace" then + return del_namespace + end + + if ngx_get_phase() == "init" then + return + end + local nctx = ngx.ctx local key @@ -109,7 +153,8 @@ local function new(self) key = _CTX_SHARED_KEY else - key = t.keys[k] + local namespaces = get_namespaces(nctx) + key = namespaces[k] end if key then diff --git a/t/01-pdk/05-client/09-load-consumer.t b/t/01-pdk/05-client/09-load-consumer.t index d092475a68c0..acc240ad403f 100644 --- a/t/01-pdk/05-client/09-load-consumer.t +++ b/t/01-pdk/05-client/09-load-consumer.t @@ -126,4 +126,3 @@ GET /t cannot load a consumer with an id that is not a uuid --- no_error_log [error] - diff --git a/t/02-global/02-set-named-ctx.t b/t/02-global/02-set-named-ctx.t index 7e2478190287..6cbed4df406f 100644 --- a/t/02-global/02-set-named-ctx.t +++ b/t/02-global/02-set-named-ctx.t @@ -80,7 +80,7 @@ marry -=== TEST 3: set_named_ctx() arbitrary namespaces can be discarded +=== TEST 3: set_named_ctx() arbitrary namespaces can be discarded via del_named_ctx() --- http_config eval: $t::Util::HttpConfig --- config location = /t { @@ -93,7 +93,7 @@ marry kong.ctx.custom.cats = "marry" ngx.say(kong.ctx.custom.cats) - kong_global.set_named_ctx(kong, "custom", nil) + kong_global.del_named_ctx(kong, "custom") ngx.say(kong.ctx.custom) } } @@ -181,3 +181,64 @@ GET /t ctx PDK module not initialized --- no_error_log [error] + + + +=== TEST 7: set_named_ctx() arbitrary namespaces are light-thread safe +--- http_config + init_by_lua_block { + kong_global = require "kong.global" + kong = kong_global.new() + kong_global.init_pdk(kong) + } +--- config + location = /lua { + content_by_lua_block { + local args = assert(ngx.req.get_uri_args()) + local thread_name = args.name + local value = args.val + + kong_global.set_named_ctx(kong, "thread", thread_name) + kong.ctx.thread.val = value + + ngx.sleep(0.1) + + ngx.say("thread ", thread_name, ": ", kong.ctx.thread.val) + } + } + + location = /t { + echo_subrequest_async GET '/lua' -q 'name=A&val=hello'; + echo_subrequest_async GET '/lua' -q 'name=B&val=world'; + } +--- request +GET /t +--- response_body +thread A: hello +thread B: world +--- no_error_log +[error] + + + +=== TEST 8: set_named_ctx() arbitrary namespaces invalid argument 'key' +--- http_config eval: $t::Util::HttpConfig +--- config + location = /t { + content_by_lua_block { + local kong_global = require "kong.global" + local kong = kong_global.new() + kong_global.init_pdk(kong) + + local pok, perr = pcall(kong_global.set_named_ctx, kong, "custom", nil) + if not pok then + ngx.say(perr) + end + } + } +--- request +GET /t +--- response_body +key cannot be nil +--- no_error_log +[error]