From a98270b0d30818a4ac2f3202a909ff4f7b93426c Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Sat, 22 Oct 2022 19:44:50 +0000 Subject: [PATCH] Mimic GET reforwarding decisions when our CONNECT fails (#1168) Use FwdState::reforwardableStatus() logic when deciding whether to reforward our CONNECT request after a failed tunneling attempt. Also honor forward_max_tries limit when retrying tunneling attempts. These TunnelStateData fixes deal with ordinary CONNECT traffic (no SslBump). FwdState also handles CONNECT requests (with SslBump). We make that CONNECT handling more consistent across these classes (in addition to making it more consistent across CONNECT and GET/etc. methods). Co-authored-by: Alex Rousskov --- src/FwdState.cc | 28 +--------------------- src/FwdState.h | 1 - src/http.cc | 5 ++-- src/http/StatusCode.cc | 23 ++++++++++++++++++ src/http/StatusCode.h | 2 ++ src/tunnel.cc | 54 +++++++++++++++++++++++++++++++++++------- 6 files changed, 75 insertions(+), 38 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index 8bdf5206f1..e07bea3233 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1339,7 +1339,7 @@ FwdState::reforward() const auto s = entry->mem().baseReply().sline.status(); debugs(17, 3, "status " << s); - return reforwardableStatus(s); + return Http::IsReforwardableStatus(s); } static void @@ -1371,32 +1371,6 @@ fwdStats(StoreEntry * s) /**** STATIC MEMBER FUNCTIONS *************************************************/ -bool -FwdState::reforwardableStatus(const Http::StatusCode s) const -{ - switch (s) { - - case Http::scBadGateway: - - case Http::scGatewayTimeout: - return true; - - case Http::scForbidden: - - case Http::scInternalServerError: - - case Http::scNotImplemented: - - case Http::scServiceUnavailable: - return Config.retry.onerror; - - default: - return false; - } - - /* NOTREACHED */ -} - void FwdState::initModule() { diff --git a/src/FwdState.h b/src/FwdState.h index b7adbce7c5..ad9e75cc3b 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -85,7 +85,6 @@ public: void handleUnregisteredServerEnd(); int reforward(); - bool reforwardableStatus(const Http::StatusCode s) const; void serverClosed(); void connectStart(); void connectDone(const Comm::ConnectionPointer & conn, Comm::Flag status, int xerrno); diff --git a/src/http.cc b/src/http.cc index afc0b7623f..cc6ee208e0 100644 --- a/src/http.cc +++ b/src/http.cc @@ -33,6 +33,7 @@ #include "http.h" #include "http/one/ResponseParser.h" #include "http/one/TeChunkedParser.h" +#include "http/StatusCode.h" #include "http/Stream.h" #include "HttpControlMsg.h" #include "HttpHdrCc.h" @@ -967,7 +968,7 @@ HttpStateData::haveParsedReplyHeaders() // TODO: check whether such responses are shareable. // Do not share for now. entry->makePrivate(false); - if (fwd->reforwardableStatus(rep->sline.status())) + if (Http::IsReforwardableStatus(rep->sline.status())) EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT); varyFailure = true; } else { @@ -986,7 +987,7 @@ HttpStateData::haveParsedReplyHeaders() * If its not a reply that we will re-forward, then * allow the client to get it. */ - if (fwd->reforwardableStatus(rep->sline.status())) + if (Http::IsReforwardableStatus(rep->sline.status())) EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT); ReuseDecision decision(entry, statusCode); diff --git a/src/http/StatusCode.cc b/src/http/StatusCode.cc index a4aac20ba9..24fb294234 100644 --- a/src/http/StatusCode.cc +++ b/src/http/StatusCode.cc @@ -9,6 +9,7 @@ #include "squid.h" #include "debug/Stream.h" #include "http/StatusCode.h" +#include "SquidConfig.h" const char * Http::StatusCodeString(const Http::StatusCode status) @@ -276,3 +277,25 @@ Http::StatusCodeString(const Http::StatusCode status) return "Unassigned"; } +bool +Http::IsReforwardableStatus(const StatusCode s) +{ + switch (s) { + + case scBadGateway: + case scGatewayTimeout: + return true; + + case scForbidden: + case scInternalServerError: + case scNotImplemented: + case scServiceUnavailable: + return Config.retry.onerror; + + default: + return false; + } + + /* NOTREACHED */ +} + diff --git a/src/http/StatusCode.h b/src/http/StatusCode.h index b12a01eee6..38ece6d9a1 100644 --- a/src/http/StatusCode.h +++ b/src/http/StatusCode.h @@ -92,6 +92,8 @@ const char *StatusCodeString(const Http::StatusCode status); inline bool Is1xx(const int sc) { return scContinue <= sc && sc < scOkay; } /// whether this response status code prohibits sending Content-Length inline bool ProhibitsContentLength(const StatusCode sc) { return sc == scNoContent || Is1xx(sc); } +/// whether to send the request to another peer based on the current response status code +bool IsReforwardableStatus(StatusCode); } // namespace Http diff --git a/src/tunnel.cc b/src/tunnel.cc index 5761d11d48..b93be36cbc 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -31,6 +31,7 @@ #include "globals.h" #include "HappyConnOpener.h" #include "http.h" +#include "http/StatusCode.h" #include "http/Stream.h" #include "HttpRequest.h" #include "icmp/net_db.h" @@ -188,13 +189,15 @@ public: time_t startTime; ///< object creation time, before any peer selection/connection attempts ResolvedPeersPointer destinations; ///< paths for forwarding the request bool destinationsFound; ///< At least one candidate path found - /// whether another destination may be still attempted if the TCP connection - /// was unexpectedly closed - bool retriable; /// whether the decision to tunnel to a particular destination was final bool committedToServer; + int n_tries; ///< the number of forwarding attempts so far + + /// a reason to ban reforwarding attempts (or nil) + const char *banRetries; + // TODO: remove after fixing deferred reads in TunnelStateData::copyRead() CodeContext::Pointer codeContext; ///< our creator context @@ -250,6 +253,7 @@ private: bool transporting() const; + // TODO: convert to unique_ptr /// details of the "last tunneling attempt" failure (if it failed) ErrorState *savedError = nullptr; @@ -260,6 +264,8 @@ private: void cancelStep(const char *reason); + bool exhaustedTries() const; + public: bool keepGoingAfterRead(size_t len, Comm::Flag errcode, int xerrno, Connection &from, Connection &to); void copy(size_t len, Connection &from, Connection &to, IOCB *); @@ -344,8 +350,9 @@ TunnelStateData::TunnelStateData(ClientHttpRequest *clientRequest) : startTime(squid_curtime), destinations(new ResolvedPeers()), destinationsFound(false), - retriable(true), committedToServer(false), + n_tries(0), + banRetries(nullptr), codeContext(CodeContext::Current()) { debugs(26, 3, "TunnelStateData constructed this=" << this); @@ -391,12 +398,20 @@ TunnelStateData::checkRetry() { if (shutting_down) return "shutting down"; + if (exhaustedTries()) + return "exhausted tries"; if (!FwdState::EnoughTimeToReForward(startTime)) return "forwarding timeout"; - if (!retriable) - return "not retriable"; + if (banRetries) + return banRetries; if (noConnections()) return "no connections"; + + // TODO: Use Optional for peer_reply_status to avoid treating zero value specially. + if (request->hier.peer_reply_status != Http::scNone && !Http::IsReforwardableStatus(request->hier.peer_reply_status)) + return "received HTTP status code is not reforwardable"; + + // TODO: check pinned connections; see FwdState::pinnedCanRetry() return nullptr; } @@ -951,6 +966,9 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) al->cache.code.update(LOG_TCP_TUNNEL); + // XXX: al->http.code (i.e. *status_ptr) should not be (re)set + // until we actually start responding to the client. Right here/now, we only + // know how this cache_peer has responded to us. if (answer.peerResponseStatus != Http::scNone) *status_ptr = answer.peerResponseStatus; @@ -1008,7 +1026,7 @@ void TunnelStateData::commitToServer(const Comm::ConnectionPointer &conn) { committedToServer = true; - retriable = false; // may already be false + banRetries = "committed to server"; PeerSelectionInitiator::subscribed = false; // may already be false server.initConnection(conn, tunnelServerClosed, "tunnelServerClosed", this); } @@ -1034,8 +1052,12 @@ TunnelStateData::noteConnection(HappyConnOpener::Answer &answer) { transportWait.finish(); + Assure(n_tries <= answer.n_tries); // n_tries cannot decrease + n_tries = answer.n_tries; + ErrorState *error = nullptr; if ((error = answer.error.get())) { + banRetries = "HappyConnOpener gave up"; Must(!Comm::IsConnOpen(answer.conn)); syncHierNote(answer.conn, request->url.host()); answer.error.clear(); @@ -1062,6 +1084,8 @@ TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *or ResetMarkingsToServer(request.getRaw(), *conn); // else Comm::ConnOpener already applied proper/current markings + // TODO: add pconn race state tracking + syncHierNote(conn, origin); #if USE_DELAY_POOLS @@ -1090,6 +1114,13 @@ TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *or } } +/// whether we have used up all permitted forwarding attempts +bool +TunnelStateData::exhaustedTries() const +{ + return n_tries >= Config.forward_max_tries; +} + void tunnelStart(ClientHttpRequest * http) { @@ -1357,8 +1388,13 @@ TunnelStateData::startConnecting() assert(!destinations->empty()); assert(!transporting()); + + delete savedError; // may still be nil + savedError = nullptr; + request->hier.peer_reply_status = Http::scNone; // TODO: Move to startPeerClock()? + const auto callback = asyncCallback(17, 5, TunnelStateData::noteConnection, this); - const auto cs = new HappyConnOpener(destinations, callback, request, startTime, 0, al); + const auto cs = new HappyConnOpener(destinations, callback, request, startTime, n_tries, al); cs->setHost(request->url.host()); cs->setRetriable(false); cs->allowPersistent(false); @@ -1376,6 +1412,8 @@ TunnelStateData::usePinned() const auto serverConn = ConnStateData::BorrowPinnedConnection(request.getRaw(), al); debugs(26, 7, "pinned peer connection: " << serverConn); + ++n_tries; + // Set HttpRequest pinned related flags for consistency even if // they are not really used by tunnel.cc code. request->flags.pinned = true; -- 2.39.2