Can't include 4.0 from 4.1

[gquintard]

This has been tested on master on an up-to-date Arch linux. It's also present on 6.4

man vcl says (emphasis mine):

Multiple versions of the VCL syntax can coexist within certain constraints.

The VCL syntax version at the start of VCL file specified with -f sets the hard limit that cannot be exceeded anywhere, and it selects the appropriate version of the builtin VCL. That means that you can never include vcl 9.1; from vcl 8.7;, but the opposite may be possible, to the extent the compiler supports it. ... The version of Varnish this file belongs to supports syntax 4.0 and 4.1.

So if I understand it correctly, we should be able to use a vcl 4.0 included from a vcl 4.1 file, and this should work:

varnishtest "include 4.0 from 4.1"

shell {
    cat << EOF > ${tmpdir}/inc.vcl
    vcl 4.0;
    sub vcl_recv {
        set req.esi = false;
    }
EOF
}

varnish v1 -syntax 4.1 -vcl {
    include "${tmpdir}/inc.vcl";
} -start

but it fails with:

**** v1   CLI RX|Message from VCC-compiler:
**** v1   CLI RX|Symbol not found: 'req.esi' (Only available when VCL syntax <= 4.0)
**** v1   CLI RX|At: ('/tmp/vtc.361713.6d37478f/inc.vcl' Line 3 Pos 21) -- (Pos 27)
**** v1   CLI RX|                set req.esi = false;
**** v1   CLI RX|--------------------#######---------
**** v1   CLI RX|
**** v1   CLI RX|Running VCC-compiler failed, exited with 2

[dridi]

The answer is no, even minor bumps in VCL can break (which is arguably fine) to the extent that it affects includes as well (which has been my complaint for a while).

[bsdphk]

"certain constraints" may be overselling our capabilities :-)

[gquintard]

So docfix, saying "you can include a file with a lower vcl version as long as the code in it is compatible with the top vcl version"?

[dridi]

I'd rather we have 4.1 as the actual version of the VCL, and 4.0 as the effective version to allow 4.0-only code.

[dridi]

Bugwash consensus: I will attempt a patch and we can resume the discussion on whether we want this or not.

[dridi]

Would an explicit syntax change be an acceptable compromise?

varnishtest "include 4.0 from 4.1"

shell {
    cat << EOF > ${tmpdir}/inc.vcl
    vcl 4.0;
    sub vcl_recv {
        set req.esi = false;
    }
EOF
}

varnish v1 -syntax 4.1 -vcl {
    vcl push;
    include "${tmpdir}/inc.vcl";
    vcl pop;
} -start

[bsdphk]

I talked about this and #3584 with @Dridi, but I think his suggestion above needs a little bit more context :-)

The fundamental question is if include "file"; (and macro expansion, if we do that) is pure text-processing or has semantic significance.

If it is pure text-processing, tokens are inserted as instructed and then the complete token sequence is parsed, with no regard to where the individual tokens came from.

This allows gruesome hacks like:

backend include "backend_name"; { ... }
if (obj.ttl include "operator"; 120s) { ... }

The problem arises when the included file contains vcl x.y, because in a purely text-processing modus, that version continues to control after the included tokens.

Making include semantic on this point, amounts to "magically" inserting vcl x.y to restore the previous version if the included file contained any vcl tokens[1]. A really big downside of this is that it can cause really confusing error messages:

If x.vcl contains:

vcl x.y;
if (req.url = "/bla) {

Then the automatic version restoration makes any attempt to include that file a syntax error, complaining about a vcl token which the user cannot find in his source files, because it was automagically created.

So sticking with "pure text-processing" sounds like the best solution.

The the the question becomes what good tools can we give the users to manage vcl versions.

The vcl push; and vcl pop; @Dridi mentions above seems like good tools for the job, if given a few hand-rails.

By default (to be disabled by parameter), we should issue an error if the stack-depth is not preserved across an include (or macro expansion). This is a parse-time-check unfortunately, because simply counting vcl push|pop in the token stream will not be robust, but this is workable because we ban recursive includes.

Needless to say, more vcl pop than vcl push or lingering vcl push at the end of the token sequence should result in non-overridable errors.

[1] Because any token starting with vcl is reserved already, this crude check would be sufficient.

[dridi]

I talked about this and #3584 with @Dridi, but I think his suggestion above needs a little bit more context :-)

More context: we were discussing two approaches, one of them in progress on my end that basically consists in doing the push/pop implicitly, and the challenge of proper test coverage considering that includes are resolved before parsing begins. This approach would offload some complexity to the VCL author, but also remove some magic by being explicit about it, and finally explicit vcl {push,pop}; statements wouldn´t lead unexpected pops in the middle of a compound statement.

[nigoroll]

Bugwash today seemed to be in agreement to drop 4.0 support #3352