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.

Sign up for GitHub

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

Jump to bottom

http2: make maximum invalid frames and maximum rejected streams configurable #30534

Closed
wants to merge 6 commits into from
Closed

http2: make maximum invalid frames and maximum rejected streams configurable #30534

wants to merge 6 commits into from

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Nov 18, 2019 •
edited
Loading

http2: replace direct array usage with struct for js_fields_

http2: allow to configure maximum tolerated invalid frames

test: update and harden http2-reset-flood

  • use new maxSessionInvalidFrames to lower the needed frames
  • slow down requests to generate less redundant after-session-close
    requests

http2: make maximum tolerated rejected streams configurable

Closes: #30505
This should also decrease the flakiness of test-http2-reset-flood.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Also, do we have a dedicated test for argument validation of createServer? I'm not sure where to put them.

/cc @nodejs/http2

@lundibundi lundibundi requested a review from addaleax November 18, 2019 20:20
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 18, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26715/

ZYSzys
ZYSzys reviewed Nov 19, 2019
View reviewed changes
doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
@ZYSzys
Copy link
Member

ZYSzys commented Nov 19, 2019

Also, do we have a dedicated test for argument validation of createServer? I'm not sure where to put them.

Seem like we actually don't have a dedicated test for argument validation of createServer, but we test things like this by adding a new test file called test-http2-too-many-*.js.

Maybe add a new test and name it as test-http2-too-many-invalid-frame.js is suitable here.

@ZYSzys ZYSzys added the http2 Issues or PRs related to the http2 subsystem. label Nov 19, 2019
addaleax
addaleax approved these changes Nov 19, 2019
View reviewed changes
lib/internal/http2/core.js Outdated Show resolved Hide resolved
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 19, 2019
@lundibundi lundibundi force-pushed the http2-invalid-frames-i30505 branch from 53d5a16 to f132068 Compare November 19, 2019 21:56
@lundibundi
Copy link
Member Author

Added dedicated test for maxSessionInvalidFrames that should be more reliable.
Added simple options validation testing for createServer/createSecureServer.
Fixed missing changelog for maxSessionRejectedStreams option.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26765/

addaleax
addaleax approved these changes Nov 19, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 19, 2019
@lundibundi lundibundi force-pushed the http2-invalid-frames-i30505 branch from f132068 to b9c679c Compare November 22, 2019 13:52
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26847/

@lundibundi
Copy link
Member Author

(nothing changed, force pushed to resolve conflicts)

vsemozhetbyt
vsemozhetbyt reviewed Nov 23, 2019
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Some doc nits.

doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
@lundibundi
Copy link
Member Author

@vsemozhetbyt done, thanks.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26884/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27059/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27083/

@lundibundi lundibundi mentioned this pull request Nov 27, 2019
Closed
3 tasks
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
Merged
BridgeAR added a commit that referenced this pull request Dec 3, 2019
@BridgeAR
2019-12-03, Version 13.3.0 (Current)
8906c52 
Notable changes:

* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
BridgeAR added a commit that referenced this pull request Dec 3, 2019
@BridgeAR
2019-12-03, Version 13.3.0 (Current)
f5fe384 
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    #30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
BridgeAR added a commit that referenced this pull request Dec 3, 2019
@BridgeAR
2019-12-03, Version 13.3.0 (Current)
4091ea9 
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    #30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
targos pushed a commit that referenced this pull request Jan 13, 2020
@lundibundi @targos
http2: replace direct array usage with struct for js_fields_
46cb0da 
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 13, 2020
@lundibundi @targos
http2: allow to configure maximum tolerated invalid frames
3bed1fa 
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 13, 2020
@lundibundi @targos
test: update and harden http2-reset-flood
518fd8f 
* use new maxSessionInvalidFrames to lower the needed frames
* slow down requests to generate less redundant after-session-close
  requests

PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 13, 2020
@lundibundi @targos
http2: make maximum tolerated rejected streams configurable
fb4f71b 
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
@sxa sxa mentioned this pull request Jan 21, 2020
Closed
2 tasks
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@lundibundi @BethGriggs
http2: replace direct array usage with struct for js_fields_
7998fbb 
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@lundibundi @BethGriggs
http2: allow to configure maximum tolerated invalid frames
7b26606 
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@lundibundi @BethGriggs
test: update and harden http2-reset-flood
8601394 
* use new maxSessionInvalidFrames to lower the needed frames
* slow down requests to generate less redundant after-session-close
  requests

PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@lundibundi @BethGriggs
http2: make maximum tolerated rejected streams configurable
a4ae272 
PR-URL: #30534
Fixes: #30505
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@puzpuzpuz puzpuzpuz mentioned this pull request Mar 8, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@devnexen devnexen devnexen approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Http2Session maximum invalid frame count configurable
7 participants
@lundibundi @nodejs-github-bot @ZYSzys @addaleax @jasnell @devnexen @vsemozhetbyt