Skip to content

rate_limit: catch YAML errors instead of dying on reload - #13600

Open
sinhaparth5 wants to merge 1 commit into
apache:masterfrom
sinhaparth5:fix-rate-limit-yaml-exceptions
Open

rate_limit: catch YAML errors instead of dying on reload#13600
sinhaparth5 wants to merge 1 commit into
apache:masterfrom
sinhaparth5:fix-rate-limit-yaml-exceptions

Conversation

@sinhaparth5

Copy link
Copy Markdown

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

Only YAML::LoadFile was guarded, so a malformed value threw out of
yamlParser() and unwound into the event loop from the management update
continuation, killing a running server on config reload. Put the parse
behind an exception boundary so a bad config is rejected and the running
one kept, and check for the sni key before reading it so that case
reaches the error it already has.

Fixes: apache#13598
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.

rate_limit: YAML parse errors escape yamlParser() and terminate ATS on config reload

1 participant