sql, server: add new system privileges for observability #85280

Santamaura · 2022-07-28T21:08:19Z

This patch introduces 2 new system privileges
VIEWDEBUG and VIEWCLUSTERMETADATA. VIEWDEBUG
will now be used to gate taking traces and viewing
debug endpoints. VIEWCLUSTERMETADATA will now be
used to gate the node and range reports.

Resolves #83844, #83856, #83857, #83858, #83861

Release note (sql change): add VIEWDEBUG and
VIEWCLUSTERMETADATA system privileges.

cockroach-teamcity · 2022-07-28T21:08:25Z

This change is

Santamaura · 2022-08-03T13:43:30Z

Some tests that don't seem related seem to fail so might be missing something re: adding the new builtin roles.

Edit: irrelevant now

RichardJCai

LGTM other than nit about error message in version gate. We should return the old error message about admin user

RichardJCai · 2022-08-08T21:39:52Z

pkg/server/admin.go

+		if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasViewClusterMetadata = c.checkHasSystemPrivilege(ctx, userName, privilege.VIEWCLUSTERMETADATA)
+		}
+		if !hasViewClusterMetadata {


I think this should go into the IsActive since if it's not active, we can't actually grant VIEWCLUSTERMETADATA

RichardJCai · 2022-08-08T21:40:02Z

pkg/server/admin.go

+		if c.st.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
+			hasViewDebug = c.checkHasSystemPrivilege(ctx, userName, privilege.VIEWDEBUG)
+		}
+		if !hasViewDebug {


for observability This patch introduces 2 new system privileges VIEWDEBUG and VIEWCLUSTERMETADATA. VIEWDEBUG will now be used to gate taking traces and viewing debug endpoints. VIEWCLUSTERMETADATA will now be used to gate the node and range reports. Resolves cockroachdb#83844, cockroachdb#83856, cockroachdb#83857, cockroachdb#83858, cockroachdb#83861 Release note (sql change): add VIEWDEBUG and VIEWCLUSTERMETADATA system privileges.

Santamaura

Ack

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @koorosh, @rhu713, @RichardJCai, and @zachlite)

pkg/server/admin.go line 3563 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think this should go into the IsActive since if it's not active, we can't actually grant VIEWCLUSTERMETADATA

Done

pkg/server/admin.go line 3584 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Ditto

Done

RichardJCai

LGTM modulo nits

RichardJCai · 2022-08-09T17:59:52Z

pkg/server/admin.go

+					codes.PermissionDenied, "this operation requires the %s system privilege",
+					privilege.VIEWDEBUG)
+			}
+		} else {


nit: don't need else, I think linter might complain about this too

RichardJCai · 2022-08-09T18:00:00Z

pkg/server/admin.go

+					codes.PermissionDenied, "this operation requires the %s system privilege",
+					privilege.VIEWCLUSTERMETADATA)
+			}
+		} else {


nit: don't need else, I think linter might complain about this too

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @koorosh, @rhu713, @RichardJCai, @Santamaura, and @zachlite)

pkg/server/admin.go line 3565 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: don't need else, I think linter might complain about this too

@RichardJCai wouldn't we need the else, if the user doesn't have admin but has the privilege it would hit the admin error without the else right?

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @koorosh, @rhu713, @RichardJCai, @Santamaura, and @zachlite)

pkg/server/admin.go line 3565 at r3 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

@RichardJCai wouldn't we need the else, if the user doesn't have admin but has the privilege it would hit the admin error without the else right?

I meant we can return without the else.

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @koorosh, @rhu713, @RichardJCai, @Santamaura, and @zachlite)

pkg/server/admin.go line 3565 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I meant we can return without the else.

I'm a kind of confused, if we omit the else the scenario I mentioned above will occur or am I going crazy?

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @koorosh, @rhu713, @RichardJCai, @Santamaura, and @zachlite)

pkg/server/admin.go line 3565 at r3 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

I'm a kind of confused, if we omit the else the scenario I mentioned above will occur or am I going crazy?

if {
   ...
   return ...
} else {
   return ...
}

is the same as just

if { 
  ...
  return ...
}
return ...

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @koorosh, @rhu713, @RichardJCai, @Santamaura, and @zachlite)

pkg/server/admin.go line 3565 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

if {
   ...
} else {
   return ...
}

is the same as just

if { 
  ...
}
return ...

Wait sorry I read the brackets wrong and there has to be a return in the if

Santamaura · 2022-08-09T19:39:11Z

Per our async discussion I will go ahead and bors it

Santamaura · 2022-08-09T19:39:20Z

bors r+

craig · 2022-08-09T22:02:48Z

Build succeeded:

Bazel Essential CI (Cockroach)

Santamaura changed the title ~~sql, server: add new builtin roles and system privileges~~ sql, server: add new builtin roles and system privileges for observability Jul 28, 2022

Santamaura force-pushed the new-obs-privileges-roles branch 15 times, most recently from 3bccb5e to fb679c4 Compare August 3, 2022 13:41

Santamaura requested review from koorosh, zachlite, RichardJCai and a team August 3, 2022 13:42

Santamaura marked this pull request as ready for review August 3, 2022 13:43

Santamaura requested review from a team August 3, 2022 13:43

Santamaura requested review from a team as code owners August 3, 2022 13:43

Santamaura requested a review from rhu713 August 3, 2022 13:43

Santamaura marked this pull request as draft August 3, 2022 16:22

Santamaura force-pushed the new-obs-privileges-roles branch 3 times, most recently from 080a490 to cc5f81e Compare August 4, 2022 15:19

shermanCRL requested a review from adityamaru August 4, 2022 15:20

Santamaura changed the title ~~sql, server: add new builtin roles and system privileges for observability~~ sql, server: add new system privileges for observability Aug 4, 2022

Santamaura force-pushed the new-obs-privileges-roles branch from cc5f81e to 3f7d07c Compare August 4, 2022 15:26

Santamaura marked this pull request as ready for review August 4, 2022 15:39

Santamaura force-pushed the new-obs-privileges-roles branch from 3f7d07c to 8cf565d Compare August 8, 2022 20:51

RichardJCai reviewed Aug 8, 2022

View reviewed changes

Santamaura force-pushed the new-obs-privileges-roles branch from 8cf565d to 71f0298 Compare August 9, 2022 14:14

Santamaura commented Aug 9, 2022

View reviewed changes

Santamaura requested a review from RichardJCai August 9, 2022 17:51

RichardJCai approved these changes Aug 9, 2022

View reviewed changes

RichardJCai reviewed Aug 9, 2022

View reviewed changes

Santamaura commented Aug 9, 2022

View reviewed changes

RichardJCai reviewed Aug 9, 2022

View reviewed changes

Santamaura commented Aug 9, 2022

View reviewed changes

RichardJCai reviewed Aug 9, 2022

View reviewed changes

craig bot merged commit 8bcd0cd into cockroachdb:master Aug 9, 2022

cockroach-teamcity mentioned this pull request Aug 9, 2022

sql, server: add new system privileges cockroachdb/docs#14766

Closed

Santamaura deleted the new-obs-privileges-roles branch August 10, 2022 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql, server: add new system privileges for observability #85280

sql, server: add new system privileges for observability #85280

Santamaura commented Jul 28, 2022 •

edited

Loading

cockroach-teamcity commented Jul 28, 2022

Santamaura commented Aug 3, 2022 •

edited

Loading

RichardJCai left a comment

RichardJCai Aug 8, 2022

RichardJCai Aug 8, 2022

Santamaura left a comment

RichardJCai left a comment

RichardJCai Aug 9, 2022

RichardJCai Aug 9, 2022

Santamaura left a comment

RichardJCai left a comment

Santamaura left a comment

RichardJCai left a comment •

edited

Loading

RichardJCai left a comment

Santamaura commented Aug 9, 2022

Santamaura commented Aug 9, 2022

craig bot commented Aug 9, 2022

sql, server: add new system privileges for observability #85280

sql, server: add new system privileges for observability #85280

Conversation

Santamaura commented Jul 28, 2022 • edited Loading

cockroach-teamcity commented Jul 28, 2022

Santamaura commented Aug 3, 2022 • edited Loading

RichardJCai left a comment

Choose a reason for hiding this comment

RichardJCai Aug 8, 2022

Choose a reason for hiding this comment

RichardJCai Aug 8, 2022

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

RichardJCai Aug 9, 2022

Choose a reason for hiding this comment

RichardJCai Aug 9, 2022

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

RichardJCai left a comment • edited Loading

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

Santamaura commented Aug 9, 2022

Santamaura commented Aug 9, 2022

craig bot commented Aug 9, 2022

Santamaura commented Jul 28, 2022 •

edited

Loading

Santamaura commented Aug 3, 2022 •

edited

Loading

RichardJCai left a comment •

edited

Loading