executor: fix case-insensitive grant and revoke on schema names#68456
Conversation
ti-chi-bot
Bot
added
release-note
|
@expxiaoli 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
Bot
added
the
size/XL
|
Hi @expxiaoli. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGRANT and REVOKE now normalize database and table names via infoschema metadata to resolve case-insensitive references consistently when ChangesSchema-name normalization for GRANT/REVOKE with case-insensitive collation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
pkg/executor/revoke_test.go (1)
138-166: ⚡ Quick winCover the revoke-on-missing-table compatibility branch too.
revokeOneUsernow intentionally toleratesinfoschema.ErrTableNotExistsbefore checking the privilege row, but these subtests only revoke against existing tables. Please add one case where the privilege row exists and the metadata lookup fails, so this fallback path is covered as well.🤖 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/executor/revoke_test.go` around lines 138 - 166, Add a subcase to TestRevokeTableScopeCaseInsensitiveWithNewCollationDisabled that exercises the branch in revokeOneUser which tolerates infoschema.ErrTableNotExists: using newCollationDisabledBootstrapTestKit(), create user and table, GRANT SELECT to create the mysql.tables_priv row, DROP the underlying table to force the metadata lookup to fail with ErrTableNotExists, then run the REVOKE (using the same mixed-case target used elsewhere) and assert the mysql.tables_priv entry is removed; reference the test function name and revokeOneUser when locating where to add this flow.pkg/executor/grant.go (1)
571-577: ⚡ Quick winDocument why
ErrTableNotExistsis intentionally tolerated here.This branch is carrying a SQL-compatibility rule, not generic best-effort error handling. A brief comment would make it much clearer that continuing here is only valid because the earlier validation already decided GRANT-on-missing-table is allowed.
Proposed clarification
dbName, tbl, err := getTargetSchemaAndTable(ctx, e.Ctx(), e.Level.DBName, e.Level.TableName, e.is) - if err != nil && !terror.ErrorEqual(err, infoschema.ErrTableNotExists) { + // Keep going for the CREATE-on-missing-table compatibility path. The + // caller has already validated whether GRANT is allowed when the target + // table does not exist. + if err != nil && !terror.ErrorEqual(err, infoschema.ErrTableNotExists) { return err }As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear."
🤖 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/executor/grant.go` around lines 571 - 577, Add a brief explanatory comment above the error check around err and infoschema.ErrTableNotExists in the GRANT execution path (the block that sets tblName using e.Level.TableName and tbl.Meta().Name.O) stating that ErrTableNotExists is intentionally tolerated due to SQL-compatibility: earlier validation allows GRANT on a missing table so we must continue rather than treat this as a hard failure; mention the invariant that prior validation already decided GRANT-on-missing-table is allowed and therefore skipping the error here preserves that SQL compatibility contract.pkg/executor/grant_test.go (1)
192-230: ⚡ Quick winAdd a regression for the missing-table fallback path.
These subtests only cover existing tables, but
grantTableLevelnow has a new compatibility branch that keeps going afterinfoschema.ErrTableNotExists. A small case likeGRANT CREATE ON TEST.missing_tbl ...would exercise that branch directly and protect the row-key behavior this PR is trying to preserve.🤖 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/executor/grant_test.go` around lines 192 - 230, Add a subtest inside TestGrantTableScopeCaseInsensitiveWithNewCollationDisabled that exercises the missing-table fallback in grantTableLevel: using newCollationDisabledBootstrapTestKit create a user (e.g., 'testTblCaseMissing'@'%'), ensure the target table does NOT exist (DROP TABLE IF EXISTS test.missing_tbl), run GRANT CREATE ON TEST.missing_tbl TO 'testTblCaseMissing'@'%', then query mysql.tables_priv (and optionally SHOW GRANTS FOR the user) to assert the expected grant row exists; this will force the grantTableLevel branch that catches infoschema.ErrTableNotExists and validate the row-key behavior.
🤖 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.
Nitpick comments:
In `@pkg/executor/grant_test.go`:
- Around line 192-230: Add a subtest inside
TestGrantTableScopeCaseInsensitiveWithNewCollationDisabled that exercises the
missing-table fallback in grantTableLevel: using
newCollationDisabledBootstrapTestKit create a user (e.g.,
'testTblCaseMissing'@'%'), ensure the target table does NOT exist (DROP TABLE IF
EXISTS test.missing_tbl), run GRANT CREATE ON TEST.missing_tbl TO
'testTblCaseMissing'@'%', then query mysql.tables_priv (and optionally SHOW
GRANTS FOR the user) to assert the expected grant row exists; this will force
the grantTableLevel branch that catches infoschema.ErrTableNotExists and
validate the row-key behavior.
In `@pkg/executor/grant.go`:
- Around line 571-577: Add a brief explanatory comment above the error check
around err and infoschema.ErrTableNotExists in the GRANT execution path (the
block that sets tblName using e.Level.TableName and tbl.Meta().Name.O) stating
that ErrTableNotExists is intentionally tolerated due to SQL-compatibility:
earlier validation allows GRANT on a missing table so we must continue rather
than treat this as a hard failure; mention the invariant that prior validation
already decided GRANT-on-missing-table is allowed and therefore skipping the
error here preserves that SQL compatibility contract.
In `@pkg/executor/revoke_test.go`:
- Around line 138-166: Add a subcase to
TestRevokeTableScopeCaseInsensitiveWithNewCollationDisabled that exercises the
branch in revokeOneUser which tolerates infoschema.ErrTableNotExists: using
newCollationDisabledBootstrapTestKit(), create user and table, GRANT SELECT to
create the mysql.tables_priv row, DROP the underlying table to force the
metadata lookup to fail with ErrTableNotExists, then run the REVOKE (using the
same mixed-case target used elsewhere) and assert the mysql.tables_priv entry is
removed; reference the test function name and revokeOneUser when locating where
to add this flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e1020706-3eb2-4eb1-b2e2-5f6e7795401d
📒 Files selected for processing (4)
pkg/executor/grant.gopkg/executor/grant_test.gopkg/executor/revoke.gopkg/executor/revoke_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68456 +/- ##
================================================
- Coverage 77.2799% 77.0246% -0.2554%
================================================
Files 2010 2036 +26
Lines 555615 572939 +17324
================================================
+ Hits 429379 441304 +11925
- Misses 125316 129046 +3730
- Partials 920 2589 +1669
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Agree. Code is fixed. |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
bb7133
added
the
needs-cherry-pick-release-8.5
|
/test-required |
|
/check-issue-triage-complete |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fzzf678, tiancaiamao 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/test-required |
2 similar comments
|
/test-required |
|
/test-required |
|
In response to a cherrypick label: new pull request created to branch |
5 participants
What problem does this PR solve?
Issue Number: close #68406, ref #66867
Problem Summary:
When
new_collation_enabled = false, mixed-case schema names in privilege statements can behave inconsistently.For table-scope and column-scope privilege operations, statements like
GRANT ... ON TEST.t1andGRANT ... ON test.t1may not target the same privilege row inmysql.tables_priv/mysql.columns_priv. This can cause:GRANTto fail withget table privilege fail/get column privilege failREVOKEto miss the existing grantIn addition, the existing
WithNewCollationDisabledunit tests only toggled the in-process collation flag, but did not actually bootstrap the test cluster withnew_collation_enabled = false, so the test premise was misleading and missed the real schema-name case.What changed and how does it work?
Add shared schema canonicalization for privilege object resolution:
getTargetSchemaName()to resolve the canonical schema name from infoschema when the schema exists.getTargetSchemaAndTable()to canonicalize the schema name before resolving the table.Use canonical schema/table names consistently in privilege DDL paths:
GRANTGRANTREVOKEREVOKEGRANT/REVOKEPreserve existing compatibility for non-existent objects:
Fix the test setup for
new_collation_enabled = false:NewCollationsEnabledOnFirstBootstrap = falsemysql.tidb.new_collation_enabledvalue isFalseAdd/extend regression tests for mixed-case schema names under disabled new collation:
TestGrantTableScopeCaseInsensitiveWithNewCollationDisabledTestRevokeTableScopeCaseInsensitiveWithNewCollationDisabledTestGrantColumnScopeTestRevokeColumnScopeCheck List
Tests
Test commands:
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests