You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "asdf2014 (via GitHub)" <gi...@apache.org> on 2026/07/09 17:17:41 UTC
[PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
asdf2014 opened a new pull request, #13686:
URL: https://github.com/apache/apisix/pull/13686
### Description
Standalone mode fails to boot when `apisix.yaml` contains a commented-out env var reference:
```yaml
# ${{MY_ENV_VAR}}
```
Since #13078 (shipped in 3.17.0), `${{VAR}}` is substituted on the raw text *before* YAML parsing so the parser can infer scalar types — which means comment lines are substituted too, and an unset var inside a comment aborts startup with `can't find environment variable MY_ENV_VAR`. (`config.yaml` is unaffected: it substitutes on the parsed table where comments are already gone; same for JSON standalone configs, which also resolve post-parse.)
This patch makes the raw-text substitution run line by line, keeping full-line comments (and blank / no-reference lines) verbatim, reusing the existing `is_empty_yaml_line` predicate. A leading UTF-8 BOM is detached first so a first-line comment is still recognized. Unset vars on data lines still fail with the same error message; output is byte-identical to the previous implementation for comment-free input (line endings, missing trailing newline, and the `#END` marker are all preserved). Both callers — CLI init (`cli/file.lua`) and the runtime hot-reload (`core/config_yaml.lua`) — share the fixed function.
Behavior notes (accepted trade-offs of the text-level heuristic, documented in the code comment):
- inline trailing comments (`key: val # ${{VAR}}`) are still substituted — `#` may legally appear inside YAML strings, and telling those apart requires a full parse, which can only happen after substitution;
- a block-scalar content line that itself starts with `#` is now skipped like a comment (no in-tree config or doc example hits this; the failure mode is a visible literal `${{VAR}}`, not a crash);
- a `${{...}}` reference spanning a physical line no longer matches (all in-tree references are single-line);
- a var referenced *only* in a comment no longer emits an `env VAR;` nginx directive (comments having side effects was part of the bug).
The new test in `t/cli/test_standalone.sh` covers both paths: `make init` (CLI) and `make run` + request (runtime hot-reload) against a config whose commented-out lines reference unset vars.
#### Which issue(s) this PR fixes:
Fixes #13685
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "cfitzw (via GitHub)" <gi...@apache.org>.
cfitzw commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-5329539597
Any further updates/progress on this?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-5114732172
Concrete sketch of the token approach, in case it helps the discussion. The exported API changes from `resolve_conf_var_in_text(text) -> text` to `load_yaml_conf(text) -> conf, err`, because resolution now happens after parsing; the two call sites are `apisix/cli/file.lua:321` and `apisix/core/config_yaml.lua:101`.
```lua
local TOKEN_PREFIX = "__APISIX_ENV_VAR_"
local TOKEN_SUFFIX = "__"
-- pass 1: replace every `${{VAR}}` reference with a token that survives YAML
-- parsing as a plain scalar. References inside comments become tokens too, but
-- the YAML parser drops them, so an unset var in a comment never surfaces.
local function tokenize_conf_var(text)
local refs = {}
local parts = {}
local pos = 1
while true do
local from, to, body = str_find(text, "%$%{%{%s*(.-)%s*%}%}", pos)
if not from then
break
end
local var, default = body, nil
local i = str_find(body, "%:%=")
if i then
var = str_sub(body, 1, i - 1)
default = str_sub(body, i + 2):gsub("^%s*(.-)%s*$", "%1")
end
-- a reference wrapped in quotes stays a string, an unquoted one takes
-- its type from the value (#13078)
local quote = str_sub(text, from - 1, from - 1)
local quoted = (quote == '"' or quote == "'")
and str_sub(text, to + 1, to + 1) == quote
refs[#refs + 1] = {var = var, default = default, quoted = quoted}
parts[#parts + 1] = str_sub(text, pos, from - 1)
parts[#parts + 1] = TOKEN_PREFIX .. #refs .. TOKEN_SUFFIX
pos = to + 1
end
parts[#parts + 1] = str_sub(text, pos)
return table_concat(parts), refs
end
local function lookup(ref)
local v = getenv(ref.var) or ref.default
if not v then
return nil, "failed to handle configuration: " ..
"can't find environment variable " .. ref.var
end
if not exported_vars then
exported_vars = {}
end
exported_vars[ref.var] = v -- still feeds get_exported_vars()
return v
end
-- pass 2: resolve the tokens that made it into the parsed config
local function resolve_token(str, refs)
local idx = str:match("^" .. TOKEN_PREFIX .. "(%d+)" .. TOKEN_SUFFIX .. "$")
if idx then
local ref = refs[tonumber(idx)]
local v, err = lookup(ref)
if not v then
return nil, err
end
if ref.quoted then
return v
end
if tonumber(v) then
return tonumber(v)
elseif v == "true" then
return true
elseif v == "false" then
return false
end
return v
end
local err
local new_str = str:gsub(TOKEN_PREFIX .. "(%d+)" .. TOKEN_SUFFIX, function(i)
local v, e = lookup(refs[tonumber(i)])
if not v then
err = e
return ""
end
return v
end)
if err then
return nil, err
end
return new_str
end
local function resolve_conf_var_in_tree(conf, refs)
local renamed_keys
for key, val in pairs(conf) do
if type(val) == "table" then
local ok, err = resolve_conf_var_in_tree(val, refs)
if not ok then
return nil, err
end
elseif type(val) == "string" then
local new_val, err = resolve_token(val, refs)
if err then
return nil, err
end
conf[key] = new_val
end
if type(key) == "string" then
local new_key, err = resolve_token(key, refs)
if err then
return nil, err
end
if new_key ~= key then
renamed_keys = renamed_keys or {}
renamed_keys[#renamed_keys + 1] = {key, tostring(new_key)}
end
end
end
-- deferred, inserting a key while iterating with pairs() is undefined
for _, pair in ipairs(renamed_keys or {}) do
conf[pair[2]] = conf[pair[1]]
conf[pair[1]] = nil
end
return true
end
function _M.load_yaml_conf(text)
local tokenized, refs = tokenize_conf_var(text)
local ok, conf = pcall(yaml.load, tokenized)
if not ok then
return nil, "invalid yaml: " .. tostring(conf)
end
if type(conf) ~= "table" then
return conf
end
local resolved, err = resolve_conf_var_in_tree(conf, refs)
if not resolved then
return nil, err
end
return conf
end
```
Output of that sketch on the cases from my previous comment (`W=3 B=true K=mykey HASHY='secret # not-a-comment' MULTI=$'a\nb: 2' BIG=356002209726529540`):
```
full-line comment {"key":1}
inline comment {"key":1}
missing var ERR: failed to handle configuration: can't find environment variable UNSET_XYZ
unquoted number {"weight":3}
quoted number {"key":"3"}
unquoted bool {"flag":true}
default fallback {"key":"fallback"}
var in key {"mykey":1}
embedded string {"key":"pre-3-post"}
value with # {"key":"secret # not-a-comment"}
multiline value {"key":"a\nb: 2"}
block scalar {"body":"# 3 stays\n"}
big int quoted {"id":"356002209726529540"}
two refs one line {"key":"mykey-3"}
exported_vars {"B":"true","BIG":"356002209726529540","HASHY":"secret # not-a-comment",
"K":"mykey","MULTI":"a\nb: 2","NOPE":"fallback","W":"3"}
```
Two notes on it:
- `exported_vars` is still populated during pass 2, so `get_exported_vars()` and the generated nginx `env` directives keep working — the resolution just moves later.
- token collision is the one thing to guard: if the raw text already contains `TOKEN_PREFIX`, bail out or pick another prefix.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-5114688528
The full-line skip does fix the reported case, but I'd suggest deciding whether we want to keep patching at the text level at all before merging this.
Three things I verified by calling master's `resolve_conf_var_in_text()` directly (`82fcf548f`):
```
key: 1 # ${{UNSET}} -> ERR: can't find environment variable UNSET # inline comment, not covered here
key: ${{V}} V='secret # x' -> {"key":"secret"} # value silently truncated
key: ${{V}} V=$'a\nb: 2' -> {"b":2,"key":"a"} # env value injects a top-level key
```
And one regression this PR would add: a block-scalar content line starting with `#` is now skipped, so
```yaml
response_example: |
# ${{TITLE}}
```
silently stops being substituted.
The root cause is that pre-parse text substitution (#13078) re-implements YAML lexing outside lyaml. An alternative that avoids all four: replace each `${{...}}` with a unique plain-scalar token before parsing (recording var / default / whether it was directly wrapped in quotes), let lyaml parse — comments, block scalars and quoting are then handled by the parser — then resolve the tokens that survived into the parsed tree. "Env var not found" only fires when a reference actually reaches the config, so commented-out references never fail by construction, in either comment style.
I prototyped that and ran it against the cases #13078 added plus the new ones:
```
full-line comment {"key":1} inline comment {"key":1}
missing var ERR (unchanged) unquoted number {"weight":3}
quoted number {"key":"3"} unquoted bool {"flag":true}
default fallback {"key":"fallback"} var in key {"mykey":1}
embedded in string {"key":"pre-3-post"} big int quoted {"id":"356002209726529540"}
value containing # {"key":"secret # not-a-comment"}
multiline value {"key":"a\nb: 2"} block scalar {"body":"# 3 stays\n"}
```
One intentional behavior change there: env values are always injected as scalars, so `nodes: ${{NODES_JSON}}` no longer expands into a YAML structure. That is the same path as the injection case above, so I'd argue for closing it, but it needs a changelog note.
If the preference is to keep this PR minimal, it should at least become comment-aware instead of line-aware: find the first `#` that is outside quotes and preceded by start-of-line or whitespace, substitute only what precedes it, and track `|`/`>` block-scalar ranges so their content is still substituted. That covers the inline-comment case and removes the regression, though truncation and injection would remain.
Ref #13685.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "cfitzw (via GitHub)" <gi...@apache.org>.
cfitzw commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-5697982322
@AlinsRan - can you review this further and provide directive?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on code in PR #13686:
URL: https://github.com/apache/apisix/pull/13686#discussion_r4284600625
##########
apisix/cli/file.lua:
##########
@@ -92,12 +93,32 @@ end
-- Substitute env vars in raw text before parsing. YAML parser then infers
-- types naturally: `"${{VAR}}"` stays string, `${{VAR}}` infers from value.
+-- Substitution runs line by line so that full-line comments stay untouched:
+-- a commented-out `# ${{VAR}}` must not fail startup when VAR is unset.
+-- Known limits of this text-level heuristic: an inline trailing comment is
+-- still substituted (`#` may legally appear inside YAML strings), and a
+-- block-scalar content line starting with `#` is skipped like a comment.
local function resolve_conf_var_in_text(text)
- local new_text, _, err = var_sub(text)
- if err then
- return nil, err
+ local parts = {}
+ -- a UTF-8 BOM would hide the `#` of a first-line comment, so detach it
+ if str_sub(text, 1, 3) == "\239\187\191" then
+ parts[1] = str_sub(text, 1, 3)
+ text = str_sub(text, 4)
+ end
+ for line, eol in str_gmatch(text, "([^\n]*)(\n?)") do
Review Comment:
When ${{VAR and }} span lines, var_sub never sees the complete reference. YAML folds the remaining text into an unresolved value, breaking configuration that previously loaded.
##########
t/cli/test_standalone.sh:
##########
@@ -660,5 +660,55 @@ fi
echo "passed: missing env var produces clear startup error"
+# test: unset env vars referenced only in commented-out lines must be ignored
+echo '
+apisix:
+ enable_admin: false
+deployment:
+ role: data_plane
+ role_data_plane:
+ config_provider: yaml
+' > conf/config.yaml
+
+echo '
+# ${{COMMENTED_UNSET_VAR}}
+routes:
+ -
+ uri: /test-commented-var
+ # indented comment: ${{ANOTHER_COMMENTED_UNSET_VAR}}
+ plugins:
+ response-rewrite:
+ body: "commented-ok"
+ status_code: 200
+ upstream:
+ nodes:
+ "127.0.0.1:9091": 1
+ type: roundrobin
+#END
+' > conf/apisix.yaml
+
+unset COMMENTED_UNSET_VAR
+unset ANOTHER_COMMENTED_UNSET_VAR
+make init
+
+if ! make run > output.log 2>&1; then
+ cat output.log
+ echo "failed: make run should ignore env vars in commented-out lines"
+ exit 1
+fi
+sleep 0.1
+
+code=$(curl -o /tmp/response_body -s -m 5 -w %{http_code} http://127.0.0.1:9080/test-commented-var)
Review Comment:
The test writes the YAML before make run and makes one request. It covers initial loading, not a file change during runtime as the PR description claims.
##########
apisix/cli/file.lua:
##########
@@ -92,12 +93,32 @@ end
-- Substitute env vars in raw text before parsing. YAML parser then infers
-- types naturally: `"${{VAR}}"` stays string, `${{VAR}}` infers from value.
+-- Substitution runs line by line so that full-line comments stay untouched:
+-- a commented-out `# ${{VAR}}` must not fail startup when VAR is unset.
+-- Known limits of this text-level heuristic: an inline trailing comment is
+-- still substituted (`#` may legally appear inside YAML strings), and a
+-- block-scalar content line starting with `#` is skipped like a comment.
local function resolve_conf_var_in_text(text)
- local new_text, _, err = var_sub(text)
- if err then
- return nil, err
+ local parts = {}
+ -- a UTF-8 BOM would hide the `#` of a first-line comment, so detach it
+ if str_sub(text, 1, 3) == "\239\187\191" then
+ parts[1] = str_sub(text, 1, 3)
+ text = str_sub(text, 4)
+ end
+ for line, eol in str_gmatch(text, "([^\n]*)(\n?)") do
+ if is_empty_yaml_line(line) or not str_find(line, "${{", 1, true) then
Review Comment:
When a block scalar contains a line starting with #, is_empty_yaml_line skips its environment reference. The loaded value retains ${{VAR}} instead of the variable's value.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "cfitzw (via GitHub)" <gi...@apache.org>.
cfitzw commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-6201984338
As of helm-chart `2.18.2` - the instance came up successfully -- meaning, this PR can likely be closed.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cli): skip commented-out lines when resolving env vars in apisix.yaml [apisix]
Posted by "asdf2014 (via GitHub)" <gi...@apache.org>.
asdf2014 commented on PR #13686:
URL: https://github.com/apache/apisix/pull/13686#issuecomment-5119194280
Agreed on the root cause: pre-parse text substitution re-implements YAML lexing outside lyaml, and the tokenize-then-parse-then-resolve approach is the right end state. It fixes all four cases by construction, and your prototype results (especially `value containing #` and the multiline injection) make a strong case. Closing the structural-expansion path also seems right to me, since it is the same injection surface, and a changelog entry covers it.
On the middle option (comment-aware substitution): I would rather not go there. Tracking quotes plus `|`/`>` ranges at the text level is halfway to a YAML lexer, with its own corner cases (multi-line quoted strings, `#` inside flow collections, nested block-scalar indentation), while truncation and injection still remain. If we want more than the minimal fix, the token approach is the better place to spend that complexity.
So my suggestion for sequencing: keep this PR as the minimal stopgap for the 3.17 regression (#13685 blocks startup today, and this change is byte-transparent for comment-free input), and land your tokenization as the follow-up that replaces this code path entirely. The block-scalar direction change here is documented in the code comment and hits nothing in-tree, and it disappears together with the rest of the text-level heuristic once tokenization lands. If the maintainers would rather go straight to the token approach in one step, I am also happy to close this in favor of your prototype, whichever you prefer.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org