From 3efa8b16ebdb3ae8e274c9619deb56f30aea9fe6 Mon Sep 17 00:00:00 2001 From: Michal Cichra Date: Tue, 12 Jun 2018 18:06:40 +0200 Subject: [PATCH] [spec] unify style of rate limit policy test * use setup / perform / assert blocks separated by whitespace * use one redis connection * group similar tests into groups * every test has to have assert * prepare shdict mock or extraction --- .../apicast/policy/rate_limit/rate_limit.lua | 12 +- spec/policy/rate_limit/rate_limit_spec.lua | 453 +++++++++--------- 2 files changed, 244 insertions(+), 221 deletions(-) diff --git a/gateway/src/apicast/policy/rate_limit/rate_limit.lua b/gateway/src/apicast/policy/rate_limit/rate_limit.lua index c58e874ce..57f17f85e 100644 --- a/gateway/src/apicast/policy/rate_limit/rate_limit.lua +++ b/gateway/src/apicast/policy/rate_limit/rate_limit.lua @@ -180,11 +180,12 @@ function _M:access(context) if comerr == "rejected" then ngx.log(ngx.WARN, "Requests over the limit.") error(self.error_settings, "limits_exceeded") - return + return nil, 'limits exceeded' + else + ngx.log(ngx.ERR, "failed to limit traffic: ", comerr) + error(self.error_settings, "configuration_issue") + return nil, comerr or 'invalid configuration' end - ngx.log(ngx.ERR, "failed to limit traffic: ", comerr) - error(self.error_settings, "configuration_issue") - return end for i, lim in ipairs(limiters) do @@ -205,6 +206,7 @@ function _M:access(context) ngx.sleep(delay) end + return true, delay end local function checkin(_, ctx, time, semaphore, redis_url, error_settings) @@ -246,7 +248,7 @@ function _M:log() if limiters and next(limiters) ~= nil then local semaphore = ngx_semaphore.new() ngx.timer.at(0, checkin, ngx.ctx, ngx.var.request_time, semaphore, self.redis_url, self.error_settings) - semaphore:wait(10) + return semaphore:wait(10) end end diff --git a/spec/policy/rate_limit/rate_limit_spec.lua b/spec/policy/rate_limit/rate_limit_spec.lua index 282d631d1..7bbd7a781 100644 --- a/spec/policy/rate_limit/rate_limit_spec.lua +++ b/spec/policy/rate_limit/rate_limit_spec.lua @@ -1,30 +1,22 @@ local RateLimitPolicy = require('apicast.policy.rate_limit') local match = require('luassert.match') local env = require('resty.env') -local function init_val() - ngx.var = {} - ngx.var.request_time = '0.060' - ngx.var.host = 'test3' - - ngx.shared.limiter = {} - ngx.shared.limiter.get = function(_, key) - return ngx.shared.limiter[key] - end - ngx.shared.limiter.set = function(_, key, val) - ngx.shared.limiter[key] = val - end - ngx.shared.limiter.incr = function(_, key, val, init) - local v = ngx.shared.limiter[key] - if not v then - ngx.shared.limiter[key] = val + init - else - ngx.shared.limiter[key] = v + val - end - return ngx.shared.limiter[key] - end - ngx.shared.limiter.expire = function(_, _, _) - return true, nil - end + +local shdict_mt = { + __index = { + get = function(t, k) return rawget(t, k) end, + set = function(t, k, v) rawset(t, k , v); return true end, + incr = function(t, key, inc, init, _) + local value = t:get(key) or init + if not value then return nil, 'not found' end + + t:set(key, value + inc) + return t:get(key) + end, + } +} +local function shdict() + return setmetatable({ }, shdict_mt) end local function is_gt(_, arguments) @@ -35,9 +27,14 @@ local function is_gt(_, arguments) end assert:register("matcher", "gt", is_gt) +local ts = require ('apicast.threescale_utils') + local redis_host = env.get('TEST_NGINX_REDIS_HOST') or 'localhost' local redis_port = env.get('TEST_NGINX_REDIS_PORT') or 6379 +local redis_url = 'redis://'..redis_host..':'..redis_port..'/1' +local redis = ts.connect_redis{ url = redis_url } + describe('Rate limit policy', function() local ngx_exit_spy local ngx_sleep_spy @@ -49,11 +46,17 @@ describe('Rate limit policy', function() end) before_each(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', '5_leaky_bucket_test4') - init_val() + redis:flushdb() + end) + + before_each(function() + ngx.var = { + request_time = '0.060', + host = 'test3', + } + + ngx.shared.limiter = mock(shdict(), true) + context = { service = { id = 5 @@ -62,207 +65,225 @@ describe('Rate limit policy', function() end) describe('.access', function() - it('success with multiple limiters', function() - local config = { - connection_limiters = { - { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } - }, - leaky_bucket_limiters = { - { key = { name = 'test2', scope = 'global' }, rate = 18, burst = 9 } - }, - fixed_window_limiters = { - { 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(context) - end) + describe('using #shmem', function() + setup(function() + ngx.shared.limiter = shdict() + end) - it('no redis url', function() - local config = { - connection_limiters = { - { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } - }, - leaky_bucket_limiters = { - { key = { name = 'test2', scope = 'global' }, rate = 18, burst = 9 } - }, - fixed_window_limiters = { - { key = { name = 'test3', scope = 'global' }, count = 10, window = 10 } - } - } - local rate_limit_policy = RateLimitPolicy.new(config) - rate_limit_policy:access(context) - end) + it('works without redis', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } + } + }) - it('invalid redis url', function() - local config = { - connection_limiters = { - { 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(context) - assert.spy(ngx_exit_spy).was_called_with(500) - end) + assert(rate_limit_policy:access(context)) + end) - it('rejected (conn)', function() - local config = { - connection_limiters = { - { 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(context) - rate_limit_policy:access(context) - assert.spy(ngx_exit_spy).was_called_with(429) - end) + it('works with multiple limiters', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } + }, + leaky_bucket_limiters = { + { key = { name = 'test2', scope = 'global' }, rate = 18, burst = 9 } + }, + fixed_window_limiters = { + { key = { name = 'test3', scope = 'global' }, count = 10, window = 10 } + }, + }) - it('rejected (req)', function() - local config = { - leaky_bucket_limiters = { - { 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(context) - rate_limit_policy:access(context) - assert.spy(ngx_exit_spy).was_called_with(429) + assert(rate_limit_policy:access(context)) + end) end) - it('rejected (count), name_type is plain', function() - local config = { - fixed_window_limiters = { - { key = { name = 'test3', name_type = 'plain', 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(context) - rate_limit_policy:access(context) - assert.spy(ngx_exit_spy).was_called_with(429) - - local redis = require('resty.redis'):new() - redis:connect(redis_host, redis_port) - redis:select(1) - local fixed_window = redis:get('fixed_window_test3') - assert.equal('2', fixed_window) - end) + describe('using #redis', function() + it('works with multiple limiters', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } + }, + leaky_bucket_limiters = { + { key = { name = 'test2', scope = 'global' }, rate = 18, burst = 9 } + }, + fixed_window_limiters = { + { key = { name = 'test3', scope = 'global' }, count = 10, window = 10 } + }, + redis_url = redis_url + }) - it('rejected (count), name_type is liquid', function() - context = { service = { id = 5 }, var_in_context = 'test3' } - local config = { - fixed_window_limiters = { - { key = { name = '{{ var_in_context }}', name_type = 'liquid', 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(context) - rate_limit_policy:access(context) - assert.spy(ngx_exit_spy).was_called_with(429) - - local redis = require('resty.redis'):new() - redis:connect(redis_host, redis_port) - redis:select(1) - local fixed_window = redis:get('fixed_window_test3') - assert.equal('2', fixed_window) - end) + assert(rate_limit_policy:access(context)) + end) - it('rejected (count), name_type is liquid, ngx variable', function() - local config = { - fixed_window_limiters = { - { key = { name = '{{ host }}', name_type = 'liquid', 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(context) - rate_limit_policy:access(context) - assert.spy(ngx_exit_spy).was_called_with(429) - - local redis = require('resty.redis'):new() - redis:connect(redis_host, redis_port) - redis:select(1) - local fixed_window = redis:get('fixed_window_test3') - assert.equal('2', fixed_window) - end) + it('invalid redis url', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } + }, + redis_url = 'redis://invalidhost:'..redis_port..'/1' + }) - it('delay (conn)', function() - local config = { - connection_limiters = { - { 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(context) - rate_limit_policy:access(context) - assert.spy(ngx_sleep_spy).was_called_with(match.is_gt(0.001)) - end) + rate_limit_policy:access(context) - it('delay (req)', function() - local config = { - leaky_bucket_limiters = { - { key = { name = 'test2', scope = 'global' }, 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) + assert.spy(ngx_exit_spy).was_called_with(500) + end) - it('delay (req) service scope', function() - local config = { - leaky_bucket_limiters = { - { - 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) + describe('rejection', function() + it('rejected (conn)', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 1, burst = 0, delay = 0.5 } + }, + redis_url = redis_url + }) - it('delay (req) default service scope', function() - local config = { - leaky_bucket_limiters = { - { - key = { name = 'test4' }, - 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)) + 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 rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { key = { name = 'test2', scope = 'global' }, rate = 1, burst = 0 } + }, + redis_url = redis_url + }) + + rate_limit_policy:access(context) + rate_limit_policy:access(context) + + assert.spy(ngx_exit_spy).was_called_with(429) + end) + + it('rejected (count), name_type is plain', function() + local rate_limit_policy = RateLimitPolicy.new({ + fixed_window_limiters = { + { key = { name = 'test3', name_type = 'plain', scope = 'global' }, count = 1, window = 10 } + }, + redis_url = redis_url + }) + + rate_limit_policy:access(context) + rate_limit_policy:access(context) + + assert.spy(ngx_exit_spy).was_called_with(429) + assert.equal('2', redis:get('fixed_window_test3')) + end) + + it('rejected (count), name_type is liquid', function() + local ctx = { service = { id = 5 }, var_in_context = 'test3' } + local rate_limit_policy = RateLimitPolicy.new({ + fixed_window_limiters = { + { key = { name = '{{ var_in_context }}', name_type = 'liquid', scope = 'global' }, count = 1, window = 10 } + }, + redis_url = redis_url + }) + + rate_limit_policy:access(ctx) + rate_limit_policy:access(ctx) + + assert.equal('2', redis:get('fixed_window_test3')) + assert.spy(ngx_exit_spy).was_called_with(429) + end) + + it('rejected (count), name_type is liquid, ngx variable', function() + local rate_limit_policy = RateLimitPolicy.new({ + fixed_window_limiters = { + { key = { name = '{{ host }}', name_type = 'liquid', scope = 'global' }, count = 1, window = 10 } + }, + redis_url = redis_url + }) + + rate_limit_policy:access(context) + rate_limit_policy:access(context) + + assert.equal('2', redis:get('fixed_window_test3')) + assert.spy(ngx_exit_spy).was_called_with(429) + end) + end) + + describe('delay', function() + it('delay (conn)', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 1, burst = 1, delay = 2 } + }, + redis_url = redis_url + }) + + 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)', function() + local rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { key = { name = 'test2', scope = 'global' }, rate = 1, burst = 1 } + }, + redis_url = redis_url + }) + + 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) service scope', function() + local rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { + key = { name = 'test4', scope = 'service' }, + rate = 1, + burst = 1 + } + }, + redis_url = redis_url + }) + + 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 rate_limit_policy = RateLimitPolicy.new({ + leaky_bucket_limiters = { + { + key = { name = 'test4' }, + rate = 1, + burst = 1 + } + }, + redis_url = redis_url + }) + + 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) end) - end) - describe('.log', function() - it('success in leaving', function() - local config = { - connection_limiters = { - { 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(context) - rate_limit_policy:log() + describe('.log', function() + it('success in leaving', function() + local rate_limit_policy = RateLimitPolicy.new({ + connection_limiters = { + { key = { name = 'test1', scope = 'global' }, conn = 20, burst = 10, delay = 0.5 } + }, + redis_url = redis_url + }) + + rate_limit_policy:access(context) + + assert(rate_limit_policy:log()) + end) end) end) end)