Skip to content

fix(cli): preserve YAML data references when ignoring comments - #14085

Closed
ruanweihong-aaa wants to merge 6 commits into
apache:masterfrom
ruanweihong-aaa:codex/fix-yaml-comment-expansion
Closed

ruanweihong-aaa wants to merge 6 commits into
apache:masterfrom
ruanweihong-aaa:codex/fix-yaml-comment-expansion

Conversation

@ruanweihong-aaa

@ruanweihong-aaa ruanweihong-aaa commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Description

Environment-variable references in commented-out Standalone YAML can cause startup or configuration updates to fail when a variable is unset. This PR uses the existing YAML scanner to distinguish comments from configuration data. References identified as comments remain verbatim and are not added to exported variables; data references retain the existing environment lookup, default-value and error-handling behavior.

This builds on #13686 by @asdf2014 and retains its standalone regression coverage.

Implementation

  1. Classify references together. scan_reference_contexts replaces each complete reference with a single-line marker in a separate buffer and records its source position. The existing YAML scanner classifies the markers using token spans. This also supports multiline placeholders: replacing a data reference consumes the placeholder's internal line breaks.
  2. Distinguish block headers from content. For literal and folded block scalars, reference and token line numbers distinguish comments on the | / > header from the body. A hash in the block body remains data. Line tracking handles LF, CRLF, CR, NEL, LS and PS.
  3. Reuse only when the YAML context is preserved. Conservative UTF-8, line-break and scalar-boundary checks replace the narrow plain-scalar character whitelist. Ordinary values such as host:port, URLs, text containing spaces and Unicode text can reuse the classification. Quoted scalars retain their quote/escape checks. Block content uses conservative checks as well, including protection against empty or leading-space values changing inferred indentation.
  4. Preserve the sequential fallback. If a substituted value may change YAML syntax, or a skipped comment preserves line breaks that the marker removed, the remaining classification is discarded. The sequential resolver then examines the already-expanded prefix. References are processed in original order and inserted references are not expanded recursively.

The scan result is local to one substitution call. No new dependency, persistent cache, escaping rule or restriction on accepted substitutions is introduced.

Regression tests included in this PR

t/cli/test_standalone.sh covers:

  • Parsed values and bounded scanner calls at 32, 256 and 1,024 entries, including unquoted addresses, URLs, spaces, Unicode, quoted punctuation and multiline placeholders.
  • Literal/folded block headers, header comments and hash-prefixed body content across six YAML line-break forms, asserting one scan for 256 references.
  • Preserved multiline comments, whitespace boundaries, invalid UTF-8 and substitutions that generate YAML syntax.
  • Startup with unset variables occurring only in comments, and an update to the running instance's YAML file that combines a block scalar with a cross-line reference. The test checks the HTTP response and confirms that the master PID is unchanged.

Validation

On a Rocky Linux host, using the official apache/apisix:3.19.0-ubuntu runtime and dependencies with the 021de40d source tarball plus these changes:

  • Full t/cli/test_standalone.sh: passed.
  • t/config-center-yaml/route.t: 26/26 assertions passed. Test::Nginx emitted the previously observed process-cleanup timeout warnings.
  • Changed-file luacheck and lj-releng, shell syntax, test-style and diff checks: passed.

The tested production and test files match the submitted files. This is validation against the image's dependencies, not a fresh full dependency build. The new commit still needs GitHub CI.

Remaining limitation

The sequential fallback can still rescan growing prefixes and have quadratic overhead for syntax-changing values or contexts that cannot be classified in a batch. Scanner errors also retain the existing conservative data classification. This PR does not establish linear behavior for every supported YAML template. Concurrent request latency during configuration updates has not been measured.

Which issue(s) this PR fixes:

Fixes #13685.
Follow-up to #13686; this does not modify or close the original PR.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Documentation is unchanged. Functional compatibility cases pass; the compatibility checkbox remains open because the fallback performance concern still needs review.

Address block scalar, cross-line reference and runtime reload feedback on apache#13686.

Co-authored-by: asdf2014 <asdf2014@apache.org>
@ruanweihong-aaa
ruanweihong-aaa marked this pull request as ready for review October 26, 2026 05:15

@shreemaan-abhishek shreemaan-abhishek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once a configuration contains any #, file.lua rescans the entire growing prefix for every reference... we need to find a better (performant) way to solve this problem.

@AlinsRan AlinsRan left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a significant performance regression in environment-variable substitution.

Once the expanded prefix contains #, is_in_yaml_data() rescans the entire growing prefix with libyaml for every subsequent reference. A single leading comment is enough to trigger this, even when the references are all in YAML data.

The local measurements recorded for 9bb1504, with one reference per route, show approximately quadratic growth:

Routes Before this PR With this PR
500 < 0.01 s 1.56 s
1,000 < 0.01 s 6.1 s
2,000 < 0.01 s 23.6 s

This affects both startup and runtime configuration updates. config_yaml.parse runs synchronously in workers, so the additional parsing work can block request handling during reload. Concurrent request latency has not been measured.

Please address the repeated-prefix scanning before merging and add a scaling regression check alongside the functional tests. Ignoring environment references in comments should not make configuration loading quadratic.

Check YAML syntax boundaries for ordinary and Unicode values. Classify multiline references and block content in the batch scan while preserving the sequential fallback when context changes. Add scanner-count and compatibility regressions.
@ruanweihong-aaa

ruanweihong-aaa commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Pushed d8c14d8e to extend scan reuse and address more of the repeated-prefix scanning cases.

  • Replaced the narrow plain-value whitelist with conservative checks for UTF-8, YAML line breaks and scalar boundaries. Unquoted addresses, URLs, text with spaces and Unicode text can now retain the batch classification.
  • Added batch classification for multiline placeholders and block scalars, distinguishing block-header comments from body content.
  • Kept invalidation and sequential fallback when a value may change YAML syntax or a skipped comment preserves line breaks removed by the marker.

The committed t/cli/test_standalone.sh tests check parsed values and scanner counts at 32/256/1,024 entries, block references across six line-break forms, and compatibility cases involving preserved comments and syntax-changing values. The existing running-instance test also checks the changed HTTP response after a YAML file update and confirms that the master PID stays unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: commented out env variables cause app to fail to boot

3 participants