[squid-dev] [PATCH] Add reply_header_add

[Nathan Hoad]

On 15 March 2016 at 12:11, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 03/14/2016 05:46 PM, Nathan Hoad wrote:
>
>> The attached patch implements reply_header_add, for adding HTTP
>> headers to reply objects as they're sent to the client.
>
> Thank you for this useful addition. Unfortunately, it needs quite a bit
> of work.
>
>
> * Please _carefully_ review your src/cf.data.pre changes. There appear
> to be many copy-paste errors there, some of which seriously misleading
> (e.g., "incoming" vs. "outgoing" and "request" vs. "response").

My bad! Good catch.

>
> * You have not adjusted HTTP response headers produced by
> Http::One::Server::writeControlMsgAndCall(). Please either apply the
> same logic to those headers or explicitly document that control message
> headers are out of this option reach.

Done.

>
> * You have not adjusted HTTP response headers produced by
> tunnelConnected() and ClientHttpRequest::sslBumpStart(). Please either
> apply the same logic to those headers or explicitly document that
> successful CONNECT responses are out of this option reach.

I've opted to document that CONNECT responses aren't covered by this
option. Making it work in these places is outside my capabilities.

>
>
> I also recommend the following adjustments:
>
> * The scary "In theory, all of ..." paragraph that made a lot of sense
> for requests is barely applicable to post-cache responses, where most
> information is already available. Please consider making that text
> context-neutral and moving it to an option-independent location in the
> begging of squid.conf. That would be really useful IMO because we have
> many options using logformat codes now and that old text is applicable,
> to various degrees, to all of them.

Sure, I've added this at the top of the documentation, after SMP
Macros, and removed the paragraphs from both request_header_add and
reply_header_add. I'm not quite sure of the wording, feel free to make
suggestions for how it could be improved.

>
> * Adding a sentence or two explicitly stating that this directive does
> not affect cached responses. Your copied text says that it "has no
> affect on cache hit detection", and that is also true, if somewhat
> redundant for responses.

Done.

>
> * Adding a "See also: request_header_add" statement and the symmetrical
> statement for request_header_add.

Done.

>
>
>
>> +    if (Config.reply_header_add && !Config.reply_header_add->empty())
>> +        httpHdrAdd(hdr, request, http->al, *Config.reply_header_add);
>
>
> I know you are reusing an existing code template here, but this
> particular copy starts duplicating a lot of logic. I recommend moving
> both guards/conditions from the if-statement into httpHdrAdd() itself.
> If others object to moving both conditions on performance grounds,
> please move at least the second/empty() condition:
>
> That is, ideally:
>
>   httpHdrAdd(hdr, request, http->al, Config.reply_header_add);
>
> Or, if others object:
>
>   if (Config.reply_header_add)
>       httpHdrAdd(hdr, request, http->al, *Config.reply_header_add);

I've gone with your ideal version. If people object, I'm happy to move
to the second version.

>
>
>
> Thank you,
>
> Alex.
>


Thank you,

Nathan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reply-header-add-v2.patch
Type: text/x-patch
Size: 7645 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160316/cf1ada13/attachment.bin>