Skip to content

Commit

Permalink
feat(admin) add better support for OPTIONS requests (#7830)
Browse files Browse the repository at this point in the history
### Summary

Previously Admin API always replied the same on all OPTIONS requests:

```lua
header["Access-Control-Allow-Methods"] = "GET, HEAD, PUT, PATCH, POST, DELETE"
header["Access-Control-Allow-Headers"] = "Content-Type"
return ngx.exit(204)
```

This commit changes that OPTIONS request only replies to routes that our
Admin API has. Non-existing routes will get `404`.

It also adds `Allow` header to responses, and both `Allow` and
 `Access-Control-Allow-Methods` now contain only the methods that
the specific API supports.
  • Loading branch information
bungle authored Sep 14, 2021
1 parent 2bb067a commit f92d245
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 17 deletions.
72 changes: 66 additions & 6 deletions kong/api/api_helpers.lua
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,31 @@ local Errors = require "kong.db.errors"
local singletons = require "kong.singletons"
local hooks = require "kong.hooks"

local ngx = ngx
local sub = string.sub
local find = string.find
local type = type
local pairs = pairs
local ipairs = ipairs

local ngx = ngx
local sub = string.sub
local find = string.find
local type = type
local pairs = pairs
local ipairs = ipairs


local _M = {}
local NO_ARRAY_INDEX_MARK = {}


local HTTP_METHODS = {
["GET"] = true,
["HEAD"] = true,
["POST"] = true,
["PUT"] = true,
["DELETE"] = true,
["CONNECT"] = true,
["OPTIONS"] = true,
["TRACE"] = true,
["PATCH"] = true,
}

-- Parses a form value, handling multipart/data values
-- @param `v` The value object
-- @return The parsed value
Expand Down Expand Up @@ -357,6 +372,17 @@ local function on_error(self)
end


local function options_method(methods)
return function()
kong.response.exit(204, nil, {
["Allow"] = methods,
["Access-Control-Allow-Methods"] = methods,
["Access-Control-Allow-Headers"] = "Content-Type"
})
end
end


local handler_helpers = {
yield_error = app_helpers.yield_error
}
Expand All @@ -366,16 +392,33 @@ function _M.attach_routes(app, routes)
for route_path, methods in pairs(routes) do
methods.on_error = methods.on_error or on_error

local http_methods_array = {}
local http_methods_count = 0

for method_name, method_handler in pairs(methods) do
local wrapped_handler = function(self)
return method_handler(self, {}, handler_helpers)
end

methods[method_name] = parse_params(wrapped_handler)

if HTTP_METHODS[method_name] then
http_methods_count = http_methods_count + 1
http_methods_array[http_methods_count] = method_name
end
end

if not methods["HEAD"] and methods["GET"] then
methods["HEAD"] = methods["GET"]
http_methods_count = http_methods_count + 1
http_methods_array[http_methods_count] = "HEAD"
end

if not methods["OPTIONS"] then
http_methods_count = http_methods_count + 1
http_methods_array[http_methods_count] = "OPTIONS"
table.sort(http_methods_array)
methods["OPTIONS"] = options_method(table.concat(http_methods_array, ", ", 1, http_methods_count))
end

app:match(route_path, route_path, app_helpers.respond_to(methods))
Expand All @@ -393,6 +436,9 @@ function _M.attach_new_db_routes(app, routes)

methods.on_error = methods.on_error or new_db_on_error

local http_methods_array = {}
local http_methods_count = 0

for method_name, method_handler in pairs(methods) do
local wrapped_handler = function(self)
self.args = arguments.load({
Expand All @@ -404,10 +450,24 @@ function _M.attach_new_db_routes(app, routes)
end

methods[method_name] = parse_params(wrapped_handler)

if HTTP_METHODS[method_name] then
http_methods_count = http_methods_count + 1
http_methods_array[http_methods_count] = method_name
end
end

if not methods["HEAD"] and methods["GET"] then
methods["HEAD"] = methods["GET"]
http_methods_count = http_methods_count + 1
http_methods_array[http_methods_count] = "HEAD"
end

if not methods["OPTIONS"] then
http_methods_count = http_methods_count + 1
http_methods_array[http_methods_count] = "OPTIONS"
table.sort(http_methods_array)
methods["OPTIONS"] = options_method(table.concat(http_methods_array, ", ", 1, http_methods_count))
end

app:match(route_path, route_path, app_helpers.respond_to(methods))
Expand Down
11 changes: 0 additions & 11 deletions kong/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -1387,17 +1387,6 @@ local function serve_content(module, options)

header["Access-Control-Allow-Origin"] = options.allow_origin or "*"

if ngx.req.get_method() == "OPTIONS" then
header["Access-Control-Allow-Methods"] = "GET, HEAD, PUT, PATCH, POST, DELETE"
header["Access-Control-Allow-Headers"] = "Content-Type"

ctx.KONG_ADMIN_CONTENT_ENDED_AT = get_now_ms()
ctx.KONG_ADMIN_CONTENT_TIME = ctx.KONG_ADMIN_CONTENT_ENDED_AT - ctx.KONG_ADMIN_CONTENT_START
ctx.KONG_ADMIN_LATENCY = ctx.KONG_ADMIN_CONTENT_ENDED_AT - ctx.KONG_PROCESSING_START

return ngx.exit(204)
end

lapis.serve(module)

ctx.KONG_ADMIN_CONTENT_ENDED_AT = get_now_ms()
Expand Down
24 changes: 24 additions & 0 deletions spec/02-integration/04-admin_api/02-kong_routes_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ describe("Admin API - Kong routes with strategy #" .. strategy, function()
assert.same(res1.headers, res2.headers)
end)

it("returns allow and CORS headers with OPTIONS method", function()
local res = assert(client:send {
method = "OPTIONS",
path = "/"
})

local body = assert.res_status(204, res)
assert.equal("", body)
assert.equal("GET, HEAD, OPTIONS", res.headers["Allow"])
assert.equal("GET, HEAD, OPTIONS", res.headers["Access-Control-Allow-Methods"])
assert.equal("Content-Type", res.headers["Access-Control-Allow-Headers"])
assert.equal("*", res.headers["Access-Control-Allow-Origin"])
assert.not_nil(res.headers["X-Kong-Admin-Latency"])
end)

it("returns Kong's version number and tagline", function()
local res = assert(client:send {
method = "GET",
Expand Down Expand Up @@ -458,6 +473,15 @@ describe("Admin API - Kong routes with strategy #" .. strategy, function()
local body = assert.res_status(404, res)
assert.equal("", body)
end)
it("returns 404 with OPTIONS", function()
local res = assert(client:send {
method = "OPTIONS",
path = "/non-existing"
})
local body = assert.res_status(404, res)
local json = cjson.decode(body)
assert.equal("Not found", json.message)
end)
it("returns 404 with GET", function()
local res = assert(client:send {
method = "GET",
Expand Down
33 changes: 33 additions & 0 deletions spec/02-integration/04-admin_api/09-routes_routes_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,22 @@ for _, strategy in helpers.each_strategy() do
end)

describe("/routes", function()
describe("OPTIONS", function()
it("returns allow and CORS headers with OPTIONS method", function()
local res = assert(client:send {
method = "OPTIONS",
path = "/routes"
})

local body = assert.res_status(204, res)
assert.equal("", body)
assert.equal("GET, HEAD, OPTIONS, POST", res.headers["Allow"])
assert.equal("GET, HEAD, OPTIONS, POST", res.headers["Access-Control-Allow-Methods"])
assert.equal("Content-Type", res.headers["Access-Control-Allow-Headers"])
assert.equal("*", res.headers["Access-Control-Allow-Origin"])
assert.not_nil(res.headers["X-Kong-Admin-Latency"])
end)
end)
describe("POST", function()
it_content_types("creates a route", function(content_type)
return function()
Expand Down Expand Up @@ -600,6 +616,23 @@ for _, strategy in helpers.each_strategy() do
end)

describe("/routes/{route}", function()
describe("OPTIONS", function()
it("returns allow and CORS headers with OPTIONS method", function()
local res = assert(client:send {
method = "OPTIONS",
path = "/routes/test"
})

local body = assert.res_status(204, res)
assert.equal("", body)
assert.equal("DELETE, GET, HEAD, OPTIONS, PATCH, PUT", res.headers["Allow"])
assert.equal("DELETE, GET, HEAD, OPTIONS, PATCH, PUT", res.headers["Access-Control-Allow-Methods"])
assert.equal("Content-Type", res.headers["Access-Control-Allow-Headers"])
assert.equal("*", res.headers["Access-Control-Allow-Origin"])
assert.not_nil(res.headers["X-Kong-Admin-Latency"])
end)
end)

describe("GET", function()
it("retrieves by id", function()
local route = bp.routes:insert({ paths = { "/my-route" } }, { nulls = true })
Expand Down

0 comments on commit f92d245

Please sign in to comment.