diff --git a/CHANGELOG.md b/CHANGELOG.md index f525fea86..68f583cb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - `THREESCALE_PORTAL_ENDPOINT` and `THREESCALE_CONFIG_FILE` are not required anymore [PR #702](https://github.com/3scale/apicast/pull/702) +- The `scope` of the Rate Limit policy is `service` by default [PR #704](https://github.com/3scale/apicast/pull/704) ## [3.2.0-rc2] - 2018-05-11 diff --git a/gateway/src/apicast/policy/rate_limit/apicast-policy.json b/gateway/src/apicast/policy/rate_limit/apicast-policy.json index 836f1348f..6027abda8 100644 --- a/gateway/src/apicast/policy/rate_limit/apicast-policy.json +++ b/gateway/src/apicast/policy/rate_limit/apicast-policy.json @@ -6,6 +6,44 @@ "version": "builtin", "configuration": { "type": "object", + "definitions": { + "key": { + "$id": "#/definitions/key", + "description": "The key corresponding to the limiter object", + "type": "object", + "properties": { + "name": { + "type": "string", + "description": "The name of the key, must be unique in the scope" + }, + "scope": { + "type": "string", + "description": "Scope of the key", + "default": "service", + "oneOf": [{ + "enum": ["global"], + "description": "Global scope, affecting to all services" + }, { + "enum": ["service"], + "description": "Service scope, affecting to one service" + }] + } + } + }, + "error_handling": { + "$id": "#/definitions/error_handling", + "type": "string", + "description": "How to handle an error", + "default": "exit", + "oneOf": [{ + "enum": ["exit"], + "description": "Respond with an error" + }, { + "enum": ["log"], + "description": "Let the request go through and only output logs" + }] + } + }, "properties": { "connection_limiters": { "type": "array", @@ -13,30 +51,7 @@ "type": "object", "properties": { "key": { - "description": "The key corresponding to the limiter object", - "type": "object", - "properties": { - "name": { - "type": "string", - "description": "The name of the key, must be unique in the scope" - }, - "scope": { - "type": "string", - "description": "Scope of the key", - "default": "global", - "oneOf": [{ - "enum": ["global"], - "description": "Global scope, affecting to all services" - }, { - "enum": ["service"], - "description": "Service scope, affecting to one service" - }] - }, - "service_name": { - "type": "string", - "description": "Name of service, necessary for service scope" - } - } + "$ref": "#/definitions/key" }, "conn": { "type": "integer", @@ -62,30 +77,7 @@ "type": "object", "properties": { "key": { - "description": "The key corresponding to the limiter object", - "type": "object", - "properties": { - "name": { - "type": "string", - "description": "The name of the key, must be unique in the scope" - }, - "scope": { - "type": "string", - "description": "Scope of the key", - "default": "global", - "oneOf": [{ - "enum": ["global"], - "description": "Global scope, affecting to all services" - }, { - "enum": ["service"], - "description": "Service scope, affecting to one service" - }] - }, - "service_name": { - "type": "string", - "description": "Name of service, necessary for service scope" - } - } + "$ref": "#/definitions/key" }, "rate": { "type": "integer", @@ -106,30 +98,7 @@ "type": "object", "properties": { "key": { - "description": "The key corresponding to the limiter object", - "type": "object", - "properties": { - "name": { - "type": "string", - "description": "The name of the key, must be unique in the scope" - }, - "scope": { - "type": "string", - "description": "Scope of the key", - "default": "global", - "oneOf": [{ - "enum": ["global"], - "description": "Global scope, affecting to all services" - }, { - "enum": ["service"], - "description": "Service scope, affecting to one service" - }] - }, - "service_name": { - "type": "string", - "description": "Name of service, necessary for service scope" - } - } + "$ref": "#/definitions/key" }, "count": { "type": "integer", @@ -157,16 +126,7 @@ "default": 429 }, "error_handling": { - "type": "string", - "description": "How to handle an error", - "default": "exit", - "oneOf": [{ - "enum": ["exit"], - "description": "Respond with an error" - }, { - "enum": ["log"], - "description": "Let the request go through and only output logs" - }] + "$ref": "#/definitions/error_handling" } } }, @@ -179,23 +139,7 @@ "default": 500 }, "error_handling": { - "type": "string", - "description": "How to handle an error", - "default": "exit", - "oneOf": [ - { - "enum": [ - "exit" - ], - "description": "Respond with an error" - }, - { - "enum": [ - "log" - ], - "description": "Let the request go through and only output logs" - } - ] + "$ref": "#/definitions/error_handling" } } } diff --git a/gateway/src/apicast/policy/rate_limit/rate_limit.lua b/gateway/src/apicast/policy/rate_limit/rate_limit.lua index bba13c1a2..f96923689 100644 --- a/gateway/src/apicast/policy/rate_limit/rate_limit.lua +++ b/gateway/src/apicast/policy/rate_limit/rate_limit.lua @@ -15,6 +15,8 @@ local shdict_key = 'limiter' local insert = table.insert local ipairs = ipairs local unpack = table.unpack +local format = string.format +local concat = table.concat local new = _M.new @@ -108,7 +110,7 @@ local function init_error_settings(limits_exceeded_error, configuration_error) return error_settings end -local function build_limiters_and_keys(type, limiters, redis, error_settings) +local function build_limiters_and_keys(type, limiters, redis, error_settings, service_id) local res_limiters = {} local res_keys = {} @@ -125,10 +127,10 @@ local function build_limiters_and_keys(type, limiters, redis, error_settings) insert(res_limiters, lim) local key - if limiter.key.scope == "service" then - key = limiter.key.service_name.."_"..type.."_"..limiter.key.name + if limiter.key.scope == "global" then + key = format("%s_%s", type, limiter.key.name) else - key = type.."_"..limiter.key.name + key = format("%s_%s_%s", service_id, type, limiter.key.name) end insert(res_keys, key) @@ -150,7 +152,7 @@ function _M.new(config) return self end -function _M:access() +function _M:access(context) local red if self.redis_url then local rederr @@ -163,13 +165,13 @@ function _M:access() end local conn_limiters, conn_keys = build_limiters_and_keys( - 'connections', self.connection_limiters, red, self.error_settings) + 'connections', self.connection_limiters, red, self.error_settings, context.service.id) local leaky_bucket_limiters, leaky_bucket_keys = build_limiters_and_keys( - 'leaky_bucket', self.leaky_bucket_limiters, red, self.error_settings) + 'leaky_bucket', self.leaky_bucket_limiters, red, self.error_settings, context.service.id) local fixed_window_limiters, fixed_window_keys = build_limiters_and_keys( - 'fixed_window', self.fixed_window_limiters, red, self.error_settings) + 'fixed_window', self.fixed_window_limiters, red, self.error_settings, context.service.id) local limiters = {} local limiter_groups = { conn_limiters, leaky_bucket_limiters, fixed_window_limiters } @@ -205,8 +207,8 @@ function _M:access() for i, lim in ipairs(limiters) do if lim.is_committed and lim:is_committed() then - table.insert(connections_committed, lim) - table.insert(keys_committed, keys[i]) + insert(connections_committed, lim) + insert(keys_committed, keys[i]) end end @@ -217,7 +219,7 @@ function _M:access() end if delay > 0 then - ngx.log(ngx.WARN, 'need to delay by: ', delay, 's, states: ', table.concat(states, ", ")) + ngx.log(ngx.WARN, 'need to delay by: ', delay, 's, states: ', concat(states, ", ")) ngx.sleep(delay) end diff --git a/spec/policy/rate_limit/rate_limit_spec.lua b/spec/policy/rate_limit/rate_limit_spec.lua index 840e81008..fa09c04e8 100644 --- a/spec/policy/rate_limit/rate_limit_spec.lua +++ b/spec/policy/rate_limit/rate_limit_spec.lua @@ -40,6 +40,7 @@ local redis_port = env.get('TEST_NGINX_REDIS_PORT') or 6379 describe('Rate limit policy', function() local ngx_exit_spy local ngx_sleep_spy + local context setup(function() ngx_exit_spy = spy.on(ngx, 'exit') @@ -50,105 +51,110 @@ describe('Rate limit policy', function() local redis = require('resty.redis'):new() redis:connect(redis_host, redis_port) redis:select(1) - redis:del('connections_test1', 'leaky_bucket_test2', 'fixed_window_test3', 'bank_A_leaky_bucket_test4') + redis:del('connections_test1', 'leaky_bucket_test2', 'fixed_window_test3', '5_leaky_bucket_test4') init_val() + context = { + service = { + id = 5 + } + } end) describe('.access', function() it('success with multiple limiters', function() local config = { connection_limiters = { - { key = { name = 'test1' }, conn = 20, burst = 10, delay = 0.5 } + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } }, leaky_bucket_limiters = { - { key = { name = 'test2' }, rate = 18, burst = 9 } + { key = { name = 'test2', scope = 'global' }, rate = 18, burst = 9 } }, fixed_window_limiters = { - { key = {name = 'test3'}, count = 10, window = 10 } + { key = { name = 'test3', scope = 'global' }, count = 10, window = 10 } }, redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() + rate_limit_policy:access(context) end) it('no redis url', function() local config = { connection_limiters = { - { key = { name = 'test1' }, conn = 20, burst = 10, delay = 0.5 } + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } }, leaky_bucket_limiters = { - { key = { name = 'test2' }, rate = 18, burst = 9 } + { key = { name = 'test2', scope = 'global' }, rate = 18, burst = 9 } }, fixed_window_limiters = { - { key = { name = 'test3' }, count = 10, window = 10 } + { key = { name = 'test3', scope = 'global' }, count = 10, window = 10 } } } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() + rate_limit_policy:access(context) end) it('invalid redis url', function() local config = { connection_limiters = { - { key = { name = 'test1' }, conn = 20, burst = 10, delay = 0.5 } + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } }, redis_url = 'redis://invalidhost:'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() + rate_limit_policy:access(context) assert.spy(ngx_exit_spy).was_called_with(500) end) it('rejected (conn)', function() local config = { connection_limiters = { - { key = { name = 'test1' }, conn = 1, burst = 0, delay = 0.5 } + { key = { name = 'test1', scope = 'global' }, conn = 1, burst = 0, delay = 0.5 } }, redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() - rate_limit_policy:access() + rate_limit_policy:access(context) + rate_limit_policy:access(context) assert.spy(ngx_exit_spy).was_called_with(429) end) it('rejected (req)', function() local config = { leaky_bucket_limiters = { - { key = { name = 'test2' }, rate = 1, burst = 0 } + { key = { name = 'test2', scope = 'global' }, rate = 1, burst = 0 } }, redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() - rate_limit_policy:access() + rate_limit_policy:access(context) + rate_limit_policy:access(context) assert.spy(ngx_exit_spy).was_called_with(429) end) it('rejected (count)', function() local config = { fixed_window_limiters = { - { key = { name = 'test3' }, count = 1, window = 10 } + { key = { name = 'test3', scope = 'global' }, count = 1, window = 10 } }, redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() - rate_limit_policy:access() + rate_limit_policy:access(context) + rate_limit_policy:access(context) assert.spy(ngx_exit_spy).was_called_with(429) end) it('delay (conn)', function() local config = { connection_limiters = { - { key = {name = 'test1'}, conn = 1, burst = 1, delay = 2 } + { key = { name = 'test1', scope = 'global' }, conn = 1, burst = 1, delay = 2 } }, redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() - rate_limit_policy:access() + rate_limit_policy:access(context) + rate_limit_policy:access(context) assert.spy(ngx_sleep_spy).was_called_with(match.is_gt(0.001)) end) @@ -160,8 +166,8 @@ describe('Rate limit policy', function() redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() - rate_limit_policy:access() + rate_limit_policy:access(context) + rate_limit_policy:access(context) assert.spy(ngx_sleep_spy).was_called_with(match.is_gt(0.001)) end) @@ -169,7 +175,24 @@ describe('Rate limit policy', function() local config = { leaky_bucket_limiters = { { - key = { name = 'test4', scope = 'service', service_name = 'bank_A' }, + key = { name = 'test4', scope = 'service' }, + rate = 1, + burst = 1 + } + }, + redis_url = 'redis://'..redis_host..':'..redis_port..'/1' + } + local rate_limit_policy = RateLimitPolicy.new(config) + rate_limit_policy:access(context) + rate_limit_policy:access(context) + assert.spy(ngx_sleep_spy).was_called_with(match.is_gt(0.001)) + end) + + it('delay (req) default service scope', function() + local config = { + leaky_bucket_limiters = { + { + key = { name = 'test4' }, rate = 1, burst = 1 } @@ -177,8 +200,8 @@ describe('Rate limit policy', function() redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() - rate_limit_policy:access() + rate_limit_policy:access(context) + rate_limit_policy:access(context) assert.spy(ngx_sleep_spy).was_called_with(match.is_gt(0.001)) end) end) @@ -187,12 +210,12 @@ describe('Rate limit policy', function() it('success in leaving', function() local config = { connection_limiters = { - { key = {name = 'test1'}, conn = 20, burst = 10, delay = 0.5 } + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } }, redis_url = 'redis://'..redis_host..':'..redis_port..'/1' } local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access() + rate_limit_policy:access(context) rate_limit_policy:log() end) end) diff --git a/t/apicast-policy-rate-limit.t b/t/apicast-policy-rate-limit.t index 375ccca14..485b3ab99 100644 --- a/t/apicast-policy-rate-limit.t +++ b/t/apicast-policy-rate-limit.t @@ -30,7 +30,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test1", scope = "service", service_name = "service_C"}, + key = {name = "test1", scope = "service"}, conn = 1, burst = 1, delay = 2 @@ -44,7 +44,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test1", scope = "service", service_name = "service_C"}, + key = {name = "test1", scope = "service"}, conn = 1, burst = 1, delay = 2 @@ -77,7 +77,83 @@ Return 200 code. local redis = require('resty.redis'):new() redis:connect(redis_host, redis_port) redis:select(1) - redis:del("service_C_connections_test1") + redis:del("42_connections_test1") + } + } + +--- pipelined_requests eval +["GET /flush_redis","GET /"] +--- error_code eval +[200, 200] + +=== TEST 2: Delay (conn) default service scope. +Return 200 code. +--- 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 = { + connection_limiters = { + { + key = {name = "test2"}, + conn = 1, + burst = 1, + delay = 2 + } + }, + redis_url = "redis://$TEST_NGINX_REDIS_HOST:$TEST_NGINX_REDIS_PORT/1" + } + }, + { + name = "apicast.policy.rate_limit", + configuration = { + connection_limiters = { + { + key = {name = "test2"}, + conn = 1, + burst = 1, + delay = 2 + } + }, + redis_url = "redis://$TEST_NGINX_REDIS_HOST:$TEST_NGINX_REDIS_PORT/1" + } + } + } + } + } + } + }) + } + lua_shared_dict limiter 1m; + +--- config + include $TEST_NGINX_APICAST_CONFIG; + resolver $TEST_NGINX_RESOLVER; + + location /transactions/authrep.xml { + content_by_lua_block { ngx.exit(200) } + } + + location /flush_redis { + content_by_lua_block { + local env = require('resty.env') + local redis_host = "$TEST_NGINX_REDIS_HOST" or '127.0.0.1' + local redis_port = "$TEST_NGINX_REDIS_PORT" or 6379 + local redis = require('resty.redis'):new() + redis:connect(redis_host, redis_port) + redis:select(1) + redis:del("42_connections_test2") } } @@ -106,7 +182,7 @@ Return 500 code. configuration = { connection_limiters = { { - key = {name = "test3"}, + key = {name = "test3", scope = "global"}, conn = 20, burst = 10, delay = 0.5 @@ -150,7 +226,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test4"}, + key = {name = "test4", scope = "global"}, conn = 1, burst = 0, delay = 2 @@ -165,7 +241,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test4"}, + key = {name = "test4", scope = "global"}, conn = 1, burst = 0, delay = 2 @@ -224,7 +300,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test5"}, + key = {name = "test5", scope = "global"}, conn = 20, burst = 10, delay = 0.5 @@ -269,14 +345,14 @@ Return 200 code. configuration = { leaky_bucket_limiters = { { - key = {name = "test6_1"}, + key = {name = "test6_1", scope = "global"}, rate = 20, burst = 10 } }, connection_limiters = { { - key = {name = "test6_2"}, + key = {name = "test6_2", scope = "global"}, conn = 20, burst = 10, delay = 0.5 @@ -284,7 +360,7 @@ Return 200 code. }, fixed_window_limiters = { { - key = {name = "test6_3"}, + key = {name = "test6_3", scope = "global"}, count = 20, window = 10 } @@ -344,7 +420,7 @@ Return 429 code. configuration = { connection_limiters = { { - key = {name = "test7"}, + key = {name = "test7", scope = "global"}, conn = 1, burst = 0, delay = 2 @@ -358,7 +434,7 @@ Return 429 code. configuration = { connection_limiters = { { - key = {name = "test7"}, + key = {name = "test7", scope = "global"}, conn = 1, burst = 0, delay = 2 @@ -418,7 +494,7 @@ Return 503 code. configuration = { leaky_bucket_limiters = { { - key = {name = "test8"}, + key = {name = "test8", scope = "global"}, rate = 1, burst = 0 } @@ -538,7 +614,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test10"}, + key = {name = "test10", scope = "global"}, conn = 1, burst = 1, delay = 2 @@ -552,7 +628,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test10"}, + key = {name = "test10", scope = "global"}, conn = 1, burst = 1, delay = 2 @@ -614,7 +690,7 @@ Return 200 code. configuration = { leaky_bucket_limiters = { { - key = {name = "test11"}, + key = {name = "test11", scope = "global"}, rate = 1, burst = 1 } @@ -675,7 +751,7 @@ Return 429 code. configuration = { connection_limiters = { { - key = {name = "test12"}, + key = {name = "test12", scope = "global"}, conn = 1, burst = 0, delay = 2 @@ -688,7 +764,7 @@ Return 429 code. configuration = { connection_limiters = { { - key = {name = "test12"}, + key = {name = "test12", scope = "global"}, conn = 1, burst = 0, delay = 2 @@ -733,7 +809,7 @@ Return 429 code. configuration = { leaky_bucket_limiters = { { - key = {name = "test13"}, + key = {name = "test13", scope = "global"}, rate = 1, burst = 0 } @@ -777,7 +853,7 @@ Return 429 code. configuration = { fixed_window_limiters = { { - key = {name = "test14"}, + key = {name = "test14", scope = "global"}, count = 1, window = 10 } @@ -820,7 +896,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test15"}, + key = {name = "test15", scope = "global"}, conn = 1, burst = 1, delay = 2 @@ -833,7 +909,7 @@ Return 200 code. configuration = { connection_limiters = { { - key = {name = "test15"}, + key = {name = "test15", scope = "global"}, conn = 1, burst = 1, delay = 2 @@ -882,7 +958,7 @@ Return 200 code. configuration = { leaky_bucket_limiters = { { - key = {name = "test16"}, + key = {name = "test16", scope = "global"}, rate = 1, burst = 1 }