You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "moonchen (via GitHub)" <gi...@apache.org> on 2026/09/09 15:19:24 UTC
[PR] rate_limit: reject unknown YAML configuration keys [trafficserver]
moonchen opened a new pull request, #13657:
URL: https://github.com/apache/trafficserver/pull/13657
The rate_limit YAML parser silently ignored unknown keys, so a typo such as `max-age` left aging disabled. Validate each configuration mapping against its accepted keys and reject unknown keys with a diagnostic naming the key, node, and line. Propagate queue and metrics parsing failures so they prevent configuration loading.
Adds 13 AuTest scenarios covering valid settings, unknown keys at every mapping level, and malformed nodes, and documents the configuration compatibility change.
Fixes: #13529
Validation:
- Built the `rate_limit` plugin.
- Passed `rate_limit_yaml_keys`, `rate_limit_iprep`, and `rate_limit_sni_expiry` AuTests.
- Passed pre-commit formatting checks and `git diff --check`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] rate_limit: reject unknown YAML configuration keys [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #13657:
URL: https://github.com/apache/trafficserver/pull/13657#discussion_r4009881089
##########
plugins/prefetch/benchmark_evaluate:
##########
Review Comment:
looks like this was added accidentally
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] rate_limit: reject unknown YAML configuration keys [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #13657:
URL: https://github.com/apache/trafficserver/pull/13657#discussion_r4353729693
##########
plugins/experimental/rate_limit/limiter.h:
##########
@@ -202,7 +202,7 @@ template <class T> class RateLimiter
}
if (node["rate"]) {
- _limit = node["rate"].as<uint32_t>();
+ _rate = node["rate"].as<uint32_t>();
Review Comment:
This fixes a real bug from #11271 (in 10.1.0 onward): `rate` used to overwrite `_limit`, and no bucket was created. But it changes what existing configs do — the documented example (`limit: 1000`, `rate: 200`) currently behaves as a 200 active-transaction cap with no rate limit, and after this it becomes a 1000 cap plus a 200/s new-session limit. Could you add that to the upgrading notes (and mention it in the description)? It might also be worth its own PR so it can be considered for 10.2.x independently of the strict-key change.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] rate_limit: reject unknown YAML configuration keys [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] rate_limit: reject unknown YAML configuration keys [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org