Impact
Test-only defect, no production impact. Introduced by #13146, now on master.
Test G of config_reload_plugin_api.test.py is the only test asserting that a core
reload task does not carry the [plugin: <name>] attribution tag that
traffic_ctl config status adds for plugin-owned tasks. ExcludesExpression compiles its
first argument as a regex, and the unescaped brackets in 'ip_allow [plugin]' make it a
character class rather than a literal. The pattern cannot match the command's output in
the correct case or in the regressed case, so the assertion passes unconditionally.
If core reload tasks ever started being reported as plugin-owned, this test would still go
green and the regression would ship undetected. Nothing else covers that behaviour.
Version: master @ 83335d4eff9694061409d889c73627a847f46d24
Platform: test-only; not platform specific
Config: none
Proof
ExcludesExpression compiles its first argument as a regex and applies re.search per
line, and IncludesExpression is an alias of ContainsExpression, so all three matchers
behave this way:
# autest/testers/__init__.py:16
from .contains_expression import ContainsExpression as IncludesExpression
# autest/testers/excludes_expression.py:38-42
if isinstance(regexp, str):
regexp = re.compile(regexp, reflags)
The assertion, at
|
tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression( |
|
'ip_allow [plugin]', 'Core task ip_allow must not have [plugin] tag') |
tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
'ip_allow [plugin]', 'Core task ip_allow must not have [plugin] tag')
[plugin] is a character class matching one of p l u g i n, so the pattern is
ip_allow followed by one of those six characters. The tag it is meant to detect is
built at
|
// Build label and right-aligned duration |
|
std::string label = std::string(status_icon(f.status)) + " " + fname; |
|
if (!f.meta.plugin_name.empty()) { |
|
label += " [plugin: " + f.meta.plugin_name + "]"; |
|
} |
|
label = sanitize_label(label); |
// Build label and right-aligned duration
std::string label = std::string(status_icon(f.status)) + " " + fname;
if (!f.meta.plugin_name.empty()) {
label += " [plugin: " + f.meta.plugin_name + "]";
}
so the character following ip_allow is [, which is not in the class. Found by
inspection, then confirmed by running the exact pattern the test passes:
import re
pat = re.compile('ip_allow [plugin]') # exactly what the test passes
good = " * ip_allow ................ 12ms" # core task, no tag
bad = " * ip_allow [plugin: my_plugin] .... 12ms" # the regression it guards
for line in (good, bad):
print(bool(pat.search(line)))
Observed:
core task (want: no match) search=False ExcludesExpression -> PASS
wrongly tagged (want: MATCH) search=False ExcludesExpression -> PASS
Expected, the second case must fail. With r'ip_allow.*\[plugin:':
core task (want: no match) search=False ExcludesExpression -> PASS
wrongly tagged (want: MATCH) search=True ExcludesExpression -> FAIL
The two positive assertions in the same file are already written correctly as
r'\[plugin: ' (lines 130 and 167); this negative one was missed.
A sweep of all 502 files under tests/gold_tests on master found 12 other bracketed
matchers, all of them the intentional [Uu]sing HTTP/?2 in the h2 tests. This is the
only instance of the defect.
Proposed change
--- a/tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py
+++ b/tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py
@@ -268,8 +268,8 @@
tr.Processes.Default.Env = ts.Env
tr.Processes.Default.ReturnCode = 0
-tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
- 'ip_allow [plugin]', 'Core task ip_allow must not have [plugin] tag')
+tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
+ r'ip_allow.*\[plugin:', 'Core task ip_allow must not have a [plugin: <name>] tag')
tr.StillRunningAfter = ts
.* rather than a literal space, because the label goes through sanitize_label() and a
dot fill sits between the task name and the duration, so the tag is not adjacent to the
name at every column width. Matching the [plugin: prefix rather than a full
[plugin: <name>] keeps the assertion independent of the plugin name.
No production code changes, no compatibility impact, nothing to backport beyond wherever
#13146 lands.
Impact
Test-only defect, no production impact. Introduced by #13146, now on
master.Test G of
config_reload_plugin_api.test.pyis the only test asserting that a corereload task does not carry the
[plugin: <name>]attribution tag thattraffic_ctl config statusadds for plugin-owned tasks.ExcludesExpressioncompiles itsfirst argument as a regex, and the unescaped brackets in
'ip_allow [plugin]'make it acharacter class rather than a literal. The pattern cannot match the command's output in
the correct case or in the regressed case, so the assertion passes unconditionally.
If core reload tasks ever started being reported as plugin-owned, this test would still go
green and the regression would ship undetected. Nothing else covers that behaviour.
Proof
ExcludesExpressioncompiles its first argument as a regex and appliesre.searchperline, and
IncludesExpressionis an alias ofContainsExpression, so all three matchersbehave this way:
The assertion, at
trafficserver/tests/gold_tests/jsonrpc/config_reload_plugin_api.test.py
Lines 271 to 272 in 83335d4
[plugin]is a character class matching one ofp l u g i n, so the pattern isip_allowfollowed by one of those six characters. The tag it is meant to detect isbuilt at
trafficserver/src/traffic_ctl/CtrlPrinters.cc
Lines 345 to 350 in 83335d4
so the character following
ip_allowis[, which is not in the class. Found byinspection, then confirmed by running the exact pattern the test passes:
Observed:
Expected, the second case must fail. With
r'ip_allow.*\[plugin:':The two positive assertions in the same file are already written correctly as
r'\[plugin: '(lines 130 and 167); this negative one was missed.A sweep of all 502 files under
tests/gold_testsonmasterfound 12 other bracketedmatchers, all of them the intentional
[Uu]sing HTTP/?2in theh2tests. This is theonly instance of the defect.
Proposed change
.*rather than a literal space, because the label goes throughsanitize_label()and adot fill sits between the task name and the duration, so the tag is not adjacent to the
name at every column width. Matching the
[plugin:prefix rather than a full[plugin: <name>]keeps the assertion independent of the plugin name.No production code changes, no compatibility impact, nothing to backport beyond wherever
#13146 lands.