Loosen assertion on ctx->(req|bo), fix shard and vcl_pipe

in VRT_priv_task() we asserted that only one of ctx->req and ctx->bo is
set when not in vcl_pipe {}, but we also need to extend that assertion
to when ctx->method == 0 after vcl_pipe as finished because
VRT_priv_task() could be called from director resolution.

Being at it, I also noticed that our behavior in vcl_pipe {} is
inconsistent as, from the shard director perspective, it is a backend
method. So now, vcl_pipe {} is handled like vcl_backend_* {}.

We still need to make up our mind about #3329 / #3330 and depending on
the outcome we might need to touch some places again which were changed
in this commit.

Fixes #3361

bin/varnishd/cache/cache_vrt_priv.c

@@ -134,8 +134,15 @@ VRT_priv_task(VRT_CTX, const void *vmod_id)
 {
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	/*
+	 * XXX when coming from VRT_DirectorResolve() in pipe mode
+	 * (ctx->method == 0), both req and bo are set.
+	 * see #3329 #3330: we should make up our mind where
+	 * pipe objects live
+	 */
+
 	assert(ctx->req == NULL || ctx->bo == NULL ||
-	    ctx->method == VCL_MET_PIPE);
+	    ctx->method == VCL_MET_PIPE || ctx->method == 0);
 
 	if (ctx->req) {
 		CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);

bin/varnishtest/tests/d00029.vtc

 varnishtest "shard director LAZY - d18.vtc"
 
-server s1 {
+server s1 -repeat 3 {
 	rxreq
 	txresp -body "ech3Ooj"
 } -start
 
-server s2 {
+server s2 -repeat 3 {
 	rxreq
 	txresp -body "ieQu2qua"
 } -start
 
-server s3 {
+server s3 -repeat 3 {
 	rxreq
 	txresp -body "xiuFi3Pe"
 } -start
@@ -45,10 +45,13 @@ varnish v1 -vcl+backend {
 	}
 
 	sub vcl_recv {
+	    if (req.http.pipe) {
+		return (pipe);
+	    }
 	    return(pass);
 	}
 
-	sub vcl_backend_fetch {
+	sub shard_be {
 	    set bereq.backend=vd.backend(resolve=LAZY);
 
 	    if (bereq.url == "/1") {
@@ -64,6 +67,14 @@ varnish v1 -vcl+backend {
 	    }
 	}
 
+	sub vcl_backend_fetch {
+	    call shard_be;
+	}
+
+	sub vcl_pipe {
+	    call shard_be;
+	}
+
 	sub vcl_backend_response {
 	    set beresp.http.backend = bereq.backend;
 	}
@@ -85,4 +96,20 @@ client c1 {
 	rxresp
 	expect resp.body == "xiuFi3Pe"
 	expect resp.http.backend == "vd"
+
+	txreq -url /1 -hdr "pipe: true"
+	rxresp
+	expect resp.body == "ech3Ooj"
+} -run
+
+client c1 {
+	txreq -url /2 -hdr "pipe: true"
+	rxresp
+	expect resp.body == "ieQu2qua"
+} -run
+
+client c1 {
+	txreq -url /3 -hdr "pipe: true"
+	rxresp
+	expect resp.body == "xiuFi3Pe"
 } -run

bin/varnishtest/tests/d00030.vtc

@@ -31,7 +31,7 @@ logexpect l2 -v v1 -g raw {
     expect * 1001 VCL_Error {shard .backend param invalid}
 } -start
 logexpect l3 -v v1 -g raw {
-    expect * 1003 VCL_Error {shard_param.set.. may only be used in vcl_init and in backend context}
+    expect * 1003 VCL_Error {shard_param.set.. may only be used in vcl_init and in backend/pipe context}
 } -start
 
 client c1 {
@@ -159,7 +159,7 @@ varnish v1 -errvcl {invalid warmup argument 1.1} {
     }
 }
 
-varnish v1 -errvcl {resolve=LAZY with other parameters can only be used in backend context} {
+varnish v1 -errvcl {resolve=LAZY with other parameters can only be used in backend/pipe context} {
     import directors;
     import blob;
 

doc/changes.rst

@@ -38,6 +38,9 @@ NEXT (scheduled 2020-09-15)
   argument which allows to extend the effective privilege set of the
   worker process.
 
+* The shard director and shard director parameter objects should now
+  work in ``vcl_pipe {}`` like in ``vcl_backend_* {}`` subs.
+
 ================================
 Varnish Cache 6.4.0 (2020-03-16)
 ================================

lib/libvmod_directors/vmod_directors.vcc

@@ -462,10 +462,11 @@ is _not_ the order given when backends are added.
 
   * ``HASH``:
 
-    * when called in backend context: Use the varnish hash value as
-      set by ``vcl_hash{}``
+    * when called in backend context and in ``vcl_pipe {}``: Use the
+      varnish hash value as set by ``vcl_hash{}``
 
-    * when called in client context: hash ``req.url``
+    * when called in client context other than ``vcl_pipe {}``: hash
+      ``req.url``
 
   * ``URL``: hash req.url / bereq.url
 
@@ -556,9 +557,10 @@ is _not_ the order given when backends are added.
     In ``vcl_init{}`` and on the client side, ``LAZY`` mode can not be
     used with any other argument.
 
-    On the backend side, parameters from arguments or an associated
-    parameter set affect the shard director instance for the backend
-    request irrespective of where it is referenced.
+    On the backend side and in ``vcl_pipe {}``, parameters from
+    arguments or an associated parameter set affect the shard director
+    instance for the backend request irrespective of where it is
+    referenced.
 
 * *param*
 
@@ -607,10 +609,11 @@ Parameter sets have two scopes:
 * per backend request scope
 
 The per-VCL scope defines defaults for the per backend scope. Any
-changes to a parameter set in backend context only affect the
-respective backend request.
+changes to a parameter set in backend context and in ``vcl_pipe {}``
+only affect the respective backend request.
 
-Parameter sets can not be used in client context.
+Parameter sets can not be used in client context except for
+``vcl_pipe {}``.
 
 The following example is a typical use case: A parameter set is
 associated with several directors. Director choice happens on the
@@ -649,11 +652,11 @@ $Method VOID .clear()
 Reset the parameter set to default values as documented for
 `xshard.backend()`_.
 
-* in ``vcl_init{}``, resets the parameter set default for this VCL
-* in backend context, resets the parameter set for this backend
-  request to the VCL defaults
+* in ``vcl_init{}``, resets the parameter set default for this VCL in
+* backend context and in ``vcl_pipe {}``, resets the parameter set for
+  this backend request to the VCL defaults
 
-This method may not be used in client context
+This method may not be used in client context other than ``vcl_pipe {}``.
 
 $Method VOID .set(
 	[ ENUM {HASH, URL, KEY, BLOB} by ],
@@ -669,11 +672,11 @@ Change the given parameters of a parameter set as documented for
 
 * in ``vcl_init{}``, changes the parameter set default for this VCL
 
-* in backend context, changes the parameter set for this backend
-  request, keeping the defaults set for this VCL for unspecified
-  arguments.
+* in backend context and in ``vcl_pipe {}``, changes the parameter set
+  for this backend request, keeping the defaults set for this VCL for
+  unspecified arguments.
 
-This method may not be used in client context
+This method may not be used in client context other than ``vcl_pipe {}``.
 
 $Method STRING .get_by()
 
@@ -709,7 +712,7 @@ shard director using this parameter object would use. See
 
 $Method BLOB .use()
 
-This method may only be used in backend context.
+This method may only be used in backend context and in ``vcl_pipe {}``.
 
 For use with the *param* argument of `xshard.backend()`_
 to associate this shard parameter set with a shard director.

lib/libvmod_directors/vmod_shard.c

@@ -170,6 +170,9 @@ vmod_shard_param_read(VRT_CTX, const void *id,
     const struct vmod_directors_shard_param *p,
     struct vmod_directors_shard_param *pstk, const char *who);
 
+// XXX #3329 #3330 revisit - for now, treat pipe like backend
+#define SHARD_VCL_TASK_REQ (VCL_MET_TASK_C & ~VCL_MET_PIPE)
+#define SHARD_VCL_TASK_BEREQ (VCL_MET_TASK_B | VCL_MET_PIPE)
 /* -------------------------------------------------------------------------
  * shard vmod interface
  */
@@ -617,14 +620,14 @@ vmod_shard_backend(VRT_CTX, struct vmod_directors_shard *vshard,
 			return (vshard->dir);
 		}
 
-		if ((ctx->method & VCL_MET_TASK_B) == 0) {
+		if ((ctx->method & SHARD_VCL_TASK_BEREQ) == 0) {
 			VRT_fail(ctx, "shard .backend resolve=LAZY with other "
-				 "parameters can only be used in backend "
+				 "parameters can only be used in backend/pipe "
 				 "context");
 			return (NULL);
 		}
 
-		assert(ctx->method & VCL_MET_TASK_B);
+		assert(ctx->method & SHARD_VCL_TASK_BEREQ);
 
 		pp = shard_param_task(ctx, vshard->shardd,
 				      vshard->shardd->param);
@@ -918,11 +921,11 @@ shard_param_prep(VRT_CTX, struct vmod_directors_shard_param *p,
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(p, VMOD_SHARD_SHARD_PARAM_MAGIC);
 
-	if (ctx->method & VCL_MET_TASK_C) {
+	if (ctx->method & SHARD_VCL_TASK_REQ) {
 		VRT_fail(ctx, "%s may only be used "
-			 "in vcl_init and in backend context", who);
+			 "in vcl_init and in backend/pipe context", who);
 		return (NULL);
-	} else if (ctx->method & VCL_MET_TASK_B)
+	} else if (ctx->method & SHARD_VCL_TASK_BEREQ)
 		p = shard_param_task(ctx, p, p);
 	else
 		assert(ctx->method & VCL_MET_TASK_H);
@@ -967,7 +970,7 @@ vmod_shard_param_read(VRT_CTX, const void *id,
 	CHECK_OBJ_NOTNULL(p, VMOD_SHARD_SHARD_PARAM_MAGIC);
 	(void) who; // XXX
 
-	if (ctx->method == 0 || (ctx->method & VCL_MET_TASK_B))
+	if (ctx->method == 0 || (ctx->method & SHARD_VCL_TASK_BEREQ))
 		p = shard_param_task(ctx, id, p);
 
 	if (p == NULL)