From dc6beb68e3c57e8c3995276cd51ca35ddb78bd55 Mon Sep 17 00:00:00 2001 From: Harry Bagdi Date: Wed, 22 Nov 2017 15:52:01 -0800 Subject: [PATCH] feat(admin) change behaviour of GET target requests The endpoints to list active and all Targets associated with an Upstream have been changed as follows: * `GET /upstreams/:upstream_name_or_id/targets/` now lists only the active Targets instead of listing all The targets. Previously, active targets were available at the endpoint `/upstreams/:upstream_name_or_id/targets/active`, which is now removed. * `GET /upstreams/:upstream_name_or_id/targets/all` endpiont is introduced, which returns a list of all the Targets (old and current). --- kong/api/routes/upstreams.lua | 34 ++--- .../04-admin_api/08-targets_routes_spec.lua | 136 +++++++++--------- 2 files changed, 85 insertions(+), 85 deletions(-) diff --git a/kong/api/routes/upstreams.lua b/kong/api/routes/upstreams.lua index ce96f445128a..93f04f9d6f7c 100644 --- a/kong/api/routes/upstreams.lua +++ b/kong/api/routes/upstreams.lua @@ -107,23 +107,6 @@ return { self.params.upstream_id = self.upstream.id end, - GET = function(self, dao_factory) - crud.paginated_set(self, dao_factory.targets) - end, - - POST = function(self, dao_factory, helpers) - clean_history(self.params.upstream_id, dao_factory) - - crud.post(self.params, dao_factory.targets) - end, - }, - - ["/upstreams/:upstream_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 @@ -171,6 +154,23 @@ return { total = active_n, data = active, } + end, + + POST = function(self, dao_factory, helpers) + clean_history(self.params.upstream_id, dao_factory) + + crud.post(self.params, dao_factory.targets) + end, + }, + + ["/upstreams/:upstream_name_or_id/targets/all"] = { + 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) + crud.paginated_set(self, dao_factory.targets) end }, diff --git a/spec/02-integration/04-admin_api/08-targets_routes_spec.lua b/spec/02-integration/04-admin_api/08-targets_routes_spec.lua index 59fab906a974..7fb8f3a98954 100644 --- a/spec/02-integration/04-admin_api/08-targets_routes_spec.lua +++ b/spec/02-integration/04-admin_api/08-targets_routes_spec.lua @@ -170,6 +170,65 @@ describe("Admin API", function() end) end) + describe("GET", function() + local upstream_name3 = "example.com" + local apis = {} + + before_each(function() + local upstream3 = assert(helpers.dao.upstreams:insert { + name = upstream_name3, + }) + + -- 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 + } + + for i = 1, #weights do + for j = 1, #weights[i] do + ngx.sleep(0.01) + 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() + for _, append in ipairs({ "", "/" }) do + local res = assert(client:send { + method = "GET", + path = "/upstreams/" .. upstream_name3 .. "/targets" .. append, + }) + assert.response(res).has.status(200) + local json = assert.response(res).has.jsonbody() + + -- 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) + end) + + describe("/upstreams/{upstream}/targets/all/", function() describe("GET", function() before_each(function() for i = 1, 10 do @@ -184,7 +243,7 @@ describe("Admin API", function() it("retrieves the first page", function() local res = assert(client:send { methd = "GET", - path = "/upstreams/" .. upstream_name .. "/targets/", + path = "/upstreams/" .. upstream_name .. "/targets/all", }) assert.response(res).has.status(200) local json = assert.response(res).has.jsonbody() @@ -198,7 +257,7 @@ describe("Admin API", function() for i = 1, 4 do local res = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name .. "/targets/", + path = "/upstreams/" .. upstream_name .. "/targets/all", query = {size = 3, offset = offset} }) assert.response(res).has.status(200) @@ -223,7 +282,7 @@ describe("Admin API", function() it("handles invalid filters", function() local res = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name .. "/targets/", + path = "/upstreams/" .. upstream_name .. "/targets/all", query = {foo = "bar"}, }) local body = assert.response(res).has.status(400) @@ -233,7 +292,7 @@ describe("Admin API", function() it("ignores an invalid body", function() local res = assert(client:send { methd = "GET", - path = "/upstreams/" .. upstream_name .. "/targets/", + path = "/upstreams/" .. upstream_name .. "/targets/all", body = "this fails if decoded as json", headers = { ["Content-Type"] = "application/json", @@ -255,7 +314,7 @@ describe("Admin API", function() it("data property is an empty array", function() local res = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name2 .. "/targets/", + path = "/upstreams/" .. upstream_name2 .. "/targets/all", }) local body = assert.response(res).has.status(200) local json = cjson.decode(body) @@ -265,65 +324,6 @@ describe("Admin API", function() end) end) - describe("/upstreams/{upstream}/targets/active/", function() - describe("GET", function() - local upstream_name3 = "example.com" - local apis = {} - - before_each(function() - local upstream3 = assert(helpers.dao.upstreams:insert { - name = upstream_name3, - }) - - -- 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 - } - - for i = 1, #weights do - for j = 1, #weights[i] do - ngx.sleep(0.01) - 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() - for _, append in ipairs({ "", "/" }) do - local res = assert(client:send { - method = "GET", - path = "/upstreams/" .. upstream_name3 .. "/targets/active" .. append, - }) - assert.response(res).has.status(200) - local json = assert.response(res).has.jsonbody() - - -- 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) - end) - describe("/upstreams/{upstream}/targets/{target}", function() describe("DELETE", function() local target @@ -357,7 +357,7 @@ describe("Admin API", function() local targets = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name4 .. "/targets/", + path = "/upstreams/" .. upstream_name4 .. "/targets/all", }) assert.response(targets).has.status(200) local json = assert.response(targets).has.jsonbody() @@ -366,7 +366,7 @@ describe("Admin API", function() local active = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name4 .. "/targets/active", + path = "/upstreams/" .. upstream_name4 .. "/targets", }) assert.response(active).has.status(200) json = assert.response(active).has.jsonbody() @@ -384,7 +384,7 @@ describe("Admin API", function() local targets = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name4 .. "/targets/", + path = "/upstreams/" .. upstream_name4 .. "/targets/all", }) assert.response(targets).has.status(200) local json = assert.response(targets).has.jsonbody() @@ -393,7 +393,7 @@ describe("Admin API", function() local active = assert(client:send { method = "GET", - path = "/upstreams/" .. upstream_name4 .. "/targets/active", + path = "/upstreams/" .. upstream_name4 .. "/targets", }) assert.response(active).has.status(200) json = assert.response(active).has.jsonbody()