varnish-cache PoC: introduce beresp private headers

The goal of this PoC is to enable improvement of the hit ratio through better standards compliance. It will serve as a reference for #3205 to build a VIP on better standards compliance. Reviews and feedback are still welcome independently of that work. Another goal is to throw my raw notes away and refer to this PoC for a bunch of issues I ran into.

Consider the use case where a resource can be cached, but the backend MAY give a cookie to the client if it's not presenting one:

Cache-Control: must-revalidate, max-age=300[, private]
[Set-Cookie: foo=bar]

Before hit-for-miss was introduced, the default behavior in the scenario where Varnish (on behalf of the client) didn't present a foo cookie and the backend as a consequence added the Set-Cookie header, there would have been a 120s hit-for-pass during which all subsequent requests would be passed to the backend. After that the next cache miss would either lead to a cacheable response, or rinse-and-repeat if once again the client is missing the dreaded cookie (or worse, we unset the cookie in vcl_recv because the resource is "static").

With hit-for-miss, the current default, in the scenario where the response contains a Set-Cookie header all subsequent requests would get the opportunity to supersede the hit-for-miss object with a proper one.

This PoC introduces a new default behavior that still leads to a hit-for-miss unless the backend responds with this variation in the Set-Cookie case:

Cache-Control: must-revalidate, max-age=300, private="set-cookie"
Set-Cookie: foo=bar

https://tools.ietf.org/html/rfc7234#section-5.2.2.6

It is now possible to cache the response if no other condition triggers the beresp.uncacheable flag. Only the client that triggered the fetch will see the Set-Cookie header, cache hits will see the redacted object.

This variation also works:

Cache-Control: must-revalidate, max-age=300, no-cache="set-cookie"
Set-Cookie: foo=bar

https://tools.ietf.org/html/rfc7234#section-5.2.2.2

This PoC however treats the #field-name form of the no-cache and private directives identically, storing private headers even though the RFC clearly states that we MUST NOT. More on that later.

To see it in action, here is a basic test case:

varnishtest "private headers"

server s1 {
	rxreq
	txresp -hdr {Cache-Control: no-cache="xkey", private="set-cookie"} \
	    -hdr "set-cookie: foo=bar" -hdr "xkey: abc, def" -hdr "foo: bar"
} -start

varnish v1 -vcl+backend {
	sub vcl_hit {
		if (obj.http.set-cookie || obj.http.xkey || !obj.http.foo) {
			return (fail);
		}
	}
	sub vcl_backend_response {
		if (!beresp.http.Set-Cookie.is_private) {
			return (fail);
		}
	}
} -start

logexpect l1 -v v1 -q "Debug:beresp.private" {
	expect * * Debug "beresp.private: xkey,set-cookie"
} -start

client c1 {
	txreq
	rxresp
	expect resp.status == 200
	expect resp.http.set-cookie == foo=bar
	expect resp.http.foo == bar
} -run

logexpect l1 -wait

client c2 {
	txreq
	rxresp
	expect resp.status == 200
	expect resp.http.set-cookie == <undef>
	expect resp.http.foo == bar
} -run

This test case confirms that the Set-Cookie header only reaches c1. That's also true for the XKey header. It also checks the response status because of the VCL sanity checks: we can see that the private headers are not just hidden from the client, but from VCL itself for the hit case. We can also see that the HEADER type grew an .is_private property.

The .is_private property is needed to have a VMOD-less solution to dismiss the Set-Cookie header in the built-in VCL. Here is the new vcl_backend_response:

sub vcl_backend_response {
    if (bereq.uncacheable) {
        return (deliver);
    } else if (beresp.ttl <= 0s ||
      (beresp.http.Set-Cookie && !beresp.http.Set-Cookie.is_private) ||
      beresp.http.Surrogate-control ~ "(?i)no-store" ||
      (!beresp.http.Surrogate-Control &&
        beresp.http.Cache-Control ~ "(?i:(private|no-cache)(?!=)|no-store)") ||
      beresp.http.Vary == "*") {
        # Mark as "Hit-For-Miss" for the next 2 minutes
        set beresp.ttl = 120s;
        set beresp.uncacheable = true;
    }
    return (deliver);
}

The notable changes are the handling of Set-Cookie and Cache-Control headers: we ignore private Set-Cookie headers and we also ignore no-cache and private Cache-Control directives if they take a value.

In order to implement the HEADER.is_private property this PoC is built on top of #3158. It highlights that it is impossible today to have a method or property attached to the HEADER type, because it is almost systematically turned into a STRINGS. This PoC spreads the conversion to multiple locations where it is effectively needed.

After getting some input from @hermunn I managed to find a solution that wouldn't break response headers order. My initial idea was to create a new "transient" object attribute only visible the client transaction that triggerred the fetch. This would have allowed to comply with the wording from the Cache-Control private directive:

If the private response directive specifies one or more field-names, this requirement is limited to the field-values associated with the listed response header fields. That is, a shared cache MUST NOT store the specified field-names(s), whereas it MAY store the remainder of the response message.

Again, with some input from @mbgrydeland I acknowledge that the storage infrastructure doesn't have the ability to do what I want, and don't want to engage that route. So I traded strict compliance with simplicity and merely changed how the headers pack from OA_HEADER is encoded. A single-byte marker is added, but only private headers pay this one byte overhead, maintaining forward compatibility of OA_HEADER's ABI for persistent caches.

From an application point of view, this is compliant because we can never access private obj.http.* by design^Waccident in VCL. Only inline C code or a VMOD could craft a VCL_HEADER capable of finding a private header in obj. Because the private marker "corrupts" private headers they are virtually NOT in storage, but in practice they are resident and may be extracted from RAM, a core file, or a persisted storage.

I could technically wipe private headers from OA_HEADER once they found their way to the relevant resp, but this is only a PoC and I didn't feel like smashing the read-only view of storage from the client's perspective. Also, could I even ensure that the client transaction wipes the private headers? What it it fails before reaching that step? Could the wipe happen too late for persistence? Maybe it should be the persistent storage's responsibility to wipe it on disk?

It should be understood at this point that this PoC is by no means complete, and it should definitely not be a single patch. Many details were omitted and marked with XXX comments or assertions. As a result two test cases should fail and panic because of missing error handling.

What the test case above doesn't show about this change is the introduction of a beresp.private variable in VCL. It can be read, but also set similar to how beresp.filters gets an initial value that can manually be overriden.

The initial value of beresp.private is extracted from no-cache and private Cache-Control directives using http_GetHdrField(), but this function doesn't take quoted-strings into account and can as a result be confused if it reaches a comma in the middle of a header list:

Cache-Control: public, private="Set-Cookie,XKey", max-age=300
(four fields)  ######  ################### #####  ###########

Ideas that I'm registering here for the lack of a better venue:

teach http_GetHdrField about quoted-strings

We probably also need a plural version (please, not callback-based) called http_GetHdrFields() for example in case we have directives spread over multiple Cache-Control headers before we collect it. Currently we can only get the first directive for a given name.

add a new reset action in VCL

For values that are computed, do the computation again:

sub vcl_backend_response {
    set beresp.Cache-Control = <expr>;
    reset beresp.ttl;
    reset beresp.grace;
    reset beresp.private;
}

stop adding SLTs for new variables

Having a SLT_Filters dedicated to beresp.filters may have been a wasted slot. Instead we could have an SLT_Field for current and future variables to log them when their value change:

Field beresp.filters: <string>
Field beresp.private: <string>
Field req.grace: <duration>

EOF

Feb 06 '20 07:02 dridi

Only one commit matters here: bd07815e1d51e6f76fcd9027f264e1acd95791e8.

And this was prompted by a real-world case where Varnish came up short.

Feb 06 '20 07:02 dridi

Preamble: I assume we agree that private headers can be implemented today to some extend (for a set of headers known a priori) by looking at obj.hits and changing the response in vcl_deliver {}. So in my mind, this PoC is not about something which cannot be done, but about about generalization, ease of use and more standards compliance by default.

I did not read much of the code, but i did read all of your excellent description. At this point, I would like to find an agreement if we want something like this and regarding the general implementation aspects.

On the general idea:

Yes, I want this feature in varnish-cache and I want it to work out of the box with the/a bundled vcl (following the idea that we might want a new builtin or standard VCL and keep the old behavior as an alternative).

On the implementation aspects you mentioned:

Regarding the storage for the private headers, I wonder if we couldn't write them to the request from the backend side: As they are not relevant for background fetches, we do not need to store them if we do not have a request, and also their lifetime is the client request.
Instead of the reset action I would propose to move parsing of the ttl/grace/keep timers to a bundled vmod and make it explicit in VCL. The implicit parsing has already been cause of a lot of confusion and I generally think that we should go back to making more of the internal mechanics explicit (and user visible) in VCL. So your reset example could look like this idea (with the hardest of all CS Problems still unsolved ;) )

rfc.parse_CacheControl(beresp.http.Cache-Control);

Regarding SLTs I passionately think you are wrong (until you convince me otherwise ;) ). For one, using less tags would move more of the parsing and filtering burden towards (prefix) string matching, but the killer point is that we want to make VSL contain structured data, in particular to generate better JSON on the VSL consumer side (ie varnishlog).

Feb 06 '20 09:02 nigoroll

Preamble: I assume we agree that private headers can be implemented today to some extend (for a set of headers known a priori) by looking at obj.hits and changing the response in vcl_deliver {}. So in my mind, this PoC is not about something which cannot be done, but about about generalization, ease of use and more standards compliance by default.

Yes, you can actively cache a response that wouldn't fly by the built-in vcl_backend_response but without VMOD support it's hard to work with arbitrary headers.

Is obj.hits reliable if more than one client was in the waiting list? Can the client originating the fetch see a positive number of hits despite coming from vcl_miss?

I did not read much of the code, but i did read all of your excellent description.

Thank you!

At this point, I would like to find an agreement if we want something like this and regarding the general implementation aspects.

As stated, this is meant to serve as a reference for the VIP that we concluded we would write for #3205. I hope to get more feedback from stevedore-minded folks, this is definitely just to show that it's *easily* within reach and all we need to do is agree on a direction. We have most tools in place and what we don't have isn't hard to implement.

Regarding the storage for the private headers, I wonder if we couldn't write them to the request from the backend side: As they are not relevant for background fetches, we do not need to store them if we do not have a request, and also their lifetime is the client request.

Like I said, I didn't want to take that route. The difference being that what I had in mind was to keep using storage to pass information back to the client, instead of accessing req directly (and for a bgfetch, you may not have one, another aspect that I avoided on purpose for this PoC).

The implicit parsing has already been cause of a lot of confusion and I generally think that we should go back to making more of the internal mechanics explicit (and user visible) in VCL.

Your suggestion implies to have an always-loaded VMOD, which I considered a show stopper, and took a detour to add a HEADER.is_private property instead (which luckily showed that it was impossible to do in the first place).

I also think that the implicit parsing, while it may not be perfect, reflects what we IMHO should do: come up with default values based on the protocol specification, and only then allow end-users to override the initial values.

The reset proposal is inspired by how certain variables can be unset, but (hopefully not just for me) implies that it will compute a value based on the current state. And of course it doesn't have to be implicit, the effect of a reset should be documented.

I agree with the "implicit" problem, so much that recently I tried to improve the situation in 0aec0c9e33178f7709936f4243e7592f35c6f114.

Regarding SLTs I passionately think you are wrong

Not my main focus, I will not try to argue that here :)

One minor thing I forgot to mention in the commit log, I added bound checks to HTTP_Decode() not because I needed them for the PoC but because I think we should do it and I happened to be in the neighborhood.

Feb 06 '20 11:02 dridi

Yes, you can actively cache a response that wouldn't fly by the built-in vcl_backend_response but without VMOD support it's hard to work with arbitrary headers.

I have not come across a case which could not be implemented in pure VCL as long as the potentially private headers are known a priory and I do not see why the basic scheme ...

sub vcl_deliver {
    if (obj.hits > 0 && resp.http.Cache-Control ~ "\bprivate=MyPrivateHeader") {
        unset resp.http.MyPrivateHeader;
    }
}

.... would not work.

Is obj.hits reliable if more than one client was in the waiting list?

It should be. See 382c5284d2812137fd336c1bcb3d7e4cde50f62f

Can the client originating the fetch see a positive number of hits despite coming from vcl_miss?

If it did, that was a bug.

Like I said, I didn't want to take that route. The difference being that what I had in mind was to keep using storage to pass information back to the client, instead of accessing req directly (and for a bgfetch, you may not have one, another aspect that I avoided on purpose for this PoC).

I wrote:

As they are not relevant for background fetches, we do not need to store them if we do not have a request

Can you please explain for which reason you want to put something into the object which has a lifetime exactly equivalent to that of the triggering request?

@mbgrydeland Dridi mentioned that you might be someone to comment on this also?

Your suggestion implies to have an always-loaded VMOD, which I considered a show stopper, and took a detour to add a HEADER.is_private property instead (which luckily showed that it was impossible to do in the first place).

It appears to me that you might have misunderstood or that I missed to explain properly:

I am suggesting the vmod for Cache-Control parsing, which you seem to imply with reset.

Regarding HEADER.is_private I agree with your proposal.

I also think that the implicit parsing, while it may not be perfect, reflects what we IMHO should do: come up with default values based on the protocol specification, and only then allow end-users to override the initial values.

Yes and no.

A default makes sense, but if it does not happen in the (builtin) VCL, it is not pola. Providing the same default but with a VCL call helps users understand what is happening.

The reset proposal is inspired by how certain variables can be unset, but (hopefully not just for me) implies that it will compute a value based on the current state. And of course it doesn't have to be implicit, the effect of a reset should be documented.

It's not that I would fundamentally oppose that proposal, I just think that calling some function which explicitly does something is better than reset to magic default.

I agree with the "implicit" problem, so much that recently I tried to improve the situation in 0aec0c9.

much appreciated :)

Feb 06 '20 11:02 nigoroll

This is interesting. My one problem here is that this pollutes the beresp/resp namespace. What this means is that anytime we touch a header, we now need to think about: is this header private? This pretty much adds a layer of complexity to all headers and all VCL logic which plays with headers. Also, behind the scenes, all header code needs to change to start dropping private headers. So storing these headers with the object is a bit dangerous since there is no telling when these headers can re-appear (mistakenly).

If this problem were sitting on my desk, I would simply make a new object: beresp_private/resp_private. Private headers would be put into beresp_private.http.* in vcl_backend_response. Those headers can be manipulated, added, deleted in vcl. The beresp_private object is then stored as a transient objected associated with the request. So this request has both a clean obj.* and a transient obj_private.http*. Then in vcl_deliver, you are presented with resp_private.http.*, you can do any final manipulations, and then its merged into the response.

After that request, the transient obj_private.http.* disappears and we are left with a clean obj.* without any private headers.

This would give us the needed namespace partitioning to make this entire thing work. Anything, code or VCL, touching or using beresp or resp would continue to work unchanged.

Feb 06 '20 13:02 rezan

@nigoroll

I have not come across a case which could not be implemented in pure VCL as long as the potentially private headers are known a priory and I do not see why the basic scheme ... [...] .... would not work.

That's the point, not needing to know beforehand, not requiring a tight sync between VCL and backends.

Can you please explain for which reason you want to put something into the object which has a lifetime exactly equivalent to that of the triggering request?

I didn't want to bypass the storage as a means to pass information from a backend to a client transaction. This was mainly in response to @hermunn's feedback on the approach I was initially considering, stashing private headers aside, that this would change the order of the headers (not that it should matter, I ~think~hope).

It appears to me that you might have misunderstood or that I missed to explain properly:

I am suggesting the vmod for Cache-Control parsing, which you seem to imply with reset.

I understood well, but I'm thinking under the constraint that the built-in VCL doesn't import VMODs. If that changes, different story, new can of worms...

A default makes sense, but if it does not happen in the (builtin) VCL, it is not pola.

Considering that the built-in kicks in *after* end-user VCL, then we need those initial values computed *before* entering vcl_backend_response. If suddenly the built-in parses the Cache-Control after a manual override of beresp.* variables then we are talking about a breaking VCL change of the same scale as what Varnish 4.0 brought (a bit dramatic, I agree=).

It's not that I would fundamentally oppose that proposal, I just think that calling some function which explicitly does something is better than reset to magic default.

Yes, it should be reset to documented default ;-)

@rezan

This is interesting. My one problem here is that this pollutes the beresp/resp namespace. What this means is that anytime we touch a header, we now need to think about: is this header private?

I understand what you mean, but I can't agree. I'm coming from the idea that VCL is not where we should actively make cache decisions, and for me that's the role of the backend. Unless you are dealing with a poor backend, the point of this change is to handle the logic transparently, so that you shouldn't have to think about it, unless you are not lucky in which case you can fiddle with beresp.private.

resp in vcl_deliver is and should remain oblivious to what comes from storage, whether it is a hit or a fetch.

If this problem were sitting on my desk, I would simply make a new object: [...] After that request, the transient obj_private.http.* disappears and we are left with a clean obj.* without any private headers.

What you are suggesting in terms of storage is what I was initially going after, not with a transient object, but a transient object attribute. @nigoroll suggested to allocate this directly somewhere in the struct req that initiated the fetch.

I think that in general we agree that this is just a PoC, and while "I traded strict compliance with simplicity" (quoting myself) this is mostly to show how we can kill two birds with one stone (better hit ratio and compliance).

I'm not saying this is how we should do this, I'm saying it's one approach that would satisfy forward compatibility for persistent caches. What if a persistent cache made sure not to persist private headers? They would only stay resident in memory. If we can agree on how not to allocate those durably without breaking other parts, then I'm all for it: unlike @hermunn I can live with a mangled beresp where headers are reordered without explicit VCL/VMOD operations :p

This would give us the needed namespace partitioning to make this entire thing work. Anything, code or VCL, touching or using beresp or resp would continue to work unchanged.

I think this is actually the other way around, my changes to VCL shouldn't break existing VCL (we even have the option to enable this only after a VCL bump to a future 4.2 version) and yours will pull the rug under existing VCL's feet.

It bothers me that suddenly Set-Cookie MAY or MAY NOT be available under beresp.http.Set-Cookie depending on what's inside the Cache-Control header. Contrary to what you suggest, any header manipulation (#3184) would have to check both namespaces and existing VCL would likely break!

On the other hand beresp.http.Set-Cookie.is_private is all I need to make this possible in the built-in VCL.

So far I'm happy with the feedback. Please bear in mind that this only shows one way to get there, and so far nobody is saying that we shouldn't get there in the first place :)

Feb 06 '20 16:02 dridi

Can you please explain for which reason you want to put something into the object which has a lifetime exactly equivalent to that of the triggering request?

I didn't want to bypass the storage as a means to pass information from a backend to a client transaction. This was mainly in response to @hermunn's feedback on the approach I was initially considering, stashing private headers aside, that this would change the order of the headers (not that it should matter, I ~think~hope).

@hermunn where is this (very surprising, in my mind) requirement to preserve header ordering coming from? I am not aware of anything like this in anything I ever read.

@Dridi if we put the private headers into storage, then the storage API should be explicit about them.

But I am still having a hard time understanding the argument. OK, we got one step further from "we want it so" to "because the storage should see it", but the next level is still unanswered: Why should the storage see the private headers.

A default makes sense, but if it does not happen in the (builtin) VCL, it is not pola.

Considering that the built-in kicks in after end-user VCL, then we need those initial values computed before entering vcl_backend_response. If suddenly the built-in parses the Cache-Control after a manual override of beresp.* variables then we are talking about a breaking VCL change of the same scale as what Varnish 4.0 brought (a bit dramatic, I agree=).

This is really a two-edged sword: If we put stuff into core code just to have proper defaults available for a custom VCL's sub, then we hide the functionality and make it less obviously what happens. Yet I agree that if we did this, users would have to (potentially) amend their VCL.

I would still argue that the latter is the better option.

Feb 10 '20 12:02 nigoroll

@hermunn where is this (very surprising, in my mind) requirement to preserve header ordering coming from? I am not aware of anything like this in anything I ever read.

I agree that the correct behavior of clients and servers is to ignore the order, but my spider sense tells me that there will be cases where applications break when you reorder the headers. The question is if we should care or not.

Feb 10 '20 12:02 hermunn

@hermunn with all due respect to your arachnal superpowers, we should design our software around the standard and leave special casing to special implementations, like a custom header sorting vmod.

Feb 10 '20 13:02 nigoroll

The order should only be important for same headers:

Accept-Language: fr
Accept-Language: en

vs

Accept-Language: en
Accept-Language: fr

I was mostly pulling @hermunn's leg, this shouldn't be a problem :)

Again, only a PoC, we don't have to do things this way.

Feb 10 '20 15:02 dridi

@Dridi In that case Accept-Language should be merged.

And yes, I think we all understand it's only a PoC, but in order to make progress, we should make decisions which lead to an implementation.

Feb 10 '20 15:02 nigoroll

We discussed private headers during the last VDD and have a more solid plan relying on header flags instead, we can close this ticket for now.

https://github.com/varnishcache/varnish-cache/wiki/VDD23Q1#compliance

May 15 '23 08:05 dridi

All the yak shaving leading to the PoC was merged in https://github.com/varnishcache/varnish-cache/pull/3344.

May 15 '23 08:05 dridi