Merge #87053 #92545 #92787

87053: GitHub Workflows security hardening r=rail a=sashashura

This PR adds explicit [permissions section](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions) to workflows. This is a security best practice because by default workflows run with [extended set of permissions](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token) (except from `on: pull_request` [from external forks](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/)). By specifying any permission explicitly all others are set to none. By using the principle of least privilege the damage a compromised workflow can do (because of an [injection](https://securitylab.github.com/research/github-actions-untrusted-input/) or compromised third party tool or action) is restricted.
It is recommended to have [most strict permissions on the top level](https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions) and grant write permissions on [job level](https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs) case by case.

Release justification: build infrastructure change only.


92545: sql: fix attidentity for `GENERATED BY DEFAULT AS IDENTITY` column r=rafiss a=ZhouXing19

It should be `d` rather than `b` in this case.

Release note (bug fix): `attidentity` in `pg_attribute` for `GENERATED BY DEFAULT AS IDENTITY` column should be `d`.

Epic: None

92787: multitenant: replace KV NodeLocality endpoint with GetNodeDescriptor r=arulajmani a=ecwall

Fixes #92786

Replace the newly added KV NodeLocality endpoint with Connector.GetNodeDescriptor to make use of the in-memory NodeDescriptor cache.

Release note: None

Co-authored-by: Alex <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: Evan Wall <[email protected]>

.github/workflows/bincheck.yml

@@ -4,6 +4,9 @@ on:
   push:
     tags: [ v* ]
 
+permissions:
+  contents: read
+
 jobs:
 
   linux:

docs/generated/http/full.md

@@ -208,60 +208,6 @@ RegionsResponse describes the available regions.
 
 
 
-## NodeLocality
-
-
-
-NodeLocality retrieves the locality of the node with the given ID.
-
-Support status: [reserved](#support-status)
-
-#### Request Parameters
-
-
-
-
-
-
-
-
-
-
-
-
-
-#### Response Parameters
-
-
-
-
-
-
-
-| Field | Type | Label | Description | Support status |
-| ----- | ---- | ----- | ----------- | -------------- |
-| node_localities | [NodeLocalityResponse.NodeLocalitiesEntry](#cockroach.server.serverpb.NodeLocalityResponse-cockroach.server.serverpb.NodeLocalityResponse.NodeLocalitiesEntry) | repeated |  | [reserved](#support-status) |
-
-
-
-
-
-
-<a name="cockroach.server.serverpb.NodeLocalityResponse-cockroach.server.serverpb.NodeLocalityResponse.NodeLocalitiesEntry"></a>
-#### NodeLocalityResponse.NodeLocalitiesEntry
-
-
-
-| Field | Type | Label | Description | Support status |
-| ----- | ---- | ----- | ----------- | -------------- |
-| key | [int32](#cockroach.server.serverpb.NodeLocalityResponse-int32) |  |  |  |
-| value | [cockroach.roachpb.Locality](#cockroach.server.serverpb.NodeLocalityResponse-cockroach.roachpb.Locality) |  |  |  |
-
-
-
-
-
-
 ## NodesList
 
 

pkg/ccl/kvccl/kvtenantccl/connector.go

@@ -421,17 +421,6 @@ func (c *Connector) Regions(
 	return
 }
 
-// NodeLocality implements the serverpb.TenantStatusServer interface
-func (c *Connector) NodeLocality(
-	ctx context.Context, req *serverpb.NodeLocalityRequest,
-) (resp *serverpb.NodeLocalityResponse, retErr error) {
-	retErr = c.withClient(ctx, func(ctx context.Context, client *client) (err error) {
-		resp, err = client.NodeLocality(ctx, req)
-		return
-	})
-	return
-}
-
 // TenantRanges implements the serverpb.TenantStatusServer interface
 func (c *Connector) TenantRanges(
 	ctx context.Context, req *serverpb.TenantRangesRequest,

pkg/ccl/workloadccl/BUILD.bazel

@@ -18,6 +18,7 @@ go_library(
         "//pkg/security/username",
         "//pkg/settings/cluster",
         "//pkg/util/ctxgroup",
+        "//pkg/util/errorutil",
         "//pkg/util/humanizeutil",
         "//pkg/util/log",
         "//pkg/util/timeutil",

pkg/ccl/workloadccl/fixture.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/cloud"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -350,7 +351,7 @@ func ImportFixture(
 
 	var numNodes int
 	if err := sqlDB.QueryRow(numNodesQuery).Scan(&numNodes); err != nil {
-		if strings.Contains(err.Error(), "operation is unsupported in multi-tenancy mode") {
+		if strings.Contains(err.Error(), errorutil.UnsupportedWithMultiTenancyMessage) {
 			// If the query is unsupported because we're in multi-tenant mode. Assume
 			// that the cluster has 1 node for the purposes of running CSV servers.
 			// Tenants won't use DistSQL to parallelize IMPORT across SQL pods. Doing

pkg/server/server_sql.go

@@ -907,6 +907,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		DescIDGenerator:            descidgen.NewGenerator(cfg.Settings, codec, cfg.db),
 		RangeStatsFetcher:          rangeStatsFetcher,
 		EventsExporter:             cfg.eventsServer,
+		NodeDescs:                  cfg.nodeDescs,
 	}
 
 	if sqlSchemaChangerTestingKnobs := cfg.TestingKnobs.SQLSchemaChanger; sqlSchemaChangerTestingKnobs != nil {

pkg/server/serverpb/status.go

@@ -79,7 +79,6 @@ type NodesStatusServer interface {
 type TenantStatusServer interface {
 	TenantRanges(context.Context, *TenantRangesRequest) (*TenantRangesResponse, error)
 	Regions(context.Context, *RegionsRequest) (*RegionsResponse, error)
-	NodeLocality(context.Context, *NodeLocalityRequest) (*NodeLocalityResponse, error)
 }
 
 // OptionalNodesStatusServer returns the wrapped NodesStatusServer, if it is

pkg/server/serverpb/status.proto

@@ -308,14 +308,6 @@ message RegionsResponse {
   map<string, Region> regions = 1;
 }
 
-message NodeLocalityRequest{}
-
-message NodeLocalityResponse{
-  map<int32, roachpb.Locality> node_localities = 1 [
-    (gogoproto.castkey) = "github.com/cockroachdb/cockroach/pkg/roachpb.NodeID"
-  ];
-}
-
 // NodesListRequest requests list of all nodes.
 // The nodes are KV nodes when the cluster is a single
 // tenant cluster or the host cluster in case of multi-tenant
@@ -1908,9 +1900,6 @@ service Status {
   // RegionsRequest retrieves all available regions.
   rpc Regions(RegionsRequest) returns (RegionsResponse) {}
 
-  // NodeLocality retrieves the locality of the node with the given ID.
-  rpc NodeLocality(NodeLocalityRequest) returns (NodeLocalityResponse) {}
-
   // NodesList returns all available nodes with their addresses.
   rpc NodesList(NodesListRequest) returns (NodesListResponse) {}
 

pkg/server/status.go

@@ -1385,23 +1385,6 @@ func (s *statusServer) Regions(
 	return regionsResponseFromNodesResponse(resp), nil
 }
 
-// NodeLocality implements the serverpb.StatusServer interface.
-func (s *statusServer) NodeLocality(
-	ctx context.Context, req *serverpb.NodeLocalityRequest,
-) (*serverpb.NodeLocalityResponse, error) {
-	resp, _, err := s.nodesHelper(ctx, 0 /* limit */, 0 /* offset */)
-	if err != nil {
-		return nil, serverError(ctx, err)
-	}
-	nodes := resp.Nodes
-	res := make(map[roachpb.NodeID]*roachpb.Locality, len(nodes))
-	for _, node := range nodes {
-		desc := node.Desc
-		res[desc.NodeID] = &desc.Locality
-	}
-	return &serverpb.NodeLocalityResponse{NodeLocalities: res}, nil
-}
-
 func regionsResponseFromNodesResponse(nr *serverpb.NodesResponse) *serverpb.RegionsResponse {
 	regionsToZones := make(map[string]map[string]struct{})
 	for _, node := range nr.Nodes {

pkg/sql/crdb_internal.go

@@ -3667,12 +3667,6 @@ CREATE TABLE crdb_internal.ranges_no_leases (
 			return nil, nil, err
 		}
 
-		resp, err := p.ExecCfg().TenantStatusServer.NodeLocality(ctx, &serverpb.NodeLocalityRequest{})
-		if err != nil {
-			return nil, nil, err
-		}
-		nodeIDToLocality := resp.NodeLocalities
-
 		var desc roachpb.RangeDescriptor
 
 		i := 0
@@ -3726,8 +3720,12 @@ CREATE TABLE crdb_internal.ranges_no_leases (
 
 			replicaLocalityArr := tree.NewDArray(types.String)
 			for _, replica := range votersAndNonVoters {
-				replicaLocality := nodeIDToLocality[replica.NodeID].String()
-				if err := replicaLocalityArr.Append(tree.NewDString(replicaLocality)); err != nil {
+				nodeDesc, err := p.ExecCfg().NodeDescs.GetNodeDescriptor(replica.NodeID)
+				if err != nil {
+					return nil, err
+				}
+				replicaLocality := tree.NewDString(nodeDesc.Locality.String())
+				if err := replicaLocalityArr.Append(replicaLocality); err != nil {
 					return nil, err
 				}
 			}

pkg/sql/exec_util.go

@@ -1328,14 +1328,17 @@ type ExecutorConfig struct {
 	// DescIDGenerator generates unique descriptor IDs.
 	DescIDGenerator eval.DescIDGenerator
 
-	// SyntheticPrivilegeCache
+	// SyntheticPrivilegeCache stores synthetic privileges in an in-memory cache.
 	SyntheticPrivilegeCache *cacheutil.Cache
 
 	// RangeStatsFetcher is used to fetch RangeStats.
 	RangeStatsFetcher eval.RangeStatsFetcher
 
 	// EventsExporter is the client for the Observability Service.
 	EventsExporter obs.EventsExporter
+
+	// NodeDescs stores node descriptors in an in-memory cache.
+	NodeDescs kvcoord.NodeDescStore
 }
 
 // UpdateVersionSystemSettingHook provides a callback that allows us

pkg/sql/logictest/testdata/logic_test/pg_catalog

@@ -783,7 +783,7 @@ t1                 j                         -1         NULL      NULL        NU
 t1                 k                         14         NULL      NULL        NULL      false       false      ·            ·
 t1                 l                         -1         NULL      NULL        NULL      false       false      ·            v
 t1                 m                         -1         NULL      NULL        NULL      true        true       a            ·
-t1                 n                         -1         NULL      NULL        NULL      true        true       b            ·
+t1                 n                         -1         NULL      NULL        NULL      true        true       d            ·
 t1_pkey            p                         -1         NULL      NULL        NULL      true        false      ·            ·
 t1_a_key           a                         -1         NULL      NULL        NULL      false       false      ·            ·
 index_key          b                         -1         NULL      NULL        NULL      false       false      ·            ·

pkg/sql/pg_catalog.go

@@ -416,7 +416,7 @@ https://www.postgresql.org/docs/12/catalog-pg-attribute.html`,
 				if column.IsGeneratedAlwaysAsIdentity() {
 					generatedAsIdentityType = "a"
 				} else if column.IsGeneratedByDefaultAsIdentity() {
-					generatedAsIdentityType = "b"
+					generatedAsIdentityType = "d"
 				} else {
 					return errors.AssertionFailedf(
 						"column %s is of wrong generated as identity type (neither ALWAYS nor BY DEFAULT)",

pkg/workload/workloadsql/workloadsql.go

@@ -107,7 +107,7 @@ func Split(ctx context.Context, db *gosql.DB, table workload.Table, concurrency
 	_, err := db.Exec("SHOW RANGES FROM TABLE system.descriptor")
 	if err != nil {
 		if strings.Contains(err.Error(), "not fully contained in tenant") ||
-			strings.Contains(err.Error(), "operation is unsupported in multi-tenancy mode") {
+			strings.Contains(err.Error(), errorutil.UnsupportedWithMultiTenancyMessage) {
 			log.Infof(ctx, `skipping workload splits; can't split on tenants'`)
 			//nolint:returnerrcheck
 			return nil
@@ -159,8 +159,7 @@ func Split(ctx context.Context, db *gosql.DB, table workload.Table, concurrency
 					// not) help you.
 					stmt := buf.String()
 					if _, err := db.Exec(stmt); err != nil {
-						mtErr := errorutil.UnsupportedWithMultiTenancy(0)
-						if strings.Contains(err.Error(), mtErr.Error()) {
+						if strings.Contains(err.Error(), errorutil.UnsupportedWithMultiTenancyMessage) {
 							// We don't care about split errors if we're running a workload
 							// in multi-tenancy mode; we can't do them so we'll just continue
 							break