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

[nigoroll]

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 error reporting added.

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. 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.

[slimhazard]

+1 on this feature, but I have a comment about:

NULL string is returned if the ban succeeded.

Since VCL doesn't have a NULL constant, I wonder if VCL authors who are less familiar with the innards of Varnish will know what to make of that. I'd guess that many of them would like to just check whether there was an error, regardless of the error message, in order to generate a synthetic error status rather than success. The choice would be to look in the log for the error message, and not include the error in the response, which seems legit to me.

For that they'd have to check whether the return value is NULL, and it wouldn't surprise me if many VCL authors don't know how. (AFAIK NULL is not documented as a value of the language in vcl(7), although the value sometimes gets mentioned like this as a return value.)

Maybe just an addition to the documentation is enough:

if (std.ban("req.url ~ ^/foo")) {
    # Ban failed
    return(synth(503));
}
else {
    # Ban succeeded
    return(synth(200));
}

But the true/false sense in if and else is the opposite of what I would have thought intuitive. Truthy VCL strings are false if NULL, true otherwise, and I would have expected boolean true on success.

So I wonder if VMOD std could use a convenience function that issues a ban and just returns BOOL true on success, false on failure. Or maybe std.ban() could return a string constant like "SUCCESS" rather than NULL.

Another consideration would be to add a NULL constant to VCL, but that opens another can of worms.

[nigoroll]

Thank you for your suggestions @slimhazard . I'd like to gather some more feedback from others and have thus suggested the ticket for bugwash in 2021.
To summarize the options:

[slimhazard]

Note: the second option is "make std.ban() return BOOL for ban success and add std.ban_error() to return the error of the last ban". (github cuts of the end of the sentence, I'm reading the poll URL.)

That gets us into using task scope to save the error message. VMOD std has avoided that kind of thing for the most part. I guess return NULL as the error message if the last ban was successful, or if there hasn't been a ban during the task? Seems to be getting a bit complicated, for something meant to be fairly simple.

The third option is at risk of POLA, since IMO the truthiness of VCL strings makes the usage in BOOL context unintuitive. So I voted for the first option (return "SUCCESS" when the ban succeeded).

[nigoroll]

re @slimhazard I see no problem with saving the last ban error in a priv_task

[nigoroll]

bugwash decision:

• BOOL ban(STRING)
• STRING ban_error() returning the empty string if ok

roadmap:

1. add std.ban*(),
2. add implicit import std
3. vcc aliases ban() to std.ban() (some concerns raised)
4. deprecated ban() without std.
5. remove ban() without std.

[nigoroll]

changed accordingly

[Dridi]

I have little changes to consider for this pull request.

In addition, I would like to point other potential changes in the ban code.

We have two static ban_error() functions, I suggest we rename one to vrt_ban_error().

The BAN_Abandon() function should take a struct ban_proto ** and wipe it.

Conversely, BAN_Commit() should do the same thing since it calls the former.

Circling back to this pull request, the changes suggested above could later help simplify the code touched by this diff. I'm not suggesting this to be fixed now, only as a followup cleanup task.

For the requested changes to this pull request, see below.

[nigoroll]

@Dridi I have changed the documentation wording. May I ask for your OK? I am with you on the implementation changes, and I work on them.