sql: deprecate "GRANT" privilege #74103
Conversation
…rant option Release note (sql change): The roachtest ensures that the validation check for grant options that is performed on a grant/revoke only runs when all nodes on a cluster are upgraded to 22.1. The migration scans through existing users and sets their grant option bits equal to their privilege bits if they have GRANT or ALL privileges (otherwise, it would have been impossible for them to grant anyways, so we do not do anything).
jackcwu
requested review from
shermanCRL and
stevendanna
and removed request for
a team
|
This change is |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| dataSourceName := strings.Replace(urls[0], "root", user, 1) |
There was a problem hiding this comment.
Please parse the url (url.Parse) and replace the username properly (u.User). Then u.String() is the result.
There was a problem hiding this comment.
Thanks for catching this! I resolved it in my other PR #72982
Release note (sql change): We will be deprecating the GRANT privilege in 22.1 before eventually removing it in 22.2 in favor of grant options. To promote backwards compatibility for users with code still using GRANT, we will give grant options on every privilege a user has when they are granted GRANT and remove all their grant options when GRANT is revoked, in addition to the existing grant option behavior.
jackcwu
force-pushed
the
deprecate_grant
branch
from
dd12538 to
b8eb1bd
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jackcwu and @stevendanna)
pkg/sql/alter_default_privileges.go, line 174 at r3 (raw file):
for _, priv := range privileges { grantOrAll = grantOrAll || (priv == privilege.GRANT || priv == privilege.ALL) if priv == privilege.GRANT {
nit: since we are adding in this extra logic for both GRANT and ALL, i think we should also show the notice in the ALL case too. or maybe a different notice that at least mentions that the grant option was automatically applied, but that behavior is being deprecated.
pkg/sql/grant_revoke.go, line 67 at r3 (raw file):
for _, priv := range privileges { grantOrAll = grantOrAll || (priv == privilege.GRANT || priv == privilege.ALL) if priv == privilege.GRANT {
nit: same comment about notice for ALL
pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 20 at r3 (raw file):
# Granting ALL in 22.1 will give grant options automatically since it includes GRANT # statement ok
fyi: you can add noticetrace to a logic test to verify that the notice is what you expect. (see examples in other tests.) i don't think you should add it for everything, but it would be good to check some of them
pkg/sql/logictest/testdata/logic_test/grant_revoke_with_grant_option, line 38 at r3 (raw file):
user testuser statement error pq: missing WITH GRANT OPTION privilege type ALL
optional nit: usually we don't include the "pq: " in the expected error since it's not that useful to test
Resolves #73065
Release note (sql change): We will be deprecating the GRANT privilege in 22.1 before eventually removing it in 22.2 in favor of grant options. To promote backwards compatibility for users with code still using GRANT, we will give grant options on every privilege a user has when they are granted GRANT and remove all their current grant options when GRANT is revoked, in addition to the existing grant option behavior.