diff --git a/squid-6.13-cache-peer-connect-errors.patch b/squid-6.13-cache-peer-connect-errors.patch new file mode 100644 index 0000000..339d9ec --- /dev/null +++ b/squid-6.13-cache-peer-connect-errors.patch @@ -0,0 +1,287 @@ +From 2e7dea3cedd3ef2f071dee82867c4147f17376dd Mon Sep 17 00:00:00 2001 +From: Alex Rousskov +Date: Tue, 2 Apr 2024 20:37:31 +0000 +Subject: [PATCH] Do not blame cache_peer for CONNECT errors (#1772) + + ERROR: Connection to [such-and-such-cache_peer] failed + TCP_TUNNEL/503 CONNECT nxdomain.test:443 FIRSTUP_PARENT + +Squid does not alert an admin about (and decrease health level of) a +cache_peer that responded with an error to a GET request. Just like GET +responses from a cache_peer, CONNECT responses may (and often do!) +reflect client or origin server failures. We should not penalize +cache_peers (and alert admins) until we can distinguish these frequent +client/origin failures from (relatively rare) cache_peer problems. This +change absolves cache_peers of CONNECT problems, restoring parity with +GETs and restoring v4 behavior changed (probably by accident) in v5. + +Also removed Http::StatusCode parameter from failure notification +functions because it became essentially unused after the primary +Http::Tunneler changes. Tunneler was the only source of status code +information that (in some cases) used received HTTP response to compute +that status code. All other cases extracted that status code from +Squid-generated errors. Those errors were arguably never meant to supply +status code information for "this failure is not our fault" decision, +and they do not supply 4xx status codes driving that decision. + +### Problem evolution + +2019 commit f5e1794 effectively started blaming cache_peer for all +FwdState CONNECT errors. That functionality change was probably +accidental, likely influenced by the names of noteConnectFailure() and +peerConnectFailed() functions that abbreviated "Connection", making the +functions look as applicable to CONNECT failures. Prior to that commit, +the functions were never used for CONNECT errors. After it, FwdState +started calling peerConnectFailed() for all CONNECT failures. + +In 2020 commit 25b0ce4, TunnelStateData started blaming cache_peers as +well (by moving that FwdState-only error handling code into Tunneler). +The same "accidental functionality change" speculations apply here. + +In 2022 commit 022dbab, we made an exception for 4xx CONNECT errors as +folks deploying newer code started complaining about cache_peers getting +blamed for client-caused errors (e.g., HTTP 403 Forbidden replies). We +did not realize that the blaming code itself was an unwanted accident. + +Now we are getting complaints about cache_peers getting blamed for 502 +and 503 CONNECT errors caused by, for example, domain names without IPs: +As these CONNECT error responses are propagated from parent to child +caches, every child cache in the chain logs ERRORs and every cache_peer +in the chain gets its health counter decreased! +--- + src/CachePeer.cc | 11 +---------- + src/CachePeer.h | 12 +++++------- + src/HappyConnOpener.cc | 2 +- + src/PeerPoolMgr.cc | 2 +- + src/clients/HttpTunneler.cc | 10 ++++++---- + src/clients/HttpTunneler.h | 2 +- + src/neighbors.cc | 2 +- + src/security/BlindPeerConnector.cc | 2 +- + src/security/PeerConnector.cc | 8 ++++---- + src/security/PeerConnector.h | 2 +- + src/tests/stub_libsecurity.cc | 2 +- + 11 files changed, 23 insertions(+), 32 deletions(-) + +diff --git a/src/CachePeer.cc b/src/CachePeer.cc +index a5c3adf..91045ef 100644 +--- a/src/CachePeer.cc ++++ b/src/CachePeer.cc +@@ -68,20 +68,11 @@ CachePeer::noteSuccess() + } + } + +-void +-CachePeer::noteFailure(const Http::StatusCode code) +-{ +- if (Http::Is4xx(code)) +- return; // this failure is not our fault +- +- countFailure(); +-} +- + // TODO: Require callers to detail failures instead of using one (and often + // misleading!) "connection failed" phrase for all of them. + /// noteFailure() helper for handling failures attributed to this peer + void +-CachePeer::countFailure() ++CachePeer::noteFailure() + { + stats.last_connect_failure = squid_curtime; + if (tcp_up > 0) +diff --git a/src/CachePeer.h b/src/CachePeer.h +index 5b13e29..14e40ff 100644 +--- a/src/CachePeer.h ++++ b/src/CachePeer.h +@@ -38,9 +38,8 @@ public: + /// reacts to a successful establishment of a connection to this cache_peer + void noteSuccess(); + +- /// reacts to a failure on a connection to this cache_peer +- /// \param code a received response status code, if any +- void noteFailure(Http::StatusCode code); ++ /// reacts to a failed attempt to establish a connection to this cache_peer ++ void noteFailure(); + + /// (re)configure cache_peer name=value + void rename(const char *); +@@ -238,14 +237,13 @@ NoteOutgoingConnectionSuccess(CachePeer * const peer) + peer->noteSuccess(); + } + +-/// reacts to a failure on a connection to an origin server or cache_peer ++/// reacts to a failed attempt to establish a connection to an origin server or cache_peer + /// \param peer nil if the connection is to an origin server +-/// \param code a received response status code, if any + inline void +-NoteOutgoingConnectionFailure(CachePeer * const peer, const Http::StatusCode code) ++NoteOutgoingConnectionFailure(CachePeer * const peer) + { + if (peer) +- peer->noteFailure(code); ++ peer->noteFailure(); + } + + /// identify the given cache peer in cache.log messages and such +diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc +index 5ab9294..5e17a76 100644 +--- a/src/HappyConnOpener.cc ++++ b/src/HappyConnOpener.cc +@@ -638,7 +638,7 @@ HappyConnOpener::handleConnOpenerAnswer(Attempt &attempt, const CommConnectCbPar + lastError = makeError(ERR_CONNECT_FAIL); + lastError->xerrno = params.xerrno; + +- NoteOutgoingConnectionFailure(params.conn->getPeer(), lastError->httpStatus); ++ NoteOutgoingConnectionFailure(params.conn->getPeer()); + + if (spareWaiting) + updateSpareWaitAfterPrimeFailure(); +diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc +index 9cb038e..6fb5b09 100644 +--- a/src/PeerPoolMgr.cc ++++ b/src/PeerPoolMgr.cc +@@ -86,7 +86,7 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams ¶ms) + } + + if (params.flag != Comm::OK) { +- NoteOutgoingConnectionFailure(peer, Http::scNone); ++ NoteOutgoingConnectionFailure(peer); + checkpoint("conn opening failure"); // may retry + return; + } +diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc +index 2fbc3fb..a6e49db 100644 +--- a/src/clients/HttpTunneler.cc ++++ b/src/clients/HttpTunneler.cc +@@ -90,7 +90,7 @@ Http::Tunneler::handleConnectionClosure(const CommCloseCbParams &) + { + closer = nullptr; + if (connection) { +- countFailingConnection(nullptr); ++ countFailingConnection(); + connection->noteClosure(); + connection = nullptr; + } +@@ -355,7 +355,7 @@ Http::Tunneler::bailWith(ErrorState *error) + + if (const auto failingConnection = connection) { + // TODO: Reuse to-peer connections after a CONNECT error response. +- countFailingConnection(error); ++ countFailingConnection(); + disconnect(); + failingConnection->close(); + } +@@ -374,10 +374,12 @@ Http::Tunneler::sendSuccess() + } + + void +-Http::Tunneler::countFailingConnection(const ErrorState * const error) ++Http::Tunneler::countFailingConnection() + { + assert(connection); +- NoteOutgoingConnectionFailure(connection->getPeer(), error ? error->httpStatus : Http::scNone); ++ // No NoteOutgoingConnectionFailure(connection->getPeer()) call here because ++ // we do not blame cache_peer for CONNECT failures (on top of a successfully ++ // established connection to that cache_peer). + if (noteFwdPconnUse && connection->isOpen()) + fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses); + } +diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h +index 7886f09..596efcf 100644 +--- a/src/clients/HttpTunneler.h ++++ b/src/clients/HttpTunneler.h +@@ -80,7 +80,7 @@ private: + void disconnect(); + + /// updates connection usage history before the connection is closed +- void countFailingConnection(const ErrorState *); ++ void countFailingConnection(); + + AsyncCall::Pointer writer; ///< called when the request has been written + AsyncCall::Pointer reader; ///< called when the response should be read +diff --git a/src/neighbors.cc b/src/neighbors.cc +index 04b69c1..75f56c9 100644 +--- a/src/neighbors.cc ++++ b/src/neighbors.cc +@@ -1320,7 +1320,7 @@ peerProbeConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int + if (status == Comm::OK) + p->noteSuccess(); + else +- p->noteFailure(Http::scNone); ++ p->noteFailure(); + + -- p->testing_now; + conn->close(); +diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc +index b9e5659..4c37f34 100644 +--- a/src/security/BlindPeerConnector.cc ++++ b/src/security/BlindPeerConnector.cc +@@ -76,7 +76,7 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) + // based on TCP results, SSL results, or both. And the code is probably not + // consistent in this aspect across tunnelling and forwarding modules. + if (peer && peer->secure.encryptTransport) +- peer->noteFailure(error->httpStatus); ++ peer->noteFailure(); + return; + } + +diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc +index d458f99..d0131a1 100644 +--- a/src/security/PeerConnector.cc ++++ b/src/security/PeerConnector.cc +@@ -115,7 +115,7 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) + err->detailError(d); + + if (serverConn) { +- countFailingConnection(err); ++ countFailingConnection(); + serverConn->noteClosure(); + serverConn = nullptr; + } +@@ -507,7 +507,7 @@ Security::PeerConnector::bail(ErrorState *error) + answer().error = error; + + if (const auto failingConnection = serverConn) { +- countFailingConnection(error); ++ countFailingConnection(); + disconnect(); + failingConnection->close(); + } +@@ -525,10 +525,10 @@ Security::PeerConnector::sendSuccess() + } + + void +-Security::PeerConnector::countFailingConnection(const ErrorState * const error) ++Security::PeerConnector::countFailingConnection() + { + assert(serverConn); +- NoteOutgoingConnectionFailure(serverConn->getPeer(), error ? error->httpStatus : Http::scNone); ++ NoteOutgoingConnectionFailure(serverConn->getPeer()); + // TODO: Calling PconnPool::noteUses() should not be our responsibility. + if (noteFwdPconnUse && serverConn->isOpen()) + fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); +diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h +index a1d5ef9..401df06 100644 +--- a/src/security/PeerConnector.h ++++ b/src/security/PeerConnector.h +@@ -150,7 +150,7 @@ protected: + void disconnect(); + + /// updates connection usage history before the connection is closed +- void countFailingConnection(const ErrorState *); ++ void countFailingConnection(); + + /// If called the certificates validator will not used + void bypassCertValidator() {useCertValidator_ = false;} +diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc +index 6bd6204..b513a22 100644 +--- a/src/tests/stub_libsecurity.cc ++++ b/src/tests/stub_libsecurity.cc +@@ -97,7 +97,7 @@ void PeerConnector::bail(ErrorState *) STUB + void PeerConnector::sendSuccess() STUB + void PeerConnector::callBack() STUB + void PeerConnector::disconnect() STUB +-void PeerConnector::countFailingConnection(const ErrorState *) STUB ++void PeerConnector::countFailingConnection() STUB + void PeerConnector::recordNegotiationDetails() STUB + EncryptorAnswer &PeerConnector::answer() STUB_RETREF(EncryptorAnswer) + } diff --git a/squid.spec b/squid.spec index dbdcc3a..fea08a9 100644 --- a/squid.spec +++ b/squid.spec @@ -3,7 +3,7 @@ Name: squid Version: 6.13 -Release: 1%{?dist} +Release: 2%{?dist} Summary: The Squid proxy caching server Epoch: 7 # See CREDITS for breakdown of non GPLv2+ code @@ -26,7 +26,12 @@ Source98: perl-requires-squid.sh # Upstream patches # Backported patches -# Patch101: patch +# Upstream PR: https://github.com/squid-cache/squid/pull/1442 +Patch101: squid-6.1-crash-half-closed.patch +# Upstream PR: https://github.com/squid-cache/squid/pull/1914 +Patch102: squid-6.11-ignore-wsp-after-chunk-size.patch +# Upstream commit: https://github.com/squid-cache/squid/commit/022dbabd89249f839d1861aa87c1ab9e1a008a47 +Patch103: squid-6.13-cache-peer-connect-errors.patch # Local patches # Applying upstream patches first makes it less likely that local patches @@ -37,10 +42,6 @@ Patch203: squid-6.1-perlpath.patch # revert this upstream patch - https://bugzilla.redhat.com/show_bug.cgi?id=1936422 # workaround for #1934919 Patch204: squid-6.1-symlink-lang-err.patch -# Upstream PR: https://github.com/squid-cache/squid/pull/1442 -Patch205: squid-6.1-crash-half-closed.patch -# Upstream PR: https://github.com/squid-cache/squid/pull/1914 -Patch206: squid-6.11-ignore-wsp-after-chunk-size.patch # cache_swap.sh Requires: bash gawk @@ -315,6 +316,9 @@ fi %changelog +* Wed Mar 12 2025 Luboš Uhliarik - 7:6.13-2 +- Do not blame cache_peer for 4xx CONNECT responses + * Tue Feb 04 2025 Luboš Uhliarik - 7:6.13-1 - new version 6.13