add std.ban() and std.ban_error() for vcl access to ban errors

The ban() vcl action adds bans like the ban CLI command, but has no
facility for in-band error reporting. Errors are only reported to VSL.

We add std.ban() as a replacement for ban() with identical semantics,
but returning if the ban was successfully added.

std.ban_error() is added to report the error, if any.

We add v00009.vtc mirroring v00011.vtc, but with std.ban() replacing
ban(). The test number was chosen to fill a gap close to v00011.vtc.

Implementation:

We change VRT_ban_string() to return any error or NULL for
success. Where the error is not a constant string, we need to format
it on the workspace. For workspace overflows, we need to fall back to
canned constant error strings to ensure that the error case never
appears as success.

bin/varnishd/cache/cache_vrt.c

@@ -829,7 +829,7 @@ VRT_synth_page(VRT_CTX, VCL_STRANDS s)
 
 /*--------------------------------------------------------------------*/
 
-static VCL_VOID
+static VCL_STRING
 vrt_ban_error(VRT_CTX, VCL_STRING err)
 {
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
@@ -837,77 +837,83 @@ vrt_ban_error(VRT_CTX, VCL_STRING err)
 	AN(err);
 
 	VSLb(ctx->vsl, SLT_VCL_Error, "ban(): %s", err);
+	return (err);
 }
 
-VCL_VOID
+VCL_STRING
 VRT_ban_string(VRT_CTX, VCL_STRING str)
 {
 	char *a1, *a2, *a3;
 	char **av;
 	struct ban_proto *bp;
-	const char *err;
+	const char *err = NULL, *berr = NULL;
 	int i;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 
-	if (str == NULL) {
-		vrt_ban_error(ctx, "Null argument");
-		return;
-	}
+	if (str == NULL)
+		return (vrt_ban_error(ctx, "Null argument"));
 
 	bp = BAN_Build();
-	if (bp == NULL) {
-		vrt_ban_error(ctx, "Out of Memory");
-		return;
-	}
+	if (bp == NULL)
+		return (vrt_ban_error(ctx, "Out of Memory"));
+
 	av = VAV_Parse(str, NULL, ARGV_NOESC);
 	AN(av);
 	if (av[0] != NULL) {
-		vrt_ban_error(ctx, av[0]);
+		err = av[0];
 		VAV_Free(av);
 		BAN_Abandon(bp);
-		return;
+		return (vrt_ban_error(ctx, err));
 	}
 	for (i = 0; ;) {
 		a1 = av[++i];
 		if (a1 == NULL) {
-			vrt_ban_error(ctx, "No ban conditions found.");
+			err = "No ban conditions found.";
 			break;
 		}
 		a2 = av[++i];
 		if (a2 == NULL) {
-			vrt_ban_error(ctx, "Expected comparison operator.");
+			err = "Expected comparison operator.";
 			break;
 		}
 		a3 = av[++i];
 		if (a3 == NULL) {
-			vrt_ban_error(ctx, "Expected second operand.");
+			err = "Expected second operand.";
 			break;
 		}
-		err = BAN_AddTest(bp, a1, a2, a3);
-		if (err != NULL) {
-			vrt_ban_error(ctx, err);
+		berr = BAN_AddTest(bp, a1, a2, a3);
+		if (berr != NULL)
 			break;
-		}
+
 		if (av[++i] == NULL) {
-			err = BAN_Commit(bp);
-			if (err == NULL)
+			berr = BAN_Commit(bp);
+			if (berr == NULL)
 				bp = NULL;
-			else
-				vrt_ban_error(ctx, err);
 			break;
 		}
 		if (strcmp(av[i], "&&")) {
-			// XXX refactoring pending via PR
-			VSLb(ctx->vsl, SLT_VCL_Error,
-			    "ban(): Expected && between conditions,"
-			    " found \"%s\"", av[i]);
+			err = WS_Printf(ctx->ws, "Expected && between "
+			    "conditions, found \"%s\"", av[i]);
+			if (err == NULL)
+				err = "Expected && between conditions "
+				    "(workspace overflow)";
 			break;
 		}
 	}
+	if (berr != NULL) {
+		AZ(err);
+		err = WS_Copy(ctx->ws, berr, -1);
+		if (err == NULL)
+			err = "Unknown error (workspace overflow)";
+		berr = NULL;
+	}
 	if (bp != NULL)
 		BAN_Abandon(bp);
 	VAV_Free(av);
+	if (err == NULL)
+		return (NULL);
+	return (vrt_ban_error(ctx, err));
 }
 
 VCL_BYTES

bin/varnishtest/tests/v00009.vtc

+varnishtest "Test std.ban()"
+
+# see also v00011.vtc
+
+server s1 {
+	rxreq
+	txresp -body "foo"
+
+	rxreq
+	txresp -body "foo"
+} -start
+
+varnish v1 -vcl+backend {
+	import std;
+
+	sub vcl_synth {
+		set resp.http.b1 = std.ban(req.http.doesntexist);
+		set resp.http.b1e = std.ban_error();
+		set resp.http.b2 = std.ban("");
+		set resp.http.b2e = std.ban_error();
+		set resp.http.b3 = std.ban("req.url");
+		set resp.http.b3e = std.ban_error();
+		set resp.http.b4 = std.ban("req.url //");
+		set resp.http.b4e = std.ban_error();
+		set resp.http.b5 = std.ban("req.url // bar");
+		set resp.http.b5e = std.ban_error();
+		set resp.http.b6 = std.ban("req.url == bar //");
+		set resp.http.b6e = std.ban_error();
+		set resp.http.b7 = std.ban("foo == bar //");
+		set resp.http.b7e = std.ban_error();
+		set resp.http.b8 = std.ban("obj.age == 4");
+		set resp.http.b8e = std.ban_error();
+		set resp.http.b9 = std.ban("obj.age // 4d");
+		set resp.http.b9e = std.ban_error();
+		set resp.http.b10 = std.ban("obj.http.foo > 4d");
+		set resp.http.b10e = std.ban_error();
+		set resp.http.b11 = std.ban("req.url ~ ^/$");
+		set resp.http.b11e = std.ban_error();
+	}
+	sub vcl_recv {
+		if (req.method == "BAN") {
+			return (synth(209,"foo"));
+		}
+	}
+
+} -start
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.http.X-Varnish == "1001"
+} -run
+
+logexpect l1 -v v1 -d 1 -g vxid {
+	expect * 1004	VCL_Error {ban[(][)]: Null argument}
+	expect * 1004	VCL_Error {ban[(][)]: No ban conditions found[.]}
+	expect * 1004	VCL_Error {ban[(][)]: Expected comparison operator[.]}
+	expect * 1004	VCL_Error {ban[(][)]: Expected second operand[.]}
+	expect * 1004	VCL_Error {ban[(][)]: expected conditional [(]==, !=, ~ or !~[)] got "//"}
+	expect * 1004	VCL_Error {ban[(][)]: Expected && between conditions, found "//"}
+	expect * 1004	VCL_Error {ban[(][)]: Unknown or unsupported field "foo"}
+	expect * 1004	VCL_Error {ban[(][)]: expected duration <n.nn>.ms|s|m|h|d|w|y. got "4"}
+	expect * 1004	VCL_Error {ban[(][)]: expected conditional [(]==, !=, >, >=, < or <=[)] got "//"}
+	expect * 1004	VCL_Error {ban[(][)]: expected conditional [(]==, !=, ~ or !~[)] got ">"}
+
+
+} -start
+
+client c1 {
+	txreq -req "BAN"
+	rxresp
+	expect resp.http.X-Varnish == "1004"
+	expect resp.status == 209
+	expect resp.http.b1 == false
+	expect resp.http.b1e == {Null argument}
+	expect resp.http.b2 == false
+	expect resp.http.b2e == {No ban conditions found.}
+	expect resp.http.b3 == false
+	expect resp.http.b3e == {Expected comparison operator.}
+	expect resp.http.b4 == false
+	expect resp.http.b4e == {Expected second operand.}
+	expect resp.http.b5 == false
+	expect resp.http.b5e == {expected conditional (==, !=, ~ or !~) got "//"}
+	expect resp.http.b6 == false
+	expect resp.http.b6e == {Expected && between conditions, found "//"}
+	expect resp.http.b7 == false
+	expect resp.http.b7e == {Unknown or unsupported field "foo"}
+	expect resp.http.b8 == false
+	expect resp.http.b8e == {expected duration <n.nn>[ms|s|m|h|d|w|y] got "4"}
+	expect resp.http.b9 == false
+	expect resp.http.b9e == {expected conditional (==, !=, >, >=, < or <=) got "//"}
+	expect resp.http.b10 == false
+	expect resp.http.b10e == {expected conditional (==, !=, ~ or !~) got ">"}
+	expect resp.http.b11 == true
+	expect resp.http.b11e == {}
+} -run
+
+logexpect l1 -wait
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.http.X-Varnish == "1006"
+} -run

bin/varnishtest/tests/v00011.vtc

 varnishtest "Test vcl ban()"
 
+# see also v00009.vtc
+
 server s1 {
 	rxreq
 	txresp -body "foo"

include/vrt.h

@@ -58,6 +58,7 @@
  *	struct vmod_priv_methods added
  *	struct vmod_priv free member replaced with methods
  *	VRT_CTX_Assert() added
+ *	VRT_ban_string() signature changed
  * 12.0 (2020-09-15)
  *	Added VRT_DirectorResolve()
  *	Added VCL_STRING VRT_BLOB_string(VRT_CTX, VCL_BLOB)
@@ -494,7 +495,7 @@ VCL_BYTES VRT_CacheReqBody(VRT_CTX, VCL_BYTES maxsize);
 /* Regexp related */
 
 const char *VRT_regsub(VRT_CTX, int all, const char *, void *, const char *);
-VCL_VOID VRT_ban_string(VRT_CTX, VCL_STRING);
+VCL_STRING VRT_ban_string(VRT_CTX, VCL_STRING);
 VCL_INT VRT_purge(VRT_CTX, VCL_DURATION, VCL_DURATION, VCL_DURATION);
 VCL_VOID VRT_synth(VRT_CTX, VCL_INT, VCL_STRING);
 VCL_VOID VRT_hit_for_pass(VRT_CTX, VCL_DURATION);

lib/libvcc/vcc_action.c

@@ -187,7 +187,7 @@ vcc_act_ban(struct vcc *tl, struct token *t, struct symbol *sym)
 	ExpectErr(tl, '(');
 	vcc_NextToken(tl);
 
-	Fb(tl, 1, "VRT_ban_string(ctx, \n");
+	Fb(tl, 1, "(void) VRT_ban_string(ctx, \n");
 	tl->indent += INDENT;
 	vcc_Expr(tl, STRING);
 	tl->indent -= INDENT;

lib/libvmod_std/vmod_std.c

@@ -311,3 +311,51 @@ vmod_fnmatch(VRT_CTX, VCL_STRING pattern, VCL_STRING subject,
 		flags |= FNM_PERIOD;
 	return (fnmatch(pattern, subject, flags) != FNM_NOMATCH);
 }
+
+static const void * const priv_task_id_ban = &priv_task_id_ban;
+
+VCL_BOOL v_matchproto_(td_std_ban)
+vmod_ban(VRT_CTX, VCL_STRING s)
+{
+	struct vmod_priv *priv_task;
+	VCL_STRING r;
+
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+
+	priv_task = VRT_priv_task(ctx, priv_task_id_ban);
+	if (priv_task == NULL) {
+		VRT_fail(ctx, "std.ban(): no priv_task (out of workspace?)");
+		return (0);
+	}
+
+	r = VRT_ban_string(ctx, s);
+
+	/*
+	 * TRUST_ME: the ban error is const. We save it in the un-const priv
+	 * pointer, but promise to only ever return it as a (const) VCL_STRING
+	 */
+	priv_task->priv = TRUST_ME(r);
+
+	return (r == NULL);
+}
+
+VCL_STRING v_matchproto_(td_std_ban_error)
+vmod_ban_error(VRT_CTX)
+{
+	struct vmod_priv *priv_task;
+	VCL_STRING r;
+
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+
+	priv_task = VRT_priv_task(ctx, priv_task_id_ban);
+	if (priv_task == NULL) {
+		VRT_fail(ctx, "std.ban_error():"
+		    " no priv_task (out of workspace?)");
+		return ("no priv_task");
+	}
+
+	r = priv_task->priv;
+	if (r == NULL)
+		r = "";
+	return (r);
+}

lib/libvmod_std/vmod_std.vcc

@@ -561,6 +561,16 @@ Example::
 
 	std.rollback(bereq);
 
+$Function BOOL ban(STRING)
+
+Same as :ref:`vcl(7)_ban`, but returns true if the ban succeeded and
+false otherwise.
+
+$Function STRING ban_error()
+
+Returns a textual error description of the last `std.ban()`_ call from
+the same task or the empty string if there either was no error or no
+`std.ban()`_ call.
 
 DEPRECATED functions
 ====================
@@ -630,8 +640,6 @@ Example::
 
 	set req.http.real = std.time2real(now, 1.0);
 
-
-
 SEE ALSO
 ========