]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Fix logged request size (%http::>st) and other size-related %codes.
authorEduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Fri, 16 Sep 2016 11:53:28 +0000 (23:53 +1200)
committerAmos Jeffries <squid3@treenet.co.nz>
Fri, 16 Sep 2016 11:53:28 +0000 (23:53 +1200)
The %[http::]>st logformat code should log the actual number of
[dechunked] bytes received from the client. However, for requests with
Content-Length, Squid was logging the sum of the request header size and
the Content-Length header field value instead. The logged value was
wrong when the client sent fewer than Content-Length body bytes.

Also:

* Fixed %http::>h and %http::<h format codes for icap_log. The old code
  was focusing on preserving the "request header" (i.e. not "response
  header") meaning of the %http::>h logformat code, but since ICAP
  services deal with (and log) both HTTP requests and responses, that
  focus on the HTTP message kind actually complicates ICAP logging
  configuration and will eventually require introduction of new %http
  logformat codes that would be meaningless in access.log context.

  - In ICAP context, %http::>h should log to-be-adapted HTTP message
    headers embedded in an ICAP request (HTTP request headers in REQMOD;
    HTTP response headers in RESPMOD). Before this change, %http::>h
    logged constant/"FYI" HTTP request headers in RESPMOD.

  - Similarly, %http::<h should log adapted HTTP message headers
    embedded in an ICAP response (HTTP request headers in regular
    REQMOD; HTTP response headers in RESPMOD and during request
    satisfaction in REQMOD). Before this change, %http::<h logged "-" in
    REQMOD.

  In other words, in ICAP logging context, the ">" and "<" characters
  should indicate ICAP message kind (where the being-logged HTTP message
  is embedded), not HTTP message kind, even for %http codes.

  TODO: %http::>h code logs "-" in RESPMOD because AccessLogEntry does
  not store virgin HTTP response headers.

* Correctly initialize ICAP ALE HTTP fields related to HTTP message
  sizes for icap_log, including %http::>st, %http::<st, %http::>sh, and
  %http::>sh format codes.

* Initialize HttpMsg::hdr_sz in HTTP header parsing code. Among other
  uses, this field is needed to initialize HTTP fields inside ICAP ALE.

* Synced default icap_log format documentation with the current code.

src/adaptation/icap/ModXact.cc
src/cf.data.pre
src/client_side.cc
src/format/Format.cc

index 6f154f6b37dc3b57dac9d646da09a70468f70ced..7e1158c42cc74d6f3f826b3ce83e786dddaee340 100644 (file)
@@ -1259,31 +1259,32 @@ void prepareLogWithRequestDetails(HttpRequest *, AccessLogEntry::Pointer &);
 
 void Adaptation::Icap::ModXact::finalizeLogInfo()
 {
-    HttpRequest * request_ = NULL;
-    HttpRequest * adapted_request_ = NULL;
-    HttpReply * reply_ = NULL;
-    request_ = (virgin.cause? virgin.cause: dynamic_cast<HttpRequest*>(virgin.header));
+    HttpRequest *adapted_request_ = NULL;
+    HttpReply *adapted_reply_ = NULL;
+    HttpRequest *virgin_request_ = (virgin.cause ? virgin.cause : dynamic_cast<HttpRequest*>(virgin.header));
     if (!(adapted_request_ = dynamic_cast<HttpRequest*>(adapted.header))) {
-        adapted_request_ = request_;
-        reply_ = dynamic_cast<HttpReply*>(adapted.header);
+        // if the request was not adapted, use virgin request to simplify
+        // the code further below
+        adapted_request_ = virgin_request_;
+        adapted_reply_ = dynamic_cast<HttpReply*>(adapted.header);
     }
 
-    Adaptation::Icap::History::Pointer h = (request_ ? request_->icapHistory() : NULL);
+    Adaptation::Icap::History::Pointer h = (virgin_request_ ? virgin_request_->icapHistory() : NULL);
     Must(h != NULL); // ICAPXaction::maybeLog calls only if there is a log
     al.icp.opcode = ICP_INVALID;
     al.url = h->log_uri.termedBuf();
     const Adaptation::Icap::ServiceRep  &s = service();
     al.icap.reqMethod = s.cfg().method;
 
-    al.cache.caddr = request_->client_addr;
+    al.cache.caddr = virgin_request_->client_addr;
 
-    al.request = request_;
+    al.request = virgin_request_;
     HTTPMSGLOCK(al.request);
     al.adapted_request = adapted_request_;
     HTTPMSGLOCK(al.adapted_request);
 
-    if (reply_) {
-        al.reply = reply_;
+    if (adapted_reply_) {
+        al.reply = adapted_reply_;
         HTTPMSGLOCK(al.reply);
     } else
         al.reply = NULL;
@@ -1296,25 +1297,29 @@ void Adaptation::Icap::ModXact::finalizeLogInfo()
         al.cache.ssluser = h->ssluser.termedBuf();
 #endif
     al.cache.code = h->logType;
-    // XXX: should use icap-specific counters instead ?
-    al.http.clientRequestSz.payloadData = h->req_sz;
+
+    const HttpMsg *virgin_msg = dynamic_cast<HttpReply*>(virgin.header);
+    if (!virgin_msg)
+        virgin_msg = virgin_request_;
+    assert(virgin_msg != virgin.cause);
+    al.http.clientRequestSz.header = virgin_msg->hdr_sz;
+    al.http.clientRequestSz.payloadData = virgin_msg->body_pipe->producedSize();
 
     // leave al.icap.bodyBytesRead negative if no body
     if (replyHttpHeaderSize >= 0 || replyHttpBodySize >= 0) {
         const int64_t zero = 0; // to make max() argument types the same
-        al.icap.bodyBytesRead =
-            max(zero, replyHttpHeaderSize) + max(zero, replyHttpBodySize);
+        const uint64_t headerSize = max(zero, replyHttpHeaderSize);
+        const uint64_t bodySize =  max(zero, replyHttpBodySize);
+        al.icap.bodyBytesRead = headerSize + bodySize;
+        al.http.clientReplySz.header = headerSize;
+        al.http.clientReplySz.payloadData = bodySize;
     }
 
-    if (reply_) {
-        al.http.code = reply_->sline.status();
-        al.http.content_type = reply_->content_type.termedBuf();
-        if (replyHttpBodySize >= 0) {
-            // XXX: should use icap-specific counters instead ?
-            al.http.clientReplySz.payloadData = replyHttpBodySize;
-            al.http.clientReplySz.header =  reply_->hdr_sz;
+    if (adapted_reply_) {
+        al.http.code = adapted_reply_->sline.status();
+        al.http.content_type = adapted_reply_->content_type.termedBuf();
+        if (replyHttpBodySize >= 0)
             al.cache.highOffset = replyHttpBodySize;
-        }
         //don't set al.cache.objectSize because it hasn't exist yet
 
         Packer p;
@@ -1323,7 +1328,7 @@ void Adaptation::Icap::ModXact::finalizeLogInfo()
         mb.init();
         packerToMemInit(&p, &mb);
 
-        reply_->header.packInto(&p);
+        adapted_reply_->header.packInto(&p);
         al.headers.reply = xstrdup(mb.buf);
 
         packerClean(&p);
index 4d6df80dbed8efedd011c82c932ab1df9598d1e4..639a7e2dadc4d419616ae5ba0a69f443d0a06a59 100644 (file)
@@ -4427,13 +4427,35 @@ DOC_START
        ICAP transaction log lines will correspond to a single access
        log line.
 
-       ICAP log uses logformat codes that make sense for an ICAP
-       transaction. Header-related codes are applied to the HTTP header
-       embedded in an ICAP server response, with the following caveats:
-       For REQMOD, there is no HTTP response header unless the ICAP
-       server performed request satisfaction. For RESPMOD, the HTTP
-       request header is the header sent to the ICAP server. For
-       OPTIONS, there are no HTTP headers.
+       ICAP log supports many access.log logformat %codes. In ICAP context,
+       HTTP message-related %codes are applied to the HTTP message embedded
+       in an ICAP message. Logformat "%http::>..." codes are used for HTTP
+       messages embedded in ICAP requests while "%http::<..." codes are used
+       for HTTP messages embedded in ICAP responses. For example:
+
+               http::>h        To-be-adapted HTTP message headers sent by Squid to
+                               the ICAP service. For REQMOD transactions, these are
+                               HTTP request headers. For RESPMOD, these are HTTP
+                               response headers, but Squid currently cannot log them
+                               (i.e., %http::>h will expand to "-" for RESPMOD).
+
+               http::<h        Adapted HTTP message headers sent by the ICAP
+                               service to Squid (i.e., HTTP request headers in regular
+                               REQMOD; HTTP response headers in RESPMOD and during
+                               request satisfaction in REQMOD).
+
+       ICAP OPTIONS transactions do not embed HTTP messages.
+
+       Several logformat codes below deal with ICAP message bodies. An ICAP
+       message body, if any, typically includes a complete HTTP message
+       (required HTTP headers plus optional HTTP message body). When
+       computing HTTP message body size for these logformat codes, Squid
+       either includes or excludes chunked encoding overheads; see
+       code-specific documentation for details.
+
+       For Secure ICAP services, all size-related information is currently
+       computed before/after TLS encryption/decryption, as if TLS was not
+       in use at all.
 
        The following format codes are also available for ICAP logs:
 
@@ -4447,19 +4469,16 @@ DOC_START
                icap::rm        ICAP request method (REQMOD, RESPMOD, or 
                                OPTIONS). Similar to existing rm.
 
-               icap::>st       Bytes sent to the ICAP server (TCP payload
-                               only; i.e., what Squid writes to the socket).
+               icap::>st       The total size of the ICAP request sent to the ICAP
+                               server (ICAP headers + ICAP body), including chunking
+                               metadata (if any).
 
-               icap::<st       Bytes received from the ICAP server (TCP
-                               payload only; i.e., what Squid reads from
-                               the socket).
+               icap::<st       The total size of the ICAP response received from the
+                               ICAP server (ICAP headers + ICAP body), including
+                               chunking metadata (if any).
 
-               icap::<bs       Number of message body bytes received from the
-                               ICAP server. ICAP message body, if any, usually
-                               includes encapsulated HTTP message headers and
-                               possibly encapsulated HTTP message body. The
-                               HTTP body part is dechunked before its size is
-                               computed.
+               icap::<bs       The size of the ICAP response body received from the
+                               ICAP server, excluding chunking metadata (if any).
 
                icap::tr        Transaction response time (in
                                milliseconds).  The timer starts when
@@ -4489,9 +4508,9 @@ DOC_START
        The default ICAP log format, which can be used without an explicit
        definition, is called icap_squid:
 
-logformat icap_squid %ts.%03tu %6icap::tr %>a %icap::to/%03icap::Hs %icap::<size %icap::rm %icap::ru% %un -/%icap::<A -
+logformat icap_squid %ts.%03tu %6icap::tr %>A %icap::to/%03icap::Hs %icap::<st %icap::rm %icap::ru %un -/%icap::<A -
 
-       See also: logformat, log_icap, and %adapt::<last_h 
+       See also: logformat and %adapt::<last_h
 DOC_END
 
 NAME: logfile_daemon
index 5f17984ffeb2b625e34ed717d236dde3731b7b06..0615e00628fb897288bc571de810b08f22e2595f 100644 (file)
@@ -555,8 +555,6 @@ prepareLogWithRequestDetails(HttpRequest * request, AccessLogEntry::Pointer &aLo
     aLogEntry->http.method = request->method;
     aLogEntry->http.version = request->http_ver;
     aLogEntry->hier = request->hier;
-    if (request->content_length > 0) // negative when no body or unknown length
-        aLogEntry->http.clientRequestSz.payloadData += request->content_length; // XXX: actually adaptedRequest payload size ??
     aLogEntry->cache.extuser = request->extacl_user.termedBuf();
 
     // Adapted request, if any, inherits and then collects all the stats, but
@@ -593,6 +591,9 @@ ClientHttpRequest::logRequest()
         al->cache.objectSize = loggingEntry()->contentLen(); // payload duplicate ?? with or without TE ?
 
     al->http.clientRequestSz.header = req_sz;
+    // the virgin request is saved to al->request
+    if (al->request && al->request->body_pipe != NULL)
+        al->http.clientRequestSz.payloadData = al->request->body_pipe->producedSize();
     al->http.clientReplySz.header = out.headers_sz;
     // XXX: calculate without payload encoding or headers !!
     al->http.clientReplySz.payloadData = out.size - out.headers_sz; // pretend its all un-encoded data for now.
@@ -2744,6 +2745,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
     request->my_addr = conn->clientConnection->local;
     request->myportname = conn->port->name;
     request->http_ver = http_ver;
+    request->hdr_sz = http->req_sz;
 
     // Link this HttpRequest to ConnStateData relatively early so the following complex handling can use it
     // TODO: this effectively obsoletes a lot of conn->FOO copying. That needs cleaning up later.
index d48e6703e7e71c051939658c75596fe7fad6c93a..4540a47c4f738bc124c3966df185f25b07fd5fbe 100644 (file)
@@ -309,6 +309,38 @@ log_quoted_string(const char *str, char *out)
     *p = '\0';
 }
 
+/// XXX: Misnamed. TODO: Split <h (and this function) to distinguish received
+/// headers from sent headers rather than failing to distinguish requests from responses.
+/// \retval HttpReply sent to the HTTP client (access.log and default context).
+/// \retval HttpReply received (encapsulated) from the ICAP server (icap.log context).
+/// \retval HttpRequest received (encapsulated) from the ICAP server (icap.log context).
+static const HttpMsg *
+actualReplyHeader(const AccessLogEntry::Pointer &al)
+{
+    const HttpMsg *msg = al->reply;
+#if USE_ADAPTATION
+    // al->icap.reqMethod is methodNone in access.log context
+    if (!msg && al->icap.reqMethod == Adaptation::methodReqmod)
+        msg = al->adapted_request;
+#endif
+    return msg;
+}
+
+/// XXX: Misnamed. See actualReplyHeader().
+/// \return HttpRequest or HttpReply for %http::>h.
+static const HttpMsg *
+actualRequestHeader(const AccessLogEntry::Pointer &al)
+{
+#if USE_ADAPTATION
+    // al->icap.reqMethod is methodNone in access.log context
+    if (al->icap.reqMethod == Adaptation::methodRespmod) {
+        // XXX: for now AccessLogEntry lacks virgin response headers
+        return NULL;
+    }
+#endif
+    return al->request;
+}
+
 void
 Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logSequenceNumber) const
 {
@@ -547,8 +579,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
 
         case LFT_REQUEST_HEADER:
 
-            if (al->request)
-                sb = al->request->header.getByName(fmt->data.header.header);
+            if (const HttpMsg *msg = actualRequestHeader(al))
+                sb = msg->header.getByName(fmt->data.header.header);
 
             out = sb.termedBuf();
 
@@ -567,15 +599,15 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
 
             break;
 
-        case LFT_REPLY_HEADER:
-            if (al->reply)
-                sb = al->reply->header.getByName(fmt->data.header.header);
+        case LFT_REPLY_HEADER: {
+            if (const HttpMsg *msg = actualReplyHeader(al))
+                sb = msg->header.getByName(fmt->data.header.header);
 
             out = sb.termedBuf();
 
             quote = 1;
-
-            break;
+        }
+        break;
 
 #if USE_ADAPTATION
         case LFT_ADAPTATION_SUM_XACT_TIMES:
@@ -757,8 +789,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
             break;
 #endif
         case LFT_REQUEST_HEADER_ELEM:
-            if (al->request)
-                sb = al->request->header.getByNameListMember(fmt->data.header.header, fmt->data.header.element, fmt->data.header.separator);
+            if (const HttpMsg *msg = actualRequestHeader(al))
+                sb = msg->header.getByNameListMember(fmt->data.header.header, fmt->data.header.element, fmt->data.header.separator);
 
             out = sb.termedBuf();
 
@@ -776,18 +808,27 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
 
             break;
 
-        case LFT_REPLY_HEADER_ELEM:
-            if (al->reply)
-                sb = al->reply->header.getByNameListMember(fmt->data.header.header, fmt->data.header.element, fmt->data.header.separator);
+        case LFT_REPLY_HEADER_ELEM: {
+            if (const HttpMsg *msg = actualReplyHeader(al))
+                sb = msg->header.getByNameListMember(fmt->data.header.header, fmt->data.header.element, fmt->data.header.separator);
 
             out = sb.termedBuf();
 
             quote = 1;
-
-            break;
+        }
+        break;
 
         case LFT_REQUEST_ALL_HEADERS:
-            out = al->headers.request;
+#if USE_ADAPTATION
+            if (al->icap.reqMethod == Adaptation::methodRespmod) {
+                // XXX: since AccessLogEntry::Headers lacks virgin response
+                // headers, do nothing for now
+                out = NULL;
+            } else
+#endif
+            {
+                out = al->headers.request;
+            }
 
             quote = 1;
 
@@ -802,6 +843,10 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
 
         case LFT_REPLY_ALL_HEADERS:
             out = al->headers.reply;
+#if USE_ADAPTATION
+            if (!out && al->icap.reqMethod == Adaptation::methodReqmod)
+                out = al->headers.adapted_request;
+#endif
 
             quote = 1;