8.0 Compliance improvements

[bsdphk]

We have agreed to improve RFC compliance in the next "big" release (7.0/vcl x.y)

This ticket is meant to be the coordination point for this, in particular for the necessary documentation updates.

Each of the subtasks will have their own tickets, listed here:

• Followup for #3014 (502 status) #3016 (status 500, 502, 503 or 504)
• beresp 304 header merging makes impossible to distinguish between headers set by varnish and backend #3102 (304 headers update)
• Implement 412 response for non-GET/HEAD Cond fetch #3121 (status 412)
• why do we not use 413 / 414? #3205 (status 413/414)
  • We could consider other 4XX like 431
• hash: Transfer waiting list ownership to the objcore #3992 (objcore waiting list, no-cache premises)
• Cache-Control not respected for HTTP 303 responses #3340 (cache-control precedence)
• Implementing the Cache-Control "must-revalidate" directive #3364 (cache-control must-revalidate)
• reject h2 req with connection-specific header (see 9210c46)
• add support for doing expect-100-continue dance with backend.

Completed Subtasks for 7.0:

• Handle changing Content-Encoding for inconsistent backends? #3169 (inconsistent backends)
• We set Accept-Range on pass, is that OK ? #3251 (range headers)
• vcl: Allow header names to be quoted (compliance improvements) #3638 (quoted headers)
• HTTP case sensitivity hardening (compliance improvements) #3650 (case sensitivity)
• vrg: Consistency check of backend responses (compliance improvements) #3673 (content-range headers)
• vcl: Allow header names to be quoted (compliance improvements) #3638 (quoted headers)

Discarded Subtasks:

• add multiple ranges support? RFC7233
• vct: Strict HTTP/1 CRLF parsing #3668 (HTTP/1 strict parsing)
• PoC: introduce beresp private headers #3211 (private headers)

Todo list:

• RFC9110 7.6.1 says that "Proxy-Connection" header should be filtered.

[dridi]

Registering the idea here: we should try to improve RFC coverage in the code as we make progress in this area.

By that I mean comments like:

varnish-cache/bin/varnishd/cache/cache_req_fsm.c

Lines 414 to 420 in e896529

	if (status < 200 || status == 204) {
	// rfc7230,l,1691,1695
	http_Unset(req->resp, H_Content_Length);
	} else if (status == 304) {
	// rfc7230,l,1675,1677
	http_Unset(req->resp, H_Content_Length);
	} else if (clval >= 0 && clval == req->resp_len) {

[dridi]

I'm wondering whether VIP24 also falls under the compliance umbrella.

[ThijsFeryn]

I had a look at the Edge Architecture Specification, and noticed something that is not properly implemented in our vcl_backend_response logic.

The Surrogate-Control response header contains several directives that influence entity cacheability; specifically, "no-store", "no-store-remote", and "max-age" (see "Surrogate-Control Header" for more information). Collectively, these directives and their behaviors are described by the capability token

Surrogate/1.0

This token should be included in all requests sent by compliant surrogates (see "Surrogate-Capabilities Header").

When any of these directives are present, they override any HTTP cacheability information present in the response.

Surrogate-Control: no-store

In https://github.com/varnishcache/varnish-cache/blob/master/bin/varnishd/builtin.vcl#L154, we unconditionally allow Surrogate-Control: no-store to have precedence over Cache-Control.

According to the spec Surrogate-Capability: varnish="Surrogate/1.0" should be set first.

If we want to improve compliance, we could change the Surrogate check as follows:

bereq.http.Surrogate-Capability  ~ "(?i)Surrogate\/1\.0" && beresp.http.Surrogate-control ~ "(?i)no-store"

Surrogate-Control: max-age

The Surrogate-Control header also supports max-age. Setting the TTL of an object based on this, is a bit more complicated, as it is not part of builtin.vcl.

This would be a change in the core. But the same check on Surrogate-Capability: varnish="Surrogate/1.0" should happen before allowing Surrogate-Control: max-age=30 to take precedence over Cache-Control: max-age=50, for example.

Setting a Surrogate-Capability header in builtin.vcl

If we really want to support surrogates, it would also make sense to already set a Surrogate-Capability header in the vcl_backend_fetch subroutine of the builtin.vcl:

set bereq.http.Surrogate-Capability = "varnish=\"Surrogate/1.0 ESI/1.0\"";

That would allow the origin to know that we support both the surrogate caching syntax and ESI.

Processing ESI based on the Surrogate-Control header

If we send a proper Surrogate-Capability header announcing ESI support, we can just as wel process ESI when the origin sends us a Surrogate-Control header.

We could add the following to vcl_backend_response:

if(beresp.http.Surrogate-Control ~ "content=\"ESI/1\.0\"") {
    set beresp.do_esi = true;
}

If this would undermine any other proxy or CDN that has ESI capabilities, we can use device targeting to make it more specific. The corresponding VCL code would then look like this:

if(beresp.http.Surrogate-Control ~ "content=\"ESI/1\.0\";varnish") {
    set beresp.do_esi = true;
}

Next steps

• Is this level of compliance in regards to surrogates of interest here?
• Should I pursue this?
• Should this be part of PoC: introduce beresp private headers #3211?
• Should I split this up into 2 different issues? One for builtin.vcl, and one for the TTL?
• Does it make sense to announce our own surrogate capabilities using the proper header?
• Does it make sense to process ESI automatically if the right Surrogate-Control header is set?

[dridi]

I added a list of parsing mistakes in the issue description, two items I'm aware of so far.

[karptonite]

@ThijsFeryn

• Is this level of compliance in regards to surrogates of interest here?
• Should I pursue this?
• Does it make sense to process ESI automatically if the right Surrogate-Control header is set?

I'm super late to this, but I would certainly make use of at least two of these improved use of the Surrogate-Control headers. In particular, we use ESI, and have custom handling in our vcl to process ESI based on that header. It's only a few lines, but it would be simpler for everyone using ESI if those few lines were unnecessary.

Also, I sometimes run into an issue when I want to set Cache-control no-cache for the browse, but cache on Varnish. Being able to override the no-cache in Surrogate-Control with a max-age would simplify things, again removing the need for custom VCL.