Re: [PATCH] Deprecate log_icap and log_access configuration directives

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?

>
> Also, this is not exactly backward-compatible. We *do* have a number of
> installations using log_access to restrict their logging for limited
> storage spaces which will suddenly get much more log data and face DoS
> effects sometime after an upgrade. This will need to have a small code
> snippet in src/parse_cf.cc function parse_obsolete() which detects at
> minimum log_access, maybe log_icap, and uses self_destruct() to
> highlight the change.

OK.

> in src/adaptation/icap/icap_log.cc:
> * including HttpReply.h pulls in HttpMsg.h. No need to #include both.

fixed

> * FilledChecklist can be created as local variables. Please remove the
> unnecessary new/delete in icapLogLog()

done

> + this also goes for the src/client_side.cc checklist.

well this is not possible here...

>
> - 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.

In this version I just removed the dash_str from icap_log...

> This does appear to have been a problem inherited from maybeLog(), but
> now is a good time to fix it.
>
> in src/cf.data.pre:
> * directives of type "obsolete" have their DOC text turned into debugs() .
> Please replace the description with a short sentence informing the
> admin what to do to their config file. (one-liner if possible)

OK.

> * please remove the DEFAULT_DOC, DEFAULT, COMMENT, IFDEF and LOC lines
> which are now useless.

OK.

>
>
> Amos
>