Adds the %{GROUP} condition to HRW, and else clause (#12009)

* Adds the %{GROUP} condition to HRW

This allows for grouping of conditions, like ()'s, and as such,
better control for logical AND and OR's.

* Adds an else clause to HRW conditions

doc/admin-guide/plugins/header_rewrite.en.rst

@@ -122,11 +122,24 @@ like the following::
 
     cond %{STATUS} >399 [AND]
     cond %{STATUS} <500
-    set-status 404
+      set-status 404
 
 Which converts any 4xx HTTP status code from the origin server to a 404. A
 response from the origin with a status of 200 would be unaffected by this rule.
 
+An optional ``else`` clause may be specified, which will be executed if the
+conditions are not met. The ``else`` clause is specified by starting a new line
+with the word ``else``. The following example illustrates this::
+
+    cond %{STATUS} >399 [AND]
+    cond %{STATUS} <500
+      set-status 404
+    else
+      set-status 503
+
+The ``else`` clause is not a condition, and does not take any flags, it is
+of course optional, but when specified must be followed by at least one operator.
+
 Conditions
 ----------
 
@@ -297,6 +310,56 @@ setting headers. For example::
         set-header ATS-Geo-ASN %{GEO:ASN}
         set-header ATS-Geo-ASN-NAME %{GEO:ASN-NAME}
 
+GROUP
+~~~~~
+;;
+    cond %{GROUP}
+    cond %{GROUP:END}
+
+This condition is a pseudo condition that is used to group conditions together.
+Using these groups, you can construct more complex expressions, that can mix and
+match AND, OR and NOT operators. These groups are the equivalent of parenthesis
+in expressions.The following pseudo example illustrates this. Lets say you want
+to express::
+
+      (A and B) or (C and (D or E))
+
+Assuming A, B, C, D and E are all valid conditions, you would write this as::
+
+    cond %{GROUP} [OR]
+        cond A [AND]
+        cond B
+    cond %{GROUP:END}
+    cond %{GROUP]
+       cond C [AND]
+       cond %{GROUP}
+             cond D [OR]
+             cond E
+       cond %{GROUP:END}
+    cond %{GROUP:END}
+
+Here's a more realistic example, abeit constructed, showing how to use the
+groups to construct a complex expression with real header value comparisons::
+
+    cond %{SEND_REQUEST_HDR_HOOK} [AND]
+    cond %{GROUP} [OR]
+        cond %{CLIENT-HEADER:X-Bar} /foo/ [AND]
+        cond %{CLIENT-HEADER:User-Agent} /Chrome/
+    cond %{GROUP:END}
+    cond %{GROUP}
+        cond %{CLIENT-HEADER:X-Bar} /fie/ [AND]
+        cond %{CLIENT-HEADER:User-Agent} /MSIE/
+    cond %{GROUP:END}
+        set-header X-My-Header "This is a test"
+
+Note that the ``GROUP`` and ``GROUP:END`` conditions do not take any operands per se,
+and you are still limited to operations after the last condition. Also, the ``GROUP:END``
+condition must match exactly with the last ``GROUP`` conditions, and they can be
+nested in one or several levels.
+
+When closing a group with ``GROUP::END``, the modifiers are not used, in fact that entire
+condition is discarded, being used only to close the group. You may still decorate it
+with the same modifier as the opening ``GROUP`` condition, but it is not necessary.
 
 HEADER
 ~~~~~~

plugins/header_rewrite/conditions.h

@@ -659,3 +659,69 @@ class ConditionHttpCntl : public Condition
 private:
   TSHttpCntlType _http_cntl_qual = TS_HTTP_CNTL_LOGGING_MODE;
 };
+
+class ConditionGroup : public Condition
+{
+public:
+  ConditionGroup() { Dbg(dbg_ctl, "Calling CTOR for ConditionGroup"); }
+
+  ~ConditionGroup() override
+  {
+    Dbg(dbg_ctl, "Calling DTOR for ConditionGroup");
+    delete _cond;
+  }
+
+  void
+  set_qualifier(const std::string &q) override
+  {
+    Condition::set_qualifier(q);
+
+    if (!q.empty()) { // Anything goes here, but prefer END
+      _end = true;
+    }
+  }
+
+  // noncopyable
+  ConditionGroup(const ConditionGroup &) = delete;
+  void operator=(const ConditionGroup &) = delete;
+
+  bool
+  closes() const
+  {
+    return _end;
+  }
+
+  void
+  append_value(std::string & /* s ATS_UNUSED */, const Resources & /* res ATS_UNUSED */) override
+  {
+    TSAssert(!"%{GROUP} should never be used as a condition value!");
+    TSError("[%s] %%{GROUP} should never be used as a condition value!", PLUGIN_NAME);
+  }
+
+  void
+  add_condition(Condition *cond)
+  {
+    if (_cond) {
+      _cond->append(cond);
+    } else {
+      _cond = cond;
+    }
+  }
+
+  // This can't be protected, because we actually evaluate this condition directly from the Ruleset
+  bool
+  eval(const Resources &res) override
+  {
+    Dbg(pi_dbg_ctl, "Evaluating GROUP()");
+
+    if (_cond) {
+      return _cond->do_eval(res);
+    } else {
+      return true;
+    }
+  }
+
+private:
+  Condition *_cond = nullptr; // First pre-condition (linked list)
+  bool       _end  = false;
+};

plugins/header_rewrite/factory.cc

@@ -162,8 +162,10 @@ condition_factory(const std::string &cond)
     c = new ConditionCache();
   } else if (c_name == "NEXT-HOP") { // This condition adapts to the hook
     c = new ConditionNextHop();
-  } else if (c_name == "HTTP-CNTL") { // This condition adapts to the hook
+  } else if (c_name == "HTTP-CNTL") {
     c = new ConditionHttpCntl();
+  } else if (c_name == "GROUP") {
+    c = new ConditionGroup();
   } else {
     TSError("[%s] Unknown condition %s", PLUGIN_NAME, c_name.c_str());
     return nullptr;

plugins/header_rewrite/header_rewrite.cc

@@ -19,6 +19,7 @@
 #include <fstream>
 #include <mutex>
 #include <string>
+#include <stack>
 #include <stdexcept>
 #include <getopt.h>
 
@@ -145,10 +146,12 @@ RulesConfig::add_rule(RuleSet *rule)
 bool
 RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, char *from_url, char *to_url)
 {
-  std::unique_ptr<RuleSet> rule(nullptr);
-  std::string              filename;
-  std::ifstream            f;
-  int                      lineno = 0;
+  std::unique_ptr<RuleSet>     rule(nullptr);
+  std::string                  filename;
+  std::ifstream                f;
+  int                          lineno = 0;
+  std::stack<ConditionGroup *> group_stack;
+  ConditionGroup              *group = nullptr;
 
   if (0 == fname.size()) {
     TSError("[%s] no config filename provided", PLUGIN_NAME);
@@ -168,11 +171,12 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
     return false;
   }
 
+  Dbg(dbg_ctl, "Parsing started on file: %s", filename.c_str());
   while (!f.eof()) {
     std::string line;
 
     getline(f, line);
-    ++lineno; // ToDo: we should probably use this for error messages ...
+    ++lineno;
     Dbg(dbg_ctl, "Reading line: %d: %s", lineno, line.c_str());
 
     while (std::isspace(line[0])) {
@@ -191,7 +195,8 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
 
     // Tokenize and parse this line
     if (!p.parse_line(line)) {
-      Dbg(dbg_ctl, "Error parsing line '%s'", line.c_str());
+      TSError("[%s] Error parsing file '%s', line '%s', lineno: %d", PLUGIN_NAME, filename.c_str(), line.c_str(), lineno);
+      Dbg(dbg_ctl, "Error parsing line '%s', lineno: %d", line.c_str(), lineno);
       continue;
     }
 
@@ -208,14 +213,20 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
     bool         is_hook = p.cond_is_hook(hook); // This updates the hook if explicitly set, if not leaves at default
 
     if (nullptr == rule) {
+      if (!group_stack.empty()) {
+        TSError("[%s] mismatched %%{GROUP} conditions in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
+        return false;
+      }
+
       rule = std::make_unique<RuleSet>();
       rule->set_hook(hook);
+      group = rule->get_group(); // This the implicit rule group to begin with
 
       if (is_hook) {
         // Check if the hooks are not available for the remap mode
         if ((default_hook == TS_REMAP_PSEUDO_HOOK) &&
             ((TS_HTTP_READ_REQUEST_HDR_HOOK == hook) || (TS_HTTP_PRE_REMAP_HOOK == hook))) {
-          TSError("[%s] you can not use cond %%{%s} in a remap rule", PLUGIN_NAME, p.get_op().c_str());
+          TSError("[%s] you can not use cond %%{%s} in a remap rule, lineno: %d", PLUGIN_NAME, p.get_op().c_str(), lineno);
           return false;
         }
         continue;
@@ -233,20 +244,54 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c
     // Long term, maybe we need to percolate all this up through add_condition() / add_operator() rather than this big ugly try.
     try {
       if (p.is_cond()) {
-        if (!rule->add_condition(p, filename.c_str(), lineno)) {
+        Condition *cond = rule->make_condition(p, filename.c_str(), lineno);
+
+        if (!cond) {
           throw std::runtime_error("add_condition() failed");
+        } else {
+          ConditionGroup *ngrp = dynamic_cast<ConditionGroup *>(cond);
+
+          if (ngrp) {
+            if (ngrp->closes()) {
+              // Closing a group, pop the stack
+              if (group_stack.empty()) {
+                throw std::runtime_error("unmatched %{GROUP}");
+              } else {
+                delete cond; // We don't care about the closing group condition, it's a no-op
+                ngrp  = group;
+                group = group_stack.top();
+                group_stack.pop();
+                group->add_condition(ngrp); // Add the previous group to the current group's conditions
+              }
+            } else {
+              // New group
+              group_stack.push(group);
+              group = ngrp;
+            }
+          } else {
+            group->add_condition(cond);
+          }
         }
-      } else {
+      } else if (p.is_else()) {
+        // Switch to the else portion of operators
+        rule->switch_branch();
+      } else { // Operator
         if (!rule->add_operator(p, filename.c_str(), lineno)) {
           throw std::runtime_error("add_operator() failed");
         }
       }
     } catch (std::runtime_error &e) {
-      TSError("[%s] header_rewrite configuration exception: %s in file: %s", PLUGIN_NAME, e.what(), fname.c_str());
+      TSError("[%s] header_rewrite configuration exception: %s in file: %s, lineno: %d", PLUGIN_NAME, e.what(), fname.c_str(),
+              lineno);
       return false;
     }
   }
 
+  if (!group_stack.empty()) {
+    TSError("[%s] missing final %%{GROUP:END} condition in file: %s, lineno: %d", PLUGIN_NAME, fname.c_str(), lineno);
+    return false;
+  }
+
   // Add the last rule (possibly the only rule)
   if (add_rule(rule.get())) {
     rule.release();
@@ -301,25 +346,25 @@ cont_rewrite_headers(TSCont contp, TSEvent event, void *edata)
   }
 
   bool reenable{true};
+
   if (hook != TS_HTTP_LAST_HOOK) {
-    const RuleSet *rule = conf->rule(hook);
-    Resources      res(txnp, contp);
+    RuleSet  *rule = conf->rule(hook);
+    Resources res(txnp, contp);
 
     // Get the resources necessary to process this event
     res.gather(conf->resid(hook), hook);
 
     // Evaluation of all rules. This code is sort of duplicate in DoRemap as well.
     while (rule) {
-      if (rule->eval(res)) {
-        OperModifiers rt = rule->exec(res);
+      const RuleSet::OperatorPair &ops = rule->eval(res);
+      const OperModifiers          rt  = rule->exec(ops, res);
 
-        if (rt & OPER_NO_REENABLE) {
-          reenable = false;
-        }
+      if (rt & OPER_NO_REENABLE) {
+        reenable = false;
+      }
 
-        if (rule->last() || (rt & OPER_LAST)) {
-          break; // Conditional break, force a break with [L]
-        }
+      if (rule->last() || (rt & OPER_LAST)) {
+        break; // Conditional break, force a break with [L]
       }
       rule = rule->next;
     }
@@ -328,6 +373,7 @@ cont_rewrite_headers(TSCont contp, TSEvent event, void *edata)
   if (reenable) {
     TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE);
   }
+
   return 0;
 }
 
@@ -528,19 +574,19 @@ TSRemapDoRemap(void *ih, TSHttpTxn rh, TSRemapRequestInfo *rri)
 
   res.gather(RSRC_CLIENT_REQUEST_HEADERS, TS_REMAP_PSEUDO_HOOK);
   while (rule) {
-    if (rule->eval(res)) {
-      OperModifiers rt = rule->exec(res);
+    const RuleSet::OperatorPair &ops = rule->eval(res);
+    const OperModifiers          rt  = rule->exec(ops, res);
 
-      ink_assert((rt & OPER_NO_REENABLE) == 0);
+    ink_assert((rt & OPER_NO_REENABLE) == 0);
 
-      if (res.changed_url == true) {
-        rval = TSREMAP_DID_REMAP;
-      }
+    if (res.changed_url == true) {
+      rval = TSREMAP_DID_REMAP;
+    }
 
-      if (rule->last() || (rt & OPER_LAST)) {
-        break; // Conditional break, force a break with [L]
-      }
+    if (rule->last() || (rt & OPER_LAST)) {
+      break; // Conditional break, force a break with [L]
     }
+
     rule = rule->next;
   }
 

plugins/header_rewrite/parser.cc

@@ -173,6 +173,11 @@ Parser::preprocess(std::vector<std::string> tokens)
   } else if (tokens[0] == "cond") {
     _cond = true;
     tokens.erase(tokens.begin());
+  } else if (tokens[0] == "else") {
+    _cond = false;
+    _else = true;
+
+    return true;
   }
 
   // Is it a condition or operator?

plugins/header_rewrite/parser.h

@@ -65,6 +65,12 @@ class Parser
     return _cond;
   }
 
+  bool
+  is_else() const
+  {
+    return _else;
+  }
+
   const std::string &
   get_op() const
   {
@@ -110,6 +116,7 @@ class Parser
   bool preprocess(std::vector<std::string> tokens);
 
   bool                     _cond     = false;
+  bool                     _else     = false;
   bool                     _empty    = false;
   char                    *_from_url = nullptr;
   char                    *_to_url   = nullptr;