Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/plugin request termination #2051

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f3cd113
feat(schema): regex for numbers
pauldaustin Feb 4, 2017
fb608ba
feat(tools): HTTP Response 503 Service unavailable
pauldaustin Jan 26, 2017
ff4e2b8
Code style changes
pauldaustin Jan 26, 2017
97a24d5
Implemented tests and updated comments
pauldaustin Jan 27, 2017
330ab87
Revert "feat(schema): regex for numbers"
pauldaustin Feb 7, 2017
1dc3467
feat(plugin): request-termination
pauldaustin Feb 7, 2017
55d74be
feat(schema): regex for numbers
pauldaustin Feb 4, 2017
c2bfe7e
feat(tools): HTTP Response 503 Service unavailable
pauldaustin Jan 26, 2017
4d0802c
Code style changes
pauldaustin Jan 26, 2017
1ea22c5
Implemented tests and updated comments
pauldaustin Jan 27, 2017
6ee52b5
Revert "feat(schema): regex for numbers"
pauldaustin Feb 7, 2017
a641fd3
feat(plugin): request-termination
pauldaustin Feb 7, 2017
651a669
Merge branch 'feature/plugin-request-termination' of
pauldaustin Feb 7, 2017
8ada13f
Removed custom table definitions as the config is stored directly in
pauldaustin Feb 7, 2017
4b5f437
in-line default vakue for status_code
pauldaustin Feb 7, 2017
c2f9133
removed links to request-termination lua files that were no longer
pauldaustin Feb 14, 2017
cf8ad0b
Duplicate imports
pauldaustin Feb 14, 2017
43a9c0c
Merge remote-tracking branch 'kong/next' into feature/plugin-request-…
pauldaustin Feb 22, 2017
174e9ce
Changed the list of built in plugins to be one per line. This makes it
pauldaustin Mar 6, 2017
26476d7
Implemented changes as requested in pull request
pauldaustin Mar 6, 2017
69c59f0
Removed accidental commit of kong.conf
pauldaustin Mar 6, 2017
759fcd8
Renamed variable error to err
pauldaustin Mar 7, 2017
d918411
Merge remote-tracking branch 'upstream/next' into feature/plugin-requ…
pauldaustin Mar 12, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions kong-0.9.9-0.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -272,5 +272,15 @@ build = {
["kong.plugins.aws-lambda.handler"] = "kong/plugins/aws-lambda/handler.lua",
["kong.plugins.aws-lambda.schema"] = "kong/plugins/aws-lambda/schema.lua",
["kong.plugins.aws-lambda.v4"] = "kong/plugins/aws-lambda/v4.lua",

["kong.plugins.request-termination.migrations.cassandra"] = "kong/plugins/request-termination/migrations/cassandra.lua",
["kong.plugins.request-termination.daos"] = "kong/plugins/request-termination/daos.lua",
["kong.plugins.request-termination.handler"] = "kong/plugins/request-termination/handler.lua",
["kong.plugins.request-termination.schema"] = "kong/plugins/request-termination/schema.lua",

["kong.plugins.request-termination.migrations.cassandra"] = "kong/plugins/request-termination/migrations/cassandra.lua",
["kong.plugins.request-termination.daos"] = "kong/plugins/request-termination/daos.lua",
["kong.plugins.request-termination.handler"] = "kong/plugins/request-termination/handler.lua",
["kong.plugins.request-termination.schema"] = "kong/plugins/request-termination/schema.lua",
}
}
3 changes: 2 additions & 1 deletion kong/constants.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ local plugins = {
"galileo", "request-transformer", "response-transformer",
"request-size-limiting", "rate-limiting", "response-ratelimiting", "syslog",
"loggly", "datadog", "runscope", "ldap-auth", "statsd", "bot-detection",
"aws-lambda"
"aws-lambda",
"request-termination",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just combine those 2 lines

}

local plugin_map = {}
Expand Down
3 changes: 3 additions & 0 deletions kong/plugins/request-termination/daos.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
return {
tables = {"request_termination"}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this entire file is not needed, unless I'm missing something...

Kong will handle the plugin configuration including all fetching/storing of that data. The dao files (and also the migration files) are only required if the plugin has its own entities to store (for example the authentication plugins need to store credentials besides their own config)

so this can go, as well as the migrations files

42 changes: 42 additions & 0 deletions kong/plugins/request-termination/handler.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
local BasePlugin = require "kong.plugins.base_plugin"
local responses = require "kong.tools.responses"
local meta = require "kong.meta"

local server_header = meta._NAME.."/"..meta._VERSION

local RequestTerminationHandler = BasePlugin:extend()

RequestTerminationHandler.PRIORITY = 1

function RequestTerminationHandler:new()
RequestTerminationHandler.super.new(self, "request-termination")
end

function RequestTerminationHandler:access(conf)
RequestTerminationHandler.super.access(self)

local status_code = conf.status_code
local content_type = conf.content_type
local body = conf.body
local message = conf.message
if not status_code then
status_code = 503
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common idiom in Lua for these 'default' checks is to do

local status_code = conf.status_code or 503

if body then
ngx.status = status_code

if not content_type then
content_type = "application/json; charset=utf-8";
end
ngx.header["Content-Type"] = content_type
ngx.header["Server"] = server_header

ngx.say(body)

return ngx.exit(status_code)
else
return responses.send(status_code, message)
end
end

return RequestTerminationHandler
23 changes: 23 additions & 0 deletions kong/plugins/request-termination/migrations/cassandra.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
return {
{
name = "2017-01-28-000001_init_request_termination",
up = [[
CREATE TABLE IF NOT EXISTS request_terminations(
id uuid,
api_id uuid,
status_code smallint,
message text,
content_type text,
body text,
created_at timestamp,
PRIMARY KEY (id)
);

CREATE INDEX IF NOT EXISTS ON request_terminations(group);
CREATE INDEX IF NOT EXISTS request_terminations_api_id ON request_terminations(api_id);
]],
down = [[
DROP TABLE request_terminations;
]]
}
}
27 changes: 27 additions & 0 deletions kong/plugins/request-termination/migrations/postgres.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
return {
{
name = "2017-01-28-000001_init_request_termination",
up = [[
CREATE TABLE IF NOT EXISTS request_terminations(
id uuid,
api_id uuid REFERENCES apis (id) ON DELETE CASCADE,
status_code smallint,
message text,
content_type text,
body text,
created_at timestamp without time zone default (CURRENT_TIMESTAMP(0) at time zone 'utc'),
PRIMARY KEY (id)
);

DO $$
BEGIN
IF (SELECT to_regclass('request_terminations_api_id')) IS NULL THEN
CREATE INDEX request_terminations_api_id ON request_terminations(api_id);
END IF;
END$$;
]],
down = [[
DROP TABLE request_terminations;
]]
}
}
33 changes: 33 additions & 0 deletions kong/plugins/request-termination/schema.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
local Errors = require "kong.dao.errors"
local utils = require "kong.tools.utils"

return {
no_consumer = true,
fields = {
status_code = { type = "number" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add the default 503 here

message = { type = "string" },
content_type = { type = "string" },
body = { type = "string" },
},
self_check = function(schema, plugin_t, dao, is_updating)
local errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably fails on the linter as an unused local variable, can be tested using make lint


if plugin_t.status_code then
if plugin_t.status_code < 100 or plugin_t.status_code > 599 then
return false, "status_code must be between 100..599"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ought to be

return false, Errors.schema("status_code must be between 100..599")

same for the other errors below

end
end

if plugin_t.message then
if plugin_t.content_type or plugin_t.body then
return false, "message cannot be used with content_type or body"
end
else
if plugin_t.content_type and not plugin_t.body then
return false, "content_type requires a body"
end
end

return true
end
}
16 changes: 11 additions & 5 deletions kong/tools/responses.lua
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ local server_header = meta._NAME.."/"..meta._VERSION
-- @field HTTP_CONFLICT 409 Conflict
-- @field HTTP_UNSUPPORTED_MEDIA_TYPE 415 Unsupported Media Type
-- @field HTTP_INTERNAL_SERVER_ERROR Internal Server Error
-- @field HTTP_SERVICE_UNAVAILABLE 503 Service Unavailable
-- @usage return responses.send_HTTP_OK()
-- @usage return responses.HTTP_CREATED("Entity created")
-- @usage return responses.HTTP_INTERNAL_SERVER_ERROR()
Expand All @@ -55,7 +56,8 @@ local _M = {
HTTP_METHOD_NOT_ALLOWED = 405,
HTTP_CONFLICT = 409,
HTTP_UNSUPPORTED_MEDIA_TYPE = 415,
HTTP_INTERNAL_SERVER_ERROR = 500
HTTP_INTERNAL_SERVER_ERROR = 500,
HTTP_SERVICE_UNAVAILABLE = 503,
}
}

Expand All @@ -68,6 +70,7 @@ local _M = {
-- @field status_codes.HTTP_UNAUTHORIZED Default: Unauthorized
-- @field status_codes.HTTP_INTERNAL_SERVER_ERROR Always "Internal Server Error"
-- @field status_codes.HTTP_METHOD_NOT_ALLOWED Always "Method not allowed"
-- @field status_codes.HTTP_SERVICE_UNAVAILABLE Default: "Service unavailable"
local response_default_content = {
[_M.status_codes.HTTP_UNAUTHORIZED] = function(content)
return content or "Unauthorized"
Expand All @@ -83,20 +86,23 @@ local response_default_content = {
end,
[_M.status_codes.HTTP_METHOD_NOT_ALLOWED] = function(content)
return "Method not allowed"
end
end,
[_M.status_codes.HTTP_SERVICE_UNAVAILABLE] = function(content)
return content or "Service unavailable"
end,
}

-- Return a closure which will be usable to respond with a certain status code.
-- @local
-- @param[type=number] status_code The status for which to define a function
local function send_response(status_code)
-- Send a JSON response for the closure's status code with the given content.
-- If the content happens to be an error (>500), it will be logged by ngx.log as an ERR.
-- If the content happens to be an error (500), it will be logged by ngx.log as an ERR.
-- @see https://github.com/openresty/lua-nginx-module
-- @param content (Optional) The content to send as a response.
-- @return ngx.exit (Exit current context)
return function(content, headers)
if status_code >= _M.status_codes.HTTP_INTERNAL_SERVER_ERROR then
if status_code == _M.status_codes.HTTP_INTERNAL_SERVER_ERROR then
if content then
ngx.log(ngx.ERR, tostring(content))
end
Expand Down Expand Up @@ -141,7 +147,7 @@ local closure_cache = {}
--- Send a response with any status code or body,
-- Not all status codes are available as sugar methods, this function can be
-- used to send any response.
-- If the `status_code` parameter is in the 5xx range, it is expectde that the `content` parameter be the error encountered. It will be logged and the response body will be empty. The user will just receive a 500 status code.
-- If the `status_code` parameter is in the 5xx range, it is expected that the `content` parameter be the error encountered. For 500 errors it will be logged. The response body will be empty. The user will just receive a 500 status code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is not entirely clear imo. Maybe something like "Specifically for 500 errors: the message will be logged, but not passed in the response. The user will just get a 500 with standard message to prevent leaking sensitive information."

-- Will call `ngx.say` and `ngx.exit`, terminating the current context.
-- @see ngx.say
-- @see ngx.exit
Expand Down
19 changes: 18 additions & 1 deletion spec/01-unit/10-responses_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,12 @@ describe("Response helpers", function()
end)
end
end)
it("calls `ngx.log` if and only if a 500 status code range was given", function()
it("calls `ngx.log` if and only if a 500 status code was given", function()
responses.send_HTTP_BAD_REQUEST()
assert.stub(ngx.log).was_not_called()

responses.send_HTTP_BAD_REQUEST("error")
assert.stub(ngx.log).was_not_called()

responses.send_HTTP_INTERNAL_SERVER_ERROR()
assert.stub(ngx.log).was_not_called()
Expand All @@ -69,6 +72,14 @@ describe("Response helpers", function()
assert.stub(ngx.log).was_called()
end)

it("don't call `ngx.log` if a 503 status code was given", function()
responses.send_HTTP_SERVICE_UNAVAILABLE()
assert.stub(ngx.log).was_not_called()

responses.send_HTTP_SERVICE_UNAVAILABLE()
assert.stub(ngx.log).was_not_called("error")
end)

describe("default content rules for some status codes", function()
it("should apply default content rules for some status codes", function()
responses.send_HTTP_NOT_FOUND()
Expand All @@ -86,6 +97,12 @@ describe("Response helpers", function()
responses.send_HTTP_INTERNAL_SERVER_ERROR("override")
assert.stub(ngx.say).was.called_with("{\"message\":\"An unexpected error occurred\"}")
end)
it("should apply default content rules for some status codes", function()
responses.send_HTTP_SERVICE_UNAVAILABLE()
assert.stub(ngx.say).was.called_with("{\"message\":\"Service unavailable\"}")
responses.send_HTTP_SERVICE_UNAVAILABLE("override")
assert.stub(ngx.say).was.called_with("{\"message\":\"override\"}")
end)
end)

describe("send()", function()
Expand Down
57 changes: 57 additions & 0 deletions spec/03-plugins/18-request-termination/01-schema_spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
local schemas_validation = require "kong.dao.schemas_validation"
local schema = require "kong.plugins.request-termination.schema"

local v = schemas_validation.validate_entity

describe("Plugin: request-termination (schema)", function()
it("should accept a valid status_code", function()
assert(v({status_code = 404}, schema))
end)
it("should accept a valid message", function()
assert(v({message = "Not found"}, schema))
end)
it("should accept a valid content_type", function()
assert(v({content_type = "text/html",body = "<body><h1>Not found</h1>"}, schema))
end)
it("should accept a valid body", function()
assert(v({body = "<body><h1>Not found</h1>"}, schema))
end)

describe("errors", function()
it("status_code should only accept numbers", function()
local ok, err = v({status_code = "abcd"}, schema)
assert.same({status_code = "status_code is not a number"}, err)
assert.False(ok)
end)
it("status_code < 100", function()
local ok, err, message = v({status_code = "99"}, schema)
assert.False(ok)
assert.same("status_code must be between 100..599", message)
end)
it("status_code > 599", function()
local ok, err, message = v({status_code = "600"}, schema)
assert.False(ok)
assert.same("status_code must be between 100..599", message)
end)
it("message with body", function()
local ok, err, message = v({message = "error", body = "test"}, schema)
assert.False(ok)
assert.same("message cannot be used with content_type or body", message)
end)
it("message with body and content_type", function()
local ok, err, message = v({message = "error", content_type="text/html", body = "test"}, schema)
assert.False(ok)
assert.same("message cannot be used with content_type or body", message)
end)
it("message with content_type", function()
local ok, err, message = v({message = "error", content_type="text/html"}, schema)
assert.False(ok)
assert.same("message cannot be used with content_type or body", message)
end)
it("content_type without body", function()
local ok, err, message = v({content_type="text/html"}, schema)
assert.False(ok)
assert.same("content_type requires a body", message)
end)
end)
end)
Loading