checker: support HeatWave grants in precheck#12752
Conversation
Signed-off-by: gmhdbjd <gmhdbjd@gmail.com>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces dbutil.ShowGrants with a local showGrants implementation to better handle MySQL 8.0 and HeatWave role grants, specifically ignoring or trimming the WITH ADMIN OPTION suffix. It also adds support for ignoring REVOKE statements and mapping the FLUSH_TABLES privilege to RELOAD. The reviewer feedback highlights two important issues: first, trimAdminOption can potentially panic or slice incorrectly when handling non-ASCII characters due to index mismatches after calling strings.ToUpper; second, failing the entire showGrants execution when a single grant statement fails to parse during role discovery is too fragile and should instead log a warning and continue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func trimAdminOption(grant string) string { | ||
| upperGrant := strings.ToUpper(grant) | ||
| idx := strings.LastIndex(upperGrant, " WITH ADMIN OPTION") | ||
| if idx < 0 { | ||
| return grant | ||
| } | ||
| return strings.TrimSpace(grant[:idx]) | ||
| } |
There was a problem hiding this comment.
Using strings.ToUpper to find the index of a substring and then slicing the original string grant using that index can lead to panics or incorrect slicing if the string contains non-ASCII characters (e.g., in usernames or role names). This is because strings.ToUpper can change the byte length of certain Unicode characters (such as Turkish 'i' or other special characters), causing the byte indices to shift.\n\nSince WITH ADMIN OPTION is expected to be a suffix, we can safely perform a case-insensitive suffix check and slice using the known ASCII suffix length.
func trimAdminOption(grant string) string {\n\ttrimmed := strings.TrimSpace(grant)\n\tconst suffix = \" WITH ADMIN OPTION\"\n\tif len(trimmed) >= len(suffix) && strings.EqualFold(trimmed[len(trimmed)-len(suffix):], suffix) {\n\t\treturn strings.TrimSpace(trimmed[:len(trimmed)-len(suffix)])\n\t}\n\treturn grant\n}| node, err := p.ParseOneStmt(grantForParse, "", "") | ||
| if err != nil { | ||
| return nil, errors.New(err.Error()) | ||
| } |
There was a problem hiding this comment.
Failing the entire showGrants function (and thus the entire precheck) if a single grant statement fails to parse during role discovery is too restrictive and fragile. If a user has a custom or unsupported grant syntax that is ignored during privilege verification (via shouldIgnoreGrant), it will still cause a hard failure here.\n\nSince this loop is only used for best-effort role discovery, we should log a warning and continue instead of returning an error.
\t\tnode, err := p.ParseOneStmt(grantForParse, \"\", \"\")\n\t\tif err != nil {\n\t\t\tlog.L().Warn(\"failed to parse grant statement during role discovery\", zap.String(\"grant\", grant), zap.Error(err))\n\t\t\tcontinue\n\t\t}
|
@GMHDBJD: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| return readGrants(builder.String()) | ||
| } | ||
|
|
||
| func shouldIgnoreGrant(grant string) bool { |
There was a problem hiding this comment.
maybe called containsWithAdminOption
| if shouldIgnoreGrant(grant) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
as we have consider the WITH ADMIN OPTION inside showGrants, we can remove this check?
| case ast.GrantLevelGlobal: | ||
| for _, privElem := range grantStmt.Privs { | ||
| if privElem.Priv == mysql.ExtendedPriv { | ||
| if strings.EqualFold(privElem.Name, "FLUSH_TABLES") { |
There was a problem hiding this comment.
is this HW specific? maybe add a comment
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 4
- Inline comments: 4
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (2)
- Partial revokes can be reported as sufficient privileges (
dm/pkg/checker/privilege.go:320) - Version-specific MariaDB grants can fail before MariaDB parsing is enabled (
dm/pkg/checker/privilege.go:501)
🟡 [Minor] (1)
- Grant predicate name hides the role-admin case (
dm/pkg/checker/privilege.go:505 and dm/pkg/checker/privilege.go:534)
ℹ️ [Info] (1)
- FLUSH_TABLES-to-RELOAD mapping needs rationale (
dm/pkg/checker/privilege.go:336)
| if !ok { | ||
| switch node.(type) { | ||
| case *ast.GrantProxyStmt, *ast.GrantRoleStmt: | ||
| case *ast.GrantProxyStmt, *ast.GrantRoleStmt, *ast.RevokeStmt: |
There was a problem hiding this comment.
⚠️ [Major] Partial revokes can be reported as sufficient privileges
Why
The new *ast.RevokeStmt branch treats parsed partial-revoke rows from SHOW GRANTS as non-operative metadata, but MySQL partial revokes subtract privileges from earlier global grants for a specific schema or table. The verifier only removes privileges from lackPrivs; it never applies a revoke back to the required table or schema set.
Scope
dm/pkg/checker/privilege.go:320
Risk if unchanged
A source account with a global required privilege plus REVOKE ... ON checked_db.* can pass DM's dump privilege precheck even though the account is denied on the checked schema or table. DM may then start after a checker success and fail later during dump/load/replication access, and an unsupported partial-revoke case becomes a silent compatibility pass.
Evidence
VerifyPrivileges now continues on *ast.RevokeStmt, while the global grant branch deletes required privileges from lackPrivs. The concrete state transition is: pending SelectPriv for app.t -> GRANT SELECT ON *.* clears it -> REVOKE SELECT ON app.* is skipped -> final result incorrectly has no missing SELECT. The parser AST exposes RevokeStmt.Privs and RevokeStmt.Level, and the added test at dm/pkg/checker/privilege_test.go:219 uses REVOKE ... ON sys.* without a checked sys table, so it does not cover an overlapping revoke.
Change request
Please model RevokeStmt for required privileges, or fail the precheck conservatively when a partial revoke overlaps the checked schema/table; add a unit test for global SELECT plus REVOKE SELECT ON checked_db.* where the checker must fail.
| // For MySQL 8.0, collect granted roles and read grants using those roles. | ||
| // HeatWave SHOW GRANTS may append `WITH ADMIN OPTION` to role grants, which | ||
| // TiDB parser cannot parse yet. Strip the suffix for role discovery only. | ||
| var roles []string |
There was a problem hiding this comment.
⚠️ [Major] Version-specific MariaDB grants can fail before MariaDB parsing is enabled
Why
The new showGrants helper pre-parses every returned grant only to discover roles, but it does so with a plain TiDB parser before the existing MariaDB parser mode in VerifyPrivileges can run.
Scope
dm/pkg/checker/privilege.go:501
Risk if unchanged
MariaDB sources can get a checker error for grant syntax that the existing verifier is explicitly prepared to parse, changing the precheck result from a privilege decision into an early parse failure.
Evidence
VerifyPrivileges enables p.SetMariaDB(true) when the stored version contains MariaDB, with a comment calling out BINLOG MONITOR and other MariaDB things. The new showGrants has no version parameter and returns immediately on any parse error before verifyPrivilegesWithResult receives the grant list and version.
Change request
Please either pass the DB version into showGrants and use the same parser mode, or avoid failing role discovery on non-role grant parse errors; add a case for a MariaDB-specific grant row to keep this path covered.
| p := parser.New() | ||
| for _, grant := range grants { | ||
| grantForParse := grant | ||
| if shouldIgnoreGrant(grant) { |
There was a problem hiding this comment.
🟡 [Minor] Grant predicate name hides the role-admin case
Why
shouldIgnoreGrant reads like a general privilege-check decision, but the implementation only detects role grants with WITH ADMIN OPTION; in showGrants the same predicate is used to trim and parse the role instead of ignoring it.
Scope
dm/pkg/checker/privilege.go:505 and dm/pkg/checker/privilege.go:534
Risk if unchanged
Future changes can reuse the predicate as a broad ignore gate and accidentally skip grant forms that still need parsing, or miss that role discovery depends on this exact HeatWave role-grant shape.
Evidence
The predicate returns true only for GRANT ... TO ... WITH ADMIN OPTION statements without ON. VerifyPrivileges continues on it, while showGrants treats the same true value as trimAdminOption(grant) before parsing roles.
Change request
Prefer a more specific name, for example isRoleGrantWithAdminOption, and use a separate shouldSkipGrantForPrivilegeCheck wrapper if the verification path needs the ignore wording.
| switch grantStmt.Level.Level { | ||
| case ast.GrantLevelGlobal: | ||
| for _, privElem := range grantStmt.Privs { | ||
| if privElem.Priv == mysql.ExtendedPriv { |
There was a problem hiding this comment.
ℹ️ [Info] FLUSH_TABLES-to-RELOAD mapping needs rationale
Why
The new dynamic-privilege branch maps FLUSH_TABLES to mysql.ReloadPriv, which is a non-obvious cross-vendor privilege equivalence.
Scope
dm/pkg/checker/privilege.go:336
Risk if unchanged
Maintainers may treat other FLUSH_* dynamic privileges the same way, or remove the special case, because the intent is not captured near the enforcement point.
Evidence
The code deletes mysql.ReloadPriv only for mysql.ExtendedPriv named FLUSH_TABLES, while the adjacent comments still describe only AllPriv; the added tests separately assert FLUSH_STATUS does not satisfy RELOAD.
Change request
Please add a short why-comment for this branch explaining why FLUSH_TABLES is accepted as satisfying RELOAD and why sibling FLUSH_* extended privileges are not.
2 participants
What problem does this PR solve?
Issue Number: ref #12751
This PR fixes DM precheck compatibility with Oracle HeatWave/MySQL 8 style
SHOW GRANTSoutput.Problems addressed:
WITH ADMIN OPTION, which TiDB parser cannot parse asGrantRoleStmtyet.SHOW GRANTS; these are not direct grants and should not fail privilege verification.FLUSH_TABLESas a dynamic privilege. It should satisfy the FTWRL dump privilege requirement currently modeled asRELOADin DM precheck.What is changed and how it works?
showGrantshelper to tolerate role grants withWITH ADMIN OPTIONwhile still discovering active roles viaSHOW GRANTS ... USING ....FLUSH_TABLESas satisfying the dump privilege checker’s FTWRL requirement.WITH ADMIN OPTION, andFLUSH_TABLESdynamic privilege handling.Check List
Tests:
Manual validation:
go test ./dm/pkg/checker ./dm/checkerdm-master,dm-worker, anddmctlfrom this branch.ignore-checking-items.check-taskpassed with only the MySQL version warning.start-tasksucceeded.Related issue: #12751