Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-23.1: server: allow users with VIEWACTIVITY to get console settings #108780

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Aug 15, 2023

Backport 1/1 commits from #108486.

/cc @cockroachdb/release


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.


Release justification: Bug fix

@maryliag maryliag requested review from a team August 15, 2023 16:20
@maryliag maryliag requested review from a team as code owners August 15, 2023 16:20
@maryliag maryliag requested review from abarganier and removed request for a team August 15, 2023 16:20
@blathers-crl
Copy link

blathers-crl bot commented Aug 15, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl
Copy link

blathers-crl bot commented Aug 15, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@maryliag maryliag requested review from a team and removed request for a team August 15, 2023 16:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@abarganier abarganier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from an obs-infra perspective, but will leave the final approval to cluster-obs

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@maryliag maryliag force-pushed the backport23.1-108486 branch from ae90d74 to 7984b94 Compare August 16, 2023 17:22
@maryliag maryliag requested a review from a team as a code owner August 16, 2023 17:22
@maryliag maryliag requested review from a team, rachitgsrivastava and DarrylWong and removed request for a team August 16, 2023 17:22
@maryliag maryliag force-pushed the backport23.1-108486 branch from 7984b94 to c61025a Compare August 16, 2023 17:24
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 cockroachdb#108373

Release note (bug fix): Users with VIEWACTIVITY can now view
correct values for timezone.
@maryliag maryliag force-pushed the backport23.1-108486 branch from c61025a to 94d2ec4 Compare August 16, 2023 19:21
Copy link
Contributor

@gtr gtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)

@maryliag maryliag merged commit 7128760 into cockroachdb:release-23.1 Aug 16, 2023
@maryliag maryliag deleted the backport23.1-108486 branch August 16, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants