Merge pull request #2256 from Mashape/feat/delete-target

feat(admin) sugar method to delete upstream target

kong/api/routes/upstreams.lua

@@ -1,4 +1,70 @@
 local crud = require "kong.api.crud_helpers"
+local app_helpers = require "lapis.application"
+local responses = require "kong.tools.responses"
+
+-- clean the target history for a given upstream
+local function clean_history(upstream_id, dao_factory)
+  -- when to cleanup: invalid-entries > (valid-ones * cleanup_factor)
+  local cleanup_factor = 10
+
+  --cleaning up history, check if it's necessary...
+  local target_history = dao_factory.targets:find_all({
+    upstream_id = upstream_id
+  })
+
+  if target_history then
+    -- sort the targets
+    for _,target in ipairs(target_history) do
+      target.order = target.created_at..":"..target.id
+    end
+
+    -- sort table in reverse order
+    table.sort(target_history, function(a,b) return a.order>b.order end)
+    -- do clean up
+    local cleaned = {}
+    local delete = {}
+
+    for _, entry in ipairs(target_history) do
+      if cleaned[entry.target] then
+        -- we got a newer entry for this target than this, so this one can go
+        delete[#delete+1] = entry
+
+      else
+        -- haven't got this one, so this is the last one for this target
+        cleaned[entry.target] = true
+        cleaned[#cleaned+1] = entry
+        if entry.weight == 0 then
+          delete[#delete+1] = entry
+        end
+      end
+    end
+
+    -- do we need to cleanup?
+    -- either nothing left, or when 10x more outdated than active entries
+    if (#cleaned == 0 and #delete > 0) or
+       (#delete >= (math.max(#cleaned,1)*cleanup_factor)) then
+
+      ngx.log(ngx.INFO, "[admin api] Starting cleanup of target table for upstream ",
+                        tostring(upstream_id))
+      local cnt = 0
+      for _, entry in ipairs(delete) do
+        -- not sending update events, one event at the end, based on the
+        -- post of the new entry should suffice to reload only once
+        dao_factory.targets:delete(
+          { id = entry.id },
+          { quiet = true }
+        )
+        -- ignoring errors here, deleted by id, so should not matter
+        -- in case another kong-node does the same cleanup simultaneously
+        cnt = cnt + 1
+      end
+
+      ngx.log(ngx.INFO, "[admin api] Finished cleanup of target table",
+        " for upstream ", tostring(upstream_id),
+        " removed ", tostring(cnt), " target entries")
+    end
+  end
+end
 
 return {
   ["/upstreams/"] = {
@@ -44,67 +110,81 @@ return {
     end,
 
     POST = function(self, dao_factory, helpers)
-      -- when to cleanup: invalid-entries > (valid-ones * cleanup_factor)
-      local cleanup_factor = 10
-
-      --cleaning up history, check if it's necessary...
-      local target_history = dao_factory.targets:find_all(
-            { upstream_id = self.params.upstream_id })
-
-      if target_history then --ignoring errors here, will be caught when posting below
-        -- sort the targets
-        for _,target in ipairs(target_history) do
-          target.order = target.created_at..":"..target.id
-        end
+      clean_history(self.params.upstream_id, dao_factory)
+
+      crud.post(self.params, dao_factory.targets)
+    end,
+  },
+
+  ["/upstreams/:name_or_id/targets/active/"] = {
+    before = function(self, dao_factory, helpers)
+      crud.find_upstream_by_name_or_id(self, dao_factory, helpers)
+      self.params.upstream_id = self.upstream.id
+    end,
+
+    GET = function(self, dao_factory)
+      self.params.active = nil
+
+      local target_history, err = dao_factory.targets:find_all({
+        upstream_id = self.params.upstream_id,
+      })
+      if not target_history then
+        return app_helpers.yield_error(err)
+      end
+
+      --sort and walk based on target and creation time
+      for _, target in ipairs(target_history) do
+        target.order = target.target .. ":" ..
+          target.created_at .. ":" ..target.id
+      end
+      table.sort(target_history, function(a, b) return a.order > b.order end)
 
-        -- sort table in reverse order
-        table.sort(target_history, function(a,b) return a.order>b.order end)
-        -- do clean up
-        local cleaned = {} 
-        local delete = {}
+      local ignored = {}
+      local found   = {}
+      local found_n = 0
 
-        for _, entry in ipairs(target_history) do
-          if cleaned[entry.target] then
-            -- we got a newer entry for this target than this, so this one can go
-            delete[#delete+1] = entry
+      for _, entry in ipairs(target_history) do
+        if not found[entry.target] and not ignored[entry.target] then
+          if entry.weight ~= 0 then
+            entry.order = nil -- dont show our order key to the client
+            found_n = found_n + 1
+            found[found_n] = entry
 
           else
-            -- haven't got this one, so this is the last one for this target
-            cleaned[entry.target] = true
-            cleaned[#cleaned+1] = entry
-            if entry.weight == 0 then
-              delete[#delete+1] = entry
-            end
+            ignored[entry.target] = true
           end
         end
-
-        -- do we need to cleanup?
-        -- either nothing left, or when 10x more outdated than active entries
-        if (#cleaned == 0 and #delete > 0) or
-           (#delete >= (math.max(#cleaned,1)*cleanup_factor)) then
-
-          ngx.log(ngx.INFO, "[admin api] Starting cleanup of target table for upstream ", 
-                            tostring(self.params.upstream_id))
-          local cnt = 0
-          for _, entry in ipairs(delete) do
-            -- not sending update events, one event at the end, based on the
-            -- post of the new entry should suffice to reload only once
-            dao_factory.targets:delete(
-              { id = entry.id }, 
-              { quiet = true }
-            )
-            -- ignoring errors here, deleted by id, so should not matter
-            -- in case another kong-node does the same cleanup simultaneously
-            cnt = cnt + 1 
-          end
-
-          ngx.log(ngx.INFO, "[admin api] Finished cleanup of target table",
-            " for upstream ", tostring(self.params.upstream_id),
-            " removed ", tostring(cnt), " target entries")
-        end
       end
 
-      crud.post(self.params, dao_factory.targets)
-    end,
+      -- for now lets not worry about rolling our own pagination
+      -- we also end up returning a "backwards" list of targets because
+      -- of how we sorted- do we care?
+      return responses.send_HTTP_OK {
+        total = found_n,
+        data  = found,
+      }
+    end
   },
+
+  ["/upstreams/:name_or_id/targets/:target"] = {
+    before = function(self, dao_factory, helpers)
+      crud.find_upstream_by_name_or_id(self, dao_factory, helpers)
+    end,
+
+    DELETE = function(self, dao_factory)
+      clean_history(self.upstream.id, dao_factory)
+
+      -- this is just a wrapper around POSTing a new target with weight=0
+      local data, err = dao_factory.targets:insert({
+        target      = self.params.target,
+        upstream_id = self.upstream.id,
+        weight      = 0,
+      })
+      if err then
+        return app_helpers.yield_error(err)
+      end
+
+      return responses.send_HTTP_NO_CONTENT()
+    end
+  }
 }

spec/02-integration/03-admin_api/09-targets_routes_spec.lua

@@ -259,4 +259,118 @@ describe("Admin API", function()
       end)
     end)
   end)
+
+  describe("/upstreams/{upstream}/targets/active/", function()
+    describe("only shows active targets", function()
+      local upstream_name3 = "example.com"
+
+      before_each(function()
+        local upstream3 = assert(helpers.dao.upstreams:insert {
+          name = upstream_name3,
+        })
+
+        -- two target inserts, resulting in a 'down' target
+        assert(helpers.dao.targets:insert {
+          target = "api-1:80",
+          weight = 10,
+          upstream_id = upstream3.id,
+        })
+        assert(helpers.dao.targets:insert {
+          target = "api-1:80",
+          weight = 0,
+          upstream_id = upstream3.id,
+        })
+
+        -- three target inserts, resulting in an 'up' target
+        assert(helpers.dao.targets:insert {
+          target = "api-2:80",
+          weight = 10,
+          upstream_id = upstream3.id,
+        })
+        assert(helpers.dao.targets:insert {
+          target = "api-2:80",
+          weight = 0,
+          upstream_id = upstream3.id,
+        })
+        assert(helpers.dao.targets:insert {
+          target = "api-2:80",
+          weight = 10,
+          upstream_id = upstream3.id,
+        })
+
+        -- and an insert of a separate active target
+        assert(helpers.dao.targets:insert {
+          target = "api-3:80",
+          weight = 10,
+          upstream_id = upstream3.id,
+        })
+      end)
+
+      it("only shows active targets", function()
+        local res = assert(client:send {
+          method = "GET",
+          path = "/upstreams/" .. upstream_name3 .. "/targets/active/",
+        })
+        assert.response(res).has.status(200)
+        local json = assert.response(res).has.jsonbody()
+        assert.equal(2, #json.data)
+        assert.equal(2, json.total)
+        assert.equal("api-3:80", json.data[1].target)
+        assert.equal("api-2:80", json.data[2].target)
+      end)
+    end)
+  end)
+
+  describe("/upstreams/{upstream}/targets/{target}", function()
+    describe("DELETE", function()
+      local target
+      local upstream_name4 = "example4.com"
+
+      before_each(function()
+        local upstream4 = assert(helpers.dao.upstreams:insert {
+          name = upstream_name4,
+        })
+
+        assert(helpers.dao.targets:insert {
+          target = "api-1:80",
+          weight = 10,
+          upstream_id = upstream4.id,
+        })
+
+        -- predefine the target to mock delete
+        target = assert(helpers.dao.targets:insert {
+          target = "api-2:80",
+          weight = 10,
+          upstream_id = upstream4.id,
+        })
+      end)
+
+      it("acts as a sugar method to POST a target with 0 weight", function()
+        local res = assert(client:send {
+          method = "DELETE",
+          path = "/upstreams/" .. upstream_name4 .. "/targets/" .. target.target
+        })
+        assert.response(res).has.status(204)
+
+        local targets = assert(client:send {
+          method = "GET",
+          path = "/upstreams/" .. upstream_name4 .. "/targets/",
+        })
+        assert.response(targets).has.status(200)
+        local json = assert.response(targets).has.jsonbody()
+        assert.equal(3, #json.data)
+        assert.equal(3, json.total)
+
+        local active = assert(client:send {
+          method = "GET",
+          path = "/upstreams/" .. upstream_name4 .. "/targets/active/",
+        })
+        assert.response(active).has.status(200)
+        json = assert.response(active).has.jsonbody()
+        assert.equal(1, #json.data)
+        assert.equal(1, json.total)
+        assert.equal("api-1:80", json.data[1].target)
+      end)
+    end)
+  end)
 end)