Feature/plugin request termination #2051

Closed
wants to merge 23 commits into
from

Conversation

Projects
None yet
4 participants
@pauldaustin

Summary

Added a request termination plugin as described in issue #1279.

Full changelog

  • Implemented request-termination-plugin
  • Added 03-plugins/18-request-termination tests

pauldaustin added some commits Feb 4, 2017

feat(schema): regex for numbers
Modified the schema validation to allow regex on numbers. For example
\\d{3} to only accept 3 digits in the number.
feat(tools): HTTP Response 503 Service unavailable
Add HTTP response for 503 Service unavailable. This will be used for a
new api-unavailable plugin that will send a 503 error. See #1279.
Code style changes
Added , to end of list
Changed to only log 500 errors
feat(plugin): request-termination
A new plug-in that allows a request to be terminated and a specified
HTTP status code and body returned.

This is useful to temporarily return a status page for a service. For
example if the service is unavailable due to scheduled maintenance.
feat(tools): HTTP Response 503 Service unavailable
Add HTTP response for 503 Service unavailable. This will be used for a
new api-unavailable plugin that will send a 503 error. See #1279.
Code style changes
Added , to end of list
Changed to only log 500 errors
feat(schema): regex for numbers
Modified the schema validation to allow regex on numbers. For example
\\d{3} to only accept 3 digits in the number.
feat(plugin): request-termination
A new plug-in that allows a request to be terminated and a specified
HTTP status code and body returned.

This is useful to temporarily return a status page for a service. For
example if the service is unavailable due to scheduled maintenance.
Merge branch 'feature/plugin-request-termination' of
git@github.com:pauldaustin/kong.git into
feature/plugin-request-termination

Conflicts:
	kong-0.9.8-0.rockspec
@Tieske

Thx for the PR, looks like you put some serious effort in it!

Few comments on a first glance, will have to look more in depth later.

+ local message = conf.message
+ if not status_code then
+ status_code = 503
+ end

This comment has been minimized.

@Tieske

Tieske Feb 7, 2017

Member

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

local status_code = conf.status_code or 503
@Tieske

Tieske Feb 7, 2017

Member

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

local status_code = conf.status_code or 503
@@ -0,0 +1,3 @@
+return {
+ tables = {"request_termination"}
+}

This comment has been minimized.

@Tieske

Tieske Feb 7, 2017

Member

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

@Tieske

Tieske Feb 7, 2017

Member

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

@pauldaustin

This comment has been minimized.

Show comment
Hide comment
@pauldaustin

pauldaustin Feb 7, 2017

Thanks for the comments, let me know if there is anything else.

Thanks for the comments, let me know if there is anything else.

@Tieske

Can you rework the comments?

overall looks complete and well tested! 👍

kong/constants.lua
@@ -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",

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

just combine those 2 lines

@Tieske

Tieske Mar 3, 2017

Member

just combine those 2 lines

+function RequestTerminationHandler:access(conf)
+ RequestTerminationHandler.super.access(self)
+
+ local status_code = conf.status_code or 503

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

this 503 act as a default, hence it would be better placed in the schema.lua file

@Tieske

Tieske Mar 3, 2017

Member

this 503 act as a default, hence it would be better placed in the schema.lua file

+return {
+ no_consumer = true,
+ fields = {
+ status_code = { type = "number" },

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

you can add the default 503 here

@Tieske

Tieske Mar 3, 2017

Member

you can add the default 503 here

+ body = { type = "string" },
+ },
+ self_check = function(schema, plugin_t, dao, is_updating)
+ local errors

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

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

@Tieske

Tieske Mar 3, 2017

Member

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"

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

this ought to be

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

same for the other errors below

@Tieske

Tieske Mar 3, 2017

Member

this ought to be

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

same for the other errors below

kong/tools/responses.lua
@@ -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.

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

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."

@Tieske

Tieske Mar 3, 2017

Member

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."

+ api_id = api6.id,
+ config = {
+ status_code=503,
+ body='{"code": 1, "message": "Service unavailable}'

This comment has been minimized.

@Tieske

Tieske Mar 3, 2017

Member

there is a missing quote at the end. Test passes, but is this intentional? if so a small comment as to why would make sense.

@Tieske

Tieske Mar 3, 2017

Member

there is a missing quote at the end. Test passes, but is this intentional? if so a small comment as to why would make sense.

@pauldaustin

This comment has been minimized.

Show comment
Hide comment
@pauldaustin

pauldaustin Mar 3, 2017

@Tieske

This comment has been minimized.

Show comment
Hide comment
@Tieske

Tieske Mar 6, 2017

Member

@pauldaustin

For the constants.lua local_plugins. Would it not be better to have one per line with a trailing , . Then it would be easier to merge new changes?

yes please! 😄

Member

Tieske commented Mar 6, 2017

@pauldaustin

For the constants.lua local_plugins. Would it not be better to have one per line with a trailing , . Then it would be easier to merge new changes?

yes please! 😄

- local ok, err = v({status_code = "abcd"}, schema)
- assert.same({status_code = "status_code is not a number"}, err)
+ local ok, error = v({status_code = "abcd"}, schema)
+ assert.same({status_code = "status_code is not a number"}, error)

This comment has been minimized.

@Tieske

Tieske Mar 7, 2017

Member

please do not use error as a variable name. The common name for errors is err because error is a built in Lua function (which will be shadowed by the local declaration as applied above)

@Tieske

Tieske Mar 7, 2017

Member

please do not use error as a variable name. The common name for errors is err because error is a built in Lua function (which will be shadowed by the local declaration as applied above)

@pauldaustin

This comment has been minimized.

Show comment
Hide comment
@pauldaustin

pauldaustin Mar 11, 2017

I noticed that 0.10 has been released.

Let me know if I need to merge from next or master into this branch to make it ready to merge again

I noticed that 0.10 has been released.

Let me know if I need to merge from next or master into this branch to make it ready to merge again

@Tieske

This comment has been minimized.

Show comment
Hide comment
@Tieske

Tieske Mar 12, 2017

Member

rebase on next please

Member

Tieske commented Mar 12, 2017

rebase on next please

@pauldaustin

This comment has been minimized.

Show comment
Hide comment
@pauldaustin

pauldaustin Mar 16, 2017

Let me know if there is anything else required

Let me know if there is anything else required

@coopr

This comment has been minimized.

Show comment
Hide comment
@coopr

coopr Mar 23, 2017

Member

Thanks @pauldaustin please stay tuned...

Member

coopr commented Mar 23, 2017

Thanks @pauldaustin please stay tuned...

@Tieske

This comment has been minimized.

Show comment
Hide comment
@Tieske

Tieske Mar 24, 2017

Member

if anything, its gonna require docs on the getkong.org repo

Member

Tieske commented Mar 24, 2017

if anything, its gonna require docs on the getkong.org repo

@Tieske Tieske modified the milestones: 0.10.1, 0.10.2 Mar 27, 2017

Tieske added a commit that referenced this pull request Apr 4, 2017

feat(plugin) request termination (#2328, #2051)
* feat(plugin): request-termination

A new plug-in that allows a request to be terminated and a specified
HTTP status code and body returned.
This is useful to temporarily return a status page for a service. For
example if the service is unavailable due to scheduled maintenance.
@Tieske

This comment has been minimized.

Show comment
Hide comment
@Tieske

Tieske Apr 4, 2017

Member

merged through #2328 closing this.

@pauldaustin thank you!

Member

Tieske commented Apr 4, 2017

merged through #2328 closing this.

@pauldaustin thank you!

@Tieske Tieske closed this Apr 4, 2017

@pauldaustin pauldaustin deleted the pauldaustin:feature/plugin-request-termination branch May 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment