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

send Varnish BAN requests with smaller headers #2865

Merged
merged 11 commits into from Mar 10, 2020

Conversation

jocel1
Copy link
Contributor

@jocel1 jocel1 commented Jun 17, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

The idea of this PR is to split the iris in several smaller chunks to avoid the "400 Request Header Or Cookie Too Large" on Varnish when the iris contains a large amount of elements.
CHUNK_SIZE is currently hardcoded, but if this could be controlled as a YAML config parameter, it would be even better.

@teohhanhui teohhanhui requested a review from dunglas June 17, 2019 14:10
@soyuka
Copy link
Member

soyuka commented Jun 18, 2019

Is there a limit on varnish? How did you come up with the value 100?

Nice improvement!

@bastnic
Copy link
Contributor

bastnic commented Jun 18, 2019

FYI, there is a chunk made in FosHTTPCache https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/src/ProxyClient/Varnish.php#L84 and there is a method dedicated to calculate how much tags fits in the http header : https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/src/ProxyClient/HttpProxyClient.php#L137

@jocel1
Copy link
Contributor Author

jocel1 commented Jun 18, 2019

@soyuka it's a really conservative value, but default values in Varnish are not that big (you have by default http_req_hdr_len=8k and http_req_size=32k in Varnish, but even with http_req_size=64k & http_req_hdr_len=4096k I was still experiencing 400 Bad Request issues).

@jocel1
Copy link
Contributor Author

jocel1 commented Jun 18, 2019

@bastnic thanks for the tips, this looks great! I'll check if I can improve this PR using the same kind of computation

@dunglas
Copy link
Member

dunglas commented Jun 26, 2019

👍 on my side! Copying (and crediting) the method from FOSHttpCache looks like a good idea. Could you also fix the PHPStan issue please?

@dunglas dunglas added this to the 2.5.0 milestone Jul 1, 2019
@jocel1 jocel1 changed the base branch from master to 2.4 August 22, 2019 09:53
@jocel1 jocel1 changed the base branch from 2.4 to master August 22, 2019 09:54
@jocel1 jocel1 force-pushed the varnish-purge-by-chunk branch 2 times, most recently from cc22c6c to 4d0e185 Compare August 22, 2019 10:16
@jocel1
Copy link
Contributor Author

jocel1 commented Aug 22, 2019

@dunglas changes done, adding a yaml config variable to set the max_header_length supported by the server + tests

@jocel1
Copy link
Contributor Author

jocel1 commented Dec 8, 2019

Hi @dunglas , any plan to merge this one ? :)

@Cocray
Copy link

Cocray commented Dec 17, 2019

Hi @dunglas , any plan to merge this one ? :)

Hello there! Should we have a release date for this feature? It's something we're waiting with excitement :)

src/HttpCache/VarnishPurger.php Outdated Show resolved Hide resolved
src/HttpCache/VarnishPurger.php Outdated Show resolved Hide resolved
src/HttpCache/VarnishPurger.php Outdated Show resolved Hide resolved
@jocel1
Copy link
Contributor Author

jocel1 commented Mar 4, 2020

Hi there! Any update on this one?

Co-Authored-By: Teoh Han Hui <teohhanhui@gmail.com>
@teohhanhui
Copy link
Contributor

phpstan failure not related.

@teohhanhui teohhanhui merged commit 3b8dc7d into api-platform:master Mar 10, 2020
@teohhanhui
Copy link
Contributor

Thanks @jocel1! 🎉 🚀

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

Successfully merging this pull request may close these issues.

None yet

6 participants