Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+82 −8
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 5 additions & 8 deletions bin/varnishd/cache/cache_req_fsm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,15 +484,12 @@ cnt_transmit(struct worker *wrk, struct req *req)
http_Unset(req->resp, H_Content_Length);
} else if (clval >= 0 && clval == req->resp_len) {
/* Reuse C-L header */
} else if (head && req->objcore->flags & OC_F_HFM) {
/*
* Don't touch C-L header (debatable)
*
* The only way to do it correctly would be to GET
* to the backend, and discard the body once the
* filters have had a chance to chew on it, but that
* would negate the "pass for huge objects" use case.
} else if (head) {
/* rfc9110,l,3226,3227
* "MAY send Content-Length ... [for] HEAD"
* do not touch to support cached HEAD #4245
*/
req->resp_len = 0;
} else {
http_Unset(req->resp, H_Content_Length);
if (req->resp_len >= 0)
Expand Down
77 changes: 77 additions & 0 deletions bin/varnishtest/tests/r04245.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
varnishtest "cache a HEAD as a fallback for a GET"

server s1 {
rxreq
expect req.method == "HEAD"
expect req.http.t == "headmiss"
txresp -nolen -hdr "Content-Length: 42"

rxreq
expect req.method == "GET"
expect req.http.t == "getmiss"
txresp -bodylen 42
} -start

varnish v1 -vcl+backend {
sub vcl_recv {
if (req.method == "HEAD") {
set req.http.X-Fetch-Method = "HEAD";
} else {
unset req.http.X-Fetch-Method;
}
}

sub vcl_backend_fetch {
if (bereq.http.X-Fetch-Method) {
set bereq.method = bereq.http.X-Fetch-Method;
}
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 {

}

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 (beresp.http.Vary) {
set beresp.http.Vary += ", X-Fetch-Method";
} else {
set beresp.http.Vary = "X-Fetch-Method";
}
}
set beresp.http.t = bereq.http.t;
}

sub vcl_deliver {
# Vary cleanup
if (resp.http.Vary == "X-Fetch-Method") {
unset resp.http.Vary;
} else if (resp.http.Vary ~ ", X-Fetch-Method$") {
set resp.http.Vary =
regsub(resp.http.Vary, ", X-Fetch-Method$", "");
}
}
} -start

client c1 {
# miss
txreq -method "HEAD" -hdr "t: headmiss"
rxresphdrs
expect resp.http.t == "headmiss"
# hit
txreq -method "HEAD" -hdr "t: headhit"
rxresphdrs
expect resp.http.t == "headmiss"

# miss
txreq -hdr "t: getmiss"
rxresp
expect resp.http.t == "getmiss"
# hits on full object
txreq -hdr "t: gethit"
rxresp
expect resp.http.t == "getmiss"
txreq -method "HEAD" -hdr "t: getheadhit"
rxresphdrs
expect resp.http.t == "getmiss"
} -run

server s1 -wait
Loading