PLT-1125: Bound concurrent in-flight RPCs and connections per IP on the gRPC query plane - #4078
PLT-1125: Bound concurrent in-flight RPCs and connections per IP on the gRPC query plane#4078amir-deris wants to merge 3 commits into
Conversation
Per-IP admission on the gRPC query plane charges one token at the HTTP/2 HEADERS frame and records the admitted IP on the RPC context. A client can spend a token on headers alone, withhold the request body while the bucket refills, repeat across streams, and release every body at once. The token bucket smooths arrival rate; it caps no concurrency. Two orthogonal controls close that, each useful on its own. Per-IP connection cap. ConnLimitListener bounds simultaneously-open connections per client address on :9090 and :9091, wrapping the raw listener inside the global cap so a refused connection never spends a global slot. Over-cap connections are closed rather than surfaced as an Accept error, which would turn one client's excess into an outage for everyone else. It applies whether or not rate limiting is enabled. Per-IP in-flight RPC cap. RateLimitTapHandle takes a slot at the HEADERS frame, after the token check, and InFlightStatsHandler returns it on stats.End -- the only hook that brackets a stream the tap admitted, whatever ends it. An interceptor cannot: grpc-go reaches one only after decoding the request message, which is precisely the event a stockpiling client withholds. Two failure modes shape the implementation. grpc-go answers an unknown or malformed method name without emitting stats at all, so acquisition is gated on the registered method set: a leaked slot fails closed and locks an IP out permanently, which is worse than the burst. And gRPC-Web reaches the same handleStream through ServeHTTP and emits the same stats events without ever running the tap, so RateLimitHTTPMiddleware releases its own slot under a defer and leaves no context marker, keeping the stats handler from returning it twice. New keys, all read behind presence guards: [grpc] max-connections-per-ip, [grpc-web] max-connections-per-ip, and [grpc] max-in-flight-per-ip, each defaulting to 100. Rejections are counted by rpc_inflight_rejected_total and rpc_connection_rejected_total, siblings of rpc_rate_limit_rejected_total so an operator can tell the controls apart. collectRejectionMetrics now shares one reader across the package. It installed a fresh meter provider per test, but the otel global takes only the first, so a second caller left the earlier test collecting nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4078 +/- ##
==========================================
- Coverage 61.23% 60.22% -1.02%
==========================================
Files 2177 2072 -105
Lines 190632 178395 -12237
==========================================
- Hits 116729 107433 -9296
+ Misses 62892 60994 -1898
+ Partials 11011 9968 -1043
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Preserve pre-upgrade behavior unless an operator opts in to a per-IP connection cap. Update CHANGELOG to match. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview Connections: optional Concurrent RPCs: Both planes share the same per-IP keying as rate-limit buckets (including IPv6 /64). Config, Reviewed by Cursor Bugbot for commit e6fcbf8. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The per-IP in-flight slot design (tap acquire + stats.End release, with the unknown-method exemption to avoid leaks) is careful and well tested, and the listener/config wiring is clean. The blocking issue is that the new connection cap keys on the raw TCP peer and therefore ignores trusted-proxy-cidrs, contradicting both its godoc and the CHANGELOG upgrade guide in a way that would lead an operator behind an ingress to cap their entire client population at one allowance.
Findings: 1 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
[grpc-web] max-connections-per-iphas no test:TestStartGRPCServer_ConnectionsPerIPCapRefusesExcessonly covers the :9090 listener, and it runs withrate-limiting-enabled = true, so the CHANGELOG's claim that the connection cap applies "regardless ofrate-limiting-enabled" (the wiring is outside the rate-limit branch in both start functions) is not pinned by anything. A case withRateLimitingEnabled: falseplus a positiveMaxConnectionsPerIP, and one drivingStartGRPCWebwithGRPCWeb.MaxConnectionsPerIPset, would cover both promises. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // share of the global connection budget. Wrap the raw listener with this before | ||
| // the global cap, so a connection this rejects never spends a global slot. | ||
| // | ||
| // Addresses are keyed the way rate-limit buckets are, so a client rotating |
There was a problem hiding this comment.
[blocker] This claim does not hold when trusted-proxy-cidrs is set. Accept keys on conn.RemoteAddr(), which at accept time is always the proxy's address — there is no request and no X-Forwarded-For yet — so the connection cap cannot use the forwarded client IP that Registry.IPFromHTTPRequest / IPFromGRPCContext use for the rate-limit buckets. The two keyings therefore diverge exactly in the deployment trusted-proxy-cidrs exists for.
The CHANGELOG upgrade guide states the opposite and makes it operationally load-bearing: "Keying matches the rate-limit buckets: … as does any population behind a single egress address that trusted-proxy-cidrs does not cover", immediately followed by "Operators exposing public gRPC query endpoints may want to set [grpc] max-connections-per-ip (for example to 100)". An operator whose ingress CIDRs are in trusted-proxy-cidrs reads that as per-client keying, sets 100, and caps their entire ingress at 100 simultaneous connections on :9090 (and again on :9091) — a self-inflicted capacity cut, while the abusive client the cap targets is still indistinguishable from every other client behind that proxy.
Either skip the cap for peers matching trustedProxies (the cap can protect nothing behind them anyway), or state plainly here and in the CHANGELOG that this cap is keyed on the TCP peer only, is unaffected by trusted-proxy-cidrs, and must not be set to a per-client value on a node behind a proxy or load balancer.
| } | ||
| // Same ordering as :9090: the per-IP cap sits below the global one, so an | ||
| // address at its limit cannot consume the shared budget to be refused. | ||
| listener = ratelimiter.ConnLimitListener(listener, ratelimiter.PlaneGRPC, clampToMaxInt(config.GRPCWeb.MaxConnectionsPerIP)) |
There was a problem hiding this comment.
[suggestion] Both listeners pass ratelimiter.PlaneGRPC, so every rpc_connection_rejected_total sample carries plane="grpc" and an operator cannot tell a :9091 refusal from a :9090 one — even though the two caps are configured by separate keys and enforced by separate listeners with separate counters. Sharing the label made sense for rpc_rate_limit_rejected_total (both planes really do draw one bucket), but here it hides which limit fired.
A distinct constant is cheap for this metric specifically: rpc_connection_rejected_total carries no method label, so a new plane value never reaches bucketRPCMethod and cannot change method bucketing for the web plane's other counters.
| grpc.InTapHandle(RateLimitTapHandle(registry)), | ||
| // The tap handler takes a per-IP concurrency slot alongside the token; | ||
| // this is what gives it back, on every path a stream can end. | ||
| grpc.StatsHandler(InFlightStatsHandler(registry)), |
There was a problem hiding this comment.
[suggestion] The stats handler is installed whenever rate-limiting-enabled is true, including when max-in-flight-per-ip = 0. In that configuration registry.inflight is nil, AcquireInFlight always returns true and ReleaseInFlight is a no-op, so HandleRPC can never do any work — but grpc-go still allocates and dispatches Begin/InHeader/InPayload/OutPayload/OutTrailer/End for every RPC on the query plane, because it only builds those events when at least one stats handler is registered. Gating this option on cfg.MaxInFlightPerIP > 0 keeps the off switch free.
1 participant
Closes PLT-1125.
Impact
Caps how much load a single client IP can place on the gRPC query plane (
:9090and:9091):max-connections-per-ip(default 0, unlimited). Set a positive value to cap one address's share of the global connection budget.max-in-flight-per-ip), complementing the per-IP token bucket from PLT-1072: Wire rate limiter into native gRPC (:9090) unary+stream interceptors #4021.Both planes share the same per-IP pools. Rejections are counted separately from rate-limit rejections (
rpc_connection_rejected_total,rpc_inflight_rejected_total).Operators: clients behind a shared egress or reaching the node over
127.0.0.1share one allowance when a positive cap is set.