PMM-13715: allow viewers/editors access limited PMM settings #3518

idoqo · 2025-01-29T09:28:51Z

Link to the Feature Build: SUBMODULES-3840

If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:

API Docs updated

If this PR is related to some other PRs in this or other repositories, please provide links to those PRs:

Links to related pull requests (optional).

codecov · 2025-01-29T09:39:21Z

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.

Project coverage is 43.76%. Comparing base (3112dea) to head (6c4520a).
Report is 3 commits behind head on v3.

Files with missing lines	Patch %	Lines
managed/services/server/server.go	0.00%	24 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##               v3    #3518      +/-   ##
==========================================
- Coverage   43.76%   43.76%   -0.01%     
==========================================
  Files         367      367              
  Lines       44514    44538      +24     
==========================================
+ Hits        19481    19491      +10     
- Misses      23350    23363      +13     
- Partials     1683     1684       +1

Flag	Coverage Δ
admin	`11.51% <ø> (+0.04%)`	⬆️
agent	`52.32% <ø> (+0.04%)`	⬆️
managed	`45.36% <0.00%> (-0.03%)`	⬇️
vmproxy	`73.45% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ademidoff · 2025-01-29T11:19:28Z

api/server/v1/json/v1.json

+                "details": {
+                  "type": "array",
+                  "items": {
+                    "description": "`Any` contains an arbitrary serialized protocol buffer message along with a\nURL that describes the type of the serialized message.\n\nProtobuf library provides support to pack/unpack Any values in the form\nof utility functions or additional generated methods of the Any type.\n\nExample 1: Pack and unpack a message in C++.\n\n    Foo foo = ...;\n    Any any;\n    any.PackFrom(foo);\n    ...\n    if (any.UnpackTo(\u0026foo)) {\n      ...\n    }\n\nExample 2: Pack and unpack a message in Java.\n\n    Foo foo = ...;\n    Any any = Any.pack(foo);\n    ...\n    if (any.is(Foo.class)) {\n      foo = any.unpack(Foo.class);\n    }\n    // or ...\n    if (any.isSameTypeAs(Foo.getDefaultInstance())) {\n      foo = any.unpack(Foo.getDefaultInstance());\n    }\n\n Example 3: Pack and unpack a message in Python.\n\n    foo = Foo(...)\n    any = Any()\n    any.Pack(foo)\n    ...\n    if any.Is(Foo.DESCRIPTOR):\n      any.Unpack(foo)\n      ...\n\n Example 4: Pack and unpack a message in Go\n\n     foo := \u0026pb.Foo{...}\n     any, err := anypb.New(foo)\n     if err != nil {\n       ...\n     }\n     ...\n     foo := \u0026pb.Foo{}\n     if err := any.UnmarshalTo(foo); err != nil {\n       ...\n     }\n\nThe pack methods provided by protobuf library will by default use\n'type.googleapis.com/full.type.name' as the type URL and the unpack\nmethods only use the fully qualified type name after the last '/'\nin the type URL, for example \"foo.bar.com/x/y.z\" will yield type\nname \"y.z\".\n\nJSON\n====\nThe JSON representation of an `Any` value uses the regular\nrepresentation of the deserialized, embedded message, with an\nadditional field `@type` which contains the type URL. Example:\n\n    package google.profile;\n    message Person {\n      string first_name = 1;\n      string last_name = 2;\n    }\n\n    {\n      \"@type\": \"type.googleapis.com/google.profile.Person\",\n      \"firstName\": \u003cstring\u003e,\n      \"lastName\": \u003cstring\u003e\n    }\n\nIf the embedded message type is well-known and has a custom JSON\nrepresentation, that representation will be embedded adding a field\n`value` which holds the custom JSON in addition to the `@type`\nfield. Example (for message [google.protobuf.Duration][]):\n\n    {\n      \"@type\": \"type.googleapis.com/google.protobuf.Duration\",\n      \"value\": \"1.212s\"\n    }",


I wonder, where does this come from? Does it have to be that lengthy?

Looks like it's generated for all v1/server endpoints

ademidoff · 2025-01-29T11:20:59Z

api/server/v1/server.proto

@@ -181,12 +181,42 @@ message Settings {
  uint32 default_role_id = 18;
 }

+// Settings represents a stripped-down version of PMM Server settings that can be accessed by users of all roles.


Suggested change

// Settings represents a stripped-down version of PMM Server settings that can be accessed by users of all roles.

// ReadOnlySettings represents a stripped-down version of PMM Server settings that can be accessed by users of all roles.

ademidoff · 2025-01-29T11:27:25Z

managed/services/grafana/auth_server.go

@@ -63,6 +63,7 @@ var rules = map[string]role{
 	"/v1/alerting":                    viewer,
 	"/v1/advisors":                    editor,
 	"/v1/advisors/checks:":            editor,
+	"/v1/advisors/failedServices":     editor,


ademidoff · 2025-01-29T11:34:14Z

managed/services/server/server.go

@@ -494,6 +494,35 @@ func (s *Server) convertSettings(settings *models.Settings, connectedToPlatform
 	return res
 }

+// convertReadOnlySettings merges database settings and settings from environment variables into API response.


Suggested change

// convertReadOnlySettings merges database settings and settings from environment variables into API response.

// convertReadOnlySettings created a subset of database settings for the "viewer" role.

I don't see the envvars involved here. :)

BupycHuk · 2025-01-29T11:54:47Z

api/server/v1/server.proto

+  // Percona Platform user's email, if this PMM instance is linked to the Platform.
+  string platform_email = 8;


do we really need this setting to be public?

BupycHuk · 2025-01-29T11:55:00Z

api/server/v1/server.proto

+  MetricsResolutions metrics_resolutions = 3;
+  google.protobuf.Duration data_retention = 4;


do we really need these settings to be public?

let's reserve these numbers, but not provide value yet until it's required

BupycHuk · 2025-01-29T11:55:59Z

api/server/v1/server.proto

+  string pmm_public_address = 11;
+  // Intervals between Advisor runs.
+  AdvisorRunIntervals advisor_run_intervals = 12;


do we really need these settings to be public?

Not really - they are shown on the advanced settings page, but non-admin roles can't access that page anyway - we can reserve the number as in the other cases

BupycHuk · 2025-01-29T11:56:54Z

api/server/v1/server.proto

+  // True if Azure Discover is enabled.
+  bool azurediscover_enabled = 14;
+  // True if Access Control is enabled.
+  bool enable_access_control = 17;


oh shit, it's inconsistent with other fields.

Yeah I saw too - will need to prep separate PR to update it

BupycHuk · 2025-03-03T12:26:24Z

api/server/v1/server.proto

@@ -288,6 +312,14 @@ service ServerService {
      description: "Returns current PMM Server settings."
    };
  }
+  // GetReadOnlySettings returns a limited number of PMM settings that is opened to authenticated users of all roles.
+  rpc GetReadOnlySettings(GetSettingsRequest) returns (GetReadOnlySettingsResponse) {


@idoqo please check CI linters

* add new map for failed services * add readonly settings endpoint * update method comment * drop retention data from readonly settings * drop unused fields from settings proto * use separate proto message

add new map for failed services

f3d1609

add readonly settings endpoint

76ed3ac

idoqo changed the title ~~PMM-12356 allow editor for check results~~ PMM-13715: allow viewers/editors access limited PMM settings Jan 29, 2025

update method comment

a61b152

idoqo marked this pull request as ready for review January 29, 2025 11:17

idoqo requested review from BupycHuk and a team as code owners January 29, 2025 11:17

idoqo requested review from ademidoff and JiriCtvrtka and removed request for a team January 29, 2025 11:17

ademidoff reviewed Jan 29, 2025

View reviewed changes

ademidoff approved these changes Jan 29, 2025

View reviewed changes

BupycHuk requested changes Jan 29, 2025

View reviewed changes

idoqo added 2 commits January 30, 2025 07:33

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

fc0cd43

drop retention data from readonly settings

c88f399

idoqo force-pushed the PMM-12356-allow-editor-for-check-results branch from 81319bf to c88f399 Compare January 30, 2025 06:45

idoqo requested a review from BupycHuk February 3, 2025 11:02

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

55bf0f3

idoqo requested review from a team and yurkovychv as code owners February 5, 2025 09:04

ademidoff deployed to PMM-12356-allow-editor-for-check-results - pmm-doc-3 PR #3518 February 5, 2025 09:04 — with Render View deployment

idoqo mentioned this pull request Feb 5, 2025

PMM-12356: allow editor access to check results Percona-Lab/pmm-submodules#3836

Closed

1 task

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

c129890

ademidoff deployed to PMM-12356-allow-editor-for-check-results - pmm-doc-3 PR #3518 February 11, 2025 09:13 — with Render View deployment

idoqo added 2 commits February 19, 2025 09:23

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

39a64c7

drop unused fields from settings proto

9550f77

ademidoff deployed to PMM-12356-allow-editor-for-check-results - pmm-doc-3 PR #3518 February 19, 2025 08:59 — with Render View deployment

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

9b2a6b3

ademidoff deployed to PMM-12356-allow-editor-for-check-results - pmm-doc-3 PR #3518 February 25, 2025 07:42 — with Render View deployment

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

90890ff

ademidoff temporarily deployed to PMM-12356-allow-editor-for-check-results - pmm-doc-3 PR #3518 March 3, 2025 09:29 — with Render Destroyed

BupycHuk approved these changes Mar 3, 2025

View reviewed changes

BupycHuk reviewed Mar 3, 2025

View reviewed changes

idoqo added 2 commits March 4, 2025 08:01

Merge branch 'v3' into PMM-12356-allow-editor-for-check-results

7f6e7bb

use separate proto message

6c4520a

idoqo merged commit 51d5ed3 into v3 Mar 4, 2025
32 checks passed

idoqo deleted the PMM-12356-allow-editor-for-check-results branch March 4, 2025 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMM-13715: allow viewers/editors access limited PMM settings #3518

PMM-13715: allow viewers/editors access limited PMM settings #3518

idoqo commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

ademidoff Jan 29, 2025

idoqo Jan 30, 2025

ademidoff Jan 29, 2025

ademidoff Jan 29, 2025

ademidoff Jan 29, 2025

ademidoff Jan 29, 2025

BupycHuk Jan 29, 2025

BupycHuk Jan 29, 2025

BupycHuk Jan 29, 2025

BupycHuk Jan 29, 2025

idoqo Jan 30, 2025

BupycHuk Jan 29, 2025

idoqo Jan 30, 2025

BupycHuk Mar 3, 2025

	// Settings represents a stripped-down version of PMM Server settings that can be accessed by users of all roles.
	// ReadOnlySettings represents a stripped-down version of PMM Server settings that can be accessed by users of all roles.

	// convertReadOnlySettings merges database settings and settings from environment variables into API response.
	// convertReadOnlySettings created a subset of database settings for the "viewer" role.

		// Percona Platform user's email, if this PMM instance is linked to the Platform.
		string platform_email = 8;

		MetricsResolutions metrics_resolutions = 3;
		google.protobuf.Duration data_retention = 4;

PMM-13715: allow viewers/editors access limited PMM settings #3518

PMM-13715: allow viewers/editors access limited PMM settings #3518

Conversation

idoqo commented Jan 29, 2025 • edited Loading

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idoqo commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading