Varnisncsa: Change matching rules to reflect reality #4213
Conversation
This is needed in order to differentiate between what was sent/received to/from the client/server and the changes made to [be]req/resp in VCL
With this change, [Be]rsp/[Be]req formats should match exactly what was received/sent from/to the backend/client without taking irrelevant VCL changes into account. Note that this is a breaking change in varnishncsa's default behavior. Refs: varnishcache#3528
| int vcl_recv_seen; | ||
| int vcl_br_seen; |
There was a problem hiding this comment.
I think I would prefer more neutral names like recd_req and recd_beresp. What tells us today that we are done receiving a request or backend response are VCL_Call logs but it could change in the future.
For example, we set X-Forwarded-For before entering vcl_recv so we could consider a new log record between the last ReqHeader coming from the client and the first ReqHeader coming from varnishd.
There was a problem hiding this comment.
Wouldn't it be better to add some new vsl tag that indicates the end of reception from a peer so that we can rely on it in varnishncsa and avoid taking headers added by the core code before vcl_recv/vcl_backend_response into account ?
There was a problem hiding this comment.
We should focus on the new model for NCSA formats here.
| enum update_mode { | ||
| UM_NEVER, | ||
| UM_IF_NOT_SET, | ||
| UM_ALWAYS | ||
| }; | ||
|
|
||
| #define UPDATE_REQ_C(CTX) (CTX.vcl_recv_seen ? UM_NEVER : UM_ALWAYS) | ||
| #define UPDATE_REQ_B(CTX) (CTX.vcl_br_seen ? UM_NEVER : UM_ALWAYS) | ||
|
|
||
| #define UPDATE_REQ(CTX) (*CTX.side == 'b' ? UPDATE_REQ_B(CTX) : UPDATE_REQ_C(CTX)) | ||
|
|
||
| #define UPDATE_RESP_C(CTX) (UM_ALWAYS) | ||
| #define UPDATE_RESP_B(CTX) (CTX.vcl_br_seen ? UM_NEVER : UM_ALWAYS) | ||
|
|
||
| #define UPDATE_RESP(CTX) (*CTX.side == 'b' ? UPDATE_RESP_B(CTX) : UPDATE_RESP_C(CTX)) |
There was a problem hiding this comment.
I think I would prefer something like this:
enum format_policy {
FMTPOL_FIRST,
FMTPOL_REQ,
FMTPOL_RESP,
FMTPOL_BEREQ,
FMTPOL_BERESP,
};Remember that you always have access to CTX to apply the policy, so we shouldn't need macros. A single function could tell you what to do:
static unsigned
process_format(enum format_policy fp)
{
/* switch (fp) => check CTX */
}Then you can use that in functions like frag_fields() to bail out early:
if (!process_format(fp))
return;It might prevent cascading enum arguments, I could only have a quick look at the patch series.
This PR introduces the changes discussed in #3528
The relevant parts of [BE]requests/responses are now shown as they were received/sent from/to the peer, without taking irrelevant VCL changes into account. This PR also introduces the handling of Unset headers in varnishncsa.
Note: this breaks the default behavior of varnishncsa.
Fixes #3528.