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

tiancaiamao · 2021-03-18T09:04:31Z

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.

Fix for issue pingcap#23387

morgo · 2021-03-22T02:05:11Z

/LGTM

zz-jason

could you add some tests?

tiancaiamao · 2021-03-23T01:16:01Z

could you add some tests?

Done

djshow832 · 2021-03-23T08:19:23Z

session/bootstrap.go

 )

+// 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 {


Why not define it as a non-const variable?

djshow832 · 2021-03-23T08:23:50Z

session/bootstrap.go

@@ -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'")


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

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.

xhebox

Rest LGTM

xhebox · 2021-03-23T08:23:34Z

session/bootstrap.go

 )

+// 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 {


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

PTAL @tiancaiamao

session/bootstrap.go

zimulala · 2021-03-24T08:46:43Z

/LGTM

ti-chi-bot · 2021-03-24T08:46:45Z

[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.

zimulala · 2021-03-24T08:48:41Z

/merge

tiancaiamao · 2021-03-24T09:50:14Z

/merge

tiancaiamao · 2021-03-25T00:56:15Z

/merge

tiancaiamao · 2021-03-25T02:21:54Z

/merge

xhebox · 2021-03-25T02:32:50Z

/merge cancel

xhebox · 2021-03-25T02:32:57Z

/merge

ti-chi-bot · 2021-03-25T02:33:00Z

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

Commit hash: bc1b873f2dea600b96d6e5338877b0a87c708be6

xhebox · 2021-03-25T04:50:29Z

/merge

ti-chi-bot · 2021-03-25T04:50:32Z

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

Commit hash: ed96ff8

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-03-25T05:08:13Z

cherry pick to release-5.0 in PR #23527

…5.0 (#23403) (#23527)

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

c367254

Fix for issue pingcap#23387

ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 18, 2021

tiancaiamao requested review from wjhuang2016 and bb7133 March 18, 2021 09:07

tiancaiamao added the type/bugfix This PR fixes a bug. label Mar 18, 2021

github-actions bot added the sig/sql-infra SIG: SQL Infra label Mar 18, 2021

ichn-hu mentioned this pull request Mar 18, 2021

Welcome to contribute #20804

Closed

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

zz-jason reviewed Mar 22, 2021

View reviewed changes

address comment

34d842d

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 added 3 commits March 22, 2021 20:03

make golint happy

cbe74de

make golint happy

e59eb30

Merge branch 'master' into issue-23387

78795f5

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

djshow832 reviewed Mar 23, 2021

View reviewed changes

xhebox reviewed Mar 23, 2021

View reviewed changes

zimulala added the needs-cherry-pick-release-5.0 label Mar 24, 2021

address comment

e8f3544

zimulala reviewed Mar 24, 2021

View reviewed changes

session/bootstrap.go Show resolved Hide resolved

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

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

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

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

fix a testleak fail in CI

ed96ff8

tiancaiamao force-pushed the issue-23387 branch from 12713a3 to ed96ff8 Compare March 25, 2021 04:17

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

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

Merge branch 'master' into issue-23387

08363bf

ti-chi-bot merged commit 5a4d894 into pingcap:master Mar 25, 2021

ti-srebot mentioned this pull request Mar 25, 2021

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

Merged

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 25, 2021

cherry pick pingcap#23403 to release-5.0

387541c

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-chi-bot pushed a commit that referenced this pull request Mar 25, 2021

session: fix bug user privileges change after upgrade from 4.0.11 to …

de9f188

…5.0 (#23403) (#23527)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tiancaiamao commented Mar 18, 2021 •

edited

Loading

morgo commented Mar 22, 2021

zz-jason left a comment

tiancaiamao commented Mar 23, 2021

djshow832 Mar 23, 2021

tiancaiamao Mar 24, 2021

djshow832 Mar 23, 2021

xhebox Mar 23, 2021

xhebox left a comment

xhebox Mar 23, 2021

zimulala Mar 24, 2021

zimulala commented Mar 24, 2021

ti-chi-bot commented Mar 24, 2021

zimulala commented Mar 24, 2021

tiancaiamao commented Mar 24, 2021

tiancaiamao commented Mar 25, 2021

tiancaiamao commented Mar 25, 2021

xhebox commented Mar 25, 2021

xhebox commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

xhebox commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

ti-srebot commented Mar 25, 2021

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

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

Conversation

tiancaiamao commented Mar 18, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

morgo commented Mar 22, 2021

zz-jason left a comment

Choose a reason for hiding this comment

tiancaiamao commented Mar 23, 2021

djshow832 Mar 23, 2021

Choose a reason for hiding this comment

tiancaiamao Mar 24, 2021

Choose a reason for hiding this comment

djshow832 Mar 23, 2021

Choose a reason for hiding this comment

xhebox Mar 23, 2021

Choose a reason for hiding this comment

xhebox left a comment

Choose a reason for hiding this comment

xhebox Mar 23, 2021

Choose a reason for hiding this comment

zimulala Mar 24, 2021

Choose a reason for hiding this comment

zimulala commented Mar 24, 2021

ti-chi-bot commented Mar 24, 2021

zimulala commented Mar 24, 2021

tiancaiamao commented Mar 24, 2021

tiancaiamao commented Mar 25, 2021

tiancaiamao commented Mar 25, 2021

xhebox commented Mar 25, 2021

xhebox commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

xhebox commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

ti-srebot commented Mar 25, 2021

tiancaiamao commented Mar 18, 2021 •

edited

Loading