Re: [PATCH] meta_header option

On 13/10/2012 5:00 a.m., Alex Rousskov wrote:
> On 10/12/2012 01:06 AM, Amos Jeffries wrote:
>>>> I got a request to implement stacked tags in external ACL the other day.
>>>> It would seem they are suited well for combining with this feature. How
>>>> about "tag" or "meta_tag" ?
>>> I assume stacked tags are key:value pairs. Today, tags are essentially
>>> anonymous or valueless annotations (values without keys or keys without
>>> values). Please correct me if I am wrong.
>> You are correct. I consider them as a key of "tag" and a value since
>> they arrive from the helper as "tag=X".
> "tag":value is a good way to map current tags provided we want to
> support having two distinct key:value pairs with the same key. The
> proposed code supports that.
>
>
>> The no-replace semantics from
>> secondary helpers or tags given is a small problem, which we will need
>> to work around. But that can remain isolated to external ACL code for
>> now until people are used to the.
>> I was going to use these meta-X items as stacked tags and allow the
>> admin to pick their own arbitrary key name.
> If I understand the "stacked tags" feature correctly, we would need to
> add something like this to the external acl keywords support:
>
> note=<key>:<value>

This syntax just looks nasty. I''m not talking about any inteface
passing a toke "note". I'm talking about documenting them as "notes"
amnd squid.conf diretive name being meta_note.
   As in:
     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")

We already have key-value coming out of the helpers, so why nest it?
make them all these meta things and helper syntax stays the same
simple: key=<value> , constraining key names as the valid charset of
HTTP header names ?
  The internal delimiter parsing helper channels being '=' and ICAP/eCAP
channels being ':' or ': '.

>
> and leave the old "tag=value" as a deprecated way of saying
>
> note=tag:<value>
>
> There should be some limits on what keys can be, of course. For example,
> they must not contain HTTP delimiters.
>
> Note that there is nothing "stacked" about these annotations in the
> proposed implementation. They are treated as HTTP extension #value
> headers with a "comma-separated list" value syntax (with key being the
> header name), essentially. For a given key, the values are listed in the
> arrival order. There is no specific order among different keys.
>
> Unlike HTTP #value headers, key:value duplicates are ignored.
>
>
>>> The reason I did not include "tag" in the above names is because I was
>>> worried that preserving compatibility with the existing tags will create
>>> a mess. Here are a few potentially messy areas:
>> I'm not so worried. Elsewhere the concept of tagging things is common.
>> Squid is the weird case where we only allow one tag per transaction (so
>> far).
> Oh, I do think that the annotation/tagging concept itself is very
> useful! I am just worried (more than you are) about us trying to mix the
> new and old features, each having its own little nuances that may not
> mash together in corner cases.

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.

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

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.

>
>>> Overall, I agree that it would be nice to have one name/concept for all
>>> annotations, but reusing "tag"s may create a mess that will either
>>> significantly delay the new feature acceptance or will cause
>>> upgrade problems. It may be better to leave "tag"s to old configs and
>>> standardize on new "note"s going forward (e.g., add them to external
>>> ACLs). What do you think?
>> I think its not as bad as you think. But agree to avoid it for now.
> OK, so here is the summary of the required changes for the proposed
> meta_header patch:
>
> 1. Rename proposed "meta_header" directive to "note".
> 2. Rename proposed "%meta_header" logformat code to "%note".
> 3. Enforce restrictions on characters in keys if not already
> enforced.

+1.

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

Amos
Received on Sat Oct 13 2012 - 02:25:27 MDT