vcl: Make 'none' a reserved keyword and use it for probes #4309
Conversation
Getting "Name default must have type 'func'" is very confusing.
This is a breaking change, and alternatively, a new VCL 4.2 syntax could
treat it as a keyword while still retaining the current semantics with
VCL <= 4.1, but I think we should simply bite this bullet. Having it
both as a possible symbol name and a special keyword is sufficient to
justify this breakage.
Case in point, all the test cases affected by this change, showing usage
of 'none' as both a symbol name and a keyword:
backend none none;
In general, reserving this keyword could be a solution to clarify VCL.
In the future, these two blocks could become identical:
if (req.http.unsure) { }
if (req.http.unsure != none) { }
That would solve a long-standing confusion regarding header presence in
a message. Another actionable change this promotion to a reserved word
is leading to is the ability to ignore the default probe in a backend
definition.
This allows VMODs to create a backend without a probe, regardless of the presence of a default probe.
VCL authors can define a default probe for all backends without their own probe definitions. Until now, a backend could override the default probe with a different probe configuration. From now on, a backend can opt out of the default probe.
|
So this looks very sensible to me. Just one question: Is it really the best option to keep |
This is what I tried first, but that magic value would live in |
nigoroll
left a comment
nigoroll
left a comment
There was a problem hiding this comment.
I am fine with the VRT argument, but noticed one important aspect during a second review round
| debug.rot52(obj); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| backend s0 { | ||
| .host = "${s0_sock}"; | ||
| .probe = none; |
There was a problem hiding this comment.
Can we also make sure please that this also works for a VCL_PROBE vmod function/method argument?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@dridi bugwash is go when #4309 (comment) is addressed. |
|
Am I understanding the scope correctly?
In other words the current patch series amended with #4309 (comment) in mind. My goal was to start with this scope in the first place, and move the other talking points to #3844. |
|
@dridi your last comment aligns with my understanding. |
2 participants
This patch series does several things, but the effective goal is to enable this:
Having backends we can't probe also prevents the use of a default probe, so my suggestion here is to allow
noneas a special value for the.probefield in backend definitions.This is a breaking change, but I believe we should treat it as a bug fix and proceed without concern for broken VCL. In exchange, it should of course be one of the headlines in the release notes.
Once
nonebecomes a reserved word, we can imagine solving the recurring confusion around unset values:We could imagine the two if statements becoming equivalent:
In the⚠️ ACTUALLY BREAKING ⚠️ department, some would like to change the behavior of
set HEADER = HEADER;:I personally think it's an interesting question that should extend to
set HEADER = STRING;for null strings. For comparisons in general, it could apply to any expression that can yield a null pointer. Something to consider for the next revision of VCL.This pull requests stops after turning
noneinto the keyword it should have become on day one, in hindsight, and then uses this keyword to explicitly circumvent the default probe.