From 66925366a669c378d7c6ebcacecaa102fb113b10 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 22 Aug 2024 15:56:59 -0300 Subject: [PATCH 1/7] Fix flaky test depending on DB ordering (#4907) See https://github.com/grafana/oncall/actions/runs/10513279782/job/29128524241 --- engine/apps/grafana_plugin/tests/test_sync.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/apps/grafana_plugin/tests/test_sync.py b/engine/apps/grafana_plugin/tests/test_sync.py index ddf93d3704..6c33d700f0 100644 --- a/engine/apps/grafana_plugin/tests/test_sync.py +++ b/engine/apps/grafana_plugin/tests/test_sync.py @@ -1,4 +1,4 @@ -from unittest.mock import patch +from unittest.mock import ANY, patch import pytest from django.conf import settings @@ -58,8 +58,8 @@ def test_start_sync_organization_filter(make_organization): with patch("apps.grafana_plugin.tasks.sync.sync_organization_async.apply_async") as mock_sync: start_sync_organizations() assert mock_sync.call_count == 2 - mock_sync.assert_any_call((org2.pk,), countdown=0) - mock_sync.assert_any_call((org3.pk,), countdown=1) + mock_sync.assert_any_call((org2.pk,), countdown=ANY) + mock_sync.assert_any_call((org3.pk,), countdown=ANY) @pytest.mark.django_db From 042fb49aafe3f5ea69354c8eacce622d7a9d1a24 Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Thu, 22 Aug 2024 14:40:18 -0600 Subject: [PATCH 2/7] Add logging for invalid api_tokens during sync (#4905) # What this PR does Add logging for when we skip an organization for sync if it is missing its api token. ## Which issue(s) this PR closes Related to [issue link here] ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- engine/apps/grafana_plugin/tasks/sync_v2.py | 21 ++++++++------ .../apps/grafana_plugin/tests/test_sync_v2.py | 28 +++++++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/engine/apps/grafana_plugin/tasks/sync_v2.py b/engine/apps/grafana_plugin/tasks/sync_v2.py index 973322c7d1..7f2d9bcfcb 100644 --- a/engine/apps/grafana_plugin/tasks/sync_v2.py +++ b/engine/apps/grafana_plugin/tasks/sync_v2.py @@ -41,14 +41,17 @@ def sync_organizations_v2(org_ids=None): orgs_per_second = math.ceil(len(organization_qs) / SYNC_PERIOD.seconds) logger.info(f"Syncing {len(organization_qs)} organizations @ {orgs_per_second} per 1s pause") for idx, org in enumerate(organization_qs): - client = GrafanaAPIClient(api_url=org.grafana_url, api_token=org.api_token) - _, status = client.sync() - if status["status_code"] != 200: - logger.error( - f"Failed to request sync stack_slug={org.stack_slug} status_code={status['status_code']} url={status['url']} message={status['message']}" - ) - if idx % orgs_per_second == 0: - logger.info(f"Sleep 1s after {idx + 1} organizations processed") - sleep(1) + if org.api_token: + client = GrafanaAPIClient(api_url=org.grafana_url, api_token=org.api_token) + _, status = client.sync() + if status["status_code"] != 200: + logger.error( + f"Failed to request sync stack_slug={org.stack_slug} status_code={status['status_code']} url={status['url']} message={status['message']}" + ) + if idx % orgs_per_second == 0: + logger.info(f"Sleep 1s after {idx + 1} organizations processed") + sleep(1) + else: + logger.info(f"Skipping stack_slug={org.stack_slug}, api_token is not set") else: logger.info(f"Issuing sync requests already in progress lock_id={lock_id}, check slow outgoing requests") diff --git a/engine/apps/grafana_plugin/tests/test_sync_v2.py b/engine/apps/grafana_plugin/tests/test_sync_v2.py index d84de38a94..0c118da4b0 100644 --- a/engine/apps/grafana_plugin/tests/test_sync_v2.py +++ b/engine/apps/grafana_plugin/tests/test_sync_v2.py @@ -6,6 +6,7 @@ from rest_framework.test import APIClient from apps.api.permissions import LegacyAccessControlRole +from apps.grafana_plugin.tasks import sync_organizations_v2 @pytest.mark.django_db @@ -44,3 +45,30 @@ def test_invalid_auth(make_organization_and_user_with_plugin_token, make_user_au assert response.status_code == status.HTTP_401_UNAUTHORIZED assert not mock_sync.called + + +@pytest.mark.parametrize( + "api_token, sync_called", + [ + ("", False), + ("abc", True), + ], +) +@pytest.mark.django_db +def test_skip_org_without_api_token(make_organization, api_token, sync_called): + organization = make_organization(api_token=api_token) + + with patch( + "apps.grafana_plugin.helpers.GrafanaAPIClient.sync", + return_value=( + None, + { + "url": "", + "connected": True, + "status_code": status.HTTP_200_OK, + "message": "", + }, + ), + ) as mock_sync: + sync_organizations_v2(org_ids=[organization.id]) + assert mock_sync.called == sync_called From 5f5eefbc53543a340cfbf12902d933fae1c66c9e Mon Sep 17 00:00:00 2001 From: Dominik Broj Date: Fri, 23 Aug 2024 09:47:04 +0200 Subject: [PATCH 3/7] optionally prefix oncall api path with grafana sub url (#4910) # What this PR does consider the grafanaSubUrl in case Grafana is served from subpath ## Which issue(s) this PR closes Related to https://github.com/grafana/oncall-private/issues/2656 https://github.com/grafana/oncall/issues/4850 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- grafana-plugin/src/utils/consts.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/grafana-plugin/src/utils/consts.ts b/grafana-plugin/src/utils/consts.ts index d21d683824..98d72dd657 100644 --- a/grafana-plugin/src/utils/consts.ts +++ b/grafana-plugin/src/utils/consts.ts @@ -72,7 +72,18 @@ export const getProcessEnvVarSafely = (name: string) => { } }; -export const getOnCallApiPath = (subpath = '') => `/api/plugins/${PLUGIN_ID}/resources${subpath}`; +const getGrafanaSubUrl = () => { + try { + return window.grafanaBootData.settings.appSubUrl || ''; + } catch (_err) { + return ''; + } +}; + +export const getOnCallApiPath = (subpath = '') => { + // We need to consider the grafanaSubUrl in case Grafana is served from subpath, e.g. http://localhost:3000/grafana + return `${getGrafanaSubUrl()}/api/plugins/${PLUGIN_ID}/resources${subpath}`; +}; // Faro export const FARO_ENDPOINT_DEV = From b2b64da86c50a7126b4cdf57ad96fd91fcae4901 Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Fri, 23 Aug 2024 07:22:54 -0600 Subject: [PATCH 4/7] Fix api_token not being updated (#4912) # What this PR does Fixes organization api_token not being updated when it differs from what is stored in the DB. ## Which issue(s) this PR closes Related to [issue link here] ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- engine/apps/grafana_plugin/helpers/gcom.py | 1 + engine/apps/grafana_plugin/tests/test_gcom.py | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 engine/apps/grafana_plugin/tests/test_gcom.py diff --git a/engine/apps/grafana_plugin/helpers/gcom.py b/engine/apps/grafana_plugin/helpers/gcom.py index 185538abb7..8d8afe8dd4 100644 --- a/engine/apps/grafana_plugin/helpers/gcom.py +++ b/engine/apps/grafana_plugin/helpers/gcom.py @@ -86,6 +86,7 @@ def check_gcom_permission(token_string: str, context) -> GcomToken: "gcom_token", "gcom_token_org_last_time_synced", "cluster_slug", + "api_token", ] ) logger.debug(f"Finish authenticate by making request to gcom api for org={org_id}, stack_id={stack_id}") diff --git a/engine/apps/grafana_plugin/tests/test_gcom.py b/engine/apps/grafana_plugin/tests/test_gcom.py new file mode 100644 index 0000000000..e5524e7a03 --- /dev/null +++ b/engine/apps/grafana_plugin/tests/test_gcom.py @@ -0,0 +1,47 @@ +from unittest.mock import patch + +import pytest + +from apps.grafana_plugin.helpers.gcom import check_gcom_permission + + +@pytest.mark.django_db +def test_check_gcom_permission_updates_fields(make_organization): + gcom_token = "gcom:test_token" + fixed_token = "fixed_token" + instance_info = { + "id": 324534, + "slug": "testinstance", + "url": "http://example.com", + "orgId": 5671, + "orgSlug": "testorg", + "orgName": "Test Org", + "regionSlug": "us", + "clusterSlug": "us-test", + } + context = { + "stack_id": str(instance_info["id"]), + "org_id": str(instance_info["orgId"]), + "grafana_token": fixed_token, + } + + org = make_organization(stack_id=instance_info["id"], org_id=instance_info["orgId"], api_token="broken_token") + + with patch( + "apps.grafana_plugin.helpers.GcomAPIClient.get_instance_info", + return_value=instance_info, + ) as mock_instance_info: + check_gcom_permission(gcom_token, context) + mock_instance_info.assert_called() + + org.refresh_from_db() + assert org.stack_id == instance_info["id"] + assert org.stack_slug == instance_info["slug"] + assert org.grafana_url == instance_info["url"] + assert org.org_id == instance_info["orgId"] + assert org.org_slug == instance_info["orgSlug"] + assert org.org_title == instance_info["orgName"] + assert org.region_slug == instance_info["regionSlug"] + assert org.cluster_slug == instance_info["clusterSlug"] + assert org.api_token == fixed_token + assert org.gcom_token == gcom_token From fefa9d1106697c3bc5d3a92686644ace8d6e4990 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 23 Aug 2024 11:49:19 -0400 Subject: [PATCH 5/7] update go toolchain version to use the 1.N.P syntax (#4877) # What this PR does Fixes [this CodeQL warning](https://github.com/grafana/oncall/security/code-scanning/tools/CodeQL/status/configurations/automatic/69f48dd4390b44ab31e994b324fcb2b55dc7b51c7542feb4e14e5a6a5b7f869b): Screenshot 2024-08-20 at 15 10 42 [Go docs](https://go.dev/doc/toolchain#version) on 1.N.P syntax versioning ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- grafana-plugin/go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grafana-plugin/go.mod b/grafana-plugin/go.mod index a905ffd3eb..f059ec7554 100644 --- a/grafana-plugin/go.mod +++ b/grafana-plugin/go.mod @@ -1,6 +1,6 @@ module github.com/grafana-labs/grafana-oncall-app -go 1.21 +go 1.21.5 require github.com/grafana/grafana-plugin-sdk-go v0.228.0 From a5770309d9bbd91d2f5182cc5dbf4f4eee68057f Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Fri, 23 Aug 2024 12:21:57 -0600 Subject: [PATCH 6/7] Add more validation when updating api_token (#4918) # What this PR does Skip updating stored api_token for Grafana if it does not look like one. Note: Exact format is not checked (prefix) since there are some differences between versions for what API tokens might look like and this should tolerate those differences. ## Which issue(s) this PR closes Related to [issue link here] ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- engine/apps/grafana_plugin/helpers/gcom.py | 23 +++++++++++++++++- engine/apps/grafana_plugin/tests/test_gcom.py | 24 +++++++++++++++---- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/engine/apps/grafana_plugin/helpers/gcom.py b/engine/apps/grafana_plugin/helpers/gcom.py index 8d8afe8dd4..e327c7f7b1 100644 --- a/engine/apps/grafana_plugin/helpers/gcom.py +++ b/engine/apps/grafana_plugin/helpers/gcom.py @@ -12,6 +12,7 @@ logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) GCOM_TOKEN_CHECK_PERIOD = timezone.timedelta(minutes=60) +MIN_GRAFANA_TOKEN_LENGTH = 16 class GcomToken: @@ -19,6 +20,14 @@ def __init__(self, organization): self.organization = organization +def _validate_grafana_token_format(grafana_token: str) -> bool: + if not grafana_token or not isinstance(grafana_token, str): + return False + if len(grafana_token) < MIN_GRAFANA_TOKEN_LENGTH: + return False + return True + + def check_gcom_permission(token_string: str, context) -> GcomToken: """ Verify that request from plugin is valid. Check it and synchronize the organization details @@ -45,6 +54,8 @@ def check_gcom_permission(token_string: str, context) -> GcomToken: if not instance_info or str(instance_info["orgId"]) != org_id: raise InvalidToken + grafana_token_format_is_valid = _validate_grafana_token_format(grafana_token) + if not organization: from apps.base.models import DynamicSetting @@ -52,6 +63,11 @@ def check_gcom_permission(token_string: str, context) -> GcomToken: name="allow_plugin_organization_signup", defaults={"boolean_value": True} )[0].boolean_value if allow_signup: + if not grafana_token_format_is_valid: + logger.debug( + f"grafana token sent when creating stack_id={stack_id} was invalid format. api_token will still be written to DB" + ) + # Get org from db or create a new one organization, _ = Organization.objects.get_or_create( stack_id=instance_info["id"], @@ -74,8 +90,13 @@ def check_gcom_permission(token_string: str, context) -> GcomToken: organization.grafana_url = instance_info["url"] organization.cluster_slug = instance_info["clusterSlug"] organization.gcom_token = token_string - organization.api_token = grafana_token organization.gcom_token_org_last_time_synced = timezone.now() + if not grafana_token_format_is_valid: + logger.debug( + f"grafana token sent when updating stack_id={stack_id} was invalid, api_token in DB will be unchanged" + ) + else: + organization.api_token = grafana_token organization.save( update_fields=[ "stack_slug", diff --git a/engine/apps/grafana_plugin/tests/test_gcom.py b/engine/apps/grafana_plugin/tests/test_gcom.py index e5524e7a03..c70f0719e4 100644 --- a/engine/apps/grafana_plugin/tests/test_gcom.py +++ b/engine/apps/grafana_plugin/tests/test_gcom.py @@ -5,10 +5,22 @@ from apps.grafana_plugin.helpers.gcom import check_gcom_permission +@pytest.mark.parametrize( + "api_token, api_token_updated", + [ + ("glsa_abcdefghijklmnopqrztuvwxyz", True), + ("abcdefghijklmnopqrztuvwxyz", True), + ("abc", False), + ("", False), + ("", False), + (None, False), + (24, False), + ], +) @pytest.mark.django_db -def test_check_gcom_permission_updates_fields(make_organization): +def test_check_gcom_permission_updates_fields(make_organization, api_token, api_token_updated): gcom_token = "gcom:test_token" - fixed_token = "fixed_token" + broken_token = "broken_token" instance_info = { "id": 324534, "slug": "testinstance", @@ -22,10 +34,11 @@ def test_check_gcom_permission_updates_fields(make_organization): context = { "stack_id": str(instance_info["id"]), "org_id": str(instance_info["orgId"]), - "grafana_token": fixed_token, + "grafana_token": api_token, } - org = make_organization(stack_id=instance_info["id"], org_id=instance_info["orgId"], api_token="broken_token") + org = make_organization(stack_id=instance_info["id"], org_id=instance_info["orgId"], api_token=broken_token) + last_time_gcom_synced = org.gcom_token_org_last_time_synced with patch( "apps.grafana_plugin.helpers.GcomAPIClient.get_instance_info", @@ -43,5 +56,6 @@ def test_check_gcom_permission_updates_fields(make_organization): assert org.org_title == instance_info["orgName"] assert org.region_slug == instance_info["regionSlug"] assert org.cluster_slug == instance_info["clusterSlug"] - assert org.api_token == fixed_token + assert org.api_token == api_token if api_token_updated else broken_token assert org.gcom_token == gcom_token + assert org.gcom_token_org_last_time_synced != last_time_gcom_synced From a25d44da1ae26715bc7b865a027fdc6701e36a76 Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Fri, 23 Aug 2024 13:52:53 -0600 Subject: [PATCH 7/7] Move validate_grafana_token_format to common location, use in sync_v2 (#4919) # What this PR does Moves validate_grafana_token_format to GrafanaAPIClient, use it in sync_v2 to improve logging and skip requests that would not work. ## Which issue(s) this PR closes Related to [issue link here] ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --- engine/apps/grafana_plugin/helpers/client.py | 10 ++++++++++ engine/apps/grafana_plugin/helpers/gcom.py | 13 ++----------- engine/apps/grafana_plugin/tasks/sync_v2.py | 4 ++-- engine/apps/grafana_plugin/tests/test_sync_v2.py | 3 ++- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index 3d54cd0d87..4ed9c34692 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -166,6 +166,8 @@ class GrafanaAPIClient(APIClient): USER_PERMISSION_ENDPOINT = f"api/access-control/users/permissions/search?actionPrefix={ACTION_PREFIX}" + MIN_GRAFANA_TOKEN_LENGTH = 16 + class Types: class _BaseGrafanaAPIResponse(typing.TypedDict): totalCount: int @@ -330,6 +332,14 @@ def get_service_account_token_permissions(self) -> APIClientResponse[typing.Dict def sync(self) -> APIClientResponse: return self.api_post("api/plugins/grafana-oncall-app/resources/plugin/sync") + @staticmethod + def validate_grafana_token_format(grafana_token: str) -> bool: + if not grafana_token or not isinstance(grafana_token, str): + return False + if len(grafana_token) < GrafanaAPIClient.MIN_GRAFANA_TOKEN_LENGTH: + return False + return True + class GcomAPIClient(APIClient): ACTIVE_INSTANCE_QUERY = "instances?status=active" diff --git a/engine/apps/grafana_plugin/helpers/gcom.py b/engine/apps/grafana_plugin/helpers/gcom.py index e327c7f7b1..e83ab9696a 100644 --- a/engine/apps/grafana_plugin/helpers/gcom.py +++ b/engine/apps/grafana_plugin/helpers/gcom.py @@ -6,13 +6,12 @@ from apps.auth_token.exceptions import InvalidToken from apps.auth_token.models import PluginAuthToken -from apps.grafana_plugin.helpers import GcomAPIClient +from apps.grafana_plugin.helpers import GcomAPIClient, GrafanaAPIClient from apps.user_management.models import Organization logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) GCOM_TOKEN_CHECK_PERIOD = timezone.timedelta(minutes=60) -MIN_GRAFANA_TOKEN_LENGTH = 16 class GcomToken: @@ -20,14 +19,6 @@ def __init__(self, organization): self.organization = organization -def _validate_grafana_token_format(grafana_token: str) -> bool: - if not grafana_token or not isinstance(grafana_token, str): - return False - if len(grafana_token) < MIN_GRAFANA_TOKEN_LENGTH: - return False - return True - - def check_gcom_permission(token_string: str, context) -> GcomToken: """ Verify that request from plugin is valid. Check it and synchronize the organization details @@ -54,7 +45,7 @@ def check_gcom_permission(token_string: str, context) -> GcomToken: if not instance_info or str(instance_info["orgId"]) != org_id: raise InvalidToken - grafana_token_format_is_valid = _validate_grafana_token_format(grafana_token) + grafana_token_format_is_valid = GrafanaAPIClient.validate_grafana_token_format(grafana_token) if not organization: from apps.base.models import DynamicSetting diff --git a/engine/apps/grafana_plugin/tasks/sync_v2.py b/engine/apps/grafana_plugin/tasks/sync_v2.py index 7f2d9bcfcb..0f351e305a 100644 --- a/engine/apps/grafana_plugin/tasks/sync_v2.py +++ b/engine/apps/grafana_plugin/tasks/sync_v2.py @@ -41,7 +41,7 @@ def sync_organizations_v2(org_ids=None): orgs_per_second = math.ceil(len(organization_qs) / SYNC_PERIOD.seconds) logger.info(f"Syncing {len(organization_qs)} organizations @ {orgs_per_second} per 1s pause") for idx, org in enumerate(organization_qs): - if org.api_token: + if GrafanaAPIClient.validate_grafana_token_format(org.api_token): client = GrafanaAPIClient(api_url=org.grafana_url, api_token=org.api_token) _, status = client.sync() if status["status_code"] != 200: @@ -52,6 +52,6 @@ def sync_organizations_v2(org_ids=None): logger.info(f"Sleep 1s after {idx + 1} organizations processed") sleep(1) else: - logger.info(f"Skipping stack_slug={org.stack_slug}, api_token is not set") + logger.info(f"Skipping stack_slug={org.stack_slug}, api_token format is invalid or not set") else: logger.info(f"Issuing sync requests already in progress lock_id={lock_id}, check slow outgoing requests") diff --git a/engine/apps/grafana_plugin/tests/test_sync_v2.py b/engine/apps/grafana_plugin/tests/test_sync_v2.py index 0c118da4b0..5fd67fdd99 100644 --- a/engine/apps/grafana_plugin/tests/test_sync_v2.py +++ b/engine/apps/grafana_plugin/tests/test_sync_v2.py @@ -51,7 +51,8 @@ def test_invalid_auth(make_organization_and_user_with_plugin_token, make_user_au "api_token, sync_called", [ ("", False), - ("abc", True), + ("abc", False), + ("glsa_abcdefghijklmnopqrstuvwxyz", True), ], ) @pytest.mark.django_db