patch minor sync issue when HTTP 302 is received from Grafana API instance (#1393)

# What this PR does

this PR refactors the `sync_organization` and
`GrafanaAPIClient.is_rbac_enabled_for_organization` methods to check the
connected response bool rather than explicit check on HTTP 200. This
handles the legitimate case where the Grafana instance may return an
HTTP 302 (redirect) rather than an HTTP 200.

## Which issue(s) this PR fixes

See
[this](https://grafana.slack.com/archives/C02LSUUSE2G/p1677136582890269)
Slack thread in the community channel for more context

## Checklist

- [x] Tests updated
- [ ] Documentation added (N/A)
- [x] `CHANGELOG.md` updated
This commit is contained in:
Joey Orlando 2023-02-23 14:23:57 +01:00 committed by GitHub
parent a2eed312f9
commit b61f2ce41f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 15 additions and 11 deletions

View file

@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Allow creating schedules with type "web" using public API - Allow creating schedules with type "web" using public API
### Fixed
- Fixed minor issue during the sync process where an HTTP 302 (redirect) status code from the Grafana
instance would cause the sync to not properly finish
## v1.1.28 (2023-02-23) ## v1.1.28 (2023-02-23)
### Fixed ### Fixed

View file

@ -152,7 +152,7 @@ class GrafanaAPIClient(APIClient):
def is_rbac_enabled_for_organization(self) -> bool: def is_rbac_enabled_for_organization(self) -> bool:
_, resp_status = self.api_head(self.USER_PERMISSION_ENDPOINT) _, resp_status = self.api_head(self.USER_PERMISSION_ENDPOINT)
return resp_status["status_code"] == status.HTTP_200_OK return resp_status["connected"]
def get_users(self, rbac_is_enabled_for_org: bool, **kwargs) -> List[GrafanaUserWithPermissions]: def get_users(self, rbac_is_enabled_for_org: bool, **kwargs) -> List[GrafanaUserWithPermissions]:
users, _ = self.api_get("api/org/users", **kwargs) users, _ = self.api_get("api/org/users", **kwargs)

View file

@ -1,7 +1,6 @@
from unittest.mock import patch from unittest.mock import patch
import pytest import pytest
from rest_framework import status
from apps.grafana_plugin.helpers.client import GrafanaAPIClient from apps.grafana_plugin.helpers.client import GrafanaAPIClient
@ -44,17 +43,17 @@ class TestGetUsersPermissions:
class TestIsRbacEnabledForOrganization: class TestIsRbacEnabledForOrganization:
@pytest.mark.parametrize( @pytest.mark.parametrize(
"grafana_api_status_code,expected", "api_response_connected,expected",
[ [
(status.HTTP_200_OK, True), (True, True),
(status.HTTP_404_NOT_FOUND, False), (False, False),
], ],
) )
@patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_head") @patch("apps.grafana_plugin.helpers.client.GrafanaAPIClient.api_head")
def test_it_returns_based_on_status_code_of_head_call( def test_it_returns_based_on_status_code_of_head_call(
self, mocked_grafana_api_client_api_head, grafana_api_status_code, expected self, mocked_grafana_api_client_api_head, api_response_connected, expected
): ):
mocked_grafana_api_client_api_head.return_value = (None, {"status_code": grafana_api_status_code}) mocked_grafana_api_client_api_head.return_value = (None, {"connected": api_response_connected})
api_client = GrafanaAPIClient(API_URL, API_TOKEN) api_client = GrafanaAPIClient(API_URL, API_TOKEN)
assert api_client.is_rbac_enabled_for_organization() == expected assert api_client.is_rbac_enabled_for_organization() == expected

View file

@ -31,7 +31,7 @@ def sync_organization(organization):
_sync_instance_info(organization) _sync_instance_info(organization)
_, check_token_call_status = grafana_api_client.check_token() _, check_token_call_status = grafana_api_client.check_token()
if check_token_call_status["status_code"] == 200: if check_token_call_status["connected"]:
organization.api_token_status = Organization.API_TOKEN_STATUS_OK organization.api_token_status = Organization.API_TOKEN_STATUS_OK
sync_users_and_teams(grafana_api_client, organization) sync_users_and_teams(grafana_api_client, organization)
organization.last_time_synced = timezone.now() organization.last_time_synced = timezone.now()

View file

@ -134,7 +134,7 @@ def test_sync_organization(make_organization, make_team, make_user_for_organizat
}, },
) )
api_check_token_call_status = {"status_code": 200} api_check_token_call_status = {"connected": True}
with patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=False): with patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=False):
with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response): with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response):
@ -203,7 +203,7 @@ def test_sync_organization_is_rbac_permissions_enabled_open_source(make_organiza
"userId": 1, "userId": 1,
}, },
) )
api_check_token_call_status = {"status_code": 200} api_check_token_call_status = {"connected": True}
with patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=grafana_api_response): with patch.object(GrafanaAPIClient, "is_rbac_enabled_for_organization", return_value=grafana_api_response):
with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response): with patch.object(GrafanaAPIClient, "get_users", return_value=api_users_response):
@ -230,7 +230,7 @@ def test_sync_organization_is_rbac_permissions_enabled_cloud(mocked_gcom_client,
stack_id = 5 stack_id = 5
organization = make_organization(stack_id=stack_id) organization = make_organization(stack_id=stack_id)
api_check_token_call_status = {"status_code": 200} api_check_token_call_status = {"connected": True}
mocked_gcom_client.return_value.is_rbac_enabled_for_stack.return_value = gcom_api_response mocked_gcom_client.return_value.is_rbac_enabled_for_stack.return_value = gcom_api_response