From 1e36a8a258dae5108149b2fbc4d63fce5de24964 Mon Sep 17 00:00:00 2001 From: An Tran Date: Wed, 27 May 2026 12:22:19 +1000 Subject: [PATCH 1/5] refactor(3scale_batcher): return error from handler functions --- .../src/apicast/policy/3scale_batcher/3scale_batcher.lua | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua b/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua index 844aa982d..e466438fc 100644 --- a/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua +++ b/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua @@ -203,7 +203,7 @@ function _M:access(context) if cached_auth then - handle_cached_auth(self, cached_auth, service, transaction) + return handle_cached_auth(self, cached_auth, service, transaction) else local formatted_usage = usage:format() local backend_res = backend:authorize(formatted_usage, credentials) @@ -218,12 +218,12 @@ function _M:access(context) end if backend_status == 200 then - handle_backend_ok(self, transaction) + return handle_backend_ok(self, transaction) elseif backend_status >= 400 and backend_status < 500 then - handle_backend_denied( + return handle_backend_denied( self, service, transaction, backend_status, backend_res.headers) else - handle_backend_error(self, service, transaction, cache_handler) + return handle_backend_error(self, service, transaction, cache_handler) end end end From 42faaf4b333c8ada81ef30965fe098d3574c509f Mon Sep 17 00:00:00 2001 From: An Tran Date: Wed, 27 May 2026 12:32:23 +1000 Subject: [PATCH 2/5] Fix potential race condition New keys can be added by concurrent requests. And keys read by get_keys() can be incremented by add() between read and delete operations, resulting in the loss of those increments. Therefore, moving get_keys() inside the lock to prevent a race condition as add() will block on the same service_id lock. --- gateway/src/apicast/policy/3scale_batcher/reports_batcher.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/reports_batcher.lua b/gateway/src/apicast/policy/3scale_batcher/reports_batcher.lua index 2d9e0f1c6..2fa7b3a7b 100644 --- a/gateway/src/apicast/policy/3scale_batcher/reports_batcher.lua +++ b/gateway/src/apicast/policy/3scale_batcher/reports_batcher.lua @@ -72,8 +72,6 @@ end function _M:get_all(service_id) local cached_reports = {} - local cached_report_keys = self.storage:get_keys() - local lock, new_lock_err = resty_lock:new(self.lock_shdict_name, lock_options) if not lock then ngx.log(ngx.ERR, 'failed to create lock: ', new_lock_err) @@ -86,6 +84,8 @@ function _M:get_all(service_id) return {} end + local cached_report_keys = self.storage:get_keys() + for _, key in ipairs(cached_report_keys) do local value = self.storage:get(key) local report, err = keys_helper.report_from_key_batched_report(key, value) From bfe33d232d85509a79b08397843593a7974dac78 Mon Sep 17 00:00:00 2001 From: An Tran Date: Wed, 27 May 2026 12:48:32 +1000 Subject: [PATCH 3/5] perf(3scale_batcher): avoid regex matching loop Each key is matched against all 4 regex pattern, credential type can be extract directly from key, so refactoring to avoid lookup loop. --- .../policy/3scale_batcher/keys_helper.lua | 56 ++++++++++++------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua b/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua index 21854ee5f..cc4eb0ef1 100644 --- a/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua +++ b/gateway/src/apicast/policy/3scale_batcher/keys_helper.lua @@ -1,5 +1,6 @@ local ipairs = ipairs local format = string.format +local str_find = string.find local insert = table.insert local concat = table.concat local sort = table.sort @@ -39,12 +40,25 @@ local function metrics_part_in_key(usage) end local regexes_report_key = { - [[service_id:(?[\w-]+),user_key:(?[\S-]+),metric:(?[\S-]+)]], - [[service_id:(?[\w-]+),access_token:(?[\S-]+),metric:(?[\S-]+)]], - [[service_id:(?[\w-]+),app_id:(?[\S-]+),app_key:(?[\S-]+),metric:(?[\S-]+)]], - [[service_id:(?[\w-]+),app_id:(?[\S-]+),metric:(?[\S-]+)]], + user_key= [[service_id:(?[\w-]+),user_key:(?[\S-]+),metric:(?[\S-]+)]], + access_token = [[service_id:(?[\w-]+),access_token:(?[\S-]+),metric:(?[\S-]+)]], + app_key = [[service_id:(?[\w-]+),app_id:(?[\S-]+),app_key:(?[\S-]+),metric:(?[\S-]+)]], + app_id = [[service_id:(?[\w-]+),app_id:(?[\S-]+),metric:(?[\S-]+)]], } + +local function detect_cred_type(key) + if str_find(key, 'user_key:', 1, true) then + return 'user_key' + elseif str_find(key, 'access_token:', 1, true) then + return 'access_token' + elseif str_find(key, 'app_key:', 1, true) then + return 'app_key' + elseif str_find(key, 'app_id:', 1, true) then + return 'app_id' + end +end + function _M.key_for_cached_auth(transaction) local service_part = format("service_id:%s", transaction.service_id) local creds_part = creds_part_in_key(transaction.credentials) @@ -61,24 +75,26 @@ function _M.key_for_batched_report(service_id, credentials, metric_name) end function _M.report_from_key_batched_report(key, value) - for _, regex in ipairs(regexes_report_key) do - local match = ngx_re.match(key, regex, 'oj') - - if match then - -- some credentials will be nil. - return { - service_id = match.service_id, - metric = match.metric, - user_key = match.user_key, - access_token = match.access_token, - app_id = match.app_id, - app_key = match.app_key, - value = value - } - end + local cred_type = detect_cred_type(key) + if not cred_type then + return nil, 'credentials not found' + end + + local match = ngx_re.match(key, regexes_report_key[cred_type], 'oj') + if not match then + return nil, "credentials not found" end - return nil, 'credentials not found' + -- some credentials will be nil. + return { + service_id = match.service_id, + metric = match.metric, + user_key = match.user_key, + access_token = match.access_token, + app_id = match.app_id, + app_key = match.app_key, + value = value + } end return _M From d4c3d8994f23be8ff3942bb7f2f04d4ec2c83900 Mon Sep 17 00:00:00 2001 From: An Tran Date: Wed, 27 May 2026 12:56:30 +1000 Subject: [PATCH 4/5] perf(3scale_batcher): replace regex split with string operations --- .../apicast/policy/3scale_batcher/auths_cache.lua | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua b/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua index e01364e71..d080207bb 100644 --- a/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua +++ b/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua @@ -1,7 +1,7 @@ local keys_helper = require('apicast.policy.3scale_batcher.keys_helper') -local re = require('ngx.re') -local re_split = re.split +local str_find = string.find +local str_sub = string.sub local setmetatable = setmetatable local format = string.format @@ -39,8 +39,15 @@ function _M:get(transaction) if not cached_value then return nil end - local split_val = re_split(cached_value, ':', 'oj', nil, 2) - return { status = tonumber(split_val[1]), rejection_reason = split_val[2] } + local colon = str_find(cached_value, ':', 1, true) + if colon then + return { + status = tonumber(str_sub(cached_value, 1, colon - 1)), + rejection_reason = str_sub(cached_value, colon + 1) + } + end + + return { status = tonumber(cached_value) } end --- Store an authorization in the cache. From 6e7835f5f9f0095ca8dbba50ce87ac07cbe3cf28 Mon Sep 17 00:00:00 2001 From: An Tran Date: Wed, 27 May 2026 13:12:47 +1000 Subject: [PATCH 5/5] perf(3scale_batcher): avoid table allocation --- .../policy/3scale_batcher/3scale_batcher.lua | 14 ++--- .../policy/3scale_batcher/auths_cache.lua | 8 +-- .../3scale_batcher/auths_cache_spec.lua | 61 +++++++++++++++---- 3 files changed, 60 insertions(+), 23 deletions(-) diff --git a/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua b/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua index e466438fc..7b726c576 100644 --- a/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua +++ b/gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua @@ -151,11 +151,11 @@ local function handle_backend_error(self, service, transaction, cache_handler) end end -local function handle_cached_auth(self, cached_auth, service, transaction) - if cached_auth.status == 200 then +local function handle_cached_auth(self, cached_status, rejection_reason, service, transaction) + if cached_status == 200 then self.reports_batcher:add(transaction) else - return error(service, cached_auth.rejection_reason) + return error(service, rejection_reason) end end @@ -197,13 +197,13 @@ function _M:access(context) ensure_timer_task_created(self, service_id, backend) - local cached_auth = self.auths_cache:get(transaction) - local auth_is_cached = (cached_auth and true) or false + local cached_status, rejection_reason = self.auths_cache:get(transaction) + local auth_is_cached = cached_status ~= nil metrics.update_cache_counters(auth_is_cached) - if cached_auth then - return handle_cached_auth(self, cached_auth, service, transaction) + if cached_status then + return handle_cached_auth(self, cached_status, rejection_reason, service, transaction) else local formatted_usage = usage:format() local backend_res = backend:authorize(formatted_usage, credentials) diff --git a/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua b/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua index d080207bb..e6b51bb71 100644 --- a/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua +++ b/gateway/src/apicast/policy/3scale_batcher/auths_cache.lua @@ -41,13 +41,11 @@ function _M:get(transaction) local colon = str_find(cached_value, ':', 1, true) if colon then - return { - status = tonumber(str_sub(cached_value, 1, colon - 1)), - rejection_reason = str_sub(cached_value, colon + 1) - } + return tonumber(str_sub(cached_value, 1, colon - 1)), + str_sub(cached_value, colon + 1) end - return { status = tonumber(cached_value) } + return tonumber(cached_value) end --- Store an authorization in the cache. diff --git a/spec/policy/3scale_batcher/auths_cache_spec.lua b/spec/policy/3scale_batcher/auths_cache_spec.lua index b0b3ad7b7..9c50b391d 100644 --- a/spec/policy/3scale_batcher/auths_cache_spec.lua +++ b/spec/policy/3scale_batcher/auths_cache_spec.lua @@ -25,8 +25,8 @@ describe('Auths cache', function() cache:set(transaction, auth_status) - local cached = cache:get(transaction) - assert.equals(auth_status, cached.status) + local cached_status = cache:get(transaction) + assert.equals(auth_status, cached_status) end) it('caches auth with app id + app key', function() @@ -35,8 +35,8 @@ describe('Auths cache', function() cache:set(transaction, auth_status) - local cached = cache:get(transaction) - assert.equals(auth_status, cached.status) + local cached_status = cache:get(transaction) + assert.equals(auth_status, cached_status) end) it('caches auth with access token', function() @@ -45,8 +45,8 @@ describe('Auths cache', function() cache:set(transaction, auth_status) - local cached = cache:get(transaction) - assert.equals(auth_status, cached.status) + local cached_status = cache:get(transaction) + assert.equals(auth_status, cached_status) end) it('caches auths with same usages but different order in the same key', function() @@ -65,8 +65,8 @@ describe('Auths cache', function() cache:set(transaction_with_order_1, auth_status) - local cached = cache:get(transaction_with_order_2) - assert.equals(auth_status, cached.status) + local cached_status = cache:get(transaction_with_order_2) + assert.equals(auth_status, cached_status) end) it('caches a rejection reason when given', function() @@ -77,15 +77,54 @@ describe('Auths cache', function() cache:set(transaction, not_authorized_status, rejection_reason) - local cached = cache:get(transaction) - assert.equals(not_authorized_status, cached.status) - assert.equals(rejection_reason, cached.rejection_reason) + local cached_status, cached_rejection_reason = cache:get(transaction) + assert.equals(not_authorized_status, cached_status) + assert.equals(rejection_reason, cached_rejection_reason) end) it('returns nil when something is not cached', function() local user_key = { user_key = 'uk' } local transaction = Transaction.new(service_id, user_key, usage) + assert.is_nil(cache:get(transaction)) end) + + it('returns status without rejection_reason when cached without one', function() + local user_key = { user_key = 'uk' } + local transaction = Transaction.new(service_id, user_key, usage) + + cache:set(transaction, auth_status) + + local cached_status, cached_rejection_reason = cache:get(transaction) + assert.equals(auth_status, cached_status) + assert.is_nil(cached_rejection_reason) + end) + + it('parses rejection reason containing colons', function() + local app_id_and_key = { app_id = 'an_id', app_key = 'a_key' } + local transaction = Transaction.new(service_id, app_id_and_key, usage) + local rejection_reason = 'reason:with:colons' + + cache:set(transaction, 409, rejection_reason) + + local cached_status, cached_rejection_reason = cache:get(transaction) + assert.equals(409, cached_status) + assert.equals(rejection_reason, cached_rejection_reason) + end) + + it('returns correct status for different HTTP status codes', function() + local user_key = { user_key = 'uk' } + + for _, status_code in ipairs({ 200, 403, 404, 409, 500 }) do + local tx_usage = Usage.new() + tx_usage:add('m_' .. status_code, 1) + local transaction = Transaction.new(service_id, user_key, tx_usage) + + cache:set(transaction, status_code) + + local cached_status, cached_rejection_reason = cache:get(transaction) + assert.equals(status_code, cached_status) + end + end) end)