Feature/plugin request termination #2051

pauldaustin · 2017-02-07T16:55:18Z

Summary

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

Full changelog

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

Modified the schema validation to allow regex on numbers. For example \\d{3} to only accept 3 digits in the number.

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.

Added , to end of list Changed to only log 500 errors

This reverts commit f3cd113.

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.

Modified the schema validation to allow regex on numbers. For example \\d{3} to only accept 3 digits in the number.

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.

Added , to end of list Changed to only log 500 errors

This reverts commit f3cd113.

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.

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.

Tieske · 2017-02-07T19:48:26Z

kong/plugins/request-termination/handler.lua

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


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

local status_code = conf.status_code or 503

Tieske · 2017-02-07T19:51:46Z

kong/plugins/request-termination/daos.lua

@@ -0,0 +1,3 @@
+return {
+  tables = {"request_termination"}
+}


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

kong's plugins table

pauldaustin · 2017-02-07T20:01:05Z

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

required

…termination

Tieske

Can you rework the comments?

overall looks complete and well tested! 👍

Tieske · 2017-03-03T08:10:11Z

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


just combine those 2 lines

Tieske · 2017-03-03T08:10:46Z

kong/plugins/request-termination/handler.lua

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


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

Tieske · 2017-03-03T08:14:01Z

kong/plugins/request-termination/schema.lua

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


you can add the default 503 here

Tieske · 2017-03-03T08:15:30Z

kong/plugins/request-termination/schema.lua

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


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

Tieske · 2017-03-03T08:21:55Z

kong/plugins/request-termination/schema.lua

+
+    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 ought to be

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

same for the other errors below

Tieske · 2017-03-03T08:32:34Z

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.


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 · 2017-03-03T09:19:06Z

spec/03-plugins/18-request-termination/02-access_spec.lua

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


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 · 2017-03-03T15:29:58Z

I'll make the changes Monday 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?

Tieske · 2017-03-06T12:21:33Z

@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! 😄

easier to merge changes

Tieske · 2017-03-07T09:19:54Z

spec/03-plugins/18-request-termination/01-schema_spec.lua

-      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)


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 · 2017-03-11T01:58:46Z

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 · 2017-03-12T21:09:41Z

rebase on next please

…est-termination

pauldaustin · 2017-03-16T17:46:19Z

Let me know if there is anything else required

coopr · 2017-03-23T19:09:23Z

Thanks @pauldaustin please stay tuned...

Tieske · 2017-03-24T17:17:18Z

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

* 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 · 2017-04-04T08:33:40Z

merged through #2328 closing this.

@pauldaustin thank you!

pauldaustin added 13 commits February 3, 2017 19:27

feat(schema): regex for numbers

f3cd113

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

fb608ba

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

ff4e2b8

Added , to end of list Changed to only log 500 errors

Implemented tests and updated comments

97a24d5

Revert "feat(schema): regex for numbers"

330ab87

This reverts commit f3cd113.

feat(plugin): request-termination

1dc3467

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(schema): regex for numbers

55d74be

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

c2bfe7e

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

4d0802c

Added , to end of list Changed to only log 500 errors

Implemented tests and updated comments

1ea22c5

Revert "feat(schema): regex for numbers"

6ee52b5

This reverts commit f3cd113.

feat(plugin): request-termination

a641fd3

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

651a669

git@github.com:pauldaustin/kong.git into feature/plugin-request-termination Conflicts: kong-0.9.8-0.rockspec

Tieske requested changes Feb 7, 2017

View reviewed changes

pauldaustin added 2 commits February 7, 2017 11:58

Removed custom table definitions as the config is stored directly in

8ada13f

kong's plugins table

in-line default vakue for status_code

4b5f437

This was referenced Feb 13, 2017

Response Override plugin #669

Closed

Path/Operation restriction plugin #1234

Closed

pauldaustin added 3 commits February 13, 2017 19:53

removed links to request-termination lua files that were no longer

c2f9133

required

Duplicate imports

cf8ad0b

Merge remote-tracking branch 'kong/next' into feature/plugin-request-…

43a9c0c

…termination

thibaultcha added the pr/status/needs review label Feb 28, 2017

Tieske mentioned this pull request Mar 2, 2017

Allow multiple authentication methods per API and/or Consumer #590

Closed

Tieske requested changes Mar 3, 2017

View reviewed changes

pauldaustin added 3 commits March 6, 2017 10:06

Changed the list of built in plugins to be one per line. This makes it

174e9ce

easier to merge changes

Implemented changes as requested in pull request

26476d7

Removed accidental commit of kong.conf

69c59f0

Tieske reviewed Mar 7, 2017

View reviewed changes

Renamed variable error to err

759fcd8

Merge remote-tracking branch 'upstream/next' into feature/plugin-requ…

d918411

…est-termination

Tieske added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/needs review labels Mar 16, 2017

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

Tieske mentioned this pull request Apr 4, 2017

Feature/plugin request termination #2328

Merged

Tieske closed this Apr 4, 2017

thibaultcha mentioned this pull request Apr 14, 2017

suggestion:add a enable property to api and consumers #2392

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/plugin request termination #2051

Feature/plugin request termination #2051

pauldaustin commented Feb 7, 2017

Tieske left a comment

Tieske Feb 7, 2017

Tieske Feb 7, 2017

pauldaustin commented Feb 7, 2017

Tieske left a comment

Tieske Mar 3, 2017

Tieske Mar 3, 2017

Tieske Mar 3, 2017

Tieske Mar 3, 2017

Tieske Mar 3, 2017

Tieske Mar 3, 2017

Tieske Mar 3, 2017

pauldaustin commented Mar 3, 2017 via email

Tieske commented Mar 6, 2017

Tieske Mar 7, 2017

pauldaustin commented Mar 11, 2017

Tieske commented Mar 12, 2017

pauldaustin commented Mar 16, 2017

coopr commented Mar 23, 2017

Tieske commented Mar 24, 2017

Tieske commented Apr 4, 2017

Feature/plugin request termination #2051

Feature/plugin request termination #2051

Conversation

pauldaustin commented Feb 7, 2017

Summary

Full changelog

Tieske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldaustin commented Feb 7, 2017

Tieske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldaustin commented Mar 3, 2017 via email

Tieske commented Mar 6, 2017

Choose a reason for hiding this comment

pauldaustin commented Mar 11, 2017

Tieske commented Mar 12, 2017

pauldaustin commented Mar 16, 2017

coopr commented Mar 23, 2017

Tieske commented Mar 24, 2017

Tieske commented Apr 4, 2017