Xkey: Allow comma between keys per RFC2616

[SpikesDivZero]

A possible fix for Issue #102

In this PR, I'm assuming that Varnish itself is handling folded header values for us.

One possible point where this may break from existing (invalid) assumptions is if an origin server is sending a surrogate key like "foo,bar", however RFC 2616 does suggest that this should be considered two separate headers having been combined into one.

[gquintard]

Looking pretty good, can you also have a test where you kill an object based on a tag at the end of the list? In you vtc, that would be "fdsa"

[hermunn]

I am concerned that this might break people's setups, if they actually rely on foo,bar being interpreted as a single key. I agree that the comma ideally should be interpreted as a separator, but there should be a way to not break previous behavior. Is sub vcl_init a good location for specifying how the comma should be interpreted? I am not sure.

[SpikesDivZero]

Pushed an updated commit, PR should update whenever GitHub's systems recover. Good catch, thanks. :)

Edit from web: This email reply was sent before hermunn commented; GitHub outage means this only just showed up.

[dridi]

Add a separator argument that defaults to whitespaces. Doesn't break existing VCL, adds comma-separated capabilities, doesn't need vcl_init-time state to maintain.

2cts from my phone

On Mon, Oct 22, 2018, 15:57 hermunn notifications@github.com wrote:

I am concerned that this might break people's setups, if they actually rely on foo,bar being interpreted as a single key. I agree that the comma ideally should be interpreted as a separator, but there should be a way to not break previous behavior. Is sub vcl_init a good location for specifying how the comma should be interpreted? I am not sure.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/varnish/varnish-modules/pull/124#issuecomment-431843238, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2bYK4v1SYXufKAbXRnY5mmV9-Y1NHDks5unc7NgaJpZM4XyrPo .

[gquintard]

we need to ponder on that one, I personally think we should go with what makes the most sense and not care to much about potentially non-existing setups.

[gquintard]

but let's get this into master now to avoid bitrot, we tend to ponder for a while sometimes

[gquintard]

thanks @SpikesDivZero !

[SpikesDivZero]

Thanks @gquintard et al.

Re: The fact that this is a potentially breaking change, I feel a bit conflicted, thus my disclaimer above. On one hand, this is a clear bug fix, where vmod_xkey was not obeying RFC. On the other hand, I've seen enough in the wild to know that it's entirely possible someone's doing this.

At a minimum, it should probably show up in CHANGES.rst, maybe even with a bit of bold highlighting the behavioral change.

With regards to a potential option in init, I'm concerned as to the potential increase in complexity and surface area for bugs. Currently, we don't have a priv; adding a priv would need to be plumbed through a good number of methods.