From 7468e9abe0947fb1ca975cf2330b8dca0ae993a9 Mon Sep 17 00:00:00 2001 From: Walid Boudebouda Date: Fri, 11 Oct 2024 12:01:25 +0200 Subject: [PATCH] varnishncsa: Change matching rules to reflect reality 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: #3528 --- bin/varnishncsa/varnishncsa.c | 42 ++++++-- bin/varnishtest/tests/u00020.vtc | 163 +++++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 bin/varnishtest/tests/u00020.vtc diff --git a/bin/varnishncsa/varnishncsa.c b/bin/varnishncsa/varnishncsa.c index 4497126d12..d18d2104d5 100644 --- a/bin/varnishncsa/varnishncsa.c +++ b/bin/varnishncsa/varnishncsa.c @@ -165,10 +165,21 @@ static struct ctx { } CTX; 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)) + static void parse_format(const char *format); static void @@ -889,7 +900,8 @@ frag_fields(enum update_mode um, const char *b, const char *e, ...) assert(p != NULL && q != NULL); if (p >= e || q <= p) continue; - if (frag->gen != CTX.gen || um == UM_ALWAYS) { + if ((frag->gen != CTX.gen && um == UM_IF_NOT_SET) + || um == UM_ALWAYS) { /* We only grab the same matching field once */ frag->gen = CTX.gen; frag->b = p; @@ -902,6 +914,9 @@ frag_fields(enum update_mode um, const char *b, const char *e, ...) static void frag_line(enum update_mode um, const char *b, const char *e, struct fragment *f) { + if (um == UM_NEVER) + return; + if (f->gen == CTX.gen && um == UM_IF_NOT_SET) /* We only grab the same matching record once */ return; @@ -1019,23 +1034,28 @@ dispatch_f(struct VSL_data *vsl, struct VSL_transaction * const pt[], break; case SLT_BereqMethod: case SLT_ReqMethod: - frag_line(UM_IF_NOT_SET, b, e, &CTX.frag[F_m]); + frag_line(UPDATE_REQ(CTX), + b, e, &CTX.frag[F_m]); break; case SLT_BereqURL: case SLT_ReqURL: p = memchr(b, '?', e - b); if (p == NULL) p = e; - frag_line(UM_IF_NOT_SET, b, p, &CTX.frag[F_U]); - frag_line(UM_IF_NOT_SET, p, e, &CTX.frag[F_q]); + frag_line(UPDATE_REQ(CTX), + b, p, &CTX.frag[F_U]); + frag_line(UPDATE_REQ(CTX), + p, e, &CTX.frag[F_q]); break; case SLT_BereqProtocol: case SLT_ReqProtocol: - frag_line(UM_IF_NOT_SET, b, e, &CTX.frag[F_H]); + frag_line(UPDATE_REQ(CTX), + b, e, &CTX.frag[F_H]); break; case SLT_BerespStatus: case SLT_RespStatus: - frag_line(UM_ALWAYS, b, e, &CTX.frag[F_s]); + frag_line(UPDATE_RESP(CTX), + b, e, &CTX.frag[F_s]); break; case SLT_BereqAcct: case SLT_ReqAcct: @@ -1066,19 +1086,21 @@ dispatch_f(struct VSL_data *vsl, struct VSL_transaction * const pt[], break; case SLT_BereqHeader: case SLT_ReqHeader: - process_hdr(&CTX.watch_reqhdr, b, e, UM_ALWAYS); + process_hdr(&CTX.watch_reqhdr, b, e, + UPDATE_REQ(CTX)); if (ISPREFIX("Authorization:", b, e, &p) && ISPREFIX("basic ", p, e, &p)) - frag_line(UM_IF_NOT_SET, p, e, + frag_line(UPDATE_REQ(CTX), p, e, &CTX.frag[F_auth]); else if (ISPREFIX("Host:", b, e, &p)) - frag_line(UM_IF_NOT_SET, p, e, + frag_line(UPDATE_REQ(CTX), p, e, &CTX.frag[F_host]); #undef ISPREFIX break; case SLT_BerespHeader: case SLT_RespHeader: - process_hdr(&CTX.watch_resphdr, b, e, UM_ALWAYS); + process_hdr(&CTX.watch_resphdr, b, e, + UPDATE_RESP(CTX)); break; case SLT_VCL_call: if (!strcasecmp(b, "recv")) { diff --git a/bin/varnishtest/tests/u00020.vtc b/bin/varnishtest/tests/u00020.vtc new file mode 100644 index 0000000000..ade3fde25b --- /dev/null +++ b/bin/varnishtest/tests/u00020.vtc @@ -0,0 +1,163 @@ +varnishtest "new varnishncsa matching rules" + +# Test things we send to the backend + +server s1 { + rxreq + txresp +} -start + +varnish v1 -vcl+backend { + + sub vcl_backend_fetch { + set bereq.http.bereqhdr = "vbf-modified"; + set bereq.method = "HEAD"; + set bereq.url = "/vbf-url?q=vbfQuerry"; + set bereq.http.Authorization = "basic dmJmOnBhc3M="; + } + + sub vcl_backend_response { + set bereq.http.bereqhdr = "vbr-modified"; + set bereq.http.notsent = "notsent"; + set bereq.method = "CONNECT"; + set bereq.url = "/vbr-url?q=vbrQuerry"; + set bereq.http.Authorization = "basic dmJyOnBhc3M="; + } + +} -start + + +client c1 { + txreq -url "/client-url?q=clientQuerry" -hdr "bereqhdr: client-header" -hdr "Authorization:basic Y2xpZW50OnBhc3M=" + rxresp +} -run + +shell { + varnishncsa -n ${v1_name} -d -b -F '%H %{bereqhdr}i %{notsent}i %m %q %U %u' > ncsa_sb.txt + + cat >expected_sb.txt <<-EOF + HTTP/1.1 vbf-modified - HEAD ?q=vbfQuerry /vbf-url vbf + EOF + diff -u expected_sb.txt ncsa_sb.txt +} + +varnish v1 -stop + +# Test things we receive from the backend + +server s1 { + rxreq + txresp -status 202 -hdr "beresp: origin" +} -start + +varnish v1 -vcl+backend { + + sub vcl_backend_response { + set beresp.http.beresp = "vbr-updated"; + set beresp.status = 200; + } + +} -start + + +client c1 { + txreq + rxresp +} -run + + +shell { + varnishncsa -n ${v1_name} -d -b -F '%s %{beresp}o' > ncsa_rb.txt + + cat >expected_rb.txt <<-EOF + 202 origin + EOF + diff -u expected_rb.txt ncsa_rb.txt +} + +varnish v1 -stop + +# Test things we send to the client + +server s1 { + rxreq + txresp -status 202 -hdr "resp: origin" +} -start + +varnish v1 -vcl+backend { + + sub vcl_backend_response { + set beresp.http.resp = "vbr-updated"; + set beresp.status = 200; + } + + sub vcl_deliver { + set resp.http.resp = "deliver-updated"; + set resp.status = 201; + set resp.http.added = "deliver"; + } + +} -start + + +client c1 { + txreq + rxresp +} -run + +shell { + varnishncsa -n ${v1_name} -d -c -F '%s %{resp}o %{added}o' > ncsa_sc.txt + + cat >expected_sc.txt <<-EOF + 201 deliver-updated deliver + EOF + diff -u expected_sc.txt ncsa_sc.txt +} + +varnish v1 -stop + +# Test things we receive from the client + +server s1 { + rxreq + txresp +} -start + +varnish v1 -vcl+backend { + + sub vcl_recv { + set req.http.reqhdr = "recv-modified"; + set req.method = "HEAD"; + set req.url = "/recv-url?q=recvQuerry"; + set req.http.Authorization = "basic cmVjdjpwYXNz"; + set req.http.notreceived = "recv"; + } + + sub vcl_hash { + set req.http.reqhdr = "hash-modified"; + set req.method = "GET"; + set req.url = "/hash-url?q=hashQuerry"; + set req.http.Authorization = "basic aGFzaDpwYXNz"; + set req.http.notreceived = "hash"; + } + +} -start + + +client c1 { + txreq -req "POST" -url "/client-url?q=clientQuerry" \ + -hdr "reqhdr: client-header" \ + -hdr "Authorization:basic Y2xpZW50OnBhc3M=" + rxresp +} -run + + +shell { + varnishncsa -n ${v1_name} -d -c -F '%H %{reqhdr}i %{notreceived}i %m %q %U %u' > ncsa_rc.txt + + cat >expected_rc.txt <<-EOF + HTTP/1.1 client-header - POST ?q=clientQuerry /client-url client + EOF + diff -u expected_rc.txt ncsa_rc.txt +} +# TODO: Handle Unset