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

On 28/06/2012 5:29 a.m., Tsantilas Christos wrote:
> This patch:
>
> 1)add request_header_add, a new ACL-driven squid.conf option that allows
> addition of HTTP request header fields before the request is sent to the
> next HTTP hop (a peer proxy or an origin server):
> request_header_add <field-name> <field-value> acl1 [acl2] …
> where:
> * Field-name is a token specifying an HTTP header name.
> * Field-value is either a constant token or a quoted string
> containing %macros. The following field-value macros are supported:
> %LOGIN, %SRC, %DST, %user_cert_subject, %user_ca, %%

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.

NP 1: The API is in format/Format.h. You create a Format::Format(name)
object and call its parse() function on a config string. To produce the
output text call assemble() with a filled out AccessLogEntry object for
data source and a MemBuf for the output results.

NP 2: You will need to add %ssl:: tokens for the SSL related details.
That is probably best done as a separate patch update outside of these
header alterations as it will affect the log details support (in a good
way).

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.

> * One or more Squid ACLs may be specified to restrict header
> insertion to matching requests. The request_header_add option supports
> “fast” ACLs only.
>
> 2) Fix the [request|reply]_header_[access|replace] configuration
> parameters to support custom headers. Currently the user has the option
> to remove/replace/allow all custom headers using the "Other" as header
> name, but no one or some of the custom headers.
>
> This is a Measurement Factory project.
>

Just to start with. If it is not too difficult can this be split into
two patches? one patch for (1) and another for (2) the new ADD abilities.

I'd like to port the OTHER-header behaviour to 3.2 series (and 3.1)
where things are a bit too frozen to accept the new ADD directive.

Audit results:
  * skipping audit on the format parser and dumper code until the
comments above are resolved.

src/cache_cf.cc:
* chunk @73: please wrap "#include <list>" with HAVE_ macros.

* chunk @793: please wrap the boolean "request_header_access != NULL"
test in () and remove the line wrapping.

* please de-duplicate free_http_header_access() and
free_http_header_replace().
  We are using #define to de-duplicate down to shared names when things
like this need identical free_*() functions.

src/cf.data.pre:
* under documentatino for request_header_access ... s/A a list/A list/

* 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.
"

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.

Amos
Received on Fri Jun 29 2012 - 07:44:58 MDT