fix(cli): remove stale key after resolving env var in config keys#12885
Conversation
|
Hi @pronchakov, can you add corresponding tests for this fix? |
Ok, I'll add some tests. |
moonming
left a comment
There was a problem hiding this comment.
Hi @pronchakov, thank you for catching this bug!
This is a clear and correct fix — conf.key = nil should be conf[key] = nil. The current code sets a literal field named key to nil instead of deleting the dynamic key. Great catch!
One request: Could you please add a test case that verifies this behavior? Something like:
- Define a route/config where a key name contains an environment variable reference (e.g.,
\${{ENV_KEY_NAME}}) - Verify that after resolution, the old key is properly removed and the new key exists
This is a one-line fix with clear correctness, so once the test is added, this should be ready to merge. Thank you!
|
Hi @pronchakov, following up on the previous review comments. Please let us know if you have any updates. Thank you. |
Inserting the resolved key while iterating conf with pairs() is undefined behavior; the new_keys guard only masked one of its symptoms. Collect the renames and apply them after the loop instead. Also add a standalone JSON test, which is the only remaining caller of resolve_conf_var() that reproduces the upstream node key case from apache#12884.
|
@pronchakov I pushed two commits to your branch — one reshapes the fix, so here's why. The one-char fix isn't enough.
New shape: collect renames during the traversal, apply them after. No insert while iterating, so if renamed_keys then
for key, new_key in pairs(renamed_keys) do
conf[new_key] = conf[key]
conf[key] = nil
end
endTests. Yours covers the Worth noting what it exposes — with the pre-fix code the request doesn't just log a DNS error, it times out. Roundrobin hits the stale unresolved node and hangs. The impact in standalone JSON is heavier than the issue suggests. Verified: luacheck clean, TEST 8 passes with the fix and fails without it, |
Description
There is a little bug in function
local function resolve_conf_var(conf)incli/file.luaconf.key = nildeletes entry from conf with name "key"but after variable substitution, old entry with name contained in variable
keyshould be removed.so it needs to be rewritten to
conf[key] = nilconf.key = nilresults in adding entry with resolved variable to conf, but doesn't remove entry with unresolved variable name.In case of using it in routes->upstream->nodes it results in two node entries. one is correct and one with unresolved variable name in it, that results in dns error in logs. I suppose that another places will result in different consequences.
Which issue(s) this PR fixes:
Fixes #12884
Checklist