Routing: Fix issues with invalid metric update on routing policy.

Due to the changes in APIAP in 3Scale, a service can have multiple
backends(using routing policy) and multiple mapping rules can be hit and
it creates a bad behaviour around metrics reported to Apisonator.

This commit adds a ownership field on routing and mapping rules, where a
ownership can be used to filter the mapping rules that you want to
report. Here an example:

Routing policy:

```
path      backend           owner
----------------------------------
/foo/bar  http://1.1.1.1/   t1
/foo      http://2.2.2.2/   t2
```

Mapping rules:

```
path      metric        owner
-----------------------------------
/foo      hits          t2
/foo/bar  barMetric     t1
```

In this case, due to the request is going to /foo/bar the metric that
will be updated will be `barMetric` and it'll use `2.2.2.2` backend, the
mapping rules to match will filter by the owner, in this case, t2.

Signed-off-by: Eloy Coto <[email protected]>

CHANGELOG.md

@@ -20,7 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Add an option to enable keepalive_timeout on gateway [THREESCALE-2886](https://issues.jboss.org/browse/THREESCALE-2886) [PR #1106](https://github.com/3scale/APIcast/pull/1106)
 - Added a new replace path option in routing policy [THREESCALE-3512](https://issues.jboss.org/browse/THREESCALE-3512) [PR #1119](https://github.com/3scale/APIcast/pull/1119) [PR #1121](https://github.com/3scale/APIcast/pull/1121) [PR #1122](https://github.com/3scale/APIcast/pull/1122)
 - Added usage metrics on logging policy [PR #1126](https://github.com/3scale/APIcast/pull/1126), [THREESCALE-1234](https://issues.jboss.org/browse/THREESCALE-1234)
-
+- Added owner_id on mapping rule and routing_policy [THREESCALE-3623](https://issues.jboss.org/browse/THREESCALE-3623) [PR #1125](https://github.com/3scale/APIcast/pull/1125)
 
 ### Fixed
 

gateway/src/apicast/mapping_rule.lua

@@ -13,6 +13,11 @@ local re_match = ngx.re.match
 local insert = table.insert
 local re_gsub = ngx.re.gsub
 
+-- This is introduced by API as a product feature. There are two kinds of
+-- mapping rules owner: `BackendAPI` that means that is used by API as a
+-- product and `Proxy` that means that it's a normal mapping rule.
+local BackendAPIconst = "BackendApi"
+
 local _M = {
   any_method = "ANY"
 }
@@ -79,7 +84,7 @@ local function matches_uri(rule_pattern, uri)
   return re_match(uri, format("^%s", rule_pattern), 'oj')
 end
 
-local function new(http_method, pattern, params, querystring_params, metric, delta, last)
+local function new(http_method, pattern, params, querystring_params, metric, delta, last, owner_id, owner_type)
   local self = setmetatable({}, mt)
 
   local querystring_parameters = hash_to_array(querystring_params)
@@ -92,6 +97,11 @@ local function new(http_method, pattern, params, querystring_params, metric, del
   self.delta = delta
   self.last = last or false
 
+  if owner_type == BackendAPIconst then
+    self.owner_id = owner_id
+  end
+
+
   self.querystring_params = function(args)
     return matches_querystring_params(querystring_parameters, args)
   end
@@ -123,7 +133,9 @@ function _M.from_proxy_rule(proxy_rule)
     proxy_rule.querystring_parameters,
     proxy_rule.metric_system_name,
     proxy_rule.delta,
-    proxy_rule.last
+    proxy_rule.last,
+    tonumber(proxy_rule.owner_id),
+    proxy_rule.owner_type
   )
 end
 

gateway/src/apicast/mapping_rules_matcher.lua

@@ -52,4 +52,21 @@ function _M.matches(method, uri, args, rules)
   return false
 end
 
+function _M.clean_usage_by_owner_id(rules, owner_id)
+  local usage = Usage.new()
+  if not owner_id then
+      return usage
+  end
+
+  for _, rule in ipairs(rules) do
+    -- Checking that the rule has owner_id, if it does not have owner_id means
+    -- that does not belongs to a backendAPI.
+    if rule.owner_id and owner_id ~= rule.owner_id then
+      usage:add(rule.system_name, 0 - (rule.delta or 0))
+    end
+  end
+
+  return usage
+end
+
 return _M

gateway/src/apicast/policy/3scale_batcher/3scale_batcher.lua

@@ -200,6 +200,13 @@ end
 function _M:access(context)
   local backend = backend_client:new(context.service, http_ng_resty)
   local usage = context.usage
+
+  -- If routing policy changes the upstream and it only belongs to a specified
+  -- owner, we need to filter out the usage for APIs that are not used at all.
+  if context.route_upstream_usage_cleanup then
+    context:route_upstream_usage_cleanup(usage, ngx.ctx.matched_rules)
+  end
+
   local service = context.service
   local service_id = service.id
   local credentials = context.credentials

gateway/src/apicast/policy/routing/apicast-policy.json

@@ -158,6 +158,10 @@
               "type": "string",
               "description": "Liquid filter to modify the request path to the matched Upstream URL. When no specified, keep the original path"
             },
+            "owner_id": {
+              "type": "integer",
+              "description": "Value to only increment hits on the mapping rules owner by the same id. "
+            },
             "host_header": {
               "description": "Host for the Host header. When not specified, defaults to the host of the URL.",
               "type": "string"

gateway/src/apicast/policy/routing/routing.lua

@@ -3,6 +3,7 @@ local tab_insert = table.insert
 local tab_new = require('resty.core.base').new_tab
 
 local balancer = require('apicast.balancer')
+local mapping_rules_matcher = require('apicast.mapping_rules_matcher')
 local UpstreamSelector = require('upstream_selector')
 local Request = require('request')
 local Rule = require('rule')
@@ -36,19 +37,42 @@ function _M.new(config)
   return self
 end
 
-function _M:content(context)
+function _M:access(context)
+  -- All route definition needs to happen in the access phase to make sure that
+  -- the mapping rule with the owner_id happens before the APIcast policy and
+  -- metrics in APIcast rule can be updated correctly.
+  --
+  -- This can also make sense to have in the rewrite phase, but on that phase
+  -- headers are not available to read so some matcher will not work correctly.
+  --
   -- This should be moved to the place where the context is started, so other
   -- policies can use it.
   context.request = context.request or Request.new()
 
   -- Once request is in the context, we should move this to wherever the jwt is
   -- validated.
-  context.request:set_validated_jwt(context.jwt)
+  context.request:set_validated_jwt(context.jwt or {})
+
+  context.route_upstream = self.upstream_selector:select(self.rules, context)
 
-  local upstream = self.upstream_selector:select(self.rules, context)
+  -- this function substract the usage that does not match with the owner_id by
+  -- the matched_rules
+  context.route_upstream_usage_cleanup = function(self, usage, matched_rules)
+    if not self.route_upstream then
+      return
+    end
+
+    local usage_diff = mapping_rules_matcher.clean_usage_by_owner_id(
+      matched_rules , self.route_upstream:has_owner_id())
+    usage:merge(usage_diff)
+  end
 
-  if upstream then
-    upstream:call(context)
+end
+
+
+function _M:content(context)
+  if context.route_upstream then
+    context.route_upstream:call(context)
   else
     return nil, 'no upstream'
   end

gateway/src/apicast/policy/routing/rule.lua

@@ -68,6 +68,7 @@ function _M.new_from_config_rule(config_rule)
 
     self.host_header = config_rule.host_header
     self.condition = init_condition(config_rule.condition)
+    self.owner_id = tonumber(config_rule.owner_id)
     return self
   else
     return nil, 'failed to initialize upstream from url: ',

gateway/src/apicast/policy/routing/upstream_selector.lua

@@ -32,6 +32,11 @@ function _M.select(_, rules, context)
       if rule.host_header and rule.host_header ~= '' then
         upstream:use_host_header(rule.host_header)
       end
+
+      if rule.owner_id then
+        upstream:set_owner_id(rule.owner_id)
+      end
+
       if rule.replace_path then
         upstream:append_path(rule.replace_path:render(context))
         -- Set uri as nil if not will be appended to the upstream

gateway/src/apicast/proxy.lua

@@ -273,6 +273,7 @@ function _M:rewrite(service, context)
   context.usage:merge(usage)
 
   ctx.usage = context.usage
+  ctx.matched_rules = matched_rules
   ctx.credentials = credentials
 
   var.cached_key = concat(cached_key, ':')
@@ -302,8 +303,15 @@ end
 
 function _M:access(service, usage, credentials, ttl)
   local ctx = ngx.ctx
+  local final_usage = usage or ctx.usage
+
+  -- If routing policy changes the upstream and it only belongs to a specified
+  -- owner, we need to filter out the usage for APIs that are not used at all.
+  if ctx.context.route_upstream_usage_cleanup then
+    ctx.context:route_upstream_usage_cleanup(final_usage, ctx.matched_rules)
+  end
+  return self:authorize(service, final_usage, credentials or ctx.credentials, ttl or ctx.ttl)
 
-  return self:authorize(service, usage or ctx.usage, credentials or ctx.credentials, ttl or ctx.ttl)
 end
 
 local function response_codes_data(status)

gateway/src/apicast/upstream.lua

@@ -187,4 +187,12 @@ function _M:call(context)
     return exec(self)
 end
 
+function _M:set_owner_id(owner_id)
+  self.owner_id = owner_id
+end
+
+function _M:has_owner_id()
+  return self.owner_id
+end
+
 return _M

gateway/src/apicast/usage.lua

@@ -5,6 +5,7 @@
 local setmetatable = setmetatable
 local ipairs = ipairs
 local insert = table.insert
+local remove = table.remove
 
 local _M = {}
 
@@ -42,13 +43,32 @@ function _M:add(metric, value)
   end
 end
 
+-- Remove metric from the usage map.
+-- Note that this mutates self.
+-- @tparam string metrc Metric.
+function _M:remove_metric(metric)
+  local tablefind = function (tab,el)
+    for index, value in pairs(tab) do
+      if value == el then
+        return index
+      end
+    end
+  end
+
+  remove(self.metrics, tablefind(self.metrics, metric))
+  self.deltas[metric] = nil
+end
+
 --- Merge usages
 -- Merges two usages. This means that:
 --
 -- 1) When a metric appears in both usages, its delta is updated in self by
 --    adding the two values.
 -- 2) When a metric does not appear in self, it is added in self.
 --
+-- 3) If the metric added is negative and the result is negative or 0, metric
+-- will be deleted.
+--
 -- Note that this mutates self.
 -- @tparam another_usage Usage Usage.
 function _M:merge(another_usage)
@@ -58,6 +78,10 @@ function _M:merge(another_usage)
   for _, metric in ipairs(another_usage_metrics) do
     local delta = another_usage_deltas[metric]
     self:add(metric, delta)
+
+    if self.deltas[metric] <= 0 then
+      self:remove_metric(metric)
+    end
   end
 end
 

spec/policy/routing/routing_spec.lua

@@ -17,12 +17,13 @@ describe('Routing policy', function()
 
       it('calls call() on the upstream passing the context as param', function()
         local routing = RoutingPolicy.new()
-        routing.upstream_selector = upstream_selector
 
+        routing.upstream_selector = upstream_selector
+        routing:access(context)
         routing:content(context)
 
         assert.stub(upstream_selector.select).was_called_with(
-          upstream_selector, routing.rules, context
+          upstream_selector, routing.rules, {request=request}
         )
 
         assert.stub(upstream_that_matches.call).was_called_with(
@@ -41,12 +42,9 @@ describe('Routing policy', function()
       it('returns nil and the msg "no upstream"', function()
         local routing = RoutingPolicy.new()
         routing.upstream_selector = upstream_selector
-
+        routing:access(context)
         local res, err = routing:content(context)
 
-        assert.stub(upstream_selector.select).was_called_with(
-          upstream_selector, routing.rules, context
-        )
         assert.is_nil(res)
         assert.equals('no upstream', err)
       end)

spec/usage_spec.lua

@@ -65,6 +65,59 @@ describe('usage', function()
         assert.same({ a = 1 }, a_usage.deltas)
       end)
     end)
+
+    describe("when the given usage is negative", function()
+
+      local usage = Usage.new()
+
+      before_each(function()
+        usage = Usage.new()
+        usage:add("a", 3)
+      end)
+
+      it("keeps metrics if are positive", function()
+        local a_usage = Usage.new()
+        a_usage:add("a", -2)
+
+        usage:merge(a_usage)
+
+        assert.same({ a = 1 }, usage.deltas)
+        assert.same({ "a" }, usage.metrics)
+      end)
+
+      it("Delete metric if value is 0", function()
+        local a_usage = Usage.new()
+        a_usage:add("a", -3)
+
+        usage:merge(a_usage)
+
+        assert.same({  }, usage.deltas)
+        assert.same({ }, usage.metrics)
+      end)
+
+      it("Delete metric if value is equal or bellow 0", function()
+        local a_usage = Usage.new()
+        a_usage:add("a", -4)
+
+        usage:merge(a_usage)
+
+        assert.same({  }, usage.deltas)
+        assert.same({ }, usage.metrics)
+      end)
+
+      it("keep metrics value if one is bellow 0", function()
+        usage:add("b", 10)
+
+        local a_usage = Usage.new()
+        a_usage:add("a", -4)
+
+        usage:merge(a_usage)
+
+        assert.same({ b = 10 }, usage.deltas)
+        assert.same({ "b" }, usage.metrics)
+      end)
+
+    end)
   end)
 
   describe('.metrics', function()