Re: [PATCH] meta_header option

On 10/12/2012 08:25 PM, Amos Jeffries wrote:

> meta_note hi "Hello world"
>
> external_acl_type ... %note{hi} ...
>
> acl world note foo "Ehlo"
>
> logformat ... %note{hi} %note{foo}
>
> (where the helper recieves "... hi=\"Hello world\" ...") and responds
> "OK foo="Ehlo")

Are you sure that it is OK to treat everything the helper returns as
annotation? We can do that, but I am a worried that it would then
require a lot of extra work (on Squid and admin parts) to prevent
certain annotations from being exposed to adaptation services and such.
For example, most admins will not want password=foo to be exposed.

I think we should separate/mark "exposable" annotations. For example,

    OK password=foo notes: key1=value1 key2=value2

or, borrowing a trick from some Unix command line interfaces:

    OK password=foo -- key1=value1 key2=value2

Note that if the admin does want to expose password, they can:

    OK password=foo -- key1=value1 password=foo

Since Squid does not interpret annotations, there is no problem with
repeating password twice in different "sections". However, we can also
teach Squid to interpret known annotations to avoid such duplication
when password exposure is desirable:

    OK -- key1=value1 password=foo

> There are only two real hangups I see ahead (the list you made earlier
> is handling events for #2):
>
> 1) searching a list of meta keys for a certain name is slower than
> loading value out of a object member field. We can optimize certain
> popular keys though to avoid that - but still get the list maintenance
> overheaders to retain the as-received ordering semantics.

IMO, annotations are meant primarily for things Squid does not know
about; things that admins introduce for their internal use. The above
examples with the password key illustrate that the interface can also be
used to expose things that Squid does know about, without any extra
effort on our part (and no significant performance penalty either).

> 2) the existing external ACL keys have no-replace / always-replace
> semantics which we have to take care of specially for certain keys.

Yes.

> Overall I see the performance hit (1) as being the bigger issue. Since
> workaround for both will likely be the same actions.
>
>>
>>> Just had another idea. "key" or "keys".
>> Since each annotation contains both a key and a value, it seems wrong to
>> name the whole concept "key".
>
> When you abstract a key into a value of another key (ie
> key1=(key2:value2), yes it gets horrible. I'm not in favour of doing that.
> Plan to have the key2 passed by name to the helper and the value2 as its
> value and the abstraction issues all disappear along with the conceptual
> problems it brings in.

Sure, but I am thinking about things other than helper responses.

    note group Group1 !CONNECT fromGroup1

is bearable (although "annotate" for the directive name would be better
here), but

    key group Group1 !CONNECT fromGroup1

looks pretty bad IMO because there is more than just key being set here.

>> Going forward, if somebody wants to support "stacked tags" using
>> external ACLs, they will:
>>
>> 1. Add a "note=key:value" keyword support to external ACLs
>> 2. Add a new "note" ACL type that will match key:value annotations.
>> 3. Adjust external ACL code to store tags as tag:<value> annotations,
>> using annotations API added by the meta_headers patch.
>> 4. Adjust "tag" ACL type code to look for tag:<value> annotations,
>> using annotations API added by the meta_headers patch.
>> 5. Make sure that external ACL annotations are not sent to adaptation
>> services (enabling such sending may be a separate project).
>>
>> Any objections to this plan?

> Given the "stacked" definition as being the ability to add multiple
> identically named keys with different values.
>
> If you want to leave off the helper API portion of this feature, fine I
> will bundle it in as the next stage of the new HelperReply upgrade
> feature. (nearly ready for re-submit just trying to figure out a new
> "squid -z" crash in trunk that stops me testing it in production).

Sounds good. The proposed patch provides internal API that future code
can use to add support for helper-provided annotations. This will allow
us to introduce immediately useful/needed feature now, with a plan on
how to expand its coverage [to helpers] in a consistent way.

Thank you,

Alex.
Received on Sat Oct 13 2012 - 04:21:39 MDT