From 4bf980a3afd35d8f79ba1a25c75226aa418047a7 Mon Sep 17 00:00:00 2001 From: samugi Date: Thu, 31 Aug 2023 12:04:20 +0200 Subject: [PATCH] feat(request-aware-table): enable checks when log_level is debug --- CHANGELOG/unreleased/kong/11017.yaml | 5 + kong/tools/request_aware_table.lua | 18 ++- spec/01-unit/28-request-aware-table_spec.lua | 51 +++---- .../05-proxy/31-request-aware-table_spec.lua | 127 ++++++++---------- .../plugins/request-aware-table/handler.lua | 17 +-- 5 files changed, 107 insertions(+), 111 deletions(-) create mode 100644 CHANGELOG/unreleased/kong/11017.yaml diff --git a/CHANGELOG/unreleased/kong/11017.yaml b/CHANGELOG/unreleased/kong/11017.yaml new file mode 100644 index 000000000000..6ce700d75fab --- /dev/null +++ b/CHANGELOG/unreleased/kong/11017.yaml @@ -0,0 +1,5 @@ +message: Add a request-aware table able to detect accesses from different requests. +type: feature +scope: Core +prs: + - 11017 diff --git a/kong/tools/request_aware_table.lua b/kong/tools/request_aware_table.lua index bc7ef1828025..8226fe3cf9f8 100644 --- a/kong/tools/request_aware_table.lua +++ b/kong/tools/request_aware_table.lua @@ -1,5 +1,8 @@ local table_clear = require "table.clear" +local LOG_LEVELS = require "kong.constants".LOG_LEVELS +local get_log_level = require("resty.kong.log").get_log_level + local get_phase = ngx.get_phase local fmt = string.format local ngx_var = ngx.var @@ -16,15 +19,18 @@ local NGX_VAR_PHASES = { } +local function is_debug_mode() + local log_level = get_log_level(LOG_LEVELS[kong.configuration.log_level]) + return log_level == LOG_LEVELS.debug +end + + --- Request aware table constructor -- Wraps an existing table (or creates a new one) with request-aware access -- logic to protect the underlying data from race conditions. -- @param data_table (optional) The target table to use as underlying data --- @param request_awareness_mode (optional) The mode for request awareness --- - "off": No request awareness mode --- - any other value, or nil: Control access for every r/w operation -- @return The newly created table with request-aware access -local function new(data_table, request_awareness_mode) +local function new(data_table) if data_table and type(data_table) ~= "table" then error("data_table must be a table", 2) end @@ -65,7 +71,7 @@ local function new(data_table, request_awareness_mode) local _proxy_mt = { __index = function(_, k) - if request_awareness_mode ~= "off" then + if is_debug_mode() then enforce_sequential_access() end @@ -73,7 +79,7 @@ local function new(data_table, request_awareness_mode) end, __newindex = function(_, k, v) - if request_awareness_mode ~= "off" then + if is_debug_mode() then enforce_sequential_access() end diff --git a/spec/01-unit/28-request-aware-table_spec.lua b/spec/01-unit/28-request-aware-table_spec.lua index b82456b65a1f..3c3344c33e88 100644 --- a/spec/01-unit/28-request-aware-table_spec.lua +++ b/spec/01-unit/28-request-aware-table_spec.lua @@ -3,6 +3,7 @@ local tablex = require "pl.tablex" local rat describe("Request aware table", function() + local orig_t = {} local old_ngx local tab @@ -12,6 +13,11 @@ describe("Request aware table", function() get_phase = function() return "access" end, var = {}, } + _G.kong = { + configuration = { + log_level = "debug", + }, + } rat = require "kong.tools.request_aware_table" end) @@ -19,34 +25,29 @@ describe("Request aware table", function() _G.ngx = old_ngx end) - describe("with concurrency check enabled", function() - local orig_t = {} + before_each(function() + tab = rat.new(orig_t) + end) - before_each(function() - tab = rat.new(orig_t, "on") - end) + it("allows defining a custom clear function", function() + orig_t.persist = "persistent_value" + orig_t.foo = "bar" + orig_t.baz = "qux" - it("allows defining a custom clear function", function() - orig_t.persist = "persistent_value" - orig_t.foo = "bar" - orig_t.baz = "qux" - - -- custom clear function that keeps persistent_value - tab.clear(function(t) - for k in pairs(t) do - if k ~= "persist" then - t[k] = nil - end + -- custom clear function that keeps persistent_value + tab.clear(function(t) + for k in pairs(t) do + if k ~= "persist" then + t[k] = nil end - end) - - -- confirm persistent_value is the only key left - assert.same(1, tablex.size(orig_t)) - assert.equal("persistent_value", tab.persist) - - -- clear the whole table and confirm it's empty - tab.clear() - assert.same(0, tablex.size(orig_t)) + end end) + + -- confirm persistent_value is the only key left + assert.same(1, tablex.size(orig_t)) + assert.equal("persistent_value", tab.persist) + -- clear the whole table and confirm it's empty + tab.clear() + assert.same(0, tablex.size(orig_t)) end) end) diff --git a/spec/02-integration/05-proxy/31-request-aware-table_spec.lua b/spec/02-integration/05-proxy/31-request-aware-table_spec.lua index ca34bf9eedf8..42d4a80b88cf 100644 --- a/spec/02-integration/05-proxy/31-request-aware-table_spec.lua +++ b/spec/02-integration/05-proxy/31-request-aware-table_spec.lua @@ -1,10 +1,15 @@ local helpers = require "spec.helpers" +local LOG_LEVELS = { + "debug", + "info", + -- any other log level behaves the same as "info" +} -local function clear_table(client, checks) + +local function clear_table(client) local res = client:get("/", { query = { - checks = checks, clear = true, } }) @@ -12,83 +17,73 @@ local function clear_table(client, checks) assert.logfile().has.no.line("[error]", true) end -for _, checks in ipairs({ true, false }) do -for _, strategy in helpers.each_strategy() do - describe("request aware table tests [#" .. strategy .. "] .. checks=" .. tostring(checks), function() - local client - lazy_setup(function() - local bp = helpers.get_db_utils(strategy, { - "plugins", - "routes", - "services", - }, { - "request-aware-table" - }) +for _, log_level in ipairs(LOG_LEVELS) do + local concurrency_checks = log_level == "debug" - local service = assert(bp.services:insert({ - url = helpers.mock_upstream_url - })) + for _, strategy in helpers.each_strategy() do + describe("request aware table tests [#" .. strategy .. "] concurrency checks: " .. tostring(concurrency_checks), function() + local client + lazy_setup(function() + local bp = helpers.get_db_utils(strategy, { + "plugins", + "routes", + "services", + }, { + "request-aware-table" + }) - local route = bp.routes:insert({ - service = service, - paths = { "/" } - }) + local service = assert(bp.services:insert({ + url = helpers.mock_upstream_url + })) - bp.plugins:insert({ - name = "request-aware-table", - route = { id = route.id }, - }) + local route = bp.routes:insert({ + service = service, + paths = { "/" } + }) - helpers.start_kong({ - database = strategy, - plugins = "bundled, request-aware-table", - nginx_conf = "spec/fixtures/custom_nginx.template", - }) - end) + bp.plugins:insert({ + name = "request-aware-table", + route = { id = route.id }, + }) - lazy_teardown(function() - helpers.stop_kong() - end) + helpers.start_kong({ + database = strategy, + plugins = "bundled, request-aware-table", + nginx_conf = "spec/fixtures/custom_nginx.template", + log_level = log_level, + }) + end) - before_each(function() - helpers.clean_logfile() - client = helpers.proxy_client() - clear_table(client, checks) - end) + lazy_teardown(function() + helpers.stop_kong() + end) - after_each(function() - if client then - client:close() - end - end) + before_each(function() + helpers.clean_logfile() + client = helpers.proxy_client() + clear_table(client) + end) + + after_each(function() + if client then + client:close() + end + end) - describe("with concurrency check enabled", function() it("allows access when there are no race conditions", function() - local res = client:get("/", { - query = { - checks = checks, - } - }) + local res = client:get("/") assert.response(res).has.status(200) assert.logfile().has.no.line("[error]", true) end) it("denies access when there are race conditions and checks are enabled", function() -- access from request 1 (don't clear) - local ok, r = pcall(client.get, client, "/", { - query = { - checks = checks, - }, - }) + local ok, r = pcall(client.get, client, "/") assert(ok) assert.response(r).has.status(200) -- access from request 2 - ok, r = pcall(client.get, client, "/", { - query = { - checks = checks, - }, - }) - if checks then + ok, r = pcall(client.get, client, "/") + if concurrency_checks then assert(not ok) assert.logfile().has.line("race condition detected", true) else @@ -100,22 +95,16 @@ for _, strategy in helpers.each_strategy() do -- access from request 1 (clear) local r = client:get("/", { query = { - checks = checks, clear = true, }, }) assert.response(r).has.status(200) -- access from request 2 - r = client:get("/", { - query = { - checks = checks, - }, - }) + r = client:get("/") assert.response(r).has.status(200) assert.logfile().has.no.line("[error]", true) end) end) - end) -end + end end \ No newline at end of file diff --git a/spec/fixtures/custom_plugins/kong/plugins/request-aware-table/handler.lua b/spec/fixtures/custom_plugins/kong/plugins/request-aware-table/handler.lua index c51c6aa29b6c..c75f94e22233 100644 --- a/spec/fixtures/custom_plugins/kong/plugins/request-aware-table/handler.lua +++ b/spec/fixtures/custom_plugins/kong/plugins/request-aware-table/handler.lua @@ -10,29 +10,24 @@ local _M = { VERSION = "1.0", } -local checks_tab = RAT.new({}, "on") -local no_checks_tab = RAT.new({}, "off") +local tab = RAT.new({}) local function access_tables() local query = kong.request.get_query() - local tab - - if query.checks ~= "false" then - tab = checks_tab - else - tab = no_checks_tab - end if query.clear == "true" and get_phase() == "access" then + -- clear before access tab.clear() end -- write access tab.foo = "bar" - -- read access - ngx.log(ngx.DEBUG, "accessing to tab.foo" .. tab.foo) + tab.bar = "baz" + -- read/write access + tab.baz = tab.foo .. tab.bar if query.clear == "true" and get_phase() == "body_filter" then + -- clear after access tab.clear() end end