rate_limit: catch YAML errors instead of dying on reload

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
 

plugins/experimental/rate_limit/sni_selector.cc

@@ -27,22 +27,29 @@ std::atomic<SniSelector *> 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<std::string>();
+      if (sni.IsMap() && sni_node && !sni_node.IsSequence()) {
+        auto name = sni_node.as<std::string>();
 
         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;

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

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")