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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c367254
session: fix bug user privileges change after upgrade from 4.0.11 to 5.0
tiancaiamao Mar 18, 2021
34d842d
address comment
tiancaiamao Mar 22, 2021
cbe74de
make golint happy
tiancaiamao Mar 22, 2021
e59eb30
make golint happy
tiancaiamao Mar 22, 2021
78795f5
Merge branch 'master' into issue-23387
tiancaiamao Mar 22, 2021
e8f3544
address comment
tiancaiamao Mar 24, 2021
ed96ff8
fix a testleak fail in CI
tiancaiamao Mar 25, 2021
08363bf
Merge branch 'master' into issue-23387
ti-chi-bot Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions session/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,11 +477,14 @@ const (
version66 = 66
// version67 restore all SQL bindings.
version67 = 67

// please make sure this is the largest version
currentBootstrapVersion = version67
)

// currentBootstrapVersion is modified to a function so we can hook its value for testing.
// please make sure this is the largest version
zimulala marked this conversation as resolved.
Show resolved Hide resolved
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

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.

PTAL @tiancaiamao

return version67
}

var (
bootstrapVersion = []func(Session, int64){
upgradeToVer2,
Expand Down Expand Up @@ -612,7 +615,7 @@ func getTiDBVar(s Session, name string) (sVal string, isNull bool, e error) {
func upgrade(s Session) {
ver, err := getBootstrapVersion(s)
terror.MustNil(err)
if ver >= currentBootstrapVersion {
if ver >= currentBootstrapVersion() {
// It is already bootstrapped/upgraded by a higher version TiDB server.
return
}
Expand All @@ -634,13 +637,13 @@ func upgrade(s Session) {
if err1 != nil {
logutil.BgLogger().Fatal("upgrade failed", zap.Error(err1))
}
if v >= currentBootstrapVersion {
if v >= currentBootstrapVersion() {
// It is already bootstrapped/upgraded by a higher version TiDB server.
return
}
logutil.BgLogger().Fatal("[Upgrade] upgrade failed",
zap.Int64("from", ver),
zap.Int("to", currentBootstrapVersion),
zap.Int64("to", currentBootstrapVersion()),
zap.Error(err))
}
}
Expand Down Expand Up @@ -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.

}

func upgradeToVer65(s Session, ver int64) {
Expand Down Expand Up @@ -1471,7 +1474,7 @@ func writeOOMAction(s Session) {
func updateBootstrapVer(s Session) {
// Update bootstrap version.
mustExecute(s, `INSERT HIGH_PRIORITY INTO %n.%n VALUES (%?, %?, "TiDB bootstrap version.") ON DUPLICATE KEY UPDATE VARIABLE_VALUE=%?`,
mysql.SystemDB, mysql.TiDBTable, tidbServerVersionVar, currentBootstrapVersion, currentBootstrapVersion,
mysql.SystemDB, mysql.TiDBTable, tidbServerVersionVar, currentBootstrapVersion(), currentBootstrapVersion(),
)
}

Expand Down Expand Up @@ -1593,7 +1596,7 @@ func doDMLWorks(s Session) {
)

mustExecute(s, `INSERT HIGH_PRIORITY INTO %n.%n VALUES(%?, %?, "Bootstrap version. Do not delete.")`,
mysql.SystemDB, mysql.TiDBTable, tidbServerVersionVar, currentBootstrapVersion,
mysql.SystemDB, mysql.TiDBTable, tidbServerVersionVar, currentBootstrapVersion(),
)

writeSystemTZ(s)
Expand Down
52 changes: 43 additions & 9 deletions session/bootstrap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/sessionctx/variable"
"github.com/pingcap/tidb/statistics"
"github.com/pingcap/tidb/store/mockstore"
"github.com/pingcap/tidb/util/testleak"
)

Expand Down Expand Up @@ -209,13 +210,13 @@ func (s *testBootstrapSuite) TestUpgrade(c *C) {
c.Assert(err, IsNil)
c.Assert(req.NumRows() == 0, IsFalse)
c.Assert(row.Len(), Equals, 1)
c.Assert(row.GetBytes(0), BytesEquals, []byte(fmt.Sprintf("%d", currentBootstrapVersion)))
c.Assert(row.GetBytes(0), BytesEquals, []byte(fmt.Sprintf("%d", currentBootstrapVersion())))
c.Assert(r.Close(), IsNil)

se1 := newSession(c, store, s.dbName)
ver, err := getBootstrapVersion(se1)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())

// Do something to downgrade the store.
// downgrade meta bootstrap version
Expand Down Expand Up @@ -255,12 +256,12 @@ func (s *testBootstrapSuite) TestUpgrade(c *C) {
c.Assert(req.NumRows() == 0, IsFalse)
row = req.GetRow(0)
c.Assert(row.Len(), Equals, 1)
c.Assert(row.GetBytes(0), BytesEquals, []byte(fmt.Sprintf("%d", currentBootstrapVersion)))
c.Assert(row.GetBytes(0), BytesEquals, []byte(fmt.Sprintf("%d", currentBootstrapVersion())))
c.Assert(r.Close(), IsNil)

ver, err = getBootstrapVersion(se2)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())

// Verify that 'new_collation_enabled' is false.
r = mustExecSQL(c, se2, fmt.Sprintf(`SELECT VARIABLE_VALUE from mysql.TiDB where VARIABLE_NAME='%s';`, tidbNewCollationEnabled))
Expand Down Expand Up @@ -307,7 +308,7 @@ func (s *testBootstrapSuite) TestIssue17979_1(c *C) {
seV4 := newSession(c, store, s.dbName)
ver, err = getBootstrapVersion(seV4)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())
r := mustExecSQL(c, seV4, "select variable_value from mysql.tidb where variable_name='default_oom_action'")
req := r.NewChunk()
r.Next(ctx, req)
Expand Down Expand Up @@ -350,7 +351,7 @@ func (s *testBootstrapSuite) TestIssue17979_2(c *C) {
seV4 := newSession(c, store, s.dbName)
ver, err = getBootstrapVersion(seV4)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())
r := mustExecSQL(c, seV4, "select variable_value from mysql.tidb where variable_name='default_oom_action'")
req := r.NewChunk()
r.Next(ctx, req)
Expand Down Expand Up @@ -387,7 +388,7 @@ func (s *testBootstrapSuite) TestIssue20900_1(c *C) {
seV4 := newSession(c, store, s.dbName)
ver, err = getBootstrapVersion(seV4)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())
r := mustExecSQL(c, seV4, "select @@tidb_mem_quota_query")
req := r.NewChunk()
r.Next(ctx, req)
Expand Down Expand Up @@ -428,7 +429,7 @@ func (s *testBootstrapSuite) TestIssue20900_2(c *C) {
seV4 := newSession(c, store, s.dbName)
ver, err = getBootstrapVersion(seV4)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())
r := mustExecSQL(c, seV4, "select @@tidb_mem_quota_query")
req := r.NewChunk()
r.Next(ctx, req)
Expand Down Expand Up @@ -637,7 +638,7 @@ func (s *testBootstrapSuite) TestUpgradeVersion66(c *C) {
seV66 := newSession(c, store, s.dbName)
ver, err = getBootstrapVersion(seV66)
c.Assert(err, IsNil)
c.Assert(ver, Equals, int64(currentBootstrapVersion))
c.Assert(ver, Equals, currentBootstrapVersion())
r := mustExecSQL(c, seV66, `select @@global.tidb_track_aggregate_memory_usage, @@session.tidb_track_aggregate_memory_usage`)
req := r.NewChunk()
c.Assert(r.Next(ctx, req), IsNil)
Expand All @@ -646,3 +647,36 @@ func (s *testBootstrapSuite) TestUpgradeVersion66(c *C) {
c.Assert(row.GetInt64(0), Equals, int64(1))
c.Assert(row.GetInt64(1), Equals, int64(1))
}

func (s *testBootstrapSuite) TestForIssue23387(c *C) {
// For issue https://github.com/pingcap/tidb/issues/23387
saveCurrentBootstrapVersion := currentBootstrapVersion
currentBootstrapVersion = func() int64 {
return version57
}

// Bootstrap to an old version, create a user.
store, err := mockstore.NewMockStore()
c.Assert(err, IsNil)
_, err = BootstrapSession(store)
// domain leaked here, Close() is not called. For testing, it's OK.
// If we close it and BootstrapSession again, we'll get an error "session pool is closed".
// The problem is caused by some the global level variable, domain map is not intended for multiple instances.
c.Assert(err, IsNil)

se := newSession(c, store, s.dbName)
mustExecSQL(c, se, "create user quatest")

// Upgrade to a newer version, check the user's privilege.
currentBootstrapVersion = saveCurrentBootstrapVersion
_, err = BootstrapSession(store)
c.Assert(err, IsNil)

se = newSession(c, store, s.dbName)
rs, err := exec(se, "show grants for quatest")
c.Assert(err, IsNil)
rows, err := ResultSetToStringSlice(context.Background(), se, rs)
c.Assert(err, IsNil)
c.Assert(len(rows), Equals, 1)
c.Assert(rows[0][0], Equals, "GRANT USAGE ON *.* TO 'quatest'@'%'")
}
6 changes: 3 additions & 3 deletions session/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2144,7 +2144,7 @@ func BootstrapSession(store kv.Storage) (*domain.Domain, error) {
ver := getStoreBootstrapVersion(store)
if ver == notBootstrapped {
runInBootstrapSession(store, bootstrap)
} else if ver < currentBootstrapVersion {
} else if ver < currentBootstrapVersion() {
runInBootstrapSession(store, upgrade)
}

Expand Down Expand Up @@ -2355,7 +2355,7 @@ func getStoreBootstrapVersion(store kv.Storage) int64 {
// check in memory
_, ok := storeBootstrapped[store.UUID()]
if ok {
return currentBootstrapVersion
return currentBootstrapVersion()
}

var ver int64
Expand Down Expand Up @@ -2385,7 +2385,7 @@ func finishBootstrap(store kv.Storage) {

err := kv.RunInNewTxn(context.Background(), store, true, func(ctx context.Context, txn kv.Transaction) error {
t := meta.NewMeta(txn)
err := t.FinishBootstrap(currentBootstrapVersion)
err := t.FinishBootstrap(currentBootstrapVersion())
return err
})
if err != nil {
Expand Down