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

Bump node to 10.15.1 #27918

Merged
merged 11 commits into from Feb 5, 2019

Conversation

Projects
None yet
4 participants
@jbudz
Copy link
Contributor

jbudz commented Jan 2, 2019

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V10.md#10.15.0, notably adding support for the --max-http-header-size node option.

Closes #27724

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 2, 2019

jbudz added some commits Jan 2, 2019

@mistic

mistic approved these changes Jan 2, 2019

Copy link
Member

mistic left a comment

LGTM

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 2, 2019

@tylersmalley

This comment has been minimized.

Copy link
Member

tylersmalley commented Jan 4, 2019

Should we be setting max-http-header-size to prevent the regressions seen in #27724?

@mistic

This comment has been minimized.

Copy link
Member

mistic commented Jan 4, 2019

@tylersmalley it looks like a good idea to me! Maybe we could set this flag by default with the the apache default of 8K for example?

@jbudz

This comment has been minimized.

Copy link
Contributor Author

jbudz commented Jan 4, 2019

I think we should if we're hitting the limits by default, but if not I'd push for leaving it. The limit was added to prevent dos so the harder the limit the better imo. I wasn't able to tell if the original report had a proxy so I'll try and reproduce by exporting csv, will report back shortly.

I do think we should make it easier to set though, either via documentation or revisiting #7710 (especially now that there's more flags like --no-warnings etc)

@jbudz

This comment has been minimized.

Copy link
Contributor Author

jbudz commented Jan 4, 2019

Request was 1.8kb, response was 1.1kb on cloud. I didn't see anything that would expand with request size so root cause is possibly a proxy with more headers.

@tylersmalley

This comment has been minimized.

Copy link
Member

tylersmalley commented Jan 4, 2019

According to https://discuss.elastic.co/t/bad-request-when-generating-csv-hpe-header-overflow/161831 it looks like folks are hitting it with CSV export.

@mistic

This comment has been minimized.

Copy link
Member

mistic commented Jan 22, 2019

Well with this CSV export use case I think maybe we can setup a default value so. What do you think @jbudz ?

jbudz added some commits Jan 23, 2019

@jbudz

This comment has been minimized.

Copy link
Contributor Author

jbudz commented Jan 23, 2019

I was able to reproduce the issue after digging further, and there's potential for field size/count to trigger it. We don't enforce any limits so creating a scripted field that's 8000+ characters is one way to reproduce it. Or putting a proxy in front that adds cookies. I guess my protests before were confusion, I'm not sure why any of that(field name) data is counted as part of the header limit.

The description implies the 8kb decision is oriented towards avoiding unlimited headers than any limit in particular, so I bumped it to 64kb which should give us more room. 64kb was arbitrary, and can be overridden. I didn't base it on anything other than we know some people are hitting 8kb.

@jbudz jbudz referenced this pull request Jan 23, 2019

Closed

Upgrade to node 10.15/8.15 #27724

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 23, 2019

jbudz added some commits Jan 23, 2019

s
@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 23, 2019

@jbudz

This comment has been minimized.

Copy link
Contributor Author

jbudz commented Jan 24, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jan 24, 2019

@@ -14,7 +14,7 @@ If Not Exist "%NODE%" (
Exit /B 1
)

"%NODE%" %NODE_OPTIONS% --no-warnings "%DIR%\src\cli" %*
"%NODE%" --no-warnings --max-http-header-size=65536=65536 %NODE_OPTIONS% "%DIR%\src\cli" %*

This comment has been minimized.

@tylersmalley

tylersmalley Feb 4, 2019

Member

Looks like the value is duplicated

jbudz added some commits Feb 4, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Feb 4, 2019

@jbudz jbudz changed the title Bump node to 10.15.0 Bump node to 10.15.1 Feb 4, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Feb 4, 2019

@jbudz jbudz merged commit cfe374c into elastic:master Feb 5, 2019

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

jbudz added a commit that referenced this pull request Feb 5, 2019

Bump node to 10.15.1 (#27918)
* Bump node to 10.15.0

* newline

* -1 newline

* bump max header size to 64kb

* fix quotes

* s

* space

* Bump node to 10.15.1

* fix flag arg

jbudz added a commit that referenced this pull request Feb 5, 2019

Bump node to 10.15.1 (#27918)
* Bump node to 10.15.0

* newline

* -1 newline

* bump max header size to 64kb

* fix quotes

* s

* space

* Bump node to 10.15.1

* fix flag arg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment