sql: Introduce the concept of system cluster privileges

[RichardJCai]

System Cluster Privileges (global) privileges are the first
type of privileges that use the system.privileges table.

Release note (sql change): Introduce SYSTEM CLUSTER PRIVILEGES.
These are "global" privileges that live above the database level.

Example: GRANT SYSTEM MODIFYCLUSTERSETTING TO foo

Currently MODIFYCLUSTERSETTING is the only system cluster privilege,
it allows users to query the crdb_internal.cluster_settings table.

More functionality will be gated behind having MODIFYCLUSTERSETTING privilege
in the future. This is not the same as the MODIFYCLUSTERSETTING role option.

[cockroach-teamcity]

This change is 

[RichardJCai]

Need to add some more tests / a bit more cleanup but I think this is ready for a first pass of reviews right now.

[ecwall]

Reviewed 40 of 49 files at r1, 8 of 50 files at r3, 45 of 45 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @msbutler, @rafiss, and @RichardJCai)

---

pkg/sql/grant_revoke.go line 154 at r4 (raw file):

}

type changeNonDescriptorBackedPrivilegesNode struct {

Can you use composition here with a struct that contains the shared fields between this and changePrivilegesNode and embed it in both?

preChangePrivilegesValidation could be a method with changePrivilegesNode as a receiver.

---

pkg/sql/grant_revoke_system.go line 118 at r4 (raw file):

			systemPrivilegeObject.ToString(),
			bitarray.MakeBitArrayFromInt64(64, int64(userPrivs.Privileges), 64),
			bitarray.MakeZeroBitArray(64),

Are grant options part of the table but unused? (same question for grant above)

---

pkg/sql/catalog/catprivilege/validate.go line 68 at r4 (raw file):

	}

	if !reflect.DeepEqual(out, systemPrivilegeObject) {

Does != not work here?

---

pkg/sql/privilege/privilege.go line 46 at r4 (raw file):

	CONNECT              Kind = 11
	RULE                 Kind = 12
	MODIFYCLUSTERSETTING Kind = 13 // SYSTEM CLUSTER PRIVILEGE.

Is there a concern about adding non-standard privilege kinds here? It looks like we are limited to 32 possible values type Kind uint32.

I haven't thought about this much, but is it possible to reuse the UPDATE privilege and add it to a "cluster setting" (does something like this exist?) database object similar to table, schema, etc?

---

pkg/util/bitarray/bitarray.go line 622 at r4 (raw file):

// AsUInt64 returns the uint64 constituted from the rightmost bits in the
// bit array.
func (d BitArray) AsUInt64() uint64 {

Does having a value receiver end up copying the array?

[knz]

More functionality will be gated behind having MODIFYCLUSTERSETTING privilege
in the future. This is not the same as the MODIFYCLUSTERSETTING role option.

Sorry I missed the memo; what's the difference between the two? (Also I'm pretty sure that difference needs to be explained in a release note.)

[RichardJCai]

More functionality will be gated behind having MODIFYCLUSTERSETTING privilege
in the future. This is not the same as the MODIFYCLUSTERSETTING role option.

Sorry I missed the memo; what's the difference between the two? (Also I'm pretty sure that difference needs to be explained in a release note.)

Functionally they're going to be the same thing, the goal is to emphasize using the privilege and not the role option and in general move away from and eventually deprecate these non granular role options (the ones we added somewhat adhoc and are not in pg)

[knz]

Functionally they're going to be the same thing

if that part of the sentence is true, then the sentence "More functionality will be gated behind ..." should be moved from the release note back up higher in the commit message, outside of the release note. Because that means that change is not user-facing.

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @msbutler, and @rafiss)

---

pkg/sql/crdb_internal.go line 1564 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

would it make sense to use a singleton var for this empty catalog.SystemClusterPrivilege -- then we can name it in and comment it to include the explanation you just gave.

DOne, created catalog.SystemClusterPrivilegeObject

---

pkg/sql/catalog/privilege_object.go line 24 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

yeah constants could make sense then, i think

Added a constant for system.

---

pkg/sql/catalog/privilege_object.go line 22 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm could you walk me though again why we need this interface? why can't we just call the "synthesize system privilege descriptor" function before the point where we need to start passing around these new PrivilegeObjects?

I think it simplifies a lot of with the GetObjectType for privilege validation.
It also simplifies some of the owner code.

This is mostly so we can identify objects and unify the code paths for descriptor backed and non-descriptor backed objects. I don't think it adds much overhead here.

---

pkg/sql/catalog/systemschema/system.go line 687 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can the descriptor validation logic we already have be updated to make sure that there are only valid privlieges in this table (and validate each time we read from it)

ideally we could use an enum or CHECK constraint, but that would make adding a new privilege way too much of a pain i think, since each one would need a schema migration on a system table.

Added a validate into the synthesizePrivilegeDescriptorFromSystemPrivilegesTable method

[rafiss]

as discussed offline: let's have catalog.Descriptor implement catalog.PrivilegeObject so that it's easier to use as a parameter for existing functions

also hopefully minor nits, then lgtm

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @msbutler, @rafiss, and @RichardJCai)

---

pkg/sql/catalog/system_privilege.go line 36 at r9 (raw file):

// SystemClusterPrivilege represents a SystemClusterPrivilege.
type SystemClusterPrivilege struct {

nit: should this use a type assertion instead of embedding?

type SystemClusterPrivilege struct {}

var _ SystemPrivilegeObject = &SystemClusterPrivilege{}

(actually, i guess we wouldn't even need this because of the singleton defined below)

also i'm finding it hard to keep track of the names "SystemPrivilegeObject" versus "SystemClusterPrivilege". is there a way i should be thinking about this? why do we need both?

---

pkg/sql/privilege/privilege.go line 48 at r9 (raw file):

	CONNECT              Kind = 11
	RULE                 Kind = 12
	MODIFYCLUSTERSETTING Kind = 13 // SYSTEM CLUSTER PRIVILEGE.

is this comment correct? i believe the comment will be used for the string representation

[RichardJCai]

as discussed offline: let's have catalog.Descriptor implement catalog.PrivilegeObject so that it's easier to use as a parameter for existing functions

This was the case but forgot to update TestState

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @msbutler, and @rafiss)

---

pkg/sql/catalog/system_privilege.go line 36 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should this use a type assertion instead of embedding?

type SystemClusterPrivilege struct {}

var _ SystemPrivilegeObject = &SystemClusterPrivilege{}

(actually, i guess we wouldn't even need this because of the singleton defined below)

also i'm finding it hard to keep track of the names "SystemPrivilegeObject" versus "SystemClusterPrivilege". is there a way i should be thinking about this? why do we need both?

The mental model is SystemPrivilegeObject is a privilege object where the privileges are stored in the system table.

SystemClusterPrivilege is a type of SystemPrivilegeObject, there will be more types in the future (jobs, etc).

I can see that the naming is definitely confusing though, SystemClusterPrivilege is a "global" privilege.

Let me know if you can think of better naming solutions for these.

Also I removed the embedding since we do already have the type assertion.

---

pkg/sql/privilege/privilege.go line 48 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

is this comment correct? i believe the comment will be used for the string representation

I checked kind_string.go and that didn't seem to be the case but I'm happy to just remove this comment, it isn't adding anything.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @msbutler, and @rafiss)

---

pkg/sql/privilege/privilege.go line 48 at r9 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I checked kind_string.go and that didn't seem to be the case but I'm happy to just remove this comment, it isn't adding anything.

that's a bug because the comment from #81310 was not completely addressed

https://reviewable.io/reviews/cockroachdb/cockroach/81310#-N3R9piu3KukUO8hHuJr:-N3R9piu3KukUO8hHuJs:bd9h7vx

would you mind adding linecomment to //go:generate stringer -type=Kind above? cc @ecwall

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @ecwall, @msbutler, and @rafiss)

---

pkg/sql/privilege/privilege.go line 48 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

that's a bug because the comment from #81310 was not completely addressed

https://reviewable.io/reviews/cockroachdb/cockroach/81310#-N3R9piu3KukUO8hHuJr:-N3R9piu3KukUO8hHuJs:bd9h7vx

would you mind adding linecomment to //go:generate stringer -type=Kind above? cc @ecwall

Added it and updated the generated file.

[RichardJCai]

Failure is flake, thanks for reviewing!

bors r+

[craig]

Build failed:

• GitHub CI (Cockroach)

[RichardJCai]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)