server: restore session on change user auth failure (release-7.5-20260724-v7.5.7) (#69692) | tidb-test=release-7.5.7 tikv=v7.5.7 pd=v7.5.7 tiflash=v7.5.7 - #70171
Merged
Conversation
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughChangesChange-user session restoration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant clientConn
participant handleChangeUser
participant SessionAuth
participant TiDBContext
clientConn->>handleChangeUser: request user/database change
handleChangeUser->>SessionAuth: open and authenticate replacement session
SessionAuth-->>handleChangeUser: success or error
handleChangeUser->>TiDBContext: restore previous context on error
handleChangeUser-->>clientConn: return result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/conn_test.go`:
- Around line 1533-1566: Extend the handleChangeUser rollback test around
clientConn initialization and assertions to set a non-empty prior authPlugin
value before invoking cc.handleChangeUser. After the failed change-user attempt,
assert that cc.authPlugin retains the original value alongside the existing
user, database, and session checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22a0c7de-c093-43d5-92bc-c60a957c3ae1
📒 Files selected for processing (2)
pkg/server/conn.gopkg/server/conn_test.go
Comment on lines
+1533
to
+1566
| cc := &clientConn{ | ||
| connectionID: 1, | ||
| alloc: arena.NewAllocator(1024), | ||
| chunkAlloc: chunk.NewAllocator(), | ||
| peerHost: "localhost", | ||
| collation: mysql.DefaultCollationID, | ||
| capability: mysql.ClientProtocol41, | ||
| pkt: internal.NewPacketIOForTest(bufio.NewWriter(bytes.NewBuffer(nil))), | ||
| server: srv, | ||
| user: "root", | ||
| dbname: "old_db", | ||
| } | ||
| se, err := session.CreateSession4Test(store) | ||
| require.NoError(t, err) | ||
| require.NoError(t, se.Auth(&auth.UserIdentity{Username: "root", Hostname: "localhost"}, nil, nil, nil)) | ||
| tc := &TiDBContext{ | ||
| Session: se, | ||
| stmts: make(map[int]*TiDBStatement), | ||
| } | ||
| cc.SetCtx(tc) | ||
|
|
||
| data := []byte{} | ||
| data = append(data, "missing_user"...) | ||
| data = append(data, 0) | ||
| data = append(data, 0) | ||
| data = append(data, "new_db"...) | ||
| data = append(data, 0) | ||
| data = append(data, 0, 0) | ||
| err = cc.handleChangeUser(context.Background(), data) | ||
| require.Error(t, err) | ||
| require.Same(t, tc, cc.getCtx()) | ||
| require.Equal(t, "root", cc.user) | ||
| require.Equal(t, "old_db", cc.dbname) | ||
| require.Equal(t, "root", cc.ctx.GetSessionVars().User.Username) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert authPlugin rollback too.
This test never sets or verifies a non-empty old plugin, so regression of cc.authPlugin = oldAuthPlugin would still pass.
Proposed test addition
server: srv,
user: "root",
dbname: "old_db",
+ authPlugin: mysql.AuthNativePassword,
}
...
require.Equal(t, "old_db", cc.dbname)
+ require.Equal(t, mysql.AuthNativePassword, cc.authPlugin)
require.Equal(t, "root", cc.ctx.GetSessionVars().User.Username)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cc := &clientConn{ | |
| connectionID: 1, | |
| alloc: arena.NewAllocator(1024), | |
| chunkAlloc: chunk.NewAllocator(), | |
| peerHost: "localhost", | |
| collation: mysql.DefaultCollationID, | |
| capability: mysql.ClientProtocol41, | |
| pkt: internal.NewPacketIOForTest(bufio.NewWriter(bytes.NewBuffer(nil))), | |
| server: srv, | |
| user: "root", | |
| dbname: "old_db", | |
| } | |
| se, err := session.CreateSession4Test(store) | |
| require.NoError(t, err) | |
| require.NoError(t, se.Auth(&auth.UserIdentity{Username: "root", Hostname: "localhost"}, nil, nil, nil)) | |
| tc := &TiDBContext{ | |
| Session: se, | |
| stmts: make(map[int]*TiDBStatement), | |
| } | |
| cc.SetCtx(tc) | |
| data := []byte{} | |
| data = append(data, "missing_user"...) | |
| data = append(data, 0) | |
| data = append(data, 0) | |
| data = append(data, "new_db"...) | |
| data = append(data, 0) | |
| data = append(data, 0, 0) | |
| err = cc.handleChangeUser(context.Background(), data) | |
| require.Error(t, err) | |
| require.Same(t, tc, cc.getCtx()) | |
| require.Equal(t, "root", cc.user) | |
| require.Equal(t, "old_db", cc.dbname) | |
| require.Equal(t, "root", cc.ctx.GetSessionVars().User.Username) | |
| cc := &clientConn{ | |
| connectionID: 1, | |
| alloc: arena.NewAllocator(1024), | |
| chunkAlloc: chunk.NewAllocator(), | |
| peerHost: "localhost", | |
| collation: mysql.DefaultCollationID, | |
| capability: mysql.ClientProtocol41, | |
| pkt: internal.NewPacketIOForTest(bufio.NewWriter(bytes.NewBuffer(nil))), | |
| server: srv, | |
| user: "root", | |
| dbname: "old_db", | |
| authPlugin: mysql.AuthNativePassword, | |
| } | |
| se, err := session.CreateSession4Test(store) | |
| require.NoError(t, err) | |
| require.NoError(t, se.Auth(&auth.UserIdentity{Username: "root", Hostname: "localhost"}, nil, nil, nil)) | |
| tc := &TiDBContext{ | |
| Session: se, | |
| stmts: make(map[int]*TiDBStatement), | |
| } | |
| cc.SetCtx(tc) | |
| data := []byte{} | |
| data = append(data, "missing_user"...) | |
| data = append(data, 0) | |
| data = append(data, 0) | |
| data = append(data, "new_db"...) | |
| data = append(data, 0) | |
| data = append(data, 0, 0) | |
| err = cc.handleChangeUser(context.Background(), data) | |
| require.Error(t, err) | |
| require.Same(t, tc, cc.getCtx()) | |
| require.Equal(t, "root", cc.user) | |
| require.Equal(t, "old_db", cc.dbname) | |
| require.Equal(t, mysql.AuthNativePassword, cc.authPlugin) | |
| require.Equal(t, "root", cc.ctx.GetSessionVars().User.Username) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/conn_test.go` around lines 1533 - 1566, Extend the
handleChangeUser rollback test around clientConn initialization and assertions
to set a non-empty prior authPlugin value before invoking cc.handleChangeUser.
After the failed change-user attempt, assert that cc.authPlugin retains the
original value alongside the existing user, database, and session checks.
1 task
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.5-20260724-v7.5.7 #70171 +/- ##
================================================================
Coverage ? 73.0238%
================================================================
Files ? 1445
Lines ? 421806
Branches ? 0
================================================================
Hits ? 308019
Misses ? 94509
Partials ? 19278
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
Author
|
/retest |
1 similar comment
|
/retest |
YangKeao
approved these changes
Aug 3, 2026
ti-chi-bot
Bot
added
needs-1-more-lgtm
wjhuang2016
approved these changes
Aug 3, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wjhuang2016, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
jebter
changed the title
Contributor
|
/retest |
Contributor
|
/test check-dev2 |
1 similar comment
|
/test check-dev2 |
|
@jebter: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
Member
Author
|
/retest |
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
5 participants
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.
What problem does this PR solve?
Issue Number: close #69691
Problem Summary:
Cherry-pick #69692 (restore session on change user auth failure) to
release-7.5-20260724-v7.5.7.What changed and how does it work?
Cherry-pick of f1be26c. Conflict adaptations for 7.5:
openSessionAndDoAuth(auth, authPlugin)signature (noZstdLevelparameter on this branch).go vet ./pkg/server/passes. See server: restore session on change user auth failure #69692 for the full description.Check List
Tests
Release note
Summary by CodeRabbit