Merge pull request #531 from 3scale/url-rewriting-before-apicast-policy

Make it possible to place url rewriting before apicast in the policy chain

CHANGELOG.md

@@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ## Changed
 
 - Consolidate apicast-0.1-0.rockspec into apicast-scm-1.rockspec [PR #526](https://github.com/3scale/apicast/pull/526)
+- Deprecated `Configuration.extract_usage` in favor of `Service.get_usage` [PR #531](https://github.com/3scale/apicast/pull/531)
 
 ## [3.2.0-alpha2] - 2017-11-30
 

gateway/src/apicast/configuration.lua

@@ -3,15 +3,13 @@ local _M = {
 }
 
 local len = string.len
-local format = string.format
 local pairs = pairs
 local type = type
 local error = error
 local tostring = tostring
 local next = next
 local lower = string.lower
 local insert = table.insert
-local concat = table.concat
 local setmetatable = setmetatable
 local re_match = ngx.re.match
 
@@ -32,62 +30,10 @@ local function map(func, tbl)
   return newtbl
 end
 
-local function set_or_inc(t, name, delta)
-  return (t[name] or 0) + (delta or 0)
-end
-
 local function regexpify(path)
   return path:gsub('?.*', ''):gsub("{.-}", '([\\w_.-]+)'):gsub("%.", "\\.")
 end
 
-local function check_rule(req, rule, usage_t, matched_rules, params)
-  local pattern = rule.regexpified_pattern
-  local match = re_match(req.path, format("^%s", pattern), 'oj')
-
-  if match and req.method == rule.method then
-    local args = req.args
-
-    if rule.querystring_params(args) then -- may return an empty table
-      local system_name = rule.system_name
-      -- FIXME: this had no effect, what is it supposed to do?
-      -- when no querystringparams
-      -- in the rule. it's fine
-      -- for i,p in ipairs(rule.parameters or {}) do
-      --   param[p] = match[i]
-      -- end
-
-      local value = set_or_inc(usage_t, system_name, rule.delta)
-
-      usage_t[system_name] = value
-      params['usage[' .. system_name .. ']'] = value
-      insert(matched_rules, rule.pattern)
-    end
-  end
-end
-
-local function get_auth_params(method)
-  local params = ngx.req.get_uri_args()
-
-  if method == "GET" then
-    return params
-  else
-    ngx.req.read_body()
-    local body_params, err = ngx.req.get_post_args()
-
-    if not body_params then
-      ngx.log(ngx.NOTICE, 'Error while getting post args: ', err)
-      body_params = {}
-    end
-
-    -- Adds to body_params URI params that are not included in the body. Doing
-    -- the reverse would be more expensive, because in general, we expect the
-    -- size of body_params to be larger than the size of params.
-    setmetatable(body_params, { __index = params })
-
-    return body_params
-  end
-end
-
 local regex_variable = '\\{[-\\w_]+\\}'
 
 local function hash_to_array(hash)
@@ -212,26 +158,6 @@ function _M.parse_service(service)
         app_id = lower(proxy.auth_app_id or 'app_id'),
         app_key = lower(proxy.auth_app_key or 'app_key') -- TODO: use App-Key if location is headers
       },
-      extract_usage = function (config, request, _)
-        local req = re.split(request, " ", 'oj')
-        local method, url = req[1], req[2]
-        local path = re.split(url, "\\?", 'oj')[1]
-        local usage_t =  {}
-        local matched_rules = {}
-        local params = {}
-        local rules = config.rules
-
-        local args = get_auth_params(method)
-
-        ngx.log(ngx.DEBUG, '[mapping] service ', config.id, ' has ', #config.rules, ' rules')
-
-        for i = 1, #rules do
-          check_rule({path=path, method=method, args=args}, rules[i], usage_t, matched_rules, params)
-        end
-
-        -- if there was no match, usage is set to nil and it will respond a 404, this behavior can be changed
-        return usage_t, concat(matched_rules, ", "), params
-      end,
       rules = map(function(proxy_rule)
         local querystring_parameters = hash_to_array(proxy_rule.querystring_parameters)
 

gateway/src/apicast/configuration/service.lua

@@ -7,7 +7,12 @@ local tostring = tostring
 local rawget = rawget
 local lower = string.lower
 local gsub = string.gsub
+local format = string.format
 local select = select
+local concat = table.concat
+local insert = table.insert
+local re = require 'ngx.re'
+local re_match = ngx.re.match
 
 local http_authorization = require 'resty.http_authorization'
 
@@ -160,6 +165,58 @@ function backend_version_credentials.version_oauth(config)
   return setmetatable({ access_token, access_token = access_token }, credentials_oauth_mt)
 end
 
+local function set_or_inc(t, name, delta)
+  return (t[name] or 0) + (delta or 0)
+end
+
+local function check_rule(req, rule, usage_t, matched_rules, params)
+  local pattern = rule.regexpified_pattern
+  local match = re_match(req.path, format("^%s", pattern), 'oj')
+
+  if match and req.method == rule.method then
+    local args = req.args
+
+    if rule.querystring_params(args) then -- may return an empty table
+      local system_name = rule.system_name
+      -- FIXME: this had no effect, what is it supposed to do?
+      -- when no querystringparams
+      -- in the rule. it's fine
+      -- for i,p in ipairs(rule.parameters or {}) do
+      --   param[p] = match[i]
+      -- end
+
+      local value = set_or_inc(usage_t, system_name, rule.delta)
+
+      usage_t[system_name] = value
+      params['usage[' .. system_name .. ']'] = value
+      insert(matched_rules, rule.pattern)
+    end
+  end
+end
+
+local function get_auth_params(method)
+  local params = ngx.req.get_uri_args()
+
+  if method == "GET" then
+    return params
+  else
+    ngx.req.read_body()
+    local body_params, err = ngx.req.get_post_args()
+
+    if not body_params then
+      ngx.log(ngx.NOTICE, 'Error while getting post args: ', err)
+      body_params = {}
+    end
+
+    -- Adds to body_params URI params that are not included in the body. Doing
+    -- the reverse would be more expensive, because in general, we expect the
+    -- size of body_params to be larger than the size of params.
+    setmetatable(body_params, { __index = params })
+
+    return body_params
+  end
+end
+
 -- This table can be used with `table.concat` to serialize
 -- just the numeric keys, but also with `pairs` to iterate
 -- over just the non numeric keys (for query building).
@@ -196,4 +253,50 @@ function _M:oauth()
   end
 end
 
+local function extract_usage_v2(config, method, path)
+  local usage_t =  {}
+  local matched_rules = {}
+  local params = {}
+  local rules = config.rules
+
+  local args = get_auth_params(method)
+
+  ngx.log(ngx.DEBUG, '[mapping] service ', config.id, ' has ', #rules, ' rules')
+
+  for i = 1, #rules do
+    check_rule({path=path, method=method, args=args}, rules[i], usage_t, matched_rules, params)
+  end
+
+  -- if there was no match, usage is set to nil and it will respond a 404, this
+  -- behavior can be changed
+  return usage_t, concat(matched_rules, ", "), params
+end
+
+-- Deprecated
+function _M:extract_usage(request)
+  ngx.log(ngx.WARN, 'extract_usage is deprecated, please use get_usage(method, path)')
+  local req = re.split(request, " ", 'oj')
+  local method, url = req[1], req[2]
+  local path = re.split(url, "\\?", 'oj')[1]
+
+  return extract_usage_v2(self, method, path)
+end
+
+--- Get the usage associated with a request
+-- @tparam string method Method of the request (GET, POST, etc.)
+-- @tparam string path Path of the request
+function _M:get_usage(method, path)
+  -- This is a simple dispatcher. If it detects that the 'extract_usage' method
+  -- has been defined, it calls it. Otherwise, it calls the new version of the
+  -- method. This is done to keep backwards compatibility, because in previous
+  -- versions it was possible to ovewrite that method and expect to be called
+  -- from where get_usage is currently being called.
+
+  if self.extract_usage and self.extract_usage ~= _M.extract_usage then
+    return self:extract_usage(ngx.var.request)
+  else
+    return extract_usage_v2(self, method, path)
+  end
+end
+
 return _M

gateway/src/apicast/policy/find_service/policy.lua

@@ -27,8 +27,9 @@ end
 
 local function find_service_cascade(configuration, host)
   local found
-  local request = ngx.var.request
   local services = configuration:find_by_host(host)
+  local method = ngx.req.get_method()
+  local uri = ngx.var.uri
 
   for s=1, #services do
     local service = services[s]
@@ -38,7 +39,7 @@ local function find_service_cascade(configuration, host)
       if hosts[h] == host then
         local name = service.system_name or service.id
         ngx.log(ngx.DEBUG, 'service ', name, ' matched host ', hosts[h])
-        local usage, matched_patterns = service:extract_usage(request)
+        local usage, matched_patterns = service:get_usage(method, uri)
 
         if next(usage) and matched_patterns ~= '' then
           ngx.log(ngx.DEBUG, 'service ', name, ' matched patterns ', matched_patterns)

gateway/src/apicast/proxy.lua

@@ -236,8 +236,6 @@ function _M:call(service)
 end
 
 function _M:access(service)
-  local request = ngx.var.request -- NYI: return to lower frame
-
   ngx.var.secret_token = service.secret_token
 
   local credentials, err = service:extract_credentials()
@@ -247,7 +245,7 @@ function _M:access(service)
     return error_no_credentials(service)
   end
 
-  local _, matched_patterns, usage_params = service:extract_usage(request)
+  local _, matched_patterns, usage_params = service:get_usage(ngx.req.get_method(), ngx.var.uri)
   local cached_key = { service.id }
 
   -- remove integer keys for serialization

spec/configuration/service_spec.lua

@@ -180,4 +180,20 @@ describe('Service object', function()
 
     end)
   end)
+
+  describe(':get_usage', function()
+    describe('when the old and deprecated extract_usage method is defined', function()
+      it('is called. To keep backwards compatibility', function()
+        local service = Service.new({
+          extract_usage = function() return 42 end
+        })
+
+        -- Used in the code, need to initialize it.
+        ngx.var = { request = 'GET /' }
+
+        local usage = service:get_usage('GET', '/')
+        assert.equal(42, usage)
+      end)
+    end)
+  end)
 end)

spec/policy/find_service/policy_spec.lua

@@ -6,11 +6,14 @@ describe('find_service', function()
       it('finds the service by matching rules and stores it in the given context', function()
         require('apicast.configuration_store').path_routing = true
 
-        -- We access ngx.var.request and ngx.req.get_uri_args directly in the
-        -- code, so we need to mock them. We should probably try to avoid this
-        -- kind of coupling.
-        ngx.var = { request = 'GET /def HTTP/1.1' }
-        ngx.req = { get_uri_args = function() return {} end }
+        -- We access ngx.var.uri, ngx.req.get_method, and ngx.req.get_uri_args
+        -- directly in the code, so we need to mock them. We should probably
+        -- try to avoid this kind of coupling.
+        ngx.var = { uri = '/def' }
+        ngx.req = {
+          get_uri_args = function() return {} end,
+          get_method = function() return 'GET' end
+        }
 
         local find_service_policy = require('apicast.policy.find_service').new()
         local host = 'example.com'
@@ -55,8 +58,11 @@ describe('find_service', function()
       describe('and no rules are matched', function()
         it('finds a service for the host in the context and stores the service there', function()
           require('apicast.configuration_store').path_routing = true
-          ngx.var = { request = 'GET /abc HTTP/1.1' }
-          ngx.req = { get_uri_args = function() return {} end }
+          ngx.var = { uri = '/abc' }
+          ngx.req = {
+            get_uri_args = function() return {} end,
+            get_method = function() return 'GET' end
+          }
 
           local host = 'example.com'
           local find_service_policy = require('apicast.policy.find_service').new()
@@ -80,11 +86,14 @@ describe('find_service', function()
         end)
       end)
 
-      describe('and no rules are matches and there is not a service for the host', function()
+      describe('and no rules are matched and there is not a service for the host', function()
         it('stores nil in the service field of the given context', function()
           require('apicast.configuration_store').path_routing = true
-          ngx.var = { request = 'GET /abc HTTP/1.1' }
-          ngx.req = { get_uri_args = function() return {} end }
+          ngx.var = { uri = '/abc' }
+          ngx.req = {
+            get_uri_args = function() return {} end,
+            get_method = function() return 'GET' end
+          }
 
           local find_service_policy = require('apicast.policy.find_service').new()
           local configuration_store = require('apicast.configuration_store').new()

spec/proxy_spec.lua

@@ -49,7 +49,8 @@ describe('Proxy', function()
   describe(':access', function()
     local service
     before_each(function()
-      ngx.var = { backend_endpoint = 'http://localhost:1853' }
+      ngx.var = { backend_endpoint = 'http://localhost:1853', uri = '/a/uri' }
+      ngx.req = { get_method = function () return 'GET' end}
       service = Service.new({ extract_usage = function() end })
     end)