cache_fetch: Send Content-Length: 0 for anything not GET or HEAD #4340
Conversation
|
bugwash: +1 from phk, walid passes on to @dridi |
dridi
left a comment
dridi
left a comment
There was a problem hiding this comment.
It popped up in my inbox at a moment when I was here to see it, so here goes my review.
| if (bo->bereq_body == NULL && bo->req == NULL) | ||
| http_Unset(bo->bereq, H_Content_Length); | ||
| if (bo->bereq_body == NULL && bo->req == NULL) { | ||
| const char *met = http_GetMethod(bo->bereq); | ||
| if (http_method_eq(met, GET) || | ||
| http_method_eq(met, HEAD)) | ||
| http_Unset(bo->bereq, H_Content_Length); | ||
| else | ||
| http_ForceHeader(bo->bereq, H_Content_Length, "0"); | ||
| } |
There was a problem hiding this comment.
I already shared my preference for VCL control over this behavior, at the expense of a new flag:
sub vcl_builtin_bereq_length {
if (req.method == "GET" ||
req.method == "HEAD" ||
req.method == "TRACE" ||
req.method == "OPTIONS" ||
req.method == "DELETE")
set bereq.length_needed = false;
}
}It can be limited to GET and HEAD, of course, having an extraneous cl:0 header shouldn't hurt anyway.
See #4331 (comment) for more details.
There was a problem hiding this comment.
This PR implements the default based on the insights from #4331
IMHO, We should discuss improving the VCL interface there.
Is there any reason we should not make the improvement suggested here?
There was a problem hiding this comment.
We can of course iterate and do this first.
| varnish v1 -vcl+backend { | ||
|
|
||
| sub vcl_backend_error { | ||
| if (bereq.method == "GETCL0") { | ||
| return (retry); | ||
| } | ||
| } | ||
| sub vcl_backend_fetch { | ||
| # trick to get a GET with a C-L: 0 | ||
| if (bereq.retries == 0 && bereq.method == "GET") { | ||
| set bereq.method = "GETCL0"; | ||
| return (error); | ||
| } | ||
| else if (bereq.method == "GETCL0") { | ||
| set bereq.method = "GET"; | ||
| } | ||
| return (fetch); | ||
| } | ||
| } -start |
There was a problem hiding this comment.
With my proposal the demo could look like this:
sub vcl_builtin_bereq_length {
# never suppress zero content length
return;
}Arguably less convoluted.
There was a problem hiding this comment.
The purpose of the vtc code you commented on is to demo what can be done today with the protected headers implementation as is. It was never intended to show a how VCL should look like, so commenting that, with better VCL support, it would look nicer, completely misses the point.
FWIW, the alternative proposal wold be:
set bereq.http.Content-Length = "0";But this is starting to get ridiculous.
There was a problem hiding this comment.
If we iterate from a core change to adding VCL control later, then I don't see the value of this "demo".
nigoroll
force-pushed
the
get_head_no_cl0
branch
2 times, most recently
from
8f979b7 to
3b259ec
Compare
|
@thomasklinger1234 the only way to know for sure if a PR is going to be in the next release is a merge. Whenever we or someone thought to make different predictions in the past, chances were very high that they turned out wrong. That said, if you have a concrete requirement to solve, you might want to look at https://github.com/varnishcache/varnish-cache/pull/4340/files#diff-9d1d0b70f01c3c5306ebd1b610e210697ed18e0dcb6b4af9f9245b01a7972c2d |
nigoroll
force-pushed
the
get_head_no_cl0
branch
from
3b259ec to
5975ca1
Compare
otherwise keep the existing logic of sending no Content-Length header if there is no request body. This approach was determined to be better than the inverse of sending C-L: 0 for specific methods to avoid backends sending 411 responses as noted in varnishcache#4331 (comment) This code runs before vcl_backend_fetch, so should VCL gain back control over Content-Length as proposed in varnishcache#4331, it will be possible again to override this default. Fixes varnishcache#4331 for the special case
nigoroll
force-pushed
the
get_head_no_cl0
branch
from
5975ca1 to
133aaa5
Compare
3 participants
otherwise keep the existing logic of sending no Content-Length header if there is no request body.
Supersedes #4332
This approach was determined to be better than the inverse of sending C-L: 0 for specific methods to avoid backends sending 411 responses as noted in #4331 (comment)
This code runs before vcl_backend_fetch, so should VCL gain back control over Content-Length as proposed in #4331, it will be possible again to override this default.
Fixes #4331 for the special case