From 52456911ca7d2fe3ac664da23592ffc3842665e5 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Fri, 29 Mar 2024 14:10:29 -0400 Subject: [PATCH 1/4] ui: add static images to asset build step 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. --- pkg/ui/workspaces/db-console/BUILD.bazel | 61 +++++++++++++++++++ .../workspaces/db-console/webpack.config.js | 3 + 2 files changed, 64 insertions(+) diff --git a/pkg/ui/workspaces/db-console/BUILD.bazel b/pkg/ui/workspaces/db-console/BUILD.bazel index 8d7df76da2fa..60ca9b6adaee 100644 --- a/pkg/ui/workspaces/db-console/BUILD.bazel +++ b/pkg/ui/workspaces/db-console/BUILD.bazel @@ -58,7 +58,38 @@ webpack_bin.webpack_cli( ":node_modules", ], outs = [ + "db-console-ccl/assets/Inconsolata-Regular.woff", + "db-console-ccl/assets/Inconsolata-Regular.woff2", + "db-console-ccl/assets/Lato-Bold.woff", + "db-console-ccl/assets/Lato-Bold.woff2", + "db-console-ccl/assets/Lato-Light.woff", + "db-console-ccl/assets/Lato-Light.woff2", + "db-console-ccl/assets/Lato-Medium.woff", + "db-console-ccl/assets/Lato-Medium.woff2", + "db-console-ccl/assets/Lato-Regular.woff", + "db-console-ccl/assets/Lato-Regular.woff2", + "db-console-ccl/assets/RobotoMono-Bold.woff", + "db-console-ccl/assets/RobotoMono-Bold.woff2", + "db-console-ccl/assets/RobotoMono-Medium.woff", + "db-console-ccl/assets/RobotoMono-Medium.woff2", + "db-console-ccl/assets/RobotoMono-Regular.woff", + "db-console-ccl/assets/RobotoMono-Regular.woff2", + "db-console-ccl/assets/SFMono-Semibold.woff", + "db-console-ccl/assets/SFMono-Semibold.woff2", + "db-console-ccl/assets/SourceSansPro-Bold.woff", + "db-console-ccl/assets/SourceSansPro-Bold.woff2", + "db-console-ccl/assets/SourceSansPro-Regular.woff", + "db-console-ccl/assets/SourceSansPro-Regular.woff2", + "db-console-ccl/assets/SourceSansPro-SemiBold.woff", + "db-console-ccl/assets/SourceSansPro-SemiBold.woff2", "db-console-ccl/assets/bundle.js", + "db-console-ccl/assets/email_signup_bg.png", + "db-console-ccl/assets/favicon.ico", + "db-console-ccl/assets/heroBannerLp.png", + "db-console-ccl/assets/login-background.png", + "db-console-ccl/assets/not-found.svg", + "db-console-ccl/assets/questionMap.svg", + "db-console-ccl/assets/spinner.gif", ], args = [ "--config webpack.config.js", @@ -79,7 +110,37 @@ webpack_bin.webpack_cli( ":node_modules", ], outs = [ + "db-console-oss/assets/Inconsolata-Regular.woff", + "db-console-oss/assets/Inconsolata-Regular.woff2", + "db-console-oss/assets/Lato-Bold.woff", + "db-console-oss/assets/Lato-Bold.woff2", + "db-console-oss/assets/Lato-Light.woff", + "db-console-oss/assets/Lato-Light.woff2", + "db-console-oss/assets/Lato-Medium.woff", + "db-console-oss/assets/Lato-Medium.woff2", + "db-console-oss/assets/Lato-Regular.woff", + "db-console-oss/assets/Lato-Regular.woff2", + "db-console-oss/assets/RobotoMono-Bold.woff", + "db-console-oss/assets/RobotoMono-Bold.woff2", + "db-console-oss/assets/RobotoMono-Medium.woff", + "db-console-oss/assets/RobotoMono-Medium.woff2", + "db-console-oss/assets/RobotoMono-Regular.woff", + "db-console-oss/assets/RobotoMono-Regular.woff2", + "db-console-oss/assets/SFMono-Semibold.woff", + "db-console-oss/assets/SFMono-Semibold.woff2", + "db-console-oss/assets/SourceSansPro-Bold.woff", + "db-console-oss/assets/SourceSansPro-Bold.woff2", + "db-console-oss/assets/SourceSansPro-Regular.woff", + "db-console-oss/assets/SourceSansPro-Regular.woff2", + "db-console-oss/assets/SourceSansPro-SemiBold.woff", + "db-console-oss/assets/SourceSansPro-SemiBold.woff2", "db-console-oss/assets/bundle.js", + "db-console-oss/assets/email_signup_bg.png", + "db-console-oss/assets/favicon.ico", + "db-console-oss/assets/heroBannerLp.png", + "db-console-oss/assets/login-background.png", + "db-console-oss/assets/not-found.svg", + "db-console-oss/assets/spinner.gif", ], args = [ "--config webpack.config.js", diff --git a/pkg/ui/workspaces/db-console/webpack.config.js b/pkg/ui/workspaces/db-console/webpack.config.js index 7489805dca9f..40f1e670d779 100644 --- a/pkg/ui/workspaces/db-console/webpack.config.js +++ b/pkg/ui/workspaces/db-console/webpack.config.js @@ -165,6 +165,9 @@ module.exports = (env, argv) => { loader: "url-loader", options: { limit: 10000, + // Preserve the original filename instead of using hash + // in order to play nice with bazel. + name: "[name].[ext]", }, }, { test: /\.html$/, loader: "file-loader" }, From c5e95e9b326e70d4ee539b5dddc94ee896ae4f9a Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Wed, 10 Apr 2024 16:11:24 -0400 Subject: [PATCH 2/4] ui: make custom chart tool work at store level Fixes: https://github.com/cockroachdb/cockroach/issues/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. --- .../containers/customChart/customMetric.tsx | 26 +++---- .../reports/containers/customChart/index.tsx | 74 +++++++++++++------ 2 files changed, 63 insertions(+), 37 deletions(-) diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx index 0f7549dd241a..61e813b500e1 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/customMetric.tsx @@ -54,9 +54,9 @@ export class CustomMetricState { downsampler = TimeSeriesQueryAggregator.AVG; aggregator = TimeSeriesQueryAggregator.SUM; derivative = TimeSeriesQueryDerivative.NONE; - perNode = false; + perSource = false; perTenant = false; - source = ""; + nodeSource = ""; tenantSource = ""; } @@ -112,9 +112,9 @@ export class CustomMetricRow extends React.Component { }); }; - changeSource = (selectedOption: DropdownOption) => { + changeNodeSource = (selectedOption: DropdownOption) => { this.changeState({ - source: selectedOption.value, + nodeSource: selectedOption.value, }); }; @@ -124,9 +124,9 @@ export class CustomMetricRow extends React.Component { }); }; - changePerNode = (selection: React.FormEvent) => { + changePerSource = (selection: React.FormEvent) => { this.changeState({ - perNode: selection.currentTarget.checked, + perSource: selection.currentTarget.checked, }); }; @@ -151,8 +151,8 @@ export class CustomMetricRow extends React.Component { downsampler, aggregator, derivative, - source, - perNode, + nodeSource, + perSource, tenantSource, perTenant, }, @@ -217,17 +217,17 @@ export class CustomMetricRow extends React.Component { className="metric-table-dropdown__select" clearable={false} searchable={false} - value={source} + value={nodeSource} options={nodeOptions} - onChange={this.changeSource} + onChange={this.changeNodeSource} /> {canViewTenantOptions && ( @@ -340,7 +340,7 @@ export class CustomChartTable extends React.Component { Aggregator Rate Source - Per Node + Per Node/Store {canViewTenantOptions && ( Virtual Cluster )} diff --git a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx index fcdccb77b5a3..c01662b6d16a 100644 --- a/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx +++ b/pkg/ui/workspaces/db-console/src/views/reports/containers/customChart/index.tsx @@ -208,56 +208,79 @@ export class CustomChart extends React.Component< ); }; + getSources = ( + nodesSummary: NodesSummary, + metricState: CustomMetricState, + ): string[] => { + if (!(nodesSummary?.nodeStatuses?.length > 0)) { + return []; + } + // If we have no nodeSource, and we're not asking for perSource metrics, + // then the user is asking for cluster-wide metrics. We can return an empty + // source list. + if (metricState.nodeSource === "" && !metricState.perSource) { + return []; + } + if (isStoreMetric(nodesSummary.nodeStatuses[0], metricState.metric)) { + // If a specific node is selected, return the storeIDs associated with that node. + // Otherwise, we're at the cluster level, so we grab each store ID. + return metricState.nodeSource + ? nodesSummary.storeIDsByNodeID[metricState.nodeSource] + : Object.values(nodesSummary.storeIDsByNodeID).flatMap(s => s); + } else { + // If it's not a store metric, and a specific nodeSource is chosen, just return that. + // Otherwise, return all known node IDs. + return metricState.nodeSource + ? [metricState.nodeSource] + : nodesSummary.nodeIDs; + } + }; + // This function handles the logic related to creating Metric components // based on perNode and perTenant flags. renderMetricComponents = (metrics: CustomMetricState[], index: number) => { const { nodesSummary, tenantOptions } = this.props; + // We require nodes information to determine sources (storeIDs/nodeIDs) down below. + if (!(nodesSummary?.nodeStatuses?.length > 0)) { + return; + } const tenants = tenantOptions.length > 1 ? tenantOptions.slice(1) : []; return metrics.map((m, i) => { if (m.metric === "") { return ""; } - if (m.perNode && m.perTenant) { - return _.flatMap(nodesSummary.nodeIDs, nodeID => { + if (m.perSource && m.perTenant) { + const sources = this.getSources(nodesSummary, m); + return _.flatMap(sources, source => { return tenants.map(tenant => ( - n.toString(), - ) - : [nodeID] - } + sources={[source]} tenantSource={tenant.value} /> )); }); - } else if (m.perNode) { - return _.map(nodesSummary.nodeIDs, nodeID => ( + } else if (m.perSource) { + const sources = this.getSources(nodesSummary, m); + return _.map(sources, source => ( - n.toString(), - ) - : [nodeID] - } + sources={[source]} tenantSource={m.tenantSource} /> )); } else if (m.perTenant) { + const sources = this.getSources(nodesSummary, m); return tenants.map(tenant => ( )); @@ -279,7 +302,7 @@ export class CustomChart extends React.Component< aggregator={m.aggregator} downsampler={m.downsampler} derivative={m.derivative} - sources={m.source === "" ? [] : [m.source]} + sources={this.getSources(nodesSummary, m)} tenantSource={m.tenantSource} /> ); @@ -402,5 +425,8 @@ export default withRouter( ); function isStoreMetric(nodeStatus: INodeStatus, metricName: string) { + if (metricName?.startsWith("cr.store")) { + return true; + } return _.has(nodeStatus.store_statuses[0].metrics, metricName); } From 91b20749d3466030bb0e7bf9ced98ce5fdfa2e0d Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Wed, 17 Apr 2024 15:14:54 +0000 Subject: [PATCH 3/4] catalog: add descriptor repair to remove missing roles 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. --- pkg/sql/catalog/BUILD.bazel | 1 + .../catalog/dbdesc/database_desc_builder.go | 19 +++++++++ pkg/sql/catalog/dbdesc/database_test.go | 13 ++++++- pkg/sql/catalog/descriptor.go | 5 +++ pkg/sql/catalog/funcdesc/func_desc_builder.go | 19 +++++++++ pkg/sql/catalog/funcdesc/func_desc_test.go | 19 +++++---- .../catalog/post_deserialization_changes.go | 4 ++ pkg/sql/catalog/schemadesc/BUILD.bazel | 1 + .../catalog/schemadesc/schema_desc_builder.go | 19 +++++++++ .../catalog/schemadesc/schema_desc_test.go | 39 +++++++++++++++++++ pkg/sql/catalog/tabledesc/BUILD.bazel | 1 + .../catalog/tabledesc/table_desc_builder.go | 21 ++++++++++ pkg/sql/catalog/tabledesc/table_desc_test.go | 19 ++++++++- pkg/sql/catalog/typedesc/BUILD.bazel | 1 + pkg/sql/catalog/typedesc/type_desc_builder.go | 21 ++++++++++ pkg/sql/catalog/typedesc/type_desc_test.go | 14 ++++++- pkg/sql/catalog/validate.go | 20 ++++++++++ pkg/sql/crdb_internal.go | 21 ++++++++++ pkg/sql/evalcatalog/BUILD.bazel | 1 + pkg/sql/evalcatalog/pg_updatable.go | 5 +++ .../testdata/logic_test/crdb_internal_catalog | 2 +- pkg/sql/sem/builtins/builtins.go | 38 +++++++++++++++++- pkg/sql/sem/builtins/fixed_oids.go | 2 +- pkg/sql/sem/eval/deps.go | 2 + 24 files changed, 291 insertions(+), 16 deletions(-) diff --git a/pkg/sql/catalog/BUILD.bazel b/pkg/sql/catalog/BUILD.bazel index 5c6eb02f4c27..105b137fcf20 100644 --- a/pkg/sql/catalog/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/dbdesc/database_desc_builder.go b/pkg/sql/catalog/dbdesc/database_desc_builder.go index 96a4fca51907..8ff64312cebf 100644 --- a/pkg/sql/catalog/dbdesc/database_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/dbdesc/database_test.go b/pkg/sql/catalog/dbdesc/database_test.go index 19677aeaf5e2..bd962c6c895b 100644 --- a/pkg/sql/catalog/dbdesc/database_test.go +++ b/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,6 +411,7 @@ func TestStripDanglingBackReferences(t *testing.T) { "bar": {ID: 101}, "baz": {ID: 12345}, }, + Privileges: badPrivilege, }, expectedOutput: descpb.DatabaseDescriptor{ Name: "foo", @@ -413,6 +419,7 @@ func TestStripDanglingBackReferences(t *testing.T) { 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()) }) } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 042f9f531ab5..18f9dba7cce4 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/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) diff --git a/pkg/sql/catalog/funcdesc/func_desc_builder.go b/pkg/sql/catalog/funcdesc/func_desc_builder.go index fb76c80a8c99..368f8abf5b23 100644 --- a/pkg/sql/catalog/funcdesc/func_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/funcdesc/func_desc_test.go b/pkg/sql/catalog/funcdesc/func_desc_test.go index 276ca64ace9e..16d00eb0a6ae 100644 --- a/pkg/sql/catalog/funcdesc/func_desc_test.go +++ b/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()) }) } diff --git a/pkg/sql/catalog/post_deserialization_changes.go b/pkg/sql/catalog/post_deserialization_changes.go index 909cde2efe81..3d65086e0b9e 100644 --- a/pkg/sql/catalog/post_deserialization_changes.go +++ b/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 ) diff --git a/pkg/sql/catalog/schemadesc/BUILD.bazel b/pkg/sql/catalog/schemadesc/BUILD.bazel index 5955bfcec8c1..fc1500576249 100644 --- a/pkg/sql/catalog/schemadesc/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/schemadesc/schema_desc_builder.go b/pkg/sql/catalog/schemadesc/schema_desc_builder.go index 7cb61af31c1a..cbb7572b92c3 100644 --- a/pkg/sql/catalog/schemadesc/schema_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/schemadesc/schema_desc_test.go b/pkg/sql/catalog/schemadesc/schema_desc_test.go index 916168239a47..6b3e405cfe30 100644 --- a/pkg/sql/catalog/schemadesc/schema_desc_test.go +++ b/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()) + } +} diff --git a/pkg/sql/catalog/tabledesc/BUILD.bazel b/pkg/sql/catalog/tabledesc/BUILD.bazel index be197b510e64..a1da6105dc27 100644 --- a/pkg/sql/catalog/tabledesc/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/tabledesc/table_desc_builder.go b/pkg/sql/catalog/tabledesc/table_desc_builder.go index 02070fc8aa35..fbeb64cd5b09 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/tabledesc/table_desc_test.go b/pkg/sql/catalog/tabledesc/table_desc_test.go index 1d601a4ac88f..7bb60915eceb 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_test.go +++ b/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,7 +60,7 @@ 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 @@ -66,6 +68,11 @@ func TestStripDanglingBackReferences(t *testing.T) { 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", @@ -85,6 +92,7 @@ func TestStripDanglingBackReferences(t *testing.T) { {OriginTableID: 105}, }, ReplacementOf: descpb.TableDescriptor_Replacement{ID: 12345}, + Privileges: badPrivilege, }, expectedOutput: descpb.TableDescriptor{ Name: "foo", @@ -96,6 +104,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{}{}, @@ -114,7 +123,8 @@ func TestStripDanglingBackReferences(t *testing.T) { {MutationID: 1}, {MutationID: 2}, }, - DropJobID: 1, + DropJobID: 1, + Privileges: badPrivilege, }, expectedOutput: descpb.TableDescriptor{ Name: "foo", @@ -126,6 +136,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: {}}, @@ -142,8 +153,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()) }) } diff --git a/pkg/sql/catalog/typedesc/BUILD.bazel b/pkg/sql/catalog/typedesc/BUILD.bazel index f0b661bd4d32..69804352e277 100644 --- a/pkg/sql/catalog/typedesc/BUILD.bazel +++ b/pkg/sql/catalog/typedesc/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//pkg/clusterversion", "//pkg/jobs/jobspb", "//pkg/keys", + "//pkg/security/username", "//pkg/sql/catalog", "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/catprivilege", diff --git a/pkg/sql/catalog/typedesc/type_desc_builder.go b/pkg/sql/catalog/typedesc/type_desc_builder.go index 04567f76b851..a7ac0af10e6f 100644 --- a/pkg/sql/catalog/typedesc/type_desc_builder.go +++ b/pkg/sql/catalog/typedesc/type_desc_builder.go @@ -13,7 +13,9 @@ package typedesc 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" @@ -149,6 +151,25 @@ func (tdb *typeDescriptorBuilder) StripDanglingBackReferences( return nil } +// StripNonExistentRoles implements the catalog.DescriptorBuilder +// interface. +func (tdb *typeDescriptorBuilder) 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 *typeDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) { tdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy diff --git a/pkg/sql/catalog/typedesc/type_desc_test.go b/pkg/sql/catalog/typedesc/type_desc_test.go index 4b08b2019d5f..6832a4f52fcc 100644 --- a/pkg/sql/catalog/typedesc/type_desc_test.go +++ b/pkg/sql/catalog/typedesc/type_desc_test.go @@ -848,13 +848,17 @@ func TestValidateTypeDesc(t *testing.T) { } } -func TestStripDanglingBackReferences(t *testing.T) { +func TestStripDanglingBackReferencesAndRoles(t *testing.T) { type testCase struct { name string input, expectedOutput descpb.TypeDescriptor 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: "referencing descriptor IDs", @@ -866,6 +870,7 @@ func TestStripDanglingBackReferences(t *testing.T) { ArrayTypeID: 105, Kind: descpb.TypeDescriptor_TABLE_IMPLICIT_RECORD_TYPE, ReferencingDescriptorIDs: []descpb.ID{12345, 105, 5678}, + Privileges: badPrivilege, }, expectedOutput: descpb.TypeDescriptor{ Name: "foo", @@ -875,6 +880,7 @@ func TestStripDanglingBackReferences(t *testing.T) { ArrayTypeID: 105, Kind: descpb.TypeDescriptor_TABLE_IMPLICIT_RECORD_TYPE, ReferencingDescriptorIDs: []descpb.ID{105}, + Privileges: goodPrivilege, }, validIDs: catalog.MakeDescriptorIDSet(100, 101, 104, 105), }, @@ -889,8 +895,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.BuildCreatedMutableType() require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences)) + require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles)) require.Equal(t, out.BuildCreatedMutableType().TypeDesc(), desc.TypeDesc()) }) } diff --git a/pkg/sql/catalog/validate.go b/pkg/sql/catalog/validate.go index 47c168c15308..cca52af05707 100644 --- a/pkg/sql/catalog/validate.go +++ b/pkg/sql/catalog/validate.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/errors" @@ -196,3 +197,22 @@ func ValidateOutboundTypeRefBackReference(selfID descpb.ID, typ TypeDescriptor) return errors.AssertionFailedf("depends-on type %q (%d) has no corresponding referencing-descriptor back references", typ.GetName(), typ.GetID()) } + +// ValidateRolesInDescriptor validates roles within a descriptor. +func ValidateRolesInDescriptor( + descriptor Descriptor, RoleExists func(username username.SQLUsername) (bool, error), +) error { + for _, priv := range descriptor.GetPrivileges().Users { + exists, err := RoleExists(priv.User()) + if err != nil { + return err + } + if !exists { + return errors.AssertionFailedf("descriptor %q (%d) has privilege on a role %q that doesn't exist", + descriptor.GetName(), + descriptor.GetID(), + priv.User()) + } + } + return nil +} diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 0aa5352789db..2ab925c7bf17 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -6176,6 +6176,21 @@ CREATE TABLE crdb_internal.invalid_objects ( for _, validationError := range ve { doError(validationError) } + doError(catalog.ValidateRolesInDescriptor(descriptor, func(username username.SQLUsername) (bool, error) { + if username.IsRootUser() || + username.IsAdminRole() || + username.IsPublicRole() { + return true, nil + } + err := p.CheckRoleExists(ctx, username) + if err != nil && pgerror.GetPGCode(err) == pgcode.UndefinedObject { + err = nil + return false, err + } else if err != nil { + return false, err + } + return true, nil + })) jobs.ValidateJobReferencesInDescriptor(descriptor, jmg, doError) return err } @@ -6622,6 +6637,12 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions ( system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed') + ), + ( SELECT + array_agg(username) as username_array FROM + (SELECT username + FROM system.users UNION + SELECT 'public' as username) ) ) AS repaired_descriptor diff --git a/pkg/sql/evalcatalog/BUILD.bazel b/pkg/sql/evalcatalog/BUILD.bazel index 73ab778139e4..a8604817f49b 100644 --- a/pkg/sql/evalcatalog/BUILD.bazel +++ b/pkg/sql/evalcatalog/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//pkg/jobs/jobspb", "//pkg/keys", "//pkg/kv", + "//pkg/security/username", "//pkg/sql/catalog", "//pkg/sql/catalog/descbuilder", "//pkg/sql/catalog/descpb", diff --git a/pkg/sql/evalcatalog/pg_updatable.go b/pkg/sql/evalcatalog/pg_updatable.go index a232674fa08a..091f83253697 100644 --- a/pkg/sql/evalcatalog/pg_updatable.go +++ b/pkg/sql/evalcatalog/pg_updatable.go @@ -14,6 +14,7 @@ import ( "context" "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/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -64,6 +65,7 @@ func (b *Builtins) RepairedDescriptor( encodedDescriptor []byte, descIDMightExist func(id descpb.ID) bool, nonTerminalJobIDMightExist func(id jobspb.JobID) bool, + roleExists func(username username.SQLUsername) bool, ) ([]byte, error) { // Use the largest-possible timestamp as a sentinel value when decoding the // descriptor bytes. This will be used to set the modification time field in @@ -82,6 +84,9 @@ func (b *Builtins) RepairedDescriptor( if err := db.StripDanglingBackReferences(descIDMightExist, nonTerminalJobIDMightExist); err != nil { return nil, err } + if err := db.StripNonExistentRoles(roleExists); err != nil { + return nil, err + } mut := db.BuildCreatedMutable() // Strip the sentinel value from the descriptor. if mvccTimestampSentinel.Equal(mut.GetModificationTime()) { diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog index d9f771bb71f8..adbbee31019b 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog @@ -403,7 +403,7 @@ SELECT id, strip_volatile(descriptor) FROM crdb_internal.kv_catalog_descriptor O 4294967195 {"table": {"columns": [{"id": 1, "name": "job_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "start_key", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 3, "name": "end_key", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "resolved", "nullable": true, "type": {"family": "DecimalFamily", "oid": 1700}}, {"id": 5, "name": "resolved_age", "nullable": true, "type": {"family": "IntervalFamily", "intervalDurationField": {}, "oid": 1186}}], "formatVersion": 3, "id": 4294967195, "name": "cluster_replication_spans", "nextColumnId": 6, "nextConstraintId": 1, "nextMutationId": 1, "primaryIndex": {"foreignKey": {}, "geoConfig": {}, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1", "viewQuery": "WITH spans AS (SELECT j.id AS job_id, jsonb_array_elements(((crdb_internal.pb_to_json('progress', i.value)->'streamIngest')->'checkpoint')->'resolvedSpans') AS s FROM system.jobs AS j LEFT JOIN system.job_info AS i ON (j.id = i.job_id) AND (i.info_key = 'legacy_progress') WHERE j.job_type = 'REPLICATION STREAM INGESTION') SELECT job_id, crdb_internal.pretty_key(decode((s->'span')->>'key', 'base64'), 0) AS start_key, crdb_internal.pretty_key(decode((s->'span')->>'endKey', 'base64'), 0) AS end_key, ((((s->'timestamp')->>'wallTime') || '.') || COALESCE(((s->'timestamp')->'logical'), '0'))::DECIMAL AS resolved, date_trunc('second', ((cluster_logical_timestamp() - ((s->'timestamp')->>'wallTime')::INT8) / 1e9)::INTERVAL) AS resolved_age FROM spans"}} 4294967196 {"table": {"columns": [{"id": 1, "name": "desc_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "version", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "sql_instance_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "session_id", "type": {"family": "BytesFamily", "oid": 17}}, {"id": 5, "name": "crdb_region", "type": {"family": "BytesFamily", "oid": 17}}], "formatVersion": 3, "id": 4294967196, "name": "kv_session_based_leases", "nextColumnId": 6, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} 4294967197 {"table": {"columns": [{"id": 1, "name": "id", "type": {"family": "UuidFamily", "oid": 2950}}, {"id": 2, "name": "ts", "type": {"family": "DecimalFamily", "oid": 1700}}, {"id": 3, "name": "meta_type", "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "meta", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}, {"id": 5, "name": "num_spans", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 6, "name": "spans", "type": {"family": "BytesFamily", "oid": 17}}, {"id": 7, "name": "verified", "type": {"oid": 16}}, {"id": 8, "name": "target", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}, {"id": 9, "name": "decoded_meta", "nullable": true, "type": {"family": "JsonFamily", "oid": 3802}}, {"id": 10, "name": "decoded_target", "nullable": true, "type": {"family": "JsonFamily", "oid": 3802}}, {"id": 11, "name": "internal_meta", "nullable": true, "type": {"family": "JsonFamily", "oid": 3802}}, {"id": 12, "name": "num_ranges", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 13, "name": "last_updated", "nullable": true, "type": {"family": "DecimalFamily", "oid": 1700}}], "formatVersion": 3, "id": 4294967197, "name": "kv_protected_ts_records", "nextColumnId": 14, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} -4294967198 {"table": {"columns": [{"id": 1, "name": "parent_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "parent_schema_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "name", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 5, "name": "corruption", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "formatVersion": 3, "id": 4294967198, "name": "kv_repairable_catalog_corruptions", "nextColumnId": 6, "nextConstraintId": 1, "nextMutationId": 1, "primaryIndex": {"foreignKey": {}, "geoConfig": {}, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1", "viewQuery": "WITH data AS (SELECT ns.\"parentID\" AS parent_id, ns.\"parentSchemaID\" AS parent_schema_id, ns.name, COALESCE(ns.id, d.id) AS id, d.descriptor, crdb_internal.descriptor_with_post_deserialization_changes(d.descriptor) AS updated_descriptor, crdb_internal.repaired_descriptor(d.descriptor, (SELECT array_agg(id) AS desc_id_array FROM system.descriptor), (SELECT array_agg(id) AS job_id_array FROM system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed'))) AS repaired_descriptor FROM system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id), diag AS (SELECT *, CASE WHEN (descriptor IS NULL) AND (id != 29) THEN 'namespace' WHEN updated_descriptor != repaired_descriptor THEN 'descriptor' ELSE NULL END AS corruption FROM data) SELECT parent_id, parent_schema_id, name, id, corruption FROM diag WHERE corruption IS NOT NULL ORDER BY parent_id, parent_schema_id, name, id"}} +4294967198 {"table": {"columns": [{"id": 1, "name": "parent_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "parent_schema_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "name", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 5, "name": "corruption", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "formatVersion": 3, "id": 4294967198, "name": "kv_repairable_catalog_corruptions", "nextColumnId": 6, "nextConstraintId": 1, "nextMutationId": 1, "primaryIndex": {"foreignKey": {}, "geoConfig": {}, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1", "viewQuery": "WITH data AS (SELECT ns.\"parentID\" AS parent_id, ns.\"parentSchemaID\" AS parent_schema_id, ns.name, COALESCE(ns.id, d.id) AS id, d.descriptor, crdb_internal.descriptor_with_post_deserialization_changes(d.descriptor) AS updated_descriptor, crdb_internal.repaired_descriptor(d.descriptor, (SELECT array_agg(id) AS desc_id_array FROM system.descriptor), (SELECT array_agg(id) AS job_id_array FROM system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed')), (SELECT array_agg(username) AS username_array FROM (SELECT username FROM system.users UNION SELECT 'public' AS username))) AS repaired_descriptor FROM system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id), diag AS (SELECT *, CASE WHEN (descriptor IS NULL) AND (id != 29) THEN 'namespace' WHEN updated_descriptor != repaired_descriptor THEN 'descriptor' ELSE NULL END AS corruption FROM data) SELECT parent_id, parent_schema_id, name, id, corruption FROM diag WHERE corruption IS NOT NULL ORDER BY parent_id, parent_schema_id, name, id"}} 4294967199 {"table": {"columns": [{"id": 1, "name": "range_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "tenant_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "store_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "priority", "type": {"family": "StringFamily", "oid": 25}}, {"id": 5, "name": "log_term", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 6, "name": "log_index", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 7, "name": "tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "formatVersion": 3, "id": 4294967199, "indexes": [{"foreignKey": {}, "geoConfig": {}, "id": 2, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["range_id"], "name": "kv_flow_token_deductions_range_id_idx", "partitioning": {}, "sharded": {}, "storeColumnIds": [2, 3, 4, 5, 6, 7], "storeColumnNames": ["tenant_id", "store_id", "priority", "log_term", "log_index", "tokens"], "version": 3}], "name": "kv_flow_token_deductions", "nextColumnId": 8, "nextConstraintId": 2, "nextIndexId": 3, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} 4294967200 {"table": {"columns": [{"id": 1, "name": "range_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "tenant_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "store_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "total_tracked_tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "formatVersion": 3, "id": 4294967200, "indexes": [{"foreignKey": {}, "geoConfig": {}, "id": 2, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["range_id"], "name": "kv_flow_control_handles_range_id_idx", "partitioning": {}, "sharded": {}, "storeColumnIds": [2, 3, 4], "storeColumnNames": ["tenant_id", "store_id", "total_tracked_tokens"], "version": 3}], "name": "kv_flow_control_handles", "nextColumnId": 5, "nextConstraintId": 2, "nextIndexId": 3, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} 4294967201 {"table": {"columns": [{"id": 1, "name": "tenant_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "store_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "available_regular_tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "available_elastic_tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "formatVersion": 3, "id": 4294967201, "name": "kv_flow_controller", "nextColumnId": 5, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 90810baa1180..35b121cca92f 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -41,6 +41,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/password" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" @@ -5366,8 +5367,11 @@ DO NOT USE -- USE 'CREATE VIRTUAL CLUSTER' INSTEAD`, jobIDAlwaysValid := func(id jobspb.JobID) bool { return true } + roleAlwaysValid := func(username username.SQLUsername) bool { + return true + } ret, err := evalCtx.CatalogBuiltins.RepairedDescriptor( - ctx, []byte(s), descIDAlwaysValid, jobIDAlwaysValid, + ctx, []byte(s), descIDAlwaysValid, jobIDAlwaysValid, roleAlwaysValid, ) if err != nil { return nil, err @@ -5389,6 +5393,7 @@ DO NOT USE -- USE 'CREATE VIRTUAL CLUSTER' INSTEAD`, {Name: "descriptor", Typ: types.Bytes}, {Name: "valid_descriptor_ids", Typ: types.IntArray}, {Name: "valid_job_ids", Typ: types.IntArray}, + {Name: "valid_roles", Typ: types.StringArray}, }, ReturnType: tree.FixedReturnType(types.Bytes), Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { @@ -5434,8 +5439,31 @@ DO NOT USE -- USE 'CREATE VIRTUAL CLUSTER' INSTEAD`, return found } } + + roleMightExist := func(username username.SQLUsername) bool { + return true + } + if args[3] != tree.DNull { + roles, ok := tree.AsDArray(args[3]) + if !ok { + return nil, errors.Newf("expected array value, got %T", args[3]) + } + roleMap := make(map[username.SQLUsername]struct{}) + for _, roleDatum := range (*roles).Array { + role := tree.MustBeDString(roleDatum) + roleName, err := username.MakeSQLUsernameFromUserInput(string(role), username.PurposeValidation) + if err != nil { + return nil, err + } + roleMap[roleName] = struct{}{} + } + roleMightExist = func(username username.SQLUsername) bool { + _, ok := roleMap[username] + return ok + } + } ret, err := evalCtx.CatalogBuiltins.RepairedDescriptor( - ctx, []byte(s), descIDMightExist, nonTerminalJobIDMightExist, + ctx, []byte(s), descIDMightExist, nonTerminalJobIDMightExist, roleMightExist, ) if err != nil { return nil, err @@ -5479,6 +5507,12 @@ SELECT system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed') + ), + ( SELECT + array_agg(username) as username_array FROM + (SELECT username + FROM system.users UNION + SELECT 'public' as username) ) ), true diff --git a/pkg/sql/sem/builtins/fixed_oids.go b/pkg/sql/sem/builtins/fixed_oids.go index bbb777b181fe..88eaee930f88 100644 --- a/pkg/sql/sem/builtins/fixed_oids.go +++ b/pkg/sql/sem/builtins/fixed_oids.go @@ -2419,7 +2419,7 @@ var builtinOidsArray = []string{ 2449: `st_asmvtgeom(geometry: geometry, bbox: box2d, extent: int, buffer: int) -> geometry`, 2450: `st_asmvtgeom(geometry: geometry, bbox: box2d, extent: int) -> geometry`, 2451: `st_asmvtgeom(geometry: geometry, bbox: box2d) -> geometry`, - 2452: `crdb_internal.repaired_descriptor(descriptor: bytes, valid_descriptor_ids: int[], valid_job_ids: int[]) -> bytes`, + 2452: `crdb_internal.repaired_descriptor(descriptor: bytes, valid_descriptor_ids: int[], valid_job_ids: int[], valid_roles: string[]) -> bytes`, 2453: `crdb_internal.reset_activity_tables() -> bool`, 2454: `crdb_internal.sstable_metrics(node_id: int, store_id: int, start_key: bytes, end_key: bytes) -> tuple{int AS node_id, int AS store_id, int AS level, int AS file_num, int AS approximate_span_bytes, jsonb AS metrics}`, 2455: `crdb_internal.repair_catalog_corruption(descriptor_id: int, corruption: string) -> bool`, diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index 9425bd484e5d..6a757eee09d2 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -127,12 +127,14 @@ type CatalogBuiltins interface { // puts it into a catalog.DescriptorBuilder, // calls RunPostDeserializationChanges, // calls StripDanglingBackReferences, + // calls StripNonExistentRoles, // and re-encodes it. RepairedDescriptor( ctx context.Context, encodedDescriptor []byte, descIDMightExist func(id descpb.ID) bool, nonTerminalJobIDMightExist func(id jobspb.JobID) bool, + roleExists func(username username.SQLUsername) bool, ) ([]byte, error) } From b1501c1108030e813d9e2713427352518f6a3295 Mon Sep 17 00:00:00 2001 From: rimadeodhar Date: Wed, 17 Apr 2024 11:28:30 -0700 Subject: [PATCH 4/4] spanconfigreconcilerccl: Use deterministic descriptor ID generation for test This PR updates the spanconfigreconciler data driven test to use transactional descriptor ID generation (https://github.com/cockroachdb/cockroach/pull/85444) to generate deterministic descriptor IDs. This will help avoid test flakes around changing descriptor IDs due to transaction retries etc. Epic: none Fixes: https://github.com/cockroachdb/cockroach/issues/122343 Release note: None --- pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel | 1 + .../spanconfigccl/spanconfigreconcilerccl/datadriven_test.go | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel index e95cb1dd8ef3..38a548a46543 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/BUILD.bazel +++ b/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", diff --git a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go index 7243d437ce4d..336d2ca47e8c 100644 --- a/pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go +++ b/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, + }, }, }, })