Limit GRPC Active streams #33936
Merged
Limit GRPC Active streams #33936
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703
jentfoo
requested review from
wadells,
reedloden,
tigrato,
zmb3,
codingllama,
rosstimothy and
adaadb6
jentfoo
added
the
no-changelog
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
|
I would run a scale test or wait for a full test plan before merging this one. |
zmb3
reviewed
Oct 26, 2023
rosstimothy
reviewed
Oct 26, 2023
There was a problem hiding this comment.
I share the same concerns as @zmb3 about putting this change into a release without any stress testing.
| @@ -100,6 +100,10 @@ const ( | |||
| // By default all users use /bin/bash | |||
| DefaultShell = "/bin/bash" | |||
|
|
|||
| // GRPCMaxConcurrentStreams is the max GRPC streams that can be active at a time. Once the limit is reached new | |||
There was a problem hiding this comment.
nit: slightly pedantic but it is gRPC not GRPC
|
I have some unrelated scale testing to do today. Will pull in this PR as well. |
|
Update: did a 30k agent scaling test with these changes included (2 auth/2 proxy/etcd backend). Didn't observe any ill effects in metrics/logs, and manually verified basic cluster functionality (login/UI navigation/ssh access). Seems peachy. |
fspmarshall
approved these changes
Oct 26, 2023
zmb3
approved these changes
Oct 27, 2023
public-teleport-github-review-bot
bot
removed request for
klizhentas,
wadells,
r0mant,
reedloden,
tigrato and
codingllama
public-teleport-github-review-bot
bot
removed the request for review
from adaadb6
jentfoo
added a commit
that referenced
this pull request
Oct 27, 2023
Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703
jentfoo
added a commit
that referenced
this pull request
Oct 27, 2023
Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703
jentfoo
added a commit
that referenced
this pull request
Oct 27, 2023
Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703
This was referenced Oct 27, 2023
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 27, 2023
* Limit GRPC Active streams (#33936) Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703 * Update gRPC to 1.58.3 to address GHSA-m425-mq94-257g
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 27, 2023
* Limit GRPC Active streams (#33936) Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703 * Update gRPC to 1.58.3 to address GHSA-m425-mq94-257g
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 27, 2023
* Limit GRPC Active streams (#33936) Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624 With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix requires this value to be configured in fact: grpc/grpc-go#6703 * Update gRPC to 1.58.3 to address GHSA-m425-mq94-257g
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally there was a default limit of 100 max concurrent streams, however in 2017 the GRPC team removed this default: grpc/grpc-go#1624
With the recent HTTP/2 Rapid Reset DoS, it is now being encouraged to re-introduce a limit. The fix actually requires this value to be configured: grpc/grpc-go#6703
I choose a value of
1000, which should be excessively large (10x the old default of 100).For that reason this fix will be backported. When we backport to v14 and older we will also update
grpc-goto the recent 1.58.3 which includes an additional fix (not needed inmasterdue to already being on 1.59.0)