Re: [PATCH] HelperReply upgrade stage 3

On 11/24/2012 08:27 AM, Amos Jeffries wrote:

> This stage of the helper reply protocol adds kv-pair support to the
> url_rewrite_helper interfacefor URL redirect and rewriteoperations.
>
> It uses the new Notes objects and kv-pair field added by the stage 2
> helper protocol instead of parsing the 'other' field.
>
>
> The response syntax for URL helpers becomes:
> [channel-ID SP ] result [ SP kv-pair ... ] [ SP other] EOL
>
> * 'other' field is now deprecated and will be ignored/discarded on any
> response containing a result code field.
>
>
> When result code "OK" is presented by the helper several kv-pairs are
> reserved to control Squid actions:
>
> * "rewrite-url=" is added to return a re-written URL.
> - When this key is presented the URL is re-written to the new one by
> Squid without client interaction.
> - The 'url' keys presence will override this key.
>
> * "url=" is added to return the redirected-to URL.
> - When this key name is presented an HTTP redirect response is
> generated for the client.
> - This keys presence overrides the 'rewrite-url' key actions.
>
> * "status=" is added to hold the HTTP status code for use in redirect
> operations.
> - This field is optional and status is no longer required for marking
> redirect actions.
> - If no redirect status is provided Squid will assign one (currently
> the default is 302, that may change in the future).
> - This key is only relevant when 'url' key is also presented. In all
> other uses it is currently ignored.

> When result codes BH or ERR are presented by the helper no redirect or
> rewrite action is performed and no kv-pair key names are reserved for
> use at this time.

but any key=value pairs in a BH or ERR response are still parsed into
notes and can be logged, right?

Can we tell folks which key names are guaranteed not to clash with
future Squid key names? For example, we can say that no Squid-supported
key name will end with "_". Or we could recommend using helper-specific
prefixes for custom key names instead.

N.B. We should not disclaim using X-* names because some of those (e.g.,
X-Client-Username) are a de facto standard and might be used by Squid
here eventually.

Please note that helper annotations will _not_ be sent to adaptation
services.

> Any other keys MAY be sent on any response. The URL helper interface
> makes no other use of them, but this patch does pass them on to the ALE
> object for logging as transaction Notes. All kv-pairs returned by the
> helper (including the url, stauts rewrite-url keys) are available for
> logging via the %{...}note log format option.

What about the "note" ACL? Can it be used to match against
helper-provided annotations? Or will the helper annotations be
inaccessible to it? Please document one way or another.

If the note ACL will not work for helper annotations, we may need
"stage4". It may be better to fix that now instead, but that is your
call provided you will make it work.

> + switch(reply.result) {
> + case HelperReply::Unknown:
> + case HelperReply::TT:
> + debugs(85, DBG_IMPORTANT, "ERROR: URL rewrite helper returned invalid result code. Wrong helper? " << reply);
> + break;
> +

Please add a debugs() line to print reply.result before the above switch
unless we print that elsewhere already. Not all cases inside the switch
have debugs() lines so it would be nice to know what the switch value was.

> + debugs(85, DBG_IMPORTANT, "ERROR: URL rewrite helper: " << reply);

Please add "redirect_fail_count" value to the above debug line or add a
new debug line for it. It seems critical in understanding what would
happen after the error.

> +// XXX update to handle the new format responses...
> +// #1: redirect with a specific status code OK status=NNN url="..."
> +// #2: redirect with a default status code OK url="..."
> +// #3: re-write the URL OK rewrite-url="..."

based on the patch description, the above is already implemented, no?

> + char * result = const_cast<char*>(statusNote->values[0]->value.termedBuf());
> + status = (http_status) atoi(result);

Please make result constant if possible.

> + char * result = const_cast<char*>(reply.other().content());
> + http_status status = (http_status) atoi(result);

Please make both result and status constant if possible.

> + char * result = const_cast<char*>(statusNote->values[0]->value.termedBuf());

> + http->redirect.location = xstrdup(urlNote->values[0]->value.termedBuf());

> + if (urlNote != NULL && strcmp(urlNote->values[0]->value.termedBuf(), http->uri)) {

xstrdup() calls exit() on NULL strings. strcmp() might dump core. The
above lines may assume that termedBuf() never returns NULL. Please
double check that "value" member cannot be an "undefined" String in
these contexts. It is probably prudent to check for that centrally, when
parsing annotations, if we are going to rely on that -- refuse to add
undefined Strings to annotation values.

> + Notes metaNotes; // collection of meta notes associated with this request.

I find it ironic that you insisted on renaming "meta headers" but are
now adding "meta notes" :-). Please consider using "notes" or
"annotations" instead. The annotations are already "meta".

The description seem to imply that metaNotes contains all annotations
associated with this request, but I do not think this is true because
some annotations are stored elsewhere. This is messy so let's document
exactly which annotations are using this particular storage (I think it
is currently used just for helper annotations, right?).

Finally, I am concerned that we are adding Notes to all HttpRequests
when most HttpRequest objects do not need annotations. Currently, Notes
are implemented using Vector<> which has low performance overhead when
unused. I am worried that if we later replace Vector<> with std::vector
or similar, the inclusion of Notes will cause a performance hit. What do
you think? Should we use a pointer to Notes here instead?

> + * Adds a set of notes from another notes list to this set.
> + * Create any new keys needed.
> + * If the key name already exists in list, add the given value to its set of values.
> + */
> + void add(const Notes &src);

Please polish the description to indicate that pointers to src notes
and/or pointers to individual note values are added. Modifying src after
add() may change the added notes as well!

> + uint8_t redirect_fail_count;

New member missing a doxygen description.

> --- src/HttpRequest.cc 2012-10-27 00:13:19 +0000
> +++ src/HttpRequest.cc 2012-11-24 14:55:43 +0000
> @@ -114,6 +114,7 @@
> peer_domain = NULL; // not allocated/deallocated by this class
> vary_headers = NULL;
> myportname = null_string;
> + metaNotes.clean();
> tag = null_string;

HttpRequest::init() does not need to call clean() but
HttpRequest::clean() does!

> @@ -221,6 +222,7 @@
> // XXX: what to do with copy->peer_domain?
>
> copy->myportname = myportname;
> + copy->metaNotes.notes = metaNotes.notes;
> copy->tag = tag;
> #if USE_AUTH
> copy->extacl_user = extacl_user;

Hm.. The generated Notes assignment operator will not really clone
individual notes inside the "notes" vector/array. It will just copy
pointers to them. Is that OK because we never modify a Note? I am not so
sure. Perhaps an older Note will be modified when the "note" directive
is checked, affecting cloned requests that should be independent from
it? Should we prohibit Notes assignment and add an explicit
Notes::clone() method that is guaranteed to clone Notes rather than copy
their pointers?

> + char *t = NULL;
> +
> + if ((t = strchr(result, ':')) != NULL) {

This can be merged into one line and simplified. You may also be able to
declare "t" constant.

  if (const char * t = strchr(result, ':')) { ...

HTH,

Alex.
Received on Mon Nov 26 2012 - 20:11:03 MST