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

I split the initial patch to two patches. The request_header_manglers
patch should applied first.

On 06/29/2012 10:44 AM, Amos Jeffries wrote:
> 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).

Is it so simple? I remember it was not when I tried to use Format
interface. Also I have the feeling that we will need to change the
Format interface to correctly support this project...

I need to look on it again if we decide to proceed with this changes.

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

Except that the new "format" option may not needed, I agree with you
that we should add formated parameters in x_header_replace and acls
support. But this is a separate project...

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

OK, I split the patches.

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

fixed

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

fixed

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

fixed

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

fixed

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

I used Alex's version:
  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.

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

OK. I create the header file HttpHeaderTools.h
Maybe we need to move HeadersWithAcls and HeaderManglers to structs.h
again, to be applied to squid-3.2

>
>
> Amos
>
>