From fc399234969f29c074baeefb4c872bb36a364547 Mon Sep 17 00:00:00 2001 From: samugi Date: Fri, 24 Feb 2023 11:24:36 +0100 Subject: [PATCH 1/3] feat(proxy): error outputs ensure error messages are returned via kong.response.error when the proxy request is interrupted so that the output formats are compliant with the value of the Accept header --- kong/init.lua | 8 ++++---- kong/runloop/handler.lua | 14 ++++++-------- spec/01-unit/16-runloop_handler_spec.lua | 2 +- .../33-serverless-functions/02-access_spec.lua | 3 ++- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/kong/init.lua b/kong/init.lua index 4ae555bba5f4..63ad10da5fdb 100644 --- a/kong/init.lua +++ b/kong/init.lua @@ -433,9 +433,9 @@ local function flush_delayed_response(ctx) return -- avoid tail call end - kong.response.exit(ctx.delayed_response.status_code, - ctx.delayed_response.content, - ctx.delayed_response.headers) + local dr = ctx.delayed_response + local message = dr.content and dr.content.message or dr.content + kong.response.error(dr.status_code, message, dr.headers) end @@ -1005,7 +1005,7 @@ function Kong.access() ctx.buffered_proxying = nil - return kong.response.exit(503, { message = "no Service found with those values"}) + return kong.response.error(503, "no Service found with those values") end runloop.access.after(ctx) diff --git a/kong/runloop/handler.lua b/kong/runloop/handler.lua index d2a927351c98..e5ce9271741f 100644 --- a/kong/runloop/handler.lua +++ b/kong/runloop/handler.lua @@ -1079,8 +1079,7 @@ return { after = function(ctx) local ok, err, errcode = balancer_execute(ctx) if not ok then - local body = utils.get_default_exit_body(errcode, err) - return kong.response.exit(errcode, body) + return kong.response.error(errcode, err) end end }, @@ -1118,7 +1117,7 @@ return { span:finish() end - return kong.response.exit(404, { message = "no Route matched with those values" }) + return kong.response.error(404, "no Route matched with those values") end -- ends tracing span @@ -1185,7 +1184,7 @@ return { local redirect_status_code = route.https_redirect_status_code or 426 if redirect_status_code == 426 then - return kong.response.exit(426, { message = "Please use HTTPS protocol" }, { + return kong.response.error(426, "Please use HTTPS protocol", { ["Connection"] = "Upgrade", ["Upgrade"] = "TLS/1.2, HTTP/1.1", }) @@ -1209,7 +1208,7 @@ return { if content_type and sub(content_type, 1, #"application/grpc") == "application/grpc" then if protocol_version ~= 2 then -- mismatch: non-http/2 request matched grpc route - return kong.response.exit(426, { message = "Please use HTTP2 protocol" }, { + return kong.response.error(426, "Please use HTTP2 protocol", { ["connection"] = "Upgrade", ["upgrade"] = "HTTP/2", }) @@ -1217,7 +1216,7 @@ return { else -- mismatch: non-grpc request matched grpc route - return kong.response.exit(415, { message = "Non-gRPC request matched gRPC route" }) + return kong.response.error(415, "Non-gRPC request matched gRPC route") end if not protocols.grpc and forwarded_proto ~= "https" then @@ -1339,8 +1338,7 @@ return { local ok, err, errcode = balancer_execute(ctx) if not ok then - local body = utils.get_default_exit_body(errcode, err) - return kong.response.exit(errcode, body) + return kong.response.error(errcode, err) end local ok, err = balancer.set_host_header(balancer_data, upstream_scheme, upstream_host) diff --git a/spec/01-unit/16-runloop_handler_spec.lua b/spec/01-unit/16-runloop_handler_spec.lua index 3c3bce1fe08d..2ea742491776 100644 --- a/spec/01-unit/16-runloop_handler_spec.lua +++ b/spec/01-unit/16-runloop_handler_spec.lua @@ -29,7 +29,7 @@ local function setup_it_block() warn = function() end, }, response = { - exit = function() end, + error = function() end, }, worker_events = { register = function() end, diff --git a/spec/03-plugins/33-serverless-functions/02-access_spec.lua b/spec/03-plugins/33-serverless-functions/02-access_spec.lua index 1dac0e055b37..63e19504ee8b 100644 --- a/spec/03-plugins/33-serverless-functions/02-access_spec.lua +++ b/spec/03-plugins/33-serverless-functions/02-access_spec.lua @@ -292,7 +292,8 @@ for _, plugin_name in ipairs({ "pre-function", "post-function" }) do } }) local body = assert.res_status(500, res) - assert.same('{"message":"An unexpected error occurred"}', body) + local json = cjson.decode(body) + assert.same({ message = "An unexpected error occurred" }, json) end) end) From 438a310e9fcc7e7b88f74d110c2e0332e87a033b Mon Sep 17 00:00:00 2001 From: samugi Date: Fri, 24 Feb 2023 15:15:09 +0100 Subject: [PATCH 2/3] tests(proxy): collect plugin errors --- .../29-collect-plugin-errors_spec.lua | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 spec/02-integration/05-proxy/29-collect-plugin-errors_spec.lua diff --git a/spec/02-integration/05-proxy/29-collect-plugin-errors_spec.lua b/spec/02-integration/05-proxy/29-collect-plugin-errors_spec.lua new file mode 100644 index 000000000000..52319d418231 --- /dev/null +++ b/spec/02-integration/05-proxy/29-collect-plugin-errors_spec.lua @@ -0,0 +1,68 @@ +local helpers = require "spec.helpers" +local cjson = require "cjson" + +for _, strategy in helpers.each_strategy() do + describe("Collect plugin errors [#" .. strategy .. "]", function() + local client + + lazy_setup(function() + local bp = helpers.get_db_utils(strategy, { + "routes", + "services", + "plugins", + },{ + "logger" + }) + + local service = assert(bp.services:insert { + url = helpers.mock_upstream_url + }) + + local route = assert(bp.routes:insert { + service = service, + hosts = { "error.test" } + }) + + assert(bp.plugins:insert { + name = "error-generator", + route = { id = route.id }, + config = { + access = true, + }, + }) + assert(bp.plugins:insert { + name = "logger", + route = { id = route.id }, + }) + + assert(helpers.start_kong({ + database = strategy, + plugins = "bundled, error-generator, logger", + nginx_conf = "spec/fixtures/custom_nginx.template", + })) + client = helpers.proxy_client() + end) + + lazy_teardown(function() + if client then + client:close() + end + helpers.stop_kong() + end) + + it("delays the error response", function() + local res = assert(client:get("/get", { + headers = { + Host = "error.test", + } + })) + local body = assert.res_status(500, res) + local json = cjson.decode(body) + assert.same({ message = "An unexpected error occurred" }, json) + -- the other plugin's phases were executed: + assert.logfile().has.line("header_filter phase", true) + assert.logfile().has.line("body_filter phase", true) + assert.logfile().has.line("log phase", true) + end) + end) +end From 1fac9786e52ef44ab45cf1500a774176ef85bbcf Mon Sep 17 00:00:00 2001 From: samugi Date: Fri, 24 Feb 2023 15:27:58 +0100 Subject: [PATCH 3/3] chore(*): changelog update --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae2e1113e63b..1fc1d1c0c2e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,12 @@ ## Unreleased +### Additions + +#### Core + +- Make runloop and init error response content types compliant with Accept header value + [#10366](https://github.com/Kong/kong/pull/10366) ### Dependencies - Bumped lua-resty-session from 4.0.2 to 4.0.3