From 3a24d872d57797795e9611bec0d2f59439bbfae5 Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Wed, 6 Nov 2024 13:16:56 -0500 Subject: [PATCH] server: databases apis privilege fixes - Gate status server grpc UpdateTableMetadataCache on ADMIN This should only be accessed via the api v2 http handler, which performs finer privilege checks on if the user has CONNECT privs on a database. - When filtering dbs and tables to return in the apis, permit dbs and tables that have CONNECT on the public role. - Move TestUpdateTableMetadataCacheJobRunsOnRPCTrigger test to status_test.go. This test is more well-suited to live here since it tests functionality in the status server. Epic: CRDB-37558 Fixes: #134431 Fixes: #130245 Release note (ui change): Users may access DB Console's db pages (db overview, tables overview, table details) if they have CONNECT privilege on the database. --- pkg/server/api_v2_databases_metadata.go | 16 +- pkg/server/api_v2_databases_metadata_test.go | 180 ++++++++---------- pkg/server/status.go | 8 + pkg/server/status_test.go | 79 ++++++++ .../update_table_metadata_cache_job_test.go | 65 ------- pkg/sql/tablemetadatacache/util/test_utils.go | 12 ++ pkg/sql/tablemetadatacache/util/util.go | 10 - 7 files changed, 189 insertions(+), 181 deletions(-) diff --git a/pkg/server/api_v2_databases_metadata.go b/pkg/server/api_v2_databases_metadata.go index 70a9b5d8455d..3a3c53385a32 100644 --- a/pkg/server/api_v2_databases_metadata.go +++ b/pkg/server/api_v2_databases_metadata.go @@ -474,10 +474,10 @@ func getTableMetadataBaseQuery(userName string) *safesql.Query { (SELECT "sql.stats.automatic_collection.enabled" as auto_stats_enabled FROM [SHOW CLUSTER SETTING sql.stats.automatic_collection.enabled]) csc LEFT JOIN system.role_members rm ON rm.role = 'admin' AND member = $ - WHERE (rm.role = 'admin' OR tbm.db_name in ( + WHERE (rm.role = 'admin' OR tbm.db_name IN ( SELECT cdp.database_name FROM "".crdb_internal.cluster_database_privileges cdp - WHERE grantee = $ + WHERE (grantee = $ OR grantee = 'public') AND privilege_type = 'CONNECT' )) AND tbm.table_type = 'TABLE' @@ -880,11 +880,11 @@ func getDatabaseMetadataBaseQuery(userName string) *safesql.Query { FROM system.table_metadata, unnest(store_ids) as unnested_ids GROUP BY db_id ) s ON s.db_id = tbm.db_id - WHERE (rm.role = 'admin' OR n.name in ( - SELECT cdp.database_name - FROM "".crdb_internal.cluster_database_privileges cdp - WHERE grantee = $ - AND privilege_type = 'CONNECT' + WHERE (rm.role = 'admin' OR n.name IN ( + SELECT cdp.database_name + FROM "".crdb_internal.cluster_database_privileges cdp + WHERE (grantee = $ OR grantee = 'public') + AND privilege_type = 'CONNECT' )) AND n."parentID" = 0 AND n."parentSchemaID" = 0 @@ -1063,7 +1063,7 @@ func (a *apiV2Server) updateTableMetadataJobAuthorized( UNION SELECT 1 FROM "".crdb_internal.cluster_database_privileges cdp - WHERE cdp.grantee = $ + WHERE (cdp.grantee = $ OR cdp.grantee = 'public') AND cdp.privilege_type = 'CONNECT' ) `, sqlUserStr, sqlUserStr) diff --git a/pkg/server/api_v2_databases_metadata_test.go b/pkg/server/api_v2_databases_metadata_test.go index b50d94d0517f..c38adb5c63aa 100644 --- a/pkg/server/api_v2_databases_metadata_test.go +++ b/pkg/server/api_v2_databases_metadata_test.go @@ -8,7 +8,6 @@ package server import ( "cmp" "context" - gosql "database/sql" "encoding/json" "fmt" "io" @@ -51,8 +50,7 @@ func TestGetTableMetadata(t *testing.T) { testCluster := serverutils.StartCluster(t, 1, base.TestClusterArgs{}) ctx := context.Background() defer testCluster.Stopper().Stop(ctx) - conn := testCluster.ServerConn(0) - defer conn.Close() + conn := sqlutils.MakeSQLRunner(testCluster.ServerConn(0)) var ( db1Name = "new_test_db_1" db2Name = "new_test_db_2" @@ -62,6 +60,7 @@ func TestGetTableMetadata(t *testing.T) { ts := testCluster.Server(0) client, err := ts.GetAdminHTTPClient() require.NoError(t, err) + t.Run("non GET method 405 error", func(t *testing.T) { req, err := http.NewRequest("POST", ts.AdminURL().WithPath("/api/v2/table_metadata/?dbId=10").String(), nil) require.NoError(t, err) @@ -75,67 +74,49 @@ func TestGetTableMetadata(t *testing.T) { require.NoError(t, err) require.Contains(t, string(respBytes), http.StatusText(http.StatusMethodNotAllowed)) }) + t.Run("unknown db id", func(t *testing.T) { mdResp := makeApiRequest[PaginatedResponse[[]tableMetadata]](t, client, ts.AdminURL().WithPath("/api/v2/table_metadata/?dbId=1000").String(), http.MethodGet) require.Len(t, mdResp.Results, 0) require.Equal(t, int64(0), mdResp.PaginationInfo.TotalResults) }) + t.Run("authorization", func(t *testing.T) { sessionUsername := username.TestUserName() userClient, _, err := ts.GetAuthenticatedHTTPClientAndCookie(sessionUsername, false, 1) require.NoError(t, err) - // Assert that the test user gets an empty response for db 1 uri1 := fmt.Sprintf("/api/v2/table_metadata/?dbId=%d", db1Id) - mdResp := makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri1).String(), http.MethodGet) - - require.Empty(t, mdResp.Results) - require.Zero(t, mdResp.PaginationInfo.TotalResults) - - // Assert that the test user gets an empty response for db 2 uri2 := fmt.Sprintf("/api/v2/table_metadata/?dbId=%d", db2Id) - mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri2).String(), http.MethodGet) - - require.Empty(t, mdResp.Results) - require.Zero(t, mdResp.PaginationInfo.TotalResults) - - // Grant connect access to DB 1 - _, e := conn.Exec(fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", db1Name, sessionUsername.Normalized())) - require.NoError(t, e) - mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri1).String(), http.MethodGet) - - // Assert that user now see results for db1 - require.NotEmpty(t, mdResp.Results) - require.True(t, slices.IsSortedFunc(mdResp.Results, defaultTMComparator)) - // Assert that the test user gets an empty response for db 2 - mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri2).String(), http.MethodGet) - - require.Empty(t, mdResp.Results) - require.Zero(t, mdResp.PaginationInfo.TotalResults) - - // Revoke connect access from db1 - _, e = conn.Exec(fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, sessionUsername.Normalized())) - require.NoError(t, e) - mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri1).String(), http.MethodGet) + // System database has id 1. + uriSystem := fmt.Sprintf("/api/v2/table_metadata/?dbId=%d", 1) + + // By default, the test user should see results for db1 and db2 due to + // CONNECT on public. + for _, uri := range []string{uri1, uri2} { + mdResp := makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) + require.NotEmpty(t, mdResp.Results) + require.True(t, slices.IsSortedFunc(mdResp.Results, defaultTMComparator)) + } - // Assert that user no longer sees results from db1 + // Revoke connect access from db1. + conn.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, "public")) + mdResp := makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri1).String(), http.MethodGet) + // Assert that user no longer sees results from db1. require.Empty(t, mdResp.Results) - // Make user admin - // Revoke connect access from db1 - _, e = conn.Exec(fmt.Sprintf("GRANT admin TO %s", sessionUsername.Normalized())) - require.NoError(t, e) + // Make user admin. + conn.Exec(t, fmt.Sprintf("GRANT admin TO %s", sessionUsername.Normalized())) mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri1).String(), http.MethodGet) - - // Assert that user now see results for db1 + // Assert that user now see results for db1. require.NotEmpty(t, mdResp.Results) require.True(t, slices.IsSortedFunc(mdResp.Results, defaultTMComparator)) - // Assert that user now see results for db1 - mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uri2).String(), http.MethodGet) - + // Assert that user now see results for system db. + mdResp = makeApiRequest[PaginatedResponse[[]tableMetadata]](t, userClient, ts.AdminURL().WithPath(uriSystem).String(), http.MethodGet) require.NotEmpty(t, mdResp.Results) require.True(t, slices.IsSortedFunc(mdResp.Results, defaultTMComparator)) }) + t.Run("sorting", func(t *testing.T) { nameComparator := func(first, second tableMetadata) int { return cmp.Or( @@ -223,6 +204,7 @@ func TestGetTableMetadata(t *testing.T) { }) } }) + t.Run("table name filter", func(t *testing.T) { var tableNameTests = []struct { name string @@ -251,6 +233,7 @@ func TestGetTableMetadata(t *testing.T) { }) } }) + t.Run("pagination", func(t *testing.T) { var pageTests = []struct { name string @@ -280,6 +263,7 @@ func TestGetTableMetadata(t *testing.T) { require.Empty(t, mdResp.Results) }) }) + t.Run("filter store id", func(t *testing.T) { storeIds := []int64{1, 2} uri := fmt.Sprintf("/api/v2/table_metadata/?dbId=%d&storeId=%d&storeId=%d", db1Id, storeIds[0], storeIds[1]) @@ -291,6 +275,7 @@ func TestGetTableMetadata(t *testing.T) { }) } }) + t.Run("400 bad request", func(t *testing.T) { var unprocessableTest = []struct { name string @@ -315,6 +300,7 @@ func TestGetTableMetadata(t *testing.T) { }) } }) + t.Run("no views", func(t *testing.T) { uri := fmt.Sprintf("/api/v2/table_metadata/?dbId=%d&name=%s", db1Id, "view") mdResp := makeApiRequest[PaginatedResponse[[]tableMetadata]](t, client, ts.AdminURL().WithPath(uri).String(), http.MethodGet) @@ -329,16 +315,14 @@ func TestGetTableMetadataWithDetails(t *testing.T) { testCluster := serverutils.StartCluster(t, 1, base.TestClusterArgs{}) ctx := context.Background() defer testCluster.Stopper().Stop(ctx) - conn := testCluster.ServerConn(0) - defer conn.Close() - runner := sqlutils.MakeSQLRunner(conn) + runner := sqlutils.MakeSQLRunner(testCluster.ServerConn(0)) var ( db1Name = "new_test_db_1" db2Name = "new_test_db_2" myTable1 = "myTable1" myTable11 = "myTable11" ) - setupTest(t, conn, db1Name, db2Name) + setupTest(t, runner, db1Name, db2Name) ts := testCluster.Server(0) client, err := ts.GetAdminHTTPClient() @@ -354,6 +338,7 @@ func TestGetTableMetadataWithDetails(t *testing.T) { require.NotEmpty(t, resp.Metadata) require.Contains(t, resp.CreateStatement, myTable1) }) + t.Run("authorization", func(t *testing.T) { sessionUsername := username.TestUserName() userClient, _, err := ts.GetAuthenticatedHTTPClientAndCookie(sessionUsername, false, 1) @@ -363,20 +348,19 @@ func TestGetTableMetadataWithDetails(t *testing.T) { t, userClient, ts.AdminURL().WithPath("/api/v2/table_metadata/1/").String(), http.MethodGet) require.Equal(t, TableNotFound, failed) - // grant connect access to db1 to allow request to succeed - runner.Exec(t, fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", db1Name, sessionUsername.Normalized())) + // Request should succeed by default due to CONNECT on public. resp := makeApiRequest[tableMetadataWithDetailsResponse]( t, userClient, ts.AdminURL().WithPath("/api/v2/table_metadata/1/").String(), http.MethodGet) require.NotEmpty(t, resp.Metadata) require.Contains(t, resp.CreateStatement, myTable1) - // revoke access to db1. - runner.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, sessionUsername.Normalized())) + // Revoke access to db1. + runner.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, "public")) failed = makeApiRequest[string]( t, userClient, ts.AdminURL().WithPath("/api/v2/table_metadata/1/").String(), http.MethodGet) require.Equal(t, TableNotFound, failed) - // grant admin access to the user + // Grant admin access to the user. runner.Exec(t, fmt.Sprintf("GRANT ADMIN TO %s", sessionUsername.Normalized())) resp = makeApiRequest[tableMetadataWithDetailsResponse]( t, userClient, ts.AdminURL().WithPath("/api/v2/table_metadata/1/").String(), http.MethodGet) @@ -421,13 +405,12 @@ func TestGetDbMetadata(t *testing.T) { testCluster := serverutils.StartCluster(t, 1, base.TestClusterArgs{}) ctx := context.Background() defer testCluster.Stopper().Stop(ctx) - conn := testCluster.ServerConn(0) - defer conn.Close() + conn := sqlutils.MakeSQLRunner(testCluster.ServerConn(0)) var ( db1Name = "new_test_db_1" db2Name = "new_test_db_2" ) - db1Id, _ := setupTest(t, conn, db1Name, db2Name) + setupTest(t, conn, db1Name, db2Name) ts := testCluster.Server(0) client, err := ts.GetAdminHTTPClient() @@ -493,41 +476,44 @@ func TestGetDbMetadata(t *testing.T) { userClient, _, err := ts.GetAuthenticatedHTTPClientAndCookie(sessionUsername, false, 1) require.NoError(t, err) - // Assert that the test user gets an empty response for db 1 - uri := "/api/v2/database_metadata/" - mdResp := makeApiRequest[PaginatedResponse[[]dbMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) + verifyDatabases := func(expectedDbs []string, resp []dbMetadata) { + require.Len(t, resp, len(expectedDbs)) + for i, db := range expectedDbs { + require.Equal(t, db, resp[i].DbName) + } + } - require.Empty(t, mdResp.Results) - require.Zero(t, mdResp.PaginationInfo.TotalResults) + // All databases grant CONNECT to public by default, so the user should see all databases. + // There should be 4: defaultdb, postgres, new_test_db_1, and new_test_db_2. + // The system db should not be included, since it doe snot have CONNECT granted to public. + uri := "/api/v2/database_metadata/?sortBy=name" + mdResp := makeApiRequest[PaginatedResponse[[]dbMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) + verifyDatabases([]string{"defaultdb", "new_test_db_1", "new_test_db_2", "postgres"}, mdResp.Results) - // Grant connect access to DB 1 - _, e := conn.Exec(fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", db1Name, sessionUsername.Normalized())) - require.NoError(t, e) + // Revoke connect access for public from db1. + conn.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, "public")) + // Asser that user no longer sees db1. mdResp = makeApiRequest[PaginatedResponse[[]dbMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) + verifyDatabases([]string{"defaultdb", "new_test_db_2", "postgres"}, mdResp.Results) + // Grant connect access to DB 1 for user. + conn.Exec(t, fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", db1Name, sessionUsername.Normalized())) + mdResp = makeApiRequest[PaginatedResponse[[]dbMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) // Assert that user now see results for db1 - require.Len(t, mdResp.Results, 1) - require.True(t, mdResp.Results[0].DbId == int64(db1Id)) - require.True(t, slices.IsSortedFunc(mdResp.Results, defaultDMComparator)) + verifyDatabases([]string{"defaultdb", "new_test_db_1", "new_test_db_2", "postgres"}, mdResp.Results) - // Revoke connect access from db1 - _, e = conn.Exec(fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, sessionUsername.Normalized())) - require.NoError(t, e) + // Revoke connect access from db1 again. + conn.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, sessionUsername.Normalized())) mdResp = makeApiRequest[PaginatedResponse[[]dbMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) + // Assert that user no longer sees results from db1. + verifyDatabases([]string{"defaultdb", "new_test_db_2", "postgres"}, mdResp.Results) - // Assert that user no longer sees results from db1 - require.Empty(t, mdResp.Results) - - // Make user admin - _, e = conn.Exec(fmt.Sprintf("GRANT admin TO %s", sessionUsername.Normalized())) - require.NoError(t, e) + // Make user admin. The admin user should see all databases, including system. + conn.Exec(t, fmt.Sprintf("GRANT admin TO %s", sessionUsername.Normalized())) mdResp = makeApiRequest[PaginatedResponse[[]dbMetadata]](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) - - // Assert that user now see results for all dbs (new_test_db_1, new_test_db_2, system, postgres, and defaultdb) - require.Len(t, mdResp.Results, 5) - require.True(t, slices.IsSortedFunc(mdResp.Results, defaultDMComparator)) - + verifyDatabases([]string{"defaultdb", "new_test_db_1", "new_test_db_2", "postgres", "system"}, mdResp.Results) }) + t.Run("pagination", func(t *testing.T) { var pageTests = []struct { name string @@ -551,6 +537,7 @@ func TestGetDbMetadata(t *testing.T) { }) } }) + t.Run("db name filter", func(t *testing.T) { var dbtableNameTests = []struct { name string @@ -574,6 +561,7 @@ func TestGetDbMetadata(t *testing.T) { }) } }) + t.Run("filter store id", func(t *testing.T) { storeIds := []int64{8, 9} uri := fmt.Sprintf("/api/v2/database_metadata/?storeId=%d&storeId=%d", storeIds[0], storeIds[1]) @@ -584,6 +572,7 @@ func TestGetDbMetadata(t *testing.T) { }) } }) + t.Run("400 bad request", func(t *testing.T) { var unprocessableTest = []struct { name string @@ -607,6 +596,7 @@ func TestGetDbMetadata(t *testing.T) { }) } }) + t.Run("table count only includes tables", func(t *testing.T) { uri := fmt.Sprintf("/api/v2/database_metadata/?name=%s", db1Name) mdResp := makeApiRequest[PaginatedResponse[[]dbMetadata]](t, client, ts.AdminURL().WithPath(uri).String(), http.MethodGet) @@ -630,11 +620,9 @@ func TestGetDbMetadataWithDetails(t *testing.T) { testCluster := serverutils.StartCluster(t, 1, base.TestClusterArgs{}) ctx := context.Background() defer testCluster.Stopper().Stop(ctx) - conn := testCluster.ServerConn(0) - defer conn.Close() - runner := sqlutils.MakeSQLRunner(conn) + runner := sqlutils.MakeSQLRunner(testCluster.ServerConn(0)) db1Name := "new_test_db_1" - db1Id, _ := setupTest(t, conn, db1Name, "new_test_db_2") + db1Id, _ := setupTest(t, runner, db1Name, "new_test_db_2") ts := testCluster.Server(0) client, err := ts.GetAdminHTTPClient() @@ -664,23 +652,19 @@ func TestGetDbMetadataWithDetails(t *testing.T) { require.NoError(t, err) uri := fmt.Sprintf("/api/v2/database_metadata/%d/", db1Id) - failed := makeApiRequest[string]( - t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) - require.Equal(t, DatabaseNotFound, failed) - // grant connect access to db1 to allow request to succeed - runner.Exec(t, fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", db1Name, sessionUsername.Normalized())) + // By default, dbs have CONNECT on public, so the user should see db1. resp := makeApiRequest[dbMetadataWithDetailsResponse]( t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) require.Equal(t, int64(db1Id), resp.Metadata.DbId) - // revoke access to db1. - runner.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, sessionUsername.Normalized())) - failed = makeApiRequest[string]( + // Revoke access to db1. + runner.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE %s FROM %s", db1Name, "public")) + failed := makeApiRequest[string]( t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) require.Equal(t, DatabaseNotFound, failed) - // grant admin access to the user + // Grant admin access to the user. runner.Exec(t, fmt.Sprintf("GRANT ADMIN TO %s", sessionUsername.Normalized())) resp = makeApiRequest[dbMetadataWithDetailsResponse]( t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) @@ -727,14 +711,13 @@ func TestGetTableMetadataUpdateJobStatus(t *testing.T) { failed := makeApiRequest[interface{}](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) require.Equal(t, http.StatusText(http.StatusNotFound), failed) - conn.Exec(t, fmt.Sprintf("GRANT CONNECT ON DATABASE defaultdb TO %s", sessionUsername.Normalized())) - mdResp := makeApiRequest[tmUpdateJobStatusResponse](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) require.Equal(t, "NOT_RUNNING", mdResp.CurrentStatus) require.Equal(t, false, mdResp.AutomaticUpdatesEnabled) require.Equal(t, 20*time.Minute, mdResp.DataValidDuration) - conn.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE defaultdb FROM %s", sessionUsername.Normalized())) + conn.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE defaultdb FROM %s", "public")) + conn.Exec(t, fmt.Sprintf("REVOKE CONNECT ON DATABASE postgres FROM %s", "public")) failed = makeApiRequest[string](t, userClient, ts.AdminURL().WithPath(uri).String(), http.MethodGet) require.Equal(t, http.StatusText(http.StatusNotFound), failed) @@ -914,8 +897,9 @@ func triggerAndWaitForJobToComplete( <-jobComplete } -func setupTest(t *testing.T, conn *gosql.DB, db1 string, db2 string) (dbId1 int, dbId2 int) { - runner := sqlutils.MakeSQLRunner(conn) +func setupTest( + t *testing.T, runner *sqlutils.SQLRunner, db1 string, db2 string, +) (dbId1 int, dbId2 int) { runner.Exec(t, `CREATE DATABASE IF NOT EXISTS `+db1) runner.Exec(t, `CREATE DATABASE IF NOT EXISTS `+db2) diff --git a/pkg/server/status.go b/pkg/server/status.go index 2bf6649660ab..6646624c81a4 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -117,6 +117,10 @@ var ( redactedMarker = string(redact.RedactedMarker()) ) +const ( + updateTableMetadataCachePermissionErrMsg = "only admin users can trigger table metadata cache updates" +) + type metricMarshaler interface { json.Marshaler PrintAsText(io.Writer, expfmt.Format) error @@ -4258,6 +4262,10 @@ func (s *statusServer) localUpdateTableMetadataCache() ( func (s *statusServer) UpdateTableMetadataCache( ctx context.Context, req *serverpb.UpdateTableMetadataCacheRequest, ) (*serverpb.UpdateTableMetadataCacheResponse, error) { + _, isAdmin, _ := s.privilegeChecker.GetUserAndRole(ctx) + if !isAdmin { + return nil, status.Error(codes.PermissionDenied, updateTableMetadataCachePermissionErrMsg) + } if req.Local { return s.localUpdateTableMetadataCache() } diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go index a9b4a0d81c32..9fa5ed84728c 100644 --- a/pkg/server/status_test.go +++ b/pkg/server/status_test.go @@ -14,15 +14,21 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/cockroach/pkg/server/authserver" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/status/statuspb" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights" + tablemetadatacacheutil "github.com/cockroachdb/cockroach/pkg/sql/tablemetadatacache/util" + "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" ) @@ -459,3 +465,76 @@ func TestListExecutionInsightsWhileEvictingInsights(t *testing.T) { wg.Wait() } + +// TestStatusUpdateTableMetadataCache tests that signalling the update +// table metadata cache job via the status server triggers the update +// table metadata job to run. +func TestStatusUpdateTableMetadataCache(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + jobCompleteCh := make(chan struct{}) + ctx := context.Background() + tc := serverutils.StartCluster(t, 3, base.TestClusterArgs{ + ServerArgs: base.TestServerArgs{ + Knobs: base.TestingKnobs{ + TableMetadata: &tablemetadatacacheutil.TestingKnobs{ + TableMetadataUpdater: &tablemetadatacacheutil.NoopUpdater{}, + OnJobComplete: func() { + jobCompleteCh <- struct{}{} + }, + }, + }, + }, + }) + defer tc.Stopper().Stop(context.Background()) + + conn := sqlutils.MakeSQLRunner(tc.ServerConn(0)) + + t.Run("gated on admin privilege", func(t *testing.T) { + authCtx := authserver.ForwardHTTPAuthInfoToRPCCalls(authserver.ContextWithHTTPAuthInfo(ctx, username.TestUser, 1), nil) + _, err := tc.Server(0).GetStatusClient(t).UpdateTableMetadataCache(authCtx, + &serverpb.UpdateTableMetadataCacheRequest{Local: false}) + require.Truef(t, testutils.IsError(err, updateTableMetadataCachePermissionErrMsg), "received error: %v", err) + }) + + t.Run("triggers update table metadata cache job", func(t *testing.T) { + // Get the node id that claimed the update job. We'll issue the + // RPC to a node that doesn't own the job to test that the RPC can + // propagate the request to the correct node. + var nodeID int + testutils.SucceedsSoon(t, func() error { + row := conn.Query(t, ` +SELECT claim_instance_id FROM system.jobs +WHERE id = $1 AND claim_instance_id IS NOT NULL`, jobs.UpdateTableMetadataCacheJobID) + if !row.Next() { + return errors.New("no node has claimed the job") + } + require.NoError(t, row.Scan(&nodeID)) + + rpcGatewayNode := (nodeID + 1) % 3 + _, err := tc.Server(rpcGatewayNode).GetStatusClient(t).UpdateTableMetadataCache(ctx, + &serverpb.UpdateTableMetadataCacheRequest{Local: false}) + if err != nil { + return err + } + // The job shouldn't be busy. + return nil + }) + + // Wait for the job to complete. + t.Log("waiting for job to complete") + <-jobCompleteCh + t.Log("job completed") + + row := conn.Query(t, + `SELECT running_status FROM crdb_internal.jobs WHERE job_id = $1 AND running_status IS NOT NULL`, + jobs.UpdateTableMetadataCacheJobID) + if !row.Next() { + t.Fatal("last_run_time not updated") + } + var runningStatus string + require.NoError(t, row.Scan(&runningStatus)) + require.Containsf(t, runningStatus, "Job completed at", "running_status not updated: %s", runningStatus) + }) +} diff --git a/pkg/sql/tablemetadatacache/update_table_metadata_cache_job_test.go b/pkg/sql/tablemetadatacache/update_table_metadata_cache_job_test.go index 48d4d62e5143..ac1c54ab0449 100644 --- a/pkg/sql/tablemetadatacache/update_table_metadata_cache_job_test.go +++ b/pkg/sql/tablemetadatacache/update_table_metadata_cache_job_test.go @@ -13,7 +13,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/jobs" - "github.com/cockroachdb/cockroach/pkg/server/serverpb" tablemetadatacacheutil "github.com/cockroachdb/cockroach/pkg/sql/tablemetadatacache/util" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" @@ -25,70 +24,6 @@ import ( "github.com/stretchr/testify/require" ) -// TestUpdateTableMetadataCacheJobRunsOnRPCTrigger tests that -// signalling the update table metadata cache job via the status -// server triggers the job to run. -func TestUpdateTableMetadataCacheJobRunsOnRPCTrigger(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - - jobCompleteCh := make(chan struct{}) - ctx := context.Background() - tc := serverutils.StartCluster(t, 3, base.TestClusterArgs{ - ServerArgs: base.TestServerArgs{ - Knobs: base.TestingKnobs{ - TableMetadata: &tablemetadatacacheutil.TestingKnobs{ - TableMetadataUpdater: &tablemetadatacacheutil.NoopUpdater{}, - OnJobComplete: func() { - jobCompleteCh <- struct{}{} - }, - }, - }, - }, - }) - defer tc.Stopper().Stop(context.Background()) - - conn := sqlutils.MakeSQLRunner(tc.ServerConn(0)) - - // Get the node id that claimed the update job. We'll issue the - // RPC to a node that doesn't own the job to test that the RPC can - // propagate the request to the correct node. - var nodeID int - testutils.SucceedsSoon(t, func() error { - row := conn.Query(t, ` -SELECT claim_instance_id FROM system.jobs -WHERE id = $1 AND claim_instance_id IS NOT NULL`, jobs.UpdateTableMetadataCacheJobID) - if !row.Next() { - return errors.New("no node has claimed the job") - } - require.NoError(t, row.Scan(&nodeID)) - - rpcGatewayNode := (nodeID + 1) % 3 - _, err := tc.Server(rpcGatewayNode).GetStatusClient(t).UpdateTableMetadataCache(ctx, - &serverpb.UpdateTableMetadataCacheRequest{Local: false}) - if err != nil { - return err - } - // The job shouldn't be busy. - return nil - }) - - // Wait for the job to complete. - t.Log("waiting for job to complete") - <-jobCompleteCh - t.Log("job completed") - - row := conn.Query(t, - `SELECT running_status FROM crdb_internal.jobs WHERE job_id = $1 AND running_status IS NOT NULL`, - jobs.UpdateTableMetadataCacheJobID) - if !row.Next() { - t.Fatal("last_run_time not updated") - } - var runningStatus string - require.NoError(t, row.Scan(&runningStatus)) - require.Containsf(t, runningStatus, "Job completed at", "running_status not updated: %s", runningStatus) -} - // TestUpdateTableMetadataCacheAutomaticUpdates tests that: // 1. The update table metadata cache job does not run automatically by default. // 2. The job runs automatically on the data validity interval when automatic diff --git a/pkg/sql/tablemetadatacache/util/test_utils.go b/pkg/sql/tablemetadatacache/util/test_utils.go index 11947b3927f1..d6d9551b28ed 100644 --- a/pkg/sql/tablemetadatacache/util/test_utils.go +++ b/pkg/sql/tablemetadatacache/util/test_utils.go @@ -5,6 +5,8 @@ package tablemetadatacacheutil +import "context" + // TestingKnobs provides hooks into the table metadata cache job type TestingKnobs struct { // onJobResume is called when the job is ready @@ -56,3 +58,13 @@ func CreateTestingKnobs() *TestingKnobs { aostClause: "AS OF SYSTEM TIME '-1us'", } } + +// NoopUpdater is an implementation of ITableMetadataUpdater that performs a noop when RunUpdater is called. +// This should only be used in tests when the updating of the table_metadata system table isn't necessary. +type NoopUpdater struct{} + +func (nu *NoopUpdater) RunUpdater(_ctx context.Context) error { + return nil +} + +var _ ITableMetadataUpdater = &NoopUpdater{} diff --git a/pkg/sql/tablemetadatacache/util/util.go b/pkg/sql/tablemetadatacache/util/util.go index 9092da4257ed..8866e728d588 100644 --- a/pkg/sql/tablemetadatacache/util/util.go +++ b/pkg/sql/tablemetadatacache/util/util.go @@ -14,13 +14,3 @@ import "context" type ITableMetadataUpdater interface { RunUpdater(ctx context.Context) error } - -// NoopUpdater is an implementation of ITableMetadataUpdater that performs a noop when RunUpdater is called. -// This should only be used in tests when the updating of the table_metadata system table isn't necessary. -type NoopUpdater struct{} - -func (nu *NoopUpdater) RunUpdater(_ctx context.Context) error { - return nil -} - -var _ ITableMetadataUpdater = &NoopUpdater{}