I am sending the latest patch, which include Alex and Amos suggestions
On 09/08/2011 07:18 AM, Alex Rousskov wrote:
On 09/07/2011 09:38 PM, Amos Jeffries wrote:
On 08/09/11 02:21, Tsantilas Christos wrote:
On 09/07/2011 05:03 AM, Amos Jeffries wrote:
+ case LFT_LOCAL_LISTENING_IP:
+ if (al->cache.port == NULL) {
+ break; // dash
+ } else if (al->cache.port->s.IsAnyAddr()) {
+ if (al->tcpClient != NULL&&
+ !(al->request != NULL&& (al->request->flags.spoof_client_ip ||
al->request->flags.intercepted))) {
+ out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
+ } // else dash.
+ } else {
+ out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
+ }
+ break;
My original code was not good, I agree...
I have in my mind something like this:
case LFT_LOCAL_LISTENING_IP:
+ if ((al->request->flags.spoof_client_ip ||
al->request->flags.intercepted)&& al->cache.port) {
+ if (!al->cache.port->s.IsAnyAddr())
+ out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
+ } else if (al->tcpClient != NULL)
+ out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
+ break;
If it is an intercepted connection use the listening address (if it is
available), else use the IP address used by the client to connect to
squid. Is it OK?
That seems okay. Confusing to read, but is slightly more efficient than
what I suggested with the same cases covered.
To be honest, neither version is readable to an uninitiated. May I
suggest giving each condition a name and adding comments? For example
(with wrong names/comments, I am sure),
// avoid logging a dash if we have reliable info
const bool interceptedAtKnownPort =
(al->request->flags.spoof_client_ip ||
al->request->flags.intercepted)&& al->cache.port);
if (interceptedAtKnownPort) {
const bool portAddressConfigured =
!al->cache.port->s.IsAnyAddr()
if (portAddressConfigured)
... // log listening address
...
Another option would be to add methods to al->request and al->cache.port
objects instead of local variables. That way, the same potentially
useful condition can be reused throughout the code.
For the record these are the permutations we seek to cover...
Scenario 1: client 192.168.0.3 connects to google (74.125.237.81). Gets
intercepted into Squid.
1a) squid.conf: http_port 3129 intercept|tproxy
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
al->cache.port->s.local == 0.0.0.0:3129 (%la:%lp) [log "-"]
1b) squid.conf: http_port 192.168.0.1:3129 intercept|tproxy
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
al->cache.port->s.local == 192.168.0.1:3129 (%la:%lp) [log 192...]
Scenario 2: client 192.168.0.3 connects to Squid asking for
http://google.com
2a) squid.conf: http_port 3128 [accel]
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
al->cache.port->s.local == 0.0.0.0:3128 (%la:%lp) [log 192...]
2b) squid.conf: http_port 192.168.0.1:3128 [accel]
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
al->cache.port->s.local == 192.168.0.1:3128 (%la:%lp) [log 192...]
Senario 3: squid generates an internal request.
tcpClient == NULL (%>a:%>p,%>la:%>lp) [log "-"]
al->cache.port == NULL (%la:%lp) [log "-"]
Perhaps the above list should be added to the commit message in case we
need to reshuffle this code later? It sounds rather valuable to me.
Thank you,
Alex.
%la for intercepted connections
This patch adjusts the %la logformat code handling for intercepted connections
based on the following rules:
- If the corresponding http_port or https_port option has an explicit
listening host name or IP address, then log the IP address.
- Otherwise, log a dash character.
Also adjusts %lp logformat code handling for intercepted connections to always
log the port number from the corresponding http_port or https_port option.
Amos comments about %la formating code:
For the record these are the permutations we seek to cover...
Scenario 1: client 192.168.0.3 connects to google (74.125.237.81). Gets intercepted into Squid.
1a) squid.conf: http_port 3129 intercept|tproxy
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
al->cache.port->s.local == 0.0.0.0:3129 (%la:%lp) [log "-"]
1b) squid.conf: http_port 192.168.0.1:3129 intercept|tproxy
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
al->cache.port->s.local == 192.168.0.1:3129 (%la:%lp) [log 192...]
Scenario 2: client 192.168.0.3 connects to Squid asking for http://google.com
2a) squid.conf: http_port 3128 [accel]
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
al->cache.port->s.local == 0.0.0.0:3128 (%la:%lp) [log 192...]
2b) squid.conf: http_port 192.168.0.1:3128 [accel]
tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
al->cache.port->s.local == 192.168.0.1:3128 (%la:%lp) [log 192...]
Senario 3: squid generates an internal request.
tcpClient == NULL (%>a:%>p,%>la:%>lp) [log "-"]
al->cache.port == NULL (%la:%lp) [log "-"]
=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h 2011-08-20 08:21:11 +0000
+++ src/AccessLogEntry.h 2011-08-27 14:38:03 +0000
@@ -39,6 +39,7 @@
#if ICAP_CLIENT
#include "adaptation/icap/Elements.h"
#endif
+#include "ProtoPort.h"
/* forward decls */
class HttpReply;
@@ -148,6 +149,7 @@
const char *ssluser;
#endif
+ http_port_list *port;
} cache;
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2011-09-06 20:10:26 +0000
+++ src/cf.data.pre 2011-09-08 10:27:39 +0000
@@ -2901,6 +2901,9 @@
>la Local IP address the client connected to
>lp Local port number the client connected to
+ la Local listening IP address the client connection was connected to.
+ lp Local listening port number the client connection was connected to.
+
<a Server IP address of the last server or peer connection
<A Server FQDN or peer name
<p Server port number of the last server or peer connection
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2011-09-01 05:52:59 +0000
+++ src/client_side.cc 2011-09-07 16:54:34 +0000
@@ -640,7 +640,10 @@
al.cache.caddr.SetNoAddr();
- if (getConn() != NULL) al.cache.caddr = getConn()->log_addr;
+ if (getConn() != NULL) {
+ al.cache.caddr = getConn()->log_addr;
+ al.cache.port = cbdataReference(getConn()->port);
+ }
al.cache.requestSize = req_sz;
al.cache.requestHeadersSize = req_sz;
=== modified file 'src/format/Format.cc'
--- src/format/Format.cc 2011-09-06 18:28:11 +0000
+++ src/format/Format.cc 2011-09-08 10:32:37 +0000
@@ -367,14 +367,32 @@
}
break;
- case LFT_CLIENT_LOCAL_IP_OLD_31:
+ case LFT_LOCAL_LISTENING_IP: {
+ // avoid logging a dash if we have reliable info
+ const bool interceptedAtKnownPort = (al->request->flags.spoof_client_ip ||
+ al->request->flags.intercepted) && al->cache.port;
+ if (interceptedAtKnownPort) {
+ const bool portAddressConfigured = !al->cache.port->s.IsAnyAddr();
+ if (portAddressConfigured)
+ out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
+ } else if (al->tcpClient != NULL)
+ out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
+ }
+ break;
+
case LFT_CLIENT_LOCAL_IP:
if (al->tcpClient != NULL) {
out = al->tcpClient->local.NtoA(tmp,sizeof(tmp));
}
break;
- case LFT_CLIENT_LOCAL_PORT_OLD_31:
+ case LFT_LOCAL_LISTENING_PORT:
+ if (al->cache.port) {
+ outint = al->cache.port->s.GetPort();
+ doint = 1;
+ }
+ break;
+
case LFT_CLIENT_LOCAL_PORT:
if (al->tcpClient != NULL) {
outint = al->tcpClient->local.GetPort();
=== modified file 'src/format/Tokens.cc'
--- src/format/Tokens.cc 2011-08-29 11:46:04 +0000
+++ src/format/Tokens.cc 2011-09-08 10:28:44 +0000
@@ -62,9 +62,9 @@
static struct TokenTableEntry TokenTable2C[] = {
{">la", LFT_CLIENT_LOCAL_IP},
- {"la", LFT_CLIENT_LOCAL_IP_OLD_31},
+ {"la", LFT_LOCAL_LISTENING_IP},
{">lp", LFT_CLIENT_LOCAL_PORT},
- {"lp", LFT_CLIENT_LOCAL_PORT_OLD_31},
+ {"lp", LFT_LOCAL_LISTENING_PORT},
/*{ "lA", LFT_LOCAL_NAME }, */
{"<la", LFT_SERVER_LOCAL_IP},
@@ -496,16 +496,6 @@
type = LFT_HTTP_SENT_STATUS_CODE;
break;
- case LFT_CLIENT_LOCAL_IP_OLD_31:
- debugs(46, 0, "WARNING: The \"la\" formatting code is deprecated. Use the \">la\" instead.");
- type = LFT_CLIENT_LOCAL_IP;
- break;
-
- case LFT_CLIENT_LOCAL_PORT_OLD_31:
- debugs(46, 0, "WARNING: The \"lp\" formatting code is deprecated. Use the \">lp\" instead.");
- type = LFT_CLIENT_LOCAL_PORT;
- break;
-
case LFT_SERVER_LOCAL_IP_OLD_27:
debugs(46, 0, "WARNING: The \"oa\" formatting code is deprecated. Use the \"<la\" instead.");
type = LFT_SERVER_LOCAL_IP;
=== modified file 'src/format/Tokens.h'
--- src/format/Tokens.h 2011-09-06 18:28:11 +0000
+++ src/format/Tokens.h 2011-09-07 16:54:34 +0000
@@ -35,9 +35,9 @@
LFT_SERVER_PORT,
LFT_CLIENT_LOCAL_IP,
- LFT_CLIENT_LOCAL_IP_OLD_31,
+ LFT_LOCAL_LISTENING_IP,
LFT_CLIENT_LOCAL_PORT,
- LFT_CLIENT_LOCAL_PORT_OLD_31,
+ LFT_LOCAL_LISTENING_PORT,
/*LFT_LOCAL_NAME, */
LFT_SERVER_LOCAL_IP,
=== modified file 'src/log/access_log.cc'
--- src/log/access_log.cc 2011-08-21 00:12:49 +0000
+++ src/log/access_log.cc 2011-08-27 14:36:50 +0000
@@ -596,6 +596,7 @@
HTTPMSGUNLOCK(aLogEntry->icap.reply);
HTTPMSGUNLOCK(aLogEntry->icap.request);
#endif
+ cbdataReferenceDone(aLogEntry->cache.port);
}
int
