rate_limit: catch YAML errors instead of dying on reload - #13600
Open
sinhaparth5 wants to merge 1 commit into
Open
rate_limit: catch YAML errors instead of dying on reload#13600sinhaparth5 wants to merge 1 commit into
sinhaparth5 wants to merge 1 commit into
Conversation
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
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.
SniSelector::yamlParser()guarded onlyYAML::LoadFile, so a malformed valuethrew out of the function. On
traffic_ctl config reloadthat unwinds into theevent loop from the management update continuation and terminates a running
server, bypassing the
elsebranch that exists to log the failure and keep theprevious configuration.
The parsing moves into
parseYamlFile()andyamlParser()becomes theexception boundary, so a bad config is rejected and the running one kept.
It also checks for the
snikey before reading it.sni["sni"].IsSequence()throws
YAML::InvalidNodeon the const node, so a selector entry without ansnikey never reached the "selector node is not a map or without a name"error that is already there for it.
The
percentagedocumentation gave the default as0.9, but the parser readsan integer and the default is
90. The documented value is one of the onesthat throws, so the docs are corrected as well.
Testing
New autest
rate_limit_yaml_reloadcovers both shapes: a selector entry with nosnikey, and a fractionalpercentage.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 checkreturns
000, the JSONRPC socket refuses connections, anddiags.logstopsmid-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