diff --git a/pkg/server/api_v2_databases_metadata.go b/pkg/server/api_v2_databases_metadata.go index 70a9b5d8455..3a3c53385a3 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 b50d94d0517..c38adb5c63a 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 2bf6649660a..6646624c81a 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 a9b4a0d81c3..9fa5ed84728 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 48d4d62e514..ac1c54ab044 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 11947b3927f..d6d9551b28e 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 9092da4257e..8866e728d58 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{}