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.

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

Collect cookie-headers when running h2 #2291

Closed
simonvik opened this issue Mar 30, 2017 · 2 comments
Closed

Collect cookie-headers when running h2 #2291

simonvik opened this issue Mar 30, 2017 · 2 comments
Assignees
Labels

Comments

@simonvik
Copy link

@simonvik simonvik commented Mar 30, 2017

When running h2 its common that the client sends multiple cookie headers to increase compression.

According to https://tools.ietf.org/html/rfc7540#section-8.1.2.5 a h2 -> h1 proxy should concat multiple cookies into one delimited by "0x3B 0x20"

Expected Behavior

That I only see one single req.http.cookie header delimited by "; "

Current Behavior

Varnish sends multiple Cookie-headers to the backend.

Possible Solution

  1. Collect cookie-headers default when running h2.
  2. Get std.collect( to take a delimiter option.
    std.collect(req.http.cookie, "; ");
@Dridi
Copy link
Member

@Dridi Dridi commented Mar 30, 2017

For more information on the wonderful world of cookies, I also recommend RFC 6265.

https://tools.ietf.org/html/rfc6265

Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 30, 2017
This done via a new http_CollectHdrSep function, implying a VRT minor
bump.

Refs varnishcache#2291
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 30, 2017
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 30, 2017
The default separator is now ", " to maintain the "pretty collapsing" of
the headers without risking an extra space with the "; " separator for
cookies.

Refs varnishcache#2291
Refs varnishcache#2292
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 30, 2017
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
This done via a new http_CollectHdrSep function, implying a VRT minor
bump.

Refs varnishcache#2291
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
The default separator is now ", " to maintain the "pretty collapsing" of
the headers without risking an extra space with the "; " separator for
cookies.

Refs varnishcache#2291
Refs varnishcache#2292
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
This done via a new http_CollectHdrSep function, implying a VRT minor
bump.

Refs varnishcache#2291
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
The default separator is now ", " to maintain the "pretty collapsing" of
the headers without risking an extra space with the "; " separator for
cookies.

Refs varnishcache#2291
Refs varnishcache#2292
Dridi added a commit to Dridi/varnish-cache that referenced this issue Mar 31, 2017
Dridi added a commit that referenced this issue Mar 31, 2017
This done via a new http_CollectHdrSep function, implying a VRT minor
bump.

Refs #2291
@Dridi Dridi closed this in d26a828 Mar 31, 2017
Dridi added a commit that referenced this issue Mar 31, 2017
The default separator is now ", " to maintain the "pretty collapsing" of
the headers without risking an extra space with the "; " separator for
cookies.

Refs #2291
Refs #2292
Dridi added a commit that referenced this issue Mar 31, 2017
@hermunn
Copy link
Member

@hermunn hermunn commented Apr 5, 2017

Backport review: H2 is not in Varnish 4.1, so there is nothing to backport here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.