rate_limit: catch YAML errors instead of dying on reload

[sinhaparth5]

SniSelector::yamlParser() guarded only YAML::LoadFile, so a malformed value
threw out of the function. On traffic_ctl config reload that unwinds into the
event loop from the management update continuation and terminates a running
server, bypassing the else branch that exists to log the failure and keep the
previous configuration.

The parsing moves into parseYamlFile() and yamlParser() becomes the
exception boundary, so a bad config is rejected and the running one kept.

It also checks for the sni key before reading it. sni["sni"].IsSequence()
throws YAML::InvalidNode on the const node, so a selector entry without an
sni key never reached the "selector node is not a map or without a name"
error that is already there for it.

The percentage documentation gave the default as 0.9, but the parser reads
an integer and the default is 90. The documented value is one of the ones
that throws, so the docs are corrected as well.

Testing

New autest rate_limit_yaml_reload covers both shapes: a selector entry with no
sni key, and a fractional percentage.

Run against the plugin built without this change, ATS dies on the first
malformed reload. The reload command itself reports success, because the plugin
callback is dispatched fire-and-forget on ET_TASK, and then the health check
returns 000, the JSONRPC socket refuses connections, and diags.log stops
mid-reload with no FATAL and no shutdown.

With this change the test passes: both configs are rejected, the errors are
logged, and ATS keeps serving. All 7 rate_limit autests pass.

Fixes: #13598

[Copilot]

Pull request overview

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.

Prevents rate_limit YAML parse exceptions from escaping during traffic_ctl config reload, which could terminate a running traffic_server, and adds regression coverage for malformed YAML reload behavior.

Changes:

• Move YAML parsing into parseYamlFile() and make yamlParser() the exception boundary to reject bad configs without crashing.
• Fix handling of selector entries missing the sni key to avoid YAML::InvalidNode throws.
• Correct plugin documentation for percentage (integer percent) and add an autest covering malformed reloads.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_reload.test.py	Adds an autest verifying malformed YAML reloads are rejected and ATS stays serving.
plugins/experimental/rate_limit/sni_selector.h	Declares parseYamlFile() to separate parsing from the exception boundary.
plugins/experimental/rate_limit/sni_selector.cc	Catches YAML exceptions at reload time; improves missing-sni handling.
doc/admin-guide/plugins/rate_limit.en.rst	Updates percentage docs to match the parser’s integer semantics/default.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sinhaparth5]

Reload parsing is performed on a newly allocated SniSelector, and SniSelector::swap(new_sel) is called only after yamlParser() succeeds. On failure, new_sel is deleted, so partial mutations cannot affect the active configuration. Startup likewise parses into a fresh instance and terminates on failure. I nevertheless moved the _yaml_file assignment to the successful end of parseYamlFile() in d70059b to keep the object invariant clear.

[sinhaparth5]

Updated in d70059b. The YAML::BadFile diagnostic now includes yaml_file.

[sinhaparth5]

Updated in d70059b. The sni child node is now stored once, while preserving the IsMap() guard before indexing the parent node.

[sinhaparth5]

Updated in d70059b. Both paths interpolated into the cp command now use shlex.quote().

[sinhaparth5]

Implemented the Copilot review recommendations in commit d70059b: deferred the YAML filename update until parsing succeeds, improved the BadFile diagnostic, avoided repeated sni node lookups, and quoted the test file paths. I also replied to each review thread with the corresponding details.

[brbzull0]

Reviewed the reload path end to end and ran the plugin locally against both commits. The exception boundary is the right fix and the scope is complete -- sni_selector.cc is the only YAML::LoadFile in the plugin, and the remap path takes argv rather than YAML.

One blocking item: the sni_node hoist in d70059b7 drops the sni.IsMap() guard, which makes a malformed selector entry get accepted and swapped in rather than rejected. Details and a verified one-line fix inline.

Also confirmed the _yaml_file deferral is safe -- it's never read during parsing, only in sni_config_cont() after the call returns.

[brbzull0]

issue: (blocking)

Hoisting the lookup is a good change, but moving sni.IsMap() into the ternary isn't equivalent to keeping it in the condition, and it stops non-map selector entries from being rejected.

YAML::Node{} is truthy -- the default constructor leaves m_isValid == true with a null m_pNode, so operator bool() -> IsDefined() returns true, and IsSequence() -> Type() returns NodeType::Null. A non-map entry therefore satisfies sni_node && !sni_node.IsSequence() and enters the branch that the else used to catch. as<std::string>() doesn't throw there either -- as_if<std::string, void> returns the literal "null" for a Null node -- so a limiter named "null" is constructed and parsing continues to a successful return.

I built both commits and reloaded the same config, an entry with one level of extra indentation:

selector:
  - - sni: indented.example.com
      limit: 5

At 341a0cf4:

[ET_TASK 0] ERROR: [rate_limit] selector node is not a map or without a name
[ET_TASK 0] ERROR: [rate_limit] Failed to reload YAML file: .../rate_limit.yaml

At d70059b7:

[ET_TASK 0] <sni_selector.cc:179 (parseYamlFile)> Succesfully loaded YAML file: .../rate_limit.yaml
[ET_TASK 0] <sni_selector.cc:206 (sni_config_cont)> Reloading YAML file: .../rate_limit.yaml

parseYamlFile() returns true and sni_config_cont() takes the success branch, so swap() installs a selector whose only entry is the "null" limiter. Every configured SNI limit is dropped, traffic_ctl config reload reports success, and nothing lands in diags.log. On a plugin whose job is enforcing limits, an indentation slip silently disabling enforcement seems worth catching before this merges.

Restoring the map check in the condition keeps the hoist and the original behavior:

Suggested change

	if (sni_node && !sni_node.IsSequence()) {
	if (sni.IsMap() && sni_node && !sni_node.IsSequence()) {

I applied exactly that on top of d70059b7 and re-ran: the indentation config is rejected again, Reloading YAML file appears zero times so nothing is swapped in, this PR's rate_limit_yaml_reload test still passes, and all 8 rate_limit autests pass.

Worth a third case in the test loop as well, since the existing two are both maps and take the else correctly either way:

non_map = os.path.join(Test.RunDirectory, 'non_map.yaml')
with open(non_map, 'w') as f:
    f.write('selector:\n  - - sni: indented.example.com\n      limit: 5\n')

[sinhaparth5]

Fixed in ae9257b, applying the suggestion as written. I kept the ternary so sni["sni"] is still only read on a map node.

Confirmed the Null node behaviour: as_if<std::string, void> returns "null" for it, so the entry was accepted instead of throwing, and every configured limit went with it on the swap. The indented config is now a third case in the test loop.

[brbzull0]

suggestion:

The test can't currently detect the thing it's guarding, which is why the sni_node change in this commit passes it.

Three gaps:

• The only positive signal is curl returning 200, and the origin returns 200 whether or not the rate_limit config applied. So the test can't distinguish "config rejected, previous one kept" from "malformed config accepted and the limits silently dropped" -- and the second is the outcome that actually hurts.
• These are existence checks over the whole log, not per-reload assertions. If the first reload is rejected and the second is silently accepted, "Failed to reload YAML file" is still present once and all four testers pass, despite the comment saying "Both reloads should be rejected".
• ExcludesExpression("FATAL") doesn't catch the original crash. An uncaught exception reaching the event loop is std::terminate/SIGABRT -- there is no try/catch in UnixEThread.cc -- so nothing is written to diags.log at all; the log just stops. StillRunningAfter and the 200 are what actually detect it. This tester reads like a guard but isn't one.

A direct assertion that no swap happened would close all three, since sni_config_cont() only logs that on the success path:

ts.Disk.traffic_out.Content = Testers.ExcludesExpression(
    "Reloading YAML file", "No malformed config should ever be swapped in")

[sinhaparth5]

Added the traffic_out exclusion in ae9257b. sni_config_cont() logs Reloading YAML file only on the success path, right before swap(), so that is the assertion the sni_node regression fails.

Dropped the ExcludesExpression("FATAL") tester for the reason you give: the abort writes nothing, so StillRunningAfter and the 200 are what catch it, and the tester only looked like a guard.

The per-reload point stands, the existence checks still pass if one reload is rejected and another is not. The exclusion covers all three at once, so a silent accept fails now regardless of which one it was.

[brbzull0]

nitpick: (non-blocking)

"the management update continuation" reads as though this runs on the management/RPC thread. It's ConfigUpdateCallback on an ET_TASK thread -- ConfigUpdateCbTable::invoke() does schedule_imm(new ConfigUpdateCallback(contp), ET_TASK), two hops from the RPC handler, which is why the original failure presented so oddly: the reload had already reported success by the time the process died. The ET_TASK 0 prefix on the plugin's own error lines shows it.

Something like "on an ET_TASK thread, via ConfigUpdateCallback" would point the next reader at the right place.

[sinhaparth5]

Reworded in ae9257b to "a config reload runs this on an ET_TASK thread via ConfigUpdateCallback". That points at the right place, and it explains why the reload reported success before the process died.

[brbzull0]

praise:

Making the whole parse the guarded region rather than just LoadFile is the right call, and it fixes more than the two cases in the test -- list["name"].as<std::string>() and ipr["name"].as<std::string>() in the lists and ip-rep loops throw on a non-scalar too, and were reaching the event loop the same way.

The const-node diagnosis behind the sni key check was also exactly right: the const operator[] returns a ZombieNode, IsSequence() goes through Type() which throws InvalidNode, while operator bool() goes through IsDefined() which returns false without throwing.

[sinhaparth5]

Pushed ae9257b with the three items from the review:

• Restored sni.IsMap() in the condition, keeping the hoist.
• Added the non-map selector entry as a third case in the test loop, and the traffic_out exclusion on Reloading YAML file so a silent accept fails instead of passing on the 200s. Dropped the FATAL tester.
• Reworded the comment to name the ET_TASK thread and ConfigUpdateCallback.