domain, privilege: add PLAN_REPLAYER_EXPLAIN_ADMIN dynamic privilege#67576
domain, privilege: add PLAN_REPLAYER_EXPLAIN_ADMIN dynamic privilege#67576cryo-zd wants to merge 8 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
Hi @cryo-zd. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
ti-chi-bot
Bot
added
contribution
|
Hi @cryo-zd. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new dynamic privilege Changes
Sequence Diagram(s)sequenceDiagram
participant Exec as Executor
participant Domain as PlanReplayerDumper
participant Planner
participant PrivMgr as PrivilegeManager
participant Sctx as SessionCtx
Exec->>Domain: dumpSchemasWithTask(ctx, task)
Domain->>Domain: shouldUsePlanReplayerExplainAdminBypass(sctx, task)
Domain->>PrivMgr: RequestDynamicVerification("PLAN_REPLAYER_EXPLAIN_ADMIN")
PrivMgr-->>Domain: allow/deny
Domain->>Planner: dumpExplain(ctx, task) (first attempt)
Planner->>PrivMgr: CheckPrivilege(...) (normal verification)
alt Privilege denied and domain allowed bypass
Planner-->>Domain: privilege error
Domain->>Sctx: set PlanReplayerPrivilegeBypass=true
Domain->>Planner: dumpExplain(ctx, task) (retry)
Planner->>PrivMgr: canBypassPlanReplayerPrivilegeCheck(sctx, pm)
PrivMgr-->>Planner: true
Planner-->>Domain: explain result
Domain->>Sctx: restore PlanReplayerPrivilegeBypass=false
else No bypass needed
Planner-->>Domain: explain result
end
Domain->>Exec: write artifacts (zip)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
pkg/executor/test/planreplayer/plan_replayer_test.go (1)
347-407: Add a negative control that proves the bypass stays scoped.This only checks successful dumps, so it would still pass if plain
EXPLAIN/SHOW CREATEwere accidentally widened or if a user without both grants could use the command. Please add a user with neitherSELECTnorPLAN_REPLAYER_EXPLAIN_ADMIN, and assertpr_adminis still denied on plainEXPLAIN/SHOW CREATEoutsidePLAN REPLAYER—ideally in an integration-style SQL test as well.Based on learnings: Applies to pkg/executor/** : For SQL behavior changes in executor, perform targeted unit test plus relevant integration test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/planreplayer/plan_replayer_test.go` around lines 347 - 407, Add a negative control to TestPlanReplayerExplainAdminPrivilege: create a third user (e.g., 'pr_none'@'localhost') with no SELECT and no PLAN_REPLAYER_EXPLAIN_ADMIN grants, authenticate a TestKit as that user and assert that "plan replayer dump explain ..." fails/does not produce privileged output the same way as a normal user, and also explicitly assert that pr_admin (authenticated via adminTk) is denied for plain EXPLAIN and SHOW CREATE outside of the plan replayer (e.g., run adminTk.MustQueryToErr("EXPLAIN SELECT ...") and adminTk.MustQueryToErr("SHOW CREATE VIEW v_plan_replayer_priv")). Locate changes in the TestPlanReplayerExplainAdminPrivilege function and reuse the readZipEntryNames helper to check outputs; ensure you create/grant users with rootTk.MustExec and authenticate them with TestKit.Session().Auth like pr_normal/pr_admin so the test proves the bypass is scoped.
🤖 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/privileges.go`:
- Around line 217-220: The current branch returns early when
mysqlPriv.RequestVerification(...) fails but canBypassPlanReplayerPrivilege(...)
is true, skipping any extension veto from authPluginRequestVerification; update
the control flow so that even if canBypassPlanReplayerPrivilege(activeRoles) is
true you still invoke authPluginRequestVerification(p.user, p.host, activeRoles,
db, table, column, priv) and only allow the bypass if the auth plugin allows it
(i.e., call authPluginRequestVerification when present and combine its boolean
result with canBypassPlanReplayerPrivilege), keeping references to
mysqlPriv.RequestVerification, canBypassPlanReplayerPrivilege, and
authPluginRequestVerification to locate the change.
---
Nitpick comments:
In `@pkg/executor/test/planreplayer/plan_replayer_test.go`:
- Around line 347-407: Add a negative control to
TestPlanReplayerExplainAdminPrivilege: create a third user (e.g.,
'pr_none'@'localhost') with no SELECT and no PLAN_REPLAYER_EXPLAIN_ADMIN grants,
authenticate a TestKit as that user and assert that "plan replayer dump explain
..." fails/does not produce privileged output the same way as a normal user, and
also explicitly assert that pr_admin (authenticated via adminTk) is denied for
plain EXPLAIN and SHOW CREATE outside of the plan replayer (e.g., run
adminTk.MustQueryToErr("EXPLAIN SELECT ...") and adminTk.MustQueryToErr("SHOW
CREATE VIEW v_plan_replayer_priv")). Locate changes in the
TestPlanReplayerExplainAdminPrivilege function and reuse the readZipEntryNames
helper to check outputs; ensure you create/grant users with rootTk.MustExec and
authenticate them with TestKit.Session().Auth like pr_normal/pr_admin so the
test proves the bypass is scoped.
🪄 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: 0119bb8d-4e4e-4d2b-bc80-92389dcd2821
📒 Files selected for processing (11)
pkg/domain/BUILD.bazelpkg/domain/plan_replayer_dump.gopkg/domain/plan_replayer_test.gopkg/executor/test/planreplayer/BUILD.bazelpkg/executor/test/planreplayer/plan_replayer_test.gopkg/privilege/privilege.gopkg/privilege/privileges/privileges.gopkg/sessionctx/variable/BUILD.bazelpkg/sessionctx/variable/plan_replayer.gopkg/sessionctx/variable/session.gotests/integrationtest/r/executor/executor.result
| if !mysqlPriv.RequestVerification(activeRoles, p.user, p.host, db, table, column, priv) { | ||
| return false | ||
| return p.canBypassPlanReplayerPrivilege(activeRoles) | ||
| } | ||
| return p.authPluginRequestVerification == nil || p.authPluginRequestVerification(p.user, p.host, activeRoles, db, table, column, priv) |
There was a problem hiding this comment.
Keep extension-auth vetoes on the bypass path.
When the built-in MySQL check fails and canBypassPlanReplayerPrivilege(...) returns true, this returns before authPluginRequestVerification runs. That means a deployment using an extension auth plugin loses its plugin-side static privilege veto for these helper SQLs.
Possible fix
mysqlPriv := p.Handle.Get()
- if !mysqlPriv.RequestVerification(activeRoles, p.user, p.host, db, table, column, priv) {
- return p.canBypassPlanReplayerPrivilege(activeRoles)
- }
- return p.authPluginRequestVerification == nil || p.authPluginRequestVerification(p.user, p.host, activeRoles, db, table, column, priv)
+ granted := mysqlPriv.RequestVerification(activeRoles, p.user, p.host, db, table, column, priv)
+ if !granted {
+ granted = p.canBypassPlanReplayerPrivilege(activeRoles)
+ }
+ if !granted {
+ return false
+ }
+ return p.authPluginRequestVerification == nil || p.authPluginRequestVerification(p.user, p.host, activeRoles, db, table, column, priv)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/privilege/privileges/privileges.go` around lines 217 - 220, The current
branch returns early when mysqlPriv.RequestVerification(...) fails but
canBypassPlanReplayerPrivilege(...) is true, skipping any extension veto from
authPluginRequestVerification; update the control flow so that even if
canBypassPlanReplayerPrivilege(activeRoles) is true you still invoke
authPluginRequestVerification(p.user, p.host, activeRoles, db, table, column,
priv) and only allow the bypass if the auth plugin allows it (i.e., call
authPluginRequestVerification when present and combine its boolean result with
canBypassPlanReplayerPrivilege), keeping references to
mysqlPriv.RequestVerification, canBypassPlanReplayerPrivilege, and
authPluginRequestVerification to locate the change.
There was a problem hiding this comment.
Plugin enforcement is still preserved on that path via RequestDynamicVerification("PLAN_REPLAYER_EXPLAIN_ADMIN", ...) , which in turn invokes authPluginRequestDynamicVerification(). So the plugin can still veto the helper SQLs through the dynamic privilege hook ?
There was a problem hiding this comment.
@cryo-zd, you raise a valid and well-reasoned point. RequestDynamicVerification does invoke authPluginRequestDynamicVerification before returning, so an extension auth plugin can veto the bypass by returning false for PLAN_REPLAYER_EXPLAIN_ADMIN. The dynamic hook is not skipped.
There is, however, a narrow residual gap worth being aware of:
The skipped hook is specifically authPluginRequestVerification — the static privilege check (table-level SELECT, etc.). A plugin that only implements authPluginRequestVerification (e.g., to enforce fine-grained, per-table access rules) but has not implemented authPluginRequestDynamicVerification would have its static veto bypassed. Because authPluginRequestDynamicVerification is nil, RequestDynamicVerification falls through to true, enabling the bypass even if the plugin's static rules would have denied access.
In practice this means:
- Plugins that implement the dynamic hook → fully protected (your point is correct).
- Plugins that rely solely on the static hook without a corresponding dynamic hook for
PLAN_REPLAYER_EXPLAIN_ADMIN→ their per-table static veto is silently skipped on the bypass path.
Since PLAN_REPLAYER_EXPLAIN_ADMIN is a new privilege, existing plugins that predate this PR would not have implemented a dynamic handler for it, leaving them in the second category. Whether that's acceptable depends on whether TiDB's extension plugin contract requires plugins to opt into new dynamic privileges explicitly. If so, my original comment may be overstated and your reasoning holds. But it's a nuance worth documenting.
🧠 Learnings used
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:1887-1898
Timestamp: 2026-03-12T08:41:44.509Z
Learning: In pkg/planner/core/preprocess.go (pingcap/tidb PR `#65532`), the `checkLockClauseTables` function intentionally accepts `FOR UPDATE OF <base_table>` even when the FROM item was introduced with an alias (`matched.alias.L != "" && !matchedByAlias`). Rather than returning an error (strict MySQL behavior), TiDB emits a warning to guide users toward the alias form. This backward-compatibility fallback is a deliberate acceptance criterion for the change — do not flag it as a MySQL-incompatibility bug in future reviews.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:1906-1910
Timestamp: 2026-03-12T08:03:56.097Z
Learning: In pingcap/tidb (PR `#65532`), `buildSelect` in `pkg/planner/core/logical_plan_builder.go` derives the privilege-check DB for `FOR UPDATE OF` lock targets using `getLowerDB(tName.Schema, ...)`. For alias or unqualified lock targets whose `TableName.Schema` is empty after preprocessing, this falls back to `currentDB` and authorizes against the wrong database. The correct fix is to use `tNameW.DBInfo.Name.L` (with a nil guard) directly, since `tNameW` is already fully resolved by `checkLockClauseTables` in preprocess.go. This is consistent with how privilege checks for INSERT/DELETE use `tnW.DBInfo.Name.L` elsewhere in the codebase.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: pkg/planner/core/logical_plan_builder.go:6455-6472
Timestamp: 2026-03-11T06:29:00.122Z
Learning: Ensure code reviews verify the VirtualAssignmentsOffset semantics: the planner sets Update.VirtualAssignmentsOffset = len(update.List). The executor should only apply OrderedList[:VirtualAssignmentsOffset] when composing new rows, and only after the 'changed' check should it group/evaluate OrderedList[VirtualAssignmentsOffset:] per table. This pattern applies to files under pkg/planner/core and pkg/executor (e.g., common_plans.go and update.go). Reviewers should check that updates respect slicing behavior, that the offset is consistently derived from the planner, and that downstream code does not bypass the offset when creating new rows. Add tests validating both branches: the slice before the offset for new rows, and the per-table handling of the slice after the offset.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/domain/plan_replayer_dump.go (1)
525-536: Precompute the task-level bypass decision once.
shouldUsePlanReplayerExplainAdminBypassdepends only ontaskandsctx, but the schema path recomputes it inside eachSHOW CREATEretry. Caching that boolean indumpSchemasWithTaskand passing it down would make this security-sensitive flow easier to audit and avoid rewalking every statement for each extracted object.As per coding guidelines, code SHOULD remain maintainable for future readers with basic TiDB familiarity, including readers who are not experts in the specific subsystem/feature.
Also applies to: 573-590, 943-955
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/domain/plan_replayer_dump.go` around lines 525 - 536, Precompute the result of shouldUsePlanReplayerExplainAdminBypass once in dumpSchemasWithTask and pass that boolean into getShowCreateTable (and any other helpers that currently call shouldUsePlanReplayerExplainAdminBypass internally) instead of recomputing it for every SHOW CREATE retry; update the signature of getShowCreateTable (and mirrored call sites noted around the other ranges) to accept the cached bypass flag, use it inside the retry logic, and keep dumpSchemaMeta unchanged—this avoids repeated security decision recomputation and makes the flow easier to audit.
🤖 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/domain/plan_replayer_dump.go`:
- Around line 593-596: The file-based (deferred) dump path can mutate the user's
shared SessionVars via runWithPlanReplayerSQLPrivilegeType, creating a
concurrency/security hole; change the deferred path so it does not mutate the
caller's session: update
DumpPlanReplayerInfo/handleSpecialQuery/PlanReplayerExec.Next to either accept a
PlanReplayerInternalSQLType parameter and use that instead of touching
SessionVars, or create a dedicated session/session-vars clone for the deferred
worker and set the privilege on that clone (e.g., obtain a new session via
createSessions() or clone sessionctx.GetSessionVars(), set
PlanReplayerSQLPrivilegeType there) before running the async dump, and remove
the temporary SetPlanReplayerSQLPrivilegeType calls on the original user session
in runWithPlanReplayerSQLPrivilegeType for the deferred path.
---
Nitpick comments:
In `@pkg/domain/plan_replayer_dump.go`:
- Around line 525-536: Precompute the result of
shouldUsePlanReplayerExplainAdminBypass once in dumpSchemasWithTask and pass
that boolean into getShowCreateTable (and any other helpers that currently call
shouldUsePlanReplayerExplainAdminBypass internally) instead of recomputing it
for every SHOW CREATE retry; update the signature of getShowCreateTable (and
mirrored call sites noted around the other ranges) to accept the cached bypass
flag, use it inside the retry logic, and keep dumpSchemaMeta unchanged—this
avoids repeated security decision recomputation and makes the flow easier to
audit.
🪄 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: 056935e2-2732-4ea0-a54f-45bedd4ed0ab
📒 Files selected for processing (1)
pkg/domain/plan_replayer_dump.go
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
|
Please create an issue and modify The issue should have label |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67576 +/- ##
================================================
+ Coverage 77.6380% 79.5305% +1.8925%
================================================
Files 1961 1991 +30
Lines 543853 550983 +7130
================================================
+ Hits 422237 438200 +15963
+ Misses 120806 111322 -9484
- Partials 810 1461 +651
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
| mysqlPriv := p.Handle.Get() | ||
| if !mysqlPriv.RequestVerification(activeRoles, p.user, p.host, db, table, column, priv) { | ||
| return false | ||
| return p.canBypassPlanReplayerPrivilege(activeRoles) |
There was a problem hiding this comment.
Please do not hack the privilege in this package. I prefer to skip the visitInfo of the privilege in planner when the user has the requested dynamic privilege.
There was a problem hiding this comment.
Gave it a try, PTAL.
| PlanReplayerInternalSQLTypeNone PlanReplayerInternalSQLType = iota | ||
| PlanReplayerInternalSQLTypeExplain | ||
| PlanReplayerInternalSQLTypeShowCreateTable | ||
| PlanReplayerInternalSQLTypeShowCreateView |
There was a problem hiding this comment.
Do we have different behavior for these three flags? Maybe it's fine to just use a bool value to represent whether the current statement is executed inside a plan replayer statement.
| InPlanReplayer bool | ||
|
|
||
| // planReplayerSQLPrivilegeType limits the temporary privilege bypass to one helper SQL shape. | ||
| planReplayerSQLPrivilegeType PlanReplayerInternalSQLType |
There was a problem hiding this comment.
Add this thing to the SessionVars is not a good idea, though I know many strange states are already added to the SessionVars 🤦 .
As it's not widely used, I'd suggest to add this state though SetValue and Value.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/privilege/privileges/privileges.go (2)
217-219: Consider gating the fallback byprivtoo.Right now any failed MySQL static privilege check under these helper SQL modes can fall through to
PLAN_REPLAYER_EXPLAIN_ADMIN. PassingprivintocanBypassPlanReplayerPrivilegeand whitelisting only the expected read-only checks would keep later helper changes from silently widening the bypass surface.Also applies to: 223-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 217 - 219, The fallback to canBypassPlanReplayerPrivilege currently ignores which privilege was checked, allowing any failed mysqlPriv.RequestVerification to fall through; update canBypassPlanReplayerPrivilege to accept the priv parameter and change call sites (e.g., the branch in privileges.go where mysqlPriv.RequestVerification(activeRoles, p.user, p.host, db, table, column, priv) is used) to pass priv through, then implement a whitelist inside canBypassPlanReplayerPrivilege that only allows specific read-only privileges (e.g., SELECT/SHOW/EXPLAIN) to bypass PLAN_REPLAYER_EXPLAIN_ADMIN so other privileges cannot be silently widened.
81-82: Document the exact bypass contract.
PLAN_REPLAYER_EXPLAIN_ADMINreads like a SELECT-only escape hatch, but the actual allowlist here is session-scopedEXPLAIN,SHOW CREATE TABLE, andSHOW CREATE VIEW. Please spell that out near the privilege registration or helper; this path is security-sensitive and the current wording leaves too much implicit.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."
Also applies to: 223-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/privilege/privileges/privileges.go` around lines 81 - 82, Update the documentation near the PLAN_REPLAYER_EXPLAIN_ADMIN privilege registration and the related helper at lines referenced so it explicitly states the precise bypass contract: that this privilege allows session-scoped execution of EXPLAIN, SHOW CREATE TABLE, and SHOW CREATE VIEW only (and does not grant general SELECT or other table-read rights), including any constraints (e.g., whether it applies to all schemas/tables or only those in the current session) and any security/invariant notes; mention the exact symbols PLAN_REPLAYER_EXPLAIN_ADMIN and the helper/registration function used in this file so reviewers can find the comment easily and ensure it covers both the initial declaration and the block around lines 223-238.
🤖 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/privileges.go`:
- Around line 217-219: The fallback to canBypassPlanReplayerPrivilege currently
ignores which privilege was checked, allowing any failed
mysqlPriv.RequestVerification to fall through; update
canBypassPlanReplayerPrivilege to accept the priv parameter and change call
sites (e.g., the branch in privileges.go where
mysqlPriv.RequestVerification(activeRoles, p.user, p.host, db, table, column,
priv) is used) to pass priv through, then implement a whitelist inside
canBypassPlanReplayerPrivilege that only allows specific read-only privileges
(e.g., SELECT/SHOW/EXPLAIN) to bypass PLAN_REPLAYER_EXPLAIN_ADMIN so other
privileges cannot be silently widened.
- Around line 81-82: Update the documentation near the
PLAN_REPLAYER_EXPLAIN_ADMIN privilege registration and the related helper at
lines referenced so it explicitly states the precise bypass contract: that this
privilege allows session-scoped execution of EXPLAIN, SHOW CREATE TABLE, and
SHOW CREATE VIEW only (and does not grant general SELECT or other table-read
rights), including any constraints (e.g., whether it applies to all
schemas/tables or only those in the current session) and any security/invariant
notes; mention the exact symbols PLAN_REPLAYER_EXPLAIN_ADMIN and the
helper/registration function used in this file so reviewers can find the comment
easily and ensure it covers both the initial declaration and the block around
lines 223-238.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c045304-9673-4636-b1d3-a31f1c4b796b
📒 Files selected for processing (1)
pkg/privilege/privileges/privileges.go
|
[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 |
|
@cryo-zd: 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/domain/plan_replayer.go (1)
585-585: Consider documenting the privilege-bypass field's intent and constraints.The
useExplainAdminBypassfield controls security-sensitive behavior (enabling privilege bypass forEXPLAIN/SHOW CREATEoperations). A brief inline comment would help future readers understand:
- When this field should be set to
true- That it requires the caller to hold
PLAN_REPLAYER_EXPLAIN_ADMINprivilegeThis is especially important since the field is unexported and its purpose isn't immediately obvious from the name alone.
📝 Suggested documentation
ExecStmts []ast.StmtNode Analyze bool + // useExplainAdminBypass, when true, allows internal EXPLAIN/SHOW CREATE + // operations to bypass standard privilege checks. Only set when the + // invoking session holds PLAN_REPLAYER_EXPLAIN_ADMIN dynamic privilege. useExplainAdminBypass bool HistoricalStatsTS uint64As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/domain/plan_replayer.go` at line 585, Add an inline comment above the unexported boolean field useExplainAdminBypass in plan_replayer.go explaining its intent and constraints: note that it enables privilege bypass for EXPLAIN/SHOW CREATE operations, must only be set to true when the caller holds the PLAN_REPLAYER_EXPLAIN_ADMIN privilege, and that it is security-sensitive (describe any invariants such as not changing at runtime or who is responsible for validation). Keep the comment brief and precise so future readers understand when and by whom useExplainAdminBypass may be enabled.
🤖 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/domain/plan_replayer.go`:
- Line 585: Add an inline comment above the unexported boolean field
useExplainAdminBypass in plan_replayer.go explaining its intent and constraints:
note that it enables privilege bypass for EXPLAIN/SHOW CREATE operations, must
only be set to true when the caller holds the PLAN_REPLAYER_EXPLAIN_ADMIN
privilege, and that it is security-sensitive (describe any invariants such as
not changing at runtime or who is responsible for validation). Keep the comment
brief and precise so future readers understand when and by whom
useExplainAdminBypass may be enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: afb1d679-35ee-4603-8a9b-ee0b22894e3e
📒 Files selected for processing (2)
pkg/domain/plan_replayer.gopkg/domain/plan_replayer_dump.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/domain/plan_replayer_dump.go
|
/retest |
2 similar comments
|
/retest |
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: close #67591
Problem Summary:
Cloud needs to run
PLAN REPLAYER DUMP EXPLAIN, but the backend should not use a user with generalSELECTprivileges on user data. This PR adds a standalone dynamic privilege,PLAN_REPLAYER_EXPLAIN_ADMIN, for the plan replayer dump explain flow.What changed and how does it work?
PLAN_REPLAYER_EXPLAIN_ADMINdynamic privilegeEXPLAIN/SHOW CREATEhelper SQLs with a narrow plan-replayer-only privilege bypass when the normal execution hits the expected privilege errorCheck 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
Behavior
Tests