Refactor ratelimiting #12

Makefile

@@ -36,7 +36,7 @@ test-integration:
 	@bin/kong -c $(TEST_DIR)/kong-dev.yaml -n $(TEST_DIR)/nginx-dev.conf start > /dev/null
 	@bin/kong migrate > /dev/null
 	@$(MAKE) seed > /dev/null
-	@busted $(FOLDER) || (bin/kong stop > /dev/null;make drop > /dev/null; exit 1)
+	@busted $(FOLDER) || (bin/kong stop > /dev/null;make drop > /dev/null;make clean DEV_DIR=$(TEST_DIR); exit 1)
 	@bin/kong stop > /dev/null
 	@$(MAKE) drop > /dev/null
 	@$(MAKE) clean DEV_DIR=$(TEST_DIR)

database/migrations/cassandra/2015-01-12-175310_init_schema.lua

@@ -60,8 +60,9 @@ local Migration = {
         api_id uuid,
         identifier text,
         period text,
+        period_date timestamp,
         value counter,
-        PRIMARY KEY ((api_id, identifier, period))
+        PRIMARY KEY ((api_id, identifier, period_date, period))
       );
     ]]
   end,

kong-0.1-1.rockspec

@@ -35,7 +35,13 @@ build = {
   modules = {
     ["kong"] = "src/main.lua",
     ["classic"] = "src/classic.lua",
-    ["cassandra"] = "src/cassandra.lua",
+
+    ["cassandra"] = "src/cassandra/cassandra.lua",
+    ["protocol"] = "src/cassandra/protocol.lua",
+    ["encoding"] = "src/cassandra/encoding.lua",
+    ["decoding"] = "src/cassandra/decoding.lua",
+    ["cassandra.constants"] = "src/cassandra/constants.lua",
+
     ["constants"] = "src/constants.lua",
 
     ["kong.tools.utils"] = "src/kong/tools/utils.lua",

spec/proxy/ratelimiting_plugin_spec.lua

@@ -2,58 +2,75 @@ local utils = require "kong.tools.utils"
 local cjson = require "cjson"
 
 local kProxyURL = "http://localhost:8000/"
+local kGetURL = kProxyURL.."/get"
 
 describe("RateLimiting Plugin #proxy", function()
 
-    describe("Without authentication", function()
-      it("should get blocked if exceeding limit by IP address", function()
-        local response, status, headers = utils.get(kProxyURL.."get", {}, {host = "test5.com"})
-        assert.are.equal(200, status)
+  describe("Without authentication (IP address)", function()
+
+    it("should get blocked if exceeding limit", function()
+      -- Default ratelimiting plugin for this API says 2/minute
+      local limit = 2
 
-        response, status, headers = utils.get(kProxyURL.."get", {}, {host = "test5.com"})
+      for i = 1, limit do
+        local response, status, headers = utils.get(kGetURL, {}, {host = "test5.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
 
-        response, status, headers = utils.get(kProxyURL.."get", {}, {host = "test5.com"})
-        local body = cjson.decode(response)
-        assert.are.equal(429, status)
-        assert.are.equal("API rate limit exceeded", body.message)
-      end)
+      -- Third query, while limit is 2/minute
+      local response, status, headers = utils.get(kGetURL, {}, {host = "test5.com"})
+      local body = cjson.decode(response)
+      assert.are.equal(429, status)
+      assert.are.equal("API rate limit exceeded", body.message)
     end)
 
-     describe("With authentication", function()
-      it("should get blocked if exceeding limit by apikey", function()
-        local response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey123"}, {host = "test6.com"})
-        assert.are.equal(200, status)
+  end)
 
-        response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey123"}, {host = "test6.com"})
-        assert.are.equal(200, status)
+  describe("With authentication", function()
+
+    describe("Default plugin", function()
+
+      it("should get blocked if exceeding limit", function()
+        -- Default ratelimiting plugin for this API says 2/minute
+        local limit = 2
 
-        response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey123"}, {host = "test6.com"})
+        for i = 1, limit do
+          local response, status, headers = utils.get(kGetURL, {apikey = "apikey122"}, {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, headers = utils.get(kGetURL, {apikey = "apikey122"}, {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("With authentication and overridden application plugin", function()
-      it("should get blocked if exceeding rate limiting", function()
-        local response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey124"}, {host = "test6.com"})
-        assert.are.equal(200, status)
+    describe("Plugin customized for specific application", function()
 
-        response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey124"}, {host = "test6.com"})
-        assert.are.equal(200, status)
+      it("should get blocked if exceeding limit", function()
+        -- This plugin says this application can make 4 requests/minute, not 2 like fault
+        local limit = 4
 
-        response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey124"}, {host = "test6.com"})
-        assert.are.equal(200, status)
+        for i = 1, limit do
+          local response, status, headers = utils.get(kGetURL, {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
 
-        response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey124"}, {host = "test6.com"})
-        assert.are.equal(200, status)
-
-        response, status, headers = utils.get(kProxyURL.."get", {apikey = "apikey124"}, {host = "test6.com"})
+        local response, status, headers = utils.get(kGetURL, {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)
 
+    end)
+  end)
 end)

spec/unit/dao/cassandra_spec.lua

@@ -411,7 +411,6 @@ describe("Cassandra DAO #dao #cassandra", function()
       end)
 
     end)
-
   end)
 
   describe(":delete()", function()
@@ -626,102 +625,95 @@ describe("Cassandra DAO #dao #cassandra", function()
   end)
 
   describe("Metrics", function()
+    local utils = require "kong.tools.utils"
     local metrics = dao_factory.metrics
-    local session = dao_factory._db
+
+    local api_id = uuid()
+    local identifier = uuid()
 
     after_each(function()
       dao_factory:drop()
     end)
 
+    it("should return nil when metrics are not existing", function()
+      local current_timestamp = 1424217600
+      local periods = utils.get_timestamps(current_timestamp)
+      -- Very first select should return nil
+      for period, period_date in pairs(periods) do
+        local metric, err = metrics:find_one(api_id, identifier, current_timestamp, period)
+        assert.falsy(err)
+        assert.are.same(nil, metric)
+      end
+    end)
+
     it("should increment metrics with the given period", function()
-      local api_id = uuid()
-      local identifier = uuid()
+      local current_timestamp = 1424217600
+      local periods = utils.get_timestamps(current_timestamp)
 
       -- First increment
-      local ok, err = metrics:increment(api_id, identifier, { "second", "minute" })
+      local ok, err = metrics:increment(api_id, identifier, current_timestamp)
       assert.falsy(err)
       assert.True(ok)
 
       -- First select
-      local data, err = metrics:find(api_id, identifier, { "second", "minute" })
-      assert.falsy(err)
-      assert.are.same(2, #data)
-
-      data.meta = nil
-
-      assert.are.same({
-        {
+      for period, period_date in pairs(periods) do
+        local metric, err = metrics:find_one(api_id, identifier, current_timestamp, period)
+        assert.falsy(err)
+        assert.are.same({
           api_id = api_id,
           identifier = identifier,
-          period = "second",
-          value = 1
-        },
-        {
-          api_id =  api_id,
-          identifier = identifier,
-          period = "minute",
-          value = 1
-        }
-      }, data)
+          period = period,
+          period_date = period_date,
+          value = 1 -- The important part
+        }, metric)
+      end
 
       -- Second increment
-      local ok, err = metrics:increment(api_id, identifier, { "second", "minute", "hour" })
+      local ok, err = metrics:increment(api_id, identifier, current_timestamp)
       assert.falsy(err)
       assert.True(ok)
 
       -- Second select
-      local data, err = metrics:find(api_id, identifier, { "second", "minute", "hour" })
-      assert.falsy(err)
-      assert.are.same(3, #data)
-
-      data.meta = nil
-
-      assert.are.same({
-        {
+      for period, period_date in pairs(periods) do
+        local metric, err = metrics:find_one(api_id, identifier, current_timestamp, period)
+        assert.falsy(err)
+        assert.are.same({
           api_id = api_id,
           identifier = identifier,
-          period = "second",
-          value = 2
-        },
-        {
-          api_id =  api_id,
-          identifier = identifier,
-          period = "minute",
-          value = 2
-        },
-        {
-          api_id =  api_id,
-          identifier = identifier,
-          period = "hour",
-          value = 1
-        }
-      }, data)
-    end)
-
-    it("should delete metrics with the given period", function()
-      local api_id = uuid()
-      local identifier = uuid()
-
-      -- Increment
-      local ok, err = metrics:increment(api_id, identifier, { "second", "minute", "hour", "day" })
+          period = period,
+          period_date = period_date,
+          value = 2 -- The important part
+        }, metric)
+      end
+
+      -- 1 second delay
+      current_timestamp = 1424217601
+      periods = utils.get_timestamps(current_timestamp)
+
+       -- Third increment
+      local ok, err = metrics:increment(api_id, identifier, current_timestamp)
       assert.falsy(err)
       assert.True(ok)
 
-      -- First select
-      local data, err = metrics:find(api_id, identifier, { "second", "minute", "hour", "day" })
-      assert.falsy(err)
-      assert.are.same(4, #data)
+      -- Third select with 1 second delay
+      for period, period_date in pairs(periods) do
 
-      -- Delete
-      local ok, err = metrics:delete(api_id, identifier, { "second", "minute", "hour", "day" })
-      assert.falsy(err)
-      assert.True(ok)
+        local expected_value = 3
 
-      -- Second select
-      local data, err = metrics:find(api_id, identifier, { "second", "minute", "hour", "day" })
-      assert.falsy(err)
-      assert.are.same(0, #data)
-    end)
+        if period == "second" then
+          expected_value = 1
+        end
 
+        local metric, err = metrics:find_one(api_id, identifier, current_timestamp, period)
+        assert.falsy(err)
+        assert.are.same({
+          api_id = api_id,
+          identifier = identifier,
+          period = period,
+          period_date = period_date,
+          value = expected_value -- The important part
+        }, metric)
+      end
+    end)
   end)
 end)

spec/unit/utils_spec.lua

@@ -1,6 +1,5 @@
 local utils = require "kong.tools.utils"
 local cjson = require "cjson"
-local os = require "os"
 
 describe("Utils #utils", function()
 
@@ -54,4 +53,36 @@ describe("Utils #utils", function()
     end)
   end)
 
+  describe("tables", function()
+
+    describe("#is_empty()", function()
+
+      it("should return true for empty table, false otherwise", function()
+        assert.True(utils.is_empty({}))
+        assert.is_not_true(utils.is_empty({ foo = "bar" }))
+        assert.is_not_true(utils.is_empty({ "foo", "bar" }))
+      end)
+
+    end)
+
+    describe("#table_size()", function()
+
+      it("should return the size of a table", function()
+        assert.are.same(0, utils.table_size({}))
+        assert.are.same(1, utils.table_size({ foo = "bar" }))
+        assert.are.same(2, utils.table_size({ foo = "bar", bar = "baz" }))
+        assert.are.same(2, utils.table_size({ "foo", "bar" }))
+      end)
+
+    end)
+
+    describe("#reverse_table()", function()
+
+      it("should reverse an array", function()
+        local arr = { "a", "b", "c", "d" }
+        assert.are.same({ "d", "c", "b", "a" }, utils.reverse_table(arr))
+      end)
+
+    end)
+  end)
 end)