brbzull0 commented on code in PR #13600: URL: https://github.com/apache/trafficserver/pull/13600#discussion_r3895421014
########## tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_reload.test.py: ########## @@ -0,0 +1,108 @@ +''' +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') + +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)]: + 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 + +# Both cases have 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( Review Comment: 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: ```python ts.Disk.traffic_out.Content = Testers.ExcludesExpression( "Reloading YAML file", "No malformed config should ever be swapped in") ``` ########## plugins/experimental/rate_limit/sni_selector.cc: ########## @@ -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_node && !sni_node.IsSequence()) { Review Comment: 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: ```yaml 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: ```suggestion 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: ```python 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') ``` ########## 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 +// this runs on the management update continuation during a config reload, 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); Review Comment: 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. ########## 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 +// this runs on the management update continuation during a config reload, so Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
