server, sql: add VIEWCLUSTERSETTING user privilege

[koorosh]

Before, only users with admin role or MODIFYCLUSTERSETTING
permission could view cluster settings.
Now, new role is added to provide users view-only permission
to view cluster settings from SQL shell and in Db Console (in
Advanced debugging > Cluster settings).
This change doesn't change behavior for MODIFYCLUSTERSETTING
option, it also allows view and modify cluster settings.

Release note (sql change): new user privileges are added: VIEWCLUSTERSETTING
and NOVIEWCLUSTERSETTING that allows users to view cluster settings
only.

Resolves: #74692

[cockroach-teamcity]

This change is 

[Santamaura]

Reviewed 8 of 9 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @koorosh, and @zachlite)

---

pkg/server/admin.go, line 1702 at r1 (raw file):

	}

	if !hasModify && !hasView {

Perhaps here it would be better to check if there's no modify first so we can skip the no view check if they do have the role:

Code snippet:

if !hasModify {
	if !hasView {
		...
	}	
}

---

pkg/sql/crdb_internal.go, line 1467 at r1 (raw file):

				return err
			}
			if !hasModify && !hasView {

same as above if you think it's worth the change.

---

pkg/sql/set_cluster_setting.go, line 79 at r1 (raw file):

		return err
	}
	if action == "show" && !(hasModify || hasView) {

logic here is slightly tricky to wrap one's head around maybe refactor to do the action check first:

Code snippet:

if action == "show" {
	...
}

[koorosh]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @Santamaura, and @zachlite)

---

pkg/server/admin.go, line 1702 at r1 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

Perhaps here it would be better to check if there's no modify first so we can skip the no view check if they do have the role:

&& logical operator does the same optimization. It doesn't check next condition if previous one returns false

---

pkg/sql/crdb_internal.go, line 1467 at r1 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

same as above if you think it's worth the change.

That will work in the same way

---

pkg/sql/set_cluster_setting.go, line 79 at r1 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

logic here is slightly tricky to wrap one's head around maybe refactor to do the action check first:

I would try to avoid nested conditions as well, because it introduces additional "empty" conditional branches and might be confusing as well.
Probably it can be fixed with a good comment to describe this condition :)

[Santamaura]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @zachlite)

[dhartunian]

Reviewed 3 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @koorosh, @Santamaura, and @zachlite)

---

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

	if err != nil {
		return nil, err
	}

consider creating a helper like requireViewOrModifyClusterSetting(). although now that I look at other usages it seems tough to re-use the helper since the other code areas have their own implementations. maybe best to keep the PR simple.

---

pkg/sql/logictest/testdata/logic_test/cluster_settings, line 100 at r3 (raw file):

WHERE variable IN ('sql.defaults.default_int_size')
----
sql.defaults.default_int_size  4

I'm a bit confused about this test and the one below...is the user root here? how is it able to see the clusters settings without VIEWCLUSTERSETTING being applied? is it there by default?

[koorosh]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian, @Santamaura, and @zachlite)

---

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

Previously, dhartunian (David Hartunian) wrote…

consider creating a helper like requireViewOrModifyClusterSetting(). although now that I look at other usages it seems tough to re-use the helper since the other code areas have their own implementations. maybe best to keep the PR simple.

I'm inclined to keep it simple because it doesn't allow to verify presence of single role.

---

pkg/sql/logictest/testdata/logic_test/cluster_settings, line 100 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I'm a bit confused about this test and the one below...is the user root here? how is it able to see the clusters settings without VIEWCLUSTERSETTING being applied? is it there by default?

User has VIEWCLUSTERSETTING privilege + NOMODIFYCLUSTERSETTING (removed privilege to modify settings). It is done to ensure that user has only VIEWCLUSTERSETTING role.

Code quote:

user root

statement ok
ALTER USER testuser NOMODIFYCLUSTERSETTING

statement ok
ALTER USER testuser VIEWCLUSTERSETTING

user testuser

---

pkg/sql/logictest/testdata/logic_test/cluster_settings, line 111 at r3 (raw file):

statement ok
ALTER USER testuser NOVIEWCLUSTERSETTING

@dhartunian , Tests below validate that user without VIEWCLUSTERSETTING cannot query settings.

[dhartunian]

Reviewed 3 of 9 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @koorosh, @Santamaura, and @zachlite)

---

pkg/sql/logictest/testdata/logic_test/cluster_settings, line 100 at r3 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

User has VIEWCLUSTERSETTING privilege + NOMODIFYCLUSTERSETTING (removed privilege to modify settings). It is done to ensure that user has only VIEWCLUSTERSETTING role.

ah! I got confused. you need to be root user to set the other privileges. thanks for clarifying

[koorosh]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)