From 8abbcee050bb3f8e33bdc8de7373c389e86156fc Mon Sep 17 00:00:00 2001 From: Innokentii Konstantinov Date: Thu, 5 Jan 2023 12:42:55 +0800 Subject: [PATCH] Org soft-delete (#1073) # What this PR does It introduces soft-delete of organization, since grafana stacks are soft-deleted too. Also, we had a problem with deleting orgs with large amounts of alerts, so soft-deletion will fix this problem. I think, that problem of cleaning alerts of deleted orgs should be solved as a part of alert retention --- engine/apps/auth_token/auth.py | 8 +++- engine/apps/grafana_plugin/views/install.py | 6 ++- .../mixins/alert_channel_defining_mixin.py | 6 ++- engine/apps/public_api/tests/conftest.py | 17 -------- engine/apps/user_management/exceptions.py | 11 +++++ engine/apps/user_management/middlewares.py | 12 +++++- .../0007_organization_deleted_at.py | 18 +++++++++ .../user_management/models/organization.py | 30 ++++++++++---- engine/apps/user_management/models/region.py | 7 ---- engine/apps/user_management/sync.py | 1 + .../tests/test_organization.py | 40 +++++++++++++++++-- .../apps/user_management/tests/test_region.py | 2 +- .../apps/user_management/tests/test_sync.py | 5 +-- engine/conftest.py | 10 +++++ engine/settings/base.py | 1 + 15 files changed, 130 insertions(+), 44 deletions(-) delete mode 100644 engine/apps/public_api/tests/conftest.py create mode 100644 engine/apps/user_management/exceptions.py create mode 100644 engine/apps/user_management/migrations/0007_organization_deleted_at.py diff --git a/engine/apps/auth_token/auth.py b/engine/apps/auth_token/auth.py index 8b0da92a..97a48bfa 100644 --- a/engine/apps/auth_token/auth.py +++ b/engine/apps/auth_token/auth.py @@ -10,9 +10,9 @@ from rest_framework.request import Request from apps.api.permissions import RBACPermission, user_is_authorized from apps.grafana_plugin.helpers.gcom import check_token +from apps.user_management.exceptions import OrganizationDeletedException, OrganizationMovedException from apps.user_management.models import User from apps.user_management.models.organization import Organization -from apps.user_management.models.region import OrganizationMovedException from .constants import SCHEDULE_EXPORT_TOKEN_NAME, SLACK_AUTH_TOKEN_NAME from .exceptions import InvalidToken @@ -46,6 +46,8 @@ class ApiTokenAuthentication(BaseAuthentication): except InvalidToken: raise exceptions.AuthenticationFailed("Invalid token.") + if auth_token.organization.deleted_at: + raise OrganizationDeletedException(auth_token.organization) if auth_token.organization.is_moved: raise OrganizationMovedException(auth_token.organization) @@ -170,6 +172,8 @@ class ScheduleExportAuthentication(BaseAuthentication): except InvalidToken: raise exceptions.AuthenticationFailed("Invalid token.") + if auth_token.organization.deleted_at: + raise OrganizationDeletedException(auth_token.organization) if auth_token.organization.is_moved: raise OrganizationMovedException(auth_token.organization) @@ -203,6 +207,8 @@ class UserScheduleExportAuthentication(BaseAuthentication): except InvalidToken: raise exceptions.AuthenticationFailed("Invalid token") + if auth_token.organization.deleted_at: + raise OrganizationDeletedException(auth_token.organization) if auth_token.organization.is_moved: raise OrganizationMovedException(auth_token.organization) diff --git a/engine/apps/grafana_plugin/views/install.py b/engine/apps/grafana_plugin/views/install.py index b8b883fe..f13f2f95 100644 --- a/engine/apps/grafana_plugin/views/install.py +++ b/engine/apps/grafana_plugin/views/install.py @@ -16,9 +16,11 @@ class InstallView(GrafanaHeadersMixin, APIView): stack_id = self.instance_context["stack_id"] org_id = self.instance_context["org_id"] - organization = Organization.objects.filter(stack_id=stack_id, org_id=org_id).first() + organization = Organization.objects_with_deleted.filter(stack_id=stack_id, org_id=org_id).first() + # If we receive install request to the deleted org - just restore it. + organization.deleted_at = None organization.api_token = self.instance_context["grafana_token"] - organization.save(update_fields=["api_token"]) + organization.save(update_fields=["api_token", "deleted_at"]) sync_organization(organization) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/engine/apps/integrations/mixins/alert_channel_defining_mixin.py b/engine/apps/integrations/mixins/alert_channel_defining_mixin.py index 0a867595..fee93b35 100644 --- a/engine/apps/integrations/mixins/alert_channel_defining_mixin.py +++ b/engine/apps/integrations/mixins/alert_channel_defining_mixin.py @@ -7,7 +7,7 @@ from django.core.cache import cache from django.core.exceptions import PermissionDenied from django.db import OperationalError -from apps.user_management.models.region import OrganizationMovedException +from apps.user_management.exceptions import OrganizationMovedException logger = logging.getLogger(__name__) @@ -66,6 +66,10 @@ class AlertChannelDefiningMixin(object): logger.info("Cache is empty!") raise + if alert_receive_channel.organization.deleted_at: + # It's better to raise OrganizarionDeletedException, but in legacy code PermissionDenied is returned when integration key not found. + # So, keep it consistent. + raise PermissionDenied("Integration key was not found. Permission denied.") if alert_receive_channel.organization.is_moved: raise OrganizationMovedException(alert_receive_channel.organization) diff --git a/engine/apps/public_api/tests/conftest.py b/engine/apps/public_api/tests/conftest.py deleted file mode 100644 index f8b6f8b0..00000000 --- a/engine/apps/public_api/tests/conftest.py +++ /dev/null @@ -1,17 +0,0 @@ -import pytest -from pytest_factoryboy import register - -from apps.user_management.tests.factories import OrganizationFactory, UserFactory - -register(UserFactory) -register(OrganizationFactory) - - -@pytest.fixture() -def make_organization_and_user_with_token(make_organization_and_user, make_public_api_token): - def _make_organization_and_user_with_token(): - organization, user = make_organization_and_user() - _, token = make_public_api_token(user, organization) - return organization, user, token - - return _make_organization_and_user_with_token diff --git a/engine/apps/user_management/exceptions.py b/engine/apps/user_management/exceptions.py new file mode 100644 index 00000000..343cd874 --- /dev/null +++ b/engine/apps/user_management/exceptions.py @@ -0,0 +1,11 @@ +from .models import Organization + + +class OrganizationDeletedException(Exception): + def __init__(self, organization: Organization): + self.organization = organization + + +class OrganizationMovedException(Exception): + def __init__(self, organization: Organization): + self.organization = organization diff --git a/engine/apps/user_management/middlewares.py b/engine/apps/user_management/middlewares.py index bf599ec7..528111e3 100644 --- a/engine/apps/user_management/middlewares.py +++ b/engine/apps/user_management/middlewares.py @@ -1,13 +1,14 @@ import logging import requests -from django.http import HttpResponse +from django.http import HttpResponse, JsonResponse from django.utils.deprecation import MiddlewareMixin from rest_framework import status -from apps.user_management.models.region import OrganizationMovedException from common.api_helpers.utils import create_engine_url +from .exceptions import OrganizationDeletedException, OrganizationMovedException + logger = logging.getLogger(__name__) @@ -45,3 +46,10 @@ class OrganizationMovedMiddleware(MiddlewareMixin): return requests.delete(url, headers=headers) elif method == "OPTIONS": return requests.options(url, headers=headers) + + +class OrganizationDeletedMiddleware(MiddlewareMixin): + def process_exception(self, request, exception): + if isinstance(exception, OrganizationDeletedException): + # Return drf-shaped not-found response to keep responses consistent + return JsonResponse(status=status.HTTP_404_NOT_FOUND, data={"detail": "Not found."}) diff --git a/engine/apps/user_management/migrations/0007_organization_deleted_at.py b/engine/apps/user_management/migrations/0007_organization_deleted_at.py new file mode 100644 index 00000000..fd80179e --- /dev/null +++ b/engine/apps/user_management/migrations/0007_organization_deleted_at.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.16 on 2023-01-04 05:06 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('user_management', '0006_organization_uuid'), + ] + + operations = [ + migrations.AddField( + model_name='organization', + name='deleted_at', + field=models.DateTimeField(null=True), + ), + ] diff --git a/engine/apps/user_management/models/organization.py b/engine/apps/user_management/models/organization.py index db3d7f0d..fbf554f4 100644 --- a/engine/apps/user_management/models/organization.py +++ b/engine/apps/user_management/models/organization.py @@ -7,6 +7,7 @@ from django.apps import apps from django.conf import settings from django.core.validators import MinLengthValidator from django.db import models +from django.utils import timezone from mirage import fields as mirage_fields from apps.alerts.models import MaintainableObject @@ -50,22 +51,35 @@ class OrganizationQuerySet(models.QuerySet): return instance def delete(self): - org_id = self.public_primary_key - super().delete(self) - if settings.FEATURE_MULTIREGION_ENABLED: - delete_oncall_connector_async.apply_async( - (org_id), - ) + self.update(deleted_at=timezone.now()) + + def hard_delete(self): + super().delete() + + +class OrganizationManager(models.Manager): + def get_queryset(self): + return OrganizationQuerySet(self.model, using=self._db).filter(deleted_at__isnull=True) class Organization(MaintainableObject): - objects = OrganizationQuerySet.as_manager() + objects = OrganizationManager() + objects_with_deleted = models.Manager() def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.subscription_strategy = self._get_subscription_strategy() + def delete(self): + self.deleted_at = timezone.now() + self.save(update_fields=["deleted_at"]) + if settings.FEATURE_MULTIREGION_ENABLED: + delete_oncall_connector_async.apply_async((self.public_primary_key,)) + + def hard_delete(self): + super().delete() + def _get_subscription_strategy(self): if self.pricing_version == self.FREE_PUBLIC_BETA_PRICING: return FreePublicBetaSubscriptionStrategy(self) @@ -133,6 +147,8 @@ class Organization(MaintainableObject): # uuid used to unuqie identify organization in different clusters uuid = models.UUIDField(default=uuid.uuid4, editable=False) + deleted_at = models.DateTimeField(null=True) + # Organization Settings configured from slack ( ACKNOWLEDGE_REMIND_NEVER, diff --git a/engine/apps/user_management/models/region.py b/engine/apps/user_management/models/region.py index a1d0b312..ad8ab537 100644 --- a/engine/apps/user_management/models/region.py +++ b/engine/apps/user_management/models/region.py @@ -3,8 +3,6 @@ import logging from django.apps import apps from django.db import models -from apps.user_management.models import Organization - logger = logging.getLogger(__name__) @@ -41,11 +39,6 @@ def sync_regions(regions: list[dict]): Region.objects.bulk_update(regions_to_update, ["name", "oncall_backend_url"], batch_size=5000) -class OrganizationMovedException(Exception): - def __init__(self, organization: Organization): - self.organization = organization - - class Region(models.Model): name = models.CharField(max_length=300) slug = models.CharField(max_length=50, unique=True) diff --git a/engine/apps/user_management/sync.py b/engine/apps/user_management/sync.py index 2334c5c5..7bbba71f 100644 --- a/engine/apps/user_management/sync.py +++ b/engine/apps/user_management/sync.py @@ -85,6 +85,7 @@ def delete_organization_if_needed(organization): # Organization has a manually set API token, it will not be found within GCOM # and would need to be deleted manually. if organization.gcom_token is None: + logger.info(f"Organization {organization.pk} has no gcom_token. Probably it's needed to delete org manually.") return False # Use common token as organization.gcom_token could be already revoked diff --git a/engine/apps/user_management/tests/test_organization.py b/engine/apps/user_management/tests/test_organization.py index 9cfd06e7..376bc6f0 100644 --- a/engine/apps/user_management/tests/test_organization.py +++ b/engine/apps/user_management/tests/test_organization.py @@ -1,16 +1,50 @@ import pytest from django.core.exceptions import ObjectDoesNotExist +from django.urls import reverse from django.utils import timezone +from rest_framework.test import APIClient -from apps.alerts.models import AlertGroupLogRecord, EscalationPolicy +from apps.alerts.models import AlertGroupLogRecord, AlertReceiveChannel, EscalationPolicy from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord from apps.schedules.models import OnCallScheduleCalendar from apps.telegram.models import TelegramMessage from apps.twilioapp.constants import TwilioCallStatuses, TwilioMessageStatuses +from apps.user_management.models import Organization @pytest.mark.django_db -def test_organization_delete( +def test_organization_soft_delete( + make_organization_and_user_with_token, + make_alert_receive_channel, +): + organization, _, token = make_organization_and_user_with_token() + alert_receive_channel = make_alert_receive_channel( + organization=organization, integration=AlertReceiveChannel.INTEGRATION_ALERTMANAGER + ) + + org_id = organization.id + organization.delete() + + deleted_organization = Organization.objects_with_deleted.get(id=org_id) + # check if org soft-deleted + assert deleted_organization.deleted_at is not None + + # check if public api responds with 404 + client = APIClient() + url = reverse("api-public:integrations-list") + response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") + + assert response.status_code == 404 + + # check if alert receiver view responds with 403 + url = reverse("integrations:alertmanager", kwargs={"alert_channel_key": alert_receive_channel.token}) + data = {"a": "b"} + response = client.post(url, data, format="json") + assert response.status_code == 403 + + +@pytest.mark.django_db +def test_organization_hard_delete( make_organization, make_user, make_team, @@ -159,7 +193,7 @@ def test_organization_delete( resolution_note_slack_message, ] - organization.delete() + organization.hard_delete() for obj in cascading_objects: with pytest.raises(ObjectDoesNotExist): obj.refresh_from_db() diff --git a/engine/apps/user_management/tests/test_region.py b/engine/apps/user_management/tests/test_region.py index 02756b9e..414396d1 100644 --- a/engine/apps/user_management/tests/test_region.py +++ b/engine/apps/user_management/tests/test_region.py @@ -11,7 +11,7 @@ from apps.auth_token.auth import ApiTokenAuthentication, ScheduleExportAuthentic from apps.auth_token.models import ScheduleExportAuthToken, UserScheduleExportAuthToken from apps.integrations.views import AlertManagerAPIView from apps.schedules.models import OnCallScheduleWeb -from apps.user_management.models.region import OrganizationMovedException +from apps.user_management.exceptions import OrganizationMovedException @pytest.mark.django_db diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index c896e725..5a340875 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -1,7 +1,6 @@ from unittest.mock import patch import pytest -from django.core.exceptions import ObjectDoesNotExist from apps.grafana_plugin.helpers.client import GcomAPIClient, GrafanaAPIClient from apps.user_management.models import Team, User @@ -199,5 +198,5 @@ def test_cleanup_organization_deleted(make_organization): with patch.object(GcomAPIClient, "get_instance_info", return_value={"status": "deleted"}): cleanup_organization(organization.id) - with pytest.raises(ObjectDoesNotExist): - organization.refresh_from_db() + organization.refresh_from_db() + assert organization.deleted_at is not None diff --git a/engine/conftest.py b/engine/conftest.py index 11db43ec..353b194c 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -765,3 +765,13 @@ def make_organization_and_region(make_organization, make_region): return organization, region return _make_organization_and_region + + +@pytest.fixture() +def make_organization_and_user_with_token(make_organization_and_user, make_public_api_token): + def _make_organization_and_user_with_token(): + organization, user = make_organization_and_user() + _, token = make_public_api_token(user, organization) + return organization, user, token + + return _make_organization_and_user_with_token diff --git a/engine/settings/base.py b/engine/settings/base.py index e9abfc7b..b408b1a3 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -251,6 +251,7 @@ MIDDLEWARE = [ "apps.social_auth.middlewares.SocialAuthAuthCanceledExceptionMiddleware", "apps.integrations.middlewares.IntegrationExceptionMiddleware", "apps.user_management.middlewares.OrganizationMovedMiddleware", + "apps.user_management.middlewares.OrganizationDeletedMiddleware", ] LOG_REQUEST_ID_HEADER = "HTTP_X_CLOUD_TRACE_CONTEXT"