Re: [PATCH] Deprecate log_icap and log_access configuration directives

On 12/06/2013 1:36 a.m., Tsantilas Christos wrote:
> On 06/11/2013 02:14 PM, Amos Jeffries wrote:
>> On 11/06/2013 10:16 p.m., Tsantilas Christos wrote:
>>> The log_icap and log_access are not really needed.
>>> The users have acls control for access and icap logging using the
>>> access_log and icap_log configuration directives.
>>>
>>> This patch removes these options from configuration file.
>>>
>>>
>>> This is a Measurement Factory project
>> Overall I like it. Removing code and a frequent source of user confusion
>> in one patch.
>>
>> Please also note in the descriptive message that this alters the
>> documented behaviour of "Requests denied for logging will also not be
>> accounted for in performance counters.", and now makes all traffic get
>> performane counters accounting.
> Are we OK with this?

I am okay with the change. It just needs documenting so we don't
overlook it in the release documentation.

+1 from me.

>> + this also goes for the src/client_side.cc checklist.
> well this is not possible here...

Hmm. Okay. We will need to get to that eventually. For now the skip is okay.

>
>> - is it really necessary to send dash_str as the IDENT username? doing
>> so completely breaks 'ident' ACLs on icap_log.
> Yes. This is true. My sense is that using the dash_str as IDENT
> username, just used to speedup the fast acl checking.
>
> In client_side.cc code inside clientAclChecklistCreate function it is
> used as follows:
> - If it is already exist in ConnStateData::clientConnection::rfc931
> then use it else do not spend time to find it (from cache or lookup).
>
> Probably this logic should be moved inside ident acl, or let ident acl
> to do its job. I had this dilemma when started this patch, but at the
> end I let it untouched.

The ident ACL already operates like that if the ConnStateData is
available. That is usually setup from the HttpRequest on checklist creation.
The problem is that ident ACL uses the explicit value from checklist
creation to override the ConnStateData value lookup, so using dash_str
makes it always non-match.

Amos
Received on Wed Jun 12 2013 - 07:12:46 MDT