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

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"

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;

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

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;

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

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

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

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

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

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

bin/varnishtest/tests/r02849.vtc

+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

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.