moonchen commented on code in PR #13657:
URL: https://github.com/apache/trafficserver/pull/13657#discussion_r3972318047
##########
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:
Fixed - the sentence now names the ``perma-block`` key rather than
"permanent-block".
##########
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:
Added - the diagnostic now reports `config[key].Mark().line + 1`.
##########
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:
Not applicable here: the AuTest `Pipfile` pins `python_version = "3.11"`,
and gold tests already use PEP 604 annotations (for example
`tests/gold_tests/tls/tls_sni_ticket.test.py`). Leaving as is.
##########
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:
Done - `utilities.h` now forward-declares `YAML::Node` and `utilities.cc`
includes yaml-cpp.
##########
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:
Fixed both - the producer message in `sni_selector.cc` now reads
"Successfully", and the test expectation matches.
--
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]
