Re: [PR] feat: add response source differentiation for gateway vs upstream errors [apisix]
nic-6443 commented on code in PR #13224:
URL: https://github.com/apache/apisix/pull/13224#discussion_r3077901286
##
apisix/core/response.lua:
##
@@ -159,6 +164,41 @@ function _M.get_upstream_status(ctx)
end
+--- Get the source of the current response.
+--
+-- @function core.response.get_response_source
+-- @tparam table ctx The APISIX request context (api_ctx).
+-- @treturn string One of:
+-- "apisix" — response generated by APISIX Lua code (e.g. route not found,
plugin rejection)
+-- "nginx"— error generated by NGINX proxy module (e.g. connection
refused, timeout)
+-- "upstream" — real HTTP response returned by the upstream service
+function _M.get_response_source(ctx)
+if not ctx then
+return "apisix"
+end
+
+-- Priority 1: explicitly marked by core.response.exit()
+if ctx._resp_source then
+return ctx._resp_source
+end
+
+-- Priority 2: request was proxied — inspect the last
$upstream_header_time token
+if ctx._apisix_proxied then
+local header_time = ctx.var.upstream_header_time
+-- With retries, $upstream_header_time is comma-separated (e.g. "-,
0.002").
+-- The last token corresponds to the final attempt that produced the
response.
+local last = header_time and str_match(header_time, "([^,]+)%s*$")
+if last and last ~= "-" then
+return "upstream" -- received response headers from upstream
+end
+return "nginx" -- never received response headers (connection
error, timeout, etc.)
Review Comment:
Fixed — switched from `str_match` pattern to a `gmatch("%S+")` loop that
skips commas, so leading/trailing spaces are handled correctly. Also added
tests for spaced separators like `"- , -"` and `"- , 0.002"`.
##
t/core/response-source.t:
##
@@ -0,0 +1,273 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+log_level("info");
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: get_response_source returns "apisix" when ctx is nil
+--- config
+location = /t {
+content_by_lua_block {
+local core = require("apisix.core")
+local source = core.response.get_response_source(nil)
+ngx.say(source)
+}
+}
+--- response_body
+apisix
+
+
+
+=== TEST 2: get_response_source returns "apisix" when no flags set
+--- config
+location = /t {
+content_by_lua_block {
+local core = require("apisix.core")
+local ctx = {}
+local source = core.response.get_response_source(ctx)
+ngx.say(source)
+}
+}
+--- response_body
+apisix
+
+
+
+=== TEST 3: get_response_source returns "apisix" when _resp_source is set
+--- config
+location = /t {
+content_by_lua_block {
+local core = require("apisix.core")
+local ctx = {_resp_source = "apisix"}
+local source = core.response.get_response_source(ctx)
+ngx.say(source)
+}
+}
+--- response_body
+apisix
+
+
+
+=== TEST 4: get_response_source returns "nginx" when proxied but no header time
+--- config
+location = /t {
+content_by_lua_block {
+local core = require("apisix.core")
+local ctx = {_apisix_proxied = true, var = {upstream_header_time =
"-"}}
+local source = core.response.get_response_source(ctx)
+ngx.say(source)
+}
+}
+--- response_body
+nginx
+
+
+
+=== TEST 5: get_response_source returns "nginx" when proxied and header time
nil
+--- config
+location = /t {
+content_by_lua_block {
+local core = require("apisix.core")
+local ctx = {_apisix_proxied = true, var = {}}
+local source = core.response.get_response_source(ctx)
+ngx.say(source)
+}
+}
+--- response_body
+nginx
+
+
+
+=== TEST 6: get_response_source returns "upstream" when proxied with numeric
header time
+--- config
+location = /t {
+conten
Re: [PR] feat: add response source differentiation for gateway vs upstream errors [apisix]
Copilot commented on code in PR #13224:
URL: https://github.com/apache/apisix/pull/13224#discussion_r3077885226
##
apisix/core/response.lua:
##
@@ -159,6 +164,41 @@ function _M.get_upstream_status(ctx)
end
+--- Get the source of the current response.
+--
+-- @function core.response.get_response_source
+-- @tparam table ctx The APISIX request context (api_ctx).
+-- @treturn string One of:
+-- "apisix" — response generated by APISIX Lua code (e.g. route not found,
plugin rejection)
+-- "nginx"— error generated by NGINX proxy module (e.g. connection
refused, timeout)
+-- "upstream" — real HTTP response returned by the upstream service
+function _M.get_response_source(ctx)
+if not ctx then
+return "apisix"
+end
+
+-- Priority 1: explicitly marked by core.response.exit()
+if ctx._resp_source then
+return ctx._resp_source
+end
+
+-- Priority 2: request was proxied — inspect the last
$upstream_header_time token
+if ctx._apisix_proxied then
+local header_time = ctx.var.upstream_header_time
+-- With retries, $upstream_header_time is comma-separated (e.g. "-,
0.002").
+-- The last token corresponds to the final attempt that produced the
response.
+local last = header_time and str_match(header_time, "([^,]+)%s*$")
+if last and last ~= "-" then
+return "upstream" -- received response headers from upstream
+end
+return "nginx" -- never received response headers (connection
error, timeout, etc.)
Review Comment:
`get_response_source()` misclassifies retry failure cases when
`$upstream_header_time` contains spaces after commas (e.g. "-, -"). The pattern
`([^,]+)%s*$` returns the last token with a leading space (" -"), so `last ~=
"-"` becomes true and the function incorrectly returns "upstream" (and will
fail TEST 8/9). Trim whitespace from `last` or change the pattern to capture
the last non-space token before comparing to "-".
##
apisix/core/response.lua:
##
@@ -159,6 +164,41 @@ function _M.get_upstream_status(ctx)
end
+--- Get the source of the current response.
+--
+-- @function core.response.get_response_source
+-- @tparam table ctx The APISIX request context (api_ctx).
+-- @treturn string One of:
+-- "apisix" — response generated by APISIX Lua code (e.g. route not found,
plugin rejection)
+-- "nginx"— error generated by NGINX proxy module (e.g. connection
refused, timeout)
+-- "upstream" — real HTTP response returned by the upstream service
+function _M.get_response_source(ctx)
+if not ctx then
+return "apisix"
+end
+
+-- Priority 1: explicitly marked by core.response.exit()
+if ctx._resp_source then
+return ctx._resp_source
+end
+
+-- Priority 2: request was proxied — inspect the last
$upstream_header_time token
+if ctx._apisix_proxied then
+local header_time = ctx.var.upstream_header_time
Review Comment:
`get_response_source()` assumes `ctx.var` exists when `_apisix_proxied` is
true (`ctx.var.upstream_header_time`). If a caller sets `_apisix_proxied` on a
synthetic/partial ctx, this will throw. Consider guarding with `ctx.var and
ctx.var.upstream_header_time` (or returning "nginx" when `ctx.var` is missing)
to keep the API safe to call.
```suggestion
local header_time = ctx.var and ctx.var.upstream_header_time
```
##
t/core/response-source.t:
##
@@ -0,0 +1,273 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+log_level("info");
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: get_response_source returns "apisix" when ctx is nil
+--- config
+location = /t {
+content_by_lua_block {
+local core = require("apisix.core")
+local source = core.response.get_response_source(nil)
+ngx.say(source)
+}
+}
+--- response_body
+apisix
+
+
+
+=== TEST 2: get_response_source returns "apisix" when no f
