Re: [PATCH] Add request_header_add option and [request|reply]_header_* manglers fixes

On 06/29/2012 01:44 AM, Amos Jeffries wrote:

> Can you please make this use the more flexible (logformat) tokens
> from format/libformat.la instead? I'm trying to kill off the older
> token sets and this is adding yet another format tokenizer to keep
> track of.

I do share the desire to unify macro parsing and substitution in various
Squid places (logging, error pages, helpers, and various squid.conf
options with string values). It will give us a lot more configuration
power and better code. However, I think that should not be placed as a
requirement on this feature because:

a) We already have several different pieces of code that do macro
substitution or implement a similar functionality. It would be best to
have a dedicated project that will unify all that code, taking into
consideration all the different code requirements we already have. This
will be a difficult, sizable project, focusing on internal code
structure rather than user-facing functionality.

b) Unifying various macro substitution pieces is likely to require
adjusting options syntax and possibly renaming some of the existing
macros. Discussing the best ways to do that will take time (especially
since those syntax discussions are not tied to any specific function),
and the final changes are likely to go way beyond this patch scope.

c) It would be easier to port this new feature to older releases (I
believe we even have a patch for v3.0) when it does not depend on newer
unified substitution/formatting code. Once backported, the new code in
trunk (and v3.2?) can go its own way, merging with other formatting
code, etc.

d) IIRC, the formatting code you ask us to reuse may not have access to
some of the information we need to substitute these macros (because we
currently do not log that information). In other words, the existing API
cannot be reused as is and may require significant modifications on its
own before it can be fully reused.

Would it be OK to require those unification changes as a follow-up project?

> EXTRA NOTE: along these grounds I was hoping to convert:
> X_header_replace Foo some random new header contents
>
> into:
> format <name> <some[ header contents format tokens]>
> X_header_replace Foo format=<name> [options] [acl acl ...]
>
> (and for backward compatibility absence of the "format=" option in the
> first token meaning old style random header text content and no ACLs.

Why? The proposed

    x_header_action <name> <value> [acl ...]

seems a lot more "natural" to me. The "format" approach makes sense
where the same named format is used multiple times in the same config. I
doubt that would be common for custom header values.

Long term, we should support regular variables so that naming and
reusing a commonly used string becomes possible (but, again, I doubt
these header mangling options would use such format variables a lot).

> src/cf.data.pre:
>
> * Please change:
> "
> If ACLs of a "deny" rule match, the header + field is removed (unless
> its value is replaced by the + corresponding request_header_replace option)
> "
> with:
> "
> If ACLs of a "deny" rule match, the header is removed and
> request_header_replace is then checked to identify if the removed header
> has a replacement.
> "

This is not what happens though, on two levels:

1) If there is a replacement, the entire header is not removed but the
current field value is replaced. This may not matter much in most cases,
but the in-place replacement may have an effect on the final order of
the header fields.

2) If there is no replacement, the entire header is not removed. Only
the current header field is removed. There may be multiple header fields
with the same header name.

Also, the alternative wording does not say what happens when "the
removed header has a replacement".

Would the following text address your concerns?

   If ACLs of a "deny" rule match, the header field is removed (unless
   its value is replaced by the value from the corresponding
   request_header_replace option).

Or the following version, perhaps?

   If ACLs of a "deny" rule match, the header field is either
   removed or replaced, depending on whether there is a
   corresponding request_header_replace option.

This is further complicated by the fact that the above text is in the
middle of another enumeration so we should keep this description one
sentence long.

The current functionality (unchanged by this patch) does not handle
headers with multiple fields well. Eventually, we may want to redo the
whole set of header manipulation options to make it more intuitive and
flexible/correct (and applied both pre- and post-cache) but that is
subject for another thread, after we are done with this feature!

> src/structs.h:
> * wrapping again with <list>, <map>, <string>
>
> * please make a new header file for the new class HeaderManglers and
> HeadersWithAcl. We are trying to remove structs.h file.

Ideally, the mangling code itself should be isolated. One of the reasons
we did not do it is to keep porting simple: adding those files to the
right set of Makefile.am blobs across Squid versions could be very
difficult in my experience. Perhaps this change should be done as a
separate step, after the code is committed (to provide an easier spin
off point for porting)?

Thank you,

Alex.
Received on Fri Jun 29 2012 - 15:53:05 MDT