Merge #121380 #122151 #122539 #122557

121380: ui: add static images to asset build step r=laurenbarker a=dhartunian

During the `genassets` build + embed step, we were taking just the output of the `db-console-ccl` or `db-console-oss` step which is just a build.js file. This commit adds references to the image assets we want bundled as well. This includes favicon.ico and everything in `./ assets` relative to the db-console build directory.

We disable content hashing in webpack in order to keep the filenames static, which bazel requires. The impact should be minimal as we rarely change these images so if they're cached forever, it's fine.

This change restores the favicon to the CRDB build and the nice image that shows up in the background of the email signup bar.

The size of the final zipped bundle only differs by around 1MB and is already 10MB in size.

Fixes: #117876
Epic: None

Release note (ui change): the favicon now renders properly for DB Console along with other image files.

122151: ui: make custom chart tool work at store level r=koorosh a=abarganier

Fixes: #121364

This patch fixes a bug in the DB Console custom chart tool, where
selecting the "Per Node" checkbox on a metric would not properly display
store-level metrics. The previous expected behavior was that the check
box would cause the metric to aggregate across stores at the node level
(e.g. if the node had 3 stores, it'd SUM the store-level timeseries
together and return a single timeseries for the node). Instead, the
feature was only showing the 1st store associated with the node.

This was due to a bug in the code used to determine if a metric was
store-level. A function was used that improperly assumed that the
`cr.node.*` or `cr.store.*` prefix had been stripped from the metric
name, which was not always the case. This led to us improperly
interpret store-level metrics as node-level.

The fix is to fix the logic used to determine if a metric is
store-level.

Additionally, this patch updates the code to no longer aggregate
store-level metrics across each node. Instead, we will now show a single
timeseries per-store to provide finer-grained observability into
store-level metrics within the custom chart tool.

Release note (bug fix): A bug has been fixed in the DB Console's custom
chart tool, where store-level metrics were not being displayed properly.
Previously, if a store-level metric was selected to be displayed at the
store-level on a multi-store node, only data for the 1st store ID
associated with that node would be displayed.

This patch ensures that data is displayed for all stores present on a
node. Additionally, it updates the behavior to show a single timeseries
for each store, as opposed to aggregating (e.g. SUM) all stores across
the node. This allows finer-grained observability into store-level
metrics when using the custom chart tool in DB Console.

122539: spanconfigreconcilerccl: use txn descriptor ID generation for test r=rimadeodhar a=rimadeodhar

This PR updates the spanconfigreconciler data driven test to use transactional descriptor ID generation
(#69226) to generate deterministic descriptor IDs. This will help avoid test flakes around changing descriptor IDs due to transaction retries etc.

Epic: none
Fixes: #122343
Release note: None

122557: catalog: add descriptor repair to remove missing roles r=fqazi a=fqazi

Previously, we had a bug that could lead to descriptors having privileages to roles that no longer exist. This could lead to certain commands like SHOW GRANTS breaking. To address this, this patch will add descirptor repair logic to automatically clean up oprhaned privileges.

Fixes: #122552

Release note (bug fix): Add automated clean up / validation for dropped roles inside descriptors.

Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: rimadeodhar <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel

@@ -21,6 +21,7 @@ go_test(
         "//pkg/spanconfig",
         "//pkg/spanconfig/spanconfigtestutils",
         "//pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster",
+        "//pkg/sql",
         "//pkg/sql/sqlliveness/sqllivenesstestutils",
         "//pkg/testutils",
         "//pkg/testutils/datapathutils",

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster"
+	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness/sqllivenesstestutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
@@ -106,6 +107,9 @@ func TestDataDriven(t *testing.T) {
 				Knobs: base.TestingKnobs{
 					JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(), // speeds up test
 					SpanConfig:       scKnobs,
+					SQLExecutor: &sql.ExecutorTestingKnobs{
+						UseTransactionalDescIDGenerator: true,
+					},
 				},
 			},
 		})

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

@@ -814,13 +814,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",
@@ -837,9 +842,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 						ColumnIDs: []descpb.ColumnID{1},
 					},
 				},
-				Privileges: &catpb.PrivilegeDescriptor{
-					Version: catpb.Version23_2,
-				},
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.FunctionDescriptor{
 				Name: "foo",
@@ -850,9 +853,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 						ColumnIDs: []descpb.ColumnID{1},
 					},
 				},
-				Privileges: &catpb.PrivilegeDescriptor{
-					Version: catpb.Version23_2,
-				},
+				Privileges: goodPrivilege,
 			},
 			validIDs: catalog.MakeDescriptorIDSet(104, 105),
 		},
@@ -867,8 +868,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

@@ -132,4 +132,8 @@ const (
 	// GrantExecuteOnFunctionToPublicRole indicates that EXECUTE was granted
 	// to the public role for a function.
 	GrantExecuteOnFunctionToPublicRole
+
+	// 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