Repository navigation
Conversation
cloudflared configures its HTTP/2 tunnel server with MaxConcurrentStreams = math.MaxUint32. With Go 1.27 and golang.org/x/net v0.61.0, http2.Server is backed by net/http, which converts the limit to an int and replaces anything above math.MaxInt32 with its default of 250. Each HTTP/2 tunnel connection was therefore silently limited to 250 concurrent streams. Use math.MaxInt32 instead. Fixes cloudflare#1777 Signed-off-by: Ahmed Laban <amlaban@outlook.com>
Read the SETTINGS frame the HTTP/2 server actually sends and check that SETTINGS_MAX_CONCURRENT_STREAMS equals MaxConcurrentStreams. Checking the configured http2.Server field alone would not catch the fallback to 250 streams. Signed-off-by: Ahmed Laban <amlaban@outlook.com>
This file contains hidden or 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
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.
Fixes #1777
Problem
connection.MaxConcurrentStreamsismath.MaxUint32, and the HTTP/2 tunnel server uses it ashttp2.Server.MaxConcurrentStreams. With Go 1.27.3 and golang.org/x/net v0.61.0,http2.Serveris backed by net/http. The adapter converts the limit to anint, and values abovemath.MaxInt32are replaced with the default of 250 (see golang/go#82497). So every HTTP/2 tunnel connection advertisesSETTINGS_MAX_CONCURRENT_STREAMS = 250instead of an effectively unlimited value. Long-lived requests can then use up the stream slots for everything else on that connection.Change
connection/connection.go: setMaxConcurrentStreamstomath.MaxInt32, with a comment explaining why it must fit in an int32. It is only used for the HTTP/2 server.connection/http2_test.go: addTestHTTP2AdvertisedMaxConcurrentStreams. It starts the HTTP/2 connection overnet.Pipe, sends the client preface, reads the first frame withhttp2.Framer, and checks that the SETTINGS frame advertisesMaxConcurrentStreams. It checks what is sent on the wire, as the issue suggests, because the configured field alone would not show the fallback.Testing
Built with Go 1.27.3 (matching go.mod) and golang.org/x/net v0.61.0:
0xffffffff, got0xfa(250).go test ./connectionpasses,go vet ./connectionis clean, and the changed files are gofmt-clean.Not run:
make lint(golangci-lint) and the component tests. I also didn't run the new test with thehttp2legacybuild tag.Related
#1773 also edits
connection/http2.go(ping health checks). This PR doesn't touch that file, so they shouldn't conflict.