AlinsRan commented on PR #12885:
URL: https://github.com/apache/apisix/pull/12885#issuecomment-4931618329
@pronchakov I pushed two commits to your branch — one changes the shape of
the fix, so here is the reasoning.
**What was wrong with the one-character fix.** `conf[key] = nil` is correct
as far as it goes, but it leaves both the delete *and* the insert
(`conf[new_key] = val`) inside the `pairs()` loop. Inserting a new key into a
table you are iterating with `pairs()` is undefined behavior in Lua — LuaJIT
may raise `invalid key to 'next'`, skip entries, or revisit them. The
`new_keys` table was there to paper over exactly one of those symptoms (the
freshly inserted key getting visited again), not to make the loop safe.
`new_keys` also carried a bug of its own. The guard is `if new_keys[key]
then goto continue`, keyed on the *resolved* name. If a resolved key happens to
equal another key that already exists in the same table, that pre-existing
entry is skipped wholesale — its value never gets its own placeholders
substituted. Contrived, but it is silent when it happens.
**What the new shape does.** Collect the renames during the traversal, apply
them after it. No insert while iterating, so `new_keys` and the `goto` both
disappear:
```lua
if renamed_keys then
for key, new_key in pairs(renamed_keys) do
conf[new_key] = conf[key]
conf[key] = nil
end
end
```
I also moved the key handling below the value handling, so it reads as "fix
the values, then rename the keys". This is the same structure the EE fork
converged on independently, which is a decent signal it is the right one.
**Test coverage.** Your `t/cli/test_http_config.sh` case covers the
`config.yaml` caller. I added `t/config-center-json/route-upstream.t` TEST 8,
which puts the env var in an upstream node key in standalone `apisix.json` —
the closest reproduction of #12884 that still reaches `resolve_conf_var()` on
master (the `apisix.yaml` path stopped calling it after #13078). Worth noting
what it exposes: with the pre-fix code the request does not merely log a DNS
error, it **times out** — roundrobin hits the stale unresolved node and hangs.
So the impact of this bug in standalone JSON is heavier than the issue reports.
Verified: `luacheck` clean, TEST 8 passes with the fix and fails without it,
`t/core/config.t` still green.
One more thing — could you retitle the PR? `fix: old entry with unresolved
variable name doesn't deletes from config table after var value substitution`
has a grammar slip and is long. Something like `fix(cli): remove stale key
after resolving env var in config keys` would do.
--
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]
