feature: allow configuration of h2c max_concurrent_streams#8384
feature: allow configuration of h2c max_concurrent_streams#8384theJC wants to merge 5 commits intoopen-policy-agent:mainfrom
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
srenatus
left a comment
srenatus
left a comment
There was a problem hiding this comment.
Thanks for contributing! Code looks good, couple of comments.
More importantly: can you describe the measurable change you've seen from this code change in your environment? It would be interesting to know
- How you realized that the stream limit in h2c is a performance bottleneck
- How you verified that increasing the limit does fix the issue.
cmd/run.go
Outdated
| cmdParams.rt.DiagnosticAddrs = runCommand.Flags().StringSlice("diagnostic-addr", []string{}, "set read-only diagnostic listening address of the server for /health and /metric APIs (e.g., [ip]:<port> for TCP, unix://<path> for UNIX domain socket)") | ||
| cmdParams.rt.UnixSocketPerm = runCommand.Flags().String("unix-socket-perm", "755", "specify the permissions for the Unix domain socket if used to listen for incoming connections") | ||
| runCommand.Flags().BoolVar(&cmdParams.rt.H2CEnabled, "h2c", false, "enable H2C for HTTP listeners") | ||
| runCommand.Flags().Uint32Var(&cmdParams.rt.H2CMaxConcurrentStreams, "h2c-max-concurrent-streams", 0, "set maximum concurrent HTTP/2 streams per connection when H2C is enabled (0 uses the library default)") |
There was a problem hiding this comment.
| runCommand.Flags().Uint32Var(&cmdParams.rt.H2CMaxConcurrentStreams, "h2c-max-concurrent-streams", 0, "set maximum concurrent HTTP/2 streams per connection when H2C is enabled (0 uses the library default)") | |
| runCommand.Flags().Uint32Var(&cmdParams.rt.H2CMaxConcurrentStreams, "h2c-max-concurrent-streams", 0, "set maximum concurrent HTTP/2 streams per connection when H2C is enabled (default: <....>") |
Let's please be explicit here. Most users will now know what "library default" is supposed to mean here, or they'd need to go search to find out.
There was a problem hiding this comment.
NOTE: a default of 250 is due to the actual implementation (has been that way since 2015) — it's technically consistent with "at least 100" stated in https://pkg.go.dev/golang.org/x/net/http2 — but the guarantee the library makes is "at least 100", not specifically "250".
v1/server/server_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestWithH2CMaxConcurrentStreams(t *testing.T) { |
There was a problem hiding this comment.
Let's remove that one, it's not adding value, I think.
v1/test/e2e/h2c/h2c_test.go
Outdated
| // TestH2CMaxConcurrentStreams verifies that the server starts correctly and | ||
| // serves HTTP/2 requests when H2CMaxConcurrentStreams is configured. It does | ||
| // not verify that the stream limit is enforced; that would require negotiating | ||
| // concurrent streams and inspecting the HTTP/2 SETTINGS frame. |
There was a problem hiding this comment.
Makes you wonder if we need it, then, doesn't it? It doesn't test the change at hand, only insofar as that the server doesn't implode when the setting is set, which is a low bar. Do you think we can do something here to improve the test?
There was a problem hiding this comment.
updated to actually peek at the http2 settings frame value
Signed-off-by: Jon Christiansen <467023+theJC@users.noreply.github.com>
2 participants
Why the changes in this PR are needed?
Addresses #8330
When using OPA with h2c enabled (--h2c), the HTTP/2 server uses Go's default MaxConcurrentStreams value of 250. For high-throughput deployments where clients need to multiplex many concurrent policy evaluation requests over a single connection, this limit becomes a bottleneck.
The 250 concurrent streams limit is enforced per-connection, and while clients can open multiple connections, this defeats some of the benefits of HTTP/2 multiplexing and increases connection overhead. Additionally some client libraries specifically do not support opening multiple connections, citing: https://httpwg.org/specs/rfc9113.html#rfc.section.9.1
What are the changes in this PR?
Notes to assist PR review:
Nothing I can think of at the moment.
Further comments:
N/A