Skip to content

Commit

Permalink
roachtest,sql: enhance grant option migration test and fix bugs
Browse files Browse the repository at this point in the history 
This changes the migration so that it runs on all the privilege
descriptors that don't have grant options set.

Additionally, the GRANT/REVOKE logic is changed so that it also applies
the grant option if the grantee currently has the GRANT privilege.

Release note: None
  • Loading branch information
@rafiss
rafiss committed Jan 26, 2022
1 parent e0c82d9 commit 5b77b33
Show file tree
Hide file tree
Showing 17 changed files with 230 additions and 163 deletions.
60 changes: 46 additions & 14 deletions pkg/cmd/roachtest/tests/validate_grant_option.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

func registerValidateGrantOption(r registry.Registry) {
r.Add(registry.TestSpec{
Name: "sql-experience/validate-grant-option",
Name: "validate-grant-option",
Owner: registry.OwnerSQLExperience,
Cluster: r.MakeClusterSpec(3),
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
Expand All @@ -36,6 +36,7 @@ func execSQL(sqlStatement string, expectedErrText string, node int) versionStep
return func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
conn, err := u.c.ConnE(ctx, t.L(), node)
require.NoError(t, err)
t.L().PrintfCtx(ctx, "user root on node %d executing: %s", node, sqlStatement)
_, err = conn.Exec(sqlStatement)
if len(expectedErrText) == 0 {
require.NoError(t, err)
Expand All @@ -50,6 +51,7 @@ func execSQLAsUser(sqlStatement string, user string, expectedErrText string, nod
return func(ctx context.Context, t test.Test, u *versionUpgradeTest) {
conn, err := u.c.ConnEAsUser(ctx, t.L(), node, user)
require.NoError(t, err)
t.L().PrintfCtx(ctx, "user %s on node %d executing: %s", user, node, sqlStatement)
_, err = conn.Exec(sqlStatement)
if len(expectedErrText) == 0 {
require.NoError(t, err)
Expand All @@ -62,6 +64,11 @@ func execSQLAsUser(sqlStatement string, user string, expectedErrText string, nod
func runRegisterValidateGrantOption(
ctx context.Context, t test.Test, c cluster.Cluster, buildVersion version.Version,
) {
// This is meant to test a migration from 21.2 to 22.1.
if buildVersion.Major() != 22 || buildVersion.Minor() != 1 {
t.L().PrintfCtx(ctx, "skipping test because build version is %s", buildVersion)
return
}
const mainVersion = ""
predecessorVersion, err := PredecessorVersion(buildVersion)
if err != nil {
Expand All @@ -81,6 +88,7 @@ func runRegisterValidateGrantOption(
execSQL("CREATE USER foo2;", "", 2),
execSQL("CREATE USER foo3;", "", 2),
execSQL("CREATE USER foo4;", "", 2),
execSQL("CREATE USER foo5;", "", 2),
execSQL("CREATE USER target;", "", 1),

execSQL("CREATE DATABASE d;", "", 2),
Expand All @@ -99,17 +107,39 @@ func runRegisterValidateGrantOption(
execSQLAsUser("GRANT SELECT ON TABLE t1 TO target;", "foo3", "", 1),
execSQLAsUser("GRANT USAGE ON TYPE ty TO target;", "foo4", "", 1),

// Node 1 is upgraded; because nodes 2 and 3 are on the previous version, any user can still grant
// or revoke a privilege that they hold (grant options are ignored).
// Node 1 is upgraded, and nodes 2 and 3 are on the previous version.
binaryUpgradeStep(c.Node(1), mainVersion),
allowAutoUpgradeStep(1),
execSQLAsUser("GRANT CREATE ON DATABASE d TO target;", "foo", "", 1),
execSQLAsUser("GRANT CREATE ON DATABASE defaultdb TO foo;", "root", "", 1),
execSQLAsUser("CREATE TABLE t2();", "foo", "", 1), // t2 created on a new node.
execSQLAsUser("CREATE TABLE t3();", "foo", "", 2), // t3 created on an old node.
execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo2;", "foo", "", 1), // foo2 does not get GRANT or grant option on t2.
execSQLAsUser("GRANT GRANT, CREATE ON TABLE t3 TO foo2;", "foo", "", 2), // foo2 gets GRANT and grant option on t3
execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo", "", 1), // foo3 does not get GRANT or grant option on t2.
execSQLAsUser("GRANT GRANT, CREATE ON TABLE t3 TO foo3;", "foo", "", 2), // foo3 gets GRANT, but not grant option on t3

execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4 WITH GRANT OPTION;", "foo", "pq: version 21.2-22 must be finalized to use grant options", 1),
execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo4 WITH GRANT OPTION;", "foo", "pq: at or near \"with\": syntax error", 2),

execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo2",
"pq: user foo2 does not have GRANT privilege on relation t2", 1),
execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo2",
"pq: user foo2 does not have GRANT privilege on relation t2", 2), // Same error from node 2.
execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo3;", "foo2", "", 1),
execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo3;", "foo2", "", 2),
execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4;", "foo3",
"pq: user foo3 does not have GRANT privilege on relation t2", 1),
execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4;", "foo3",
"pq: user foo3 does not have GRANT privilege on relation t2", 2), // Same error from node 2.
execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo4;", "foo3", "", 1),
execSQLAsUser("GRANT CREATE ON TABLE t3 TO foo4;", "foo3", "", 2),

execSQLAsUser("GRANT USAGE ON SCHEMA s TO target;", "foo2", "", 3),
execSQLAsUser("GRANT INSERT ON TABLE t1 TO target;", "foo3", "", 2),
execSQLAsUser("GRANT GRANT ON TYPE ty TO target;", "foo4", "", 2),

// Node 2 is upgraded; because node 3 is on the previous version, any user can still grant or revoke
// a privilege that they hold (grant options are ignored).
// Node 2 is upgraded.
binaryUpgradeStep(c.Node(2), mainVersion),
allowAutoUpgradeStep(2),
execSQLAsUser("GRANT ALL PRIVILEGES ON DATABASE d TO target;", "foo", "", 3),
Expand All @@ -135,18 +165,20 @@ func runRegisterValidateGrantOption(
execSQLAsUser("GRANT DELETE ON TABLE t1 TO foo2;", "foo3", "", 3),
execSQLAsUser("GRANT ALL PRIVILEGES ON SCHEMA s TO foo3;", "target", "", 2),
execSQLAsUser("GRANT CREATE ON DATABASE d TO foo3;", "foo2", "pq: user foo2 does not have CREATE privilege on database d", 2),
execSQLAsUser("GRANT USAGE ON SCHEMA s TO foo;", "foo3", "pq: missing WITH GRANT OPTION privilege type USAGE", 2),
execSQLAsUser("GRANT USAGE ON SCHEMA s TO foo;", "foo3", "", 2),
execSQLAsUser("GRANT INSERT ON TABLE t1 TO foo;", "foo4", "pq: user foo4 does not have INSERT privilege on relation t1", 1),
execSQLAsUser("GRANT SELECT ON TABLE t1 TO foo;", "foo4", "", 1),
execSQLAsUser("GRANT DELETE ON TABLE t1 TO foo;", "foo4", "", 1),
execSQLAsUser("GRANT DELETE ON TABLE t1 TO target;", "foo2", "pq: user foo2 missing WITH GRANT OPTION privilege on DELETE", 1),
execSQLAsUser("GRANT DELETE ON TABLE t1 TO target;", "foo4", "", 1),
execSQLAsUser("GRANT SELECT ON TABLE t1 TO target;", "foo4", "", 1),

execSQL("CREATE USER foo5;", "", 2),
execSQL("CREATE USER foo6;", "", 2),
execSQL("CREATE TABLE t2();", "", 3),
execSQL("GRANT ALL PRIVILEGES ON TABLE t2 TO foo5;", "", 1),
execSQLAsUser("GRANT DELETE ON TABLE t2 TO foo2;", "foo5", "pq: missing WITH GRANT OPTION privilege type DELETE", 2),
execSQL("GRANT ALL PRIVILEGES ON TABLE t2 TO foo6 WITH GRANT OPTION;", "", 1),
execSQLAsUser("GRANT DELETE ON TABLE t2 TO foo2;", "foo6", "", 3),
execSQLAsUser("GRANT SELECT ON TABLE t2 TO foo4 WITH GRANT OPTION;", "foo", "", 1),
execSQLAsUser("GRANT SELECT ON TABLE t3 TO foo4 WITH GRANT OPTION;", "foo", "", 2),

execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo3;", "foo2", "pq: user foo2 missing WITH GRANT OPTION privilege on CREATE", 1),
execSQLAsUser("GRANT CREATE ON TABLE t3 TO target;", "foo2", "", 2),
execSQLAsUser("GRANT CREATE ON TABLE t2 TO foo4;", "foo3", "pq: user foo3 missing WITH GRANT OPTION privilege on CREATE", 3),
execSQLAsUser("GRANT CREATE ON TABLE t3 TO target;", "foo3", "", 3),
)
u.run(ctx, t)
}
133 changes: 68 additions & 65 deletions pkg/migration/migrations/grant_option_migration_external_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ func TestGrantOptionMigration(t *testing.T) {
*/
const objectTest = `
124e0a02646210341a310a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572100418001204726f6f741802220028033a090a017312040835100040004a005a00
22420834120173183522310a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572100418001204726f6f7418022a00300240004a00
0aab020a0274311836203428023a00423a0a05726f77696410011a0c08011040180030005014600020002a0e756e697175655f726f77696428293001680070007800800100880100980100480252500a0774315f706b6579100118012205726f776964300140004a10080010001a00200028003000380040005a007a0408002000800100880100900104980101a20106080012001800a80100b20100ba010060026a310a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572106418001204726f6f741802800101880103980100b201160a077072696d61727910001a05726f77696420012800b80101c20100e80100f2010408001200f801008002009202009a020a08d8bfcfc594a985dc16b20200b80200c0021dc80200e00200f00200
12480a02646210341a2b0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210041204726f6f741802220028033a090a017312040835100040004a005a00
223c08341201731835222b0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210041204726f6f7418022a00300240004a00
0aa5020a0274311836203428023a00423a0a05726f77696410011a0c08011040180030005014600020002a0e756e697175655f726f77696428293001680070007800800100880100980100480252500a077072696d617279100118012205726f776964300140004a10080010001a00200028003000380040005a007a0408002000800100880100900104980101a20106080012001800a80100b20100ba010060026a2b0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210641204726f6f741802800101880103980100b201160a077072696d61727910001a05726f77696420012800b80101c20100e80100f2010408001200f801008002009202009a020a08e0bea485c6bbede616b20200b80200c0021dc80200e00200f00200
`

// Check that descriptors for multiple users are pulled and updated when
Expand Down Expand Up @@ -104,10 +104,10 @@ func TestGrantOptionMigration(t *testing.T) {
*/
const multipleUsersTest = `
124e0a02646210341a310a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572100218021204726f6f741802220028033a090a017312040835100040004a005a00
22530834120173183522420a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572100218020a0f0a09746573747573657232101418001204726f6f7418022a00300240004a00
0abd020a0274311836203428023a00423a0a05726f77696410011a0c08011040180030005014600020002a0e756e697175655f726f77696428293001680070007800800100880100980100480252500a0774315f706b6579100118012205726f776964300140004a10080010001a00200028003000380040005a007a0408002000800100880100900104980101a20106080012001800a80100b20100ba010060026a430a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572100218020a100a0974657374757365723210a00118001204726f6f741802800101880103980100b201160a077072696d61727910001a05726f77696420012800b80101c20100e80100f2010408001200f801008002009202009a020a08b8b091afa2d484dc16b20200b80200c00235c80200e00200f00200
0abc020a0274321837203428023a00423a0a05726f77696410011a0c08011040180030005014600020002a0e756e697175655f726f77696428293001680070007800800100880100980100480252500a0774325f706b6579100118012205726f776964300140004a10080010001a00200028003000380040005a007a0408002000800100880100900104980101a20106080012001800a80100b20100ba010060026a420a0b0a0561646d696e100218020a0a0a04726f6f74100218020a0e0a087465737475736572100218020a0f0a09746573747573657232100218001204726f6f741802800101880103980100b201160a077072696d61727910001a05726f77696420012800b80101c20100e80100f2010408001200f801008002009202009a020a08d8efb894b8d484dc16b20200b80200c00235c80200e00200f00200
12480a02646210371a2b0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210021204726f6f741802220028033a090a017312040838100040004a005a00
224b08371201731838223a0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210020a0d0a0974657374757365723210141204726f6f7418022a00300240004a00
0ab5020a0274311839203728023a00423a0a05726f77696410011a0c08011040180030005014600020002a0e756e697175655f726f77696428293001680070007800800100880100980100480252500a077072696d617279100118012205726f776964300140004a10080010001a00200028003000380040005a007a0408002000800100880100900104980101a20106080012001800a80100b20100ba010060026a3b0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210020a0e0a0974657374757365723210a0011204726f6f741802800101880103980100b201160a077072696d61727910001a05726f77696420012800b80101c20100e80100f2010408001200f801008002009202009a020a08c8c9daf5dabdede616b20200b80200c00238c80200e00200f00200
0ab4020a027432183a203728023a00423a0a05726f77696410011a0c08011040180030005014600020002a0e756e697175655f726f77696428293001680070007800800100880100980100480252500a077072696d617279100118012205726f776964300140004a10080010001a00200028003000380040005a007a0408002000800100880100900104980101a20106080012001800a80100b20100ba010060026a3a0a090a0561646d696e10020a080a04726f6f7410020a0c0a08746573747573657210020a0d0a0974657374757365723210021204726f6f741802800101880103980100b201160a077072696d61727910001a05726f77696420012800b80101c20100e80100f2010408001200f801008002009202009a020a08c8c7f884dcbdede616b20200b80200c00238c80200e00200f00200
`

// Test the migration for types
Expand Down Expand Up @@ -140,75 +140,78 @@ func TestGrantOptionMigration(t *testing.T) {
*/
const typesTest = `
123e0a02646210341a210a0b0a0561646d696e100218020a0a0a04726f6f74100218021204726f6f741802220028023a090a017312040835100040004a005a00
22320834120173183522210a0b0a0561646d696e100218020a0a0a04726f6f74100218021204726f6f7418022a00300140004a00
1a590834101d1a02747920362800403748025200680072410a0b0a0561646d696e100218020a0d0a067075626c696310800418000a0a0a04726f6f74100218020a0f0a08746573747573657210800418001204726f6f7418027a00
1a710834101d1a035f7479203728013a26080f100018003000381850d78d065a14081810001800300050d68d0660007a0410d78d066000400048015200680072300a0b0a0561646d696e100218020a0d0a067075626c696310800418000a0a0a04726f6f74100218021204726f6f7418027a00
1a59083410351a0374793220382800403948025200680072400a0b0a0561646d696e100218020a0d0a067075626c696310800418000a0a0a04726f6f74100218020a0e0a087465737475736572100218001204726f6f7418027a00
1a72083410351a045f747932203928013a26080f100018003000381850d98d065a14081810001800300050d88d0660007a0410d98d066000400048015200680072300a0b0a0561646d696e100218020a0d0a067075626c696310800418000a0a0a04726f6f74100218021204726f6f7418027a00
123a0a026462103b1a1d0a090a0561646d696e10020a080a04726f6f7410021204726f6f741802220028023a090a01731204083c100040004a005a00
222e083b120173183c221d0a090a0561646d696e10020a080a04726f6f7410021204726f6f7418022a00300140004a00
1a51083b101d1a027479203d2800403e48025200680072390a090a0561646d696e10020a0b0a067075626c69631080040a080a04726f6f7410020a0d0a0874657374757365721080041204726f6f7418027a00
1a6b083b101d1a035f7479203e28013a26080f100018003000381850de8d065a14081810001800300050dd8d0660007a0410de8d0660004000480152006800722a0a090a0561646d696e10020a0b0a067075626c69631080040a080a04726f6f7410021204726f6f7418027a00
1a51083b103c1a03747932203f2800404048025200680072380a090a0561646d696e10020a0b0a067075626c69631080040a080a04726f6f7410020a0c0a08746573747573657210021204726f6f7418027a00
1a6c083b103c1a045f747932204028013a26080f100018003000381850e08d065a14081810001800300050df8d0660007a0410e08d0660004000480152006800722a0a090a0561646d696e10020a0b0a067075626c69631080040a080a04726f6f7410021204726f6f7418027a00
`

testCases := []string{
objectTest,
multipleUsersTest,
typesTest,
testCases := map[string]string{
"object_test": objectTest,
"multipleUsersTest": multipleUsersTest,
"typesTest": typesTest,
}
for _, descriptorStringsToInject := range testCases {
var descriptorsToInject []*descpb.Descriptor
for _, s := range strings.Split(strings.TrimSpace(descriptorStringsToInject), "\n") {
encoded, err := hex.DecodeString(s)
require.NoError(t, err)
var desc descpb.Descriptor
require.NoError(t, protoutil.Unmarshal(encoded, &desc))
descriptorsToInject = append(descriptorsToInject, &desc)
}

ctx := context.Background()
tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
DisableAutomaticVersionUpgrade: 1,
BinaryVersionOverride: clusterversion.ByKey(clusterversion.ValidateGrantOption - 1), // changed cluster version
for name, descriptorStringsToInject := range testCases {
t.Run(name, func(t *testing.T) {
var descriptorsToInject []*descpb.Descriptor
for _, s := range strings.Split(strings.TrimSpace(descriptorStringsToInject), "\n") {
encoded, err := hex.DecodeString(s)
require.NoError(t, err)
var desc descpb.Descriptor
require.NoError(t, protoutil.Unmarshal(encoded, &desc))
descriptorsToInject = append(descriptorsToInject, &desc)
}

ctx := context.Background()
tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
Knobs: base.TestingKnobs{
Server: &server.TestingKnobs{
DisableAutomaticVersionUpgrade: 1,
BinaryVersionOverride: clusterversion.ByKey(clusterversion.ValidateGrantOption - 1), // changed cluster version
},
},
},
},
})
})

db := tc.ServerConn(0)
require.NoError(t, sqlutils.InjectDescriptors(
ctx, db, descriptorsToInject, true, /* force */
))
db := tc.ServerConn(0)
require.NoError(t, sqlutils.InjectDescriptors(
ctx, db, descriptorsToInject, true, /* force */
))

_, err := tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`,
clusterversion.ByKey(clusterversion.ValidateGrantOption).String())
require.NoError(t, err)

sql.TestingDescsTxn(ctx, tc.Server(0), func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error {
// Avoid running validation on the descriptors.
descs, err := col.GetCatalogUnvalidated(ctx, txn)
if err != nil {
return err
}
for _, desc := range descs.OrderedDescriptors() {
privilegeDesc := desc.GetPrivileges()
for _, u := range privilegeDesc.Users {
if privilege.GRANT.IsSetIn(u.Privileges) || privilege.ALL.IsSetIn(u.Privileges) {
if u.UserProto.Decode().IsAdminRole() || u.UserProto.Decode().IsRootUser() || u.UserProto.Decode().IsNodeUser() {
continue
}
_, err := tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`,
clusterversion.ByKey(clusterversion.ValidateGrantOption).String())
require.NoError(t, err)

if u.Privileges != u.WithGrantOption {
return errors.Newf("grant options not updated properly for %d, Privileges: %d, Grant Option: %d", u.User(), u.Privileges, u.WithGrantOption)
err = sql.TestingDescsTxn(ctx, tc.Server(0), func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error {
// Avoid running validation on the descriptors.
cat, err := col.GetCatalogUnvalidated(ctx, txn)
if err != nil {
return err
}
for _, desc := range cat.OrderedDescriptors() {
privilegeDesc := desc.GetPrivileges()
for _, u := range privilegeDesc.Users {
if privilege.GRANT.IsSetIn(u.Privileges) || privilege.ALL.IsSetIn(u.Privileges) {
if u.UserProto.Decode().IsAdminRole() || u.UserProto.Decode().IsRootUser() || u.UserProto.Decode().IsNodeUser() {
continue
}

if u.Privileges != u.WithGrantOption {
return errors.Newf(
"grant options not updated properly for %s, Privileges: %d, Grant Option: %d",
u.User(), u.Privileges, u.WithGrantOption)
}
}
}
}

}
return nil
}
return nil
})
require.NoError(t, err)
tc.Stopper().Stop(ctx)
})

require.NoError(t, err)
tc.Stopper().Stop(ctx)
}
}
Loading

0 comments on commit 5b77b33

Please sign in to comment.