From 9a7488f061369dd2d563bb9b7517ec61d9e9b84e Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Wed, 5 Feb 2025 09:24:34 -0700 Subject: [PATCH] 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 | 65 ++++++++++- plugins/header_rewrite/conditions.h | 66 +++++++++++ plugins/header_rewrite/factory.cc | 4 +- plugins/header_rewrite/header_rewrite.cc | 104 +++++++++++++----- plugins/header_rewrite/parser.cc | 5 + plugins/header_rewrite/parser.h | 7 ++ plugins/header_rewrite/ruleset.cc | 73 ++++++------ plugins/header_rewrite/ruleset.h | 82 +++++++++----- 8 files changed, 310 insertions(+), 96 deletions(-) diff --git a/doc/admin-guide/plugins/header_rewrite.en.rst b/doc/admin-guide/plugins/header_rewrite.en.rst index 7fa890d869c..903acca7731 100644 --- a/doc/admin-guide/plugins/header_rewrite.en.rst +++ b/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 ~~~~~~ diff --git a/plugins/header_rewrite/conditions.h b/plugins/header_rewrite/conditions.h index 6568fbeba98..f49fcb3c046 100644 --- a/plugins/header_rewrite/conditions.h +++ b/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; +}; diff --git a/plugins/header_rewrite/factory.cc b/plugins/header_rewrite/factory.cc index 85136f0b043..7fe0290bdb8 100644 --- a/plugins/header_rewrite/factory.cc +++ b/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; diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc index ea6aec68528..7f651014f93 100644 --- a/plugins/header_rewrite/header_rewrite.cc +++ b/plugins/header_rewrite/header_rewrite.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -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 rule(nullptr); - std::string filename; - std::ifstream f; - int lineno = 0; + std::unique_ptr rule(nullptr); + std::string filename; + std::ifstream f; + int lineno = 0; + std::stack 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(); 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(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; } diff --git a/plugins/header_rewrite/parser.cc b/plugins/header_rewrite/parser.cc index 5be73a22bac..921ec2f83a7 100644 --- a/plugins/header_rewrite/parser.cc +++ b/plugins/header_rewrite/parser.cc @@ -173,6 +173,11 @@ Parser::preprocess(std::vector 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? diff --git a/plugins/header_rewrite/parser.h b/plugins/header_rewrite/parser.h index 25bfdd0a709..d621f079842 100644 --- a/plugins/header_rewrite/parser.h +++ b/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 tokens); bool _cond = false; + bool _else = false; bool _empty = false; char *_from_url = nullptr; char *_to_url = nullptr; diff --git a/plugins/header_rewrite/ruleset.cc b/plugins/header_rewrite/ruleset.cc index dca1139bbeb..a5d57099324 100644 --- a/plugins/header_rewrite/ruleset.cc +++ b/plugins/header_rewrite/ruleset.cc @@ -40,66 +40,67 @@ RuleSet::append(RuleSet *rule) tmp->next = rule; } -bool -RuleSet::add_condition(Parser &p, const char *filename, int lineno) +// This stays here, since the condition, albeit owned by a group, is tightly couple to the ruleset. +Condition * +RuleSet::make_condition(Parser &p, const char *filename, int lineno) { Condition *c = condition_factory(p.get_op()); - if (nullptr != c) { - Dbg(pi_dbg_ctl, " Adding condition: %%{%s} with arg: %s", p.get_op().c_str(), p.get_arg().c_str()); - c->initialize(p); - if (!c->set_hook(_hook)) { - delete c; - TSError("[%s] in %s:%d: can't use this condition in hook=%s: %%{%s} with arg: %s", PLUGIN_NAME, filename, lineno, - TSHttpHookNameLookup(_hook), p.get_op().c_str(), p.get_arg().c_str()); - return false; - } - if (c->get_cond_op() == MATCH_ERROR) { - delete c; - TSError("[%s] in %s:%d: Invalid operator", PLUGIN_NAME, filename, lineno); - return false; - } - if (nullptr == _cond) { - _cond = c; - } else { - _cond->append(c); - } + if (nullptr == c) { + return nullptr; // Complete failure in the factory + } - // Update some ruleset state based on this new condition - _last |= c->last(); - _ids = static_cast(_ids | _cond->get_resource_ids()); + Dbg(pi_dbg_ctl, " Adding condition: %%{%s} with arg: %s", p.get_op().c_str(), p.get_arg().c_str()); + c->initialize(p); + if (!c->set_hook(_hook)) { + delete c; + TSError("[%s] in %s:%d: can't use this condition in hook=%s: %%{%s} with arg: %s", PLUGIN_NAME, filename, lineno, + TSHttpHookNameLookup(_hook), p.get_op().c_str(), p.get_arg().c_str()); + return nullptr; + } - return true; + if (c->get_cond_op() == MATCH_ERROR) { + delete c; + TSError("[%s] in %s:%d: Invalid operator", PLUGIN_NAME, filename, lineno); + return nullptr; } - return false; + // Update some ruleset state based on this new condition; + _last |= c->last(); + _ids = static_cast(_ids | c->get_resource_ids()); + + return c; } bool RuleSet::add_operator(Parser &p, const char *filename, int lineno) { - Operator *o = operator_factory(p.get_op()); + Operator *op = operator_factory(p.get_op()); - if (nullptr != o) { + if (nullptr != op) { Dbg(pi_dbg_ctl, " Adding operator: %s(%s)=\"%s\"", p.get_op().c_str(), p.get_arg().c_str(), p.get_value().c_str()); - o->initialize(p); - if (!o->set_hook(_hook)) { - delete o; + op->initialize(p); + if (!op->set_hook(_hook)) { + delete op; Dbg(pi_dbg_ctl, "in %s:%d: can't use this operator in hook=%s: %s(%s)", filename, lineno, TSHttpHookNameLookup(_hook), p.get_op().c_str(), p.get_arg().c_str()); TSError("[%s] in %s:%d: can't use this operator in hook=%s: %s(%s)", PLUGIN_NAME, filename, lineno, TSHttpHookNameLookup(_hook), p.get_op().c_str(), p.get_arg().c_str()); return false; } - if (nullptr == _oper) { - _oper = o; + + // Work on the appropriate operator list + OperatorPair &ops = _is_else ? _operators[1] : _operators[0]; + + if (nullptr == ops.oper) { + ops.oper = op; } else { - _oper->append(o); + ops.oper->append(op); } // Update some ruleset state based on this new operator - _opermods = static_cast(_opermods | _oper->get_oper_modifiers()); - _ids = static_cast(_ids | _oper->get_resource_ids()); + ops.oper_mods = static_cast(ops.oper_mods | ops.oper->get_oper_modifiers()); + _ids = static_cast(_ids | ops.oper->get_resource_ids()); return true; } diff --git a/plugins/header_rewrite/ruleset.h b/plugins/header_rewrite/ruleset.h index d1b647e8c1c..db19b406cf8 100644 --- a/plugins/header_rewrite/ruleset.h +++ b/plugins/header_rewrite/ruleset.h @@ -29,6 +29,7 @@ #include "factory.h" #include "resources.h" #include "parser.h" +#include "conditions.h" /////////////////////////////////////////////////////////////////////////////// // Class holding one ruleset. A ruleset is one (or more) pre-conditions, and @@ -37,14 +38,25 @@ class RuleSet { public: + // Holding the IF and ELSE operators and mods, in two separate linked lists. + struct OperatorPair { + OperatorPair() = default; + + OperatorPair(const OperatorPair &) = delete; + OperatorPair &operator=(const OperatorPair &) = delete; + + Operator *oper = nullptr; + OperModifiers oper_mods = OPER_NONE; + }; + RuleSet() { Dbg(dbg_ctl, "RuleSet CTOR"); } ~RuleSet() { Dbg(dbg_ctl, "RulesSet DTOR"); + delete _operators[0].oper; // These are pointers + delete _operators[1].oper; delete next; - delete _cond; - delete _oper; } // noncopyable @@ -53,20 +65,14 @@ class RuleSet // No reason to inline these void append(RuleSet *rule); - bool add_condition(Parser &p, const char *filename, int lineno); + Condition *make_condition(Parser &p, const char *filename, int lineno); bool add_operator(Parser &p, const char *filename, int lineno); ResourceIDs get_all_resource_ids() const; bool has_operator() const { - return nullptr != _oper; - } - - bool - has_condition() const - { - return nullptr != _cond; + return (nullptr != _operators[0].oper) || (nullptr != _operators[1].oper); } void @@ -75,6 +81,12 @@ class RuleSet _hook = hook; } + ConditionGroup * + get_group() + { + return &_group; + } + TSHttpHookID get_hook() const { @@ -88,41 +100,53 @@ class RuleSet } bool - eval(const Resources &res) const + last() const { - if (nullptr == _cond) { - return true; - } else { - return _cond->do_eval(res); - } + return _last; } - bool - last() const + void + switch_branch() { - return _last; + _is_else = !_is_else; } OperModifiers - exec(const Resources &res) const + exec(const OperatorPair &ops, const Resources &res) const { - auto no_reenable_count{_oper->do_exec(res)}; + if (nullptr == ops.oper) { + return ops.oper_mods; + } + + auto no_reenable_count{ops.oper->do_exec(res)}; + ink_assert(no_reenable_count < 2); if (no_reenable_count) { - return static_cast(_opermods | OPER_NO_REENABLE); + return static_cast(ops.oper_mods | OPER_NO_REENABLE); + } + + return ops.oper_mods; + } + + const OperatorPair & + eval(const Resources &res) + { + if (_group.eval(res)) { + return _operators[0]; // IF conditions + } else { + return _operators[1]; // ELSE conditions } - return _opermods; } RuleSet *next = nullptr; // Linked list private: - Condition *_cond = nullptr; // First pre-condition (linked list) - Operator *_oper = nullptr; // First operator (linked list) - TSHttpHookID _hook = TS_HTTP_READ_RESPONSE_HDR_HOOK; // Which hook is this rule for + ConditionGroup _group; // All conditions are now wrapped in a group + OperatorPair _operators[2]; // Holds both the IF and the ELSE set of operators // State values (updated when conds / operators are added) - ResourceIDs _ids = RSRC_NONE; - OperModifiers _opermods = OPER_NONE; - bool _last = false; + TSHttpHookID _hook = TS_HTTP_READ_RESPONSE_HDR_HOOK; // Which hook is this rule for + ResourceIDs _ids = RSRC_NONE; + bool _last = false; + bool _is_else = false; // Are we in the else clause of the new rule? For parsing. };