sql/parser: use RoleSpec for ALTER ROLE by rafiss · Pull Request #71498 · cockroachdb/cockroach

rafiss · 2021-10-12T23:06:34Z

Release note (sql change): CURRENT_USER and SESSION_USER can now be used
as the role identifier in ALTER ROLE statements.

Release note (backward-incompatible change): Placeholder values (e.g. $1)
can no longer be used for role names in ALTER ROLE statements.

Release note (backward-incompatible change): Placeholders (e.g. $1)
can no longer be used for role names in CREATE/DROP ROLE statements.

cockroach-teamcity · 2021-10-12T23:06:42Z

This change is

RichardJCai

LGTM with minor nit

Release note (sql change): CURRENT_USER and SESSION_USER can now be used as the role identifier in ALTER ROLE statements. Release note (backward-incompatible change): Placeholder values (e.g. `$1`) can no longer be used for role names in ALTER ROLE statements.

This is done mostly so that all of our "role" syntax is consistent, so that in the future we don't accidentally forget to use RoleSpec since somebody refers to the wrong implementation. Release note (backward-incompatible change): Placeholders (e.g. `$1`) can no longer be used for role names in CREATE/DROP ROLE statements.

All username normalization now happens through RoleSpec. This removes the special logic in create_role that was validating usernames. Release note: None

rafiss · 2021-10-14T05:50:58Z

this is ready for another look -- i added a refactor commit that moves the username validation, so easier to review this separately

also @RichardJCai you said you had a nit but i didn't see the comment

RichardJCai · 2021-10-14T16:09:47Z

pkg/sql/alter_role.go

-		return err
-	}
-	if name == "" {
+	if n.roleName.Normalized() == "" {


Sorry my reviewable comment didn't go through for some reason. My nit is that you can use IsUndefined() here I think.

RichardJCai

Reviewed 5 of 11 files at r1, 17 of 23 files at r2, 20 of 22 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/alter_role.go, line 543 at r4 (raw file):

		return true, security.MakeSQLUsernameFromPreNormalizedString(""), nil
	}
	if n.roleName.Normalized() == "" {

Can use IsUndefined()

Release note: None

rafiss

tftr!

bors r=RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/sql/alter_role.go, line 151 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Sorry my reviewable comment didn't go through for some reason. My nit is that you can use IsUndefined() here I think.

fixed

craig · 2021-10-14T17:46:11Z

Build succeeded:

GitHub CI (Cockroach)

andy-kimball · 2021-12-07T22:42:09Z

@rafiss, what was the reason to break backwards-compatibility here? This broke CC and will probably break some customers as well. I assume there was some really compelling reason why we had to do this?

rafiss · 2021-12-08T01:26:10Z

Huh, it's a bit surprising that CC is using a pre-alpha version of CockroachDB. I heard about the CC issue -- but in a normal upgrade this change would be evident in backward-incompatible change release notes.

This change was done to support the CURRENT_USER and SESSION_USER aliases in the CREATE/ALTER ROLE commands. They are special syntax keywords which are handled within the parser. It matches the Postgres syntax so we made this change to get some improved compatibility with tooling and to support a useful feature.

Theoretically we could still support placeholders for this with a bit of extra work, but here was my rationale for not doing so:

It aligns more closely with how every other identifier in SQL works. For example, you can't use a placeholder for a table name.
The way our parser handles usernames is a bit of a mess in versions 21.2 and earlier. It inconsistently would treat usernames as string constants, or as identifiers, or as generic expressions. Given that placeholders were already not allowed for usernames in many other contexts, I thought this would be OK.
We never documented that we do accept placeholders for the username.
Allowing placeholders here is a CRDB-specific invention, as far as I can tell.

With all of the above, I was almost getting to the point where I thought it was an accident that we ever did allow placeholders here. I'd honestly be pretty surprised if many customers were relying on this. There is somewhat compelling evidence for this not being widely used in our telemetry data -- let me know if you'd like me to share that with you separately.

Additionally, I'm working on a little bit of guidance for when backwards-incompatible changes are OK. There are quite a few other SQL bugfixes we'd like to do, but the fix itself would have incompatibility implications. @jordanlewis and I have been thinking of multi-dimensional criteria including "ease of discovering the change," "ease of fixing the incompatibility," and "severity of not accounting for the change."

This change was made before we discussed this kind of guidance, but to apply that framework:

this change is easy to discover, since the error message tells you what part of the statement isn't working;
it's easy to fix, since you just need to stop using placeholders;
and the severity seems medium, since it's unlikely to break any business logic (more likely would affect tests and operator-initiated setup).

All that being said, if these arguments are not swaying you, let me know, and I can spend a bit of time trying to re-add placeholder support here.

andy-kimball · 2021-12-08T14:41:26Z

Thanks for the detailed justification. It sounds like your team has thought this through carefully, which was the main thing I wanted to verify. I'm content to let your team own the decision.

This commit contains the non-trivial changes required to backport cockroachdb#72579. server.user_login.store_client_pre_hashed_passwords.enabled's default value was changed to false. Changing the default required updating some of the tests to clear and set the setting. The logic tests were adjusted to account for the lack of cockroachdb#71498. cockroachdb#71498 changed the AST representation of the role name to an identifier. This is mostly transparent to users, but it changes the normalized representation. Release note (sql change): This backports the server.user_login.store_client_pre_hashed_passwords.enabled setting. The backported default value is false. In 22.1 the default is true.

rafiss requested review from a team and RichardJCai October 12, 2021 23:06

RichardJCai approved these changes Oct 12, 2021

View reviewed changes

rafiss force-pushed the alter-role-rolespec branch 4 times, most recently from d3a0500 to 361894f Compare October 13, 2021 20:57

rafiss requested review from a team as code owners October 13, 2021 20:57

rafiss requested review from stevendanna and removed request for a team October 13, 2021 20:57

rafiss force-pushed the alter-role-rolespec branch 4 times, most recently from ab63652 to 6c103fd Compare October 14, 2021 03:05

rafiss added 3 commits October 14, 2021 00:05

sql: consolidate username normalization in one place

d2fc2a3

All username normalization now happens through RoleSpec. This removes the special logic in create_role that was validating usernames. Release note: None

rafiss force-pushed the alter-role-rolespec branch from 6c103fd to 1c8ef7c Compare October 14, 2021 04:10

rafiss requested review from a team and RichardJCai and removed request for a team and stevendanna October 14, 2021 05:50

RichardJCai reviewed Oct 14, 2021

View reviewed changes

RichardJCai approved these changes Oct 14, 2021

View reviewed changes

sql/tree: move role validation test to correct package

2175ad7

Release note: None

rafiss force-pushed the alter-role-rolespec branch from 1c8ef7c to 2175ad7 Compare October 14, 2021 16:32

rafiss commented Oct 14, 2021

View reviewed changes

craig bot merged commit a158794 into cockroachdb:master Oct 14, 2021

This was referenced Oct 14, 2021

sql/parser: use RoleSpec for ALTER ROLE cockroachdb/docs#11971

Closed

sql/parser: use RoleSpec for ALTER ROLE cockroachdb/docs#11972

Closed

rafiss deleted the alter-role-rolespec branch October 18, 2021 16:01

jeffswenson mentioned this pull request Dec 15, 2021

release-21.2: sql: support client-provided password hashes #73855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/parser: use RoleSpec for ALTER ROLE#71498

sql/parser: use RoleSpec for ALTER ROLE#71498
craig[bot] merged 4 commits intocockroachdb:masterfrom
rafiss:alter-role-rolespec

rafiss commented Oct 12, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Oct 12, 2021

Uh oh!

RichardJCai left a comment

Uh oh!

rafiss commented Oct 14, 2021

Uh oh!

RichardJCai Oct 14, 2021

Uh oh!

RichardJCai left a comment

Uh oh!

rafiss left a comment

Uh oh!

craig bot commented Oct 14, 2021

Uh oh!

andy-kimball commented Dec 7, 2021

Uh oh!

rafiss commented Dec 8, 2021

Uh oh!

andy-kimball commented Dec 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rafiss commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 12, 2021

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Oct 14, 2021

Uh oh!

RichardJCai Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Oct 14, 2021

Uh oh!

andy-kimball commented Dec 7, 2021

Uh oh!

rafiss commented Dec 8, 2021

Uh oh!

andy-kimball commented Dec 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rafiss commented Oct 12, 2021 •

edited

Loading