Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID
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.
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.
in Notes.*:
* findFirst() no longer makes sense. There being no "second" now.
- Please rename to find().
in src/cf.data.pre:
* "kv-pairs" is a format syntax definition used throughout the
documentation. Please retain it. If the term needs re-defining that is
an issue outside this patch scope.
* Please reword "Squid understands the following optional key=value
pairs received from URL rewriters:". Since there are optional kv-pairs
already documented in the rewrite vs redirect vs no-change syntax we
need to specify that these are optional extensions.
Perhapse this:
"In addition to the above kv-pairs Squid also understands the following
optional kv-pairs received from URL rewriters:"
* Please update the kv-pair description like so (where FOO is the term
tag or label selected to replace ID by the above mentioned design issue)
"
Associates a FOO with the client TCP connection.
The FOO is treated as a regular annotation but persists
across future requests on the client connection
rather than just the current request.
A helper may update the FOO during subsequent
requests by returning a new kv-pair.
"
* Reference to url_rewrite_extras should be directy after the helper
input syntax line. Like so:
"
[channel-ID ] URL [ extras]
see url_rewrite_extras on how to send "extras" with
optional values to the helper.
"
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
* in ClientHttpRequest::doCallouts() please use SBuf::isEmpty() instead
of length() to check for existence.
Amos
Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID
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? > The external_acl_type and store_id_program document their protocol and keys as well. Those interfaces are one where users commonly are required to create custom helpers so having it in these docs is helpful if a bit odd. The other interfaces (auth, logging, unlinkd, diskd, pinger, etc) which do not document the protocol in detail generally the Squid bundled helpers are used and the docs reference the wiki for the more advanced users. Amos
Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID
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 ...
On 06/19/2014 09:07 PM, Tsantilas Christos wrote:
On 06/16/2014 06:36 PM, Alex Rousskov wrote:
On 06/15/2014 12:07 AM, Amos Jeffries wrote:
On 15/06/2014 4:58 a.m., Alex Rousskov wrote:
On 06/11/2014 08:52 AM, Tsantilas Christos wrote:
I must also note that this patch adds an inconsistency. All annotation
key=values pairs received from helpers, accumulated to the
existing key
notes values. The clt_conn_id=Id pair is always unique and replaces
the
existing clt_conn_id=Id annotation pair.
We may want to make all annotations unique, or maybe implement a
configuration mechanism to define which annotations are overwriting
their previous values and which appending the new values.
I suggest making all annotations unique (i.e., new values overwrite old
ones) because helpers that want to accumulate annotation values can do
that by returning old values along with new ones:
received by helper: name=v1
returned by helper: name=v1,v2
Please note that the opposite does not work: If annotation values are
always accumulated, a helper cannot overwrite/remove the old value.
Doing that would mean passing all existing annotations to every helper
lookup.
Why would that mean the above?
AFAICT, the helper should get only the annotations it needs. That need
is helper-specific and, hence, is configurable via the various _extras
and equivalent directives. That is already supported and does not need
to change.
Here is the overall sketch for supporting "unique annotations":
1. Send the helper the annotations it is configured to get
(no changes here).
2. For each unique annotation key received from the helper,
remove any old annotation(s) with the same key.
3. Store annotations received from the helper
(no changes here).
To support explicit annotation deletion, we can adjust #3 to skip
key-value pairs with the value equal to '-'.
If there is not any objection I will implement this scenario.
Looks that this approach is the best and cover more cases than the
accumulated Notes values.
If someones need to accumulate Note values he can configure squid to
send old note value to helper and helper include it in its response.
This is simple.
If required in the future we can implement a configuration parameter
which configures one or more notes as always accumulated. For example:
AccumulateHelperNotes status message
HTH,
Alex.
Support client connection annotation by helpers via clt_conn_id=ID.
TCP client connections tagging is useful for faking various forms of
connection-based "authentication" when standard HTTP authentication cannot be
used. A URL rewriter or, external ACL helper may mark the "authenticated"
client connection to avoid going through "authentication" steps during
subsequent requests on the same connection and to share connection
"authentication" information with Squid ACLs, other helpers, and logs.
After this change, Squid accepts optional clt_conn_id=ID pair from a
helper and associates the received ID with the client TCP connection.
Squid treats the received clt_conn_id=ID pair as a regular annotation, but
also keeps it across all requests on the same client connection. A helper may
update the client connection ID value during subsequent requests.
To send clt_conn_id=ID pair to a URL rewriter, use url_rewrite_extras with a
%{clt_conn_id}note macro.
Also after this patch the notes comming from helpers replaces any existing
note values.
This is a Measurement Factory project
=== modified file 'src/Notes.cc'
--- src/Notes.cc 2014-04-30 09:41:25 +
+++ src/Notes.cc 2014-06-22 17:15:26 +
@@ -14,40 +14,41 @@
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published
Re: [PATCH] validate -n parameter value
On 17/06/2014 4:06 a.m., Alex Rousskov wrote:
> On 06/15/2014 12:05 AM, Amos Jeffries wrote:
>> +if (optarg) {
>> +SBuf t(optarg);
>> +::Parser::Tokenizer tok(t);
>
> Pleaser remove "t" as used-once and lacking a descriptive name. If you
> insist on keeping it, please make it "const" and try to give it a more
> descriptive name such as rawName or serviceName.
>
>> +const CharacterSet chr =
>> CharacterSet::ALPHA+CharacterSet::DIGIT;
>
> Is there a document stating that only those characters are valid? My
> quick search resulted in [1] that seems to imply many other characters
> are accepted. However, [2] lists more prohibited characters. Both seem
> to allow "-" and "_" though. It would be good to provide a reference in
> the code or commit message to explain why we are restricting its value.
>
Arbitrary design choice for guaranteed portability. Other characters can
be added if necessary, but most lack cross-platform usability for all
the situations the service name string is being used.
> Do you want to validate the total service name length? [1] says the
> limit is 256 characters.
>
> None of the above applies to -n used on Unix. Do we need to validate the
> service name (whatever that is) there?
IMO portable consistency is better for this type of thing than being
pedantically accepting for the OS-specific character sets.
Yes, length needs to be checked. Thank you.
Does an arbitrary 32 bytes seem acceptible since this is a prefix on the
UDS sockets and paths which the final total still needs to fit within
the 256 or so for some OS?
>
>> +if (!tok.prefix(service_name, chr) || !tok.atEnd())
>
> Please note that the above also rejects empty service names (-n "").
> That may be good, but the error messages do not reflect that restriction.
>
>
>> +fatal("Need to provide alphanumeric service name when
>> using -n option");
>> +opt_signal_service = true;
>> +} else {
>> +fatal("Need to provide alphanumeric service name when using
>> -n option");
>> +}
>
> I recommend adjusting the fatal() messages to distinguish the two errors
> and avoid a possible misunderstanding that the service name was provided
> but was not alphanumeric in the second case:
>
> * Expected alphanumeric service name for the -n option but got: ...
> * A service name is required for the -n option.
>
"" is not an alphanumeric string. But okay.
>
> I continue to dislike the undefined -n option meaning on non-Windows
> boxes, but I have no objection to this patch and believe the above
> changes can be done without a new review cycle.
>
>
> Cheers,
>
> Alex.
> [1] http://msdn.microsoft.com/en-us/library/ms682450%28VS.85%29.aspx
> [2] http://www.anzio.com/resources/cannot-install-or-start-windows-service
>
