diff --git a/CHANGELOG.md b/CHANGELOG.md index 419efc7ed..ae8fa0b3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Performance - Camel Policy: avoid unnecessary closure allocation per request [PR #1584](https://github.com/3scale/APIcast/pull/1584) - Logging Policy: construct all templates at init time [PR #1587](https://github.com/3scale/APIcast/pull/1587) +- Routing Policy: eliminate unnecessary TemplateString allocation per request [PR #1585](https://github.com/3scale/APIcast/pull/1585) ### Fixed - Correct FAPI header to `x-fapi-interaction-id` [PR #1557](https://github.com/3scale/APIcast/pull/1557) [THREESCALE-11957](https://issues.redhat.com/browse/THREESCALE-11957) diff --git a/gateway/src/apicast/policy/routing/routing.lua b/gateway/src/apicast/policy/routing/routing.lua index d7154ee29..196091c26 100644 --- a/gateway/src/apicast/policy/routing/routing.lua +++ b/gateway/src/apicast/policy/routing/routing.lua @@ -37,6 +37,16 @@ function _M.new(config) return self end +local function route_upstream_usage_cleanup(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 + 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 @@ -57,16 +67,7 @@ function _M:access(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 - + context.route_upstream_usage_cleanup = route_upstream_usage_cleanup end diff --git a/gateway/src/apicast/policy/routing/routing_operation.lua b/gateway/src/apicast/policy/routing/routing_operation.lua index 803e20ec7..7e8ed7662 100644 --- a/gateway/src/apicast/policy/routing/routing_operation.lua +++ b/gateway/src/apicast/policy/routing/routing_operation.lua @@ -6,20 +6,31 @@ -- to get the left operand instead of the operand itself. local setmetatable = setmetatable -local Operation = require('apicast.conditions.operation') +local match = ngx.re.match local TemplateString = require('apicast.template_string') local _M = {} local mt = { __index = _M } +local evaluate_func = { + ['=='] = function(left, right) return tostring(left) == tostring(right) end, + ['!='] = function(left, right) return tostring(left) ~= tostring(right) end, + + -- Implemented on top of ngx.re.match. Returns true when there is a match and + -- false otherwise. + ['matches'] = function(left, right) + return (match(tostring(left), tostring(right)) and true) or false + end +} + local function new(evaluate_left_side_func, op, value, value_type) local self = setmetatable({}, mt) self.evaluate_left_side_func = evaluate_left_side_func - self.op = op - self.value = value - self.value_type = value_type + self.evaluate_func = evaluate_func[op] + assert(self.evaluate_func, 'Unsupported operation: ' .. (op or 'nil')) + self.right_template = TemplateString.new(value, value_type or 'plain') return self end @@ -55,21 +66,18 @@ function _M.new_op_with_jwt_claim(jwt_claim_name, op, value, value_type) end function _M.new_op_with_liquid_templating(liquid_expression, op, value, value_type) + local template = TemplateString.new(liquid_expression or "", "liquid") local eval_left_func = function(context) - return TemplateString.new(liquid_expression or "" , "liquid"):render(context) + return template:render(context) end return new(eval_left_func, op, value, value_type) end function _M:evaluate(context) - local left_operand_val = self.evaluate_left_side_func(context) - - local op = Operation.new( - left_operand_val, 'plain', self.op, self.value, self.value_type - ) - - return op:evaluate(context) + local left = self.evaluate_left_side_func(context) + local right = self.right_template:render(context) + return self.evaluate_func(left, right) end return _M diff --git a/gateway/src/apicast/policy/routing/rule.lua b/gateway/src/apicast/policy/routing/rule.lua index b1721ecf5..264a93b50 100644 --- a/gateway/src/apicast/policy/routing/rule.lua +++ b/gateway/src/apicast/policy/routing/rule.lua @@ -74,8 +74,8 @@ function _M.new_from_config_rule(config_rule) self.owner_id = tonumber(config_rule.owner_id) return self else - return nil, 'failed to initialize upstream from url: ', - config_rule.url, ' err: ', err + return nil, 'failed to initialize upstream from url: ' .. + config_rule.url .. ' err: ' .. (err or 'unknown') end end diff --git a/spec/policy/routing/routing_operation_spec.lua b/spec/policy/routing/routing_operation_spec.lua index 5661964b1..cbd89e425 100644 --- a/spec/policy/routing/routing_operation_spec.lua +++ b/spec/policy/routing/routing_operation_spec.lua @@ -31,6 +31,46 @@ describe('RoutingOperation', function() assert.is_false(operation:evaluate(context)) end) + + it('evaluates != correctly when paths differ', function() + local operation = RoutingOperation.new_op_with_path('!=', '/expected') + + local context = { + request = { get_uri = function() return '/other' end } + } + + assert.is_true(operation:evaluate(context)) + end) + + it('evaluates != correctly when paths match', function() + local operation = RoutingOperation.new_op_with_path('!=', '/same') + + local context = { + request = { get_uri = function() return '/same' end } + } + + assert.is_false(operation:evaluate(context)) + end) + + it('evaluates matches true when regex matches', function() + local operation = RoutingOperation.new_op_with_path('matches', '^/api/.*') + + local context = { + request = { get_uri = function() return '/api/users' end } + } + + assert.is_true(operation:evaluate(context)) + end) + + it('evaluates matches false when regex does not match', function() + local operation = RoutingOperation.new_op_with_path('matches', '^/api/.*') + + local context = { + request = { get_uri = function() return '/other/path' end } + } + + assert.is_false(operation:evaluate(context)) + end) end) describe('when the operation involves a header', function() @@ -236,6 +276,12 @@ describe('RoutingOperation', function() end) + it('raises an error for unsupported operators', function() + assert.has_error(function() + RoutingOperation.new_op_with_path('unsupported_op', '/path') + end) + end) + it('can evaluate the right operand as liquid', function() -- Stub the available context to avoid depending on ngx.var.* stub(ngx_variable, 'available_context', function(context) return context end) diff --git a/spec/policy/routing/routing_spec.lua b/spec/policy/routing/routing_spec.lua index 7479ca120..990a6cc73 100644 --- a/spec/policy/routing/routing_spec.lua +++ b/spec/policy/routing/routing_spec.lua @@ -2,8 +2,67 @@ local RoutingPolicy = require('apicast.policy.routing') local UpstreamSelector = require('apicast.policy.routing.upstream_selector') local Request = require('apicast.policy.routing.request') local Upstream = require('apicast.upstream') +local mapping_rules_matcher = require('apicast.mapping_rules_matcher') describe('Routing policy', function() + describe('.access', function() + it('assigns route_upstream_usage_cleanup to the context', function() + local routing = RoutingPolicy.new() + local context = {} + routing:access(context) + + assert.is_function(context.route_upstream_usage_cleanup) + end) + + it('assigns the same function reference on every call', function() + local routing = RoutingPolicy.new() + local ctx1 = {} + local ctx2 = {} + routing:access(ctx1) + routing:access(ctx2) + + assert.equals(ctx1.route_upstream_usage_cleanup, ctx2.route_upstream_usage_cleanup) + end) + end) + + describe('route_upstream_usage_cleanup', function() + it('is a no-op when route_upstream is nil', function() + local routing = RoutingPolicy.new() + local context = {} + routing:access(context) + + assert.has_no_errors(function() + context:route_upstream_usage_cleanup({}, {}) + end) + end) + + it('calls clean_usage_by_owner_id and merges usage when route_upstream exists', function() + local routing = RoutingPolicy.new() + local context = {} + routing:access(context) + + local mock_owner_id = 42 + context.route_upstream = { + has_owner_id = function() return mock_owner_id end, + } + + local usage_diff = { some_metric = 1 } + stub(mapping_rules_matcher, 'clean_usage_by_owner_id').returns(usage_diff) + + local usage = { merge = function() end } + stub(usage, 'merge') + + local matched_rules = { 'rule1', 'rule2' } + + context:route_upstream_usage_cleanup(usage, matched_rules) + + assert.stub(mapping_rules_matcher.clean_usage_by_owner_id).was_called_with( + matched_rules, mock_owner_id + ) + assert.stub(usage.merge).was_called_with(usage, usage_diff) + end) + end) + describe('.content', function() describe('when there is an upstream that matches', function() local upstream_that_matches = Upstream.new('http://localhost')