From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Date: Tue, 20 Mar 2018 22:53:12 +0000 (+0000)
Subject: Support selective CF: collapsed_forwarding_access (#151)
X-Git-Tag: M-staged-PR161~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=819be28411d53ebb9f1c6c17fa5979b9b9568277;p=thirdparty%2Fsquid.git

Support selective CF: collapsed_forwarding_access (#151)

The new directive controls whether individual requests (including
ICP/HTCP and revalidation requests) should participate in collapsed
forwarding. Admins want to limit collapsed forwarding because it carries
significant transaction-specific risks (and benefits!).

Squid default behavior is unchanged.

Only fast ACLs are supported.
---

diff --git a/src/ICP.h b/src/ICP.h
index 52a9d597bb..e51ea13404 100644
--- a/src/ICP.h
+++ b/src/ICP.h
@@ -14,6 +14,7 @@
  \ingroup ServerProtocol
  */
 
+#include "base/RefCount.h"
 #include "comm/forward.h"
 #include "icp_opcode.h"
 #include "ip/Address.h"
@@ -21,8 +22,11 @@
 #include "store_key_md5.h"
 #include "StoreClient.h"
 
+class AccessLogEntry;
 class HttpRequest;
 
+typedef RefCount<AccessLogEntry> AccessLogEntryPointer;
+
 /**
  * Wire-level ICP header.
  * DO NOT add or move fields.
@@ -57,18 +61,27 @@ public:
  \ingroup ServerProtocolICPAPI
  \todo mempool this
  */
-class ICPState
+class ICPState: public StoreClient
 {
 
 public:
     ICPState(icp_common_t &aHeader, HttpRequest *aRequest);
     virtual ~ICPState();
+
+    /// whether the found entry warrants an ICP_HIT response
+    bool foundHit(const StoreEntry &) const;
+
     icp_common_t header;
     HttpRequest *request;
     int fd;
 
     Ip::Address from;
     char *url;
+
+protected:
+    /* StoreClient API */
+    virtual void fillChecklist(ACLFilledChecklist &) const override;
+    mutable AccessLogEntryPointer al;
 };
 
 /// \ingroup ServerProtocolICPAPI
@@ -101,13 +114,13 @@ HttpRequest* icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from);
 bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request);
 
 /// \ingroup ServerProtocolICPAPI
-void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from);
+void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from, AccessLogEntryPointer);
 
 /// \ingroup ServerProtocolICPAPI
 icp_opcode icpGetCommonOpcode();
 
 /// \ingroup ServerProtocolICPAPI
-int icpUdpSend(int, const Ip::Address &, icp_common_t *, const LogTags &, int);
+int icpUdpSend(int, const Ip::Address &, icp_common_t *, const LogTags &, int, AccessLogEntryPointer);
 
 /// \ingroup ServerProtocolICPAPI
 LogTags icpLogFromICPCode(icp_opcode opcode);
@@ -124,9 +137,6 @@ PF icpUdpSendQueue;
 /// \ingroup ServerProtocolICPAPI
 void icpHandleIcpV3(int, Ip::Address &, char *, int);
 
-/// \ingroup ServerProtocolICPAPI
-int icpCheckUdpHit(StoreEntry *, HttpRequest * request);
-
 /// \ingroup ServerProtocolICPAPI
 void icpOpenPorts(void);
 
diff --git a/src/SquidConfig.h b/src/SquidConfig.h
index 82126304bc..19233f47a7 100644
--- a/src/SquidConfig.h
+++ b/src/SquidConfig.h
@@ -399,6 +399,7 @@ public:
 
         acl_access *forceRequestBodyContinuation;
         acl_access *serverPconnForNonretriable;
+        acl_access *collapsedForwardingAccess;
     } accessList;
     AclDenyInfoList *denyInfoList;
 
diff --git a/src/Store.h b/src/Store.h
index cf33b5aedb..b65ce68911 100644
--- a/src/Store.h
+++ b/src/Store.h
@@ -188,6 +188,10 @@ public:
     /// whether there is a corresponding locked shared memory table entry
     bool hasMemStore() const { return mem_obj && mem_obj->memCache.index >= 0; }
 
+    /// whether this is a collapsed forwarding-created public entry that still
+    /// has not received its response headers; new requests may collapse on it
+    bool collapsingInitiator() const;
+
     MemObject *mem_obj;
     RemovalPolicyNode repl;
     /* START OF ON-DISK STORE_META_STD TLV field */
@@ -221,9 +225,7 @@ public:
     static void getPublicByRequest(StoreClient * aClient, HttpRequest * request);
     static void getPublic(StoreClient * aClient, const char *uri, const HttpRequestMethod& method);
 
-    virtual bool isNull() {
-        return false;
-    };
+    virtual bool isNull() const { return false; } // TODO: Replace with nullptr.
 
     void *operator new(size_t byteCount);
     void operator delete(void *address);
@@ -324,9 +326,6 @@ class NullStoreEntry:public StoreEntry
 
 public:
     static NullStoreEntry *getInstance();
-    bool isNull() {
-        return true;
-    }
 
     const char *getMD5Text() const;
     HttpReply const *getReply() const { return NULL; }
@@ -334,6 +333,8 @@ public:
 
     bool isEmpty () const {return true;}
 
+    /* StoreEntry API */
+    virtual bool isNull() const { return true; }
     virtual size_t bytesWanted(Range<size_t> const aRange, bool) const { return aRange.end; }
 
     void operator delete(void *address);
diff --git a/src/StoreClient.h b/src/StoreClient.h
index ec06c3891f..8c9f093837 100644
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -16,14 +16,34 @@
 typedef void STCB(void *, StoreIOBuffer);   /* store callback */
 
 class StoreEntry;
+class ACLFilledChecklist;
 
+/// A StoreEntry::getPublic*() caller.
 class StoreClient
 {
 
 public:
     virtual ~StoreClient () {}
 
-    virtual void created (StoreEntry *newEntry) = 0;
+    // TODO: Remove? Probably added to make lookups asynchronous, but they are
+    // still blocking. A lot more is needed to support async callbacks.
+    /// Handle a StoreEntry::getPublic*() result.
+    /// An isNull() entry indicates a cache miss.
+    virtual void created(StoreEntry *) = 0;
+
+protected:
+    /// configure the ACL checklist with the current transaction state
+    virtual void fillChecklist(ACLFilledChecklist &) const = 0;
+
+    // These methods only interpret Squid configuration. Their allowances are
+    // provisional -- other factors may prevent collapsed forwarding. The first
+    // two exist primarily to distinguish two major CF cases in callers code.
+    /// whether Squid configuration allows us to become a CF initiator
+    bool mayInitiateCollapsing() const { return onCollapsingPath(); }
+    /// whether Squid configuration allows collapsing on the initiatorEntry
+    bool mayCollapseOn(const StoreEntry &initiatorEntry) const;
+    /// whether Squid configuration allows collapsing for this transaction
+    bool onCollapsingPath() const;
 };
 
 #if USE_DELAY_POOLS
diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index e4db02c522..495cd235c4 100644
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -218,9 +218,15 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re
     rfc931[0] = '\0';
 
     changeAcl(A);
+    setRequest(http_request);
+    setIdent(ident);
+}
 
-    if (http_request != NULL) {
-        request = http_request;
+void ACLFilledChecklist::setRequest(HttpRequest *httpRequest)
+{
+    assert(!request);
+    if (httpRequest) {
+        request = httpRequest;
         HTTPMSGLOCK(request);
 #if FOLLOW_X_FORWARDED_FOR
         if (Config.onoff.acl_uses_indirect_client)
@@ -233,8 +239,13 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re
         if (request->clientConnectionManager.valid())
             conn(request->clientConnectionManager.get());
     }
+}
 
+void
+ACLFilledChecklist::setIdent(const char *ident)
+{
 #if USE_IDENT
+    assert(!rfc931[0]);
     if (ident)
         xstrncpy(rfc931, ident, USER_IDENT_SZ);
 #endif
diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h
index 9455a6ceea..3c1b0d4d17 100644
--- a/src/acl/FilledChecklist.h
+++ b/src/acl/FilledChecklist.h
@@ -38,6 +38,11 @@ public:
     ACLFilledChecklist(const acl_access *, HttpRequest *, const char *ident = nullptr);
     ~ACLFilledChecklist();
 
+    /// configure client request-related fields for the first time
+    void setRequest(HttpRequest *);
+    /// configure rfc931 user identity for the first time
+    void setIdent(const char *userIdentity);
+
 public:
     /// The client connection manager
     ConnStateData * conn() const;
diff --git a/src/cf.data.pre b/src/cf.data.pre
index 784231439b..9fb2229203 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -6581,6 +6581,36 @@ DOC_START
        disk or memory caches and for Vary-controlled cached objects.
 DOC_END
 
+NAME: collapsed_forwarding_access
+TYPE: acl_access
+DEFAULT: none
+DEFAULT_DOC: Requests may be collapsed if collapsed_forwarding is on.
+LOC: Config.accessList.collapsedForwardingAccess
+DOC_START
+	Use this directive to restrict collapsed forwarding to a subset of
+	eligible requests. The directive is checked for regular HTTP
+	requests, internal revalidation requests, and HTCP/ICP requests.
+
+		collapsed_forwarding_access allow|deny [!]aclname ...
+
+	This directive cannot force collapsing. It has no effect on
+	collapsing unless collapsed_forwarding is 'on', and all other
+	collapsing preconditions are satisfied.
+
+	* A denied request will not collapse, and future transactions will
+	  not collapse on it (even if they are allowed to collapse).
+
+	* An allowed request may collapse, or future transactions may
+	  collapse on it (provided they are allowed to collapse).
+
+	This directive is evaluated before receiving HTTP response headers
+	and without access to Squid-to-peer connection (if any).
+
+	Only fast ACLs are supported.
+
+	See also: collapsed_forwarding.
+DOC_END
+
 NAME: collapsed_forwarding_shared_entries_limit
 COMMENT: (number of entries)
 TYPE: int64_t
diff --git a/src/client_side.cc b/src/client_side.cc
index e72b692d3f..d2cbf30862 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3722,16 +3722,25 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
 ACLFilledChecklist *
 clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http)
 {
+    const auto checklist = new ACLFilledChecklist(acl, nullptr, nullptr);
+    clientAclChecklistFill(*checklist, http);
+    return checklist;
+}
+
+void
+clientAclChecklistFill(ACLFilledChecklist &checklist, ClientHttpRequest *http)
+{
+    checklist.setRequest(http->request);
+    checklist.al = http->al;
+
+    // TODO: If http->getConn is always http->request->clientConnectionManager,
+    // then call setIdent() inside checklist.setRequest(). Otherwise, restore
+    // USE_IDENT lost in commit 94439e4.
     ConnStateData * conn = http->getConn();
-    ACLFilledChecklist *ch = new ACLFilledChecklist(acl, http->request,
-            cbdataReferenceValid(conn) && conn != NULL && conn->clientConnection != NULL ? conn->clientConnection->rfc931 : dash_str);
-    ch->al = http->al;
-    /*
-     * hack for ident ACL. It needs to get full addresses, and a place to store
-     * the ident result on persistent connections...
-     */
-    /* connection oriented auth also needs these two lines for it's operation. */
-    return ch;
+    const char *ident = (cbdataReferenceValid(conn) &&
+                         conn && conn->clientConnection) ?
+                        conn->clientConnection->rfc931 : dash_str;
+    checklist.setIdent(ident);
 }
 
 bool
diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 2eb1dfbdf9..e89dc2b9b4 100644
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -297,14 +297,22 @@ clientReplyContext::processExpired()
     saveState();
 
     // TODO: support collapsed revalidation for Vary-controlled entries
-    const bool collapsingAllowed = Config.onoff.collapsed_forwarding &&
-                                   !Store::Root().smpAware() &&
-                                   http->request->vary_headers.isEmpty();
+    bool collapsingAllowed = Config.onoff.collapsed_forwarding &&
+                             !Store::Root().smpAware() &&
+                             http->request->vary_headers.isEmpty();
 
     StoreEntry *entry = nullptr;
     if (collapsingAllowed) {
-        if ((entry = storeGetPublicByRequest(http->request, ksRevalidation)))
-            entry->lock("clientReplyContext::processExpired#alreadyRevalidating");
+        if (const auto e = storeGetPublicByRequest(http->request, ksRevalidation)) {
+            if (e->collapsingInitiator() && mayCollapseOn(*e)) {
+                entry = e;
+                entry->lock("clientReplyContext::processExpired#alreadyRevalidating");
+            } else {
+                e->abandon(__FUNCTION__);
+                // assume mayInitiateCollapsing() would fail too
+                collapsingAllowed = false;
+            }
+        }
     }
 
     if (entry) {
@@ -316,7 +324,8 @@ clientReplyContext::processExpired()
                                  http->log_uri, http->request->flags, http->request->method);
         /* NOTE, don't call StoreEntry->lock(), storeCreateEntry() does it */
 
-        if (collapsingAllowed && Store::Root().allowCollapsing(entry, http->request->flags, http->request->method)) {
+        if (collapsingAllowed && mayInitiateCollapsing() &&
+                Store::Root().allowCollapsing(entry, http->request->flags, http->request->method)) {
             debugs(88, 5, "allow other revalidation requests to collapse on " << *entry);
             collapsedRevalidation = crInitiator;
         } else {
@@ -1756,6 +1765,14 @@ clientReplyContext::identifyFoundObject(StoreEntry *newEntry)
         return;
     }
 
+    if (e->collapsingInitiator() && !mayCollapseOn(*e)) {
+        debugs(85, 3, "prohibited CF MISS " << *e);
+        forgetHit();
+        http->logType = LOG_TCP_MISS;
+        doGetMoreData();
+        return;
+    }
+
     debugs(85, 3, "default HIT " << *e);
     http->logType = LOG_TCP_HIT;
     doGetMoreData();
@@ -2255,6 +2272,12 @@ clientReplyContext::sendMoreData (StoreIOBuffer result)
     return;
 }
 
+void
+clientReplyContext::fillChecklist(ACLFilledChecklist &checklist) const
+{
+    clientAclChecklistFill(checklist, http);
+}
+
 /* Using this breaks the client layering just a little!
  */
 void
@@ -2277,9 +2300,10 @@ clientReplyContext::createStoreEntry(const HttpRequestMethod& m, RequestFlags re
     // Make entry collapsable ASAP, to increase collapsing chances for others,
     // TODO: every must-revalidate and similar request MUST reach the origin,
     // but do we have to prohibit others from collapsing on that request?
-    if (Config.onoff.collapsed_forwarding && reqFlags.cachable &&
+    if (reqFlags.cachable &&
             !reqFlags.needValidation &&
-            (m == Http::METHOD_GET || m == Http::METHOD_HEAD)) {
+            (m == Http::METHOD_GET || m == Http::METHOD_HEAD) &&
+            mayInitiateCollapsing()) {
         // make the entry available for future requests now
         (void)Store::Root().allowCollapsing(e, reqFlags, m);
     }
diff --git a/src/client_side_reply.h b/src/client_side_reply.h
index 391360f8b7..4fd7aa2849 100644
--- a/src/client_side_reply.h
+++ b/src/client_side_reply.h
@@ -99,6 +99,9 @@ public:
     clientStreamNode *ourNode;  /* This will go away if/when this file gets refactored some more */
 
 private:
+    /* StoreClient API */
+    virtual void fillChecklist(ACLFilledChecklist &) const;
+
     clientStreamNode *getNextNode() const;
     void makeThisHead();
     bool errorInStream(StoreIOBuffer const &result, size_t const &sizeToProcess)const ;
diff --git a/src/client_side_request.h b/src/client_side_request.h
index a70f972594..d3a6ec7529 100644
--- a/src/client_side_request.h
+++ b/src/client_side_request.h
@@ -194,6 +194,7 @@ private:
 char *clientConstructTraceEcho(ClientHttpRequest *);
 
 ACLFilledChecklist *clientAclChecklistCreate(const acl_access * acl,ClientHttpRequest * http);
+void clientAclChecklistFill(ACLFilledChecklist &, ClientHttpRequest *);
 int clientHttpRequestStatus(int fd, ClientHttpRequest const *http);
 void clientAccessCheck(ClientHttpRequest *);
 
diff --git a/src/htcp.cc b/src/htcp.cc
index 3f16aec609..8c42006e72 100644
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -136,6 +136,7 @@ public:
 
     /* StoreClient API */
     void created(StoreEntry *);
+    virtual void fillChecklist(ACLFilledChecklist &) const;
 
 public:
     const char *method = nullptr;
@@ -150,6 +151,7 @@ private:
 
     Ip::Address from;
     htcpDataHeader *dhdr = nullptr;
+    mutable AccessLogEntryPointer al;
 };
 
 class htcpDetail {
@@ -253,7 +255,7 @@ static ssize_t htcpBuildTstOpData(char *buf, size_t buflen, htcpStuff * stuff);
 
 static void htcpHandleMsg(char *buf, int sz, Ip::Address &from);
 
-static void htcpLogHtcp(Ip::Address &, int, LogTags, const char *);
+static void htcpLogHtcp(Ip::Address &, int, LogTags, const char *, AccessLogEntryPointer);
 static void htcpHandleTst(htcpDataHeader *, char *buf, int sz, Ip::Address &from);
 
 static void htcpRecv(int fd, void *data);
@@ -266,6 +268,21 @@ static void htcpHandleTstRequest(htcpDataHeader *, char *buf, int sz, Ip::Addres
 
 static void htcpHandleTstResponse(htcpDataHeader *, char *, int, Ip::Address &);
 
+static void
+htcpSyncAle(AccessLogEntryPointer &al, const Ip::Address &caddr, int opcode, LogTags logcode, const char *url)
+{
+    if (!al)
+        al = new AccessLogEntry();
+    al->cache.caddr = caddr;
+    al->htcp.opcode = htcpOpcodeStr[opcode];
+    al->cache.code = logcode;
+    al->url = url;
+    // HTCP transactions do not wait
+    al->cache.start_time = current_time;
+    al->cache.trTime.tv_sec = 0;
+    al->cache.trTime.tv_usec = 0;
+}
+
 static void
 htcpHexdump(const char *tag, const char *s, int sz)
 {
@@ -930,12 +947,26 @@ htcpSpecifier::created(StoreEntry *e)
         debugs(31, 3, "htcpCheckHit: NO; entry not valid to send" );
     } else if (refreshCheckHTCP(e, checkHitRequest.getRaw())) {
         debugs(31, 3, "htcpCheckHit: NO; cached response is stale");
+    } else if (e->collapsingInitiator() && !mayCollapseOn(*e)) {
+        debugs(31, 3, "htcpCheckHit: NO; prohibited CF hit: " << *e);
     } else {
         debugs(31, 3, "htcpCheckHit: YES!?");
         hit = e;
     }
 
     checkedHit(hit);
+
+    // TODO: StoreClients must either store/lock or abandon found entries.
+    //if (!e->isNull())
+    //    e->abandon();
+}
+
+void
+htcpSpecifier::fillChecklist(ACLFilledChecklist &checklist) const
+{
+    checklist.setRequest(request.getRaw());
+    htcpSyncAle(al, from, dhdr->opcode, LOG_TAG_NONE, uri);
+    checklist.al = al;
 }
 
 static void
@@ -1082,7 +1113,7 @@ htcpHandleTstRequest(htcpDataHeader * dhdr, char *buf, int sz, Ip::Address &from
 
     if (!s) {
         debugs(31, 3, "htcpHandleTstRequest: htcpUnpackSpecifier failed");
-        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_INVALID, dash_str);
+        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_INVALID, dash_str, nullptr);
         return;
     } else {
         s->setFrom(from);
@@ -1091,13 +1122,13 @@ htcpHandleTstRequest(htcpDataHeader * dhdr, char *buf, int sz, Ip::Address &from
 
     if (!s->request) {
         debugs(31, 3, "htcpHandleTstRequest: failed to parse request");
-        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_INVALID, dash_str);
+        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_INVALID, dash_str, nullptr);
         return;
     }
 
     if (!htcpAccessAllowed(Config.accessList.htcp, s, from)) {
         debugs(31, 3, "htcpHandleTstRequest: Access denied");
-        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_DENIED, s->uri);
+        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_DENIED, s->uri, nullptr);
         return;
     }
 
@@ -1111,10 +1142,10 @@ htcpSpecifier::checkedHit(StoreEntry *e)
 {
     if (e) {
         htcpTstReply(dhdr, e, this, from);      /* hit */
-        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_HIT, uri);
+        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_HIT, uri, al);
     } else {
         htcpTstReply(dhdr, NULL, NULL, from);   /* cache miss */
-        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_MISS, uri);
+        htcpLogHtcp(from, dhdr->opcode, LOG_UDP_MISS, uri, al);
     }
 }
 
@@ -1131,7 +1162,7 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
 
     if (sz == 0) {
         debugs(31, 4, "htcpHandleClr: nothing to do");
-        htcpLogHtcp(from, hdr->opcode, LOG_UDP_INVALID, dash_str);
+        htcpLogHtcp(from, hdr->opcode, LOG_UDP_INVALID, dash_str, nullptr);
         return;
     }
 
@@ -1139,19 +1170,19 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
 
     if (!s) {
         debugs(31, 3, "htcpHandleClr: htcpUnpackSpecifier failed");
-        htcpLogHtcp(from, hdr->opcode, LOG_UDP_INVALID, dash_str);
+        htcpLogHtcp(from, hdr->opcode, LOG_UDP_INVALID, dash_str, nullptr);
         return;
     }
 
     if (!s->request) {
         debugs(31, 3, "htcpHandleTstRequest: failed to parse request");
-        htcpLogHtcp(from, hdr->opcode, LOG_UDP_INVALID, dash_str);
+        htcpLogHtcp(from, hdr->opcode, LOG_UDP_INVALID, dash_str, nullptr);
         return;
     }
 
     if (!htcpAccessAllowed(Config.accessList.htcp_clr, s, from)) {
         debugs(31, 3, "htcpHandleClr: Access denied");
-        htcpLogHtcp(from, hdr->opcode, LOG_UDP_DENIED, s->uri);
+        htcpLogHtcp(from, hdr->opcode, LOG_UDP_DENIED, s->uri, nullptr);
         return;
     }
 
@@ -1166,12 +1197,12 @@ htcpHandleClr(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from)
 
     case 1:
         htcpClrReply(hdr, 1, from); /* hit */
-        htcpLogHtcp(from, hdr->opcode, LOG_UDP_HIT, s->uri);
+        htcpLogHtcp(from, hdr->opcode, LOG_UDP_HIT, s->uri, nullptr);
         break;
 
     case 0:
         htcpClrReply(hdr, 0, from); /* miss */
-        htcpLogHtcp(from, hdr->opcode, LOG_UDP_MISS, s->uri);
+        htcpLogHtcp(from, hdr->opcode, LOG_UDP_MISS, s->uri, nullptr);
         break;
 
     default:
@@ -1577,19 +1608,14 @@ htcpClosePorts(void)
 }
 
 static void
-htcpLogHtcp(Ip::Address &caddr, int opcode, LogTags logcode, const char *url)
+htcpLogHtcp(Ip::Address &caddr, int opcode, LogTags logcode, const char *url, AccessLogEntryPointer al)
 {
-    AccessLogEntry::Pointer al = new AccessLogEntry;
     if (LOG_TAG_NONE == logcode.oldType)
         return;
     if (!Config.onoff.log_udp)
         return;
-    al->htcp.opcode = htcpOpcodeStr[opcode];
-    al->url = url;
-    al->cache.caddr = caddr;
-    al->cache.code = logcode;
-    al->cache.trTime.tv_sec = 0;
-    al->cache.trTime.tv_usec = 0;
+
+    htcpSyncAle(al, caddr, opcode, logcode, url);
     accessLogLog(al, NULL);
 }
 
diff --git a/src/icp_v2.cc b/src/icp_v2.cc
index 2b47edc708..2cba445e0e 100644
--- a/src/icp_v2.cc
+++ b/src/icp_v2.cc
@@ -50,7 +50,7 @@
 static void icpIncomingConnectionOpened(const Comm::ConnectionPointer &conn, int errNo);
 
 /// \ingroup ServerProtocolICPInternal2
-static void icpLogIcp(const Ip::Address &, const LogTags &, int, const char *, int);
+static void icpLogIcp(const Ip::Address &, const LogTags &, int, const char *, int, AccessLogEntryPointer);
 
 /// \ingroup ServerProtocolICPInternal2
 static void icpHandleIcpV2(int, Ip::Address &, char *, int);
@@ -58,6 +58,23 @@ static void icpHandleIcpV2(int, Ip::Address &, char *, int);
 /// \ingroup ServerProtocolICPInternal2
 static void icpCount(void *, int, size_t, int);
 
+static void
+icpSyncAle(AccessLogEntryPointer &al, const Ip::Address &caddr, LogTags logcode, const char *url, int len, int delay)
+{
+    if (!al)
+        al = new AccessLogEntry();
+    al->icp.opcode = ICP_QUERY;
+    al->cache.caddr = caddr;
+    al->cache.code = logcode;
+    al->url = url;
+    // XXX: move to use icp.clientReply instead
+    al->http.clientReplySz.payloadData = len;
+    al->cache.start_time = current_time;
+    al->cache.start_time.tv_sec -= delay;
+    al->cache.trTime.tv_sec = delay;
+    al->cache.trTime.tv_usec = 0;
+}
+
 /**
  \ingroup ServerProtocolICPInternal2
  * IcpQueueHead is global so comm_incoming() knows whether or not
@@ -123,12 +140,38 @@ ICPState::~ICPState()
     HTTPMSGUNLOCK(request);
 }
 
+bool
+ICPState::foundHit(const StoreEntry &e) const
+{
+    if (e.isNull())
+        return false;
+
+    if (!e.validToSend())
+        return false;
+
+    if (!Config.onoff.icp_hit_stale && refreshCheckICP(&e, request))
+        return false;
+
+    if (e.collapsingInitiator() && !mayCollapseOn(e))
+        return false;
+
+    return true;
+}
+
+void
+ICPState::fillChecklist(ACLFilledChecklist &checklist) const
+{
+    checklist.setRequest(request);
+    icpSyncAle(al, from, LOG_TAG_NONE, url, 0, 0);
+    checklist.al = al;
+}
+
 /* End ICPState */
 
 /* ICP2State */
 
 /// \ingroup ServerProtocolICPInternal2
-class ICP2State : public ICPState, public StoreClient
+class ICP2State: public ICPState
 {
 
 public:
@@ -147,13 +190,12 @@ ICP2State::~ICP2State()
 {}
 
 void
-ICP2State::created(StoreEntry *newEntry)
+ICP2State::created(StoreEntry *e)
 {
-    StoreEntry *entry = newEntry->isNull () ? NULL : newEntry;
     debugs(12, 5, "icpHandleIcpV2: OPCODE " << icp_opcode_str[header.opcode]);
     icp_opcode codeToSend;
 
-    if (icpCheckUdpHit(entry, request)) {
+    if (foundHit(*e)) {
         codeToSend = ICP_HIT;
     } else {
 #if USE_ICMP
@@ -171,7 +213,12 @@ ICP2State::created(StoreEntry *newEntry)
             codeToSend = ICP_MISS;
     }
 
-    icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from);
+    icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, al);
+
+    // TODO: StoreClients must either store/lock or abandon found entries.
+    //if (!e->isNull())
+    //    e->abandon();
+
     delete this;
 }
 
@@ -179,10 +226,8 @@ ICP2State::created(StoreEntry *newEntry)
 
 /// \ingroup ServerProtocolICPInternal2
 static void
-icpLogIcp(const Ip::Address &caddr, const LogTags &logcode, int len, const char *url, int delay)
+icpLogIcp(const Ip::Address &caddr, const LogTags &logcode, int len, const char *url, int delay, AccessLogEntry::Pointer al)
 {
-    AccessLogEntry::Pointer al = new AccessLogEntry();
-
     if (LOG_TAG_NONE == logcode.oldType)
         return;
 
@@ -194,19 +239,7 @@ icpLogIcp(const Ip::Address &caddr, const LogTags &logcode, int len, const char
     if (!Config.onoff.log_udp)
         return;
 
-    al->icp.opcode = ICP_QUERY;
-
-    al->url = url;
-
-    al->cache.caddr = caddr;
-
-    // XXX: move to use icp.clientReply instead
-    al->http.clientReplySz.payloadData = len;
-
-    al->cache.code = logcode;
-
-    al->cache.trTime.tv_sec = delay;
-
+    icpSyncAle(al, caddr, logcode, url, len, delay);
     accessLogLog(al, NULL);
 }
 
@@ -219,7 +252,7 @@ icpUdpSendQueue(int fd, void *)
     while ((q = IcpQueueHead) != NULL) {
         int delay = tvSubUsec(q->queue_time, current_time);
         /* increment delay to prevent looping */
-        const int x = icpUdpSend(fd, q->address, (icp_common_t *) q->msg, q->logcode, ++delay);
+        const int x = icpUdpSend(fd, q->address, (icp_common_t *) q->msg, q->logcode, ++delay, nullptr);
         IcpQueueHead = q->next;
         xfree(q);
 
@@ -278,7 +311,8 @@ icpUdpSend(int fd,
            const Ip::Address &to,
            icp_common_t * msg,
            const LogTags &logcode,
-           int delay)
+           int delay,
+           AccessLogEntryPointer al)
 {
     icpUdpData *queue;
     int x;
@@ -291,7 +325,7 @@ icpUdpSend(int fd,
 
     if (x >= 0) {
         /* successfully written */
-        icpLogIcp(to, logcode, len, (char *) (msg + 1), delay);
+        icpLogIcp(to, logcode, len, (char *) (msg + 1), delay, al);
         icpCount(msg, SENT, (size_t) len, delay);
         safe_free(msg);
     } else if (0 == delay) {
@@ -324,24 +358,6 @@ icpUdpSend(int fd,
     return x;
 }
 
-int
-icpCheckUdpHit(StoreEntry * e, HttpRequest * request)
-{
-    if (e == NULL)
-        return 0;
-
-    if (!e->validToSend())
-        return 0;
-
-    if (Config.onoff.icp_hit_stale)
-        return 1;
-
-    if (refreshCheckICP(e, request))
-        return 0;
-
-    return 1;
-}
-
 /**
  * This routine selects an ICP opcode for ICP misses.
  *
@@ -385,10 +401,10 @@ icpLogFromICPCode(icp_opcode opcode)
 }
 
 void
-icpCreateAndSend(icp_opcode opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from)
+icpCreateAndSend(icp_opcode opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from, AccessLogEntry::Pointer al)
 {
     icp_common_t *reply = icp_common_t::CreateMessage(opcode, flags, url, reqnum, pad);
-    icpUdpSend(fd, from, reply, icpLogFromICPCode(opcode), 0);
+    icpUdpSend(fd, from, reply, icpLogFromICPCode(opcode), 0, al);
 }
 
 void
@@ -403,7 +419,7 @@ icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd)
          */
         clientdbUpdate(from, LOG_UDP_DENIED, AnyP::PROTO_ICP, 0);
     } else {
-        icpCreateAndSend(ICP_DENIED, 0, url, reqnum, 0, fd, from);
+        icpCreateAndSend(ICP_DENIED, 0, url, reqnum, 0, fd, from, nullptr);
     }
 }
 
@@ -434,14 +450,14 @@ icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from)
 {
     if (strpbrk(url, w_space)) {
         url = rfc1738_escape(url);
-        icpCreateAndSend(ICP_ERR, 0, rfc1738_escape(url), reqnum, 0, fd, from);
+        icpCreateAndSend(ICP_ERR, 0, rfc1738_escape(url), reqnum, 0, fd, from, nullptr);
         return NULL;
     }
 
     HttpRequest *result;
     const MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initIcp);
     if ((result = HttpRequest::FromUrl(url, mx)) == NULL)
-        icpCreateAndSend(ICP_ERR, 0, url, reqnum, 0, fd, from);
+        icpCreateAndSend(ICP_ERR, 0, url, reqnum, 0, fd, from, nullptr);
 
     return result;
 
diff --git a/src/icp_v3.cc b/src/icp_v3.cc
index 344ea4e8ab..484084d64b 100644
--- a/src/icp_v3.cc
+++ b/src/icp_v3.cc
@@ -14,12 +14,13 @@
  */
 
 #include "squid.h"
+#include "acl/FilledChecklist.h"
 #include "HttpRequest.h"
 #include "ICP.h"
 #include "Store.h"
 
 /// \ingroup ServerProtocolICPInternal3
-class ICP3State : public ICPState, public StoreClient
+class ICP3State: public ICPState
 {
 
 public:
@@ -60,20 +61,23 @@ ICP3State::~ICP3State()
 {}
 
 void
-ICP3State::created(StoreEntry *newEntry)
+ICP3State::created(StoreEntry *e)
 {
-    StoreEntry *entry = newEntry->isNull () ? NULL : newEntry;
     debugs(12, 5, "icpHandleIcpV3: OPCODE " << icp_opcode_str[header.opcode]);
     icp_opcode codeToSend;
 
-    if (icpCheckUdpHit(entry, request)) {
+    if (foundHit(*e)) {
         codeToSend = ICP_HIT;
     } else if (icpGetCommonOpcode() == ICP_ERR)
         codeToSend = ICP_MISS;
     else
         codeToSend = icpGetCommonOpcode();
 
-    icpCreateAndSend (codeToSend, 0, url, header.reqnum, 0, fd, from);
+    icpCreateAndSend(codeToSend, 0, url, header.reqnum, 0, fd, from, al);
+
+    // TODO: StoreClients must either store/lock or abandon found entries.
+    //if (!e->isNull())
+    //    e->abandon();
 
     delete this;
 }
diff --git a/src/mime.cc b/src/mime.cc
index 0e9c3a4ad6..2e87011ffa 100644
--- a/src/mime.cc
+++ b/src/mime.cc
@@ -46,6 +46,7 @@ public:
 
     /* StoreClient API */
     virtual void created(StoreEntry *);
+    virtual void fillChecklist(ACLFilledChecklist &) const;
 
 private:
     SBuf icon_;
@@ -438,6 +439,13 @@ MimeIcon::created(StoreEntry *newEntry)
     debugs(25, 3, "Loaded icon " << url_);
 }
 
+void
+MimeIcon::fillChecklist(ACLFilledChecklist &) const
+{
+    // Unreachable: We never call mayInitiateCollapsing() or mayCollapseOn().
+    assert(false);
+}
+
 MimeEntry::~MimeEntry()
 {
     xfree(pattern);
diff --git a/src/neighbors.cc b/src/neighbors.cc
index c80902801a..3c04f2e8f9 100644
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -644,7 +644,7 @@ neighborsUdpPing(HttpRequest * request,
                 if (p->icp.port == echo_port) {
                     debugs(15, 4, "neighborsUdpPing: Looks like a dumb cache, send DECHO ping");
                     query = icp_common_t::CreateMessage(ICP_DECHO, 0, url, reqnum, 0);
-                    icpUdpSend(icpOutgoingConn->fd, p->in_addr, query, LOG_ICP_QUERY, 0);
+                    icpUdpSend(icpOutgoingConn->fd, p->in_addr, query, LOG_ICP_QUERY, 0, nullptr);
                 } else {
                     flags = 0;
 
@@ -654,7 +654,7 @@ neighborsUdpPing(HttpRequest * request,
 
                     query = icp_common_t::CreateMessage(ICP_QUERY, flags, url, reqnum, 0);
 
-                    icpUdpSend(icpOutgoingConn->fd, p->in_addr, query, LOG_ICP_QUERY, 0.0);
+                    icpUdpSend(icpOutgoingConn->fd, p->in_addr, query, LOG_ICP_QUERY, 0, nullptr);
                 }
             }
         }
@@ -1404,7 +1404,7 @@ peerCountMcastPeersStart(void *data)
     p->mcast.id = mem->id;
     reqnum = icpSetCacheKey((const cache_key *)fake->key);
     query = icp_common_t::CreateMessage(ICP_QUERY, 0, url, reqnum, 0);
-    icpUdpSend(icpOutgoingConn->fd, p->in_addr, query, LOG_ICP_QUERY, 0);
+    icpUdpSend(icpOutgoingConn->fd, p->in_addr, query, LOG_ICP_QUERY, 0, nullptr);
     fake->ping_status = PING_WAITING;
     eventAdd("peerCountMcastPeersDone",
              peerCountMcastPeersDone,
diff --git a/src/store.cc b/src/store.cc
index 2272da7049..341abb140d 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -2071,6 +2071,15 @@ StoreEntry::describeTimestamps() const
     return buf;
 }
 
+bool
+StoreEntry::collapsingInitiator() const
+{
+    if (!publicKey())
+        return false;
+    return EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) ||
+           (hasTransients() && !hasMemStore() && !hasDisk());
+}
+
 static std::ostream &
 operator <<(std::ostream &os, const Store::IoStatus &io)
 {
diff --git a/src/store_client.cc b/src/store_client.cc
index 95b0011721..b825401110 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 90    Storage Manager Client-Side Interface */
 
 #include "squid.h"
+#include "acl/FilledChecklist.h"
 #include "event.h"
 #include "globals.h"
 #include "HttpReply.h"
@@ -44,6 +45,38 @@ static bool CheckQuickAbortIsReasonable(StoreEntry * entry);
 
 CBDATA_CLASS_INIT(store_client);
 
+/* StoreClient */
+
+bool
+StoreClient::onCollapsingPath() const
+{
+    if (!Config.onoff.collapsed_forwarding)
+        return false;
+
+    if (!Config.accessList.collapsedForwardingAccess)
+        return true;
+
+    ACLFilledChecklist checklist(Config.accessList.collapsedForwardingAccess, nullptr, nullptr);
+    fillChecklist(checklist);
+    return checklist.fastCheck().allowed();
+}
+
+bool
+StoreClient::mayCollapseOn(const StoreEntry &e) const
+{
+    assert(e.collapsingInitiator()); // our result is not meaningful for regular hits
+    return onCollapsingPath();
+}
+
+void
+StoreClient::fillChecklist(ACLFilledChecklist &checklist) const
+{
+    // TODO: Consider moving all CF-related methods into a new dedicated class.
+    Must(!"mayCollapse() caller must override fillChecklist()");
+}
+
+/* store_client */
+
 bool
 store_client::memReaderHasLowerOffset(int64_t anOffset) const
 {
diff --git a/src/tests/stub_icp.cc b/src/tests/stub_icp.cc
index b2366c5615..4cf29124df 100644
--- a/src/tests/stub_icp.cc
+++ b/src/tests/stub_icp.cc
@@ -7,6 +7,7 @@
  */
 
 #include "squid.h"
+#include "AccessLogEntry.h"
 #include "comm/Connection.h"
 #include "ICP.h"
 
@@ -20,6 +21,8 @@ icp_common_t *icp_common_t::CreateMessage(icp_opcode opcode, int flags, const ch
 icp_opcode icp_common_t::getOpCode() const STUB_RETVAL(ICP_INVALID)
 ICPState::ICPState(icp_common_t &aHeader, HttpRequest *aRequest) STUB
 ICPState::~ICPState() STUB
+bool ICPState::foundHit(const StoreEntry &) const STUB_RETVAL(false)
+void ICPState::fillChecklist(ACLFilledChecklist&) const STUB
 
 Comm::ConnectionPointer icpIncomingConn;
 Comm::ConnectionPointer icpOutgoingConn;
@@ -27,13 +30,12 @@ Ip::Address theIcpPublicHostID;
 
 HttpRequest* icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from) STUB_RETVAL(NULL)
 bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) STUB_RETVAL(false)
-void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from) STUB
+void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from, AccessLogEntryPointer) STUB
 icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID)
-int icpUdpSend(int, const Ip::Address &, icp_common_t *, LogTags, int) STUB_RETVAL(0)
+int icpUdpSend(int, const Ip::Address &, icp_common_t *, LogTags, int, AccessLogEntryPointer) STUB_RETVAL(0)
 LogTags icpLogFromICPCode(icp_opcode opcode) STUB_RETVAL(LOG_TAG_NONE)
 void icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) STUB
 void icpHandleIcpV3(int, Ip::Address &, char *, int) STUB
-int icpCheckUdpHit(StoreEntry *, HttpRequest * request) STUB_RETVAL(0)
 void icpConnectionsOpen(void) STUB
 void icpConnectionShutdown(void) STUB
 void icpConnectionClose(void) STUB
diff --git a/src/urn.cc b/src/urn.cc
index a5fb7e482b..68f2acc126 100644
--- a/src/urn.cc
+++ b/src/urn.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 52    URN Parsing */
 
 #include "squid.h"
+#include "acl/FilledChecklist.h"
 #include "cbdata.h"
 #include "errorpage.h"
 #include "FwdState.h"
@@ -53,6 +54,9 @@ public:
     int reqofs;
 
 private:
+    /* StoreClient API */
+    virtual void fillChecklist(ACLFilledChecklist &) const;
+
     char *urlres;
 };
 
@@ -182,15 +186,23 @@ UrnState::start(HttpRequest * r, StoreEntry * e)
 }
 
 void
-UrnState::created(StoreEntry *newEntry)
+UrnState::fillChecklist(ACLFilledChecklist &checklist) const
 {
-    urlres_e = newEntry;
+    checklist.setRequest(request.getRaw());
+}
 
-    if (urlres_e->isNull()) {
+void
+UrnState::created(StoreEntry *e)
+{
+    if (e->isNull() || (e->collapsingInitiator() && !mayCollapseOn(*e))) {
         urlres_e = storeCreateEntry(urlres, urlres, RequestFlags(), Http::METHOD_GET);
         sc = storeClientListAdd(urlres_e, this);
         FwdState::fwdStart(Comm::ConnectionPointer(), urlres_e, urlres_r.getRaw());
+        // TODO: StoreClients must either store/lock or abandon found entries.
+        //if (!e->isNull())
+        //    e->abandon();
     } else {
+        urlres_e = e;
         urlres_e->lock("UrnState::created");
         sc = storeClientListAdd(urlres_e, this);
     }