Skip to content

Support privileges coming from roles#659

Merged
morgo merged 14 commits into
block:mainfrom
morgo:mtocker-allow-roles
Mar 15, 2026
Merged

Support privileges coming from roles#659
morgo merged 14 commits into
block:mainfrom
morgo:mtocker-allow-roles

Conversation

@morgo
Copy link
Copy Markdown
Collaborator

@morgo morgo commented Mar 13, 2026

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.
Further notes in https://github.com/block/spirit/blob/main/.github/CONTRIBUTING.md

This is an alternative fix for #658

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds role-aware privilege handling so Spirit can operate on managed MySQL services (e.g., RDS) where capabilities like CONNECTION_ADMIN, PROCESS, or LOCK TABLES may be granted via roles that aren’t enabled by default.

Changes:

  • Adds/extends privilege checks (migration + move) to detect privileges granted through roles via SET ROLE ALL.
  • Introduces dbconn.SetRoleAllOnTxn plus tests demonstrating that role state leaks across pooled connections unless explicitly reset.
  • Updates several dbconn operations to activate roles on a dedicated transaction before querying performance_schema / executing KILL / acquiring locks.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
pkg/move/check/privileges.go New move preflight privilege check, including role-aware checks for CONNECTION_ADMIN/PROCESS.
pkg/move/check/privileges_test.go Adds coverage for move privilege requirements and role-granted privileges.
pkg/migration/check/privileges.go Refactors grant scanning and adds role-aware checks for CONNECTION_ADMIN/PROCESS.
pkg/migration/check/privileges_test.go Adds test ensuring role-granted privileges are detected.
pkg/dbconn/role.go Adds helper to enable roles on a txn and return a cleanup to reset session role state.
pkg/dbconn/role_test.go Adds tests proving SET ROLE leaks across tx boundaries and cleanup is required.
pkg/dbconn/kill.go Executes performance_schema queries and KILL under role-activated transactions.
pkg/dbconn/tablelock.go Activates roles for LOCK TABLES and introduces cleanup hooks on lock close.
pkg/dbconn/dbconn.go Activates roles for ForceExec operations executed within a transaction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/dbconn/kill.go Outdated
Comment thread pkg/dbconn/tablelock.go
Comment thread pkg/dbconn/role.go Outdated
Comment thread pkg/move/check/privileges.go Outdated
Comment thread pkg/move/check/privileges.go Outdated
Comment thread pkg/migration/check/privileges.go Outdated
Comment thread pkg/migration/check/privileges.go Outdated
morgo and others added 3 commits March 13, 2026 06:18
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds role-aware privilege handling so Spirit’s preflight checks and force-kill related operations work when privileges are inherited via MySQL roles (common on managed services like RDS).

Changes:

  • Introduce dbconn.SetRoleAllOnTxn and apply it to transactions that need role-inherited privileges.
  • Update migration/move privilege checks to detect CONNECTION_ADMIN / PROCESS via roles, plus add role-based tests.
  • Run performance_schema queries and KILL operations on a dedicated transaction with roles activated.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pkg/move/check/privileges.go Adds move preflight privilege check with role-aware detection for CONNECTION_ADMIN/PROCESS.
pkg/move/check/privileges_test.go Adds move privilege tests, including role-granted privilege coverage.
pkg/migration/check/privileges.go Refactors grant scanning and adds role-aware checks for CONNECTION_ADMIN/PROCESS.
pkg/migration/check/privileges_test.go Adds migration privilege test validating role-granted privileges are accepted.
pkg/dbconn/role.go Adds helper to SET ROLE ALL on a transaction and return a reset cleanup.
pkg/dbconn/role_test.go Adds tests proving role state can leak across pooled connections and that cleanup prevents it.
pkg/dbconn/tablelock.go Activates roles for table-lock transaction and attempts to reset roles on close.
pkg/dbconn/kill.go Runs performance_schema queries and KILL under a role-activated transaction; updates KillTransaction signature.
pkg/dbconn/dbconn.go Activates roles for ForceExec transaction before executing statements.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/move/check/privileges_test.go
Comment thread pkg/move/check/privileges.go
Comment thread pkg/migration/check/privileges.go
Comment thread pkg/move/check/privileges.go
Comment thread pkg/dbconn/tablelock.go
Comment thread pkg/dbconn/role.go
morgo and others added 3 commits March 13, 2026 06:41
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds role-aware privilege handling across Spirit’s migration/move preflight checks and operational DB routines, addressing environments (e.g., RDS) where required privileges are inherited via roles that are not enabled by default.

Changes:

  • Introduce dbconn.SetRoleAllOnTxn (+ tests) and apply it in key transactional code paths to activate role-granted privileges.
  • Update privilege checks (migration + move) and add integration tests validating detection of privileges granted via roles.
  • Adjust force-kill queries/operations (performance_schema queries, KILL, LOCK TABLES, ForceExec) to run with roles activated on a pinned connection.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pkg/move/check/privileges.go Adds move privilege preflight check with role-aware validation for force-kill related privileges.
pkg/move/check/privileges_test.go Adds integration tests for move privilege checks, including role-granted privileges.
pkg/migration/check/privileges.go Refactors grant scanning and adds role-aware fallback for CONNECTION_ADMIN/PROCESS checks.
pkg/migration/check/privileges_test.go Adds integration test ensuring role-granted privileges satisfy migration preflight.
pkg/dbconn/role.go Adds helper to SET ROLE ALL on a txn and return a cleanup to reset role state.
pkg/dbconn/role_test.go Adds tests proving role state leaks across tx boundaries and cleanup requirements.
pkg/dbconn/kill.go Ensures performance_schema queries and KILL run with roles activated on a dedicated connection.
pkg/dbconn/tablelock.go Activates roles for LOCK TABLES txn and resets roles during lock Close().
pkg/dbconn/dbconn.go Activates roles for ForceExec transactions so DDL/force-kill works with role-granted privileges.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/dbconn/role.go
Comment thread pkg/move/check/privileges.go Outdated
Comment thread pkg/migration/check/privileges.go Outdated
Comment thread pkg/move/check/privileges_test.go
Comment thread pkg/migration/check/privileges_test.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds role-aware privilege handling so preflight checks and force-kill / performance_schema operations work when required privileges are granted via MySQL roles (common in managed MySQL environments).

Changes:

  • Introduce dbconn.SetRoleAllOnTxn() helper to activate roles on a pinned connection and reset role state before returning connections to the pool.
  • Update force-kill related dbconn paths (lock inspection, killing, ForceExec, table locks) to run with roles activated.
  • Add/extend privilege-check test coverage for role-granted privileges in both migration and move workflows.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/move/check/privileges.go New move preflight privilege check, including role-aware checks for force-kill requirements.
pkg/move/check/privileges_test.go New tests for move privilege validation, including role-granted privileges.
pkg/migration/check/privileges.go Refactors grant scanning and adds role-aware checks for CONNECTION_ADMIN/SUPER and PROCESS.
pkg/migration/check/privileges_test.go Adds role-based privilege test and ensures DB handles are closed.
pkg/dbconn/role.go New helper to SET ROLE ALL on a txn and return a cleanup to reset roles.
pkg/dbconn/role_test.go Tests role activation and validates session-state leakage + cleanup behavior.
pkg/dbconn/kill.go Ensures performance_schema queries and KILL run on a txn with roles activated; updates KillTransaction signature.
pkg/dbconn/dbconn.go Activates roles in ForceExec transaction to support role-granted DDL privileges.
pkg/dbconn/tablelock.go Activates roles for the lifetime of the lock txn and resets roles on close to avoid pool leakage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/dbconn/role.go
@morgo morgo enabled auto-merge March 15, 2026 22:18
@morgo morgo merged commit 21e58fb into block:main Mar 15, 2026
10 checks passed
@morgo morgo mentioned this pull request Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants