From 7a2fc923dfa6d60b3cbc5363819c93b2a7d457fd Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 29 Jul 2024 17:28:35 +0100 Subject: [PATCH] Don't update RBAC status on Grafana server error (#4753) # What this PR does Fixes a bug when RBAC permissions are getting erased when Grafana's API returns a 5xx server error on organization sync. ## Which issue(s) this PR closes Closes https://github.com/grafana/oncall-private/issues/2834 ## 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 | 4 +-- .../tests/test_grafana_api_client.py | 15 ++++++---- .../tests/test_self_hosted_install.py | 4 +-- .../views/self_hosted_install.py | 2 +- engine/apps/user_management/sync.py | 30 +++++-------------- .../apps/user_management/tests/test_sync.py | 29 ++++++++++++------ 6 files changed, 42 insertions(+), 42 deletions(-) diff --git a/engine/apps/grafana_plugin/helpers/client.py b/engine/apps/grafana_plugin/helpers/client.py index 6a0cdc89..8eec042e 100644 --- a/engine/apps/grafana_plugin/helpers/client.py +++ b/engine/apps/grafana_plugin/helpers/client.py @@ -241,9 +241,9 @@ class GrafanaAPIClient(APIClient): return all_users_permissions - def is_rbac_enabled_for_organization(self) -> bool: + def is_rbac_enabled_for_organization(self) -> tuple[bool, bool]: _, resp_status = self.api_head(self.USER_PERMISSION_ENDPOINT) - return resp_status["connected"] + return resp_status["connected"], resp_status["status_code"] >= status.HTTP_500_INTERNAL_SERVER_ERROR def get_users(self, rbac_is_enabled_for_org: bool, **kwargs) -> GrafanaUsersWithPermissions: users_response, _ = self.api_get("api/org/users", **kwargs) diff --git a/engine/apps/grafana_plugin/tests/test_grafana_api_client.py b/engine/apps/grafana_plugin/tests/test_grafana_api_client.py index 65e83504..4e72df10 100644 --- a/engine/apps/grafana_plugin/tests/test_grafana_api_client.py +++ b/engine/apps/grafana_plugin/tests/test_grafana_api_client.py @@ -1,6 +1,7 @@ from unittest.mock import patch import pytest +from rest_framework import status from apps.grafana_plugin.helpers.client import GrafanaAPIClient @@ -115,17 +116,21 @@ class TestGetUsers: class TestIsRbacEnabledForOrganization: @pytest.mark.parametrize( - "api_response_connected,expected", + "api_response_connected,api_status_code,expected", [ - (True, True), - (False, False), + (True, status.HTTP_200_OK, (True, False)), + (False, status.HTTP_404_NOT_FOUND, (False, False)), + (False, status.HTTP_503_SERVICE_UNAVAILABLE, (False, True)), ], ) @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_head") def test_it_returns_based_on_status_code_of_head_call( - self, mocked_grafana_api_client_api_head, api_response_connected, expected + self, mocked_grafana_api_client_api_head, api_response_connected, api_status_code, expected ): - mocked_grafana_api_client_api_head.return_value = (None, {"connected": api_response_connected}) + mocked_grafana_api_client_api_head.return_value = ( + None, + {"connected": api_response_connected, "status_code": api_status_code}, + ) api_client = GrafanaAPIClient(API_URL, API_TOKEN) assert api_client.is_rbac_enabled_for_organization() == expected diff --git a/engine/apps/grafana_plugin/tests/test_self_hosted_install.py b/engine/apps/grafana_plugin/tests/test_self_hosted_install.py index b91d1174..81144866 100644 --- a/engine/apps/grafana_plugin/tests/test_self_hosted_install.py +++ b/engine/apps/grafana_plugin/tests/test_self_hosted_install.py @@ -100,7 +100,7 @@ def test_if_organization_exists_it_is_updated( mocked_provision_plugin.return_value = provision_plugin_response mocked_grafana_api_client.return_value.check_token.return_value = (None, {"status_code": status.HTTP_200_OK}) - mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True + mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True, False client = APIClient() url = reverse("grafana-plugin:self-hosted-install") @@ -141,7 +141,7 @@ def test_if_organization_does_not_exist_it_is_created( mocked_provision_plugin.return_value = provision_plugin_response mocked_grafana_api_client.return_value.check_token.return_value = (None, {"status_code": status.HTTP_200_OK}) - mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True + mocked_grafana_api_client.return_value.is_rbac_enabled_for_organization.return_value = True, False client = APIClient() url = reverse("grafana-plugin:self-hosted-install") diff --git a/engine/apps/grafana_plugin/views/self_hosted_install.py b/engine/apps/grafana_plugin/views/self_hosted_install.py index 31486487..88dd8023 100644 --- a/engine/apps/grafana_plugin/views/self_hosted_install.py +++ b/engine/apps/grafana_plugin/views/self_hosted_install.py @@ -43,7 +43,7 @@ class SelfHostedInstallView(GrafanaHeadersMixin, APIView): return Response(data=provisioning_info, status=status.HTTP_400_BAD_REQUEST) organization = Organization.objects.filter(stack_id=stack_id, org_id=org_id).first() - rbac_is_enabled = grafana_api_client.is_rbac_enabled_for_organization() + rbac_is_enabled, _ = grafana_api_client.is_rbac_enabled_for_organization() if organization: organization.revoke_plugin() diff --git a/engine/apps/user_management/sync.py b/engine/apps/user_management/sync.py index 6000a9d1..42b83835 100644 --- a/engine/apps/user_management/sync.py +++ b/engine/apps/user_management/sync.py @@ -28,30 +28,14 @@ def sync_organization(organization: Organization) -> None: def _sync_organization(organization: Organization) -> None: grafana_api_client = GrafanaAPIClient(api_url=organization.grafana_url, api_token=organization.api_token) - rbac_is_enabled = organization.is_rbac_permissions_enabled + gcom_client = GcomAPIClient(settings.GRAFANA_COM_ADMIN_API_TOKEN) - # NOTE: checking whether or not RBAC is enabled depends on whether we are dealing with an open-source or cloud - # stack. For open-source, simply make a HEAD request to the grafana instance's API and consider RBAC enabled if - # the list RBAC permissions endpoint returns 200. - # - # For cloud, we need to check the stack's status first. If the stack is active, we can make the same HEAD request - # to the grafana instance's API. If the stack is not active, we will simply rely on the org's previous state of - # org.is_rbac_permissions_enabled - if settings.LICENSE == settings.CLOUD_LICENSE_NAME: - # We cannot simply rely on the HEAD call in cloud because if an instance is not active - # the grafana gateway will still return 200 for the HEAD request. - stack_id = organization.stack_id - gcom_client = GcomAPIClient(settings.GRAFANA_COM_ADMIN_API_TOKEN) - - if gcom_client.is_stack_active(stack_id): - # the stack MUST be active for this check.. if it is in any other state - # the Grafana API risks returning an HTTP 200 but the actual permissions data that is - # synced later on will be empty (and we'd erase all RBAC permissions stored in OnCall) - rbac_is_enabled = grafana_api_client.is_rbac_enabled_for_organization() - else: - rbac_is_enabled = grafana_api_client.is_rbac_enabled_for_organization() - - organization.is_rbac_permissions_enabled = rbac_is_enabled + # Update organization's RBAC status if it's an open-source instance, or it's an active cloud instance. + # Don't update non-active cloud instances (e.g. paused) as they can return 200 OK but not have RBAC enabled. + if settings.LICENSE == settings.OPEN_SOURCE_LICENSE_NAME or gcom_client.is_stack_active(organization.stack_id): + rbac_enabled, server_error = grafana_api_client.is_rbac_enabled_for_organization() + if not server_error: # Only update RBAC status if Grafana didn't return a server error + organization.is_rbac_permissions_enabled = rbac_enabled logger.info(f"RBAC status org={organization.pk} rbac_enabled={organization.is_rbac_permissions_enabled}") _sync_instance_info(organization) diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index 3cfd875a..afa889e7 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -22,7 +22,7 @@ MOCK_GRAFANA_INCIDENT_BACKEND_URL = "https://grafana-incident.test" @contextmanager -def patched_grafana_api_client(organization, is_rbac_enabled_for_organization=False): +def patched_grafana_api_client(organization, is_rbac_enabled_for_organization=(False, False)): GRAFANA_INCIDENT_PLUGIN_BACKEND_URL_KEY = "backendUrl" with patch("apps.user_management.sync.GrafanaAPIClient") as mock_grafana_api_client: @@ -262,29 +262,40 @@ def test_sync_organization(mocked_org_sync_signal, make_organization): mocked_org_sync_signal.send.assert_called_once_with(sender=None, organization=organization) -@pytest.mark.parametrize("is_rbac_enabled_for_organization", [False, True]) +@pytest.mark.parametrize( + "is_rbac_enabled_for_organization,expected", + [ + ((False, False), False), + ((True, False), True), + ((True, True), False), + ], +) @override_settings(LICENSE=settings.OPEN_SOURCE_LICENSE_NAME) @pytest.mark.django_db -def test_sync_organization_is_rbac_permissions_enabled_open_source(make_organization, is_rbac_enabled_for_organization): - organization = make_organization() +def test_sync_organization_is_rbac_permissions_enabled_open_source( + make_organization, is_rbac_enabled_for_organization, expected +): + organization = make_organization(is_rbac_permissions_enabled=False) with patched_grafana_api_client(organization, is_rbac_enabled_for_organization): sync_organization(organization) organization.refresh_from_db() - assert organization.is_rbac_permissions_enabled == is_rbac_enabled_for_organization + assert organization.is_rbac_permissions_enabled == expected @pytest.mark.parametrize( "gcom_api_response,grafana_api_response,org_initial_value,org_is_rbac_permissions_enabled_expected_value", [ # stack is in an inactive state, rely on org's previous state of is_rbac_permissions_enabled - (False, False, False, False), - (False, False, True, True), + (False, (False, False), False, False), + (False, (False, False), True, True), # stack is active, Grafana API tells us RBAC is not enabled - (True, False, True, False), + (True, (False, False), True, False), # stack is active, Grafana API tells us RBAC is enabled - (True, True, False, True), + (True, (True, False), False, True), + # stack is active, Grafana API returns error + (True, (False, True), True, True), ], ) @patch("apps.user_management.sync.GcomAPIClient")