fix(proxy): bound connection memory and admission#25858
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Tick the box to add this pull request to the merge queue (same as
|
3e05081 to
da2e56c
Compare
ff4127a to
f259642
Compare
LeftHandCold
left a comment
There was a problem hiding this comment.
Requesting changes for one availability bypass and three concrete overload/protocol regressions. I reviewed only the stacked delta from da2e56c to f259642. The full proxy test suite, focused race tests, build, and vet pass; additional focused diagnostics reproduced the absolute-deadline bypass and the incorrect ERR packet sequence.
da2e56c to
6788d7c
Compare
f259642 to
2728840
Compare
2728840 to
2c57d30
Compare
LeftHandCold
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head. The four findings from my previous review are fixed. Requesting changes for one newly confirmed PROXY v2 protocol regression. The full proxy/frontend test suites, the full proxy race suite, build, vet, and diff checks pass; a focused diagnostic reproduces this failure deterministically.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The new admission and memory limits have a concrete OOM gap, and duplicate PROXY v2 detection is incomplete.
P1 - Include retained handshake storage in the memory budget (pkg/proxy/config.go:348)
Validation accounts only for the shared 16 KiB client/backend session buffers. The goetty input buffer and the copied handshakePack.Payload are outside that allocator but remain retained for authenticated connections. With the defaults, 10,000 connections may each retain a valid 64 KiB login twice, already exceeding 1 GiB before the fixed session buffers and tunnel state, so ProtocolMemoryLimit does not prevent OOM.
P2 - Mark consumed LOCAL/UNSPEC PROXY frames for duplicate detection (pkg/proxy/ppv2.go:194)
For valid unspec/unix PROXY v2 frames, the parser consumes the frame but returns ok=false. The wrapper then delegates to the SQL codec, and readPacketBefore never sets proxyHeaderReceived. A LOCAL/UNSPEC frame followed by an address-bearing frame in a later read therefore bypasses duplicate rejection and can overwrite origin metadata used for internal classification and routing.
LeftHandCold
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head and approving. The previously reported fragmented PROXY v2 regression is fixed: matching signature prefixes, partial fixed headers, and partial declared bodies now remain buffered without consumption, while definite mismatches still delegate to the bounded MySQL codec. I found no new blocker in the updated delta. The focused fragmentation test passed 100 iterations; the full proxy suite, full proxy race suite, build, vet, and diff checks also pass.
|
Deep-review blocker (GitHub does not allow the PR author to submit a formal REQUEST_CHANGES review on their own PR): [P1] Reject handshake packet limits that cannot admit a final login
I reproduced this at the wire level on the current head: a config with Please model the minimum for a complete login rather than the SSLRequest-only size (or otherwise reject unusable limits), and add a boundary test that the smallest configuration accepted by |
|
@gouhongshen Addressed the latest handshake-limit blocker and the related phase-model gaps in a569dfa; the branch is also merged with the latest main at 80668d9. Root-cause classification:
Unhappy-path ownership coverage now proves:
Performance: the production delta is limited to startup validation and the unauthenticated handshake. The recursion-to-loop change adds only a stack boolean and performs the same packet parse/allocation work for the normal one- or two-packet flow. It does not touch the established tunnel/query hot path. Fresh validation after merging latest main:
During the unhappy-path test addition, it also exposed and fixed the existing "TSL handshake error" typo so production diagnostics now consistently say TLS. |
|
Deep self-review found and fixed one additional admission trust-boundary issue in 584ff05. Root cause:
The lifecycle is now:
This preserves the global memory/work bound during authentication while making tenant fairness depend on an authenticated identity. On tenant saturation, a freshly authenticated backend or cache hit is closed before any OK packet reaches the client. Backend authentication failures consume no tenant quota. Expected admission rejection remains debug-level and uses the existing reject metric, avoiding error-log amplification. Added regression coverage for:
Validation on the pushed commit:
Performance remains outside the established tunnel/query hot path: one existing limiter mutex operation moves from pre-auth parsing to the successful authentication boundary; rejected unauthenticated claims no longer contend on per-tenant counters. |
LeftHandCold
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head carefully and approving.\n\nI re-audited the new handshake lifecycle and admission boundary from first principles:\n- retained auth payload ownership is closed: backend connects hold a read lease, timeout closes the transport and joins the worker before returning, and client Close frees the allocator-backed payload exactly once;\n- coalesced plaintext/TLS bytes are handed to the tunnel without loss or stale ByteBuf access;\n- SSLRequest is phase-local, duplicate TLS requests are bounded/rejected, and the final login remains subject to the absolute handshake deadline and packet limit;\n- per-tenant admission is bound only after fresh-backend or cache authentication and before the backend OK is exposed; rejection closes the accepted backend and does not consume unauthenticated tenant quota;\n- PROXY v2 fragmentation/LOCAL/UNSPEC handling, oversized ERR sequencing, shared-memory accounting, cache and migration paths have no new reachable regression found.\n\nFresh validation on this exact head:\n- focused lifecycle/admission/TLS/PROXY/timeout tests: 10x;\n- focused ownership tests with : 10x;\n- full FAIL github.com/matrixorigin/matrixone/pkg/proxy [build failed]
FAIL;\n- full FAIL github.com/matrixorigin/matrixone/pkg/proxy [build failed]
FAIL;\n- ;\n- ;\n- dependent frontend SQL codec test;\n- .\n\nNo new realistic blocker found.
LeftHandCold
left a comment
There was a problem hiding this comment.
Correction to my immediately preceding approval: its Markdown command names were expanded by the local shell while submitting the review body. The two displayed build-failed lines were submission-formatting artifacts, not validation results.
This approval is for head 584ff05. The actual fresh validation all passed:
- focused lifecycle, admission, TLS, PROXY, timeout, and packet-sequence tests, 10 iterations;
- focused ownership tests under the race detector, 10 iterations;
- full pkg/proxy test suite;
- full pkg/proxy suite under the race detector;
- pkg/proxy build and vet;
- dependent frontend SQL codec test;
- diff check.
The code-review conclusion is unchanged: retained handshake ownership, timeout worker termination, pipelined plaintext and TLS handoff, authenticated tenant binding, packet bounds, cache and migration interactions form a closed lifecycle. I found no new realistic blocker on this head.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Valid configurations can fail normal migration/control-plane operations because peak shared allocations exceed the validated budget. The fragmented PROXY-v2 path also bypasses the configured handshake memory limit.
P1 - Account for the transient third protocol session (pkg/proxy/config.go:357)
Validation reserves only maxConnections*2 fixed sessions, but migration allocates the replacement backend session before replaceServerConn closes the old one (tunnel.go:491-496, tunnel.go:371-374). The same third-session peak occurs for connAndExec during upgrade/kill. A configuration accepted by the exact-boundary test can therefore reach the allocator limit and return out of space during legitimate operations. Reserve transient headroom or release/coordinate the old session before allocating its replacement.
P2 - Bound fragmented PROXY-v2 bodies by the handshake budget (pkg/proxy/ppv2.go:158)
The parser now waits for the complete declared body before delegating to the SQL codec. Length is a uint16, so an unauthenticated client can send an UNSPEC frame with a 65535-byte body and make goetty retain roughly 64 KiB outside the shared allocator; line 175 also creates a second temporary copy once complete. ClientHandshakePacketLimit can legally be as low as 35, but it is never applied to this frame, so the new fragmented-header path bypasses the configured handshake memory bound until timeout.
aptend
left a comment
There was a problem hiding this comment.
Re-review of head 584ff05: the protocol-memory bound is still not closed, and the fragmented PROXY-v2 path still bypasses it.
[P1] Account for all peak shared protocol allocations
Config.Validate accepts an exact budget of two 16 KiB IOSessions plus one retained login payload per admitted connection. That is below several reachable peaks. A valid 32 KiB login fills the accepted client + retained payload + backend budget, then forwarding that login to the backend needs an additional dynamic write block and can fail with out of space. Established connections also need a third IOSession while migration creates the replacement backend before closing the old one; connAndExec has the same peak for upgrade/kill operations. Pipelined handshake read-ahead adds another allocator-backed prefix that is also omitted. Consequently, configurations accepted by validation can reject normal login, migration, and control-plane work. Please reserve the complete peak or add explicit admission/reservation that guarantees these operations can allocate.
[P2] Bound PROXY-v2 bodies within the handshake memory budget
parseProxyHeaderV2 waits for the complete uint16-declared body before the SQL codec runs, so an unauthenticated client can retain up to 65,535 bytes in goetty input memory that is outside the shared SessionAllocator and unaffected by ClientHandshakePacketLimit. Once complete, the parser also allocates and copies another full body. This violates ProtocolMemoryLimit even when the configured handshake limit is only 35 bytes. Please reject bodies above an explicit accounted limit, or bring the goetty input allocation and parser copy under the same shared bound.
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #25856.
Depends on #25857. This is a stacked PR and should be retargeted to
mainafter #25857 merges.What this PR does / why we need it:
The Proxy previously paid two eager 1 MiB
frontend.IOSessionbuffers per live proxied connection, created one managed allocator per connection, retained an unbounded login packet for migration, and had no global or per-tenant admission boundary. Raising a memory limit alone would only delay exhaustion.This PR closes the resource lifecycle at multiple layers:
The SQL/tunnel data path is unchanged: the limiter lock is touched only during acquire, tenant bind, and release, and no query/transaction idle deadline is introduced. This avoids regressions for long HTAP queries.
Test matrix
./pkg/frontendand./pkg/proxyunit suites;go build ./pkg/frontend ./pkg/proxy;go vet ./pkg/frontend ./pkg/proxy;./pkg/proxyrace suite;Covered unhappy paths include allocator exhaustion and recovery, exact-fill and oversized packets, global and tenant saturation, concurrent bind/release, double release, constructor/TLS failure cleanup, silent-client timeout, deadline clearing after successful login, standard client-visible errors, and bounded rejection writes.