bneradt commented on code in PR #13934:
URL: https://github.com/apache/trafficserver/pull/13934#discussion_r4418357825
##########
plugins/header_rewrite/parser.cc:
##########
@@ -188,8 +188,8 @@ Parser::preprocess(std::vector<std::string> tokens)
while (getline(iss, t, ',')) {
if (std::find(_mods.begin(), _mods.end(), t) != _mods.end()) {
- // This produces an error, but it's not fatal for load / reload.
ToDo: ATS v11 fix.
TSError("[%s] Duplicate modifier: %s", PLUGIN_NAME, t.c_str());
+ return false;
Review Comment:
[P1] Propagate duplicate-modifier errors out of parse_config
Returning false here does not reject the configuration:
RulesConfig::parse_config() handles a false parse_line() result at lines
255-258 by logging and continuing. For example, `cond %{CLIENT-HEADER:foo} =bar
[AND,AND]` followed by `set-header X-Test yes` now drops the condition and
loads the operator unconditionally. Previously the duplicate was logged while
the condition was retained. Please make this parse failure abort the
configuration load, and cover the initializer result and retained rules rather
than only checking Parser::parse_line().
##########
plugins/regex_remap/regex_remap.cc:
##########
@@ -655,47 +656,45 @@ compile_rule_set(std::string const &filename, std::string
const &source)
}
pos1 = line.find_first_not_of(" \t\n");
- if (pos1 != std::string::npos) {
- if (line[pos1] == '#') {
- continue;
- }
+ if (pos1 == std::string::npos || line[pos1] == '#') {
+ continue;
Review Comment:
[P2] Include carriage returns when skipping blank lines
The whitespace set still omits `\r`. std::getline() leaves a CRLF blank line
as `"\r"`, so pos1 is 0, this check does not skip it, and the new fatal
missing-regexp path rejects the whole file. Before this PR that line was logged
and skipped. A file containing `^/ http://example.test/ @status=302\r\n\r\n`
demonstrates the regression, while the LF equivalent loads. Please recognize
carriage returns as whitespace (or normalize CRLF) and add coverage for a CRLF
file with blank lines.
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -630,10 +629,12 @@ TSPluginInit(int argc, const char *argv[])
got_config = true;
} else {
TSError("[%s] failed to parse configuration file %s", PLUGIN_NAME,
argv[i]);
+ parse_success = false;
+ break;
}
}
- if (got_config) {
+ if (got_config && parse_success) {
Review Comment:
[P1] Make a failed global configuration abort startup
When any global file fails, this condition falls through to TSError()/delete
conf and TSPluginInit() returns normally. TSPluginRegister() already succeeded,
so the core plugin loader still considers initialization successful and ATS
starts with every global header_rewrite rule disabled. With multiple files, a
malformed second file now also disables the valid first file. That does not
implement the startup-abort behavior described in this PR. Please terminate
initialization with TSFatal (or another supported fatal startup path) when
parse_success is false, and verify that ATS startup/config validation fails.
--
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]
