Skip to content

Commit

Permalink
fix(bk-error-wrapper): response from plugin been wrapped (#82)
Browse files Browse the repository at this point in the history
  • Loading branch information
@wklken
wklken authored Oct 12, 2024
1 parent 3f20147 commit 86428cc
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 39 deletions.
33 changes: 28 additions & 5 deletions src/apisix/plugins/bk-error-wrapper.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,14 @@ end
--- - 502 + failed to connect to upstream: the upstream port is not listened
--- - 504 + failed to connect to upstream: the upstream port is listened, but the handshake timeout
--- usually network problem
--- - 502 + cannot read header from upstream: upstream prematurely closed connection while reading
--- response header from upstream
--- - 504 + cannot read header from upstream: upstream timed out (110: Connection timed out)
--- while reading response header from upstream
---@param ctx apisix.Context
---@return string,string|nil @the specific phase and error message
local function _get_upstream_error_msg(ctx)
-- FIXME: add ngx.status as parameter, return specific error message in comment above
if not ctx.var.upstream_connect_time then -- 握手失败
return proxy_phases.CONNECTING, "failed to connect to upstream"
-- note: 此处删掉对$upstream_bytes_sent判断的原因是
Expand Down Expand Up @@ -109,13 +114,30 @@ function _M.header_filter(conf, ctx) -- luacheck: no unused

ctx.var.proxy_error = "1"
end
-- if not ctx.var.upstream_status, means from apisix internal or the plugin return

-- 2024-10-10 封装导致非蓝鲸插件例如 fault-injection 返回非 200 时 response body 被吞掉
-- 注释掉之后
-- 1. 插件返回的非 200 不会存在 ctx.var.bk_apigw_error
-- 2. apisix 和 openresty 默认错误不会被封装, 将返回原始 body

-- apisix or openresty default error
if not ctx.var.bk_apigw_error and ngx.status >= ngx.HTTP_BAD_REQUEST then
-- wrap and generate a bk_apigw_error
local error = errorx.new_default_error_with_status(ngx.status)
-- after set this, the body_filter will be called
ctx.var.bk_apigw_error = error
-- or it's other plugin return non-200 status
-- if not ctx.var.bk_apigw_error and ngx.status >= ngx.HTTP_BAD_REQUEST then
-- -- wrap and generate a bk_apigw_error
-- local error = errorx.new_default_error_with_status(ngx.status)
-- -- after set this, the body_filter will be called
-- ctx.var.bk_apigw_error = error
-- end

-- upstream status 报错封装成网关的报错
if upstream_error_msg then
if not ctx.var.bk_apigw_error and ngx.status >= ngx.HTTP_BAD_REQUEST then
-- wrap and generate a bk_apigw_error
local error = errorx.new_default_error_with_status(ngx.status)
-- after set this, the body_filter will be called
ctx.var.bk_apigw_error = error
end
end

local apigw_error = ctx.var.bk_apigw_error
Expand Down Expand Up @@ -146,6 +168,7 @@ local function extract_error_info_from_body(body)
if pl_types.is_empty(body) then
return nil
end
-- FIXME: should use the raw body as error message?

-- openresty default error message for non-200 status, for example
-- <html>\r\n<head><title>404 Not Found<\/title>...
Expand Down
64 changes: 30 additions & 34 deletions src/apisix/tests/test-bk-error-wrapper.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe(
)

context(
"apisix plugins error", function()
"apisix plugins error, will use raw status and response body", function()

before_each(
function()
Expand All @@ -110,48 +110,30 @@ describe(
)

it(
"apisix plugins error", function()
"apisix plugins return 404", function()
ngx.status = 404
plugin.header_filter(nil, ctx)
assert.is_nil(ctx.var.bk_apigw_error)

local bk_apigw_error = errorx.new_api_not_found()

assert.is_equal(bk_apigw_error.status, ctx.var.bk_apigw_error.status)
assert.is_equal(bk_apigw_error.error.code, ctx.var.bk_apigw_error.error.code)
-- local bk_apigw_error = errorx.new_api_not_found()

assert.stub(response.clear_header_as_body_modified).was.called()
assert.stub(response.set_header)
.was_called_with("Content-Type", "application/json; charset=utf-8")
assert.stub(response.set_header).was_called_with(
"X-Bkapi-Error-Code", tostring(bk_apigw_error.error.code)
)
assert.stub(response.set_header).was_called_with(
"X-Bkapi-Error-Message", bk_apigw_error.error.message
)
assert.is_nil(ctx.var.proxy_phase)
end
)

it(
"apisix plugins error but do'not need dealing with", function()
ngx.status = 409
plugin.header_filter(nil, ctx)
assert.is_not_nil(ctx.var.bk_apigw_error)
-- assert.is_equal(bk_apigw_error.status, ctx.var.bk_apigw_error.status)
-- assert.is_equal(bk_apigw_error.error.code, ctx.var.bk_apigw_error.error.code)

local bk_apigw_error = errorx.new_unkonwon_error(409)
assert.stub(response.clear_header_as_body_modified).was_not_called()
-- assert.stub(response.set_header)
-- .was_called_with("Content-Type", "application/json; charset=utf-8")
-- assert.stub(response.set_header).was_called_with(
-- "X-Bkapi-Error-Code", tostring(bk_apigw_error.error.code)
-- )
-- assert.stub(response.set_header).was_called_with(
-- "X-Bkapi-Error-Message", bk_apigw_error.error.message
-- )

assert.stub(response.clear_header_as_body_modified).was_called()
assert.stub(response.set_header)
.was_called_with("Content-Type", "application/json; charset=utf-8")
assert.stub(response.set_header).was_called_with(
"X-Bkapi-Error-Code", tostring(bk_apigw_error.error.code)
)
assert.stub(response.set_header).was_called_with(
"X-Bkapi-Error-Message", bk_apigw_error.error.message
)
assert.is_nil(ctx.var.proxy_phase)
end
)

end
)

Expand Down Expand Up @@ -509,6 +491,20 @@ describe(
assert.stub(core.json.encode).called(0)
end
)

it(
"apisix plugin return status not 200", function()
ngx.status = 502
ngx.arg[1] = "it's 502"
ngx.arg[2] = true
plugin.header_filter(nil, ctx)
plugin.body_filter(nil, ctx)

assert.stub(core.json.encode).called(0)
assert.equal("it's 502", ngx.arg[1])
assert.equal(true, ngx.arg[2])
end
)
end
)
end
Expand Down

0 comments on commit 86428cc

Please sign in to comment.