jjansenvr commented on PR #14074:
URL: https://github.com/apache/apisix/pull/14074#issuecomment-6415508670
Thanks for working on this. The approach looks right to me, and I like that
it is careful about backward compatibility.
**What I like**
- **Nothing changes for existing setups.** If none of the environment values
contain a line break, configuration loads exactly as it does today. Quoted
placeholders such as `"${{API_KEY}}"` stay strings, unquoted ones such as
`${{STATUS}}` are still read as numbers or booleans, and lists and `null` from
env vars keep working.
- **Multiline certificates and keys now work.** A PEM stored in an
environment variable can be used as-is in `cert`, `key`, `certs`, `keys` and
`client.ca`, both quoted and unquoted, without extra escaping.
- **PEM content is never altered.** The original value ends up in the config
exactly as it was provided.
- **Predictable failure mode.** When the loader cannot be sure which field a
value belongs to, it falls back to the existing behavior instead of guessing.
- **Same behavior in the CLI and at runtime.** Validation and reload use the
same logic, so a config that passes the check also loads when it is reloaded.
- **Clear documentation** of what is and is not supported, including the
`$ENV://` alternative for the unsupported cases.
**Questions / suggestions**
1. **Silent fallback.** Every ambiguous case (alias cycle, shared anchor,
marker outside an SSL field, parse failure) falls back to
`yaml.load(expanded)`. For a multiline value this usually means a YAML error or
folded newlines, with no hint why the protection did not apply. Could we log a
warning, or return a clearer error, when multiline values were detected but no
field was protected?
2. **Scope.** Only `ssls[].cert/key/certs/keys/client.ca` are covered.
Multiline PEMs also appear in other places, e.g.
`upstreams[].tls.client_cert/client_key` and plugin configs holding public or
private keys. Is the SSL-only scope intentional for this PR? If so, a sentence
in the docs, or a follow-up issue, would help. It looks like extending
`is_ssl_field` would be enough for the path-based cases.
3. **Assumption in `render()`.** It indexes `values[index]` and relies on
`substitute` calling the callback the same number of times and in the same
order on every run. That holds for `var_sub` today, but a short comment (or an
assertion that `index == #values` after rendering) would protect against future
changes to the substitution logic.
4. **Cost.** With multiline values there are up to three YAML parses plus
one event-parser pass per load. This only happens at startup and reload, so it
is probably fine, but it may be worth noting.
**Tests**
The current tests cover the main case. I would also add:
- multiline PEM in a quoted and an unquoted scalar;
- flow-style sequences containing placeholders;
- CRLF line endings inside the PEM;
- a multiline value in a non-SSL field (behavior should be unchanged, and
other fields must not be affected);
- a variable shared through an anchor with a non-SSL field (must fall back);
- a regression test showing that single-line values behave exactly as
before, including `"${{N}}"` with a numeric value staying a string.
--
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]
