From ef051857e7c95ba556a09c311d0c03e4f2eca668 Mon Sep 17 00:00:00 2001
From: Robert Paprocki <robert@cryptobells.com>
Date: Thu, 30 Mar 2017 19:44:58 -0700
Subject: [PATCH] hotfix(upstream) display active targets based on proper
 definition

Previously we defined active upstream targets as any target with
a nonzero weight. This definition was properly implemented, but
caused confusion. As such, we redfine 'active targets' to be the most
recent entry of each nonzero target. This presents a more accurate
picture of the targets currently in use by the balancer.
---
 kong/api/routes/upstreams.lua                 | 28 ++++---
 .../03-admin_api/09-targets_routes_spec.lua   | 73 +++++++++----------
 2 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/kong/api/routes/upstreams.lua b/kong/api/routes/upstreams.lua
index 09b4a8c818d..71feac40b5a 100644
--- a/kong/api/routes/upstreams.lua
+++ b/kong/api/routes/upstreams.lua
@@ -140,19 +140,25 @@ return {
       end
       table.sort(target_history, function(a, b) return a.order > b.order end)
 
-      local ignored = {}
-      local found   = {}
-      local found_n = 0
+      local seen     = {}
+      local active   = {}
+      local active_n = 0
 
       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
+        if not seen[entry.target] then
+          if entry.weight == 0 then
+            seen[entry.target] = true
 
           else
-            ignored[entry.target] = true
+            entry.order = nil -- dont show our order key to the client
+
+            -- add what we want to send to the client in our array
+            active_n = active_n + 1
+            active[active_n] = entry
+
+            -- track that we found this host:port so we only show
+            -- the most recent one (kinda)
+            seen[entry.target] = true
           end
         end
       end
@@ -161,8 +167,8 @@ return {
       -- 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,
+        total = active_n,
+        data  = active,
       }
     end
   },
diff --git a/spec/02-integration/03-admin_api/09-targets_routes_spec.lua b/spec/02-integration/03-admin_api/09-targets_routes_spec.lua
index 18806d8aa28..5df1db99687 100644
--- a/spec/02-integration/03-admin_api/09-targets_routes_spec.lua
+++ b/spec/02-integration/03-admin_api/09-targets_routes_spec.lua
@@ -261,49 +261,36 @@ describe("Admin API", function()
   end)
 
   describe("/upstreams/{upstream}/targets/active/", function()
-    describe("only shows active targets", function()
+    describe("GET", function()
       local upstream_name3 = "example.com"
+      local apis = {}
 
       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,
-        })
+        -- testing various behaviors
+        -- for each index in weights, create a number of targets,
+        -- each with its weight as each element of the sub-array
+        local weights = {
+          { 10, 0 },        -- two targets, eventually resulting in down
+          { 10, 0, 10 },    -- three targets, eventually resulting in up
+          { 10 },           -- one target, up
+          { 10, 10 },       -- two targets, up (we should only see one)
+          { 10, 50, 0 },    -- three targets, two up in a row, eventually down
+          { 10, 0, 20, 0 }, -- four targets, eventually down
+        }
 
-        -- 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,
-        })
+        for i = 1, #weights do
+          for j = 1, #weights[i] do
+            apis[i] = assert(helpers.dao.targets:insert {
+              target = "api-" .. tostring(i) .. ":80",
+              weight = weights[i][j],
+              upstream_id = upstream3.id
+            })
+          end
+        end
       end)
 
       it("only shows active targets", function()
@@ -313,10 +300,18 @@ describe("Admin API", function()
         })
         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)
+
+        -- we got three active targets for this upstream
+        assert.equal(3, #json.data)
+        assert.equal(3, json.total)
+
+        -- when multiple active targets are present, we only see the last one
+        assert.equal(apis[4].id, json.data[1].id)
+
+        -- validate the remaining returned targets
+        -- note the backwards order, because we walked the targets backwards
+        assert.equal(apis[3].target, json.data[2].target)
+        assert.equal(apis[2].target, json.data[3].target)
       end)
     end)
   end)