From ed102cde6cfd4ca1d76a21471a66e0bfbb8ee574 Mon Sep 17 00:00:00 2001 From: Walt Karas Date: Mon, 1 Jul 2024 12:37:26 -0400 Subject: [PATCH] header_rewrite: Allow Txn reenable to be deferred. The SetBodyFrom operator needs to defer calling Txn reenable until the fetch of the URL providing the response body completes. --- plugins/header_rewrite/header_rewrite.cc | 11 ++- plugins/header_rewrite/operator.h | 26 ++++--- plugins/header_rewrite/operators.cc | 91 ++++++++++++++---------- plugins/header_rewrite/operators.h | 50 +++++++------ plugins/header_rewrite/ruleset.h | 8 ++- 5 files changed, 116 insertions(+), 70 deletions(-) diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc index d0a2752e593..c3e534af6cb 100644 --- a/plugins/header_rewrite/header_rewrite.cc +++ b/plugins/header_rewrite/header_rewrite.cc @@ -299,6 +299,7 @@ cont_rewrite_headers(TSCont contp, TSEvent event, void *edata) break; } + bool reenable{true}; if (hook != TS_HTTP_LAST_HOOK) { const RuleSet *rule = conf->rule(hook); Resources res(txnp, contp); @@ -311,6 +312,10 @@ cont_rewrite_headers(TSCont contp, TSEvent event, void *edata) if (rule->eval(res)) { OperModifiers rt = rule->exec(res); + if (rt & OPER_NO_REENABLE) { + reenable = false; + } + if (rule->last() || (rt & OPER_LAST)) { break; // Conditional break, force a break with [L] } @@ -319,7 +324,9 @@ cont_rewrite_headers(TSCont contp, TSEvent event, void *edata) } } - TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + if (reenable) { + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); + } return 0; } @@ -523,6 +530,8 @@ TSRemapDoRemap(void *ih, TSHttpTxn rh, TSRemapRequestInfo *rri) if (rule->eval(res)) { OperModifiers rt = rule->exec(res); + ink_assert((rt & OPER_NO_REENABLE) == 0); + if (res.changed_url == true) { rval = TSREMAP_DID_REMAP; } diff --git a/plugins/header_rewrite/operator.h b/plugins/header_rewrite/operator.h index 250da57726f..2d54f6fad42 100644 --- a/plugins/header_rewrite/operator.h +++ b/plugins/header_rewrite/operator.h @@ -31,11 +31,12 @@ // Operator modifiers enum OperModifiers { - OPER_NONE = 0, - OPER_LAST = 1, - OPER_NEXT = 2, - OPER_QSA = 4, - OPER_INV = 8, + OPER_NONE = 0, + OPER_LAST = 1, + OPER_NEXT = 2, + OPER_QSA = 4, + OPER_INV = 8, + OPER_NO_REENABLE = 16, }; /////////////////////////////////////////////////////////////////////////////// @@ -55,17 +56,24 @@ class Operator : public Statement OperModifiers get_oper_modifiers() const; void initialize(Parser &p) override; - void + // Returns number of executed operators that need to defer call to TSHttpTxnReenable(). It is a fatal error if this + // returns more than 1. If multiple operators need to defer reenable on the same hook, issue 11549 should be + // revisited. In this case, one possible approach would be to add a per-transaction user parameter, pointing to + // a count of operators that are deferred the reeanable for a particular hook. + unsigned do_exec(const Resources &res) const { - exec(res); + unsigned no_reenable{exec(res) ? 0U : 1U}; if (nullptr != _next) { - static_cast(_next)->do_exec(res); + no_reenable += static_cast(_next)->do_exec(res); } + return no_reenable; } protected: - virtual void exec(const Resources &res) const = 0; + // Return false to disable call of TSHttpTxnReenable(). Operators executed in the remap pseudo-hook MUST return true, + // as reenable is implicit in remap execution. + virtual bool exec(const Resources &res) const = 0; private: OperModifiers _mods = OPER_NONE; diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index 7729a82a523..189e39b83f4 100644 --- a/plugins/header_rewrite/operators.cc +++ b/plugins/header_rewrite/operators.cc @@ -77,10 +77,6 @@ handleFetchEvents(TSCont cont, TSEvent event, void *edata) TSContDestroy(cont); TSHttpTxnReenable(http_txn, TS_EVENT_HTTP_CONTINUE); } break; - case TS_EVENT_HTTP_SEND_RESPONSE_HDR: - // Do nothing - // The transaction is reenabled with the FetchSM transaction - break; default: TSError("[%s] handleFetchEvents got unknown event: %d", PLUGIN_NAME, event); break; @@ -130,7 +126,7 @@ OperatorSetConfig::initialize(Parser &p) } } -void +bool OperatorSetConfig::exec(const Resources &res) const { if (TS_CONFIG_NULL != _key) { @@ -161,6 +157,7 @@ OperatorSetConfig::exec(const Resources &res) const break; } } + return true; } // OperatorSetStatus @@ -193,7 +190,7 @@ OperatorSetStatus::initialize_hooks() add_allowed_hook(TS_REMAP_PSEUDO_HOOK); } -void +bool OperatorSetStatus::exec(const Resources &res) const { switch (get_hook()) { @@ -212,6 +209,8 @@ OperatorSetStatus::exec(const Resources &res) const } Dbg(pi_dbg_ctl, "OperatorSetStatus::exec() invoked with status=%d", _status.get_int_value()); + + return true; } // OperatorSetStatusReason @@ -232,7 +231,7 @@ OperatorSetStatusReason::initialize_hooks() add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK); } -void +bool OperatorSetStatusReason::exec(const Resources &res) const { if (res.bufp && res.hdr_loc) { @@ -244,6 +243,7 @@ OperatorSetStatusReason::exec(const Resources &res) const TSHttpHdrReasonSet(res.bufp, res.hdr_loc, reason.c_str(), reason.size()); } } + return true; } // OperatorSetDestination @@ -258,7 +258,7 @@ OperatorSetDestination::initialize(Parser &p) require_resources(RSRC_SERVER_REQUEST_HEADERS); } -void +bool OperatorSetDestination::exec(const Resources &res) const { if (res._rri || (res.bufp && res.hdr_loc)) { @@ -274,7 +274,7 @@ OperatorSetDestination::exec(const Resources &res) const bufp = res.bufp; if (TSHttpHdrUrlGet(res.bufp, res.hdr_loc, &url_m_loc) != TS_SUCCESS) { Dbg(pi_dbg_ctl, "TSHttpHdrUrlGet was unable to return the url m_loc"); - return; + return true; } } @@ -365,6 +365,7 @@ OperatorSetDestination::exec(const Resources &res) const Dbg(pi_dbg_ctl, "OperatorSetDestination::exec() unable to continue due to missing bufp=%p or hdr_loc=%p, rri=%p!", res.bufp, res.hdr_loc, res._rri); } + return true; } #include @@ -401,7 +402,7 @@ OperatorRMDestination::initialize(Parser &p) require_resources(RSRC_SERVER_REQUEST_HEADERS); } -void +bool OperatorRMDestination::exec(const Resources &res) const { if (res._rri || (res.bufp && res.hdr_loc)) { @@ -417,7 +418,7 @@ OperatorRMDestination::exec(const Resources &res) const bufp = res.bufp; if (TSHttpHdrUrlGet(res.bufp, res.hdr_loc, &url_m_loc) != TS_SUCCESS) { Dbg(pi_dbg_ctl, "TSHttpHdrUrlGet was unable to return the url m_loc"); - return; + return true; } } @@ -467,6 +468,7 @@ OperatorRMDestination::exec(const Resources &res) const Dbg(pi_dbg_ctl, "OperatorRMDestination::exec() unable to continue due to missing bufp=%p or hdr_loc=%p, rri=%p!", res.bufp, res.hdr_loc, res._rri); } + return true; } // OperatorSetRedirect @@ -523,7 +525,7 @@ EditRedirectResponse(TSHttpTxn txnp, const std::string &location, TSHttpStatus s TSHttpTxnErrorBodySet(txnp, TSstrdup(msg.c_str()), msg.length(), TSstrdup("text/html")); } -void +bool OperatorSetRedirect::exec(const Resources &res) const { if (res.bufp && res.hdr_loc && res.client_bufp && res.client_hdr_loc) { @@ -598,6 +600,7 @@ OperatorSetRedirect::exec(const Resources &res) const Dbg(pi_dbg_ctl, "OperatorSetRedirect::exec() invoked with destination=%s and status code=%d", value.c_str(), _status.get_int_value()); } + return true; } // OperatorSetTimeoutOut @@ -622,7 +625,7 @@ OperatorSetTimeoutOut::initialize(Parser &p) _timeout.set_value(p.get_value()); } -void +bool OperatorSetTimeoutOut::exec(const Resources &res) const { switch (_type) { @@ -649,6 +652,7 @@ OperatorSetTimeoutOut::exec(const Resources &res) const TSError("[%s] unsupported timeout", PLUGIN_NAME); break; } + return true; } // OperatorSkipRemap @@ -663,15 +667,16 @@ OperatorSkipRemap::initialize(Parser &p) } } -void +bool OperatorSkipRemap::exec(const Resources &res) const { Dbg(pi_dbg_ctl, "OperatorSkipRemap::exec() skipping remap: %s", _skip_remap ? "True" : "False"); TSHttpTxnCntlSet(res.txnp, TS_HTTP_CNTL_SKIP_REMAPPING, _skip_remap); + return true; } // OperatorRMHeader -void +bool OperatorRMHeader::exec(const Resources &res) const { TSMLoc field_loc, tmp; @@ -687,6 +692,7 @@ OperatorRMHeader::exec(const Resources &res) const field_loc = tmp; } } + return true; } // OperatorAddHeader @@ -698,7 +704,7 @@ OperatorAddHeader::initialize(Parser &p) _value.set_value(p.get_value()); } -void +bool OperatorAddHeader::exec(const Resources &res) const { std::string value; @@ -708,7 +714,7 @@ OperatorAddHeader::exec(const Resources &res) const // Never set an empty header (I don't think that ever makes sense?) if (value.empty()) { Dbg(pi_dbg_ctl, "Would set header %s to an empty value, skipping", _header.c_str()); - return; + return true; } if (res.bufp && res.hdr_loc) { @@ -723,6 +729,7 @@ OperatorAddHeader::exec(const Resources &res) const TSHandleMLocRelease(res.bufp, res.hdr_loc, field_loc); } } + return true; } // OperatorSetHeader @@ -734,7 +741,7 @@ OperatorSetHeader::initialize(Parser &p) _value.set_value(p.get_value()); } -void +bool OperatorSetHeader::exec(const Resources &res) const { std::string value; @@ -744,7 +751,7 @@ OperatorSetHeader::exec(const Resources &res) const // Never set an empty header (I don't think that ever makes sense?) if (value.empty()) { Dbg(pi_dbg_ctl, "Would set header %s to an empty value, skipping", _header.c_str()); - return; + return true; } if (res.bufp && res.hdr_loc) { @@ -780,6 +787,7 @@ OperatorSetHeader::exec(const Resources &res) const } } } + return true; } // OperatorSetBody @@ -798,7 +806,7 @@ OperatorSetBody::initialize_hooks() add_allowed_hook(TS_HTTP_SEND_RESPONSE_HDR_HOOK); } -void +bool OperatorSetBody::exec(const Resources &res) const { std::string value; @@ -806,6 +814,7 @@ OperatorSetBody::exec(const Resources &res) const _value.append_value(value, res); char *msg = TSstrdup(_value.get_value().c_str()); TSHttpTxnErrorBodySet(res.txnp, msg, _value.size(), nullptr); + return true; } // OperatorCounter @@ -835,20 +844,21 @@ OperatorCounter::initialize(Parser &p) } } -void +bool OperatorCounter::exec(const Resources & /* ATS_UNUSED res */) const { // Sanity if (_counter == TS_ERROR) { - return; + return true; } Dbg(pi_dbg_ctl, "OperatorCounter::exec() invoked on %s", _counter_name.c_str()); TSStatIntIncrement(_counter, 1); + return true; } // OperatorRMCookie -void +bool OperatorRMCookie::exec(const Resources &res) const { if (res.bufp && res.hdr_loc) { @@ -859,7 +869,7 @@ OperatorRMCookie::exec(const Resources &res) const field_loc = TSMimeHdrFieldFind(res.bufp, res.hdr_loc, TS_MIME_FIELD_COOKIE, TS_MIME_LEN_COOKIE); if (nullptr == field_loc) { Dbg(pi_dbg_ctl, "OperatorRMCookie::exec, no cookie"); - return; + return true; } int cookies_len = 0; @@ -877,6 +887,7 @@ OperatorRMCookie::exec(const Resources &res) const } TSHandleMLocRelease(res.bufp, res.hdr_loc, field_loc); } + return true; } // OperatorAddCookie @@ -887,7 +898,7 @@ OperatorAddCookie::initialize(Parser &p) _value.set_value(p.get_value()); } -void +bool OperatorAddCookie::exec(const Resources &res) const { std::string value; @@ -910,7 +921,7 @@ OperatorAddCookie::exec(const Resources &res) const } TSHandleMLocRelease(res.bufp, res.hdr_loc, field_loc); } - return; + return true; } int cookies_len = 0; @@ -922,6 +933,7 @@ OperatorAddCookie::exec(const Resources &res) const Dbg(pi_dbg_ctl, "OperatorAddCookie::exec, updated_cookie = [%s]", updated_cookie.c_str()); } } + return true; } // OperatorSetCookie @@ -932,7 +944,7 @@ OperatorSetCookie::initialize(Parser &p) _value.set_value(p.get_value()); } -void +bool OperatorSetCookie::exec(const Resources &res) const { std::string value; @@ -955,7 +967,7 @@ OperatorSetCookie::exec(const Resources &res) const } TSHandleMLocRelease(res.bufp, res.hdr_loc, field_loc); } - return; + return true; } int cookies_len = 0; @@ -968,6 +980,7 @@ OperatorSetCookie::exec(const Resources &res) const } TSHandleMLocRelease(res.bufp, res.hdr_loc, field_loc); } + return true; } bool @@ -1077,13 +1090,14 @@ OperatorSetConnDSCP::initialize_hooks() add_allowed_hook(TS_REMAP_PSEUDO_HOOK); } -void +bool OperatorSetConnDSCP::exec(const Resources &res) const { if (res.txnp) { TSHttpTxnClientPacketDscpSet(res.txnp, _ds_value.get_int_value()); Dbg(pi_dbg_ctl, " Setting DSCP to %d", _ds_value.get_int_value()); } + return true; } // OperatorSetConnMark @@ -1103,13 +1117,14 @@ OperatorSetConnMark::initialize_hooks() add_allowed_hook(TS_REMAP_PSEUDO_HOOK); } -void +bool OperatorSetConnMark::exec(const Resources &res) const { if (res.txnp) { TSHttpTxnClientPacketMarkSet(res.txnp, _ds_value.get_int_value()); Dbg(pi_dbg_ctl, " Setting MARK to %d", _ds_value.get_int_value()); } + return true; } // OperatorSetDebug @@ -1128,10 +1143,11 @@ OperatorSetDebug::initialize_hooks() add_allowed_hook(TS_REMAP_PSEUDO_HOOK); } -void +bool OperatorSetDebug::exec(const Resources &res) const { TSHttpTxnCntlSet(res.txnp, TS_HTTP_CNTL_TXN_DEBUG, true); + return true; } // OperatorSetHttpCntl @@ -1190,7 +1206,7 @@ static const char *const HttpCntls[] = { "LOGGING", "INTERCEPT_RETRY", "RESP_CACHEABLE", "REQ_CACHEABLE", "SERVER_NO_STORE", "TXN_DEBUG", "SKIP_REMAP", }; -void +bool OperatorSetHttpCntl::exec(const Resources &res) const { if (_flag) { @@ -1200,6 +1216,7 @@ OperatorSetHttpCntl::exec(const Resources &res) const TSHttpTxnCntlSet(res.txnp, _cntl_qual, false); Dbg(pi_dbg_ctl, " Turning OFF %s for transaction", HttpCntls[static_cast(_cntl_qual)]); } + return true; } void @@ -1262,7 +1279,7 @@ OperatorRunPlugin::initialize_hooks() require_resources(RSRC_CLIENT_REQUEST_HEADERS); // Need this for the txnp } -void +bool OperatorRunPlugin::exec(const Resources &res) const { TSReleaseAssert(_plugin != nullptr); @@ -1270,6 +1287,7 @@ OperatorRunPlugin::exec(const Resources &res) const if (res._rri && res.txnp) { _plugin->doRemap(res.txnp, res._rri); } + return true; } // OperatorSetBody @@ -1289,14 +1307,14 @@ OperatorSetBodyFrom::initialize_hooks() add_allowed_hook(TS_HTTP_READ_RESPONSE_HDR_HOOK); } -void +bool OperatorSetBodyFrom::exec(const Resources &res) const { if (TSHttpTxnIsInternal(res.txnp)) { // If this is triggered by an internal transaction, a infinte loop may occur // It should only be triggered by the original transaction sent by the client Dbg(pi_dbg_ctl, "OperatorSetBodyFrom triggered by an internal transaction"); - return; + return true; } char req_buf[MAX_SIZE]; @@ -1305,7 +1323,6 @@ OperatorSetBodyFrom::exec(const Resources &res) const TSCont fetchCont = TSContCreate(handleFetchEvents, TSMutexCreate()); TSContDataSet(fetchCont, static_cast(res.txnp)); - TSHttpTxnHookAdd(res.txnp, TS_HTTP_SEND_RESPONSE_HDR_HOOK, fetchCont); TSHttpTxnHookAdd(res.txnp, TS_HTTP_TXN_CLOSE_HOOK, fetchCont); TSFetchEvent event_ids; @@ -1326,5 +1343,7 @@ OperatorSetBodyFrom::exec(const Resources &res) const TSHttpTxnStatusSet(res.txnp, res.resp_status); } else { TSError(PLUGIN_NAME, "OperatorSetBodyFrom:exec:: Could not create request"); + return true; } + return false; } diff --git a/plugins/header_rewrite/operators.h b/plugins/header_rewrite/operators.h index 25debe1d7e3..df4e189686f 100644 --- a/plugins/header_rewrite/operators.h +++ b/plugins/header_rewrite/operators.h @@ -44,7 +44,7 @@ class OperatorSetConfig : public Operator void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: TSOverridableConfigKey _key = TS_CONFIG_NULL; @@ -67,7 +67,7 @@ class OperatorSetStatus : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _status; @@ -88,7 +88,7 @@ class OperatorSetStatusReason : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _reason; @@ -106,7 +106,7 @@ class OperatorSetDestination : public Operator void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: UrlQualifiers _url_qual = URL_QUAL_NONE; @@ -126,7 +126,7 @@ class OperatorRMDestination : public Operator void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: UrlQualifiers _url_qual = URL_QUAL_NONE; @@ -161,7 +161,7 @@ class OperatorSetRedirect : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _status; @@ -178,7 +178,11 @@ class OperatorNoOp : public Operator void operator=(const OperatorNoOp &) = delete; protected: - void exec(const Resources & /* res ATS_UNUSED */) const override {}; + bool + exec(const Resources & /* res ATS_UNUSED */) const override + { + return true; + }; }; class OperatorSetTimeoutOut : public Operator @@ -193,7 +197,7 @@ class OperatorSetTimeoutOut : public Operator void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: enum TimeoutOutType { @@ -220,7 +224,7 @@ class OperatorSkipRemap : public Operator void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: bool _skip_remap = false; @@ -237,7 +241,7 @@ class OperatorRMHeader : public OperatorHeaders void operator=(const OperatorRMHeader &) = delete; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; }; class OperatorAddHeader : public OperatorHeaders @@ -252,7 +256,7 @@ class OperatorAddHeader : public OperatorHeaders void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _value; @@ -270,7 +274,7 @@ class OperatorSetHeader : public OperatorHeaders void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _value; @@ -288,7 +292,7 @@ class OperatorCounter : public Operator void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: std::string _counter_name; @@ -305,7 +309,7 @@ class OperatorRMCookie : public OperatorCookies void operator=(const OperatorRMCookie &) = delete; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; }; class OperatorAddCookie : public OperatorCookies @@ -320,7 +324,7 @@ class OperatorAddCookie : public OperatorCookies void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _value; @@ -338,7 +342,7 @@ class OperatorSetCookie : public OperatorCookies void initialize(Parser &p) override; protected: - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _value; @@ -369,7 +373,7 @@ class OperatorSetConnDSCP : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _ds_value; @@ -388,7 +392,7 @@ class OperatorSetConnMark : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _ds_value; @@ -407,7 +411,7 @@ class OperatorSetDebug : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; }; class OperatorSetBody : public Operator @@ -423,7 +427,7 @@ class OperatorSetBody : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _value; @@ -442,7 +446,7 @@ class OperatorSetHttpCntl : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: bool _flag = false; @@ -475,7 +479,7 @@ class OperatorRunPlugin : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: RemapPluginInst *_plugin = nullptr; @@ -496,7 +500,7 @@ class OperatorSetBodyFrom : public Operator protected: void initialize_hooks() override; - void exec(const Resources &res) const override; + bool exec(const Resources &res) const override; private: Value _value; diff --git a/plugins/header_rewrite/ruleset.h b/plugins/header_rewrite/ruleset.h index 932a433375f..d1b647e8c1c 100644 --- a/plugins/header_rewrite/ruleset.h +++ b/plugins/header_rewrite/ruleset.h @@ -23,6 +23,8 @@ #include +#include + #include "matcher.h" #include "factory.h" #include "resources.h" @@ -104,7 +106,11 @@ class RuleSet OperModifiers exec(const Resources &res) const { - _oper->do_exec(res); + auto no_reenable_count{_oper->do_exec(res)}; + ink_assert(no_reenable_count < 2); + if (no_reenable_count) { + return static_cast(_opermods | OPER_NO_REENABLE); + } return _opermods; }