Default backend cannot contain a .via backend

[stevendore]

Expected Behavior

backend default to be allowed to use via backends.

Current Behavior

Panic on VCL compilation:

Message from VCC-compiler:
Assert error in vcc_ParseHostDef(), vcc_backend.c line 565:
  Condition((sym) != 0) not true.
Running VCC-compiler failed, signal 6, core dumped
VCL compilation failed  

Possible Solution

No response

Steps to Reproduce (for bugs)

Using c42.vtc as an example, changing the name of s2 to default results in a VCC panic. https://github.com/varnishcache/varnish-cache/blob/master/bin/varnishtest/tests/c00042.vtc#L51

varnish v1 -vcl {
    backend v2 { .host = "${v2_addr}"; .port = "${v2_port}"; }
    backend s1 { .via = v2; .host = "${s1_addr}"; .port = "${s1_port}"; }
    backend default { .via = v2; .host = "${s2_addr}"; .port = "${s2_port}"; }

    sub vcl_recv {
        if (req.url ~ "^/s1/") {
            set req.backend_hint = s1;
        } else if (req.url ~ "^/s2/") {
            set req.backend_hint = default;
        } else {
            return (synth(400));
        }
    }
} -start

Context

Using a proxy on backend with only one backend.

Varnish Cache version

varnish trunk, varnish 7.5

Operating system

No response

Source of binary packages used (if any)

No response

[nigoroll]

bugwash: I will look and ask @walid-git for a review when I have a PR

[nigoroll]

@walid-git @dridi I'd like to ask for your advice here: The problem is that the default backend does not have a symbol, and before we let this special casing proliferate more, I wonder if it could the be better option to have more than one "special" symbol named default, one for each kind ?

[dridi]

Why not add a can_default flag to struct type and raise it only for BACKEND and PROBE to have this special casing "generalized"? The symbol table could back this pseudo-symbol with an actual struct symbol.

I worry about allowing any type to overload a new symbol that comes with special properties for the types where this pseudo-symbol is currently allowed.

[nigoroll]

I gave this a try, but the fundamental problem seems to be that currently the vcc symbol code does not take into account multiple symbols by the same name and different types.

[dridi]

I gave it a try myself and came up with a minimal fix:

--- i/lib/libvcc/vcc_backend.c
+++ w/lib/libvcc/vcc_backend.c
@@ -732,7 +732,9 @@ vcc_ParseBackend(struct vcc *tl)
                        vcc_ErrWhere(tl, t_first);
                        return;
                }
-               vcc_NextToken(tl);
+               sym = VCC_SymbolGet(tl, SYM_MAIN, SYM_FUNC, SYMTAB_EXISTING,
+                   XREF_NONE);
+               AN(sym);
                dn = "vgc_backend_default";
                tl->default_director = dn;
        } else {

Since the "default" pseudo symbol can evaluate to either PROBE or BACKEND, my suggestion would be to add an internal DEFAULT type for the symbol to deal with the exceptional dual-typing. We could also wrap the VCC_SymbolGet() above with a new dedicated function, VCC_SymbolDefault() for example.

Thoughts?

[nigoroll]

I gave it a try myself and came up with a minimal fix:

WOW, I am impressed.

--- i/lib/libvcc/vcc_backend.c
+++ w/lib/libvcc/vcc_backend.c
@@ -732,7 +732,9 @@ vcc_ParseBackend(struct vcc *tl)
                        vcc_ErrWhere(tl, t_first);
                        return;
                }
-               vcc_NextToken(tl);
+               sym = VCC_SymbolGet(tl, SYM_MAIN, SYM_FUNC, SYMTAB_EXISTING,
+                   XREF_NONE);

I do not understand why SYM_FUNC and not SYM_BACKEND, could you explain? Really this is not a rhetorical question, I would really like to know

[nigoroll]

BTW, so you know more about my struggle, here are my attempts which I had meant to link to earlier: https://github.com/nigoroll/varnish-cache/tree/TRY_4177_vcc_default_backend

[dridi]

I do not understand why SYM_FUNC and not SYM_BACKEND, could you explain?

default is a pseudo symbol that may resolve (with an eval function) to either a PROBE or a BACKEND. It does have the type BACKEND under the hood, but that is inaccurate and mostly a play-pretend type.

I think I will submit a some kind of all-of-the-above patch series with this fix and the other suggestions I made.