[master] c50a0d9f7 Make ESI:included requests start out in the same VCL as the top-req did.
Poul-Henning Kamp
phk at FreeBSD.org
Tue Jan 15 12:58:08 UTC 2019
commit c50a0d9f77f2affa544727d0a16fec2105dcf8c0
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date: Tue Jan 15 12:55:37 2019 +0000
Make ESI:included requests start out in the same VCL as the top-req did.
This is more complex than it sounds because the active VCL at the
time the topreq got scheduled, may no longer be by the time
the esi:include req gets processed.
Fixes: #2849
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index 8f450ee63..ce68faf59 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -454,6 +454,7 @@ struct req {
int restarts;
int esi_level;
struct req *topreq; /* esi_level == 0 request */
+ struct vcl *vcl0;
#define REQ_FLAG(l, r, w, d) unsigned l:1;
#include "tbl/req_flags.h"
diff --git a/bin/varnishd/cache/cache_esi_deliver.c b/bin/varnishd/cache/cache_esi_deliver.c
index 067d77973..ae8669f21 100644
--- a/bin/varnishd/cache/cache_esi_deliver.c
+++ b/bin/varnishd/cache/cache_esi_deliver.c
@@ -169,7 +169,10 @@ ved_include(struct req *preq, const char *src, const char *host,
req->req_body_status = REQ_BODY_NONE;
AZ(req->vcl);
- req->vcl = preq->vcl;
+ if (req->topreq->vcl0)
+ req->vcl = req->topreq->vcl0;
+ else
+ req->vcl = preq->vcl;
VCL_Ref(req->vcl);
req->req_step = R_STP_TRANSPORT;
diff --git a/bin/varnishd/cache/cache_panic.c b/bin/varnishd/cache/cache_panic.c
index db1c498bc..bf4f9dfaf 100644
--- a/bin/varnishd/cache/cache_panic.c
+++ b/bin/varnishd/cache/cache_panic.c
@@ -448,7 +448,7 @@ pan_busyobj(struct vsb *vsb, const struct busyobj *bo)
VSB_printf(vsb, "director_resp = director_req,\n");
else
VDI_Panic(bo->director_resp, vsb, "director_resp");
- VCL_Panic(vsb, bo->vcl);
+ VCL_Panic(vsb, "vcl", bo->vcl);
VSB_indent(vsb, -2);
VSB_printf(vsb, "},\n");
}
@@ -512,7 +512,8 @@ pan_req(struct vsb *vsb, const struct req *req)
if (req->resp->ws != NULL)
pan_http(vsb, "resp", req->resp);
- VCL_Panic(vsb, req->vcl);
+ VCL_Panic(vsb, "vcl", req->vcl);
+ VCL_Panic(vsb, "vcl0", req->vcl0);
if (req->body_oc != NULL)
pan_objcore(vsb, "BODY", req->body_oc);
diff --git a/bin/varnishd/cache/cache_req.c b/bin/varnishd/cache/cache_req.c
index 34c99907e..0cd80c20a 100644
--- a/bin/varnishd/cache/cache_req.c
+++ b/bin/varnishd/cache/cache_req.c
@@ -211,6 +211,7 @@ Req_Cleanup(struct sess *sp, struct worker *wrk, struct req *req)
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
CHECK_OBJ_NOTNULL(req->topreq, REQ_MAGIC);
assert(sp == req->sp);
+ AZ(req->vcl0);
req->director_hint = NULL;
req->restarts = 0;
diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index dc6da227f..1768efee1 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -1033,6 +1033,7 @@ CNT_Embark(struct worker *wrk, struct req *req)
VSLb(req->vsl, SLT_VCL_use, "%s", VCL_Name(req->vcl));
}
+ AZ(req->vcl0);
AN(req->vcl);
}
@@ -1043,6 +1044,8 @@ CNT_Request(struct req *req)
enum req_fsm_nxt nxt;
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+ assert(IS_TOPREQ(req) || req->vcl0 == NULL);
+
wrk = req->wrk;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
@@ -1085,6 +1088,8 @@ CNT_Request(struct req *req)
}
wrk->vsl = NULL;
if (nxt == REQ_FSM_DONE) {
+ if (IS_TOPREQ(req) && req->vcl0 != NULL)
+ VCL_Rel(&req->vcl0);
VCL_TaskLeave(req->vcl, req->privs);
AN(req->vsl->wid);
VRB_Free(req);
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index 563fafc18..d85892de3 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -378,7 +378,7 @@ void VRY_Finish(struct req *req, enum vry_finish_flag);
VCL_BACKEND VCL_DefaultDirector(const struct vcl *);
const struct vrt_backend_probe *VCL_DefaultProbe(const struct vcl *);
void VCL_Init(void);
-void VCL_Panic(struct vsb *, const struct vcl *);
+void VCL_Panic(struct vsb *, const char *nm, const struct vcl *);
void VCL_Poll(void);
void VCL_Ref(struct vcl *);
void VCL_Refresh(struct vcl **);
diff --git a/bin/varnishd/cache/cache_vcl.c b/bin/varnishd/cache/cache_vcl.c
index 17c56463f..270452132 100644
--- a/bin/varnishd/cache/cache_vcl.c
+++ b/bin/varnishd/cache/cache_vcl.c
@@ -193,14 +193,14 @@ vcl_find(const char *name)
/*--------------------------------------------------------------------*/
void
-VCL_Panic(struct vsb *vsb, const struct vcl *vcl)
+VCL_Panic(struct vsb *vsb, const char *nm, const struct vcl *vcl)
{
int i;
AN(vsb);
if (vcl == NULL)
return;
- VSB_printf(vsb, "vcl = {\n");
+ VSB_printf(vsb, "%s = {\n", nm);
VSB_indent(vsb, 2);
PAN_CheckMagic(vsb, vcl, VCL_MAGIC);
VSB_printf(vsb, "name = \"%s\",\n", vcl->loaded_name);
diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c
index ae7d44a3d..c6eb3a927 100644
--- a/bin/varnishd/cache/cache_vrt_vcl.c
+++ b/bin/varnishd/cache/cache_vrt_vcl.c
@@ -341,8 +341,17 @@ VRT_vcl_select(VRT_CTX, VCL_VCL vcl)
struct req *req = ctx->req;
CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+
+ if (IS_TOPREQ(req) && req->vcl0 != NULL)
+ return; // Illega, req-FSM will fail this later.
+
VCL_TaskLeave(req->vcl, req->privs);
- VCL_Rel(&req->vcl);
+ if (IS_TOPREQ(req)) {
+ req->vcl0 = req->vcl;
+ req->vcl = NULL;
+ } else {
+ VCL_Rel(&req->vcl);
+ }
vcl_get(&req->vcl, vcl);
VSLb(ctx->req->vsl, SLT_VCL_use, "%s via %s",
req->vcl->loaded_name, vcl->loaded_name);
diff --git a/bin/varnishd/http1/cache_http1_fsm.c b/bin/varnishd/http1/cache_http1_fsm.c
index deee26649..f754413a2 100644
--- a/bin/varnishd/http1/cache_http1_fsm.c
+++ b/bin/varnishd/http1/cache_http1_fsm.c
@@ -447,6 +447,7 @@ HTTP1_Session(struct worker *wrk, struct req *req)
VCL_TaskEnter(req->vcl, req->privs);
if (CNT_Request(req) == REQ_FSM_DISEMBARK)
return;
+ AZ(req->vcl0);
req->task.func = NULL;
req->task.priv = NULL;
AZ(req->ws->r);
diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c
index ee24e65c5..8269d0fb1 100644
--- a/bin/varnishd/http2/cache_http2_proto.c
+++ b/bin/varnishd/http2/cache_http2_proto.c
@@ -541,6 +541,7 @@ h2_do_req(struct worker *wrk, void *priv)
wrk->stats->client_req++;
if (CNT_Request(req) != REQ_FSM_DISEMBARK) {
AZ(req->ws->r);
+ AZ(req->vcl0);
h2 = r2->h2sess;
CHECK_OBJ_NOTNULL(h2, H2_SESS_MAGIC);
Lck_Lock(&h2->sess->mtx);
diff --git a/bin/varnishtest/tests/r02849.vtc b/bin/varnishtest/tests/r02849.vtc
new file mode 100644
index 000000000..9b4677524
--- /dev/null
+++ b/bin/varnishtest/tests/r02849.vtc
@@ -0,0 +1,123 @@
+varnishtest "ESI included req's start in the same VCL the top started."
+
+server s1 {
+ rxreq
+ expect req.url == /l3a
+ txresp -body "_Correct_"
+
+ rxreq
+ expect req.url == /l3b
+ txresp -body "_Wrong1_"
+
+ rxreq
+ expect req.url == /l3c
+ txresp -body "_Wrong2_"
+
+ rxreq
+ expect req.url == /l1
+ txresp -body {<P1/><esi:include src="/l2"/><P1/>}
+
+ rxreq
+ expect req.url == /l2
+ txresp -body {<P2/><esi:include src="/l3"/><P2/>}
+} -start
+
+varnish v1 -vcl+backend {
+ sub vcl_recv {
+ if (req.url == "/l3") {
+ set req.url = "/l3b";
+ }
+ }
+ sub vcl_backend_response {
+ set beresp.do_esi = True;
+ }
+ sub vcl_deliver {
+ set resp.http.vcl = "lab1";
+ }
+} -start
+
+varnish v1 -cliok "vcl.label lab1 vcl1"
+
+varnish v1 -vcl+backend {
+ sub vcl_recv {
+ if (req.url == "/l3") {
+ set req.url = "/l3a";
+ }
+ }
+ sub vcl_backend_response {
+ set beresp.do_esi = True;
+ }
+ sub vcl_deliver {
+ set resp.http.vcl = "lab2";
+ }
+}
+
+varnish v1 -cliok "vcl.label lab2 vcl2"
+
+varnish v1 -vcl+backend {
+ sub vcl_recv {
+ if (req.url == "/l1") {
+ return (vcl(lab1));
+ }
+ if (req.url == "/l3") {
+ return (vcl(lab2));
+ }
+ if (req.url == "/l3") {
+ set req.url = "/l3c";
+ }
+ }
+ sub vcl_backend_response {
+ set beresp.do_esi = True;
+ }
+ sub vcl_deliver {
+ set resp.http.vcl = "vcl3";
+ }
+}
+
+# First cache the two possible inclusions
+
+client c1 {
+ txreq -url /l3a
+ rxresp
+ expect resp.http.vcl == vcl3
+ txreq -url /l3b
+ rxresp
+ expect resp.http.vcl == vcl3
+ txreq -url /l3c
+ rxresp
+ expect resp.http.vcl == vcl3
+} -run
+
+varnish v1 -vsl_catchup
+
+logexpect l1 -v v1 -g raw {
+
+ expect * 1009 VCL_use "vcl1"
+ expect * 1009 BeReqURL "/l1"
+
+ expect * 1011 VCL_use "vcl3"
+ expect * 1011 BeReqURL "/l2"
+
+ expect * 1012 ReqURL "/l3"
+ expect * 1012 ReqURL "/l3"
+ expect * 1012 VCL_use "vcl2 via lab2"
+ expect * 1012 ReqURL "/l3a"
+
+ expect * 1010 ReqURL "/l2"
+ expect * 1010 ReqURL "/l2"
+
+ expect * 1008 VCL_use "vcl3"
+ expect * 1008 ReqURL "/l1"
+ expect * 1008 VCL_use "vcl1 via lab1"
+} -start
+
+# The test which one is picked
+
+client c1 {
+ txreq -url /l1
+ rxresp
+ expect resp.http.vcl == lab1
+ expect resp.body == {<P1/><P2/>_Correct_<P2/><P1/>}
+} -run
+
+logexpect l1 -wait
diff --git a/doc/sphinx/users-guide/esi.rst b/doc/sphinx/users-guide/esi.rst
index afa9236df..a88e9cc91 100644
--- a/doc/sphinx/users-guide/esi.rst
+++ b/doc/sphinx/users-guide/esi.rst
@@ -89,8 +89,8 @@ Doing ESI on JSON and other non-XML'ish content
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Varnish will peek at the first byte of an object and if it is not
-a "<" Varnish assumes you didn't really mean to ESI process.
-You can alter this behaviour by::
+a "<" Varnish assumes you didn't really mean to ESI process it.
+You can disable this check by::
param.set feature +esi_disable_xml_check
@@ -134,3 +134,11 @@ If you really know what you are doing, you can use this workaround::
set beresp.status = 1206;
}
}
+
+ESI and return(vcl(...))
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+If the original client request switched to a different VCL using
+``return(vcl(...))`` in ``vcl_recv``, any esi:include-requests
+will still start out in the same VCL as the original did, *not*
+in the one it switched to.
More information about the varnish-commit
mailing list