Skip to content

vcl: Make 'none' a reserved keyword and use it for probes #4309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

vcl: Make 'none' a reserved keyword and use it for probes #4309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20e99de
vcc_symb: Improve error message for 'default'
dridi Apr 3, 2025
829c804
vcc: Make 'none' a reserved word
dridi Apr 3, 2025
10bbd83
vrt: Use a sentinel value for none backends
dridi Apr 4, 2025
bf1f80d
vrt: Use a sentinel value for none probes
dridi Apr 4, 2025
760d063
vcc: Set backend probe to none
dridi Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bin/varnishd/cache/cache_backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,8 @@ VRT_new_backend_clustered(VRT_CTX, struct vsmw_cluster *vc,
vbp = vrt->probe;
if (vbp == NULL)
vbp = VCL_DefaultProbe(vcl);
if (vbp != NULL && vbp->window == ~0U) /* none probe */
vbp = NULL;

if (vbp != NULL) {
VBP_Insert(be, vbp, be->conn_pool);
Expand Down
8 changes: 8 additions & 0 deletions bin/varnishd/cache/cache_vrt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ const struct vrt_blob *vrt_null_blob = &(struct vrt_blob){
.blob = "\0"
};

const struct director *const vrt_none_backend = NULL;

const struct vrt_backend_probe *const vrt_none_probe =
&(const struct vrt_backend_probe){
.magic = VRT_BACKEND_PROBE_MAGIC,
.window = ~0U,
};

/*--------------------------------------------------------------------*/

VCL_VOID
Expand Down
20 changes: 20 additions & 0 deletions bin/varnishtest/tests/c00136.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
varnishtest "none probe"

server s0 {
rxreq
txresp -status 500
} -dispatch

varnish v1 -vcl {
probe default {
.interval = 100ms;
}
backend s0 {
.host = "${s0_sock}";
.probe = none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also make sure please that this also works for a VCL_PROBE vmod function/method argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can pass none for a backends either, that would be a subsequent change (among a couple others) should this one be approved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always define a none backend and then pass that, so that is not so important. For probes we don't have that option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so you'd like to align this change with backend definitions?

probe disabled none;

backend improbable {
       .probe = none;
}

backend also_improbable {
       .probe = disabled;
}

That would increase overall consistency, good idea.

}
} -start

delay 1

varnish v1 -cliexpect healthy "backend.list -p"
6 changes: 3 additions & 3 deletions bin/varnishtest/tests/m00000.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ varnish v1 -errvcl {Symbol not found: 'obj'} {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the commit message:
As with other versioning questions, I think we should only increase the major version for silent breaking changes. In this case a VCL compilation failure ensures that VCL authors become aware of the change, so we do not need a VCL version bump.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think we should consider it a bug fix, we should never have authorized none to persist as a usable symbol name once we gave it a special meaning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

varnish v1 -errvcl {rot13: VFP already registered (per-vcl)} {
import debug;
backend none none;
backend be none;
sub vcl_init {
debug.rot104();
}
Expand All @@ -167,7 +167,7 @@ varnish v1 -errvcl {Failed from VCL} {

varnish v1 -errvcl {Failed initialization} {
import debug;
backend none none;
backend be none;
sub vcl_init {
new fails = debug.obj("fail");
}
Expand All @@ -177,7 +177,7 @@ shell {
cat >${tmpdir}/f1 <<-EOF
vcl 4.1;
import debug;
backend none none;
backend be none;
sub vcl_init {
new fails = debug.obj("fail");
}
Expand Down
2 changes: 1 addition & 1 deletion bin/varnishtest/tests/m00048.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ client c1 -repeat 2 {

varnish v1 -vcl {
import debug;
backend none none;
backend be none;

sub vcl_recv {
return (synth(200));
Expand Down
4 changes: 2 additions & 2 deletions bin/varnishtest/tests/r03940.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ varnishtest "test startup_timeout vs. stevedore init / open"
# startup_timeout used, delay in stevedore init
varnish v1 -arg "-sdebug=debug,dinit=5s -pstartup_timeout=3s -pcli_timeout=2s" \
-arg "-p feature=+no_coredump" \
-vcl "backend none none;" \
-vcl "backend be none;" \
-expectexit 0x40
varnish v1 -cliexpect \
"Child failed on launch within startup_timeout=3.00s" \
Expand All @@ -31,7 +31,7 @@ shell {grep -q "Child failed on launch within startup_timeout=3.00s" ${p1_out}}

varnish v2 -arg "-sdebug=debug,dopen=5s -pstartup_timeout=2s -pcli_timeout=3s" \
-arg "-p feature=+no_coredump" \
-vcl "backend none none;" \
-vcl "backend be none;" \
-expectexit 0x40
varnish v2 -cliexpect \
"launch within cli_timeout=3.00s .tip: set startup_" \
Expand Down
2 changes: 1 addition & 1 deletion bin/varnishtest/tests/t02027.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
varnishtest "H2 RxStuff conditions"

varnish v1 -arg "-p feature=+http2" -arg "-p debug=+syncvsl" -vcl {
backend none none;
backend be none;
sub vcl_recv {
return (synth(200));
}
Expand Down
4 changes: 2 additions & 2 deletions bin/varnishtest/tests/u00002.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ server s1 -repeat 4 {
} -start

varnish v1 -vcl+backend {
backend none none;
backend be none;

sub vcl_backend_fetch {
# XXX would like to do everything in v_b_e, but
# return(pass(x)) is not supported
if (bereq.url == "/hitpass") {
set bereq.backend = s1;
} else {
set bereq.backend = none;
set bereq.backend = be;
}
}

Expand Down
18 changes: 10 additions & 8 deletions bin/varnishtest/tests/v00021.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,6 @@ varnish v1 -errvcl {Unused sub foo, defined:} {
}
}

# deliberately testing for name "none"
varnish v1 -errvcl {Unused sub none, defined:} {
backend b { .host = "${localhost}"; }

sub none {
}
}

varnish v1 -errvcl {Invalid return "deliver"} {
backend b { .host = "${localhost}"; }

Expand Down Expand Up @@ -172,6 +164,16 @@ varnish v1 -errvcl {Symbol 'default' is a reserved word.} {
}
}

varnish v1 -errvcl {Symbol 'default' is a reserved word.} {
import debug;
sub vcl_init { new default = debug.obj(); }
}

varnish v1 -errvcl {Symbol 'none' is a reserved word.} {
import debug;
sub vcl_init { new none = debug.obj(); }
}

varnish v1 -syntax 4.1 -errvcl {(Only available when VCL syntax <= 4.0)} {
sub vcl_recv {
set req.esi = false;
Expand Down
4 changes: 4 additions & 0 deletions include/vrt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ typedef int vmod_event_f(VRT_CTX, struct vmod_priv *, enum vcl_event_e);
int VRT_acl_match(VRT_CTX, VCL_ACL, VCL_IP);

/* VCL_BACKEND */
extern VCL_BACKEND const vrt_none_backend;
VCL_BACKEND VRT_DirectorResolve(VRT_CTX, VCL_BACKEND);

/* VCL_BLOB */
Expand All @@ -542,6 +543,9 @@ int VRT_VSA_GetPtr(VRT_CTX, VCL_IP sua, const unsigned char ** dst);
VCL_BOOL VRT_ipcmp(VRT_CTX, VCL_IP, VCL_IP);
void VRT_Format_Proxy(struct vsb *, VCL_INT, VCL_IP, VCL_IP, VCL_STRING);

/* VCL_PROBE */
extern VCL_PROBE const vrt_none_probe;

/* VCL_REAL */
int VRT_REAL_is_valid(VCL_REAL);

Expand Down
27 changes: 27 additions & 0 deletions lib/libvcc/vcc_backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,25 @@ vcc_ParseProbe(struct vcc *tl)
free(p);
}

/*--------------------------------------------------------------------
*/

static void
vcc_EmitNoneProbe(struct vcc *tl)
{
static unsigned emitted = 0;

if (emitted)
return;

Fh(tl, 0,
"static const struct vrt_backend_probe vgc_probe_none[] = {{\n"
"\t.magic = VRT_BACKEND_PROBE_MAGIC,\n"
"\t.window = ~0U,\n"
"}};\n");
emitted = 1;
}

/*--------------------------------------------------------------------
* Parse and emit a backend host definition
*
Expand Down Expand Up @@ -403,6 +422,7 @@ vcc_ParseHostDef(struct vcc *tl, struct symbol *sym,
ifp = New_IniFin(tl);
VSB_printf(ifp->ini, "\t(void)%s;", vgcname);
VSB_printf(ifp->fin, "\t\t(void)%s;", vgcname);
sym->rname = "vrt_none_backend";
return;
}

Expand Down Expand Up @@ -534,6 +554,13 @@ vcc_ParseHostDef(struct vcc *tl, struct symbol *sym,
ERRCHK(tl);
} else if (vcc_IdIs(t_field, "probe") && tl->t->tok == ID) {
t_probe = tl->t;
if (vcc_IdIs(t_probe, "none")) {
vcc_EmitNoneProbe(tl);
Fb(tl, 0, "\t.probe = vgc_probe_none,\n");
vcc_NextToken(tl);
SkipToken(tl, ';');
continue;
}
pb = VCC_SymbolGet(tl, SYM_MAIN, SYM_PROBE,
SYMTAB_EXISTING, XREF_REF);
ERRCHK(tl);
Expand Down
22 changes: 19 additions & 3 deletions lib/libvcc/vcc_expr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1599,10 +1599,8 @@ static void v_matchproto_(sym_expr_t)
vcc_Eval_Default(struct vcc *tl, struct expr **e, struct token *t,
struct symbol *sym, vcc_type_t fmt)
{
(void)e;
(void)fmt;

(void)sym;
(void)t;

if (fmt->default_sym == NULL) {
VSB_cat(tl->sb, "Symbol 'default' is a reserved word.\n");
Expand All @@ -1613,6 +1611,19 @@ vcc_Eval_Default(struct vcc *tl, struct expr **e, struct token *t,
*e = vcc_mk_expr(fmt, "%s", fmt->default_sym->rname);
}

static void v_matchproto_(sym_expr_t)
vcc_Eval_None(struct vcc *tl, struct expr **e, struct token *t,
struct symbol *sym, vcc_type_t fmt)
{

(void)e;
(void)fmt;
(void)sym;

VSB_cat(tl->sb, "'none' is a reserved word.\n");
vcc_ErrWhere(tl, t);
}

/*--------------------------------------------------------------------
*/

Expand Down Expand Up @@ -1649,4 +1660,9 @@ vcc_Expr_Init(struct vcc *tl)
AN(sym);
sym->type = DEFAULT;
sym->eval = vcc_Eval_Default;

sym = VCC_MkSym(tl, "none", SYM_MAIN, SYM_FUNC, VCL_LOW, VCL_HIGH);
AN(sym);
sym->type = DEFAULT;
sym->eval = vcc_Eval_None;
}
11 changes: 8 additions & 3 deletions lib/libvcc/vcc_symb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,9 +604,14 @@ VCC_HandleSymbol(struct vcc *tl, vcc_type_t fmt)
vcc_ErrWhere(tl, sym->def_b);
return (sym);
} else if (sym != NULL && sym->kind != kind) {
VSB_printf(tl->sb,
"Name %.*s must have type '%s'.\n",
PF(t), sym->kind->name);
if (sym->type == DEFAULT) {
VSB_printf(tl->sb,
"Symbol '%.*s' is a reserved word.\n", PF(t));
} else {
VSB_printf(tl->sb,
"Name %.*s must have type '%s'.\n",
PF(t), sym->kind->name);
}
vcc_ErrWhere(tl, t);
return (sym);
}
Expand Down