privilege: add ColumnsPrivMap for accelerating column-privilege (#61677)#67641
Conversation
ti-chi-bot
added
do-not-merge/hold
|
@fzzf678 This PR has conflicts, I have hold it. |
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
|
@ti-chi-bot I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions 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 ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughAdds a per-user Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Authenticator
participant PrivCache as MySQLPrivilegeCache
participant ColumnsTable as mysql.columns_priv
Client->>Authenticator: request verification (user, host, db, table, column, priv)
Authenticator->>PrivCache: locate MySQLPrivilege for user@host
PrivCache->>PrivCache: ensure ColumnsPrivMap built (buildColumnsPrivMap)
PrivCache->>ColumnsTable: (if needed) read columns_priv rows
PrivCache->>PrivCache: MatchColumns(user, db, table, column)
PrivCache-->>Authenticator: tablePriv & columnPriv result
Authenticator-->>Client: allow/deny
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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 |
fzzf678
removed
needs-cherry-pick-release-8.5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/privilege/privileges/cache_test.go`:
- Around line 182-195: The test is constructing privileges with
&privileges.MySQLPrivilege{} which leaves internal maps/trees nil; replace those
with the package constructor by creating instances via
privileges.NewMySQLPrivilege() wherever MySQLPrivilege is instantiated (both
before the first LoadColumnsPrivTable/MatchColumns block and before the second
block after truncation) so LoadColumnsPrivTable and MatchColumns operate on a
properly initialized cache.
In `@pkg/privilege/privileges/cache.go`:
- Around line 928-935: LoadColumnsPrivTable now calls p.loadTable(ctx, ...) but
the refactor only introduced a package-level loadTable(exec sqlexec.SQLExecutor,
...) and callers like LoadAll/loadSomeUsers still use sqlexec.SQLExecutor, so
the change is half-applied; restore a consistent API by either (A) add a
MySQLPrivilege receiver overload func (p *MySQLPrivilege) loadTable(ctx
sqlexec.RestrictedSQLExecutor, query string, decode func(...)) that delegates to
the package helper, then update LoadAll and loadSomeUsers to call p.loadTable
where they currently use loadTable, or (B) revert the method call in
LoadColumnsPrivTable to use the existing package-level loadTable(exec
sqlexec.SQLExecutor, ...) signature and keep callers unchanged; update all
affected symbols: MySQLPrivilege.LoadColumnsPrivTable, loadTable
(package-level), LoadAll, loadSomeUsers so the signatures and caller types
match.
🪄 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
Run ID: b6ee5346-b707-4d59-a49a-2cbc108cd5de
📒 Files selected for processing (2)
pkg/privilege/privileges/cache.gopkg/privilege/privileges/cache_test.go
fzzf678
force-pushed
the
cherry-pick-61677-to-master
branch
from
2635871 to
5141aa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/privilege/privileges/cache.go (2)
368-368: Consider adding a doc comment for the exported field.
ColumnsPrivMapis exported but lacks documentation. A brief comment explaining its purpose (accelerating column privilege lookups) and relationship tocolumnsPrivwould help maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache.go` at line 368, Add a doc comment above the exported field ColumnsPrivMap describing its purpose and usage: explain that ColumnsPrivMap maps table names (string) to slices of columnsPrivRecord to accelerate column privilege lookups and how it relates to the underlying columnsPriv structure (e.g., used as a cache/accelerated index for columnsPriv lookups). Mention any concurrency or lifecycle expectations if applicable (e.g., maintained alongside updates to columnsPriv).
1415-1422: Consider explicit parentheses for readability.The operator precedence is correct (
&&binds tighter than||), but explicit parentheses would make the intent clearer:💡 Suggested clarification
return record.baseRecord.match(user, host) && strings.EqualFold(record.DB, db) && strings.EqualFold(record.TableName, table) && - (strings.EqualFold(record.ColumnName, col) || col == "*" && (record.ColumnPriv&mysql.SelectPriv > 0)) + (strings.EqualFold(record.ColumnName, col) || (col == "*" && (record.ColumnPriv&mysql.SelectPriv > 0)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache.go` around lines 1415 - 1422, The expression in columnsPrivRecord.match mixes && and || which is correct but unclear; update the return expression in match (method columnsPrivRecord.match) to add explicit parentheses around the final OR clause so the intent is obvious — e.g. wrap the column check as (strings.EqualFold(record.ColumnName, col) || (col == "*" && (record.ColumnPriv&mysql.SelectPriv > 0))) while keeping the existing baseRecord.match and DB/Table comparisons unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/privilege/privileges/cache.go`:
- Around line 809-816: Update the outdated comment that reads "the order is
nondeterministic" to accurately reflect the deterministic lexicographic
comparison implemented here (using the x > y / x < y branches returning 1/-1/0);
locate the comparison routine that compares x and y (the function containing the
shown return 1 / return -1 / return 0 logic) and reword or remove the misleading
sentence so it states that the comparison is deterministic and follows
lexicographic ordering between x and y.
---
Nitpick comments:
In `@pkg/privilege/privileges/cache.go`:
- Line 368: Add a doc comment above the exported field ColumnsPrivMap describing
its purpose and usage: explain that ColumnsPrivMap maps table names (string) to
slices of columnsPrivRecord to accelerate column privilege lookups and how it
relates to the underlying columnsPriv structure (e.g., used as a
cache/accelerated index for columnsPriv lookups). Mention any concurrency or
lifecycle expectations if applicable (e.g., maintained alongside updates to
columnsPriv).
- Around line 1415-1422: The expression in columnsPrivRecord.match mixes && and
|| which is correct but unclear; update the return expression in match (method
columnsPrivRecord.match) to add explicit parentheses around the final OR clause
so the intent is obvious — e.g. wrap the column check as
(strings.EqualFold(record.ColumnName, col) || (col == "*" &&
(record.ColumnPriv&mysql.SelectPriv > 0))) while keeping the existing
baseRecord.match and DB/Table comparisons unchanged.
🪄 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
Run ID: 2f5aafee-5cfc-4a6f-9283-bacfc8e35e8e
📒 Files selected for processing (2)
pkg/privilege/privileges/cache.gopkg/privilege/privileges/cache_test.go
| // For other case, the order is nondeterministic. | ||
| switch x < y { | ||
| case true: | ||
| return -1 | ||
| case false: | ||
| if x > y { | ||
| return 1 | ||
| } else if x < y { | ||
| return -1 | ||
| } | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Outdated comment: the comparison is now deterministic.
The comment states "the order is nondeterministic" but the implementation performs a deterministic lexicographic comparison. This discrepancy could confuse future maintainers.
📝 Proposed fix
- // For other case, the order is nondeterministic.
+ // For other cases, use lexicographic ordering.
if x > y {
return 1
} else if x < y {📝 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.
| // For other case, the order is nondeterministic. | |
| switch x < y { | |
| case true: | |
| return -1 | |
| case false: | |
| if x > y { | |
| return 1 | |
| } else if x < y { | |
| return -1 | |
| } | |
| return 0 | |
| } | |
| // For other cases, use lexicographic ordering. | |
| if x > y { | |
| return 1 | |
| } else if x < y { | |
| return -1 | |
| } | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/privilege/privileges/cache.go` around lines 809 - 816, Update the
outdated comment that reads "the order is nondeterministic" to accurately
reflect the deterministic lexicographic comparison implemented here (using the x
> y / x < y branches returning 1/-1/0); locate the comparison routine that
compares x and y (the function containing the shown return 1 / return -1 /
return 0 logic) and reword or remove the misleading sentence so it states that
the comparison is deterministic and follows lexicographic ordering between x and
y.
|
fast_test_tiprow looks infra/flaky (bazel remote exec failure: DNS lookup storage.googleapis.com -> socket: too many open files). /retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67641 +/- ##
================================================
- Coverage 77.7969% 77.4527% -0.3443%
================================================
Files 1983 1966 -17
Lines 548948 549367 +419
================================================
- Hits 427065 425500 -1565
- Misses 120962 123865 +2903
+ Partials 921 2 -919
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fzzf678, wjhuang2016 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 |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/privilege/privileges/cache_test.go (1)
191-193: Remove unnecessaryflush privilegesfrom this unit test.
p.LoadColumnsPrivTable(...)already reloads the source under test. Keepingflush privilegeshere adds global side effects and extra work without increasing assertion value.Suggested minimal diff
p = privileges.NewMySQLPrivilege() tk.MustExec("delete from columns_priv") - tk.MustExec("flush privileges") tk.MustExec(`INSERT INTO mysql.columns_priv VALUES ("%", "db", "user", "table", "c1", "2017-01-04 16:33:42.235831", "Insert,Update")`) tk.MustExec(`INSERT INTO mysql.columns_priv VALUES ("%", "db", "user", "table", "c2", "2017-01-04 16:33:42.235831", "References")`)As per coding guidelines "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/cache_test.go` around lines 191 - 193, Remove the unnecessary global side-effect from the test by deleting the tk.MustExec("flush privileges") call; the test already reloads data via p.LoadColumnsPrivTable(...) so simply keep the delete and the INSERT into mysql.columns_priv and rely on p.LoadColumnsPrivTable to refresh state instead of calling flush privileges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/privilege/privileges/cache_test.go`:
- Around line 191-193: Remove the unnecessary global side-effect from the test
by deleting the tk.MustExec("flush privileges") call; the test already reloads
data via p.LoadColumnsPrivTable(...) so simply keep the delete and the INSERT
into mysql.columns_priv and rely on p.LoadColumnsPrivTable to refresh state
instead of calling flush privileges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c1e4387-5b1d-4172-9ba8-93f1179ec255
📒 Files selected for processing (1)
pkg/privilege/privileges/cache_test.go
|
/retest |
9 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/test unit-test |
|
@fzzf678: The specified target(s) for 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. |
4 participants
This is an automated cherry-pick of #61677
Rebuild note (2026-04-09)
upstream/masterby cherry-picking original merge commit4f65dffdd6480fc1c36c61fe7cd655f163bc9d41.5141aa329e.pkg/privilege/privileges/cache.gopkg/privilege/privileges/cache_test.go(Upstream privilege cache structure has diverged; ported the logic with minimal deltas to keep behavior aligned with privilege: add
ColumnsPrivMapfor accelerating column-privilege #61677.)What problem does this PR solve?
Issue Number: ref #61706
Problem Summary:
What changed and how does it work?
Adapt the MySQLPrivilege to support column privilege.
This PR picks
pkg/privilege/privileges/cache.goandpkg/privilege/privileges/cache_test.gofrom #61677.Key changes (master-side minimal port)
compareHost: adjust host-pattern ordering so%is least specific and''(empty host) sorts after%.columnsPrivRecord.match: add acolumn == "*"special-case gated bySELECTprivilege (coversSELECT COUNT(*) ...).ColumnsPrivMap+buildColumnsPrivMap+MatchColumns: cache/groupmysql.columns_privrecords per user and use it inRequestVerificationto speed up column privilege matching.RequestVerification: treattables_privas table-level only (Table_priv), and route column-level checks tocolumns_privviaMatchColumns(avoid usingtables_priv.Column_privas an authorization shortcut).TestMatchColumns; extendTestSortUserTableto cover%/ host-pattern ordering; improve failure output incheckUserRecord.Verification
rg -n '^(<{7}|={7}|>{7})( |$)'(0 hits)go test -tags=intest ./pkg/privilege/privileges -run TestMatchColumns -count=1go test -tags=intest ./pkg/privilege/privileges -run 'TestLoadColumnsPrivTable|TestMatchColumns|TestSortUserTable|TestPatternMatch' -count=1Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Improvements
Tests