It takes time to broadcast FLUSH PRIVILEGES to all TiDB instances

[aylei]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. What did you do?

MySQL [(none)]> UPDATE mysql.user SET Password=PASSWORD('MY_AWESOME_PASSWORD') WHERE User='root';
Query OK, 1 row affected, 1 warning (0.01 sec)
Rows matched: 1  Changed: 1  Warnings: 1

MySQL [(none)]> FLUSH PRIVILEGES;
Query OK, 0 rows affected (0.02 sec)

MySQL [(none)]> exit;
Bye
admin@ip-172-30-101-35:~$ mysql -h a4c7ba19a3b5b4d1ebb1c9c3b80717cc-8623445e5cc961e7.elb.us-west-2.amazonaws.com -P 4000 -u root
Welcome to the MariaDB monitor.  Commands end with ; or \g.
Your MySQL connection id is 2
Server version: 5.7.25-TiDB-v4.0.0-beta-52-gfa6f1c58c TiDB Server (Apache License 2.0), MySQL 5.7 compatible

Copyright (c) 2000, 2018, Oracle, MariaDB Corporation Ab and others.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

MySQL [(none)]> Ctrl-C -- exit!
Aborted

As showed above, after I changed the root password and execute FLUSH PRIVILEGES, I can still login mysql with the old password for about 3~5 minutes.

2. What did you expect to see?
   As per the document, old password should be invalidated once FLUSH PRIVILEGES succeed.

3. What did you see instead?
   Old password still took effect.

4. What version of TiDB are you using (tidb-server -V or run select tidb_version(); on TiDB)?
   v4.0.0-beta-52-gfa6f1c58c

[morgo]

There are two methods that privilege changes are "broadcast" to other nodes:

1. The privilege loop automatic reloads the on-disk tables every 5-10 minutes.
2. A notification is sent via an etcd channel:

tidb/domain/domain.go

Lines 1307 to 1323 in 7c19975

	func (do *Domain) NotifyUpdatePrivilege(ctx sessionctx.Context) {
	if do.etcdClient != nil {
	row := do.etcdClient.KV
	_, err := row.Put(context.Background(), privilegeKey, "")
	if err != nil {
	logutil.BgLogger().Warn("notify update privilege failed", zap.Error(err))
	}
	}
	// update locally
	exec := ctx.(sqlexec.RestrictedSQLExecutor)
	if stmt, err := exec.ParseWithParams(context.Background(), `FLUSH PRIVILEGES`); err == nil {
	_, _, err := exec.ExecRestrictedStmt(context.Background(), stmt)
	if err != nil {
	logutil.BgLogger().Error("unable to update privileges", zap.Error(err))
	}
	}
	}

We can see that because the code in the notification is running FLUSH PRIVILEGES itself, the second part is obviously not happening on the FLUSH PRIVILEGES statement (as this would cause recursion).

So the code will need a little bit of refactoring so that FLUSH PRIVILEGES can also post to the notification channel.

@aylei as a workaround, you can use the SET PASSWORD FOR root = 'newpassword' statement. It is recommended to use this over manually editing the privilege tables.

[morgo]

Sorry, I didn't realize how old this issue is :-) It still makes sense to fix.

[bb7133]

Can't reproduce this issue. @aylei I will close this issue and please feel free to make a try and reopen it again.

[github-actions]

Please check whether the issue should be labeled with 'affects-x.y' or 'backport-x.y.z',
and then remove 'needs-more-info' label.

[morgo]

From code-inspection, the issue still exists. I think it is worth fixing.

[github-actions]

Please check whether the issue should be labeled with 'affects-x.y' or 'fixes-x.y.z', and then remove 'needs-more-info' label.