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

Double nginx's http2_max_header_size to 32k #2698

Merged
merged 3 commits into from Dec 29, 2020
Merged

Double nginx's http2_max_header_size to 32k #2698

merged 3 commits into from Dec 29, 2020

Conversation

TravisWhitehead
Copy link
Contributor

The Problem/Issue/Bug:

When cookies become large/numerous, decompressed request headers on HTTP/2 may exceed Nginx's http2_max_header_size limit which defaults to 16k

On Safari, exceeding this limit causes a crash and the error: kCFErrorDomainCFNetwork error 303

(This is symptomatic of hitting the limit on Nginx. If you exceed a similar limit from Apache you will get a real response.)

A workaround for the router is to use router-compose.override.yaml in the global ddev config directory to add a volume mounting additional nginx config correcting the issue. For the webserver you can add nginx config in the traditional/documented way.

How this PR Solves The Problem:

Increasing http2_max_header_size by default helps prevent issues when request headers become large (e.g. due to large/many cookies).

This increases the default value globally in the ddev router as well as in the webserver container (when using nginx-fpm). Originally I was only hitting this issue in the router (because I am using apache-fpm in the webserver container), but I think it's sensible to increase the limit in both locations where Nginx is used.

Some users dealing with large request headers might exceed this limit even at 32k, or they might exceed http2_max_field_size (the HPACK-compressed limit), or they might exceed one of Apache's limits-- but I figure we should start simple.

Manual Testing Instructions:

It's slightly painful to test/reproduce this because Safari drops the actual session cookies upon hitting that error, so I can't go back and see the exact request & header sizes that triggers this.

I have been testing by spinning up my app in ddev and then using the browser tools to add a few sizeable session cookies (maybe 1 or 2 KB large). I kept adding them until I hit kCFErrorDomainCFNetwork error 303. I then updated the Nginx limit, added cookies again, and verified that I could add a larger total size than before.

I tried increasing this config and then let my teammates see whether they hit the error as frequently, and this seemed to help (though we may find additional config is needed later).

Automated Testing Overview:

I've not added any automated tests in this PR.

Related Issue Link(s):

GitHub Discussion: #2697
Drupal Slack #ddev channel discussion: https://drupal.slack.com/archives/C5TQRQZRR/p1607733440364600

Release/Deployment notes:

  • Nginx's http2_max_header_size has been increased from the default value of 16k to 32k. This is a safe change for localdev purposes, but keep in mind that this means Nginx's header size limits in ddev will not match the default limits you may encounter using Nginx in other environments.

@CLAassistant
Copy link

CLAassistant commented Dec 16, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't see why this should be in a separate file. That makes total sense when adding to config on a specific host, but here it belongs in the nginx.conf I would think.

@TravisWhitehead
Copy link
Contributor Author

I've updated this to set the value in nginx.conf for both containers.

@rfay
Copy link
Member

rfay commented Dec 25, 2020

@mglaman since you've been in this territory lately could you please review this? Thanks!

@rfay
Copy link
Member

rfay commented Dec 27, 2020

I rebased this, pushed new images, and updated version.go so it would actually use these values.

I am slightly wondering if the webserver value should have been put into the per-CMS nginx configurations instead of the main nginx.conf. It would be more obvious there, but OTOH the main culprit in most cases is going to be the ddev-router, and that's harder to fiddle with.

TravisWhitehead and others added 3 commits December 28, 2020 15:26
Increasing http2_max_header_size by default helps prevent issues when
request headers become large (e.g. due to large/many cookies).

Discussion: #2697
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. Thanks! I'll be interested to hear what the issue was that you were encountering.

@rfay rfay merged commit b6c6f06 into ddev:master Dec 29, 2020
@smuccione
Copy link

While debugging similar issues in my own web server I came across this post. I noticed that the patch author was having problems repeating requests. I use the Charles web debugging proxy to do this. I have no relationship to the author other than having used his product for several decades. It allows you to view requests, but more importantly resend the request with original headers and body directly from the proxy instead of the source. This has been invaluable for repeated debugging for me and just wanted to let you know of the existence of this tool. It is a paid product (although they have a free fully featured 30 day trial). Again, I'm not affiliated and if this response violates a policy then please forgive me and erase it.

Best, Steve.

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

Successfully merging this pull request may close these issues.

None yet

4 participants