Perf: Batcher policy refactor

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
-    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)
@@ -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

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,13 @@ 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 tonumber(str_sub(cached_value, 1, colon - 1)),
+      str_sub(cached_value, colon + 1)
+  end
+
+  return tonumber(cached_value)
 end
 
 --- Store an authorization in the cache.

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:(?<service_id>[\w-]+),user_key:(?<user_key>[\S-]+),metric:(?<metric>[\S-]+)]],
-  [[service_id:(?<service_id>[\w-]+),access_token:(?<access_token>[\S-]+),metric:(?<metric>[\S-]+)]],
-  [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),app_key:(?<app_key>[\S-]+),metric:(?<metric>[\S-]+)]],
-  [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),metric:(?<metric>[\S-]+)]],
+  user_key= [[service_id:(?<service_id>[\w-]+),user_key:(?<user_key>[\S-]+),metric:(?<metric>[\S-]+)]],
+  access_token = [[service_id:(?<service_id>[\w-]+),access_token:(?<access_token>[\S-]+),metric:(?<metric>[\S-]+)]],
+  app_key = [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),app_key:(?<app_key>[\S-]+),metric:(?<metric>[\S-]+)]],
+  app_id = [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),metric:(?<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

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)

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)