parser, executor: consistently lower identity hostnames #33172

morgo · 2022-03-16T18:23:11Z

What problem does this PR solve?

Issue Number: close #33061

Problem Summary:

MySQL always stores the hostname portion of a user identity in lower case (whether it be for a user or a role). It doesn't depend on collations - manual inspection of the tables shows it is lower.

What is changed and how it works?

We have previously implemented this correctly for user grants, and inconsistently for roles. But this is challenging because it requires adjustment in many cases.

This PR takes a different approach where the host portion is lowered in the parser, and then executed in TiDB in lower case. Previous usage of lower casing in the code have been reverted (since it will always be lower).

I have added the tag for compatibility breaker, since it does not modify existing roles or hosts that are in the incorrect case. I considered writing a bootstrap task to fix this (see below) but opted against it because I think it's too destructive to modify user data without their interaction. It should instead be mentioned in the release notes.

--- a/session/bootstrap.go
+++ b/session/bootstrap.go
@@ -583,11 +583,13 @@ const (
        version84 = 84
        // version85 updates bindings with status 'using' in mysql.bind_info table to 'enabled' status
        version85 = 85
+       // version 86 consistently fixes hostnames in privilege tables to lowercase.
+       version86 = 86
 )
 
 // currentBootstrapVersion is defined as a variable, so we can modify its value for testing.
 // please make sure this is the largest version
-var currentBootstrapVersion int64 = version84
+var currentBootstrapVersion int64 = version86
 
 var (
        bootstrapVersion = []func(Session, int64){
@@ -676,6 +678,7 @@ var (
                upgradeToVer83,
                upgradeToVer84,
                upgradeToVer85,
+               upgradeToVer86,
        }
 )
 
@@ -1752,6 +1755,18 @@ func upgradeToVer85(s Session, ver int64) {
        mustExecute(s, fmt.Sprintf("UPDATE HIGH_PRIORITY mysql.bind_info SET status= '%s' WHERE status = '%s'", bindinfo.Enabled, bindinfo.Using))
 }
 
+func upgradeToVer86(s Session, ver int64) {
+       if ver >= version86 {
+               return
+       }
+       mustExecute(s, "UPDATE HIGH_PRIORITY mysql.user SET host = lower(host)")
+       mustExecute(s, "UPDATE HIGH_PRIORITY mysql.db SET host = lower(host)")
+       mustExecute(s, "UPDATE HIGH_PRIORITY mysql.default_roles SET host = lower(host)")
+       mustExecute(s, "UPDATE HIGH_PRIORITY mysql.global_grants SET host = lower(host)")
+       mustExecute(s, "UPDATE HIGH_PRIORITY mysql.global_priv SET host = lower(host)")
+       mustExecute(s, "UPDATE HIGH_PRIORITY mysql.role_edges SET from_host = lower(from_host), to_host = lower(to_host)")
+}

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

TiDB now consistently lower cases the hostname portion of user accounts and roles during statement parsing. This behavior is more consistent with MySQL, but might lead to unexpected behaviors if accounts had previously been created with hostnames using upper case characters.

ti-chi-bot · 2022-03-16T18:48:00Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

dveeden
mjonss

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

sre-bot · 2022-03-16T18:56:56Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/90afc2aea443eb11d83f69d0c25fbf47a21b754e

morgo · 2022-03-22T20:56:48Z

/check-issue-triage-complete

morgo · 2022-03-22T20:58:47Z

/run-unit-test

dveeden · 2022-03-24T16:01:17Z

The list of the tables in upgradeToVer86 in the description might not be complete:

mysql> SELECT TABLE_NAME, COLUMN_NAME FROM information_schema.columns WHERE COLUMN_NAME COLLATE utf8mb4_general_ci LIKE '%host' AND TABLE_SCHEMA='mysql' ORDER BY
TABLE_NAME;
+---------------+-------------------+
| TABLE_NAME    | COLUMN_NAME       |
+---------------+-------------------+
| columns_priv  | Host              |
| db            | Host              |
| default_roles | HOST              |
| default_roles | DEFAULT_ROLE_HOST |
| global_grants | HOST              |
| global_priv   | Host              |
| role_edges    | FROM_HOST         |
| role_edges    | TO_HOST           |
| tables_priv   | Host              |
| user          | Host              |
+---------------+-------------------+
10 rows in set (0.13 sec)

morgo · 2022-04-11T17:05:03Z

The list of the tables in upgradeToVer86 in the description might not be complete:

Yeah, I think that's OK. I said I considered doing this but elected not to since it could create issues modifying user data.

mjonss

Will this also work with prepared statements (both binary and SQL statement interface)?

Also could you give an example of unexpected behaviour?

But still looks good to me :)

tiancaiamao · 2022-04-15T09:58:20Z

/merge

morgo · 2022-05-25T00:31:57Z

/run-mysql-test

morgo · 2022-05-25T01:08:40Z

There's something wrong with the mysql-test. The new result file has been merged to master, but it's expecting the old values:

[2022-05-25T00:35:37.788Z] time="2022-05-25T08:35:33+08:00" level=error msg="run test [role] err: sql:SELECT * FROM mysql.role_edges order by FROM_USER;: failed to run query \n\"SELECT * FROM mysql.role_edges order by FROM_USER;\" \n around line 166, \nwe need(284):\nSELECT * FROM mysql.role_edges order by FROM_USER;\nFROM_HOST\tFROM_USER\tTO_HOST\tTO_USER\tWITH_ADMIN_OPTION\n%\tconsultants\t%\tjoan\tN\n%\tconsultants\t%\tsally\tN\n%\tengineering\t%\tsally\tN\nUS\tengineering\tINDIA\tengineering\tN\n%\tqa\t%\tconsultants\tN\n%\trole\t%\troot\tN\n%\twp_administrators\tlocalhost\tjoe\tN\n\nbut got(284):\nSELECT * FROM mysql.role_edges order by FROM_USER;\nFROM_HOST\tFROM_USER\tTO_HOST\tTO_USER\tWITH_ADMIN_OPTION\n%\tconsultants\t%\tjoan\tN\n%\tconsultants\t%\tsally\tN\n%\tengineering\t%\tsally\tN\nus\tengineering\tindia\tengineering\tN\n%\tqa\t%\tconsultants\tN\n%\trole\t%\troot\tN\n%\twp_administrators\tlocalhost\tjoe\tN\n\n"