cache_req_fsm: keep the cache object's Content-Length for HEAD always

[nigoroll]

Previously, we would only keep the Content-Length header for HEAD requests on hit-for-miss objects, now we simply keep it always to enable "fallback" caching of HEAD requests.

The added vtc implements the basics of the logic to enable the (reasonable) use case documented in
#2107 (comment) but using Vary instead of cache key modification plus restart.

Fixes #4245

[dridi]

LGTM, but this was not a thorough review.

[sbraz]

In this example configuration, the X-Fetch-Method headers can't be unset here before sending the request to the backend or it breaks the Vary part, right? I don't mind sending an extra header to my backend but it's one thing that differs from the restart-based solution.

[nigoroll]

Correct. We need to (un)set the header before the cache lookup such that the right variant gets hit, if present. For a miss, the header gets copied to the backend request and, when it completes (after vcl_backend_response {} returns), the header's value gets added to the Vary specification for that cache object.

So, in short, the header needs to be present during cache lookup and at the end of vcl_backend_response {}. For practical reasons, it is also needed to signal the backend side to activate the Vary handling.

With these requirements in mind, we can change the code to not send the header by deleting it in vcl_backend_fetch {} and restoring it in vcl_backend_response {}, but we need a vmod to do so. Here's how the test case adjustment looks like with a taskvar.bool object as a simple marker to activate the vary handling:

diff --git a/bin/varnishtest/tests/r04245.vtc b/bin/varnishtest/tests/r04245.vtc
index 27244e053..42982b79a 100644
--- a/bin/varnishtest/tests/r04245.vtc
+++ b/bin/varnishtest/tests/r04245.vtc
@@ -13,6 +13,12 @@ server s1 {
 } -start
 
 varnish v1 -vcl+backend {
+    import taskvar;
+
+    sub vcl_init {
+       new vary_x_fetch_method = taskvar.bool();
+    }
+
     sub vcl_recv {
        if (req.method == "HEAD") {
            set req.http.X-Fetch-Method = "HEAD";
@@ -24,13 +30,17 @@ varnish v1 -vcl+backend {
     sub vcl_backend_fetch {
        if (bereq.http.X-Fetch-Method) {
            set bereq.method = bereq.http.X-Fetch-Method;
+           # use marker to avoid sending the header to the backend
+           unset bereq.http.X-Fetch-Method;
+           vary_x_fetch_method.set(true);
        }
     }
 
     sub vcl_backend_response {
        # NOTE: this use of Vary is specific to this case, it is
        # usually WRONG to only set Vary for a specific condition
-       if (bereq.http.X-Fetch-Method) {
+       if (vary_x_fetch_method.get()) {
+           set bereq.http.X-Fetch-Method = bereq.method;
            if (beresp.http.Vary) {
                set beresp.http.Vary += ", X-Fetch-Method";
            } else {

For the purpose within the varnish-cache tree, we only use bundled vmods, so this change can not be applied to the proposed patch.

An even simpler way would be to use bereq.method == "HEAD" as the marker in vcl_backend_response {}, which should be possible if the additional logic is only used for HEAD. That is, it should work exactly as in the test case, but might cause trouble in real world VCL:

diff --git a/bin/varnishtest/tests/r04245.vtc b/bin/varnishtest/tests/r04245.vtc
index 27244e053..44edbd5bc 100644
--- a/bin/varnishtest/tests/r04245.vtc
+++ b/bin/varnishtest/tests/r04245.vtc
@@ -24,13 +24,15 @@ varnish v1 -vcl+backend {
     sub vcl_backend_fetch {
        if (bereq.http.X-Fetch-Method) {
            set bereq.method = bereq.http.X-Fetch-Method;
+           unset bereq.http.X-Fetch-Method;
        }
     }
 
     sub vcl_backend_response {
        # NOTE: this use of Vary is specific to this case, it is
        # usually WRONG to only set Vary for a specific condition
-       if (bereq.http.X-Fetch-Method) {
+       if (bereq.method == "HEAD") {
+           set bereq.http.X-Fetch-Method = bereq.method;
            if (beresp.http.Vary) {
                set beresp.http.Vary += ", X-Fetch-Method";
            } else {

[nigoroll]

notes from bugwash:

• There should be a way for VCL to stop sending C-L.

my own homework:

• understand why the current code works for pass