Merge cockroachdb#108971 cockroachdb#109174 cockroachdb#109474 cockro…

…achdb#109478

108971: roachtest: some tweaks to unoptimized-query-oracle r=yuzefovich a=yuzefovich

This commit adjusts the `unoptimized-query-oracle` roachtest in the following manner:
- we now disable DistSQL for the unoptimized run and then randomize the DistSQL mode for the optimized run
- we no longer ignore internal errors on the unoptimized run.

Epic: None

Release note: None

109174: ui: fix metrics page when viewing a tenant with hyphenated name r=Santamaura a=Santamaura

This PR fixes an issue where no metrics would load in when a tenant had a hyphenated name. This was due to trying to use the `Long.fromString()` conversion on a string with a hyphen which it doesn't support. The logic was incorrect for this case anyway so instead a new function `determineTenantID` is introduced which will set the `tenant_id` to `undefined` in this case since we don't need to pass `tenant_id` when switching to a non-system tenant in global context.

Fixes: cockroachdb#109124

Release note (bug fix): fixed an issue on the metrics page where no metrics would load when viewing the non-system tenant in a global context when the tenant name was hyphenated.

<img width="1422" alt="Screenshot 2023-08-21 at 3 33 31 PM" src="https://github.com/cockroachdb/cockroach/assets/17861665/962ffacb-8763-4b5d-9257-c7e3982f7bd5">


109474: sql: add VIEWSYSTEMTABLE system privilege r=rafiss a=rafiss

This privilege is useful for support situations, where an engineer needs to be able to view system tables without having full admin access.

informs cockroachdb#95756
Release note (sql change): Added the VIEWSYSTEMTABLE system privilege. Users with this privilege have SELECT privileges for all tables in the system database.

109478: mirror: add logging for `go` errors r=liamgillies a=rickystewart

This can fail sometimes and the error message is very unclear. Add
logging so we can gather more data.

Epic: none
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Alex Santamaura <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

pkg/cmd/mirror/go/mirror.go

@@ -157,7 +157,12 @@ func downloadZips(
 	cmd.Dir = tmpdir
 	jsonBytes, err := cmd.Output()
 	if err != nil {
-		return nil, err
+		var stderr []byte
+		if exitErr := (*exec.ExitError)(nil); errors.As(err, &exitErr) {
+			stderr = exitErr.Stderr
+		}
+		return nil, fmt.Errorf("failed to run go with arguments %+v; got stdout %s, stderr %s; %w",
+			downloadArgs, string(jsonBytes), string(stderr), err)
 	}
 	var jsonBuilder strings.Builder
 	ret := make(map[string]downloadedModule)
@@ -184,7 +189,12 @@ func listAllModules(tmpdir string) (map[string]listedModule, error) {
 	cmd.Dir = tmpdir
 	jsonBytes, err := cmd.Output()
 	if err != nil {
-		return nil, err
+		var stderr []byte
+		if exitErr := (*exec.ExitError)(nil); errors.As(err, &exitErr) {
+			stderr = exitErr.Stderr
+		}
+		return nil, fmt.Errorf("failed to run `go list -mod=readonly -m -json all`; got stdout %s, stderr %s; %w",
+			string(jsonBytes), string(stderr), err)
 	}
 	ret := make(map[string]listedModule)
 	var jsonBuilder strings.Builder

pkg/cmd/roachtest/tests/unoptimized_query_oracle.go

@@ -120,10 +120,19 @@ func runUnoptimizedQueryOracleImpl(
 	if err := h.execStmt(disableVectorizeStmt); err != nil {
 		return h.makeError(err, "failed to disable the vectorized engine")
 	}
+	disableDistSQLStmt := "SET distsql = off"
+	if err := h.execStmt(disableDistSQLStmt); err != nil {
+		return h.makeError(err, "failed to disable DistSQL")
+	}
 
 	unoptimizedRows, err := h.runQuery(stmt)
 	if err != nil {
-		// Skip unoptimized statements that fail with an error.
+		// Skip unoptimized statements that fail with an error (unless it's an
+		// internal error).
+		if es := err.Error(); strings.Contains(es, "internal error") {
+			verboseLogging = true
+			return h.makeError(err, "internal error while running unoptimized statement")
+		}
 		//nolint:returnerrcheck
 		return nil
 	}
@@ -151,6 +160,15 @@ func runUnoptimizedQueryOracleImpl(
 			return h.makeError(err, "failed to disable not visible index feature")
 		}
 	}
+	if roll := rnd.Intn(4); roll > 0 {
+		distSQLMode := "auto"
+		if roll == 3 {
+			distSQLMode = "on"
+		}
+		if err := h.execStmt(fmt.Sprintf("SET distsql = %s", distSQLMode)); err != nil {
+			return h.makeError(err, "failed to re-enable DistSQL")
+		}
+	}
 
 	// Then, rerun the statement with optimization and/or vectorization enabled
 	// and/or not visible index feature disabled.

pkg/sql/authorization.go

@@ -262,14 +262,32 @@ func (p *planner) CheckPrivilegeForUser(
 	privilegeKind privilege.Kind,
 	user username.SQLUsername,
 ) error {
-	ok, err := p.HasPrivilege(ctx, privilegeObject, privilegeKind, user)
+	hasPriv, err := p.HasPrivilege(ctx, privilegeObject, privilegeKind, user)
 	if err != nil {
 		return err
 	}
-	if !ok {
-		return insufficientPrivilegeError(user, privilegeKind, privilegeObject)
+	if hasPriv {
+		return nil
 	}
-	return nil
+	// Special case for system tables. The VIEWSYSTEMTABLE system privilege is
+	// equivalent to having SELECT on all system tables. This is because it is not
+	// possible to dynamically grant SELECT privileges system tables, but in the
+	// context of support escalations, we need to be able to grant the ability to
+	// view system tables without granting the entire admin role.
+	if d, ok := privilegeObject.(catalog.Descriptor); ok {
+		if catalog.IsSystemDescriptor(d) && privilegeKind == privilege.SELECT {
+			hasViewSystemTablePriv, err := p.HasPrivilege(
+				ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWSYSTEMTABLE, user,
+			)
+			if err != nil {
+				return err
+			}
+			if hasViewSystemTablePriv {
+				return nil
+			}
+		}
+	}
+	return insufficientPrivilegeError(user, privilegeKind, privilegeObject)
 }
 
 // CheckPrivilege implements the AuthorizationAccessor interface.

pkg/sql/logictest/testdata/logic_test/synthetic_privileges

@@ -397,3 +397,29 @@ statement ok
 REVOKE SYSTEM ALL FROM testuser
 
 subtest end
+
+subtest view_system_table
+
+user testuser
+
+# Make sure testuser does not have privileges first.
+statement error user testuser does not have SELECT privilege on relation users
+SELECT * FROM system.users WHERE username = 'testuser'
+
+user root
+
+statement ok
+GRANT SYSTEM VIEWSYSTEMTABLE TO testuser
+
+user testuser
+
+query TT
+SELECT username, "hashedPassword" FROM system.users WHERE username = 'testuser'
+----
+testuser  NULL
+
+# testuser still should not have write privileges.
+statement error user testuser does not have INSERT privilege on relation users
+INSERT INTO system.users VALUES ('cat', null, true, 200)
+
+subtest end

pkg/sql/privilege/privilege.go

@@ -71,6 +71,7 @@ const (
 	MODIFYSQLCLUSTERSETTING  Kind = 28
 	REPLICATION              Kind = 29
 	MANAGETENANT             Kind = 30
+	VIEWSYSTEMTABLE          Kind = 31
 )
 
 // Privilege represents a privilege parsed from an Access Privilege Inquiry
@@ -149,8 +150,12 @@ var (
 	// before v22.2 we treated Sequences the same as Tables. This is to avoid making
 	// certain privileges unavailable after upgrade migration.
 	// Note that "CREATE, CHANGEFEED, INSERT, DELETE, ZONECONFIG" are no-op privileges on sequences.
-	SequencePrivileges           = List{ALL, USAGE, SELECT, UPDATE, CREATE, CHANGEFEED, DROP, INSERT, DELETE, ZONECONFIG}
-	GlobalPrivileges             = List{ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB, MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT}
+	SequencePrivileges = List{ALL, USAGE, SELECT, UPDATE, CREATE, CHANGEFEED, DROP, INSERT, DELETE, ZONECONFIG}
+	GlobalPrivileges   = List{
+		ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED,
+		VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB,
+		MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT, VIEWSYSTEMTABLE,
+	}
 	VirtualTablePrivileges       = List{ALL, SELECT}
 	ExternalConnectionPrivileges = List{ALL, USAGE, DROP}
 )
@@ -196,6 +201,7 @@ var ByName = map[string]Kind{
 	"MODIFYSQLCLUSTERSETTING":  MODIFYSQLCLUSTERSETTING,
 	"REPLICATION":              REPLICATION,
 	"MANAGETENANT":             MANAGETENANT,
+	"VIEWSYSTEMTABLE":          VIEWSYSTEMTABLE,
 }
 
 // List is a list of privileges.

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx

@@ -328,7 +328,7 @@ export class NodeGraphs extends React.Component<
     const nodeSources = selectedNode !== "" ? [selectedNode] : null;
     const selectedTenant = isSystemTenant(currentTenant)
       ? getMatchParamByName(match, tenantNameAttr) || ""
-      : currentTenant;
+      : undefined;
     // When "all" is the selected source, some graphs display a line for every
     // node in the cluster using the nodeIDs collection. However, if a specific
     // node is already selected, these per-node graphs should only display data