[master] 4ebc3cfec Make it possible to override the initial digest, and explain in a comment why it is not necessary.

Dridi Boukelmoune dridi at varni.sh
Tue Feb 16 06:09:18 UTC 2021


> > Hi,
> >
> > this thing happened when I did not have enough time to reflect, so just two
> > points on the way out:
> >
> > - Do we need a way to fix the hash within vcl_recv?
> >
> >   For example, if a backend decision has been made on the basis of the hash, we
> > do not want it to change any more.

This is exactly where this hash_data-from-vcl_recv falls short in our
current model.

The availability of variables (including read-only vs read-write) is
tied to steps today and that allows predictability and consistency.

So today we have this:

- vcl_recv
- vcl_hash
- lookup

We build req.hash in vcl_hash, after we get a chance to normalize the
request in vcl_recv. Here we have clear steps to "fix" req (vcl_recv)
and then "fix" req.hash (vcl_hash). I'm aware that req is writeable in
vcl_hash, but considering that you could leave vcl_recv from multiple
branches, that linear ordering of steps gives you in effect a clear
boundary where you can expect req to have been "fixed".

With the previous/only consensus we would have:

- vcl_recv
- vcl_hash
- vcl_lookup
- lookup

And vcl_lookup would be the step where you expect the hash to be
consistently "fixed" or "finalized" or "actionable" without having to
worry about it. It would be enforced by the type/symbol system, just
like any other VCL variable, at compile time. No surprises.

> > - Should we bump the vcl version?
> >
> >   We have done so for comparably less significant changes (see introduction of
> > UDS), and this definitely falls in the "semantic change without syntactic
> > change" category.

Not sure about that, but well, more complications.

> >   Existing VCL could, for example, rely on vcl_hash to be called.
>
> Also this might contradict or at least complicate Dridis efforts to allow "other
> version includes": For this vcl...
>
>
> # included.vcl
> vcl 4.1;
>
> sub vcl_hash {
>         hash_data("I rely on it");
> }
>
> # default.vcl
> vcl 4.2;
>
> include "included.vcl";
>
> sub vcl_recv {
>         hash_data(req.http.foo);
> }
>
> the whole point would be to _not_ silently allow the include: By the structure
> of the include, the code in "included.vcl" would have precedence over anything
> else, but with the hash_data change it now does not even get called any more.

I think this is a dead end, like the `new` keyword that can be used
in a branch, it would be harder to determine at compile time that
hash_data wasn't relied on in both vcl_recv and vcl_hash (and it's
always called in the built-in vcl_hash anyway) so that would have to
be a runtime check anyway. And this means that "I rely on it" would
have to be silently ignored and that in effect you couldn't reliably
rely on it feeding your hash.

This is the worst possible tradeoff.

> It seems the hash change would have required more deliberation.

It's not too late to revert.


More information about the varnish-commit mailing list