Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

session: fix bug user privileges change after upgrade from 4.0.11 to 5.0 #23403

Merged
merged 8 commits into from
Mar 25, 2021

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Mar 18, 2021

What problem does this PR solve?

Issue Number: close #23387

Problem Summary:

#21856 introduce the problem, it adds Repl_slave_priv to all existing users while it should just do that for the root user.

What is changed and how it works?

What's Changed:

Just assign the newly introduced privileges to the root user, rather than all existing users.

How it Works:

Make the behavior more acceptable when upgrading.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

I try to do the unit test but failed.
I thought I can use the initial versioned data, and upgrade the version one by one.
But the upgrading process failed, which means when we upgrade a very old TiDB cluster to the newest one, it may not succeed.

+func (s *testBootstrapSuite) TestAddPrivilegeColumn(c *C) {
+       // For issue https://github.com/pingcap/tidb/issues/23387
+       store, err := mockstore.NewMockStore()
+       c.Assert(err, IsNil)
+       defer store.Close()
+
+       se, err := createSession(store)
+       c.Assert(err, IsNil)
+       se.SetValue(sessionctx.Initing, true)
+       bootstrapToV1(se)
+       se.ClearValue(sessionctx.Initing)
+
+       // upgradeToCurrentBootstrapVersion(se, 33)
+
+
+       // se1, err := CreateSession4Test(store)
+       // mustExecSQL(c, se1, "use test")
+       // mustExecSQL(c, se1, "create user 'qatest'")
+       // mustExecSQL(c, se1, "grant Create on *.* to qatest")
+
+
+       // upgradeToCurrentBootstrapVersion(se, 44)
+       // upgradeToCurrentBootstrapVersion(se, 55)
+
+       // rs := mustExecSQL(c, se1, "show grants for qatest")
+       // _, err = ResultSetToStringSlice(context.Background(), se1, rs)
+       // c.Assert(err, IsNil)
+       // tk.c.Check(errors.ErrorStack(err), check.Equals, "", comment)
+       // return &Result{rows: sRows, c: tk.c, comment: comment}
+
+       upgradeToCurrentBootstrapVersion(se, 67)
+}
+
+func bootstrapToV1(s Session) {
+       const (
+               // CreateUserTable is the SQL statement creates User table in system db.
+               CreateUserTableV1 = `CREATE TABLE if not exists mysql.user (
+               Host                    CHAR(64),
+               User                    CHAR(16),
+               Password                CHAR(41),
+               Select_priv             ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Insert_priv             ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Update_priv             ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Delete_priv             ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Create_priv             ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Drop_priv               ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Grant_priv              ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Alter_priv              ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Show_db_priv            ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Execute_priv            ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Index_priv              ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               Create_user_priv        ENUM('N','Y') NOT NULL  DEFAULT 'N',
+               PRIMARY KEY (Host, User));`
+               // CreateDBPrivTable is the SQL statement creates DB scope privilege table in system db.
+               CreateDBPrivTableV1 = `CREATE TABLE if not exists mysql.db (
+               Host            CHAR(60),
+               DB              CHAR(64),
+               User            CHAR(16),
+               Select_priv     ENUM('N','Y') Not Null  DEFAULT 'N',
+               Insert_priv     ENUM('N','Y') Not Null  DEFAULT 'N',
+               Update_priv     ENUM('N','Y') Not Null  DEFAULT 'N',
+               Delete_priv     ENUM('N','Y') Not Null  DEFAULT 'N',
+               Create_priv     ENUM('N','Y') Not Null  DEFAULT 'N',
+               Drop_priv       ENUM('N','Y') Not Null  DEFAULT 'N',
+               Grant_priv      ENUM('N','Y') Not Null  DEFAULT 'N',
+               Index_priv      ENUM('N','Y') Not Null  DEFAULT 'N',
+               Alter_priv      ENUM('N','Y') Not Null  DEFAULT 'N',
+               Execute_priv    ENUM('N','Y') Not Null  DEFAULT 'N',
+               PRIMARY KEY (Host, DB, User));`
+               // CreateTablePrivTable is the SQL statement creates table scope privilege table in system db.
+               CreateTablePrivTableV1 = `CREATE TABLE if not exists mysql.tables_priv (
+               Host            CHAR(60),
+               DB              CHAR(64),
+               User            CHAR(16),
+               Table_name      CHAR(64),
+               Grantor         CHAR(77),
+               Timestamp       Timestamp DEFAULT CURRENT_TIMESTAMP,
+               Table_priv      SET('Select','Insert','Update','Delete','Create','Drop','Grant', 'Index','Alter'),
+               Column_priv     SET('Select','Insert','Update'),
+               PRIMARY KEY (Host, DB, User, Table_name));`
+               // CreateColumnPrivTable is the SQL statement creates column scope privilege table in system db.
+               CreateColumnPrivTableV1 = `CREATE TABLE if not exists mysql.columns_priv(
+               Host            CHAR(60),
+               DB              CHAR(64),
+               User            CHAR(16),
+               Table_name      CHAR(64),
+               Column_name     CHAR(64),
+               Timestamp       Timestamp DEFAULT CURRENT_TIMESTAMP,
+               Column_priv     SET('Select','Insert','Update'),
+               PRIMARY KEY (Host, DB, User, Table_name, Column_name));`
+               // CreateGloablVariablesTable is the SQL statement creates global variable table in system db.
+               // TODO: MySQL puts GLOBAL_VARIABLES table in INFORMATION_SCHEMA db.
+               // INFORMATION_SCHEMA is a virtual db in TiDB. So we put this table in system db.
+               // Maybe we will put it back to INFORMATION_SCHEMA.
+               CreateGloablVariablesTableV1 = `CREATE TABLE if not exists mysql.GLOBAL_VARIABLES(
+               VARIABLE_NAME  VARCHAR(64) Not Null PRIMARY KEY,
+               VARIABLE_VALUE VARCHAR(1024) DEFAULT Null);`
+               // CreateTiDBTable is the SQL statement creates a table in system db.
+               // This table is a key-value struct contains some information used by TiDB.
+               // Currently we only put bootstrapped in it which indicates if the system is already bootstrapped.
+               CreateTiDBTableV1 = `CREATE TABLE if not exists mysql.tidb(
+               VARIABLE_NAME  VARCHAR(64) Not Null PRIMARY KEY,
+               VARIABLE_VALUE VARCHAR(1024) DEFAULT Null,
+               COMMENT VARCHAR(1024));`
+
+               // CreateHelpTopic is the SQL statement creates help_topic table in system db.
+               // See: https://dev.mysql.com/doc/refman/5.5/en/system-database.html#system-database-help-tables
+               CreateHelpTopicV1 = `CREATE TABLE if not exists mysql.help_topic (
+               help_topic_id int(10) unsigned NOT NULL,
+               name char(64) NOT NULL,
+               help_category_id smallint(5) unsigned NOT NULL,
+               description text NOT NULL,
+               example text NOT NULL,
+               url text NOT NULL,
+               PRIMARY KEY (help_topic_id),
+               UNIQUE KEY name (name)
+               ) ENGINE=InnoDB DEFAULT CHARSET=utf8 STATS_PERSISTENT=0 COMMENT='help topics';`
+       )
+
+       mustExecute(s, "CREATE DATABASE IF NOT EXISTS test")
+       mustExecute(s, fmt.Sprintf("CREATE DATABASE IF NOT EXISTS %s;", mysql.SystemDB))
+       mustExecute(s, CreateUserTableV1)
+       mustExecute(s, CreateDBPrivTableV1)
+       mustExecute(s, CreateTablePrivTableV1)
+       mustExecute(s, CreateColumnPrivTableV1)
+       mustExecute(s, CreateGloablVariablesTableV1)
+       mustExecute(s, CreateTiDBTableV1)
+       mustExecute(s, CreateHelpTopicV1)
+
+       mustExecute(s, "BEGIN")
+       mustExecute(s, `INSERT INTO mysql.user VALUES ("%", "root", "", "Y", "Y", "Y", "Y", "Y", "Y", "Y", "Y", "Y", "Y", "Y", "Y")`)
+       mustExecute(s, `INSERT INTO mysql.tidb VALUES('bootstrapped', 'True', 'Bootstrap flag. Do not delete.')`)
+       mustExecute(s, "COMMIT")
+}

log.txt

Side effects

This just fix the code, if a user upgrades his cluster and meets the bug, he have to fix the data manually.

Release note

  • Fix a bug when upgrading from 4.X to 5.0, the existing users obtain REPLICATION CLIENT and REPLICATION privileges unexpectedly.

@ti-chi-bot ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 18, 2021
@tiancaiamao tiancaiamao added the type/bugfix This PR fixes a bug. label Mar 18, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Mar 18, 2021
@ichn-hu ichn-hu mentioned this pull request Mar 18, 2021
@morgo
Copy link
Contributor

morgo commented Mar 22, 2021

/LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 22, 2021
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

could you add some tests?

@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 22, 2021
@tiancaiamao
Copy link
Contributor Author

could you add some tests?

Done

@zimulala zimulala added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Mar 23, 2021
)

// currentBootstrapVersion is modified to a function so we can hook its value for testing.
// please make sure this is the largest version
var currentBootstrapVersion = func() int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not define it as a non-const variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1443,7 +1446,7 @@ func upgradeToVer64(s Session, ver int64) {
}
doReentrantDDL(s, "ALTER TABLE mysql.user ADD COLUMN `Repl_slave_priv` ENUM('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N' AFTER `Execute_priv`", infoschema.ErrColumnExists)
doReentrantDDL(s, "ALTER TABLE mysql.user ADD COLUMN `Repl_client_priv` ENUM('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N' AFTER `Repl_slave_priv`", infoschema.ErrColumnExists)
mustExecute(s, "UPDATE HIGH_PRIORITY mysql.user SET Repl_slave_priv='Y',Repl_client_priv='Y'")
mustExecute(s, "UPDATE HIGH_PRIORITY mysql.user SET Repl_slave_priv='Y',Repl_client_priv='Y' where Super_priv='Y'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do any operations rely on these privileges? I'm afraid it will break compatibility if you shrink the privilege.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the description has clarified the case:

it adds Repl_slave_priv to all existing users while it should just do that for the root user.

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

)

// currentBootstrapVersion is modified to a function so we can hook its value for testing.
// please make sure this is the largest version
var currentBootstrapVersion = func() int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can not we use failpoint instead of making it a variable? It is always bad to have global variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zimulala
Copy link
Contributor

/LGTM

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • morgo
  • zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 24, 2021
@zimulala
Copy link
Contributor

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2021
@tiancaiamao
Copy link
Contributor Author

/merge

@tiancaiamao
Copy link
Contributor Author

/merge

1 similar comment
@tiancaiamao
Copy link
Contributor Author

/merge

@xhebox
Copy link
Contributor

xhebox commented Mar 25, 2021

/merge cancel

@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021
@xhebox
Copy link
Contributor

xhebox commented Mar 25, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: bc1b873f2dea600b96d6e5338877b0a87c708be6

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021
@xhebox
Copy link
Contributor

xhebox commented Mar 25, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: ed96ff8

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021
@ti-chi-bot ti-chi-bot merged commit 5a4d894 into pingcap:master Mar 25, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 25, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #23527

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-5.0 priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v4.0.11 upgrade to v5.0.0, user privileges changed
8 participants