[squid-dev] [PATCH] Add reply_header_add

[Alex Rousskov]

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

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

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

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

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



> +    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);



Thank you,

Alex.