Merge pull request #124670 from fqazi/backport23.2.6-rc-122557

release-23.2.6-rc: catalog: add descriptor repair to remove missing roles

pkg/sql/catalog/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/roachpb",
+        "//pkg/security/username",
         "//pkg/server/telemetry",
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/catpb",

pkg/sql/catalog/dbdesc/database_desc_builder.go

@@ -183,6 +183,25 @@ func (ddb *databaseDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+// StripNonExistentRoles implements the catalog.DescriptorBuilder
+// interface.
+func (ddb *databaseDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(ddb.maybeModified.Privileges.Users))
+	for _, priv := range ddb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(ddb.maybeModified.Privileges.Users) {
+		ddb.maybeModified.Privileges.Users = newPrivs
+		ddb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (ddb *databaseDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	ddb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/dbdesc/database_test.go

@@ -389,13 +389,18 @@ func TestMaybeConvertIncompatibleDBPrivilegesToDefaultPrivileges(t *testing.T) {
 	}
 }
 
-func TestStripDanglingBackReferences(t *testing.T) {
+func TestStripDanglingBackReferencesAndRoles(t *testing.T) {
 	type testCase struct {
 		name                  string
 		input, expectedOutput descpb.DatabaseDescriptor
 		validIDs              catalog.DescriptorIDSet
 	}
 
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
 	testData := []testCase{
 		{
 			name: "schema",
@@ -406,13 +411,15 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					"bar": {ID: 101},
 					"baz": {ID: 12345},
 				},
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.DatabaseDescriptor{
 				Name: "foo",
 				ID:   100,
 				Schemas: map[string]descpb.DatabaseDescriptor_SchemaInfo{
 					"bar": {ID: 101},
 				},
+				Privileges: goodPrivilege,
 			},
 			validIDs: catalog.MakeDescriptorIDSet(100, 101),
 		},
@@ -427,8 +434,12 @@ func TestStripDanglingBackReferences(t *testing.T) {
 			require.NoError(t, b.StripDanglingBackReferences(test.validIDs.Contains, func(id jobspb.JobID) bool {
 				return false
 			}))
+			require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+				return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+			}))
 			desc := b.BuildCreatedMutableDatabase()
 			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences))
+			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
 			require.Equal(t, out.BuildCreatedMutableDatabase().DatabaseDesc(), desc.DatabaseDesc())
 		})
 	}

pkg/sql/catalog/descriptor.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -108,6 +109,10 @@ type DescriptorBuilder interface {
 		nonTerminalJobIDMightExist func(id jobspb.JobID) bool,
 	) error
 
+	// StripNonExistentRoles removes any privileges granted to roles that
+	// don't exist.
+	StripNonExistentRoles(roleExists func(role username.SQLUsername) bool) error
+
 	// SetRawBytesInStorage sets `rawBytesInStorage` field by deep-copying `rawBytes`.
 	SetRawBytesInStorage(rawBytes []byte)
 

pkg/sql/catalog/funcdesc/func_desc_builder.go

@@ -151,6 +151,25 @@ func (fdb *functionDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+// StripNonExistentRoles implements the catalog.DescriptorBuilder
+// interface.
+func (fdb *functionDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(fdb.maybeModified.Privileges.Users))
+	for _, priv := range fdb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(fdb.maybeModified.Privileges.Users) {
+		fdb.maybeModified.Privileges.Users = newPrivs
+		fdb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (fdb *functionDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	fdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/funcdesc/func_desc_test.go

@@ -804,13 +804,18 @@ func TestToOverload(t *testing.T) {
 	}
 }
 
-func TestStripDanglingBackReferences(t *testing.T) {
+func TestStripDanglingBackReferencesAndRoles(t *testing.T) {
 	type testCase struct {
 		name                  string
 		input, expectedOutput descpb.FunctionDescriptor
 		validIDs              catalog.DescriptorIDSet
 	}
 
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
 	testData := []testCase{
 		{
 			name: "depended on by",
@@ -827,9 +832,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 						ColumnIDs: []descpb.ColumnID{1},
 					},
 				},
-				Privileges: &catpb.PrivilegeDescriptor{
-					Version: catpb.Version23_2,
-				},
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.FunctionDescriptor{
 				Name: "foo",
@@ -840,9 +843,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 						ColumnIDs: []descpb.ColumnID{1},
 					},
 				},
-				Privileges: &catpb.PrivilegeDescriptor{
-					Version: catpb.Version23_2,
-				},
+				Privileges: goodPrivilege,
 			},
 			validIDs: catalog.MakeDescriptorIDSet(104, 105),
 		},
@@ -857,8 +858,12 @@ func TestStripDanglingBackReferences(t *testing.T) {
 			require.NoError(t, b.StripDanglingBackReferences(test.validIDs.Contains, func(id jobspb.JobID) bool {
 				return false
 			}))
+			require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+				return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+			}))
 			desc := b.BuildCreatedMutableFunction()
 			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences))
+			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
 			require.Equal(t, out.BuildCreatedMutableFunction().FuncDesc(), desc.FuncDesc())
 		})
 	}

pkg/sql/catalog/post_deserialization_changes.go

@@ -140,4 +140,8 @@ const (
 	// FixedIncorrectForeignKeyOrigins indicates that foreign key origin /
 	// reference IDs that should point to the current descriptor were fixed.
 	FixedIncorrectForeignKeyOrigins
+
+	// StrippedNonExistentRoles indicates that at least one role identified did
+	// not exist.
+	StrippedNonExistentRoles
 )

pkg/sql/catalog/schemadesc/BUILD.bazel

@@ -16,6 +16,7 @@ go_library(
         "//pkg/clusterversion",
         "//pkg/jobs/jobspb",
         "//pkg/keys",
+        "//pkg/security/username",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catpb",
         "//pkg/sql/catalog/catprivilege",

pkg/sql/catalog/schemadesc/schema_desc_builder.go

@@ -13,7 +13,9 @@ package schemadesc
 import (
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -138,6 +140,23 @@ func (sdb *schemaDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+func (sdb *schemaDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(sdb.maybeModified.Privileges.Users))
+	for _, priv := range sdb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(sdb.maybeModified.Privileges.Users) {
+		sdb.maybeModified.Privileges.Users = newPrivs
+		sdb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (sdb *schemaDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	sdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/schemadesc/schema_desc_test.go

@@ -243,3 +243,42 @@ func TestValidateCrossSchemaReferences(t *testing.T) {
 		}
 	}
 }
+
+func TestStripNonExistentRoles(t *testing.T) {
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
+	tests := []struct {
+		desc    descpb.SchemaDescriptor
+		expDesc descpb.SchemaDescriptor
+	}{
+		{ // 0
+			desc: descpb.SchemaDescriptor{
+				ID:         52,
+				ParentID:   51,
+				Name:       "schema1",
+				Privileges: badPrivilege,
+			},
+			expDesc: descpb.SchemaDescriptor{
+				ID:         52,
+				ParentID:   51,
+				Name:       "schema1",
+				Privileges: goodPrivilege,
+			},
+		},
+	}
+	for _, test := range tests {
+		b := schemadesc.NewBuilder(&test.desc)
+		require.NoError(t, b.RunPostDeserializationChanges())
+		out := schemadesc.NewBuilder(&test.expDesc)
+		require.NoError(t, out.RunPostDeserializationChanges())
+		require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+			return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+		}))
+		desc := b.BuildCreatedMutableSchema()
+		require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
+		require.Equal(t, out.BuildCreatedMutableSchema().SchemaDesc(), desc.SchemaDesc())
+	}
+}

pkg/sql/catalog/tabledesc/BUILD.bazel

@@ -25,6 +25,7 @@ go_library(
         "//pkg/jobs/jobspb",
         "//pkg/keys",
         "//pkg/roachpb",
+        "//pkg/security/username",
         "//pkg/settings",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catenumpb",

pkg/sql/catalog/tabledesc/table_desc_builder.go

@@ -14,8 +14,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/seqexpr"
@@ -264,6 +266,25 @@ func (tdb *tableDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+// StripNonExistentRoles implements the catalog.DescriptorBuilder
+// interface.
+func (tdb *tableDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(tdb.maybeModified.Privileges.Users))
+	for _, priv := range tdb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(tdb.maybeModified.Privileges.Users) {
+		tdb.maybeModified.Privileges.Users = newPrivs
+		tdb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (tdb *tableDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	tdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/tabledesc/table_desc_test.go

@@ -14,7 +14,9 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/stretchr/testify/require"
 )
@@ -58,14 +60,19 @@ func (desc *Mutable) TestingSetClusterVersion(d descpb.TableDescriptor) {
 	desc.original = makeImmutable(&d)
 }
 
-func TestStripDanglingBackReferences(t *testing.T) {
+func TestStripDanglingBackReferencesAndRoles(t *testing.T) {
 	type testCase struct {
 		name                  string
 		input, expectedOutput descpb.TableDescriptor
 		validDescIDs          catalog.DescriptorIDSet
 		validJobIDs           map[jobspb.JobID]struct{}
 	}
 
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
 	testData := []testCase{
 		{
 			name: "descriptor IDs",
@@ -86,6 +93,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					{OriginTableID: 105, ReferencedTableID: 104},
 				},
 				ReplacementOf: descpb.TableDescriptor_Replacement{ID: 12345},
+				Privileges:    badPrivilege,
 			},
 			expectedOutput: descpb.TableDescriptor{
 				Name: "foo",
@@ -97,6 +105,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 				InboundFKs: []descpb.ForeignKeyConstraint{
 					{OriginTableID: 105},
 				},
+				Privileges: goodPrivilege,
 			},
 			validDescIDs: catalog.MakeDescriptorIDSet(100, 101, 104, 105),
 			validJobIDs:  map[jobspb.JobID]struct{}{},
@@ -115,7 +124,8 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					{MutationID: 1},
 					{MutationID: 2},
 				},
-				DropJobID: 1,
+				DropJobID:  1,
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.TableDescriptor{
 				Name: "foo",
@@ -127,6 +137,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					{MutationID: 1},
 					{MutationID: 2},
 				},
+				Privileges: goodPrivilege,
 			},
 			validDescIDs: catalog.MakeDescriptorIDSet(100, 101, 104, 105),
 			validJobIDs:  map[jobspb.JobID]struct{}{111222333444: {}},
@@ -143,8 +154,12 @@ func TestStripDanglingBackReferences(t *testing.T) {
 				_, ok := test.validJobIDs[id]
 				return ok
 			}))
+			require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+				return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+			}))
 			desc := b.BuildCreatedMutableTable()
 			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences))
+			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
 			require.Equal(t, out.BuildCreatedMutableTable().TableDesc(), desc.TableDesc())
 		})
 	}