Re: [PR] fix: resolve env vars before YAML parsing to preserve types in standalone mode [apisix]
Baoyuantop merged PR #13078: URL: https://github.com/apache/apisix/pull/13078 -- 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]
Re: [PR] fix: resolve env vars before YAML parsing to preserve types in standalone mode [apisix]
lxbme commented on PR #13078:
URL: https://github.com/apache/apisix/pull/13078#issuecomment-4257850305
Hi, @nic-6443. I believe the existing regression issues require a more
long-term discussion. Currently, the following issues have been identified:
1. Handling of `"` and `\n` in double-quoted (`dquote`) scalars, as well as
support for multi-line double-quoted strings.
2. Handling of the `'` character within single-quoted (`squote`) scalars.
3. Handling of `#` or flow indicators (e.g., `,`, `[]`, `{}`) within plain
scalars.
Additionally, several scenarios need clarification:
- How block scalars (`|`, `>`) should be supported.
- Whether variable replacement should occur within comment contexts (e.g.,
`# ${{V}}`).
I am concerned about whether the current pre-parsing approach is truly
appropriate. It introduces numerous potential regressions, and addressing them
might necessitate substantial changes to the existing logic. I wonder if this
exceeds the scope of the original issue? I believe further discussion and
evaluation are necessary.
--
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]
Re: [PR] fix: resolve env vars before YAML parsing to preserve types in standalone mode [apisix]
nic-6443 commented on code in PR #13078:
URL: https://github.com/apache/apisix/pull/13078#discussion_r3085608495
##
apisix/cli/file.lua:
##
@@ -90,6 +90,17 @@ local function var_sub(val)
end
+-- Substitute env vars in raw text before parsing. YAML parser then infers
+-- types naturally: `"${{VAR}}"` stays string, `${{VAR}}` infers from value.
+local function resolve_conf_var_in_text(text)
+local new_text, _, err = var_sub(text)
+if err then
+return nil, err
+end
+return new_text
Review Comment:
Moving substitution to pre-parse introduces regressions: env var **values**
containing YAML metacharacters (`"`, `\`, `:`, `#`, `{`, newlines) now get
injected into raw YAML text before parsing.
Three concrete scenarios:
1. `DB_PASS='p@ss"word'` with template `password: "${{DB_PASS}}"` → produces
`password: "p@ss"word"` — invalid YAML.
2. `VAR='C:\temp\new'` with template `key: "${{VAR}}"` → YAML interprets
`\t` as tab, `\n` as newline — **silent data corruption**, no parse error.
3. `MY_VAR='host:8080'` with template `value: ${{MY_VAR}}` → YAML parse
error (`:` creates a mapping).
The old post-parse approach was immune to all of these since YAML parsing
happened before value substitution.
Also, `${{VAR}}` patterns inside YAML **comments** are now processed — the
old approach ignored them since comments were stripped by `yaml.load()` first.
A user documenting `# Set ${{MY_API_KEY}} to your key` would now get a startup
error if that env var is unset.
These are behavioral regressions that need to be addressed — either by
YAML-aware escaping of substituted values, or by a hybrid approach.
--
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]
Re: [PR] fix: resolve env vars before YAML parsing to preserve types in standalone mode [apisix]
nic-6443 commented on code in PR #13078:
URL: https://github.com/apache/apisix/pull/13078#discussion_r3085608495
##
apisix/cli/file.lua:
##
@@ -90,6 +90,17 @@ local function var_sub(val)
end
+-- Substitute env vars in raw text before parsing. YAML parser then infers
+-- types naturally: `"${{VAR}}"` stays string, `${{VAR}}` infers from value.
+local function resolve_conf_var_in_text(text)
+local new_text, _, err = var_sub(text)
+if err then
+return nil, err
+end
+return new_text
Review Comment:
Moving substitution to pre-parse introduces regressions: env var **values**
containing YAML metacharacters (`"`, `\`, `:`, `#`, `{`, newlines) now get
injected into raw YAML text before parsing.
Three concrete scenarios:
1. `DB_PASS='p@ss"word'` with template `password: "${{DB_PASS}}"` → produces
`password: "p@ss"word"` — invalid YAML.
2. `VAR='C:\temp\new'` with template `key: "${{VAR}}"` → YAML interprets
`\t` as tab, `\n` as newline — **silent data corruption**, no parse error.
3. `MY_VAR='host:8080'` with template `value: ${{MY_VAR}}` → YAML parse
error (`:` creates a mapping).
The old post-parse approach was immune to all of these since YAML parsing
happened before value substitution.
Also, `${{VAR}}` patterns inside YAML **comments** are now processed — the
old approach ignored them since comments were stripped by `yaml.load()` first.
A user documenting `# Set ${{MY_API_KEY}} to your key` would now get a startup
error if that env var is unset.
These are behavioral regressions that need to be addressed — either by
YAML-aware escaping of substituted values, or by a hybrid approach.
--
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]
