Browse Source

chore(hybrid-cloud): Deprecate mediator destroyers (#45656)

In the effort to deprecate mediators, I focus on removing Destroyer
objects, replacing them with

1.  `sentry.deletions` tasks that can execute synchronously
2.  controller logic to handle notifications and side effects.

I believe separating the side effects into the controllers is correct --
even in the original implementation, creating audit logs, triggering
notifications only happens when the object in question is the top level
subject (the controller that deletes installations triggers different
behavior from incidental installation removal from the sentry app
controller).

Notably, I decided to "keep" the synchronous behavior for now, but I
have in place the fittings to make sentry app deletion async. I just
need to consider (in a separate PR) how to change the frontend and apis
to accomodate.

Also, in a followup PR, I plan to refactor the notifier mediator into an
async job similar to the other notifiers for creator. I wanted to keep
this PR simple so I could validate post deploy no issues (no effective
change in behavior should be occurring).

---------

Co-authored-by: Mark Story <mark@mark-story.com>
Zach Collins 2 years ago
parent
commit
74675b9f55

+ 1 - 1
migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0384_backfill_installation_ids
+sentry: 0385_service_hook_hc_fk
 social_auth: 0001_initial

+ 1 - 1
src/sentry/api/bases/sentryapps.py

@@ -299,7 +299,7 @@ class SentryAppInstallationPermission(SentryPermission):
 
         # if user is an app, make sure it's for that same app
         if request.user.is_sentry_app:
-            return request.user == installation.sentry_app.proxy_user
+            return request.user.id == installation.sentry_app.proxy_user_id
 
         # TODO(hybrid-cloud): Replace this RPC with an org member lookup when that exists?
         org_context = organization_service.get_organization_by_id(

+ 2 - 2
src/sentry/api/endpoints/group_external_issue_details.py

@@ -1,9 +1,9 @@
 from rest_framework.request import Request
 from rest_framework.response import Response
 
+from sentry import deletions
 from sentry.api.base import region_silo_endpoint
 from sentry.api.bases.group import GroupEndpoint
-from sentry.mediators import external_issues
 from sentry.models import PlatformExternalIssue
 
 
@@ -17,6 +17,6 @@ class GroupExternalIssueDetailsEndpoint(GroupEndpoint):
         except PlatformExternalIssue.DoesNotExist:
             return Response(status=404)
 
-        external_issues.Destroyer.run(external_issue=external_issue)
+        deletions.exec_sync(external_issue)
 
         return Response(status=204)

+ 35 - 5
src/sentry/api/endpoints/integrations/sentry_apps/details.py

@@ -1,21 +1,26 @@
 import logging
 
+import sentry_sdk
+from django.db import transaction
+from requests import RequestException
 from rest_framework.request import Request
 from rest_framework.response import Response
 
-from sentry import analytics, features
-from sentry.api.base import pending_silo_endpoint
+from sentry import analytics, audit_log, deletions, features
+from sentry.api.base import control_silo_endpoint
 from sentry.api.bases.sentryapps import SentryAppBaseEndpoint, catch_raised_errors
 from sentry.api.serializers import serialize
 from sentry.api.serializers.rest_framework import SentryAppSerializer
 from sentry.constants import SentryAppStatus
-from sentry.mediators.sentry_apps import Destroyer, Updater
+from sentry.mediators import InstallationNotifier
+from sentry.mediators.sentry_apps import Updater
 from sentry.utils import json
+from sentry.utils.audit import create_audit_entry
 
 logger = logging.getLogger(__name__)
 
 
-@pending_silo_endpoint
+@control_silo_endpoint
 class SentryAppDetailsEndpoint(SentryAppBaseEndpoint):
     def get(self, request: Request, sentry_app) -> Response:
         return Response(serialize(sentry_app, request.user, access=request.access))
@@ -82,7 +87,32 @@ class SentryAppDetailsEndpoint(SentryAppBaseEndpoint):
 
     def delete(self, request: Request, sentry_app) -> Response:
         if sentry_app.is_unpublished or sentry_app.is_internal:
-            Destroyer.run(user=request.user, sentry_app=sentry_app, request=request)
+            if not sentry_app.is_internal:
+                for install in sentry_app.installations.all():
+                    try:
+                        with transaction.atomic():
+                            InstallationNotifier.run(
+                                install=install, user=request.user, action="deleted"
+                            )
+                            deletions.exec_sync(install)
+                    except RequestException as exc:
+                        sentry_sdk.capture_exception(exc)
+
+            with transaction.atomic():
+                deletions.exec_sync(sentry_app)
+                create_audit_entry(
+                    request=request,
+                    organization_id=sentry_app.owner_id,
+                    target_object=sentry_app.owner_id,
+                    event=audit_log.get_event_id("SENTRY_APP_REMOVE"),
+                    data={"sentry_app": sentry_app.name},
+                )
+            analytics.record(
+                "sentry_app.deleted",
+                user_id=request.user.id,
+                organization_id=sentry_app.owner_id,
+                sentry_app=sentry_app.slug,
+            )
             return Response(status=204)
 
         return Response({"detail": ["Published apps cannot be removed."]}, status=403)

+ 30 - 3
src/sentry/api/endpoints/integrations/sentry_apps/installation/details.py

@@ -1,21 +1,48 @@
+import sentry_sdk
+from django.db import transaction
+from requests import RequestException
 from rest_framework.request import Request
 from rest_framework.response import Response
 
+from sentry import analytics, audit_log, deletions
 from sentry.api.base import control_silo_endpoint
 from sentry.api.bases import SentryAppInstallationBaseEndpoint
 from sentry.api.serializers import serialize
 from sentry.api.serializers.rest_framework import SentryAppInstallationSerializer
-from sentry.mediators.sentry_app_installations import Destroyer, Updater
+from sentry.mediators import InstallationNotifier
+from sentry.mediators.sentry_app_installations import Updater
+from sentry.utils.audit import create_audit_entry
+from sentry.utils.functional import extract_lazy_object
 
 
 @control_silo_endpoint
 class SentryAppInstallationDetailsEndpoint(SentryAppInstallationBaseEndpoint):
     def get(self, request: Request, installation) -> Response:
-
         return Response(serialize(installation))
 
     def delete(self, request: Request, installation) -> Response:
-        Destroyer.run(install=installation, user=request.user, request=request)
+        with transaction.atomic():
+            try:
+                InstallationNotifier.run(
+                    install=installation, user=extract_lazy_object(request.user), action="deleted"
+                )
+            # if the error is from a request exception, log the error and continue
+            except RequestException as exc:
+                sentry_sdk.capture_exception(exc)
+            deletions.exec_sync(installation)
+            create_audit_entry(
+                request=request,
+                organization_id=installation.organization_id,
+                target_object=installation.organization_id,
+                event=audit_log.get_event_id("SENTRY_APP_UNINSTALL"),
+                data={"sentry_app": installation.sentry_app.name},
+            )
+        analytics.record(
+            "sentry_app.uninstalled",
+            user_id=request.user.id,
+            organization_id=installation.organization_id,
+            sentry_app=installation.sentry_app.slug,
+        )
         return Response(status=204)
 
     def put(self, request: Request, installation) -> Response:

+ 2 - 2
src/sentry/api/endpoints/integrations/sentry_apps/installation/external_issue/details.py

@@ -1,11 +1,11 @@
 from rest_framework.request import Request
 from rest_framework.response import Response
 
+from sentry import deletions
 from sentry.api.base import region_silo_endpoint
 from sentry.api.bases import (
     SentryAppInstallationExternalIssueBaseEndpoint as ExternalIssueBaseEndpoint,
 )
-from sentry.mediators import external_issues
 from sentry.models import PlatformExternalIssue
 
 
@@ -21,6 +21,6 @@ class SentryAppInstallationExternalIssueDetailsEndpoint(ExternalIssueBaseEndpoin
         except PlatformExternalIssue.DoesNotExist:
             return Response(status=404)
 
-        external_issues.Destroyer.run(external_issue=platform_external_issue)
+        deletions.exec_sync(platform_external_issue)
 
         return Response(status=204)

+ 19 - 3
src/sentry/api/endpoints/integrations/sentry_apps/internal_app_token/details.py

@@ -1,12 +1,13 @@
+from django.db import transaction
 from django.http import Http404
 from rest_framework import status
 from rest_framework.request import Request
 from rest_framework.response import Response
 
+from sentry import analytics, deletions
 from sentry.api.base import pending_silo_endpoint
 from sentry.api.bases import SentryAppBaseEndpoint, SentryInternalAppTokenPermission
-from sentry.mediators.sentry_app_installation_tokens import Destroyer
-from sentry.models import ApiToken
+from sentry.models import ApiToken, SentryAppInstallationToken
 
 
 @pending_silo_endpoint
@@ -35,6 +36,21 @@ class SentryInternalAppTokenDetailsEndpoint(SentryAppBaseEndpoint):
                 status=status.HTTP_403_FORBIDDEN,
             )
 
-        Destroyer.run(api_token=api_token, user=request.user, request=request)
+        with transaction.atomic():
+            try:
+                install_token = SentryAppInstallationToken.objects.get(api_token=api_token)
+                sentry_app_installation = install_token.sentry_app_installation
+            except SentryAppInstallationToken.DoesNotExist:
+                raise Http404
+
+            deletions.exec_sync(install_token)
+
+        analytics.record(
+            "sentry_app_installation_token.deleted",
+            user_id=request.user.id,
+            organization_id=sentry_app_installation.organization_id,
+            sentry_app_installation_id=sentry_app_installation.id,
+            sentry_app=sentry_app.slug,
+        )
 
         return Response(status=204)

+ 7 - 0
src/sentry/constants.py

@@ -472,10 +472,12 @@ class SentryAppStatus:
     PUBLISHED = 1
     INTERNAL = 2
     PUBLISH_REQUEST_INPROGRESS = 3
+    DELETION_IN_PROGRESS = 4
     UNPUBLISHED_STR = "unpublished"
     PUBLISHED_STR = "published"
     INTERNAL_STR = "internal"
     PUBLISH_REQUEST_INPROGRESS_STR = "publish_request_inprogress"
+    DELETION_IN_PROGRESS_STR = "deletion_in_progress"
 
     @classmethod
     def as_choices(cls) -> Sequence[Tuple[int, str]]:
@@ -484,6 +486,7 @@ class SentryAppStatus:
             (cls.PUBLISHED, cls.PUBLISHED_STR),
             (cls.INTERNAL, cls.INTERNAL_STR),
             (cls.PUBLISH_REQUEST_INPROGRESS, cls.PUBLISH_REQUEST_INPROGRESS_STR),
+            (cls.DELETION_IN_PROGRESS, cls.DELETION_IN_PROGRESS_STR),
         )
 
     @classmethod
@@ -496,6 +499,8 @@ class SentryAppStatus:
             return cls.INTERNAL_STR
         elif status == cls.PUBLISH_REQUEST_INPROGRESS:
             return cls.PUBLISH_REQUEST_INPROGRESS_STR
+        elif status == cls.DELETION_IN_PROGRESS:
+            return cls.DELETION_IN_PROGRESS_STR
         else:
             return None
 
@@ -509,6 +514,8 @@ class SentryAppStatus:
             return cls.INTERNAL
         elif status == cls.PUBLISH_REQUEST_INPROGRESS_STR:
             return cls.PUBLISH_REQUEST_INPROGRESS
+        elif status == cls.DELETION_IN_PROGRESS_STR:
+            return cls.DELETION_IN_PROGRESS
         else:
             return None
 

+ 12 - 0
src/sentry/deletions/__init__.py

@@ -129,6 +129,9 @@ def load_defaults():
         models.OrganizationIntegration, defaults.OrganizationIntegrationDeletionTask
     )
     default_manager.register(models.OrganizationMemberTeam, BulkModelDeletionTask)
+    default_manager.register(
+        models.PlatformExternalIssue, defaults.PlatformExternalIssueDeletionTask
+    )
     default_manager.register(models.Project, defaults.ProjectDeletionTask)
     default_manager.register(models.ProjectBookmark, BulkModelDeletionTask)
     default_manager.register(models.ProjectKey, BulkModelDeletionTask)
@@ -143,6 +146,14 @@ def load_defaults():
     default_manager.register(
         models.RepositoryProjectPathConfig, defaults.RepositoryProjectPathConfigDeletionTask
     )
+    default_manager.register(models.SentryApp, defaults.SentryAppDeletionTask)
+    default_manager.register(
+        models.SentryAppInstallation, defaults.SentryAppInstallationDeletionTask
+    )
+    default_manager.register(
+        models.SentryAppInstallationToken, defaults.SentryAppInstallationTokenDeletionTask
+    )
+    default_manager.register(models.ServiceHook, defaults.ServiceHookDeletionTask)
     default_manager.register(models.SavedSearch, BulkModelDeletionTask)
     default_manager.register(models.Team, defaults.TeamDeletionTask)
     default_manager.register(models.UserReport, BulkModelDeletionTask)
@@ -152,3 +163,4 @@ load_defaults()
 
 get = default_manager.get
 register = default_manager.register
+exec_sync = default_manager.exec_sync

+ 5 - 0
src/sentry/deletions/defaults/__init__.py

@@ -6,8 +6,13 @@ from .discoversavedquery import *  # noqa: F401,F403
 from .group import *  # noqa: F401,F403
 from .organization import *  # noqa: F401,F403
 from .organizationintegration import *  # noqa: F401,F403
+from .platform_external_issue import *  # noqa: F401,F403
 from .project import *  # noqa: F401,F403
 from .release import *  # noqa: F401,F403
 from .repository import *  # noqa: F401,F403
 from .repositoryprojectpathconfig import *  # noqa: F401,F403
+from .sentry_app import *  # noqa: F401,F403
+from .sentry_app_installation import *  # noqa: F401,F403
+from .sentry_app_installation_token import *  # noqa: F401,F403
+from .service_hook import *  # noqa: F401,F403
 from .team import *  # noqa: F401,F403

Some files were not shown because too many files changed in this diff