privilege: add ColumnsPrivMap for accelerating column-privilege (#61677)#66532
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66532 +/- ##
================================================
Coverage ? 54.9898%
================================================
Files ? 1818
Lines ? 652868
Branches ? 0
================================================
Hits ? 359011
Misses ? 267225
Partials ? 26632
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@tiancaiamao: Your lgtm message is repeated, so it is ignored. 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. |
📝 WalkthroughWalkthroughThis PR introduces column-level privilege caching through a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/privilege/privileges/cache.go (2)
1166-1173: Consider adding parentheses for clarity.The logic is correct due to operator precedence (
&&binds tighter than||), but adding explicit parentheses would improve readability and make the intent clearer for future maintainers.♻️ 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 1166 - 1173, The boolean expression in columnsPrivRecord.match is correct but unclear; update the return statement in function columnsPrivRecord.match to add explicit parentheses around the final OR condition so the intent is obvious: keep the checks record.baseRecord.match(user, host) && strings.EqualFold(record.DB, db) && strings.EqualFold(record.TableName, table) and wrap (strings.EqualFold(record.ColumnName, col) || (col == "*" && (record.ColumnPriv & mysql.SelectPriv) > 0)) to clarify grouping of ColumnName equality vs the special "*" + ColumnPriv check.
1296-1298: Misleading comment: method is used in production code.The comment states "exported only for test" but
MatchColumnsis also called inRequestVerification(line 1402). Consider updating the comment to reflect its actual usage.📝 Suggested fix
-// MatchColumns is exported only for test +// MatchColumns finds a column-level privilege record for the given user, host, db, table, and column. +// The special column value "*" matches any column with SELECT privilege. func (p *MySQLPrivilege) MatchColumns(user, host, db, table, column string) *columnsPrivRecord {🤖 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 1296 - 1298, The comment for MatchColumns is misleading—it's not only exported for tests but also invoked from production code (see RequestVerification calling MySQLPrivilege.MatchColumns); update the doc comment above func MatchColumns(user, host, db, table, column string) *columnsPrivRecord to accurately describe its purpose and usage (e.g., "MatchColumns returns the columns privilege record for the given user/host/db/table/column; used by RequestVerification and tests"), ensuring the comment references MySQLPrivilege.MatchColumns and RequestVerification so future readers know it's used in production.
🤖 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.go`:
- Around line 1166-1173: The boolean expression in columnsPrivRecord.match is
correct but unclear; update the return statement in function
columnsPrivRecord.match to add explicit parentheses around the final OR
condition so the intent is obvious: keep the checks
record.baseRecord.match(user, host) && strings.EqualFold(record.DB, db) &&
strings.EqualFold(record.TableName, table) and wrap
(strings.EqualFold(record.ColumnName, col) || (col == "*" && (record.ColumnPriv
& mysql.SelectPriv) > 0)) to clarify grouping of ColumnName equality vs the
special "*" + ColumnPriv check.
- Around line 1296-1298: The comment for MatchColumns is misleading—it's not
only exported for tests but also invoked from production code (see
RequestVerification calling MySQLPrivilege.MatchColumns); update the doc comment
above func MatchColumns(user, host, db, table, column string) *columnsPrivRecord
to accurately describe its purpose and usage (e.g., "MatchColumns returns the
columns privilege record for the given user/host/db/table/column; used by
RequestVerification and tests"), ensuring the comment references
MySQLPrivilege.MatchColumns and RequestVerification so future readers know it's
used in production.
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fzzf678, tiancaiamao, 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 |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
This is an automated cherry-pick of #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 pick
pkg/privilege/privileges/cache.goandpkg/privilege/privileges/cache_test.goin #61638.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Improvements
Tests