Produce warnings when bad mods are used (#12749)

Author: zwoop

plugins/header_rewrite/condition.cc

@@ -80,43 +80,43 @@ Condition::initialize(Parser &p)
 {
   Statement::initialize(p);
 
-  if (p.mod_exist("OR")) {
-    if (p.mod_exist("AND")) {
+  if (p.consume_mod("OR")) {
+    if (p.consume_mod("AND")) {
       TSError("[%s] Can't have both AND and OR in mods", PLUGIN_NAME);
     } else {
       _mods |= CondModifiers::OR;
     }
-  } else if (p.mod_exist("AND")) {
+  } else if (p.consume_mod("AND")) {
     _mods |= CondModifiers::AND;
   }
 
-  if (p.mod_exist("NOT")) {
+  if (p.consume_mod("NOT")) {
     _mods |= CondModifiers::NOT;
   }
 
   // The NOCASE / CASE modifier is a bit special, since it ripples down into the Matchers for
   // strings and regexes.
   int _substr_seen = 0;
 
-  if (p.mod_exist("NOCASE")) {
+  if (p.consume_mod("NOCASE")) {
     _mods |= CondModifiers::MOD_NOCASE;
-  } else if (p.mod_exist("CASE")) {
+  } else if (p.consume_mod("CASE")) {
     // Nothing to do — default is case-sensitive, but still allow this string for clearness.
   }
 
-  if (p.mod_exist("EXT")) {
+  if (p.consume_mod("EXT")) {
     _mods |= CondModifiers::MOD_EXT;
     _substr_seen++;
   }
-  if (p.mod_exist("SUF")) {
+  if (p.consume_mod("SUF")) {
     _mods |= CondModifiers::MOD_SUF;
     _substr_seen++;
   }
-  if (p.mod_exist("PRE")) {
+  if (p.consume_mod("PRE")) {
     _mods |= CondModifiers::MOD_PRE;
     _substr_seen++;
   }
-  if (p.mod_exist("MID")) {
+  if (p.consume_mod("MID")) {
     _mods |= CondModifiers::MOD_MID;
     _substr_seen++;
   }
@@ -125,10 +125,10 @@ Condition::initialize(Parser &p)
     throw std::runtime_error("Only one substring modifier (EXT, SUF, PRE, MID) may be used.");
   }
 
-  // Deal with the "last" modifier as well.
-  if (p.mod_exist("L")) {
+  if (p.consume_mod("L")) {
     _mods |= CondModifiers::MOD_L;
   }
 
   _cond_op = parse_matcher_op(p.get_arg());
+  p.validate_mods();
 }

plugins/header_rewrite/operator.cc

@@ -38,17 +38,19 @@ Operator::initialize(Parser &p)
 {
   Statement::initialize(p);
 
-  if (p.mod_exist("L") || p.mod_exist("LAST")) {
+  if (p.consume_mod("L") || p.consume_mod("LAST")) {
     _mods = static_cast<OperModifiers>(_mods | OPER_LAST);
   }
 
-  if (p.mod_exist("QSA")) {
+  if (p.consume_mod("QSA")) {
     _mods = static_cast<OperModifiers>(_mods | OPER_QSA);
   }
 
-  if (p.mod_exist("I") || p.mod_exist("INV")) {
+  if (p.consume_mod("I") || p.consume_mod("INV")) {
     _mods = static_cast<OperModifiers>(_mods | OPER_INV);
   }
+
+  p.validate_mods();
 }
 
 void

plugins/header_rewrite/parser.cc

@@ -181,7 +181,12 @@ Parser::preprocess(std::vector<std::string> tokens)
           std::string        t;
 
           while (getline(iss, t, ',')) {
-            _mods.push_back(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());
+            } else {
+              _mods.push_back(t);
+            }
           }
         } else {
           _mods.push_back(m);
@@ -303,6 +308,26 @@ Parser::cond_is_hook(TSHttpHookID &hook) const
   return false;
 }
 
+// This is a TSError() here, but where this is called from does not treat "false" as a
+// load failure. This is a systemic problem in a some of the parsing. ToDo: ATS v11 fix.
+bool
+Parser::validate_mods() const
+{
+  if (_mods.empty()) {
+    return true;
+  }
+
+  auto        it   = _mods.begin();
+  std::string list = *it++;
+
+  for (; it != _mods.end(); ++it) {
+    list += ", ";
+    list += *it;
+  }
+  TSError("[%s] Unknown modifier(s): [%s]", PLUGIN_NAME, list.c_str());
+  return false;
+}
+
 HRWSimpleTokenizer::HRWSimpleTokenizer(const std::string &line)
 {
   ParserState state            = PARSER_DEFAULT;

plugins/header_rewrite/parser.h

@@ -195,12 +195,22 @@ class Parser
     return _val;
   }
 
+  // Check if the modifier exists and consume it from the list.
   bool
-  mod_exist(const std::string &m) const
+  consume_mod(const std::string &m)
   {
-    return std::find(_mods.begin(), _mods.end(), m) != _mods.end();
+    auto it = std::find(_mods.begin(), _mods.end(), m);
+
+    if (it != _mods.end()) {
+      _mods.erase(it);
+      return true;
+    }
+    return false;
   }
 
+  // Validate that all modifiers were consumed; logs error and returns false if not
+  bool validate_mods() const;
+
   bool cond_is_hook(TSHttpHookID &hook) const;
 
   const std::vector<std::string> &