From f4fb4d63b0136371b89929685e152dfd98a03576 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 3 Aug 2018 11:12:37 +0200 Subject: [PATCH 1/5] policy/conditional: extract 'condition' in schema to 'definitions' --- .../apicast/policy/conditional/apicast-policy.json | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/gateway/src/apicast/policy/conditional/apicast-policy.json b/gateway/src/apicast/policy/conditional/apicast-policy.json index 50c9317b4..35aea8c3e 100644 --- a/gateway/src/apicast/policy/conditional/apicast-policy.json +++ b/gateway/src/apicast/policy/conditional/apicast-policy.json @@ -55,12 +55,11 @@ } }, "required": ["left", "op", "right"] - } - }, - "properties": { + }, "condition": { - "description": "conditions to be evaluated", + "$id": "#/definitions/condition", "type": "object", + "description": "condition to be evaluated", "properties": { "operations": { "type": "array", @@ -75,6 +74,11 @@ "default": "and" } } + } + }, + "properties": { + "condition": { + "$ref": "#/definitions/condition" }, "policy_chain": { "description": "the policy chain to execute when the condition is true", From e875a312b9722d1f2c0869945942a44a73838809 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 3 Aug 2018 11:19:30 +0200 Subject: [PATCH 2/5] policy/rate_limit: add conditions to keys in config schema --- .../policy/rate_limit/apicast-policy.json | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/gateway/src/apicast/policy/rate_limit/apicast-policy.json b/gateway/src/apicast/policy/rate_limit/apicast-policy.json index bc6d5fa38..721ac37d1 100644 --- a/gateway/src/apicast/policy/rate_limit/apicast-policy.json +++ b/gateway/src/apicast/policy/rate_limit/apicast-policy.json @@ -54,6 +54,72 @@ "enum": ["log"], "description": "Let the request go through and only output logs" }] + }, + "operation": { + "type": "object", + "$id": "#/definitions/operation", + "properties": { + "left": { + "type": "string" + }, + "op": { + "type": "string", + "enum": ["==", "!="] + }, + "right": { + "type": "string" + }, + "left_type": { + "description": "How to evaluate 'left'", + "type": "string", + "default": "plain", + "oneOf": [ + { + "enum": ["plain"], + "title": "Evaluate 'left' as plain text." + }, + { + "enum": ["liquid"], + "title": "Evaluate 'left' as liquid." + } + ] + }, + "right_type": { + "description": "How to evaluate 'right'", + "type": "string", + "default": "plain", + "oneOf": [ + { + "enum": ["plain"], + "title": "Evaluate 'right' as plain text." + }, + { + "enum": ["liquid"], + "title": "Evaluate 'right' as liquid." + } + ] + } + }, + "required": ["left", "op", "right"] + }, + "condition": { + "$id": "#/definitions/condition", + "type": "object", + "description": "condition to be evaluated", + "properties": { + "operations": { + "type": "array", + "items": { + "$ref": "#/definitions/operation" + }, + "minItems": 1 + }, + "combine_op": { + "type": "string", + "enum": ["and", "or"], + "default": "and" + } + } } }, "properties": { @@ -65,6 +131,9 @@ "key": { "$ref": "#/definitions/key" }, + "condition": { + "$ref": "#/definitions/condition" + }, "conn": { "type": "integer", "description": "The maximum number of concurrent requests allowed", @@ -91,6 +160,9 @@ "key": { "$ref": "#/definitions/key" }, + "condition": { + "$ref": "#/definitions/condition" + }, "rate": { "type": "integer", "description": "The specified request rate (number per second) threshold", @@ -112,6 +184,9 @@ "key": { "$ref": "#/definitions/key" }, + "condition": { + "$ref": "#/definitions/condition" + }, "count": { "type": "integer", "description": "The specified number of requests threshold", From e60c4597fd283901fbf51248ca0c60a0cf4b4cf7 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 10 Aug 2018 11:31:26 +0200 Subject: [PATCH 3/5] policy/rate_limit: add conditions --- .../apicast/policy/rate_limit/rate_limit.lua | 72 ++++++-- spec/policy/rate_limit/rate_limit_spec.lua | 158 ++++++++++++++++++ 2 files changed, 213 insertions(+), 17 deletions(-) diff --git a/gateway/src/apicast/policy/rate_limit/rate_limit.lua b/gateway/src/apicast/policy/rate_limit/rate_limit.lua index 12dab57f6..140833a18 100644 --- a/gateway/src/apicast/policy/rate_limit/rate_limit.lua +++ b/gateway/src/apicast/policy/rate_limit/rate_limit.lua @@ -10,6 +10,9 @@ local limit_traffic = require "resty.limit.traffic" local ngx_variable = require ('apicast.policy.ngx_variable') local redis_shdict = require('redis_shdict') +local Condition = require('apicast.policy.conditional.condition') +local Operation = require('apicast.policy.conditional.operation') + local tonumber = tonumber local next = next local shdict_key = 'limiter' @@ -79,6 +82,29 @@ local function init_error_settings(limits_exceeded_error, configuration_error) return error_settings end +local function build_operations(config_ops) + local res = {} + + for _, op in ipairs(config_ops or {}) do + insert(res, Operation.new(op.left, op.left_type, op.op, op.right, op.right_type)) + end + + return res +end + +local function build_condition(config_condition) + local cond = config_condition or {} + + return Condition.new( + build_operations(cond.operations), + cond.combine_op + ) +end + +local function liquid_context(policy_context) + return ngx_variable.available_context(policy_context) +end + local function build_limiters_and_keys(type, limiters, redis, error_settings, context) local res_limiters = {} local res_keys = {} @@ -94,6 +120,12 @@ local function build_limiters_and_keys(type, limiters, redis, error_settings, co lim.dict = redis or lim.dict + lim.condition_is_true = function(policy_context) + local condition = build_condition(limiter.condition) + local liquid_ctx = liquid_context(policy_context) + return condition:evaluate(liquid_ctx) + end + insert(res_limiters, lim) local key = limiter.template_string:render( @@ -122,6 +154,28 @@ local function build_templates(limiters) end end +local function select_condition_is_true(groups_of_limiters, groups_of_keys, policies_ctx) + local res_limiters = {} + local res_keys = {} + + for i = 1, #groups_of_limiters do + local limiters = groups_of_limiters[i] + local keys = groups_of_keys[i] + + for j = 1, #limiters do + local limiter = limiters[j] + local key = keys[j] + + if limiter.condition_is_true(policies_ctx) then + insert(res_limiters, limiter) + insert(res_keys, key) + end + end + end + + return res_limiters, res_keys +end + function _M.new(config) local self = new() self.config = config or {} @@ -160,25 +214,9 @@ function _M:access(context) local fixed_window_limiters, fixed_window_keys = build_limiters_and_keys( 'fixed_window', self.fixed_window_limiters, red, self.error_settings, context) - local limiters = {} local limiter_groups = { conn_limiters, leaky_bucket_limiters, fixed_window_limiters } - for _, limiter_group in ipairs(limiter_groups) do - if #limiter_group > 0 then - for _, limiter in ipairs(limiter_group) do - insert(limiters, limiter) - end - end - end - - local keys = {} local keys_groups = { conn_keys, leaky_bucket_keys, fixed_window_keys } - for _, keys_group in ipairs(keys_groups) do - if #keys_group > 0 then - for _, key in ipairs(keys_group) do - insert(keys, key) - end - end - end + local limiters, keys = select_condition_is_true(limiter_groups, keys_groups, context) local states = {} local connections_committed = {} diff --git a/spec/policy/rate_limit/rate_limit_spec.lua b/spec/policy/rate_limit/rate_limit_spec.lua index 596c54bcc..2bb614fbd 100644 --- a/spec/policy/rate_limit/rate_limit_spec.lua +++ b/spec/policy/rate_limit/rate_limit_spec.lua @@ -313,6 +313,164 @@ describe('Rate limit policy', function() assert.spy(ngx.sleep).was_called_with(match.is_gt(0.001)) end) end) + + describe('when conditions are defined', function() + local true_condition = { + operations = { + { left = '1', op = '==', right = '1' } + } + } + + local false_condition = { + operations = { + { left = '1', op = '==', right = '2' } + } + } + + describe('and the type of the limit is "connection"', function() + it('applies the limit when the condition is true', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + conn = 1, burst = 0, delay = 0.5, + condition = true_condition + } + } + }) + + assert(rate_limit_policy:access(context)) + + assert.returns_error('limits exceeded', rate_limit_policy:access(context)) + assert.spy(ngx.exit).was_called_with(429) + end) + + it('does not apply the limit when the condition is false', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + conn = 1, burst = 0, delay = 0.5, + condition = false_condition + } + } + }) + + -- Limit is 1. Make 2 requests and check that they're not limited + assert(rate_limit_policy:access(context)) + assert(rate_limit_policy:access(context)) + end) + end) + + describe('and the type of the limit is "leaky_bucket"', function() + it('applies the limit when the condition is true', function() + local rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + rate = 1, + burst = 0, + condition = true_condition + } + } + }) + + assert(rate_limit_policy:access(context)) + + assert.returns_error('limits exceeded', rate_limit_policy:access(context)) + assert.spy(ngx.exit).was_called_with(429) + end) + + it('does not apply the limit when the condition is false', function() + local rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + rate = 1, + burst = 0, + condition = false_condition + } + } + }) + + -- Limit is 1. Make 2 requests and check that they're not limited + assert(rate_limit_policy:access(context)) + assert(rate_limit_policy:access(context)) + end) + end) + + describe('and the type of the limits is "fixed_window"', function() + it('applies the limit when the condition is true', function() + local rate_limit_policy = RateLimitPolicy.new({ + fixed_window_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + count = 1, + window = 10, + condition = true_condition + } + } + }) + + assert(rate_limit_policy:access(context)) + + assert.returns_error('limits exceeded', rate_limit_policy:access(context)) + assert.spy(ngx.exit).was_called_with(429) + end) + + it('does not apply the limit when the condition is false', function() + local rate_limit_policy = RateLimitPolicy.new({ + fixed_window_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + count = 1, + window = 10, + condition = false_condition + } + } + }) + + -- Limit is 1. Make 2 requests and check that they're not limited + assert(rate_limit_policy:access(context)) + assert(rate_limit_policy:access(context)) + end) + end) + + describe('and there are several limits applied', function() + it('denies access when the condition of any limit is false', function() + local rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + rate = 1, + burst = 0, + condition = false_condition + } + }, + fixed_window_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + count = 1, + window = 10, + condition = true_condition + } + }, + connection_limiters = { + { + key = { name = 'limit_key', name_type = 'plain', scope = 'global' }, + conn = 1, burst = 0, delay = 0.5, + condition = true_condition + } + } + }) + + assert(rate_limit_policy:access(context)) + + assert.returns_error('limits exceeded', rate_limit_policy:access(context)) + assert.spy(ngx.exit).was_called_with(429) + end) + end) + end) end) describe('.log', function() From 4c279db34c886050c5ea2010f6954711fff6686b Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 10 Aug 2018 11:53:36 +0200 Subject: [PATCH 4/5] t/apicast-policy-rate-limit: add test with conditions --- t/apicast-policy-rate-limit.t | 70 +++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/t/apicast-policy-rate-limit.t b/t/apicast-policy-rate-limit.t index fd8df55ff..637228d17 100644 --- a/t/apicast-policy-rate-limit.t +++ b/t/apicast-policy-rate-limit.t @@ -1227,3 +1227,73 @@ and rejected properly. [200, 200, 200, 429] --- no_error_log [error] + +=== TEST 20: with conditions +We define a limit of 1 with a false condition, and a limit of 2 with a +condition that's true. We check that the false condition does not apply by +making 3 requests and checking that only the last one is rejected. +--- http_config + include $TEST_NGINX_UPSTREAM_CONFIG; + lua_package_path "$TEST_NGINX_LUA_PATH"; + + init_by_lua_block { + require "resty.core" + ngx.shared.limiter:flush_all() + require('apicast.configuration_loader').mock({ + services = { + { + id = 42, + proxy = { + policy_chain = { + { + name = "apicast.policy.rate_limit", + configuration = { + fixed_window_limiters = { + { + key = { name = "test20_key_1" }, + count = 2, + window = 10, + condition = { + operations = { + { + left = "{{ uri }}", + left_type = "liquid", + op = "==", + right = "/" + } + } + } + }, + { + key = { name = "test20_key_2" }, + count = 1, + window = 10, + condition = { + operations = { + { + left = "1", + op = "==", + right = "2" + } + } + } + } + }, + limits_exceeded_error = { status_code = 429 } + } + } + } + } + } + } + }) + } + lua_shared_dict limiter 1m; +--- config + include $TEST_NGINX_APICAST_CONFIG; +--- pipelined_requests eval +["GET /flush_redis", "GET /", "GET /", "GET /"] +--- error_code eval +[200, 200, 200, 429] +--- no_error_log +[error] From 27312ff59f502e5cdc558a90b96dcde614e318e2 Mon Sep 17 00:00:00 2001 From: David Ortiz Date: Fri, 10 Aug 2018 12:04:22 +0200 Subject: [PATCH 5/5] CHANGELOG: add entry for conditions in rate-limit policy --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index df53503fa..16257f0bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Request headers are now exposed in the context available when evaluating Liquid [PR #819](https://github.com/3scale/apicast/pull/819) - Rewrite URL captures policy. This policy captures arguments in a URL and rewrites the URL using them [PR #827](https://github.com/3scale/apicast/pull/827), [THREESCALE-1139](https://issues.jboss.org/browse/THREESCALE-1139) - Support for HTTP Proxy [THREESCALE-221](https://issues.jboss.org/browse/THREESCALE-221), [PR #835](https://github.com/3scale/apicast/pull/835) +- Conditions for the limits of the rate-limit policy [PR #839](https://github.com/3scale/apicast/pull/839) ### Changed