Browse Source

ref(hybrid-cloud): Allow plugins to access the integration control middleware (#54329)

This PR introduces the concept of 'classifications' to the integration
control middleware.

The middleware prior was making a lot of assumptions about the patterns
of requests it should operate on. This was fine because most first party
integrations share similar request structures (e.g. path, prefixes,
needing individual parsers). To accomodate the provisioning webhooks,
and plugins, we need to make this a bit more tweakable.

Classifications are meant to do this, letting all the existing 'routing'
code fall under the `IntegrationClassification`. A straight forward
`PluginClassification` was added to use a different set of operating
rules. Hopefully this can be extended for provisioning when I begin work
on routing those webhooks.

**todo**
- [x] Ensure existing integration webhook forwarding is unaffected
(manual testing)
- [x] Ensure plugin webhooks are forwarded (manual testing)
- [x] Refactor existing tests to accomodate 'integration classification'
changes
- [x] Write new tests for plugin classification

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Leander Rodrigues 1 year ago
parent
commit
e62c7053f3

+ 0 - 1
pyproject.toml

@@ -561,7 +561,6 @@ module = [
     "sentry.middleware.access_log",
     "sentry.middleware.auth",
     "sentry.middleware.health",
-    "sentry.middleware.integrations.integration_control",
     "sentry.middleware.integrations.parsers.base",
     "sentry.middleware.integrations.parsers.slack",
     "sentry.middleware.ratelimit",

+ 123 - 0
src/sentry/middleware/integrations/classifications.py

@@ -0,0 +1,123 @@
+from __future__ import annotations
+
+import abc
+import logging
+import re
+from typing import TYPE_CHECKING, List, Mapping, Type, cast
+
+from django.http import HttpRequest
+from django.http.response import HttpResponseBase
+
+from .parsers import (
+    BitbucketRequestParser,
+    BitbucketServerRequestParser,
+    GithubEnterpriseRequestParser,
+    GithubRequestParser,
+    GitlabRequestParser,
+    JiraRequestParser,
+    JiraServerRequestParser,
+    MsTeamsRequestParser,
+    PluginRequestParser,
+    SlackRequestParser,
+    VstsRequestParser,
+)
+
+if TYPE_CHECKING:
+    from sentry.middleware.integrations.integration_control import ResponseHandler
+
+    from .parsers.base import BaseRequestParser
+
+
+class BaseClassification(abc.ABC):
+    def __init__(self, response_handler: ResponseHandler) -> None:
+        self.response_handler = response_handler
+
+    def should_operate(self, request: HttpRequest) -> bool:
+        """
+        Determines whether the classification should act on request.
+        Middleware will return self.get_response() if this function returns True.
+        """
+        raise NotImplementedError
+
+    def get_response(self, request: HttpRequest) -> HttpResponseBase:
+        """Parse the request and return a response."""
+        raise NotImplementedError
+
+
+class PluginClassification(BaseClassification):
+    plugin_prefix = "/plugins/"
+    """Prefix for plugin requests."""
+    logger = logging.getLogger(f"{__name__}.plugin")
+
+    def should_operate(self, request: HttpRequest) -> bool:
+        is_plugin = request.path.startswith(self.plugin_prefix)
+        if not is_plugin:
+            return False
+        rp = PluginRequestParser(request=request, response_handler=self.response_handler)
+        return rp.should_operate()
+
+    def get_response(self, request: HttpRequest) -> HttpResponseBase:
+        rp = PluginRequestParser(request=request, response_handler=self.response_handler)
+        self.logger.info("routing_request.plugin", extra={"path": request.path})
+        return rp.get_response()
+
+
+class IntegrationClassification(BaseClassification):
+    integration_prefix = "/extensions/"
+    """Prefix for all integration requests. See `src/sentry/web/urls.py`"""
+    setup_suffix = "/setup/"
+    """Suffix for PipelineAdvancerView on installation. See `src/sentry/web/urls.py`"""
+    logger = logging.getLogger(f"{__name__}.integration")
+    active_parsers: List[Type[BaseRequestParser]] = [
+        BitbucketRequestParser,
+        BitbucketServerRequestParser,
+        GithubEnterpriseRequestParser,
+        GithubRequestParser,
+        GitlabRequestParser,
+        JiraRequestParser,
+        JiraServerRequestParser,
+        MsTeamsRequestParser,
+        SlackRequestParser,
+        VstsRequestParser,
+    ]
+    integration_parsers: Mapping[str, Type[BaseRequestParser]] = {
+        cast(str, parser.provider): parser for parser in active_parsers
+    }
+
+    def _identify_provider(self, request: HttpRequest) -> str | None:
+        """
+        Parses the provider out of the request path
+            e.g. `/extensions/slack/commands/` -> `slack`
+        """
+        integration_prefix_regex = re.escape(self.integration_prefix)
+        provider_regex = rf"^{integration_prefix_regex}(\w+)"
+        result = re.search(provider_regex, request.path)
+        if not result:
+            self.logger.error("invalid_provider", extra={"path": request.path})
+            return None
+        return result[1]
+
+    def should_operate(self, request: HttpRequest) -> bool:
+        return request.path.startswith(self.integration_prefix) and not request.path.endswith(
+            self.setup_suffix
+        )
+
+    def get_response(self, request: HttpRequest) -> HttpResponseBase:
+        provider = self._identify_provider(request)
+        if not provider:
+            return self.response_handler(request)
+
+        parser_class = self.integration_parsers.get(provider)
+        if not parser_class:
+            self.logger.error(
+                "unknown_provider",
+                extra={"path": request.path, "provider": provider},
+            )
+            return self.response_handler(request)
+
+        parser = parser_class(
+            request=request,
+            response_handler=self.response_handler,
+        )
+        self.logger.info(f"routing_request.{parser.provider}", extra={"path": request.path})
+        return parser.get_response()

+ 22 - 75
src/sentry/middleware/integrations/integration_control.py

@@ -1,100 +1,47 @@
 from __future__ import annotations
 
 import logging
-import re
-from typing import Callable, Mapping, Type
+from typing import Callable, List, Type
 
-from django.http import HttpRequest, HttpResponse
+from django.http import HttpRequest
+from django.http.response import HttpResponseBase
 
-from sentry.silo import SiloMode
-
-from .parsers import (
-    BitbucketRequestParser,
-    BitbucketServerRequestParser,
-    GithubEnterpriseRequestParser,
-    GithubRequestParser,
-    GitlabRequestParser,
-    JiraRequestParser,
-    JiraServerRequestParser,
-    MsTeamsRequestParser,
-    SlackRequestParser,
-    VstsRequestParser,
+from sentry.middleware.integrations.classifications import (
+    BaseClassification,
+    IntegrationClassification,
+    PluginClassification,
 )
-from .parsers.base import BaseRequestParser
+from sentry.silo import SiloMode
 
 logger = logging.getLogger(__name__)
 
-ACTIVE_PARSERS = [
-    BitbucketRequestParser,
-    BitbucketServerRequestParser,
-    GithubEnterpriseRequestParser,
-    GithubRequestParser,
-    GitlabRequestParser,
-    JiraRequestParser,
-    JiraServerRequestParser,
-    MsTeamsRequestParser,
-    SlackRequestParser,
-    VstsRequestParser,
-]
+ResponseHandler = Callable[[HttpRequest], HttpResponseBase]
 
 
 class IntegrationControlMiddleware:
-    integration_prefix: str = "/extensions/"
-    """Prefix for all integration requests. See `src/sentry/web/urls.py`"""
-    setup_suffix: str = "/setup/"
-    """Suffix for PipelineAdvancerView on installation. See `src/sentry/web/urls.py`"""
+    classifications: List[Type[BaseClassification]] = [
+        IntegrationClassification,
+        PluginClassification,
+    ]
+    """Classifications to determine whether request must be parsed, sorted in priority order."""
 
-    integration_parsers: Mapping[str, Type[BaseRequestParser]] = {
-        parser.provider: parser for parser in ACTIVE_PARSERS
-    }
-
-    def __init__(self, get_response: Callable[[], HttpResponse]):
+    def __init__(self, get_response: ResponseHandler):
         self.get_response = get_response
 
-    def _identify_provider(self, request: HttpRequest) -> str | None:
-        """
-        Parses the provider out of the request path
-            e.g. `/extensions/slack/commands/` -> `slack`
-        """
-        integration_prefix_regex = re.escape(self.integration_prefix)
-        provider_regex = rf"^{integration_prefix_regex}(\w+)"
-        result = re.search(provider_regex, request.path)
-        if not result:
-            logger.error(
-                "integration_control.invalid_provider",
-                extra={"path": request.path},
-            )
-            return None
-        return result.group(1)
-
     def _should_operate(self, request: HttpRequest) -> bool:
         """
         Determines whether this middleware will operate or just pass the request along.
         """
-        is_correct_silo = SiloMode.get_current_mode() == SiloMode.CONTROL
-        is_integration = request.path.startswith(self.integration_prefix)
-        is_not_setup = not request.path.endswith(self.setup_suffix)
-        return is_correct_silo and is_integration and is_not_setup
+        return SiloMode.get_current_mode() == SiloMode.CONTROL
 
     def __call__(self, request: HttpRequest):
         if not self._should_operate(request):
             return self.get_response(request)
 
-        provider = self._identify_provider(request)
-        if not provider:
-            return self.get_response(request)
-
-        parser_class = self.integration_parsers.get(provider)
-        if not parser_class:
-            logger.error(
-                "integration_control.unknown_provider",
-                extra={"path": request.path, "provider": provider},
-            )
-            return self.get_response(request)
-
-        parser = parser_class(
-            request=request,
-            response_handler=self.get_response,
-        )
+        # Check request against each classification, if a match is found, return early
+        for classification in self.classifications:
+            _cls = classification(response_handler=self.get_response)
+            if _cls.should_operate(request):
+                return _cls.get_response(request)
 
-        return parser.get_response()
+        return self.get_response(request)

+ 3 - 1
src/sentry/middleware/integrations/parsers/__init__.py

@@ -6,18 +6,20 @@ from .gitlab import GitlabRequestParser
 from .jira import JiraRequestParser
 from .jira_server import JiraServerRequestParser
 from .msteams import MsTeamsRequestParser
+from .plugin import PluginRequestParser
 from .slack import SlackRequestParser
 from .vsts import VstsRequestParser
 
 __all__ = (
     "BitbucketRequestParser",
     "BitbucketServerRequestParser",
+    "GithubEnterpriseRequestParser",
     "GithubRequestParser",
     "GitlabRequestParser",
     "JiraRequestParser",
     "JiraServerRequestParser",
-    "GithubEnterpriseRequestParser",
     "MsTeamsRequestParser",
+    "PluginRequestParser",
     "SlackRequestParser",
     "VstsRequestParser",
 )

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

@@ -3,7 +3,7 @@ from __future__ import annotations
 import abc
 import logging
 from concurrent.futures import ThreadPoolExecutor, as_completed
-from typing import Callable, Mapping, Optional, Sequence
+from typing import TYPE_CHECKING, Mapping, Optional, Sequence
 
 from django.http import HttpRequest, HttpResponse
 from django.urls import ResolverMatch, resolve
@@ -19,6 +19,8 @@ from sentry.silo.client import RegionSiloClient
 from sentry.types.region import Region, get_region_for_organization
 
 logger = logging.getLogger(__name__)
+if TYPE_CHECKING:
+    from sentry.middleware.integrations.integration_control import ResponseHandler
 
 
 class RegionResult:
@@ -32,7 +34,7 @@ class BaseRequestParser(abc.ABC):
 
     @property
     def provider(self) -> str:
-        raise NotImplementedError("'provider' property is required by IntegrationControlMiddleware")
+        raise NotImplementedError("'provider' property is required by IntegrationClassification")
 
     @property
     def webhook_identifier(self) -> WebhookProviderIdentifier:
@@ -40,10 +42,12 @@ class BaseRequestParser(abc.ABC):
             "'webhook_identifier' property is required for outboxing. Refer to WebhookProviderIdentifier enum."
         )
 
-    def __init__(self, request: HttpRequest, response_handler: Callable):
+    def __init__(self, request: HttpRequest, response_handler: ResponseHandler):
         self.request = request
         self.match: ResolverMatch = resolve(self.request.path)
-        self.view_class = self.match.func.view_class  # type:ignore
+        self.view_class = None
+        if hasattr(self.match.func, "view_class"):
+            self.view_class = self.match.func.view_class
         self.response_handler = response_handler
 
     # Common Helpers

+ 50 - 0
src/sentry/middleware/integrations/parsers/plugin.py

@@ -0,0 +1,50 @@
+from __future__ import annotations
+
+import logging
+from typing import Any
+
+from sentry.middleware.integrations.parsers.base import BaseRequestParser
+from sentry.models.organizationmapping import OrganizationMapping
+from sentry.models.outbox import WebhookProviderIdentifier
+from sentry.types.region import RegionResolutionError, get_region_by_name
+from sentry_plugins.bitbucket.endpoints.webhook import BitbucketPluginWebhookEndpoint
+from sentry_plugins.github.webhooks.non_integration import GithubPluginWebhookEndpoint
+
+logger = logging.getLogger(__name__)
+
+
+class PluginRequestParser(BaseRequestParser):
+    provider = "plugins"
+    webhook_identifier = WebhookProviderIdentifier.LEGACY_PLUGIN
+
+    def should_operate(self) -> bool:
+        return self.view_class in {BitbucketPluginWebhookEndpoint, GithubPluginWebhookEndpoint}
+
+    def get_response(self):
+        """
+        Used for identifying regions from Github and Bitbucket plugin webhooks
+        """
+        organization_id = self.match.kwargs.get("organization_id")
+        logging_extra: dict[str, Any] = {"path": self.request.path}
+        if not organization_id:
+            logger.info("no_organization_id", extra=logging_extra)
+            return self.get_response_from_control_silo()
+
+        try:
+            mapping: OrganizationMapping = OrganizationMapping.objects.get(
+                organization_id=organization_id
+            )
+        except OrganizationMapping.DoesNotExist as e:
+            logging_extra["error"] = str(e)
+            logging_extra["organization_id"] = organization_id
+            logger.error("no_mapping", extra=logging_extra)
+            return self.get_response_from_control_silo()
+
+        try:
+            region = get_region_by_name(mapping.region_name)
+        except RegionResolutionError as e:
+            logging_extra["error"] = str(e)
+            logging_extra["mapping_id"] = mapping.id
+            logger.error("no_region", extra=logging_extra)
+            return self.get_response_from_control_silo()
+        return self.get_response_from_outbox_creation(regions=[region])

+ 1 - 0
src/sentry/models/outbox.py

@@ -113,6 +113,7 @@ class WebhookProviderIdentifier(IntEnum):
     JIRA_SERVER = 7
     GITHUB_ENTERPRISE = 8
     BITBUCKET_SERVER = 9
+    LEGACY_PLUGIN = 10
 
 
 def _ensure_not_null(k: str, v: Any) -> Any:

+ 0 - 4
src/sentry_plugins/bitbucket/endpoints/webhook.py

@@ -91,10 +91,6 @@ class BitbucketPluginWebhookEndpoint(View):
         return super().dispatch(request, *args, **kwargs)
 
     def post(self, request: Request, organization_id):
-        logger.error(
-            "bitbucket_plugin.deprecation_check",
-            extra={"organization_id": organization_id, "meta": request.META},
-        )
         try:
             organization = Organization.objects.get_from_cache(id=organization_id)
         except Organization.DoesNotExist:

+ 1 - 0
src/sentry_plugins/bitbucket/urls.py

@@ -6,5 +6,6 @@ urlpatterns = [
     re_path(
         r"^organizations/(?P<organization_id>[^\/]+)/webhook/$",
         BitbucketPluginWebhookEndpoint.as_view(),
+        name="sentry-plugins-bitbucket-webhook",
     )
 ]

+ 2 - 0
src/sentry_plugins/github/urls.py

@@ -6,9 +6,11 @@ urlpatterns = [
     re_path(
         r"^organizations/(?P<organization_id>[^\/]+)/webhook/$",
         GithubPluginWebhookEndpoint.as_view(),
+        name="sentry-plugins-github-webhook",
     ),
     re_path(
         r"^installations/webhook/$",
         GithubPluginIntegrationsWebhookEndpoint.as_view(),
+        name="sentry-plugins-github-installation-webhook",
     ),
 ]

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