tenantcapabilities: gate tenant access to node metadata and tsdb

Previously, tenants were given access via the kv connector, to node level
metrics and metadata. This ability should be gated behind a capability in order
to give operators control over what cluster-level information their application
tenants would have access to.

This commit adds authorization checks using tenant capabilities for the node
metadata query RPC and the TSDB query RPC.

The connection between the specific capability in the RPC it enables, is
encoded in the auth_tenant.go file within the `tenantAuthorizer`. The
capability `Authorizer` type simply provides per-capability check utility
methods.

The `NodesUI` endpoint contains an additional SQL permission gate, which is
honored by checking at the tenant-level, and then delegating (via capability
gate) to a system tenant level `NodesTenant` endpoint that does no additional
SQL gating. Delegating to a system tenant `NodesUI` implementation would fail
since the tenant does not have system-level SQL permissions. The liveness and
TSDB endpoints do no additional checking at time of writing, hence no changes
are made there.

This commit additionally modifies the format of the datadriven tests in the
`tenantcapabilitiesauthorizer` and `tenantcapabilitieswatcher` packages to
conform to the standard datadriven command style instead of implementing custom
parsers.

Resolves #96975

Epic: CRDB-12100

Release note: None

pkg/ccl/backupccl/testdata/backup-restore/restore-tenants

@@ -21,9 +21,9 @@ ALTER TENANT [5] STOP SERVICE; DROP TENANT [5]
 query-sql
 SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
 ----
-1 system 1 2 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-5 <nil> 2 0 false {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "tenant-5", "tenantReplicationJobId": "0"}
-6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 system 1 2 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
+5 <nil> 2 0 false {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "DROP", "deprecatedId": "5", "droppedName": "tenant-5", "tenantReplicationJobId": "0"}
+6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
 
 exec-sql
 BACKUP INTO 'nodelocal://1/cluster'
@@ -59,8 +59,8 @@ job paused at pausepoint
 query-sql
 SELECT id,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
 ----
-1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-6 false {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "ADD", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
+6 false {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "ADD", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
 
 exec-sql
 SET CLUSTER SETTING jobs.debug.pausepoints = ''
@@ -80,8 +80,8 @@ USE defaultdb;
 query-sql
 SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
 ----
-1 system 1 2 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 system 1 2 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
+6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
 
 exec-sql expect-error-regex=(tenant 6 already exists)
 RESTORE TENANT 6 FROM LATEST IN 'nodelocal://1/tenant6';
@@ -105,9 +105,9 @@ RESTORE TENANT 6 FROM LATEST IN 'nodelocal://1/tenant6' WITH tenant_name = 'newn
 query-sql
 SELECT id,name,data_state,service_mode,active,crdb_internal.pb_to_json('cockroach.multitenant.ProtoInfo', info, true) FROM system.tenants;
 ----
-1 system 1 2 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
-2 newname 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"}
-6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
+1 system 1 2 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "1", "droppedName": "", "tenantReplicationJobId": "0"}
+2 newname 1 1 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "2", "droppedName": "", "tenantReplicationJobId": "0"}
+6 tenant-6 1 1 true {"capabilities": {"canAdminSplit": false, "canViewNodeInfo": false, "canViewTsdbMetrics": false}, "deprecatedDataState": "READY", "deprecatedId": "6", "droppedName": "", "tenantReplicationJobId": "0"}
 
 # Check that another service mode is also preserved.
 exec-sql

pkg/ccl/kvccl/kvtenantccl/connector.go

@@ -464,12 +464,12 @@ func (c *Connector) RangeLookup(
 	return nil, nil, errors.Wrap(ctx.Err(), "range lookup")
 }
 
-// NodesUI implements the serverpb.TenantStatusServer interface
-func (c *Connector) NodesUI(
+// Nodes implements the serverpb.TenantStatusServer interface
+func (c *Connector) Nodes(
 	ctx context.Context, req *serverpb.NodesRequest,
-) (resp *serverpb.NodesResponseExternal, retErr error) {
+) (resp *serverpb.NodesResponse, retErr error) {
 	retErr = c.withClient(ctx, func(ctx context.Context, client *client) (err error) {
-		resp, err = client.NodesUI(ctx, req)
+		resp, err = client.Nodes(ctx, req)
 		return
 	})
 	return

pkg/ccl/logictestccl/testdata/logic_test/tenant_capability

@@ -24,6 +24,8 @@ SHOW TENANT 'five' WITH CAPABILITIES
 id  name  data_state  service_mode  capability_name    capability_value
 5   five  ready       none          can_admin_split    false
 5   five  ready       none          can_admin_unsplit  false
+5   five  ready       none          can_view_node_info     false
+5   five  ready       none          can_view_tsdb_metrics  false
 
 statement ok
 ALTER TENANT [5] GRANT CAPABILITY can_admin_split=true
@@ -38,6 +40,8 @@ SHOW TENANT 'five' WITH CAPABILITIES
 id  name  data_state  service_mode  capability_name    capability_value
 5   five  ready       none          can_admin_split    true
 5   five  ready       none          can_admin_unsplit  false
+5   five  ready       none          can_view_node_info     false
+5   five  ready       none          can_view_tsdb_metrics  false
 
 statement ok
 ALTER TENANT [5] REVOKE CAPABILITY can_admin_split
@@ -48,3 +52,5 @@ SHOW TENANT 'five' WITH CAPABILITIES
 id  name  data_state  service_mode  capability_name    capability_value
 5   five  ready       none          can_admin_split    false
 5   five  ready       none          can_admin_unsplit  false
+5   five  ready       none          can_view_node_info     false
+5   five  ready       none          can_view_tsdb_metrics  false

pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_admin_split

@@ -3,6 +3,8 @@ SHOW TENANT [10] WITH CAPABILITIES
 ----
 10 tenant-10 ready none can_admin_split false
 10 tenant-10 ready none can_admin_unsplit false
+10 tenant-10 ready none can_view_node_info false
+10 tenant-10 ready none can_view_tsdb_metrics false
 
 exec-sql-tenant
 CREATE TABLE t(a INT)

pkg/ccl/serverccl/adminccl/BUILD.bazel

@@ -17,12 +17,15 @@ go_test(
         "//pkg/server/serverpb",
         "//pkg/spanconfig",
         "//pkg/sql/tests",
+        "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/skip",
         "//pkg/testutils/testcluster",
+        "//pkg/ts/tspb",
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/randutil",
+        "//pkg/util/timeutil",
         "@com_github_stretchr_testify//require",
     ],
 )

pkg/ccl/serverccl/adminccl/tenant_admin_test.go

@@ -10,16 +10,20 @@ package adminccl
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/ccl/serverccl"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/sql/tests"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
+	"github.com/cockroachdb/cockroach/pkg/ts/tspb"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/stretchr/testify/require"
 )
 
@@ -52,6 +56,46 @@ func TestTenantAdminAPI(t *testing.T) {
 	t.Run("tenant_metricmetadata", func(t *testing.T) {
 		testMetricMetadataRPC(ctx, t, testHelper)
 	})
+
+	t.Run("tenant_metrics_capability", func(t *testing.T) {
+		testTenantMetricsCapabilityRPC(ctx, t, testHelper)
+	})
+}
+
+func testTenantMetricsCapabilityRPC(
+	ctx context.Context, t *testing.T, helper serverccl.TenantTestHelper,
+) {
+	http := helper.TestCluster().TenantAdminHTTPClient(t, 1)
+	defer http.Close()
+
+	query := tspb.TimeSeriesQueryRequest{
+		StartNanos: 0,
+		EndNanos:   timeutil.Now().UnixNano(),
+		Queries: []tspb.Query{
+			{
+				Name: "cr.node.sql.select.count",
+			},
+		},
+		SampleNanos: 0,
+	}
+	queryResp := tspb.TimeSeriesQueryResponse{}
+	err := http.PostJSONChecked("/ts/query", &query, &queryResp)
+	require.Error(t, err)
+
+	db := helper.HostCluster().ServerConn(0)
+	_, err = db.Exec("ALTER TENANT [10] GRANT CAPABILITY can_view_tsdb_metrics=true\n")
+	require.NoError(t, err)
+
+	testutils.SucceedsSoon(t, func() error {
+		err := http.PostJSONChecked("/ts/query", &query, &queryResp)
+		if err != nil {
+			return err
+		}
+		if len(queryResp.Results) == 0 {
+			return errors.New("missing metrics data")
+		}
+		return nil
+	})
 }
 
 func testMetricMetadataRPC(ctx context.Context, t *testing.T, helper serverccl.TenantTestHelper) {

pkg/ccl/serverccl/statusccl/tenant_status_test.go

@@ -83,6 +83,12 @@ func TestTenantStatusAPI(t *testing.T) {
 
 	defer testHelper.Cleanup(ctx, t)
 
+	// Speed up propagation of tenant capability changes.
+	db := testHelper.HostCluster().ServerConn(0)
+	tdb := sqlutils.MakeSQLRunner(db)
+	tdb.Exec(t, "SET CLUSTER SETTING kv.closed_timestamp.target_duration = '10ms'")
+	tdb.Exec(t, "SET CLUSTER SETTING kv.closed_timestamp.side_transport_interval = '10 ms'")
+
 	t.Run("reset_sql_stats", func(t *testing.T) {
 		testResetSQLStatsRPCForTenant(ctx, t, testHelper)
 	})
@@ -146,6 +152,10 @@ func TestTenantStatusAPI(t *testing.T) {
 	t.Run("tenant_span_stats", func(t *testing.T) {
 		testTenantSpanStats(ctx, t, testHelper)
 	})
+
+	t.Run("tenant_nodes_capability", func(t *testing.T) {
+		testTenantNodesCapability(ctx, t, testHelper)
+	})
 }
 
 func testTenantSpanStats(ctx context.Context, t *testing.T, helper serverccl.TenantTestHelper) {
@@ -211,6 +221,31 @@ func testTenantSpanStats(ctx context.Context, t *testing.T, helper serverccl.Ten
 		require.Equal(t, controlStats.RangeCount+1, stats.RangeCount)
 		require.Equal(t, controlStats.TotalStats.LiveCount+int64(len(incKeys)), stats.TotalStats.LiveCount)
 	})
+
+}
+
+func testTenantNodesCapability(
+	ctx context.Context, t *testing.T, helper serverccl.TenantTestHelper,
+) {
+	tenant := helper.TestCluster().TenantStatusSrv(0)
+
+	_, err := tenant.NodesUI(ctx, &serverpb.NodesRequest{})
+	require.Error(t, err)
+
+	db := helper.HostCluster().ServerConn(0)
+	_, err = db.Exec("ALTER TENANT [10] GRANT CAPABILITY can_view_node_info=true\n")
+	require.NoError(t, err)
+
+	testutils.SucceedsSoon(t, func() error {
+		resp, err := tenant.NodesUI(ctx, &serverpb.NodesRequest{})
+		if err != nil {
+			return err
+		}
+		if len(resp.Nodes) == 0 || len(resp.LivenessByNodeID) == 0 {
+			return errors.New("missing nodes or liveness data")
+		}
+		return nil
+	})
 }
 
 func testTenantLogs(ctx context.Context, t *testing.T, helper serverccl.TenantTestHelper) {

pkg/multitenant/tenantcapabilities/capabilities.go

@@ -12,6 +12,7 @@ package tenantcapabilities
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities/tenantcapabilitiespb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -59,6 +60,14 @@ type Authorizer interface {
 	// implementation. Binding the Reader late allows us to break this dependency
 	// cycle.
 	BindReader(reader Reader)
+
+	// HasNodeStatusCapability returns an error if a tenant, referenced by its ID,
+	// is not allowed to access cluster-level node metadata and liveness.
+	HasNodeStatusCapability(ctx context.Context, tenID roachpb.TenantID) error
+
+	// HasTSDBQueryCapability returns an error if a tenant, referenced by its ID,
+	// is not allowed to query the TSDB for metrics.
+	HasTSDBQueryCapability(ctx context.Context, tenID roachpb.TenantID) error
 }
 
 // Entry ties together a tenantID with its capabilities.
@@ -72,3 +81,14 @@ type Update struct {
 	Entry
 	Deleted bool // whether the entry was deleted or not
 }
+
+func (u Update) String() string {
+	if u.Deleted {
+		return fmt.Sprintf("delete: %+v", u.Entry)
+	}
+	return fmt.Sprintf("update: %+v", u.Entry)
+}
+
+func (u Entry) String() string {
+	return fmt.Sprintf("ten=%s cap=%+v", u.TenantID, u.TenantCapabilities)
+}

pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go

@@ -85,3 +85,39 @@ func (a *Authorizer) HasCapabilityForBatch(
 func (a *Authorizer) BindReader(reader tenantcapabilities.Reader) {
 	a.capabilitiesReader = reader
 }
+
+func (a *Authorizer) HasNodeStatusCapability(ctx context.Context, tenID roachpb.TenantID) error {
+	if tenID.IsSystem() {
+		return nil // the system tenant is allowed to do as it pleases
+	}
+	cp, found := a.capabilitiesReader.GetCapabilities(tenID)
+	if !found {
+		log.Infof(
+			ctx,
+			"no capability information for tenant %s; requests that require capabilities may be denied",
+			tenID,
+		)
+	}
+	if !cp.CanViewNodeInfo {
+		return errors.Newf("tenant %s does not have capability to query cluster node metadata", tenID)
+	}
+	return nil
+}
+
+func (a *Authorizer) HasTSDBQueryCapability(ctx context.Context, tenID roachpb.TenantID) error {
+	if tenID.IsSystem() {
+		return nil // the system tenant is allowed to do as it pleases
+	}
+	cp, found := a.capabilitiesReader.GetCapabilities(tenID)
+	if !found {
+		log.Infof(
+			ctx,
+			"no capability information for tenant %s; requests that require capabilities may be denied",
+			tenID,
+		)
+	}
+	if !cp.CanViewTsdbMetrics {
+		return errors.Newf("tenant %s does not have capability to query timeseries data", tenID)
+	}
+	return nil
+}

pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go

@@ -31,17 +31,18 @@ import (
 // "update-state": updates the underlying global tenant capability state.
 // Example:
 //
-//	update-state
-//	upsert {ten=10}:{CanAdminSplit=true}
-//	delete {ten=15}
-//	----
+//		upsert ten=10 can_admin_split=true
+//	 ----
+//	 ok
+//
+//		delete ten=15
+//		----
+//	 ok
 //
 // "has-capability-for-batch": performs a capability check, given a tenant and
 // batch request declaration. Example:
 //
-//	has-capability-for-batch
-//	{ten=10}
-//	split
+//	has-capability-for-batch ten=10 cmds=(split)
 //	----
 func TestDataDriven(t *testing.T) {
 	defer leaktest.AfterTest(t)()
@@ -52,19 +53,36 @@ func TestDataDriven(t *testing.T) {
 		authorizer.BindReader(mockReader)
 
 		datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
+			tenID := tenantcapabilitiestestutils.GetTenantID(t, d)
 			switch d.Cmd {
-			case "update-state":
-				updates := tenantcapabilitiestestutils.ParseTenantCapabilityUpdateStateArguments(t, d.Input)
-				mockReader.updateState(updates)
-
+			case "upsert":
+				update, err := tenantcapabilitiestestutils.ParseTenantCapabilityUpsert(t, d)
+				if err != nil {
+					return err.Error()
+				}
+				mockReader.updateState([]*tenantcapabilities.Update{update})
+			case "delete":
+				update := tenantcapabilitiestestutils.ParseTenantCapabilityDelete(t, d)
+				mockReader.updateState([]*tenantcapabilities.Update{update})
 			case "has-capability-for-batch":
-				tenID, ba := tenantcapabilitiestestutils.ParseBatchRequestString(t, d.Input)
+				ba := tenantcapabilitiestestutils.ParseBatchRequests(t, d)
 				err := authorizer.HasCapabilityForBatch(context.Background(), tenID, &ba)
 				if err == nil {
 					return "ok"
 				}
 				return err.Error()
-
+			case "has-node-status-capability":
+				err := authorizer.HasNodeStatusCapability(context.Background(), tenID)
+				if err == nil {
+					return "ok"
+				}
+				return err.Error()
+			case "has-tsdb-query-capability":
+				err := authorizer.HasTSDBQueryCapability(context.Background(), tenID)
+				if err == nil {
+					return "ok"
+				}
+				return err.Error()
 			default:
 				return fmt.Sprintf("unknown command %s", d.Cmd)
 			}
@@ -75,7 +93,7 @@ func TestDataDriven(t *testing.T) {
 
 type mockReader map[roachpb.TenantID]tenantcapabilitiespb.TenantCapabilities
 
-func (m mockReader) updateState(updates []tenantcapabilities.Update) {
+func (m mockReader) updateState(updates []*tenantcapabilities.Update) {
 	for _, update := range updates {
 		if update.Deleted {
 			delete(m, update.TenantID)