diff --git a/doc/admin-guide/plugins/rate_limit.en.rst b/doc/admin-guide/plugins/rate_limit.en.rst index c96f6a3684b..1f32ee510e1 100644 --- a/doc/admin-guide/plugins/rate_limit.en.rst +++ b/doc/admin-guide/plugins/rate_limit.en.rst @@ -269,7 +269,8 @@ and the following options: .. option:: percentage This is the minimum percentage of the ``limit`` that the pressure must be at, before - we start blocking IPs. The default is ``0.9`` which means ``90%`` of the limit. + we start blocking IPs. This is an integer percentage, and the default is ``90``, + which means ``90%`` of the limit. .. option:: max_age diff --git a/plugins/experimental/rate_limit/sni_selector.cc b/plugins/experimental/rate_limit/sni_selector.cc index 02fc08f15b9..11a82aefbbc 100644 --- a/plugins/experimental/rate_limit/sni_selector.cc +++ b/plugins/experimental/rate_limit/sni_selector.cc @@ -27,22 +27,29 @@ std::atomic SniSelector::_instance = nullptr; /////////////////////////////////////////////////////////////////////////////// // YAML parser for the global YAML configuration (via plugin.config) // +// This is the exception boundary for the configuration parsing. The node +// accessors and conversions in parseYamlFile() throw on malformed input, and a +// config reload runs this on an ET_TASK thread via ConfigUpdateCallback, so +// letting an exception escape here would terminate the server. +// bool SniSelector::yamlParser(const std::string &yaml_file) { - YAML::Node config; - try { - config = YAML::LoadFile(yaml_file); + return parseYamlFile(yaml_file); } catch (YAML::BadFile const &e) { - TSError("[%s] Cannot load configuration file: %s.", PLUGIN_NAME, e.what()); - return false; + TSError("[%s] Cannot load configuration file %s: %s.", PLUGIN_NAME, yaml_file.c_str(), e.what()); } catch (std::exception const &e) { - TSError("[%s] Unknown error while loading configuration file: %s.", PLUGIN_NAME, e.what()); - return false; + TSError("[%s] Failed to parse configuration file %s: %s.", PLUGIN_NAME, yaml_file.c_str(), e.what()); } - _yaml_file = yaml_file; + return false; +} + +bool +SniSelector::parseYamlFile(const std::string &yaml_file) +{ + YAML::Node config = YAML::LoadFile(yaml_file); // First build the Lists, if any const YAML::Node &lists = config["lists"]; @@ -111,10 +118,11 @@ SniSelector::yamlParser(const std::string &yaml_file) if (sel && sel.IsSequence()) { for (const auto &i : sel) { - const YAML::Node &sni = i; + const YAML::Node &sni = i; + const YAML::Node sni_node = sni.IsMap() ? sni["sni"] : YAML::Node{}; - if (sni.IsMap() && !sni["sni"].IsSequence()) { - auto name = sni["sni"].as(); + if (sni.IsMap() && sni_node && !sni_node.IsSequence()) { + auto name = sni_node.as(); if (nullptr != findLimiter(name)) { TSError("[%s] Duplicate SNIs being added (%s)", PLUGIN_NAME, name.c_str()); @@ -167,6 +175,7 @@ SniSelector::yamlParser(const std::string &yaml_file) } } + _yaml_file = yaml_file; Dbg(dbg_ctl, "Succesfully loaded YAML file: %s", yaml_file.c_str()); return true; diff --git a/plugins/experimental/rate_limit/sni_selector.h b/plugins/experimental/rate_limit/sni_selector.h index b25c913f62b..4113fae62e5 100644 --- a/plugins/experimental/rate_limit/sni_selector.h +++ b/plugins/experimental/rate_limit/sni_selector.h @@ -185,6 +185,8 @@ class SniSelector static void startup(const std::string &yaml_file); private: + bool parseYamlFile(const std::string &yaml_file); ///< Parses the file, may throw; call via yamlParser(). + std::string _yaml_file; bool _needs_queue_cont = false; TSCont _queue_cont = nullptr; // Continuation processing the queue periodically diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_reload.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_reload.test.py new file mode 100644 index 00000000000..aeac1e7b1ed --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_reload.test.py @@ -0,0 +1,122 @@ +''' +Test that a malformed rate_limit YAML file fails the reload instead of +terminating traffic_server. +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import shlex + +Test.Summary = ''' +rate_limit: a malformed YAML config fails the reload without killing ATS. +''' + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) +Test.ContinueOnFail = True + +server = Test.MakeOriginServer("server") +server.addResponse( + "sessionlog.json", { + "headers": "GET /health HTTP/1.1\r\nHost: reload.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + }, { + "headers": "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\n", + "timestamp": "1469733493.993", + "body": "OK" + }) + +ts = Test.MakeATSProcess("ts") + +rate_limit_yaml = os.path.join(ts.Variables.CONFIGDIR, 'rate_limit.yaml') +ts.Disk.File( + rate_limit_yaml, typename="ats:config").AddLines([ + 'selector:', + ' - sni: reload.example.com', + ' limit: 100', + '', + ]) + +# A selector entry with no "sni" key. Reading sni["sni"] on the const node +# throws YAML::InvalidNode before the "without a name" check can report it. +missing_sni = os.path.join(Test.RunDirectory, 'missing_sni.yaml') +with open(missing_sni, 'w') as f: + f.write('selector:\n - limit: 100\n') + +# "percentage" is read as a uint32_t, so the fractional value that the +# documentation used to suggest throws YAML::TypedBadConversion. +bad_percentage = os.path.join(Test.RunDirectory, 'bad_percentage.yaml') +with open(bad_percentage, 'w') as f: + f.write('ip-rep:\n - name: test\n size: 15\n percentage: 0.9\n') + +# A selector entry that is a sequence rather than a map, which is what one extra +# level of indentation produces. Nothing here throws, so only the IsMap() check +# keeps it from being accepted as a limiter named "null". +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') + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', +}) + +ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server.Variables.Port}/') +ts.Disk.plugin_config.AddLine(f'rate_limit.so {rate_limit_yaml}') + +BASE_URL = f"http://127.0.0.1:{ts.Variables.port}/health" +CURL = f"curl -s -o /dev/null -w '%{{http_code}}' -H 'Host: reload.example.com' '{BASE_URL}'" + +tr = Test.AddTestRun("Start with a valid config") +tr.Processes.Default.StartBefore(server, ready=When.PortOpen(server.Variables.Port)) +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = CURL +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = Testers.ContainsExpression("200", "ATS should serve the request") +tr.StillRunningAfter = ts + +# The plugin callback only fires when the file mtime advances, so sleep before +# each overwrite to make sure it does. +for description, bad_config in [("selector entry without an sni key", missing_sni), ("a fractional percentage", bad_percentage), + ("a selector entry that is not a map", non_map)]: + tr = Test.AddTestRun(f"Install {description}") + tr.Processes.Default.Command = f"sleep 2 && cp {shlex.quote(bad_config)} {shlex.quote(rate_limit_yaml)}" + tr.Processes.Default.ReturnCode = 0 + tr.StillRunningAfter = ts + + Test.AddConfigReload(ts, delay_start=1, description=f"Reload with {description}") + + tr = Test.AddTestRun(f"ATS survives {description}") + tr.Processes.Default.Command = f"sleep 2 && {CURL}" + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression("200", "ATS should still be serving traffic") + tr.StillRunningAfter = ts + +# Every case has to be reported and rejected. Assigning here replaces the +# default "diags.log has no ERROR:" testers, since these errors are expected. +ts.Disk.diags_log.Content = Testers.ContainsExpression( + "selector node is not a map or without a name", "The malformed selector entries should be reported, not thrown") +ts.Disk.diags_log.Content += Testers.ContainsExpression( + "Failed to parse configuration file", "The bad percentage should be caught by the parser") +ts.Disk.diags_log.Content += Testers.ContainsExpression("Failed to reload YAML file", "The reloads should be rejected") + +# sni_config_cont() logs this only after a parse succeeds, right before it swaps +# the new selector in. None of the three configs may get that far, so this is +# what separates "rejected, previous config kept" from "accepted and the limits +# silently dropped". The 200s above cannot tell those apart, because the origin +# answers either way. +ts.Disk.traffic_out.Content = Testers.ExcludesExpression("Reloading YAML file", "No malformed config should be swapped in")