Copilot commented on code in PR #13364:
URL: https://github.com/apache/apisix/pull/13364#discussion_r3225422475
##########
t/core/config_etcd.t:
##########
@@ -567,3 +567,48 @@ passed
qr/etcd watch timeout, upgrade revision to/
--- grep_error_log_out eval
qr/(etcd watch timeout, upgrade revision to\n){2,}/
+
+
+
+=== TEST 15: missing X-Etcd-Index header should not crash init worker
+--- yaml_config
+deployment:
+ role: traditional
+ role_traditional:
+ config_provider: etcd
+ etcd:
+ host:
+ - "http://127.0.0.1:2379";
+ prefix: /apisix
+--- extra_init_by_lua
+ -- Inject a fake loaded_configuration entry with missing X-Etcd-Index
+ -- header so do_run_watch exercises the fallback path.
+ local config_etcd = require("apisix.core.config_etcd")
+ for i = 1, 256 do
+ local name, val = debug.getupvalue(config_etcd.new, i)
+ if not name then
+ break
+ end
Review Comment:
If the `loaded_configuration` upvalue is not found (implementation changes,
different LuaJIT behavior, etc.), the loop exits silently and the test failure
will be indirect (missing log line). Consider tracking a `found` flag and
calling `error(...)` if the upvalue isn't located so the test fails with a
clear, actionable message.
##########
t/core/config_etcd.t:
##########
@@ -567,3 +567,48 @@ passed
qr/etcd watch timeout, upgrade revision to/
--- grep_error_log_out eval
qr/(etcd watch timeout, upgrade revision to\n){2,}/
+
+
+
+=== TEST 15: missing X-Etcd-Index header should not crash init worker
+--- yaml_config
+deployment:
+ role: traditional
+ role_traditional:
+ config_provider: etcd
+ etcd:
+ host:
+ - "http://127.0.0.1:2379";
+ prefix: /apisix
+--- extra_init_by_lua
+ -- Inject a fake loaded_configuration entry with missing X-Etcd-Index
+ -- header so do_run_watch exercises the fallback path.
+ local config_etcd = require("apisix.core.config_etcd")
+ for i = 1, 256 do
+ local name, val = debug.getupvalue(config_etcd.new, i)
+ if not name then
+ break
+ end
+ if name == "loaded_configuration" and type(val) == "table" then
+ val["__test_fake"] = {
+ body = { nodes = {} },
+ headers = {},
+ }
+ break
+ end
Review Comment:
`do_run_watch` reads `local _, res = next(loaded_configuration)`. Inserting
`__test_fake` does not guarantee it will be the entry returned by `next()`
(iteration order is undefined), so this test can be flaky if
`loaded_configuration` already contains other entries. To make the test
deterministic, clear the table before inserting the fake entry (e.g., remove
all existing keys) so `next()` must return the injected value.
##########
apisix/core/config_etcd.lua:
##########
@@ -150,7 +149,11 @@ local function do_run_watch(premature)
local _, res = next(loaded_configuration)
if res then
rev = tonumber(res.headers["X-Etcd-Index"])
- assert(rev > 0, 'invalid res.headers["X-Etcd-Index"]')
+ if not rev or rev <= 0 then
+ log.warn("invalid or missing X-Etcd-Index header, ",
+ "will fetch revision from etcd directly")
Review Comment:
If a proxy consistently strips `X-Etcd-Index`, this warning may be emitted
repeatedly (potentially every watch cycle) and can create noisy logs. Consider
rate-limiting the warning or logging it only once per worker (e.g., via a
module-level flag) while still keeping the `rev = 0` fallback behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
