Skip to content

Commit e6c70b6

Browse files
fix(proxy): reject cached sessions on draining CNs (#25913)
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: issue #25895 ## What this PR does / why we need it: Normal proxy routing obtains candidates from `MOCluster.GetCNService`, which excludes draining and removed CNs. Cached backend reuse bypassed that candidate selection and `router.CanReuseCachedCN` checked only the CN health circuit breaker. A draining CN can remain breaker-healthy, so a fresh client login could continue to reuse a cached backend session on a CN that normal routing had already made ineligible. This could delay or defeat graceful draining and scale-in. This change requires cached reuse to pass both existing gates: - the health breaker must allow the CN; and - the CN UUID must still be present in the current `GetCNService` routable cluster view. The existing nil-entry behavior is preserved. Missing and draining CNs are rejected, while a healthy routable CN remains reusable. No cache capacity, authentication, FIFO, timeout, routing-label, or circuit-breaker state semantics change. Regression tests cover nil, healthy, breaker-unhealthy, recovered, draining, and removed states. ## Test `.agents/skills/mo-dev/scripts/mo-cgo-test -count=100 -timeout=5m -run '^TestCanReuseCachedCNRequiresCurrentRouteEligibility$' ./pkg/proxy` `.agents/skills/mo-dev/scripts/mo-cgo-test -race -count=20 -timeout=5m -run '^TestCanReuseCachedCNRequiresCurrentRouteEligibility$' ./pkg/proxy` `.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=10m ./pkg/proxy` `.agents/skills/mo-dev/scripts/mo-cgo-test -race -count=1 -timeout=15m ./pkg/proxy` `go build -mod=readonly ./pkg/proxy` `go vet -mod=readonly ./pkg/proxy` `git diff --check` --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent 418551c commit e6c70b6

10 files changed

Lines changed: 254 additions & 37 deletions

pkg/proxy/client_conn.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -999,6 +999,7 @@ func (c *clientConn) connectToBackendContext(
999999
c.connID,
10001000
c.mysqlProto.GetSalt(),
10011001
c.mysqlProto.GetAuthResponse(),
1002+
c.clientInfo,
10021003
)
10031004
cancel()
10041005
} else {
@@ -1007,6 +1008,7 @@ func (c *clientConn) connectToBackendContext(
10071008
c.connID,
10081009
c.mysqlProto.GetSalt(),
10091010
c.mysqlProto.GetAuthResponse(),
1011+
c.clientInfo,
10101012
)
10111013
}
10121014
if sc != nil {

pkg/proxy/client_conn_test.go

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,9 @@ func (m *mockConnCache) Push(key cacheKey, sc ServerConn) bool {
231231
return true
232232
}
233233

234-
func (m *mockConnCache) Pop(key cacheKey, connID uint32, salt, authResp []byte) ServerConn {
234+
func (m *mockConnCache) Pop(
235+
key cacheKey, connID uint32, salt, authResp []byte, _ clientInfo,
236+
) ServerConn {
235237
if m.popFn != nil {
236238
return m.popFn(key, connID, salt, authResp)
237239
}
@@ -2414,6 +2416,63 @@ func Test_connectToBackend_SkipCacheOnMigration(t *testing.T) {
24142416
require.Equal(t, 0, cache.popCount)
24152417
}
24162418

2419+
func Test_connectToBackend_PassesClientInfoToCache(t *testing.T) {
2420+
cache := &popCountConnCache{}
2421+
client := clientInfo{labelInfo: labelInfo{
2422+
Tenant: "tenant1",
2423+
Labels: map[string]string{"region": "cn"},
2424+
}}
2425+
cConn := &clientConn{
2426+
ctx: context.Background(),
2427+
router: &routeErrRouter{},
2428+
mysqlProto: &frontend.MysqlProtocolImpl{},
2429+
connCache: cache,
2430+
clientInfo: client,
2431+
log: runtime.DefaultRuntime().Logger(),
2432+
}
2433+
2434+
sConn, err := cConn.connectToBackend("")
2435+
require.Error(t, err)
2436+
require.Nil(t, sConn)
2437+
require.Equal(t, 1, cache.popCount)
2438+
require.Equal(t, client, cache.lastClient)
2439+
}
2440+
2441+
type contextPopCountConnCache struct {
2442+
popCountConnCache
2443+
popContextCount int
2444+
}
2445+
2446+
func (c *contextPopCountConnCache) PopContext(
2447+
_ context.Context, _ cacheKey, _ uint32, _ []byte, _ []byte, client clientInfo,
2448+
) ServerConn {
2449+
c.popContextCount++
2450+
c.lastClient = client
2451+
return nil
2452+
}
2453+
2454+
func Test_connectToBackend_PassesClientInfoToContextCache(t *testing.T) {
2455+
cache := &contextPopCountConnCache{}
2456+
client := clientInfo{labelInfo: labelInfo{
2457+
Tenant: "tenant1",
2458+
Labels: map[string]string{"region": "cn"},
2459+
}}
2460+
cConn := &clientConn{
2461+
ctx: context.Background(),
2462+
router: &routeErrRouter{},
2463+
mysqlProto: &frontend.MysqlProtocolImpl{},
2464+
connCache: cache,
2465+
clientInfo: client,
2466+
log: runtime.DefaultRuntime().Logger(),
2467+
}
2468+
2469+
sConn, err := cConn.connectToBackend("")
2470+
require.Error(t, err)
2471+
require.Nil(t, sConn)
2472+
require.Equal(t, 1, cache.popContextCount)
2473+
require.Equal(t, client, cache.lastClient)
2474+
}
2475+
24172476
func Test_connectToBackend_SkipCacheWhenPluginRouterEnabled(t *testing.T) {
24182477
cache := &popCountConnCache{}
24192478
cc, cleanup := createNewClientConn(t)

pkg/proxy/conn_cache.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ type ConnCache interface {
188188
// Returns if the connection is pushed into the cache.
189189
Push(cacheKey, ServerConn) bool
190190
// Pop pops a server connection from the cache.
191-
Pop(cacheKey, uint32, []byte, []byte) ServerConn
191+
Pop(cacheKey, uint32, []byte, []byte, clientInfo) ServerConn
192192
// Count returns the total number of cached connections. It is
193193
// mainly for testing.
194194
Count() int
@@ -197,7 +197,7 @@ type ConnCache interface {
197197
}
198198

199199
type contextConnCache interface {
200-
PopContext(context.Context, cacheKey, uint32, []byte, []byte) ServerConn
200+
PopContext(context.Context, cacheKey, uint32, []byte, []byte, clientInfo) ServerConn
201201
}
202202

203203
// the main cache struct.
@@ -234,7 +234,7 @@ type connCache struct {
234234
// canReuseCN decides whether a cached connection to the CN may be reused
235235
// for a fresh client login. If it returns false, the cached connection is
236236
// discarded before any SET CONNECTION ID or auth work is attempted.
237-
canReuseCN func(*CNServer) bool
237+
canReuseCN func(*CNServer, clientInfo) bool
238238
}
239239

240240
// connCacheOption is the option for connCache.
@@ -282,7 +282,7 @@ func withQueryClient(qc client.QueryClient) connCacheOption {
282282
}
283283
}
284284

285-
func withCanReuseCN(f func(*CNServer) bool) connCacheOption {
285+
func withCanReuseCN(f func(*CNServer, clientInfo) bool) connCacheOption {
286286
return func(c *connCache) {
287287
c.canReuseCN = f
288288
}
@@ -418,10 +418,12 @@ func (c *connCache) closeCachedConnection(sc *serverConnAuth) {
418418
}
419419

420420
// Pop implements the ConnCache interface.
421-
func (c *connCache) Pop(key cacheKey, connID uint32, salt []byte, authResp []byte) ServerConn {
421+
func (c *connCache) Pop(
422+
key cacheKey, connID uint32, salt []byte, authResp []byte, client clientInfo,
423+
) ServerConn {
422424
ctx, cancel := context.WithTimeout(context.Background(), defaultTransferTimeout)
423425
defer cancel()
424-
return c.PopContext(ctx, key, connID, salt, authResp)
426+
return c.PopContext(ctx, key, connID, salt, authResp, client)
425427
}
426428

427429
func (c *connCache) PopContext(
@@ -430,6 +432,7 @@ func (c *connCache) PopContext(
430432
connID uint32,
431433
salt []byte,
432434
authResp []byte,
435+
client clientInfo,
433436
) ServerConn {
434437
if ctx == nil {
435438
ctx = context.Background()
@@ -463,16 +466,15 @@ func (c *connCache) PopContext(
463466
}
464467

465468
// Before using a cached connection for a fresh login, ensure its CN is
466-
// still eligible under the current health policy. This keeps connCache
467-
// reuse aligned with the same breaker decision that Route() applies to
468-
// newly-built sessions, and avoids executing SET CONNECTION ID against a
469-
// CN that is currently cooling down.
470-
if c.canReuseCN != nil && !c.canReuseCN(sc.GetCNServer()) {
469+
// still eligible under the current login's route and health policy. This
470+
// avoids executing SET CONNECTION ID against a CN that a fresh Route()
471+
// would reject.
472+
if c.canReuseCN != nil && !c.canReuseCN(sc.GetCNServer(), client) {
471473
cnUUID := ""
472474
if cn := sc.GetCNServer(); cn != nil {
473475
cnUUID = cn.uuid
474476
}
475-
c.logger.Warn("skip cached connection on unhealthy cn",
477+
c.logger.Warn("skip cached connection on ineligible cn",
476478
zap.Uint32("conn ID", sc.ConnID()),
477479
zap.String("cn", cnUUID),
478480
)

pkg/proxy/conn_cache_test.go

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -301,17 +301,19 @@ func TestConnCache(t *testing.T) {
301301
assert.True(t, cc.Push("k100", mockConn1))
302302
assert.Equal(t, 1, cc.Count())
303303

304-
sc := cc.Pop("k100", 1, nil, nil)
304+
sc := cc.Pop("k100", 1, nil, nil, clientInfo{})
305305
assert.NotNil(t, sc)
306306
assert.Equal(t, 0, cc.Count())
307307
})
308308
})
309309

310-
t.Run("pop - skip unhealthy cn before reuse", func(t *testing.T) {
310+
t.Run("pop - skip route-ineligible cn before reuse", func(t *testing.T) {
311311
runTestWithNewConnCacheWithAuthConstructor(t, nil, func(cc ConnCache) {
312312
ccc := cc.(*connCache)
313-
ccc.canReuseCN = func(cn *CNServer) bool {
314-
return cn == nil || cn.uuid != "bad-cn"
313+
var checkedClient clientInfo
314+
ccc.canReuseCN = func(cn *CNServer, client clientInfo) bool {
315+
checkedClient = client
316+
return cn == nil || cn.uuid != "bad-cn" || client.username == "root"
315317
}
316318

317319
c1, _ := net.Pipe()
@@ -320,9 +322,11 @@ func TestConnCache(t *testing.T) {
320322
assert.True(t, cc.Push("k100", mockConn1))
321323
assert.Equal(t, 1, cc.Count())
322324

323-
sc := cc.Pop("k100", 1, nil, nil)
325+
login := clientInfo{username: "ordinary"}
326+
sc := cc.Pop("k100", 1, nil, nil, login)
324327
assert.Nil(t, sc)
325328
assert.Equal(t, 0, cc.Count())
329+
assert.Equal(t, login.username, checkedClient.username)
326330
})
327331
})
328332

@@ -335,7 +339,7 @@ func TestConnCache(t *testing.T) {
335339
assert.True(t, cc.Push("k100", mockConn1))
336340
assert.Equal(t, 1, cc.Count())
337341

338-
sc := cc.Pop("k100", 1, nil, nil)
342+
sc := cc.Pop("k100", 1, nil, nil, clientInfo{})
339343
assert.Nil(t, sc)
340344
assert.Equal(t, 0, cc.Count())
341345
})
@@ -348,7 +352,7 @@ func TestConnCache(t *testing.T) {
348352
assert.True(t, cc.Push("k100", mockConn1))
349353
assert.Equal(t, 1, cc.Count())
350354

351-
sc := cc.Pop("k100", 1, nil, nil)
355+
sc := cc.Pop("k100", 1, nil, nil, clientInfo{})
352356
assert.Nil(t, sc)
353357
assert.Equal(t, 1, cc.Count())
354358
})
@@ -361,7 +365,7 @@ func TestConnCache(t *testing.T) {
361365
assert.True(t, cc.Push("k100", mockConn1))
362366
assert.Equal(t, 1, cc.Count())
363367

364-
sc := cc.Pop("k100", 1, nil, nil)
368+
sc := cc.Pop("k100", 1, nil, nil, clientInfo{})
365369
// cannot get conn as timeout.
366370
assert.Nil(t, sc)
367371
// count is 0 because the connection has been removed.
@@ -379,7 +383,7 @@ func TestConnCache(t *testing.T) {
379383
assert.NoError(t, cc.Close())
380384
assert.Equal(t, 0, cc.Count())
381385

382-
sc := cc.Pop("k100", 1, nil, nil)
386+
sc := cc.Pop("k100", 1, nil, nil, clientInfo{})
383387
assert.Nil(t, sc)
384388

385389
c2, _ := net.Pipe()
@@ -404,7 +408,7 @@ func TestConnCacheBlockedPopDoesNotBlockOtherTenantsOrClose(t *testing.T) {
404408

405409
popDone := make(chan ServerConn, 1)
406410
go func() {
407-
popDone <- cache.Pop("tenant-a", 1, nil, nil)
411+
popDone <- cache.Pop("tenant-a", 1, nil, nil, clientInfo{})
408412
}()
409413
select {
410414
case <-blocked.entered:
@@ -457,7 +461,7 @@ func TestConnCachePopContextCancelsBackendValidation(t *testing.T) {
457461
ctx, cancel := context.WithCancel(context.Background())
458462
popDone := make(chan ServerConn, 1)
459463
go func() {
460-
popDone <- cache.(*connCache).PopContext(ctx, "tenant-a", 1, nil, nil)
464+
popDone <- cache.(*connCache).PopContext(ctx, "tenant-a", 1, nil, nil, clientInfo{})
461465
}()
462466
select {
463467
case <-blocked.entered:
@@ -540,7 +544,7 @@ func TestConnCachePopWaitsForOriginGenerationCleanup(t *testing.T) {
540544
result := make(chan ServerConn, 1)
541545
go func() {
542546
result <- cache.(*connCache).PopContext(
543-
context.Background(), "tenant-a", 1, nil, nil,
547+
context.Background(), "tenant-a", 1, nil, nil, clientInfo{},
544548
)
545549
}()
546550
select {
@@ -570,7 +574,7 @@ func TestConnCachePopWaitsForOriginGenerationCleanup(t *testing.T) {
570574
canceledResult := make(chan ServerConn, 1)
571575
go func() {
572576
canceledResult <- cache.(*connCache).PopContext(
573-
cancelCtx, "tenant-b", 2, nil, nil,
577+
cancelCtx, "tenant-b", 2, nil, nil, clientInfo{},
574578
)
575579
}()
576580
select {

pkg/proxy/plugin.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,9 @@ func (r *pluginRouter) RouteForTransfer(
123123
}
124124
}
125125

126-
func (r *pluginRouter) CanReuseCachedCN(cn *CNServer) bool {
126+
func (r *pluginRouter) CanReuseCachedCN(cn *CNServer, client clientInfo) bool {
127127
if rr, ok := r.Router.(cacheReuseChecker); ok {
128-
return rr.CanReuseCachedCN(cn)
128+
return rr.CanReuseCachedCN(cn, client)
129129
}
130130
return true
131131
}

pkg/proxy/plugin_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,17 @@ import (
3333

3434
var _ Router = (*pluginRouter)(nil)
3535

36+
type cacheReuseTestRouter struct {
37+
routeErrRouter
38+
client clientInfo
39+
result bool
40+
}
41+
42+
func (r *cacheReuseTestRouter) CanReuseCachedCN(_ *CNServer, client clientInfo) bool {
43+
r.client = client
44+
return r.result
45+
}
46+
3647
type mockPlugin struct {
3748
mockRecommendCNFn func(ctx context.Context, clientInfo clientInfo) (*plugin.Recommendation, error)
3849
}
@@ -133,6 +144,15 @@ func TestPluginRouter_PropagatesConnectionContext(t *testing.T) {
133144
require.Equal(t, 1, legacy.routeSelectedCount)
134145
}
135146

147+
func TestPluginRouterCanReuseCachedCNDelegatesClientInfo(t *testing.T) {
148+
delegate := &cacheReuseTestRouter{result: false}
149+
router := newPluginRouter("", delegate, nil)
150+
client := clientInfo{labelInfo: labelInfo{Tenant: "tenant1"}}
151+
152+
require.False(t, router.CanReuseCachedCN(&CNServer{uuid: "cn1"}, client))
153+
require.Equal(t, client, delegate.client)
154+
}
155+
136156
func TestPluginRouter_SelectHonorsBreaker(t *testing.T) {
137157
defer leaktest.AfterTest(t)()
138158

pkg/proxy/router.go

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ type transferRouter interface {
134134
// backend connection is still eligible for a fresh client login. Cached reuse
135135
// must honor the same CN health policy as a new session route.
136136
type cacheReuseChecker interface {
137-
CanReuseCachedCN(cn *CNServer) bool
137+
CanReuseCachedCN(cn *CNServer, client clientInfo) bool
138138
}
139139

140140
// RefreshableRouter is a router that can be refreshed to get latest route strategy
@@ -403,14 +403,51 @@ func (r *router) RouteForTransfer(
403403
return s, nil
404404
}
405405

406-
// CanReuseCachedCN implements cacheReuseChecker. Cached connections must only
407-
// be reused when the breaker is closed/absent; otherwise they would bypass the
408-
// new-session health gate.
409-
func (r *router) CanReuseCachedCN(cn *CNServer) bool {
406+
// CanReuseCachedCN implements cacheReuseChecker. Cached connections must pass
407+
// the same cluster-eligibility and health gates as a newly routed session.
408+
func (r *router) CanReuseCachedCN(cn *CNServer, client clientInfo) bool {
410409
if cn == nil {
411410
return true
412411
}
413-
return r.health.canReuseCachedCN(cn.uuid)
412+
if !r.health.canReuseCachedCN(cn.uuid) || r.moCluster == nil {
413+
return false
414+
}
415+
416+
candidates := make([]metadata.CNService, 0)
417+
r.moCluster.GetCNService(
418+
clusterservice.NewSelector(),
419+
func(service metadata.CNService) bool {
420+
candidates = append(candidates, service)
421+
return true
422+
},
423+
)
424+
if len(candidates) == 0 {
425+
return false
426+
}
427+
428+
// Apply the exact fresh-session routing policy to the current metadata for
429+
// all working CNs. The full snapshot is required because labeled and empty
430+
// CNs form priority tiers: whether a cached fallback remains eligible can
431+
// change when another CN is added or removed.
432+
eligible := false
433+
selector := client.labelInfo.genSelector(clusterservice.EQ_Globbing)
434+
appendFn := func(service *metadata.CNService) {
435+
if service.ServiceID == cn.uuid {
436+
eligible = true
437+
}
438+
}
439+
if client.isSuperTenant() {
440+
if err := route.RouteForSuperTenantCandidates(
441+
context.Background(), candidates, selector, client.username, nil, appendFn,
442+
); err != nil {
443+
return false
444+
}
445+
} else if err := route.RouteForCommonTenantCandidates(
446+
context.Background(), candidates, selector, nil, appendFn,
447+
); err != nil {
448+
return false
449+
}
450+
return eligible
414451
}
415452

416453
func (r *router) connect(

0 commit comments

Comments
 (0)