Copilot commented on code in PR #13657: URL: https://github.com/apache/trafficserver/pull/13657#discussion_r3970302382
########## doc/admin-guide/plugins/rate_limit.en.rst: ########## @@ -142,7 +142,10 @@ configuration file. The basic use is as:: The YAML configuration can have the following format, where the various sections -and nodes are documented below. +and nodes are documented below. Unknown keys at any level cause configuration +loading to fail, with a diagnostic identifying the key, node, and line number. +A failed reload keeps the previous configuration active. Use ``max_age`` (with +an underscore) for queue, IP reputation, and permanent-block aging settings. Review Comment: The docs refer to 'permanent-block' aging settings, but the configuration key used elsewhere is `perma-block`. Consider updating this wording to `perma-block` (or explicitly mention both the feature name and the YAML key) to avoid confusion when users search for the correct YAML field names. ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py: ########## @@ -0,0 +1,123 @@ +# 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 copy +import yaml + +Test.Summary = 'rate_limit rejects unknown YAML keys at every configuration level.' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class TestYamlKeys: + """Exercise configuration loading without sending traffic.""" + + def __init__(self) -> None: + config = { + 'lists': [{ + 'name': 'local', + 'cidr': ['127.0.0.1/32'] + }], + 'ip-rep': + [ + { + 'name': 'reputation', + 'buckets': 2, + 'size': 4, + 'percentage': 90, + 'max_age': 300, + 'perma-block': { + 'limit': 100, + 'threshold': 1, + 'max_age': 1800 + }, + } + ], + 'selector': + [ + { + 'sni': 'test.example.com', + 'aliases': ['alias.example.com'], + 'limit': 10, + 'rate': 0, + 'queue': { + 'size': 5, + 'max_age': 30 + }, + 'metrics': { + 'prefix': 'plugin.rate_limit', + 'tag': 'valid' + }, + 'ip-rep': 'reputation', + 'exclude': 'local', + } + ], + } + self._configure('valid', config) + for name, path, key, context in [ + ('root', (), 'selecter', 'configuration'), + ('list', ('lists', 0), 'cidrs', 'lists'), + ('selector', ('selector', 0), 'limti', 'selector'), + ('queue', ('selector', 0, 'queue'), 'max-age', 'queue'), + ('metrics', ('selector', 0, 'metrics'), 'prefxi', 'metrics'), + ('iprep', ('ip-rep', 0), 'max-age', 'ip-rep'), + ('perma', ('ip-rep', 0, 'perma-block'), 'max-age', 'perma-block'), + ]: + invalid = copy.deepcopy(config) + node = invalid + for part in path: + node = node[part] + node[key] = 1 + self._configure(name, invalid, f"Unknown key '{key}' in {context} node at line [0-9]+") + for name, config, error in [ + ('misspelled-sni', {'selector': [{'sin': 'test'}]}, "Unknown key 'sin' in selector node at line [0-9]+"), + ('bad-queue', {'selector': [{'sni': 'test', 'queue': []}]}, 'The queue node must be a map'), + ('bad-metrics', {'selector': [{'sni': 'test', 'metrics': []}]}, 'The metrics node must be a map'), + ('bad-selector', {'selector': {'sni': 'test'}}, 'The selector node must be a sequence'), + ('non-scalar-key', {'selector': [{'sni': 'test', 'queue': {('bad', 'key'): 1}}]}, + 'The queue node has a non-scalar key at line [0-9]+'), + ]: + self._configure(name, config, error) + + @staticmethod + def _configure(name: str, config: dict, error: str | None = None) -> None: Review Comment: Using PEP 604 unions (`str | None`) requires Python 3.10+. If AuTest environments run an older Python, this will be a syntax error at import time. To avoid compatibility issues, use `Optional[str]` (and import `Optional`) or `Union[str, None]`. ########## plugins/experimental/rate_limit/utilities.h: ########## @@ -17,13 +17,19 @@ */ #pragma once -#include <string> #include <chrono> +#include <initializer_list> +#include <string> +#include <string_view> +#include <yaml-cpp/yaml.h> #include "ts/ts.h" Review Comment: Including `<yaml-cpp/yaml.h>` in a widely included header can increase compile times. This header only needs `YAML::Node` for a function declaration, so you can forward-declare `namespace YAML { class Node; }` here and move the yaml-cpp include into the corresponding `.cc` (or a narrow internal header). ########## tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py: ########## @@ -0,0 +1,123 @@ +# 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 copy +import yaml + +Test.Summary = 'rate_limit rejects unknown YAML keys at every configuration level.' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class TestYamlKeys: + """Exercise configuration loading without sending traffic.""" + + def __init__(self) -> None: + config = { + 'lists': [{ + 'name': 'local', + 'cidr': ['127.0.0.1/32'] + }], + 'ip-rep': + [ + { + 'name': 'reputation', + 'buckets': 2, + 'size': 4, + 'percentage': 90, + 'max_age': 300, + 'perma-block': { + 'limit': 100, + 'threshold': 1, + 'max_age': 1800 + }, + } + ], + 'selector': + [ + { + 'sni': 'test.example.com', + 'aliases': ['alias.example.com'], + 'limit': 10, + 'rate': 0, + 'queue': { + 'size': 5, + 'max_age': 30 + }, + 'metrics': { + 'prefix': 'plugin.rate_limit', + 'tag': 'valid' + }, + 'ip-rep': 'reputation', + 'exclude': 'local', + } + ], + } + self._configure('valid', config) + for name, path, key, context in [ + ('root', (), 'selecter', 'configuration'), + ('list', ('lists', 0), 'cidrs', 'lists'), + ('selector', ('selector', 0), 'limti', 'selector'), + ('queue', ('selector', 0, 'queue'), 'max-age', 'queue'), + ('metrics', ('selector', 0, 'metrics'), 'prefxi', 'metrics'), + ('iprep', ('ip-rep', 0), 'max-age', 'ip-rep'), + ('perma', ('ip-rep', 0, 'perma-block'), 'max-age', 'perma-block'), + ]: + invalid = copy.deepcopy(config) + node = invalid + for part in path: + node = node[part] + node[key] = 1 + self._configure(name, invalid, f"Unknown key '{key}' in {context} node at line [0-9]+") + for name, config, error in [ + ('misspelled-sni', {'selector': [{'sin': 'test'}]}, "Unknown key 'sin' in selector node at line [0-9]+"), + ('bad-queue', {'selector': [{'sni': 'test', 'queue': []}]}, 'The queue node must be a map'), + ('bad-metrics', {'selector': [{'sni': 'test', 'metrics': []}]}, 'The metrics node must be a map'), + ('bad-selector', {'selector': {'sni': 'test'}}, 'The selector node must be a sequence'), + ('non-scalar-key', {'selector': [{'sni': 'test', 'queue': {('bad', 'key'): 1}}]}, + 'The queue node has a non-scalar key at line [0-9]+'), + ]: + self._configure(name, config, error) + + @staticmethod + def _configure(name: str, config: dict, error: str | None = None) -> None: + ts = Test.MakeATSProcess(name, disable_log_checks=error is not None) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + ts.Disk.File( + f'{ts.Variables.CONFIGDIR}/rate_limit.yaml', typename='ats:config').AddLines(yaml.safe_dump(config).splitlines()) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.yaml') + tr = Test.AddTestRun(f'{name}: rate_limit YAML configuration') + tr.Processes.Default.Command = 'echo configuration checked' + tr.Processes.Default.ReturnCode = 0 + if error: + ts.ReturnCode = 70 # EX_SOFTWARE from TSFatal. + ts.Ready = 0 + ts.Disk.diags_log.Content = Testers.ContainsExpression(error, 'Report the invalid configuration') + ts.Disk.traffic_out.Content = Testers.ExcludesExpression( + 'Traffic Server is fully initialized', 'Invalid configuration prevents startup') + watcher = Test.Processes.Process(f'{name}-watcher') + watcher.Command = 'sleep 30' + watcher.Ready = When.FileContains(ts.Disk.diags_log.Name, 'Failed to parse YAML file') + watcher.StartBefore(ts) + tr.Processes.Default.StartBefore(watcher) + else: + ts.Disk.traffic_out.Content += Testers.ContainsExpression('Succesfully loaded YAML file', 'Accept all supported keys') Review Comment: Typo in the expected log message: 'Succesfully' should be 'Successfully'. If the actual runtime log line is also misspelled, consider fixing the producer message and updating this test accordingly so diagnostics/readability improve. ########## plugins/experimental/rate_limit/sni_selector.cc: ########## @@ -42,6 +42,17 @@ SniSelector::yamlParser(const std::string &yaml_file) return false; } + if (!validate_yaml_keys(config, "configuration", {"lists", "ip-rep", "selector"})) { + return false; + } + + for (const auto *key : {"lists", "ip-rep", "selector"}) { + if (config[key] && !config[key].IsSequence()) { + TSError("[%s] The %s node must be a sequence", PLUGIN_NAME, key); Review Comment: This new validation error omits location context. Since YAML-cpp provides marks, consider including the line number (e.g., `config[key].Mark().line + 1`) in the diagnostic to better match the PR’s goal of actionable configuration errors. -- 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]
