Re: [PATCH] adaptation_meta option

On 10/28/2011 11:02 AM, Tsantilas Christos wrote:
> On 10/28/2011 02:57 AM, Amos Jeffries wrote:
>> On 28/10/11 00:38, Tsantilas Christos wrote:
>>> On 10/22/2011 04:03 AM, Amos Jeffries wrote:
>>>> On 22/10/11 03:03, Tsantilas Christos wrote:
>>>>> This option allows Squid administrator to add custom ICAP request
>>>>> headers or eCAP options to Squid ICAP requests or eCAP transactions.
>>>>> Use it to pass custom authentication tokens and other
>>>>> transaction-state
>>>>> related meta information to an ICAP/eCAP service.
>>>>>
>>>>> The addition of a meta header is ACL-driven:
>>>>> adaptation_meta name value [!]aclname ...
>>>>>
>>>>> Processing for a given header name stops after the first ACL list
>>>>> match.
>>>>> Thus, it is impossible to add two headers with the same name. If no
>>>>> ACL
>>>>> lists match for a given header name, no such header is added. For
>>>>> example:
>>>>>
>>>>> # do not debug transactions except for those that need debugging
>>>>> adaptation_meta X-Debug 1 needs_debugging
>>>>>
>>>>> # log all transactions except for those that must remain secret
>>>>> adaptation_meta X-Log 1 !keep_secret
>>>>>
>>>>> # mark transactions from users in the "G 1" group
>>>>> adaptation_meta X-Authenticated-Groups "G 1" authed_as_G1
>>>>>
>>>>> The "value" parameter may be a regular squid.conf token or a "double
>>>>> quoted string". Within the quoted string, use backslash (\) to escape
>>>>> any character, which is currently only useful for escaping backslashes
>>>>> and double quotes. For example,
>>>>> "this string has one backslash (\\) and two \"quotes\""
>>>>>
>>>>>
>>>>> This is a Measurement Factory project
>>>>
>>>
>>> I am sending a new patch here, which is also updated to apply to the
>>> latest trunk.
>>>
>>>>
>>>> Cool. I like.
>>>>
>>>> I just have one doubt on the design end of things. Can we limit it to
>>>> not overwriting or adding standard headers? rather than just warning.
>>>> Adding custom headers and ones needed for future protocol extensions is
>>>> all fine and well. But wrongly adding central protocol headers midway
>>>> down a chain is an attractive trap newbie admin are always asking about
>>>> how to do, even if though breaks the final result.
>>>
>>> Do we have any decision here? Should we deny overwriting standard
>>> headers?
>>>
>>
>> +1. This can go in if you want to.
>>
>> I am still of the opinion that we should not allow that until someone
>> comes up with a proper need case. But not strongly enough to veto the
>> change. I'm okay with the critical-level warnings.
>>
>> An alternative is to followup with another patch adding an
>> --enable-icap-violations option to configure and wrapping some warn vs
>> reject code in #if ICAP_VIOLATIONS.
>
> This is a new version which add the "--enable-icap-violations" option.
>
> I think it is OK. If we do not like we can convert it to a
> "--disable-icap-violations" option :-)

Hi Christos,

    Sorry, I really do not like another VIOLATIONS macro because while
it is simple now it is likely to bloat with more code in the future.
Moreover, as we discussed, not all user-configurable headers with
standard names are necessarily ICAP violations.

I would very much rather just leave the fatalf() code and remove the
macro. This will match Amos preferences. While it may not match yours
and mine, I think we do not have strong ones, and I really hate seeing
another VIOLATIONS macro added.

If you agree, please commit with the fatalf() code and without
USE_ICAP_VIOLATIONS.

Thank you,

Alex.
Received on Fri Oct 28 2011 - 17:16:50 MDT