Browse Source

ref(hybrid-cloud): Makes integration and organization lookup failures explicit (#62709)

Gabe Villalobos 1 year ago
parent
commit
fd2519021e

+ 10 - 4
src/sentry/middleware/integrations/parsers/base.py

@@ -197,10 +197,16 @@ class BaseRequestParser(abc.ABC):
             integration = self.get_integration_from_request()
         if not integration:
             logger.info("%s.no_integration", self.provider, extra={"path": self.request.path})
-            return []
+            raise Integration.DoesNotExist()
         organization_integrations = OrganizationIntegration.objects.filter(
             integration_id=integration.id
         )
+
+        if organization_integrations.count() == 0:
+            logger.info(
+                "%s.no_organization_integrations", self.provider, extra={"path": self.request.path}
+            )
+            raise OrganizationIntegration.DoesNotExist()
         organization_ids = [oi.organization_id for oi in organization_integrations]
         return organization_mapping_service.get_many(organization_ids=organization_ids)
 
@@ -212,8 +218,8 @@ class BaseRequestParser(abc.ABC):
         """
         if not organizations:
             organizations = self.get_organizations_from_integration()
-        if not organizations:
-            logger.info("%s.no_organizations", self.provider, extra={"path": self.request.path})
-            return []
 
         return [get_region_for_organization(organization.slug) for organization in organizations]
+
+    def get_default_missing_integration_response(self) -> HttpResponse:
+        return HttpResponse(status=400)

+ 5 - 4
src/sentry/middleware/integrations/parsers/discord.py

@@ -16,6 +16,7 @@ from sentry.integrations.discord.webhooks.base import DiscordInteractionsEndpoin
 from sentry.middleware.integrations.parsers.base import BaseRequestParser
 from sentry.middleware.integrations.tasks import convert_to_async_discord_response
 from sentry.models.integrations import Integration
+from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.outbox import ControlOutbox, WebhookProviderIdentifier
 from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviders
 from sentry.types.region import Region
@@ -112,10 +113,10 @@ class DiscordRequestParser(BaseRequestParser):
             if self.discord_request.is_ping():
                 return DiscordInteractionsEndpoint.respond_ping()
 
-        regions = self.get_regions_from_organizations()
-        if len(regions) == 0:
-            logger.info("%s.no_regions", self.provider, extra={"path": self.request.path})
-            return self.get_response_from_control_silo()
+        try:
+            regions = self.get_regions_from_organizations()
+        except (Integration.DoesNotExist, OrganizationIntegration.DoesNotExist):
+            return self.get_default_missing_integration_response()
 
         if is_discord_interactions_endpoint and self.discord_request:
             if self.discord_request.is_command():

+ 6 - 4
src/sentry/middleware/integrations/parsers/github.py

@@ -11,6 +11,7 @@ from sentry.integrations.github.webhook import (
 )
 from sentry.middleware.integrations.parsers.base import BaseRequestParser
 from sentry.models.integrations.integration import Integration
+from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.outbox import WebhookProviderIdentifier
 from sentry.services.hybrid_cloud.util import control_silo_function
 from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviders
@@ -54,8 +55,9 @@ class GithubRequestParser(BaseRequestParser):
         if event.get("installation") and event.get("action") in {"created", "deleted"}:
             return self.get_response_from_control_silo()
 
-        regions = self.get_regions_from_organizations()
-        if len(regions) == 0:
-            logger.info("%s.no_regions", self.provider, extra={"path": self.request.path})
-            return self.get_response_from_control_silo()
+        try:
+            regions = self.get_regions_from_organizations()
+        except (Integration.DoesNotExist, OrganizationIntegration.DoesNotExist):
+            return self.get_default_missing_integration_response()
+
         return self.get_response_from_outbox_creation(regions=regions)

+ 5 - 4
src/sentry/middleware/integrations/parsers/gitlab.py

@@ -10,6 +10,7 @@ from sentry.integrations.gitlab.webhooks import GitlabWebhookEndpoint, GitlabWeb
 from sentry.integrations.utils.scope import clear_tags_and_context
 from sentry.middleware.integrations.parsers.base import BaseRequestParser
 from sentry.models.integrations.integration import Integration
+from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.outbox import WebhookProviderIdentifier
 from sentry.services.hybrid_cloud.util import control_silo_function
 from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviders
@@ -66,10 +67,10 @@ class GitlabRequestParser(BaseRequestParser, GitlabWebhookMixin):
         if isinstance(maybe_http_response, HttpResponseBase):
             return maybe_http_response
 
-        regions = self.get_regions_from_organizations()
-        if len(regions) == 0:
-            logger.info("%s.no_regions", self.provider, extra={"path": self.request.path})
-            return self.get_response_from_control_silo()
+        try:
+            regions = self.get_regions_from_organizations()
+        except (Integration.DoesNotExist, OrganizationIntegration.DoesNotExist):
+            return self.get_default_missing_integration_response()
 
         return self.get_response_from_outbox_creation(regions=regions)
 

+ 9 - 2
src/sentry/middleware/integrations/parsers/msteams.py

@@ -2,6 +2,7 @@ from __future__ import annotations
 
 import logging
 from functools import cached_property
+from typing import Sequence
 
 import sentry_sdk
 from django.http.response import HttpResponseBase
@@ -9,10 +10,11 @@ from django.http.response import HttpResponseBase
 from sentry.integrations.msteams.webhook import MsTeamsWebhookEndpoint, MsTeamsWebhookMixin
 from sentry.middleware.integrations.parsers.base import BaseRequestParser
 from sentry.models.integrations.integration import Integration
+from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.outbox import WebhookProviderIdentifier
 from sentry.services.hybrid_cloud.util import control_silo_function
 from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviders
-from sentry.types.region import RegionResolutionError
+from sentry.types.region import Region, RegionResolutionError
 from sentry.utils import json
 
 logger = logging.getLogger(__name__)
@@ -49,7 +51,12 @@ class MsTeamsRequestParser(BaseRequestParser, MsTeamsWebhookMixin):
         if not self.can_infer_integration(data=self.request_data):
             return self.get_response_from_control_silo()
 
-        regions = self.get_regions_from_organizations()
+        regions: Sequence[Region] = []
+        try:
+            regions = self.get_regions_from_organizations()
+        except (Integration.DoesNotExist, OrganizationIntegration.DoesNotExist):
+            return self.get_default_missing_integration_response()
+
         if len(regions) == 0:
             with sentry_sdk.push_scope() as scope:
                 scope.set_extra("view_class", self.view_class)

+ 13 - 4
src/sentry/middleware/integrations/parsers/slack.py

@@ -4,6 +4,7 @@ import dataclasses
 import logging
 from typing import Sequence
 
+import sentry_sdk
 from django.http.response import HttpResponse, HttpResponseBase
 from rest_framework import status
 from rest_framework.request import Request
@@ -24,6 +25,7 @@ from sentry.integrations.slack.webhooks.command import SlackCommandsEndpoint
 from sentry.integrations.slack.webhooks.event import SlackEventEndpoint
 from sentry.middleware.integrations.tasks import convert_to_async_slack_response
 from sentry.models.integrations.integration import Integration
+from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.outbox import ControlOutbox, WebhookProviderIdentifier
 from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviders
 from sentry.types.region import Region
@@ -126,10 +128,17 @@ class SlackRequestParser(BaseRequestParser):
         if data and is_event_challenge(data):
             return self.get_response_from_control_silo()
 
-        regions = self.get_regions_from_organizations()
-        if len(regions) == 0:
-            logger.info("%s.no_regions", self.provider, extra={"path": self.request.path})
-            return self.get_response_from_control_silo()
+        try:
+            regions = self.get_regions_from_organizations()
+        except Integration.DoesNotExist:
+            # Alert, as there may be a misconfiguration issue
+            sentry_sdk.capture_exception()
+            return self.get_default_missing_integration_response()
+        except OrganizationIntegration.DoesNotExist:
+            # Swallow this exception, as this is likely due to a user removing
+            # their org's slack integration, and slack will continue to retry
+            # this request until it succeeds.
+            return HttpResponse(status=202)
 
         if self.view_class == SlackActionEndpoint:
             drf_request: Request = SlackDMEndpoint().initialize_request(self.request)

+ 5 - 4
src/sentry/middleware/integrations/parsers/vsts.py

@@ -8,6 +8,7 @@ from django.http.response import HttpResponseBase
 from sentry.integrations.vsts.webhooks import WorkItemWebhook, get_vsts_external_id
 from sentry.middleware.integrations.parsers.base import BaseRequestParser
 from sentry.models.integrations.integration import Integration
+from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.outbox import WebhookProviderIdentifier
 from sentry.services.hybrid_cloud.util import control_silo_function
 from sentry.utils import json
@@ -35,9 +36,9 @@ class VstsRequestParser(BaseRequestParser):
         if self.view_class not in self.region_view_classes:
             return self.get_response_from_control_silo()
 
-        regions = self.get_regions_from_organizations()
-        if len(regions) == 0:
-            logger.error("%s.no_regions", self.provider, extra={"path": self.request.path})
-            return self.get_response_from_control_silo()
+        try:
+            regions = self.get_regions_from_organizations()
+        except (Integration.DoesNotExist, OrganizationIntegration.DoesNotExist):
+            return self.get_default_missing_integration_response()
 
         return self.get_response_from_outbox_creation(regions=regions)

+ 1 - 2
tests/sentry/middleware/integrations/parsers/test_discord.py

@@ -132,8 +132,7 @@ class DiscordRequestParserTest(TestCase):
 
         response = parser.get_response()
         assert isinstance(response, HttpResponse)
-        assert response.status_code == 200
-        assert response.content == b"passthrough"
+        assert response.status_code == 400
         assert len(responses.calls) == 0
         assert_no_webhook_outboxes()
 

+ 18 - 13
tests/sentry/middleware/integrations/parsers/test_github.py

@@ -1,5 +1,5 @@
 import responses
-from django.db import router
+from django.db import router, transaction
 from django.http import HttpRequest, HttpResponse
 from django.test import RequestFactory, override_settings
 from django.urls import reverse
@@ -7,9 +7,13 @@ from django.urls import reverse
 from sentry.middleware.integrations.parsers.github import GithubRequestParser
 from sentry.models.integrations.integration import Integration
 from sentry.models.integrations.organization_integration import OrganizationIntegration
-from sentry.models.outbox import ControlOutbox, OutboxCategory, WebhookProviderIdentifier
+from sentry.models.outbox import (
+    ControlOutbox,
+    OutboxCategory,
+    WebhookProviderIdentifier,
+    outbox_context,
+)
 from sentry.silo.base import SiloMode
-from sentry.silo.safety import unguarded_write
 from sentry.testutils.cases import TestCase
 from sentry.testutils.outbox import assert_no_webhook_outboxes, assert_webhook_outboxes
 from sentry.testutils.region import override_regions
@@ -52,34 +56,34 @@ class GithubRequestParserTest(TestCase):
     @override_settings(SILO_MODE=SiloMode.CONTROL)
     @override_regions(region_config)
     @responses.activate
-    def test_routing_webhook_properly_no_regions(self):
+    def test_routing_no_organization_integration_found(self):
         integration = self.get_integration()
-        with unguarded_write(using=router.db_for_write(OrganizationIntegration)):
+        with outbox_context(transaction.atomic(using=router.db_for_write(OrganizationIntegration))):
             # Remove all organizations from integration
             OrganizationIntegration.objects.filter(integration=integration).delete()
 
-        request = self.factory.post(self.path, data={}, content_type="application/json")
+        request = self.factory.post(
+            self.path, data={"installation": {"id": "github:1"}}, content_type="application/json"
+        )
         parser = GithubRequestParser(request=request, response_handler=self.get_response)
 
         response = parser.get_response()
         assert isinstance(response, HttpResponse)
-        assert response.status_code == 200
-        assert response.content == b"passthrough"
+        assert response.status_code == 400
         assert len(responses.calls) == 0
         assert_no_webhook_outboxes()
 
     @override_settings(SILO_MODE=SiloMode.CONTROL)
     @override_regions(region_config)
     @responses.activate
-    def test_routing_webhook_properly_with_regions(self):
+    def test_routing_no_integration_found(self):
         self.get_integration()
         request = self.factory.post(self.path, data={}, content_type="application/json")
         parser = GithubRequestParser(request=request, response_handler=self.get_response)
 
         response = parser.get_response()
         assert isinstance(response, HttpResponse)
-        assert response.status_code == 200
-        assert response.content == b"passthrough"
+        assert response.status_code == 400
         assert len(responses.calls) == 0
         assert_no_webhook_outboxes()
 
@@ -94,13 +98,14 @@ class GithubRequestParserTest(TestCase):
                 "integration_id": self.integration.id,
             },
         )
-        request = self.factory.post(path, data={}, content_type="application/json")
+        request = self.factory.post(
+            path, data={"installation": {"id": "github:1"}}, content_type="application/json"
+        )
         parser = GithubRequestParser(request=request, response_handler=self.get_response)
 
         response = parser.get_response()
         assert isinstance(response, HttpResponse)
         assert response.status_code == 200
-        assert response.content == b"passthrough"
         assert len(responses.calls) == 0
         assert_no_webhook_outboxes()
 

+ 18 - 11
tests/sentry/middleware/integrations/parsers/test_github_enterprise.py

@@ -1,5 +1,5 @@
 import responses
-from django.db import router
+from django.db import router, transaction
 from django.http import HttpRequest, HttpResponse
 from django.test import RequestFactory, override_settings
 from django.urls import reverse
@@ -7,9 +7,13 @@ from django.urls import reverse
 from sentry.middleware.integrations.parsers.github_enterprise import GithubEnterpriseRequestParser
 from sentry.models.integrations.integration import Integration
 from sentry.models.integrations.organization_integration import OrganizationIntegration
-from sentry.models.outbox import ControlOutbox, OutboxCategory, WebhookProviderIdentifier
+from sentry.models.outbox import (
+    ControlOutbox,
+    OutboxCategory,
+    WebhookProviderIdentifier,
+    outbox_context,
+)
 from sentry.silo.base import SiloMode
-from sentry.silo.safety import unguarded_write
 from sentry.testutils.cases import TestCase
 from sentry.testutils.outbox import assert_no_webhook_outboxes, assert_webhook_outboxes
 from sentry.testutils.region import override_regions
@@ -52,34 +56,37 @@ class GithubEnterpriseRequestParserTest(TestCase):
     @override_settings(SILO_MODE=SiloMode.CONTROL)
     @override_regions(region_config)
     @responses.activate
-    def test_routing_properly_no_regions(self):
+    def test_routing_no_organization_integrations_found(self):
         integration = self.get_integration()
-        with unguarded_write(using=router.db_for_write(OrganizationIntegration)):
+        with outbox_context(transaction.atomic(using=router.db_for_write(OrganizationIntegration))):
             # Remove all organizations from integration
             OrganizationIntegration.objects.filter(integration=integration).delete()
 
-        request = self.factory.post(self.path, data={}, content_type="application/json")
+        request = self.factory.post(
+            self.path,
+            data={"installation": {"id": self.external_identifier}},
+            content_type="application/json",
+            HTTP_X_GITHUB_ENTERPRISE_HOST=self.external_host,
+        )
         parser = GithubEnterpriseRequestParser(request=request, response_handler=self.get_response)
 
         response = parser.get_response()
         assert isinstance(response, HttpResponse)
-        assert response.status_code == 200
-        assert response.content == b"passthrough"
+        assert response.status_code == 400
         assert len(responses.calls) == 0
         assert_no_webhook_outboxes()
 
     @override_settings(SILO_MODE=SiloMode.CONTROL)
     @override_regions(region_config)
     @responses.activate
-    def test_routing_properly_with_regions(self):
+    def test_routing_no_integrations_found(self):
         self.get_integration()
         request = self.factory.post(self.path, data={}, content_type="application/json")
         parser = GithubEnterpriseRequestParser(request=request, response_handler=self.get_response)
 
         response = parser.get_response()
         assert isinstance(response, HttpResponse)
-        assert response.status_code == 200
-        assert response.content == b"passthrough"
+        assert response.status_code == 400
         assert len(responses.calls) == 0
         assert_no_webhook_outboxes()
 

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