Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID

On 24/06/2014 1:44 a.m., Tsantilas Christos wrote:
> On 06/23/2014 09:50 AM, Amos Jeffries wrote:
>> On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
>>> Hi all,
>>>
>>> The attached patch replaces existin annotation values with the new one
>>> received from helpers.
>>>
>>> Just one question. We are documenting key-value pairs in cf.data.pre
>>> only for url-rewriter helpers, but annotations works for all squid
>>> helpers.
>>> Should we move the related url-rewriter section to a global section? If
>>> yes where?
>>>
>>> For example something like the following in a global section should be
>>> enough:
>>>
>>> The interface for all helpers has been extended to support arbitrary
>>> lists of key=value pairs, with the syntax key=value . Some keys have
>>> special meaning to Squid, as documented here.
>>>
>>> Currently Squid understands the following optional key=value pairs
>>> received from URL rewriters:
>>> clt_conn_id=ID
>>> Associates the received ID with the client TCP connection.
>>> The clt_conn_id=ID pair is treated as a regular annotation but
>>> it persists across all transactions on the client connection
>>> rather than disappearing after the current request. A helper
>>> may update the client connection ID value during subsequent
>>> requests by returning a new ID value. To send the connection
>>> ID to the URL rewriter, use url_rewrite_extras:
>>> url_rewrite_extras clt_conn_id=%{clt_conn_id}note ...
>>>
>>
>> Design issue:
>> * can we call this "client-tag=" or "connection-tag=" or
>> "connection-label=". We have ID used internally via InstanceId<>
>> template members, which are very different to this "ID" being
>> user-assigned.
>
> The clt_conn_id is not a client tag, but it can be a connection-tag or
> connection-label.
> Lets wait Alex to answer on this.
>

See my answer to Alex. By using this note the helper is explicitly
defining that connection == 1 client.

>>
>>
>> in src/Notes.cc:
>> * replaceOrAdd iterates over the notes list twice in the event of
>> removals (due to remove() iterating over it from scratch). Please fix
>> this to iterate over the list only once and do an in-place replace if
>> the key is found already there, add if end() is reached.
>>
>> * remove() appears to be unnecessary after the above correction.
>
> I can fix a litle replaceOrAdd, but the remove() method still is needed.
>

By what code? I see it only being used by replaceOrAdd(), and only to
perform the unnecessary loop. It is fine as a remove() implementation,
but to me looks needless at this time.

> The replaceOrAdd() method method will be:
> void
> NotePairs::replaceOrAdd(const NotePairs *src)
> {
> for (std::vector<NotePairs::Entry *>::const_iterator i =
> src->entries.begin(); i != src->entries.end(); ++i) {
> remove((*i)->name.termedBuf());
> entries.push_back(new NotePairs::Entry((*i)->name.termedBuf(),
> (*i)->value.termedBuf()));
> }
> }
>
>>
>>
>> in Notes.*:
>> * findFirst() no longer makes sense. There being no "second" now.
>> - Please rename to find().
>
> This is not true. We can still support multiple values per key name.
> I am suggesting to not touch this one.
>

Every helper callback is now using Update*() which uses replaceOrAddd()
to ensure uniqueness.

I see that the list output directly from the helper may contain
duplicates and might be used by the handler instead of the HttpRequest
merged list. We may want to change that to make handler behaviour match
behaviour of the ACL testing the HttpRequest list.

>>
>>
>> in src/client_side_request.cc:
>> * ClientRequestContext::clientRedirectDone was previously calling
>> SyncNotes to ensure the AccessLogEntry and HttpRequest shared a notes
>> list before updating the request. Order is important here because Sync
>> will kill Squid with an assert if the objects have different notes list
>> objects.
>> Until that is fixed please retain the order of:
>> - sync first, then update the request.
>>
>> * same in ClientRequestContext::clientStoreIdDone
>
> It is the same, it is not make difference.
> UpdateRequestNotes will just copy helper notes to the request notes. The
> SyncNotes will check if AccessLogEntry and HttpRequest object points to
> the same NotePairs object.

If you are going to retain it the way it is please add a note to the
AccessLogEntry::notes member indicating that it must NEVER be set other
than by calling SyncNotes() with the current transactions HttpRequest
object. Doing so will guarantee crashes after this patch goes in.

The burden will also be placed on auditors to check this with every
notes related patch. (I would rather avoid that extra work TBH).

Amos
Received on Tue Jun 24 2014 - 05:21:38 MDT