fix(api) use POST to add targets to specific upstream

In Kong 2.x series, for a compatibility with old versions, it was possible to update targets using POST method. When fixing this behavior, the path /upstreams/{upstream}/targets was mistakenly changed to also use PUT method, instead of only blocking adding duplicated targets. This change fixes this behavior.
Kong · May 12, 2022 · ff49f02 · ff49f02
1 parent bb2beb8
commit ff49f02
Show file tree

Hide file tree

Showing 7 changed files with 72 additions and 129 deletions.
diff --git a/kong/api/routes/upstreams.lua b/kong/api/routes/upstreams.lua
@@ -112,21 +112,6 @@ local function target_endpoint(self, db, callback)
 end
 
 
-local function update_existent_target(self, db)
-  local upstream = endpoints.select_entity(self, db, db.upstreams.schema)
-  local filter = { target = unescape_uri(self.params.target) }
-  local opts = endpoints.extract_options(self.args.uri, db.targets.schema, "select")
-  local target = db.targets:select_by_upstream_filter(upstream, filter, opts)
-
-  if target then
-    self.params.targets = db.targets.schema:extract_pk_values(target)
-    return endpoints.update_entity(self, db, db.targets.schema)
-  end
-
-  return nil
-end
-
-
 return {
   ["/upstreams/:upstreams/health"] = {
     GET = function(self, db)
@@ -180,22 +165,11 @@ return {
                                             kong.db.upstreams.schema,
                                             "upstream",
                                             "page_for_upstream"),
-    PUT = function(self, db)
-      local entity, _, err_t = update_existent_target(self, db)
-      if err_t then
-        return endpoints.handle_error(err_t)
-      end
-      if entity then
-        return kong.response.exit(200, entity, { ["Deprecation"] = "true" })
-      end
-
+    POST = function(self, db)
       local create = endpoints.post_collection_endpoint(kong.db.targets.schema,
                         kong.db.upstreams.schema, "upstream")
       return create(self, db)
     end,
-    POST = function(self, db)
-      return kong.response.exit(405)
-    end,
   },
 
   ["/upstreams/:upstreams/targets/all"] = {

diff --git a/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua b/spec/02-integration/04-admin_api/07-upstreams_routes_spec.lua
@@ -722,14 +722,12 @@ describe("Admin API: #" .. strategy, function()
         client = assert(helpers.admin_client())
 
         -- create the target
-        local res = assert(client:send {
-          method = "PUT",
-          path = "/upstreams/my-upstream/targets",
+        local res = assert(client:post("/upstreams/my-upstream/targets", {
           body = {
             target = "127.0.0.1:8000",
           },
           headers = { ["Content-Type"] = "application/json" }
-        })
+        }))
 
         assert.response(res).has.status(201)
 
@@ -790,14 +788,12 @@ describe("Admin API: #" .. strategy, function()
         client = assert(helpers.admin_client())
 
         -- create the target
-        local res = assert(client:send {
-          method = "PUT",
-          path = "/upstreams/my-upstream/targets",
+        local res = assert(client:post("/upstreams/my-upstream/targets", {
           body = {
             target = "127.0.0.1:8000",
           },
           headers = { ["Content-Type"] = "application/json" }
-        })
+        }))
 
         assert.response(res).has.status(201)
 

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
@@ -74,21 +74,19 @@ describe("Admin API #" .. strategy, function()
   end)
 
   describe("/upstreams/{upstream}/targets/", function()
-    describe("PUT", function()
+    describe("POST", function()
       it_content_types("creates a target with defaults", function(content_type)
         return function()
           local upstream = bp.upstreams:insert { slots = 10 }
-          local res = assert(client:send {
-            method = "PUT",
-            path = "/upstreams/" .. upstream.name .. "/targets/",
+          local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
             body = {
-              target = "mashape.com",
+              target = "konghq.test",
             },
             headers = {["Content-Type"] = content_type}
-          })
+          }))
           assert.response(res).has.status(201)
           local json = assert.response(res).has.jsonbody()
-          assert.equal("mashape.com:" .. default_port, json.target)
+          assert.equal("konghq.test:" .. default_port, json.target)
           assert.is_number(json.created_at)
           assert.is_string(json.id)
           assert.are.equal(weight_default, json.weight)
@@ -97,18 +95,16 @@ describe("Admin API #" .. strategy, function()
       it_content_types("creates a target without defaults", function(content_type)
         return function()
           local upstream = bp.upstreams:insert { slots = 10 }
-          local res = assert(client:send {
-            method = "PUT",
-            path = "/upstreams/" .. upstream.name .. "/targets/",
+          local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
             body = {
-              target = "mashape.com:123",
+              target = "konghq.test:123",
               weight = 99,
             },
             headers = {["Content-Type"] = content_type}
-          })
+          }))
           assert.response(res).has.status(201)
           local json = assert.response(res).has.jsonbody()
-          assert.equal("mashape.com:123", json.target)
+          assert.equal("konghq.test:123", json.target)
           assert.is_number(json.created_at)
           assert.is_string(json.id)
           assert.are.equal(99, json.weight)
@@ -118,15 +114,13 @@ describe("Admin API #" .. strategy, function()
       it_content_types("creates a target with weight = 0", function(content_type)
         return function()
           local upstream = bp.upstreams:insert { slots = 10 }
-          local res = assert(client:send {
-            method = "PUT",
-            path = "/upstreams/" .. upstream.name .. "/targets/",
+          local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
             body = {
               target = "zero.weight.test:8080",
               weight = 0,
             },
             headers = {["Content-Type"] = content_type}
-          })
+          }))
           assert.response(res).has.status(201)
           local json = assert.response(res).has.jsonbody()
           assert.equal("zero.weight.test:8080", json.target)
@@ -142,52 +136,13 @@ describe("Admin API #" .. strategy, function()
         end
       end)
 
-      it_content_types("updates and does not create duplicated targets (#deprecated)", function(content_type)
-        return function()
-          local upstream = bp.upstreams:insert { slots = 10 }
-          local res = assert(client:send {
-            method = "PUT",
-            path = "/upstreams/" .. upstream.name .. "/targets/",
-            body = {
-              target = "single-target.test:8080",
-              weight = 1,
-            },
-            headers = {["Content-Type"] = content_type}
-          })
-          assert.response(res).has.status(201)
-          local json = assert.response(res).has.jsonbody()
-          assert.equal("single-target.test:8080", json.target)
-          assert.is_number(json.created_at)
-          assert.is_string(json.id)
-          local id = json.id
-          assert.are.equal(1, json.weight)
-
-          local res = assert(client:send {
-            method = "PUT",
-            path = "/upstreams/" .. upstream.name .. "/targets/",
-            body = {
-              target = "single-target.test:8080",
-              weight = 100,
-            },
-            headers = {["Content-Type"] = content_type}
-          })
-          local body = assert.response(res).has.status(200)
-          local json = cjson.decode(body)
-          assert.are.equal(100, json.weight)
-          assert.are.equal(id, json.id)
-          assert.equal("true", res.headers["Deprecation"])
-        end
-      end)
-
       describe("errors", function()
         it("handles malformed JSON body", function()
           local upstream = bp.upstreams:insert { slots = 10 }
-          local res = assert(client:request {
-            method = "PUT",
-            path = "/upstreams/" .. upstream.name .. "/targets/",
+          local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
             body = '{"hello": "world"',
             headers = {["Content-Type"] = "application/json"}
-          })
+          }))
           local body = assert.response(res).has.status(400)
           local json = cjson.decode(body)
           assert.same({ message = "Cannot parse JSON body" }, json)
@@ -196,39 +151,35 @@ describe("Admin API #" .. strategy, function()
           return function()
             local upstream = bp.upstreams:insert { slots = 10 }
             -- Missing parameter
-            local res = assert(client:send {
-              method = "PUT",
-              path = "/upstreams/" .. upstream.name .. "/targets/",
+            local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
               body = {
                 weight = weight_min,
               },
               headers = {["Content-Type"] = content_type}
-            })
+            }))
             local body = assert.response(res).has.status(400)
             local json = cjson.decode(body)
             assert.equal("schema violation", json.name)
             assert.same({ target = "required field missing" }, json.fields)
 
             -- Invalid target parameter
-            res = assert(client:send {
-              method = "PUT",
-              path = "/upstreams/" .. upstream.name .. "/targets/",
+            res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
               body = {
                 target = "some invalid host name",
               },
               headers = {["Content-Type"] = content_type}
-            })
+            }))
             body = assert.response(res).has.status(400)
             local json = cjson.decode(body)
             assert.equal("schema violation", json.name)
             assert.same({ target = "Invalid target; not a valid hostname or ip address" }, json.fields)
 
             -- Invalid weight parameter
             res = assert(client:send {
-              method = "PUT",
+              method = "POST",
               path = "/upstreams/" .. upstream.name .. "/targets/",
               body = {
-                target = "mashape.com",
+                target = "konghq.test",
                 weight = weight_max + 1,
               },
               headers = {["Content-Type"] = content_type}
@@ -240,22 +191,50 @@ describe("Admin API #" .. strategy, function()
           end
         end)
 
-        for _, method in ipairs({"POST", "PATCH", "DELETE"}) do
+        for _, method in ipairs({"PUT", "PATCH", "DELETE"}) do
           it_content_types("returns 405 on " .. method, function(content_type)
             return function()
               local upstream = bp.upstreams:insert { slots = 10 }
               local res = assert(client:send {
                 method = method,
                 path = "/upstreams/" .. upstream.name .. "/targets/",
                 body = {
-                  target = "mashape.com",
+                  target = "konghq.test",
                 },
                 headers = {["Content-Type"] = content_type}
               })
               assert.response(res).has.status(405)
             end
           end)
         end
+
+        it_content_types("fails to create duplicated targets", function(content_type)
+          return function()
+            local upstream = bp.upstreams:insert { slots = 10 }
+            local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
+              body = {
+                target = "single-target.test:8080",
+                weight = 1,
+              },
+              headers = {["Content-Type"] = content_type}
+            }))
+            assert.response(res).has.status(201)
+            local json = assert.response(res).has.jsonbody()
+            assert.equal("single-target.test:8080", json.target)
+            assert.is_number(json.created_at)
+            assert.is_string(json.id)
+            assert.are.equal(1, json.weight)
+
+            local res = assert(client:post("/upstreams/" .. upstream.name .. "/targets/", {
+              body = {
+                target = "single-target.test:8080",
+                weight = 100,
+              },
+              headers = {["Content-Type"] = content_type}
+            }))
+            assert.response(res).has.status(409)
+          end
+        end)
       end)
     end)
 
@@ -337,7 +316,7 @@ describe("Admin API #" .. strategy, function()
 
         for i = 1, #weights do
           local status, body = client_send({
-            method = "PUT",
+            method = "POST",
             path = "/upstreams/" .. upstream.name .. "/targets",
             headers = {
               ["Content-Type"] = "application/json",
@@ -668,7 +647,7 @@ describe("Admin API #" .. strategy, function()
           local json = assert(cjson.decode(body))
 
           status, body = assert(client_send({
-            method = "PUT",
+            method = "POST",
             path = "/upstreams/" .. upstream.id .. "/targets",
             headers = {["Content-Type"] = "application/json"},
             body = {

diff --git a/spec/02-integration/05-proxy/10-balancer/01-healthchecks_spec.lua b/spec/02-integration/05-proxy/10-balancer/01-healthchecks_spec.lua
@@ -697,11 +697,11 @@ for _, strategy in helpers.each_strategy() do
 
             if mode == "ipv6" then
               -- TODO /upstreams does not understand shortened IPv6 addresses
-              bu.put_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port, "unhealthy")
+              bu.post_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port, "unhealthy")
               bu.poll_wait_health(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port, "UNHEALTHY", admin_port_1)
               bu.poll_wait_health(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port, "UNHEALTHY", admin_port_2)
             else
-              bu.put_target_endpoint(upstream_id, localhost, port, "unhealthy")
+              bu.post_target_endpoint(upstream_id, localhost, port, "unhealthy")
               bu.poll_wait_health(upstream_id, localhost, port, "UNHEALTHY", admin_port_1)
               bu.poll_wait_health(upstream_id, localhost, port, "UNHEALTHY", admin_port_2)
             end
@@ -1996,10 +1996,10 @@ for _, strategy in helpers.each_strategy() do
                 -- manually bring it back using the endpoint
                 if mode == "ipv6" then
                   -- TODO /upstreams does not understand shortened IPv6 addresses
-                  bu.put_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "healthy")
+                  bu.post_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "healthy")
                   bu.poll_wait_health(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "HEALTHY")
                 else
-                  bu.put_target_endpoint(upstream_id, localhost, port2, "healthy")
+                  bu.post_target_endpoint(upstream_id, localhost, port2, "healthy")
                   bu.poll_wait_health(upstream_id, localhost, port2, "HEALTHY")
                 end
 
@@ -2040,7 +2040,7 @@ for _, strategy in helpers.each_strategy() do
                 }
               })
               local port1 = bu.add_target(bp, upstream_id, localhost)
-              local port2 = bu.add_target(bp, upstream_id, localhost)
+              local port2, target2 = bu.add_target(bp, upstream_id, localhost)
               local api_host = bu.add_api(bp, upstream_name)
               bu.end_testcase_setup(strategy, bp)
 
@@ -2058,10 +2058,10 @@ for _, strategy in helpers.each_strategy() do
               -- manually bring it down using the endpoint
               if mode == "ipv6" then
                 -- TODO /upstreams does not understand shortened IPv6 addresses
-                bu.put_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "unhealthy")
+                bu.put_target_address_health(upstream_id, target2.id, "[0000:0000:0000:0000:0000:0000:0000:0001]:".. port2, "unhealthy")
                 bu.poll_wait_health(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "UNHEALTHY")
               else
-                bu.put_target_endpoint(upstream_id, localhost, port2, "unhealthy")
+                bu.put_target_address_health(upstream_id, target2.id, localhost .. ":" .. port2, "unhealthy")
                 bu.poll_wait_health(upstream_id, localhost, port2, "UNHEALTHY")
               end
 
@@ -2075,10 +2075,10 @@ for _, strategy in helpers.each_strategy() do
               -- manually bring it back using the endpoint
               if mode == "ipv6" then
                 -- TODO /upstreams does not understand shortened IPv6 addresses
-                bu.put_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "healthy")
+                bu.post_target_endpoint(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "healthy")
                 bu.poll_wait_health(upstream_id, "[0000:0000:0000:0000:0000:0000:0000:0001]", port2, "HEALTHY")
               else
-                bu.put_target_endpoint(upstream_id, localhost, port2, "healthy")
+                bu.put_target_address_health(upstream_id, target2.id, localhost .. ":" .. port2, "healthy")
                 bu.poll_wait_health(upstream_id, localhost, port2, "HEALTHY")
               end