Hello,
Attached is an updated patch with Alex's suggested changes, i.e.
moving the use of httpHdrAdd into httpHdrMangleList, and moving some
code out of httpHdrMangle into httpHdrMangleList.
I've opted to remove Config2.onoff.mangle_request_headers completely.
I found keeping it around and making use of it in httpHdrMangleList
made the flow of execution confusing, with regards to ensuring
httpHdrAdd was still called.
Thank you for taking the time to look at this.
Nathan.
On 16 March 2016 at 14:15, Alex Rousskov
<[email protected]> wrote:
> On 03/15/2016 07:29 PM, Nathan Hoad wrote:
>> On 15 March 2016 at 12:11, Alex Rousskov wrote:
>>> On 03/14/2016 05:46 PM, Nathan Hoad wrote:
>
>>> * 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.
>
>
> Thank you. After those improvements, you can probably see a new pattern
> emerging:
>
>> if (Config2.onoff.mangle_request_headers)
>> httpHdrMangleList(hdr_out, request, ROR_REQUEST);
>>
>> httpHdrAdd(hdr_out, request, al, Config.request_header_add);
>
> and
>
>
>> httpHdrMangleList(&rep->header, http->request, ROR_REPLY);
>> httpHdrAdd(&rep->header, http->request, http->al, Config.reply_header_add);
>
> and
>
>> httpHdrMangleList(hdr, request, ROR_REPLY);
>>
>> httpHdrAdd(hdr, request, http->al, Config.reply_header_add);
>
> and
>
>> $ bzr grep ' httpHdrMangleList' | wc -l
>> 3
>
> and, I would imagine,
>
>> bzr grep ' httpHdrAdd' | wc -l
>> 3
>
>
> Yes, there are only three cases, and in all of them, we apply the same
> set of two operations. Thus, you can now move the httpHdrAdd() call into
> httpHdrMangleList()!
>
> If you do that, move the code that gets the right Config.*_header_access
> mangler from the beginning of httpHdrMangle() to httpHdrMangleList()
> itself and adjust it to also check Config2.onoff.mangle_request_headers
> during ROR_REQUEST.
>
> Adjust as needed: httpHdrMangle() is a private/static function and you
> will be working with all three httpHdrMangleList() calls in existence
> (AFAICT).
>
>
> I cannot require these additional changes from you, but if you need an
> extra incentive/motivation, then just look at the loop in
> httpHdrMangleList() and what happens at the start of httpHdrMangle()
> that the loop calls on every iteration. Consider the common case of no
> Config.*_header_access manglers configured. See how httpHdrMangleList()
> always iterates through all header fields while repeating the same
> checks and doing nothing useful at all?
>
> With the above changes (that move those checks to be done once, in front
> of the loop, and that will entirely eliminate looping in the common
> case), you would be making Squid a tiny bit faster despite adding a new
> feature!
>
>
> Thank you,
>
> Alex.
>
=== modified file 'src/HttpHeaderTools.cc'
--- src/HttpHeaderTools.cc 2016-01-24 17:41:43 +0000
+++ src/HttpHeaderTools.cc 2016-03-16 05:23:58 +0000
@@ -40,6 +40,7 @@
#include <string>
static void httpHeaderPutStrvf(HttpHeader * hdr, Http::HdrType id, const char *fmt, va_list vargs);
+static void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer &al, HeaderWithAclList &headersAdd);
void
httpHeaderMaskInit(HttpHeaderMask * mask, int value)
@@ -267,29 +268,12 @@
* \retval 1 Header has no access controls to test
*/
static int
-httpHdrMangle(HttpHeaderEntry * e, HttpRequest * request, int req_or_rep)
+httpHdrMangle(HttpHeaderEntry * e, HttpRequest * request, HeaderManglers *hms, int req_or_rep)
{
int retval;
- /* check with anonymizer tables */
- HeaderManglers *hms = NULL;
assert(e);
- if (ROR_REQUEST == req_or_rep) {
- hms = Config.request_header_access;
- } else if (ROR_REPLY == req_or_rep) {
- hms = Config.reply_header_access;
- } else {
- /* error. But let's call it "request". */
- hms = Config.request_header_access;
- }
-
- /* manglers are not configured for this message kind */
- if (!hms) {
- debugs(66, 7, "no manglers configured for message kind " << req_or_rep);
- return 1;
- }
-
const headerMangler *hm = hms->find(*e);
/* mangler or checklist went away. default allow */
@@ -323,18 +307,40 @@
/** Mangles headers for a list of headers. */
void
-httpHdrMangleList(HttpHeader * l, HttpRequest * request, int req_or_rep)
+httpHdrMangleList(HttpHeader *l, HttpRequest *request, const AccessLogEntryPointer &al, req_or_rep_t req_or_rep)
{
HttpHeaderEntry *e;
HttpHeaderPos p = HttpHeaderInitPos;
- int headers_deleted = 0;
- while ((e = l->getEntry(&p)))
- if (0 == httpHdrMangle(e, request, req_or_rep))
- l->delAt(p, headers_deleted);
-
- if (headers_deleted)
- l->refreshMask();
+ /* check with anonymizer tables */
+ HeaderManglers *hms = nullptr;
+
+ HeaderWithAclList *headersAdd = nullptr;
+
+ switch (req_or_rep) {
+ case ROR_REQUEST:
+ hms = Config.request_header_access;
+ headersAdd = Config.request_header_add;
+ break;
+ case ROR_REPLY:
+ hms = Config.reply_header_access;
+ headersAdd = Config.reply_header_add;
+ break;
+ }
+
+ if (hms != nullptr) {
+ int headers_deleted = 0;
+ while ((e = l->getEntry(&p)))
+ if (0 == httpHdrMangle(e, request, hms, req_or_rep))
+ l->delAt(p, headers_deleted);
+
+ if (headers_deleted)
+ l->refreshMask();
+ }
+
+ if (headersAdd != nullptr && !headersAdd->empty()) {
+ httpHdrAdd(l, request, al, *headersAdd);
+ }
}
static
=== modified file 'src/HttpHeaderTools.h'
--- src/HttpHeaderTools.h 2016-01-01 00:12:18 +0000
+++ src/HttpHeaderTools.h 2016-03-16 04:52:50 +0000
@@ -10,6 +10,7 @@
#define SQUID_HTTPHEADERTOOLS_H
#include "acl/forward.h"
+#include "enums.h"
#include "format/Format.h"
#include "HttpHeader.h"
@@ -119,7 +120,7 @@
const char *getStringPrefix(const char *str, size_t len);
-void httpHdrMangleList(HttpHeader *, HttpRequest *, int req_or_rep);
+void httpHdrMangleList(HttpHeader *, HttpRequest *, const AccessLogEntryPointer &al, req_or_rep_t req_or_rep);
#endif
=== modified file 'src/SquidConfig.h'
--- src/SquidConfig.h 2016-03-12 20:27:35 +0000
+++ src/SquidConfig.h 2016-03-16 05:22:12 +0000
@@ -463,6 +463,8 @@
HeaderManglers *reply_header_access;
///request_header_add access list
HeaderWithAclList *request_header_add;
+ ///reply_header_add access list
+ HeaderWithAclList *reply_header_add;
///note
Notes notes;
char *coredump_dir;
@@ -545,7 +547,6 @@
public:
struct {
int enable_purge;
- int mangle_request_headers;
} onoff;
uid_t effectiveUserID;
gid_t effectiveGroupID;
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2016-03-01 09:58:44 +0000
+++ src/cache_cf.cc 2016-03-16 05:21:45 +0000
@@ -808,8 +808,6 @@
// TODO: replace with a dedicated "purge" ACL option?
Config2.onoff.enable_purge = (ACLMethodData::ThePurgeCount > 0);
- Config2.onoff.mangle_request_headers = (Config.request_header_access != NULL);
-
if (geteuid() == 0) {
if (NULL != Config.effectiveUser) {
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2016-03-12 20:27:35 +0000
+++ src/cf.data.pre 2016-03-16 00:58:07 +0000
@@ -109,6 +109,21 @@
${service_name} expands into the current Squid service instance
name identifier which is provided by -n on the command line.
+ Logformat Macros
+
+ Logformat macros can be used in many places outside of the logformat
+ directive. In theory, all of the logformat codes can be used as %macros,
+ where they are supported. In practice, a %macro expands as a dash (-) when
+ the transaction does not yet have enough information and a value is needed.
+
+ There is no definitive list of what tokens are available at the various
+ stages of the transaction.
+
+ And some information may already be available to Squid but not yet
+ committed where the macro expansion code can access it (report
+ such instances!). The macro will be expanded into a single dash
+ ('-') in such cases. Not all macros have been tested.
+
COMMENT_END
# options still not yet ported from 2.7 to 3.x
@@ -6086,20 +6101,47 @@
string format is used, then the surrounding quotes are removed
while escape sequences and %macros are processed.
- In theory, all of the logformat codes can be used as %macros.
- However, unlike logging (which happens at the very end of
- transaction lifetime), the transaction may not yet have enough
- information to expand a macro when the new header value is needed.
- And some information may already be available to Squid but not yet
- committed where the macro expansion code can access it (report
- such instances!). The macro will be expanded into a single dash
- ('-') in such cases. Not all macros have been tested.
-
One or more Squid ACLs may be specified to restrict header
injection to matching requests. As always in squid.conf, all
ACLs in an option ACL list must be satisfied for the insertion
to happen. The request_header_add option supports fast ACLs
only.
+
+ See also: reply_header_add.
+DOC_END
+
+NAME: reply_header_add
+TYPE: HeaderWithAclList
+LOC: Config.reply_header_add
+DEFAULT: none
+DOC_START
+ Usage: reply_header_add field-name field-value acl1 [acl2] ...
+ Example: reply_header_add X-Client-CA "CA=%ssl::>cert_issuer" all
+
+ This option adds header fields to outgoing HTTP responses (i.e., response
+ headers delivered by Squid to the client). This option has no effect on
+ cache hit detection. The equivalent adaptation vectoring point in
+ ICAP terminology is post-cache RESPMOD. This option does not apply to
+ successful CONNECT replies.
+
+ Field-name is a token specifying an HTTP header name. If a
+ standard HTTP header name is used, Squid does not check whether
+ the new header conflicts with any existing headers or violates
+ HTTP rules. If the response to be modified already contains a
+ field with the same name, the old field is preserved but the
+ header field values are not merged.
+
+ Field-value is either a token or a quoted string. If quoted
+ string format is used, then the surrounding quotes are removed
+ while escape sequences and %macros are processed.
+
+ One or more Squid ACLs may be specified to restrict header
+ injection to matching responses. As always in squid.conf, all
+ ACLs in an option ACL list must be satisfied for the insertion
+ to happen. The reply_header_add option supports fast ACLs
+ only.
+
+ See also: request_header_add.
DOC_END
NAME: note
=== modified file 'src/client_side_reply.cc'
--- src/client_side_reply.cc 2016-03-11 18:00:51 +0000
+++ src/client_side_reply.cc 2016-03-16 04:06:03 +0000
@@ -1578,7 +1578,7 @@
/* TODO: else case: drop any controls intended specifically for our surrogate ID */
}
- httpHdrMangleList(hdr, request, ROR_REPLY);
+ httpHdrMangleList(hdr, request, http->al, ROR_REPLY);
}
void
=== modified file 'src/enums.h'
--- src/enums.h 2016-01-01 00:12:18 +0000
+++ src/enums.h 2016-03-16 04:35:43 +0000
@@ -180,10 +180,10 @@
} digest_read_state_t;
/* Distinguish between Request and Reply (for header mangling) */
-enum {
+typedef enum {
ROR_REQUEST,
ROR_REPLY
-};
+} req_or_rep_t;
/* CygWin & Windows NT Port */
#if _SQUID_WINDOWS_
=== modified file 'src/http.cc'
--- src/http.cc 2016-03-11 18:00:51 +0000
+++ src/http.cc 2016-03-16 04:24:28 +0000
@@ -83,8 +83,6 @@
static void httpMaybeRemovePublic(StoreEntry *, Http::StatusCode);
static void copyOneHeaderFromClientsideRequestToUpstreamRequest(const HttpHeaderEntry *e, const String strConnection, const HttpRequest * request,
HttpHeader * hdr_out, const int we_do_ranges, const HttpStateFlags &);
-//Declared in HttpHeaderTools.cc
-void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer &al, HeaderWithAclList &headers_add);
HttpStateData::HttpStateData(FwdState *theFwdState) :
AsyncJob("HttpStateData"),
@@ -1942,11 +1940,7 @@
}
/* Now mangle the headers. */
- if (Config2.onoff.mangle_request_headers)
- httpHdrMangleList(hdr_out, request, ROR_REQUEST);
-
- if (Config.request_header_add && !Config.request_header_add->empty())
- httpHdrAdd(hdr_out, request, al, *Config.request_header_add);
+ httpHdrMangleList(hdr_out, request, al, ROR_REQUEST);
strConnection.clean();
}
=== modified file 'src/servers/Http1Server.cc'
--- src/servers/Http1Server.cc 2016-01-24 17:41:43 +0000
+++ src/servers/Http1Server.cc 2016-03-16 04:07:21 +0000
@@ -276,11 +276,13 @@
void
Http::One::Server::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call)
{
+ const ClientHttpRequest *http = pipeline.front()->http;
+
// apply selected clientReplyContext::buildReplyHeader() mods
// it is not clear what headers are required for control messages
rep->header.removeHopByHopEntries();
rep->header.putStr(Http::HdrType::CONNECTION, "keep-alive");
- httpHdrMangleList(&rep->header, pipeline.front()->http->request, ROR_REPLY);
+ httpHdrMangleList(&rep->header, http->request, http->al, ROR_REPLY);
MemBuf *mb = rep->pack();
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
