Re: [PATCH] %>la for intercepted connections

On 09/07/2011 05:03 AM, Amos Jeffries wrote:
> On Tue, 06 Sep 2011 19:04:29 +0300, Tsantilas Christos wrote:
>> I am sending a new patch which does not touch the current ">la"
>> formating code but instead adjust the "la" formatting code to follow
>> rules as discussed by Amos.
>>
>> Please look in the cf.data.per documentation and the
>> LFT_CLIENT_LOCAL_USED_IP/LFT_CLIENT_LOCAL_USED_PORT enum names I am
>> using...
>>
>
> * LFT_LOCAL_LISTENING_ would be a bit more descriptive for the enum. The
> documentation could also use the word "listening" to be clearer on where
> it comes from.
> ie:
> + la Local listening IP address the client connection uses.
> + lp Local listening port number the client connection uses.

I have my doubts that the listening IP address is what we want to log here.

Normally an administrator needs to now the IP address the client
connected to. It needs the IP address and not listening address.
In the case of intercepted connections this information does not exist
so why not get this info from listening IP address?
Moreover it is practical to have ONE formating code for the above (eg
easier to create a script for log analysis)

>
>
> * The IP address display needs a re-structure. al->tcpClient is possibly
> a backup source on non-intercepted requests if cache.port->s.IsAnyAddr()
> [only this case]. Otherwise always use cache.port->s or dash.
>
> + 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?

>
>
> * Drop all use of al->tcpClient and traffic types out of the port
> display code. To match the documentation cache.port->s should always be
> set when cache.port != NULL. If its missing from a particular path that
> needs to be known why (internally generated requests would be 0, others
> should not ever be).
>
> + case LFT_LOCAL_LISTENING_PORT:
> + if (al->cache.port) {
> + outint = al->cache.port->s.GetPort();
> + doint = 1;
> + }
> + break;

You have right here...

>
>
> * Drop the deprecation warning added for *_OLD_31 entirely now. That
> will prevent the altered codes having any effect.
>
>
> Amos
>
>>
>> On 09/01/2011 12:08 AM, Amos Jeffries wrote:
>>> %>la to always display tcpClient->local with a config documentation note
>>> about it being external IPs in intercepted traffic.
>>>
>>> %la to display cache.caddr with a config documentation note that it is
>>> the squid receiving *_port details as known by Squid (caddr also used by
>>> icp_port and htcp_port on their messages).
>>>
>>> Amos
>
>
Received on Wed Sep 07 2011 - 16:48:28 MDT