From 55352bc1a1bd2f2e735f6d3e2be2d37290ed817b Mon Sep 17 00:00:00 2001 From: thefosk Date: Sun, 5 Jul 2015 23:46:26 -0700 Subject: [PATCH] Handling multiple rate-limits --- .travis.yml | 2 +- .travis/setup_cassandra.sh | 4 +- .../cassandra/2015-06-09-170921_0.4.0.lua | 1 + kong/dao/schemas/plugins_configurations.lua | 7 ++ kong/plugins/ratelimiting/access.lua | 50 ++++++++-- kong/plugins/ratelimiting/schema.lua | 44 ++++++++- spec/plugins/ratelimiting/access_spec.lua | 97 ++++++++++++++++--- spec/plugins/ratelimiting/api_spec.lua | 43 ++++++++ spec/plugins/ratelimiting/schema_spec.lua | 36 +++++++ spec/unit/dao/cassandra/base_dao_spec.lua | 8 +- spec/unit/dao/entities_schemas_spec.lua | 5 +- 11 files changed, 263 insertions(+), 34 deletions(-) create mode 100644 spec/plugins/ratelimiting/api_spec.lua create mode 100644 spec/plugins/ratelimiting/schema_spec.lua diff --git a/.travis.yml b/.travis.yml index 9bf89193bba3..26d12886fd2b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,7 +2,7 @@ language: erlang env: global: - - CASSANDRA_VERSION=2.1.7 + - CASSANDRA_VERSION=2.1.8 matrix: - LUA=lua5.1 diff --git a/.travis/setup_cassandra.sh b/.travis/setup_cassandra.sh index ae3114446787..fc24250d5a6f 100644 --- a/.travis/setup_cassandra.sh +++ b/.travis/setup_cassandra.sh @@ -3,5 +3,5 @@ CASSANDRA_BASE=apache-cassandra-$CASSANDRA_VERSION sudo rm -rf /var/lib/cassandra/* -curl http://apache.spinellicreations.com/cassandra/$CASSANDRA_VERSION/$CASSANDRA_BASE-bin.tar.gz | tar xz -sudo sh $CASSANDRA_BASE/bin/cassandra +curl http://apache.mirrors.ionfish.org/cassandra/$CASSANDRA_VERSION/$CASSANDRA_BASE-bin.tar.gz | tar xz +sudo sh $CASSANDRA_BASE/bin/cassandra \ No newline at end of file diff --git a/database/migrations/cassandra/2015-06-09-170921_0.4.0.lua b/database/migrations/cassandra/2015-06-09-170921_0.4.0.lua index 8f7186ef7efe..f9a62e8c3f8a 100644 --- a/database/migrations/cassandra/2015-06-09-170921_0.4.0.lua +++ b/database/migrations/cassandra/2015-06-09-170921_0.4.0.lua @@ -3,6 +3,7 @@ local Migration = { up = function(options) return [[ + CREATE TABLE IF NOT EXISTS oauth2_credentials( id uuid, name text, diff --git a/kong/dao/schemas/plugins_configurations.lua b/kong/dao/schemas/plugins_configurations.lua index 5a68c0f213f3..6864496347d7 100644 --- a/kong/dao/schemas/plugins_configurations.lua +++ b/kong/dao/schemas/plugins_configurations.lua @@ -38,6 +38,13 @@ return { return false, DaoError("No consumer can be configured for that plugin", constants.DATABASE_ERROR_TYPES.SCHEMA) end + if value_schema.self_check and type(value_schema.self_check) == "function" then + local ok, err = value_schema.self_check(value_schema, plugin_t.value and plugin_t.value or {}, dao, is_update) + if not ok then + return false, err + end + end + if not is_update then local res, err = dao.plugins_configurations:find_by_keys({ name = plugin_t.name, diff --git a/kong/plugins/ratelimiting/access.lua b/kong/plugins/ratelimiting/access.lua index 87eab3d6d4eb..e02ed8789921 100644 --- a/kong/plugins/ratelimiting/access.lua +++ b/kong/plugins/ratelimiting/access.lua @@ -15,19 +15,50 @@ function _M.execute(conf) identifier = ngx.var.remote_addr end + local usage = {} + local stop + + -- Handle previous version of the rate-limiting plugin + local old_format = false + if conf.period and conf.limit then + old_format = true + conf[conf.period] = conf.limit -- Adapt to new format + + -- Delete old properties + conf.period = nil + conf.limit = nil + end + -- Load current metric for configured period - local current_metric, err = dao.ratelimiting_metrics:find_one(ngx.ctx.api.id, identifier, current_timestamp, conf.period) - if err then - return responses.send_HTTP_INTERNAL_SERVER_ERROR(err) + for period, limit in pairs(conf) do + local current_metric, err = dao.ratelimiting_metrics:find_one(ngx.ctx.api.id, identifier, current_timestamp, period) + if err then + return responses.send_HTTP_INTERNAL_SERVER_ERROR(err) + end + + -- What is the current usage for the configured period? + local current_usage = current_metric and current_metric.value or 0 + local remaining = limit - current_usage + + -- Recording usage + usage[period] = { + limit = limit, + remaining = remaining + } + + if remaining <= 0 then + stop = period + end end - -- What is the current usage for the configured period? - local current_usage = current_metric and current_metric.value or 0 - local remaining = conf.limit - current_usage - ngx.header[constants.HEADERS.RATELIMIT_LIMIT] = conf.limit - ngx.header[constants.HEADERS.RATELIMIT_REMAINING] = math.max(0, remaining - 1) -- -1 for this current request + -- Adding headers + for k,v in pairs(usage) do + ngx.header[constants.HEADERS.RATELIMIT_LIMIT..(old_format and "" or "-"..k)] = v.limit + ngx.header[constants.HEADERS.RATELIMIT_REMAINING..(old_format and "" or "-"..k)] = math.max(0, (stop == nil or stop == k) and v.remaining - 1 or v.remaining) -- -1 for this current request + end - if remaining <= 0 then + -- If limit is exceeded, terminate the request + if stop then ngx.ctx.stop_phases = true -- interrupt other phases of this request return responses.send(429, "API rate limit exceeded") end @@ -37,6 +68,7 @@ function _M.execute(conf) if stmt_err then return responses.send_HTTP_INTERNAL_SERVER_ERROR(stmt_err) end + end return _M diff --git a/kong/plugins/ratelimiting/schema.lua b/kong/plugins/ratelimiting/schema.lua index 037458d5ad31..126d2450b4c2 100644 --- a/kong/plugins/ratelimiting/schema.lua +++ b/kong/plugins/ratelimiting/schema.lua @@ -1,8 +1,44 @@ +local DaoError = require "kong.dao.error" local constants = require "kong.constants" return { fields = { - limit = { required = true, type = "number" }, - period = { required = true, type = "string", enum = constants.RATELIMIT.PERIODS } - } -} + second = { type = "number" }, + minute = { type = "number" }, + hour = { type = "number" }, + day = { type = "number" }, + month = { type = "number" }, + year = { type = "number" } + }, + self_check = function(schema, plugin_t, dao, is_update) + local ordered_periods = { "second", "minute", "hour", "day", "month", "year"} + local has_value + local invalid_order + local invalid_value + + for i, v in ipairs(ordered_periods) do + if plugin_t[v] then + has_value = true + if plugin_t[v] <=0 then + invalid_value = "Value for "..v.." must be greater than zero" + else + for t = i, #ordered_periods do + if plugin_t[ordered_periods[t]] and plugin_t[ordered_periods[t]] < plugin_t[v] then + invalid_order = "The value for "..ordered_periods[t].." cannot be lower than the value for "..v + end + end + end + end + end + + if not has_value then + return false, DaoError("You need to set at least one limit: second, minute, hour, day, month, year", constants.DATABASE_ERROR_TYPES.SCHEMA) + elseif invalid_value then + return false, DaoError(invalid_value, constants.DATABASE_ERROR_TYPES.SCHEMA) + elseif invalid_order then + return false, DaoError(invalid_order, constants.DATABASE_ERROR_TYPES.SCHEMA) + end + + return true + end +} \ No newline at end of file diff --git a/spec/plugins/ratelimiting/access_spec.lua b/spec/plugins/ratelimiting/access_spec.lua index 5ad741985714..135f4922ca26 100644 --- a/spec/plugins/ratelimiting/access_spec.lua +++ b/spec/plugins/ratelimiting/access_spec.lua @@ -20,7 +20,9 @@ describe("RateLimiting Plugin", function() spec_helper.insert_fixtures { api = { { name = "tests ratelimiting 1", public_dns = "test3.com", target_url = "http://mockbin.com" }, - { name = "tests ratelimiting 2", public_dns = "test4.com", target_url = "http://mockbin.com" } + { name = "tests ratelimiting 2", public_dns = "test4.com", target_url = "http://mockbin.com" }, + { name = "tests ratelimiting 3", public_dns = "test5.com", target_url = "http://mockbin.com" }, + { name = "tests ratelimiting 4", public_dns = "test6.com", target_url = "http://mockbin.com" } }, consumer = { { custom_id = "provider_123" }, @@ -28,9 +30,11 @@ describe("RateLimiting Plugin", function() }, plugin_configuration = { { name = "keyauth", value = {key_names = {"apikey"}, hide_credentials = true}, __api = 1 }, - { name = "ratelimiting", value = {period = "minute", limit = 6}, __api = 1 }, - { name = "ratelimiting", value = {period = "minute", limit = 8}, __api = 1, __consumer = 1 }, - { name = "ratelimiting", value = {period = "minute", limit = 6}, __api = 2 }, + { name = "ratelimiting", value = { minute = 6 }, __api = 1 }, + { name = "ratelimiting", value = { minute = 8 }, __api = 1, __consumer = 1 }, + { name = "ratelimiting", value = { minute = 6 }, __api = 2 }, + { name = "ratelimiting", value = { minute = 3, hour = 5 }, __api = 3 }, + { name = "ratelimiting", value = { minute = 33 }, __api = 4 } }, keyauth_credential = { { key = "apikey122", __consumer = 1 }, @@ -38,6 +42,25 @@ describe("RateLimiting Plugin", function() } } + -- Updating API test6.com with old plugin value, to check retrocompatibility + local dao_factory = spec_helper.get_env().dao_factory + -- Find API + local res, err = dao_factory.apis:find_by_keys({public_dns = 'test6.com'}) + if err then error(err) end + -- Find Plugin Configuration + local res, err = dao_factory.plugins_configurations:find_by_keys({api_id = res[1].id}) + if err then error(err) end + -- Set old value + local plugin_configuration = res[1] + plugin_configuration.value = { + period = "minute", + limit = 6 + } + -- Update plugin configuration + local _, err = dao_factory.plugins_configurations:execute( + "update plugins_configurations SET value = '{\"limit\":6, \"period\":\"minute\"}' WHERE id = "..plugin_configuration.id.." and name = 'ratelimiting'") + if err then error(err) end + spec_helper.start_kong() end) @@ -56,8 +79,8 @@ describe("RateLimiting Plugin", function() for i = 1, limit do local _, status, headers = http_client.get(STUB_GET_URL, {}, {host = "test4.com"}) assert.are.equal(200, status) - assert.are.same(tostring(limit), headers["x-ratelimit-limit"]) - assert.are.same(tostring(limit - i), headers["x-ratelimit-remaining"]) + assert.are.same(tostring(limit), headers["x-ratelimit-limit-minute"]) + assert.are.same(tostring(limit - i), headers["x-ratelimit-remaining-minute"]) end -- Additonal request, while limit is 6/minute @@ -67,11 +90,37 @@ describe("RateLimiting Plugin", function() assert.are.equal("API rate limit exceeded", body.message) end) - end) + it("should handle multiple limits", function() + local limits = { + minute = 3, + hour = 5 + } + + wait() + + for i = 1, 3 do + local _, status, headers = http_client.get(STUB_GET_URL, {}, {host = "test5.com"}) + assert.are.equal(200, status) + + assert.are.same(tostring(limits.minute), headers["x-ratelimit-limit-minute"]) + assert.are.same(tostring(limits.minute - i), headers["x-ratelimit-remaining-minute"]) + assert.are.same(tostring(limits.hour), headers["x-ratelimit-limit-hour"]) + assert.are.same(tostring(limits.hour - i), headers["x-ratelimit-remaining-hour"]) + end + local response, status, headers = http_client.get(STUB_GET_URL, {}, {host = "test5.com"}) + assert.are.equal("2", headers["x-ratelimit-remaining-hour"]) + assert.are.equal("0", headers["x-ratelimit-remaining-minute"]) + local body = cjson.decode(response) + assert.are.equal(429, status) + assert.are.equal("API rate limit exceeded", body.message) + end) + + end) + describe("With authentication", function() - describe("Default plugin", function() + describe("Old plugin format", function() it("should get blocked if exceeding limit", function() wait() @@ -80,12 +129,36 @@ describe("RateLimiting Plugin", function() local limit = 6 for i = 1, limit do - local _, status, headers = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "test3.com"}) + local _, status, headers = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "test6.com"}) assert.are.equal(200, status) assert.are.same(tostring(limit), headers["x-ratelimit-limit"]) assert.are.same(tostring(limit - i), headers["x-ratelimit-remaining"]) end + -- Third query, while limit is 2/minute + local response, status = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "test6.com"}) + local body = cjson.decode(response) + assert.are.equal(429, status) + assert.are.equal("API rate limit exceeded", body.message) + end) + + end) + + describe("Default plugin", function() + + it("should get blocked if exceeding limit", function() + wait() + + -- Default ratelimiting plugin for this API says 6/minute + local limit = 6 + + for i = 1, limit do + local _, status, headers = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "test3.com"}) + assert.are.equal(200, status) + assert.are.same(tostring(limit), headers["x-ratelimit-limit-minute"]) + assert.are.same(tostring(limit - i), headers["x-ratelimit-remaining-minute"]) + end + -- Third query, while limit is 2/minute local response, status = http_client.get(STUB_GET_URL, {apikey = "apikey123"}, {host = "test3.com"}) local body = cjson.decode(response) @@ -106,8 +179,8 @@ describe("RateLimiting Plugin", function() for i = 1, limit do local _, status, headers = http_client.get(STUB_GET_URL, {apikey = "apikey122"}, {host = "test3.com"}) assert.are.equal(200, status) - assert.are.same(tostring(limit), headers["x-ratelimit-limit"]) - assert.are.same(tostring(limit - i), headers["x-ratelimit-remaining"]) + assert.are.same(tostring(limit), headers["x-ratelimit-limit-minute"]) + assert.are.same(tostring(limit - i), headers["x-ratelimit-remaining-minute"]) end local response, status = http_client.get(STUB_GET_URL, {apikey = "apikey122"}, {host = "test3.com"}) @@ -117,5 +190,7 @@ describe("RateLimiting Plugin", function() end) end) + end) + end) diff --git a/spec/plugins/ratelimiting/api_spec.lua b/spec/plugins/ratelimiting/api_spec.lua new file mode 100644 index 000000000000..53e13c0938eb --- /dev/null +++ b/spec/plugins/ratelimiting/api_spec.lua @@ -0,0 +1,43 @@ +local json = require "cjson" +local http_client = require "kong.tools.http_client" +local spec_helper = require "spec.spec_helpers" + +local BASE_URL = spec_helper.API_URL.."/apis/%s/plugins/" + +describe("Rate Limiting API", function() + setup(function() + spec_helper.prepare_db() + spec_helper.insert_fixtures { + api = { + { name = "tests ratelimiting 1", public_dns = "test1.com", target_url = "http://mockbin.com" } + } + } + spec_helper.start_kong() + + local response = http_client.get(spec_helper.API_URL.."/apis/") + BASE_URL = string.format(BASE_URL, json.decode(response).data[1].id) + end) + + teardown(function() + spec_helper.stop_kong() + end) + + describe("POST", function() + + it("should not save with empty value", function() + local response, status = http_client.post(BASE_URL, { name = "ratelimiting" }) + local body = json.decode(response) + assert.are.equal(400, status) + assert.are.equal("You need to set at least one limit: second, minute, hour, day, month, year", body.message) + end) + + it("should save with proper value", function() + local response, status = http_client.post(BASE_URL, { name = "ratelimiting", ["value.second"] = 10 }) + local body = json.decode(response) + assert.are.equal(201, status) + assert.are.equal(10, body.value.second) + end) + + end) + +end) diff --git a/spec/plugins/ratelimiting/schema_spec.lua b/spec/plugins/ratelimiting/schema_spec.lua new file mode 100644 index 000000000000..aad27e1b3cc9 --- /dev/null +++ b/spec/plugins/ratelimiting/schema_spec.lua @@ -0,0 +1,36 @@ +local schemas = require "kong.dao.schemas_validation" +local validate_entity = schemas.validate_entity + +local plugin_schema = require "kong.plugins.ratelimiting.schema" + +describe("Rate Limiting schema", function() + + it("should be invalid when no value is being set", function() + local values = {} + local valid, _, err = validate_entity(values, plugin_schema) + assert.falsy(valid) + assert.are.equal("You need to set at least one limit: second, minute, hour, day, month, year", err.message) + end) + + it("should work when the proper value is being set", function() + local values = { second = 10 } + local valid, _, err = validate_entity(values, plugin_schema) + assert.truthy(valid) + assert.falsy(err) + end) + + it("should work when the proper value are being set", function() + local values = { second = 10, hour = 20 } + local valid, _, err = validate_entity(values, plugin_schema) + assert.truthy(valid) + assert.falsy(err) + end) + + it("should not work when invalid data is being set", function() + local values = { second = 20, hour = 10 } + local valid, _, err = validate_entity(values, plugin_schema) + assert.falsy(valid) + assert.are.equal("The value for hour cannot be lower than the value for second", err.message) + end) + +end) \ No newline at end of file diff --git a/spec/unit/dao/cassandra/base_dao_spec.lua b/spec/unit/dao/cassandra/base_dao_spec.lua index 3101c1a1e1eb..d35a320d41b3 100644 --- a/spec/unit/dao/cassandra/base_dao_spec.lua +++ b/spec/unit/dao/cassandra/base_dao_spec.lua @@ -537,7 +537,7 @@ describe("Cassandra", function() }, plugin_configuration = { {name = "keyauth", __api = 1}, - {name = "ratelimiting", value = {period = "minute", limit = 6}, __api = 1}, + {name = "ratelimiting", value = { minute = 6}, __api = 1}, {name = "filelog", value = {path = "/tmp/spec.log" }, __api = 1}, {name = "keyauth", __api = 2} @@ -590,7 +590,7 @@ describe("Cassandra", function() }, plugin_configuration = { {name = "keyauth", __api = 1, __consumer = 1}, - {name = "ratelimiting", value = {period = "minute", limit = 6}, __api = 1, __consumer = 1}, + {name = "ratelimiting", value = { minute = 6}, __api = 1, __consumer = 1}, {name = "filelog", value = {path = "/tmp/spec.log" }, __api = 1, __consumer = 1}, {name = "keyauth", __api = 1, __consumer = 2} @@ -658,8 +658,8 @@ describe("Cassandra", function() }, plugin_configuration = { { name = "keyauth", value = {key_names = {"apikey"}, hide_credentials = true}, __api = 1 }, - { name = "ratelimiting", value = {period = "minute", limit = 6}, __api = 1 }, - { name = "ratelimiting", value = {period = "minute", limit = 6}, __api = 2 }, + { name = "ratelimiting", value = { minute = 6}, __api = 1 }, + { name = "ratelimiting", value = { minute = 6}, __api = 2 }, { name = "filelog", value = { path = "/tmp/spec.log" }, __api = 1 } } } diff --git a/spec/unit/dao/entities_schemas_spec.lua b/spec/unit/dao/entities_schemas_spec.lua index c5e7efe91e8f..5b5fb49f0e1b 100644 --- a/spec/unit/dao/entities_schemas_spec.lua +++ b/spec/unit/dao/entities_schemas_spec.lua @@ -238,12 +238,11 @@ describe("Entities Schemas", function() assert.True(valid) -- Failure - plugin = {name = "ratelimiting", api_id = "stub", value = {period = "hello"}} + plugin = {name = "ratelimiting", api_id = "stub", value = { second = "hello" }} local valid, errors = validate_entity(plugin, plugins_configurations_schema, {dao = dao_stub}) assert.False(valid) - assert.equal("limit is required", errors["value.limit"]) - assert.equal("\"hello\" is not allowed. Allowed values are: \"second\", \"minute\", \"hour\", \"day\", \"month\", \"year\"", errors["value.period"]) + assert.equal("second is not a number", errors["value.second"]) end) describe("self_check", function()