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

cli, sql: allow usernames to start with digits #42464

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

georgebuckerfield
Copy link
Contributor

Allows CockroachDB usernames to start with digits.

Fixes #42155.

e.g.

root@:26257/defaultdb> create user '1234';
CREATE USER 1

Time: 6.698583ms

@georgebuckerfield georgebuckerfield requested a review from a team as a code owner November 13, 2019 22:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz requested review from mberhault and bdarnell November 14, 2019 07:30
@knz
Copy link
Contributor

knz commented Nov 14, 2019

@georgebuckerfield thanks for your contribution!
Note: you forgot two update two tests for the new error message.

@bdarnell @mberhault @mjibson please check whether this new username format is compatible with the various other things we care about: Kerberos auth, TLS certs, CockroachCloud auth, etc.

@georgebuckerfield
Copy link
Contributor Author

Thanks @knz - just pushed a fix for those failing tests.

@mberhault
Copy link
Contributor

That's fine for x509 certs, you can generally do UTF8 up to ridiculous lenghts. CC doesn't have any particular requirements on the SQL username.

Previousy, client usernames could only be valid identifiers, with the
first character allowed being either a letter of underscore.

This was an arbitrary limitation; in fact, production systems
where usernames are just numbers (all digits) are common.

This patch changes the limitation to allow CockroachDB usernames to
start with digits.

Release note (general change): Client usernames can now be defined to
start with a digit; in particular, all-digit usernames are now
permitted.
@knz
Copy link
Contributor

knz commented Nov 14, 2019

I'm going to cautiously green light this change, even if it happens to not be compatible with some subsystem. I'd rather document known limitations (and later fix them) than prevent integration with systems where numeric usernames are prevalent.

@georgebuckerfield thank you again for your change.

bors r+

craig bot pushed a commit that referenced this pull request Nov 14, 2019
42464: cli, sql: allow usernames to start with digits r=knz a=georgebuckerfield

Allows CockroachDB usernames to start with digits.

Fixes #42155.

e.g.
```
root@:26257/defaultdb> create user '1234';
CREATE USER 1

Time: 6.698583ms
```

Co-authored-by: georgebuckerfield <[email protected]>
@knz
Copy link
Contributor

knz commented Nov 14, 2019

@georgebuckerfield just a small note: I have amended your branch to make the commit comply with our general style and requirements:

  • squash commits that pertain to a single unit of change
  • include a before/after explanation in the commit message
  • include a release note for user-facing changes

If you intend to contribute more to CockroachDB, I recommend you to consult the git commit template produced by default upon git commit, as well as https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages and https://wiki.crdb.io/wiki/spaces/CRDB/pages/186548364/Release+notes

Best regards

@georgebuckerfield
Copy link
Contributor Author

Thanks @knz! Sorry, I was waiting to see if there were any changes needed before squashing the commits. And noted re: the commit message.

@knz
Copy link
Contributor

knz commented Nov 14, 2019

Sorry, I was waiting to see if there were any changes needed before squashing the commits.

Don't be sorry! I understood about the same. But since I don't recall you have a permission to merge yet there was no clear synchronization point for you to do this after the review and the CI results came positive.

@craig
Copy link
Contributor

craig bot commented Nov 14, 2019

Build succeeded

@craig craig bot merged commit 3009054 into cockroachdb:master Nov 14, 2019
@bdarnell
Copy link
Contributor

I'm going to cautiously green light this change, even if it happens to not be compatible with some subsystem. I'd rather document known limitations (and later fix them) than prevent integration with systems where numeric usernames are prevalent.

I agree with this. If names starting with digits don't work in some integrations, we can just document that and move on. (But note that the reason this request came up was for integration with a kerberos system that already contains numeric usernames, so we know that kerberos/gssapi already supports this in general)

One thing worth testing: names starting with digits generally have to be quoted when used in SQL (for example in GRANT statements). We should make sure we have tests that numeric user names work as expected once created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli, sql, ui: allow all-numeric usernames
5 participants