cache_ban: Forward compatibility for the binary ban representation

[nigoroll]

Our ban expressions (like obj.age > 20s) are represented in a binary format (see top of cache_ban.h) which allows for forward compatibility, yet at the respective places we currently just trigger an assertion failure if we hit an unknown argument or operator code.

This commit brings forward compatibility such that, when bans are loaded from persistent storage into older code which does not yet support newly introduced binary codes, we no longer panic.

Ban evaluation:

For bans, evaluating an expression to true is always "correct" in that the cache would not deliver banned content. It might cause objects to be removed from cache, but that is at least not incorrect. So the fail safe action this code takes is to always evaluate unknown ban expressions to true.

CLI ban.list:

For unsupported ban expressions, the unknown argument or operator codes are formatted as (0x%02x) with the string UNSUPPORTED as the user-specified argument. For example:

	1740567193.765849     0 -  (0x20) > UNSUPPORTED && obj.http.foo ~ 377.266

(note that here the operator > is supported and printed as such, and the ban contains one unsupported and one supported expression)

Logging:

For each unsupported argument or operator code, an Error VSL is output exactly once to vxid 0.

Statistics:

Whenever unsupported argument or operator codes are encountered, the newly added counters MAIN.bans_inval_arg1 and MAIN.bans_inval_oper are incremented, respectively.

Fixes #4288

[nigoroll]

bugwash: phk wants to think about it

[nigoroll]

bugwash:

• we have a related issue in SLASH/fellow which points towards an issue caused by the fact that bans use regexen which are not properly (un)serialized
• my initial proposal was to add that functionality to Varnish-Cache
• but @bsdphk proposed to export BANs in text form, which might be an even better idea, which would get us even better compatibility across versions
• then we could also go further and make the runtime representation of bans more efficient for evaluation

We should go back to https://github.com/varnishcache/varnish-cache/wiki/VDD23Q1#bans-and-persistent-storages and write down a plan how bans should look like in future, especially considering loading within a cluster and moving storage.

IRC log for reference:

(15:10:10) slink: So FYI https://gitlab.com/uplex/varnish/slash/-/issues/111 I think is caused by bans in varnish not really using serialized pcres.
(15:10:35) slink: I have asked artis to work on a patch to properly call the pcre serialize / unserialize functions
(15:11:37) phk: Hmm, didn't we switch to serialized pcres long time ago ?
(15:11:37) slink: within this general vincinity, making up out mind about how portable the export format should be would be good #4289
(15:11:37) gofer: #4289: cache_ban: Forward compatibility for the binary ban representation [open] #4289
(15:12:16) phk: I've been thinking about that one, and I think we should actually store the bans in source form.
(15:12:23) slink: And then I thought that it could be helpful for performance to have a more native representation in core
(15:12:34) slink: phk as in text form?
(15:12:55) slink: That would make a lot of sense
(15:12:56) phk: yes. That way a persistent silo can be loaded also with a new version of pcre
(15:13:12) slink: oh I think pcre is taking care of that aspect
(15:13:28) slink: but I agree that re-creating bans upon load would make a lot of sense
(15:13:36) phk: but I wonder what the performance hit of (re)compiling all the ban regexp's at startup might be
(15:13:39) phk: any data ?
(15:13:47) slink: Yes
(15:14:09) phk: The other thing I have been wondering about is silo-portability.
(15:14:47) phk: if you move a silo from machine A to machine B, where there is already another silo too, you may "poison" that silo with the bans from the moved silo.
(15:14:59) slink: I am very sure that re-creating bans would be irrelevant because:
(15:14:59) slink: a) insertion used to be the most expensive, which is now ticked off
(15:14:59) slink: b) but dup search remains very expensive
(15:14:59) slink: both are on a very long (100ks) ban list
(15:14:59) phk: That is of course primarily a hit-rate issue, but...
(15:15:50) slink: we had already agreed on the idea to change the api to also provide the ban id (aka ban time)
(15:16:03) slink: to allow nodes to catch up
(15:16:43) slink: should we (artis + me) work on this or would you realistically get to it?
(15:17:16) phk: you're welcome to it, but I want to make sure we have the principles nailed down first.
(15:17:28) phk: any comments on the "move silo" scenario above ?
(15:17:52) slink: yes, we should review the scenarios we discussed during vdd and make a plan. silos comment coming up
(15:18:50) slink: so I think the only safe principle is that we always need to merge bans.
(15:19:07) slink: for this, the API change with the externally provided ban id is fundamental
(15:19:45) slink: once this is in place, for the "move silo within cluster" scenario, which is, I think, the realistic scenario to move silos to begin with, this will become a none-issue
(15:19:47) phk: what if the id is not provided ?
(15:20:25) slink: I would think then "first ban wins" and dup check eleminates the second
(15:20:54) slink: IMHO, without a central ban service one can not expect optimal cluster banning, and in particular not silo moves
(15:22:02) phk: I guess that is a fair assumption
(15:23:03) slink: so I will leave a summary in #4289, please let me know if I misrepresented anything once posted
(15:23:04) gofer: #4289: cache_ban: Forward compatibility for the binary ban representation [open] #4289
(15:23:26) phk: slink, but stupid question:
(15:23:50) phk: is the "born with id" supposed to be unique in time and space or only space ?
(15:24:18) phk: here's the scenario I'm thinking about:
(15:24:21) phk: two varnishes
(15:24:52) phk: ban id=1 is sent to both of them, but dont arrive at the same time, so one cache fetches resource A before it gets the ban, the other after.
(15:25:12) phk: (imagine hours between the fetches)
(15:25:54) phk: if for some reason you mix the silos now, does the ban also apply to the new copy of A ?
(15:26:40) slink: notes we took in 2023: https://github.com/varnishcache/varnish-cache/wiki/VDD23Q1#bans-and-persistent-storages
(15:26:47) phk: Again, presumably the harm is (mostly) hit-rate
(15:27:22) slink: we have the "two face commit" issue
(15:27:54) slink: to do this right, we probably would need to consider not just the top ban, but for t+dt down
(15:28:20) slink: I would prefer to avoid this
(15:28:28) phk: but if faced with two copies of the same ban (id1==id2) but with different timestamps, which timestamp do you pick ?
(15:28:35) phk: it has to be the earlier, right ?
(15:29:01) slink: later rather?
(15:29:09) slink: later bans more, doesn't it?
(15:29:20) phk: that's why I'm asking :-)
(15:30:15) phk: but I guess if the objects in the silo point to the ban-id, then it sorts itself out.
(15:30:31) slink: aaah now I get what you were thinking about
(15:30:54) slink: yes, if the ids are not coordinated, I think we would merge (add all ids) and run the existing dup detection
(15:31:33) slink: my worry is more the heisenbans on a cluster
(15:31:56) phk: what about finding two bans, one in either silo, same ban-id but different substance ?
(15:32:23) slink: we should maybe think about always using ids with a delta-t in the future, which take a moment (ms) to become active
(15:32:37) slink: phk that would be a bug in my mind
(15:32:48) phk: yeah, but how do we handle it ?
(15:33:10) slink: not sure I have an answer straight away
(15:33:25) slink: nudge one id?
(15:33:56) phk: combine into one ban, OR'ing the two criteria, so they both apply ?
(15:34:15) phk: we cannot nudge the id, there are objects pointing to it.
(15:34:40) slink: but we can modify the id they point to during load
(15:34:59) phk: so which ones would you nudge ?
(15:35:13) slink: I am not sure I know a good answer
(15:35:35) slink: When in doubt, we can always safely move objects "down" a ban
(15:36:27) phk: so insert one of them, fudge the id of the other and insert right after ? (same as inserting one ban with regexp1 || regexp2)
(15:36:53) slink: It should work in the sense of "be safe" at least
(15:37:16) phk: but how will you get the the new extra ban stuffed back in the silos ?
(15:37:39) phk: or is that going to take a rewrite of the full ban list once it has been sorted out ?
(15:38:33) slink: So once the object changes bans, storage gets notified and notes the new ban
(15:39:01) slink: until then, the same logic applies as before, once the ban id change happens, there is no ban id conflict any more.
(15:39:18) phk: yeah, but you cannot change the id of the ban in the silo, and you cannot insert the other ban either. (or modify the criterial to be 1||2 for that matter)
(15:39:57) slink: maybe I do not understand "change the id of the ban in the silo".
(15:40:16) slink: why would that happen?
(15:40:26) phk: you dont want to sync the merged bans to the silos after startup ?
(15:40:48) slink: Yes I would think the merged bans should be exported
(15:41:12) phk: ok, so that takes care of the discrepancy
(15:42:29) slink: any more thoughts on this?
(15:42:38) phk: but if the ban-id is assigned by the user, we have no safe to modify it, do we ?
(15:43:46) slink: I would think the fallback would be the same, but an id clash with a central system really would be a bug or an exceptional scenario in a cluster (like amnesia of the ban central)
(15:44:36) phk: yes, "it shouldn't happen" but I want us convince ourselves that we will not die in endless asserts.
(15:45:14) phk: how about this:
(15:45:24) slink: I think the fallback algorithm we drafted should be safe at the expense of maybe removing objects which are fine plus a somehow marginal performance penalty
(15:45:34) phk: the ID we store is SHA256(user-provided-id)[:somelength]
(15:46:13) phk: that way we can prepend a control-character to the user-provided-id and get a unique new id.
(15:46:39) slink: I am confused
(15:46:44) phk: if the user does not provide id, the id becomes SHA256(ban-spec)[:somelength]
(15:47:02) slink: Shouldn't the IDs be numerical for temporal ordering?
(15:47:24) phk: ohh, I thought the were arbitrary and the timestamps ordered them ?
(15:47:32) slink: (and roughly match wall clock time for admin sanity)
(15:47:47) phk: if they are numeric, what happens if CLI received ban#10, ban#11, ban#7 in that order ?
(15:47:47) gofer: #7: CLI management interface (from Trac) [closed] #7
(15:48:04) slink: error
(15:48:24) slink: you can not ban with an id earlier than most recent
(15:48:43) phk: user answers: But that is perfectly reasonable, we're trying to back-fill the bans we issued during down-time ?
(15:49:02) slink: you can do that only during silo load
(15:50:17) phk: I sitll think the VDD summary is fine, but we need to get the crinckly bits fiddled properly...
(15:50:26) slink: agree
(15:50:46) slink: my summary is posted, would you be OK with pasting this conversation for reference?
(15:51:08) phk: sure
(15:51:18) phk: and btw: You're in a hurry in a moment
(15:51:19) slink: cool, thank you, that was really helpful
(15:51:27) slink: yes EOBW?
(15:51:32) phk: EOBW