server: allow users with VIEWACTIVITY to get console settings

[maryliag]

Previously, only users with ADMIN, VIEWCLUSTERSETTING or MODIFYCLUSTERSETTING could get the settings on the _admin/v1/settings. This API was used by the Console and then a few places retrieved that information.
Users without those permissions would not be able to see the values and some functionalities where not working as expected, for example timezone was showing as UTC even when the cluster setting ui.display_timezone was set to other value.

This commits modifies that endpoint (and that endpoint only) to return just the cluster settings that are not sensitive and that are required by the console to have all functionalities working.

The list of cluster settings:
"cross_cluster_replication.enabled",
"keyvisualizer.enabled",
"keyvisualizer.sample_interval",
"sql.index_recommendation.drop_unused_duration",
"sql.insights.anomaly_detection.latency_threshold", "sql.insights.high_retry_count.threshold",
"sql.insights.latency_threshold",
"sql.stats.automatic_collection.enabled",
"timeseries.storage.resolution_10s.ttl",
"timeseries.storage.resolution_30m.ttl",
"ui.display_timezone",
"version"

Part Of #108373
Fixes #108117

Release note (bug fix): Users with VIEWACTIVITY can now view correct values for timezone.

[cockroach-teamcity]

This change is 

[gtr]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @nkodali)

---

pkg/server/application_api/cluster_settings_test.go line 29 at r1 (raw file):

)

func TestClusterSettingsAPI(t *testing.T) {

Nice work! I just had a quick question that might be a minor thing.

In the test cases, I noticed that we're mainly focusing on single key requests, like "settings?keys=sql.stats.persisted_rows.max". Do you think it would be worthwhile to also add a few test cases where we request multiple keys at once, something like "settings?keys=key1,key2,key3"? This would help ensure that our endpoint is handling multiple key requests gracefully.

I see that there's already a similar test for this scenario in config_test.go so maybe it's not needed. Let me know what you think.

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr and @nkodali)

---

pkg/server/application_api/cluster_settings_test.go line 29 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

Nice work! I just had a quick question that might be a minor thing.

In the test cases, I noticed that we're mainly focusing on single key requests, like "settings?keys=sql.stats.persisted_rows.max". Do you think it would be worthwhile to also add a few test cases where we request multiple keys at once, something like "settings?keys=key1,key2,key3"? This would help ensure that our endpoint is handling multiple key requests gracefully.

I see that there's already a similar test for this scenario in config_test.go so maybe it's not needed. Let me know what you think.

I will leave the other tests to check for this behaviour, since the focus of this one was the permission part. But I did move the new tests to the same file, because it didn't make sense to have it separate, so at least is more clear all the tests related to this endpoint are on the same place

[gtr]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @nkodali)

---

pkg/server/application_api/cluster_settings_test.go line 29 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I will leave the other tests to check for this behaviour, since the focus of this one was the permission part. But I did move the new tests to the same file, because it didn't make sense to have it separate, so at least is more clear all the tests related to this endpoint are on the same place

Ok sounds good!

[dhartunian]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @nkodali)

---

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

		if err := s.privilegeChecker.RequireViewClusterSettingOrModifyClusterSettingPermission(ctx); err != nil {
			if err2 := s.privilegeChecker.RequireViewActivityOrViewActivityRedactedPermission(ctx); err2 != nil {
				return nil, err

nit: I think here you want to wrap the errors together or something. Otherwise err2 is lost.

---

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

			consoleSettingsOnly = true
			log.Warningf(ctx, "only cluster settings used by DB Console are returned with "+
				"VIEWACTIVITY or VIEWACTIVITYREDACTED")

This warning adds extra output the logs for something that's not "bad", right? Maybe log at Debug level if you want, but I feel like this does not rise to a level of Warning if we're implementing it.

[maryliag]

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

---

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

Previously, dhartunian (David Hartunian) wrote…

nit: I think here you want to wrap the errors together or something. Otherwise err2 is lost.

I don't really care about the err2 message here, because there is not something we want to surface to the user. The only message I want to share is one coming from err, where I say that they need the cluster settings view permission. The VIEWACTIVITY is just a special case. I create the new err2, to check if there is both err and err2, then return err with no list

I can added some comments to make this more clear

---

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

Previously, dhartunian (David Hartunian) wrote…

This warning adds extra output the logs for something that's not "bad", right? Maybe log at Debug level if you want, but I feel like this does not rise to a level of Warning if we're implementing it.

make sense, I just removed it

[maryliag]

@dhartunian friendly ping if there is any other feedback

[dhartunian]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag and @nkodali)

[maryliag]

TFTRs!
bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 2b5b762 to blathers/backport-release-23.1-108486: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

error creating merge commit from 2b5b762 to blathers/backport-release-23.1.9-rc-108486: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.9-rc failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}