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

http,https: increase server headers timeout #30071

Closed

Conversation

@timcosta
Copy link
Contributor

timcosta commented Oct 22, 2019

Fixes: #24980
Refs: eb43bc0

Increases the default server headers timeout from 40 seconds to 60 seconds in order to not break compatibility with AWS ELBs in the default configuration.

Over the last several months, I have helped no fewer than 20 different groups of people in Slack teams resolve this issue. I believe that this is worth of a change even though it was originally a security fix because this change keeps intact the intended fix, but restores compatibility in the default configuration with any load balancer that prewarms TCP connections and holds them for over 40 seconds, just like AWS ELBs do.

Node in its default configuration should not be broken with AWS ELBs.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Fixes: #24980
Refs: eb43bc0
Copy link
Member

addaleax left a comment

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 30, 2019

Also @nodejs/security since the original introduction of this feature was in a security release

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

mcollina left a comment

LGTM

Copy link
Member

bnoordhuis left a comment

Seems like a reasonable change to me. But.

I remember thinking at the time that 40 seconds was a good default because lots of load balancers and reverse proxies probably operate with 30 second timeouts (because that's how people work, they're attracted to Schelling points.)

Perhaps then it's a good idea to set the timeout to just over 60 seconds, e.g., 65?

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Dec 14, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 14, 2019

Landed in e17403e

@Trott Trott closed this Dec 14, 2019
Trott added a commit that referenced this pull request Dec 14, 2019
Fixes: #24980
Refs: eb43bc0

PR-URL: #30071
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
Fixes: #24980
Refs: eb43bc0

PR-URL: #30071
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
MylesBorins added a commit that referenced this pull request Dec 18, 2019
Notable Changes:

* cli:
  * add --trace-exit cli option (legendecas)
    #30516
* http,https:
  * increase server headers timeout (Tim Costa)
    #30071
* readline:
  * update ansi-regex (Ruben Bridgewater)
    #30907
  * promote \_getCursorPos to public api (Jeremy Albright)
    #30687
* repl:
  * add completion preview (Ruben Bridgewater)
    #30907
* util:
  * add Set and map size to inspect output (Ruben Bridgewater)
    #30225
* wasi:
  * require CLI flag to require() wasi module (Colin Ihrig)
    #30963

PR-URL: #31010
MylesBorins added a commit that referenced this pull request Dec 18, 2019
Notable Changes:

* cli:
  * add --trace-exit cli option (legendecas)
    #30516
* http,https:
  * increase server headers timeout (Tim Costa)
    #30071
* readline:
  * update ansi-regex (Ruben Bridgewater)
    #30907
  * promote \_getCursorPos to public api (Jeremy Albright)
    #30687
* repl:
  * add completion preview (Ruben Bridgewater)
    #30907
* util:
  * add Set and map size to inspect output (Ruben Bridgewater)
    #30225
* wasi:
  * require CLI flag to require() wasi module (Colin Ihrig)
    #30963

PR-URL: #31010
@Ajido

This comment has been minimized.

Copy link
Contributor

Ajido commented Dec 19, 2019

For compatibility with ELB, not only headersTimeout but also server.keepAliveTiemout must be changed from 5s to over 60s, right?

In order to prevent errors due to the race condition of each other's connection timeout, all the connection timeouts inside the ELB must be greater than ELB's idle timeout.

@timcosta

This comment has been minimized.

Copy link
Contributor Author

timcosta commented Dec 19, 2019

@Ajido actually no, at least not in the context of this PR. This PR handles the fact that the AWS ELBs pre-warm TCP connections to the backing servers. This is not a connection that is subject to keep-alive until later on the connection lifecycle. The headersTimeout affects the connection only before the first request has been made over it. If you know of any other issues you may want to create a proof-of-concept and write them up. I've never seen an issue with keepAlive in the ~7 years i've been running node behind ELBs on AWS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.