rate_limit: reject unknown YAML configuration keys - #13657
Draft
moonchen wants to merge 3 commits into
Draft
Conversation
Reject misspelled or stale settings instead of silently using defaults, and propagate queue and metrics validation failures to the config loader. Add startup coverage for each mapping and document strict key validation. Fixes: apache#13529
moonchen
added
Plugins
Configuration
WIP
Incompatible
rate_limit
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR tightens rate_limit YAML configuration parsing by rejecting unknown keys (preventing silent typos) and by failing configuration loads on queue/metrics parsing errors, with diagnostics that identify the offending key/node and line.
Changes:
- Add a shared YAML-mapping key validator (
validate_yaml_keys) and apply it across configuration levels (root, selector, lists, queue/metrics, ip-rep, perma-block). - Propagate YAML parsing failures to stop configuration loading instead of continuing with partial defaults.
- Add AuTests for unknown keys/malformed nodes and document the compatibility change.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py | Adds AuTest scenarios validating unknown-key rejection and malformed-node handling. |
| plugins/experimental/rate_limit/utilities.h | Declares validate_yaml_keys and adds required includes. |
| plugins/experimental/rate_limit/utilities.cc | Implements validate_yaml_keys with diagnostics for malformed mappings/unknown keys. |
| plugins/experimental/rate_limit/sni_selector.cc | Validates top-level YAML keys and selector mapping keys during parse. |
| plugins/experimental/rate_limit/sni_limiter.cc | Propagates base YAML parsing failure instead of ignoring it. |
| plugins/experimental/rate_limit/lists.cc | Rejects unknown YAML keys in lists mappings. |
| plugins/experimental/rate_limit/limiter.h | Rejects unknown YAML keys in queue and metrics mappings. |
| plugins/experimental/rate_limit/ip_reputation.cc | Rejects unknown YAML keys in ip-rep and perma-block mappings. |
| doc/admin-guide/plugins/rate_limit.en.rst | Documents unknown-key failure behavior and max_age compatibility guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
1. A malformed value such as "limit: abc" makes yaml-cpp throw out of as<T>(). Nothing caught it, so traffic_server aborted with no diagnostic instead of reporting the bad configuration, and a reload took down a running server. Catch YAML::Exception around the parse. 2. The "rate" setting assigned to _limit instead of _rate. Requests per second were never enforced, because _rate stayed 0 and addBucket() never ran, and setting "rate" silently overrode "limit". 3. Reject an empty configuration file, and name "selector: []" as the way to load the plugin with no rules. 4. The AuTest startup check looked for "Traffic Server is fully initialized" in traffic.out. That message is a Note, and proxy.config.diags.output.note defaults to the log file, so the check could never fail. Look in diags.log instead.
On a const YAML node, operator[] returns a zombie node for a missing key, and IsScalar() throws on it. A selector entry without an "sni" key therefore threw instead of reporting the missing name. Test the node for definedness first, which is safe, and add a case for it. Also from review: * Name the line number when a top level node is not a sequence. * Correct the spelling of "Successfully" in the load message. * Declare YAML::Node in utilities.h instead of including yaml-cpp there. * Record the strict validation in the v11 upgrading notes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The rate_limit YAML parser silently ignored unknown keys, so a typo such as
max-ageleft aging disabled. Validate each configuration mapping against its accepted keys and reject unknown keys with a diagnostic naming the key, node, and line. Propagate queue and metrics parsing failures so they prevent configuration loading.Adds 13 AuTest scenarios covering valid settings, unknown keys at every mapping level, and malformed nodes, and documents the configuration compatibility change.
Fixes: #13529
Validation:
rate_limitplugin.rate_limit_yaml_keys,rate_limit_iprep, andrate_limit_sni_expiryAuTests.git diff --check.