From 9a323ff81c21b055454b514222bfb597098d13d2 Mon Sep 17 00:00:00 2001 From: Matt Hammerly Date: Fri, 16 May 2025 14:11:08 -0700 Subject: [PATCH 1/5] feat(codecov): initial client for requests to codecov api --- src/sentry/codecov/__init__.py | 0 src/sentry/codecov/client.py | 128 ++++++++++++++++++++++++++++ src/sentry/options/defaults.py | 2 + tests/sentry/codecov/test_client.py | 95 +++++++++++++++++++++ 4 files changed, 225 insertions(+) create mode 100644 src/sentry/codecov/__init__.py create mode 100644 src/sentry/codecov/client.py create mode 100644 tests/sentry/codecov/test_client.py diff --git a/src/sentry/codecov/__init__.py b/src/sentry/codecov/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/src/sentry/codecov/client.py b/src/sentry/codecov/client.py new file mode 100644 index 00000000000000..9858c447821100 --- /dev/null +++ b/src/sentry/codecov/client.py @@ -0,0 +1,128 @@ +import datetime +import logging +from enum import StrEnum +from typing import TypeAlias + +import requests +from rest_framework import status + +from sentry import options +from sentry.api.exceptions import SentryAPIException +from sentry.utils import jwt + +GitProviderId: TypeAlias = str + + +class GitProvider(StrEnum): + """ + Enum representing the Git provider that hosts the user/org that a + `CodecovApiClient` instance is acting on behalf of. + + Codecov doesn't require this to be GitHub, but that's all that's implemented + for now. + """ + GitHub = "github" + + +logger = logging.getLogger(__name__) + +TIMEOUT_SECONDS = 10 + + +class ConfigurationError(SentryAPIException): + status_code = status.HTTP_500_INTERNAL_SERVER_ERROR + code = "configuration-error" + + +class CodecovApiClient: + """ + Thin client for making JWT-authenticated requests to the Codecov API. + + For each request, Sentry creates and signs (HS256) a JWT with a key shared + with Codecov. This JWT contains information that Codecov needs to service + the request. + """ + + def _create_jwt(self): + now = int(datetime.datetime.now(datetime.UTC).timestamp()) + exp = now + 300 # 5 minutes + claims = { + "iss": "https://sentry.io", + "iat": now, + "exp": exp, + } + claims.update(self.custom_claims) + + return jwt.encode(claims, self.signing_secret, algorithm="HS256") + + def __init__( + self, + git_provider_user: GitProviderId, + git_provider: GitProvider = GitProvider.GitHub, + ): + """ + Creates a `CodecovApiClient`. + + :param git_provider_user: The ID of the current Sentry user's linked git + provider account, according to the git provider. + :param git_provider: The git provider that the above user's account is + hosted on. + """ + + if not (base_url := options.get("codecov.base-url")): + raise ConfigurationError() + + if not (signing_secret := options.get("codecov.api-bridge-signing-secret")): + raise ConfigurationError() + + self.base_url = base_url + self.signing_secret = signing_secret + self.custom_claims = { + "g_u": git_provider_user, + "g_p": git_provider, + } + + def get(self, endpoint: str, params=None, headers=None) -> requests.Response | None: + """ + Makes a GET request to the specified endpoint of the configured Codecov + API host with the provided params and headers. + + :param endpoint: The endpoint to request, without the host portion. For + examples: `/api/v2/gh/getsentry/users` or `/graphql` + :param params: Dictionary of query params. + :param headers: Dictionary of request headers. + """ + headers = headers or {} + token = self._create_jwt() + headers.update(jwt.authorization_header(token)) + + url = f"{self.base_url}{endpoint}" + try: + response = requests.get(url, params=params, headers=headers, timeout=TIMEOUT_SECONDS) + except Exception: + logger.exception("Error when making GET request") + return None + + return response + + def post(self, endpoint: str, data=None, headers=None) -> requests.Response | None: + """ + Makes a POST request to the specified endpoint of the configured Codecov + API host with the provided data and headers. + + :param endpoint: The endpoint to request, without the host portion. For + examples: `/api/v2/gh/getsentry/users` or `/graphql` + :param data: Dictionary of form data. + :param headers: Dictionary of request headers. + """ + headers = headers or {} + token = self._create_jwt() + headers.update(jwt.authorization_header(token)) + url = f"{self.base_url}{endpoint}" + try: + response = requests.post(url, data=data, headers=headers, timeout=TIMEOUT_SECONDS) + except Exception: + logger.exception("Error when making POST request") + return None + + return response diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 770ebea943c40f..999aa6c8bfb1ee 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -602,6 +602,8 @@ # Codecov Integration register("codecov.client-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) +register("codecov.base-url", default="https://api.codecov.io") +register("codecov.api-bridge-signing-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) # GitHub Integration register("github-app.id", default=0, flags=FLAG_AUTOMATOR_MODIFIABLE) diff --git a/tests/sentry/codecov/test_client.py b/tests/sentry/codecov/test_client.py new file mode 100644 index 00000000000000..0354cd4cdb399d --- /dev/null +++ b/tests/sentry/codecov/test_client.py @@ -0,0 +1,95 @@ +import datetime + +import pytest +from unittest.mock import patch + +from sentry.codecov.client import CodecovApiClient, ConfigurationError, GitProvider +from sentry.testutils.cases import TestCase +from sentry.testutils.silo import all_silo_test +from sentry.utils import jwt + + +@all_silo_test +class TestCodecovApiClient(TestCase): + def setUp(self): + self.test_git_provider_user = "12345" + self.test_base_url = "http://example.com" + self.test_secret = "test-secret-" + "a" * 20 + + self.test_timestamp = datetime.datetime.now(datetime.UTC) + self._mock_now = patch("datetime.datetime.now", return_value=self.test_timestamp) + + with self.options({ + "codecov.base-url": self.test_base_url, + "codecov.api-bridge-signing-secret": self.test_secret, + }): + self.codecov_client = CodecovApiClient(self.test_git_provider_user) + + def test_raises_configuration_error_without_base_url(self): + with self.options({ + "codecov.base-url": None, + "codecov.api-bridge-signing-secret": self.test_secret, + }): + with pytest.raises(ConfigurationError): + CodecovApiClient(self.test_git_provider_user) + + def test_raises_configuration_error_without_signing_secret(self): + with self.options({ + "codecov.base-url": self.test_base_url, + "codecov.api-bridge-signing-secret": None, + }): + with pytest.raises(ConfigurationError): + CodecovApiClient(self.test_git_provider_user) + + def test_creates_valid_jwt(self): + encoded_jwt = self.codecov_client._create_jwt() + + header = jwt.peek_header(encoded_jwt) + assert header == { + "typ": "JWT", + "alg": "HS256", + } + + # Ensure the claims are what we expect, separate from verifying the + # signature and standard claims + claims = jwt.peek_claims(encoded_jwt) + expected_iat = int(self.test_timestamp.timestamp()) + expected_exp = expected_iat + 300 + assert claims == { + "g_u": self.test_git_provider_user, + "g_p": GitProvider.GitHub.value, + "iss": "https://sentry.io", + "iat": expected_iat, + "exp": expected_exp, + } + + # Ensure we can verify the signature and whatall + jwt.decode(encoded_jwt, self.test_secret) + + @patch("requests.get") + def test_sends_get_request_with_jwt_auth_header(self, mock_get): + with patch.object(self.codecov_client, "_create_jwt", return_value="test"): + self.codecov_client.get("/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"}) + mock_get.assert_called_once_with( + "http://example.com/example/endpoint", + params={"example-param": "foo"}, + headers={ + "Authorization": "Bearer test", + "X_TEST_HEADER": "bar", + }, + timeout=10, + ) + + @patch("requests.post") + def test_sends_post_request_with_jwt_auth_header(self, mock_post): + with patch.object(self.codecov_client, "_create_jwt", return_value="test"): + self.codecov_client.post("/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"}) + mock_post.assert_called_once_with( + "http://example.com/example/endpoint", + data={"example-param": "foo"}, + headers={ + "Authorization": "Bearer test", + "X_TEST_HEADER": "bar", + }, + timeout=10, + ) From 8c6d2beee7eec100258d051ae065f5a45da72b70 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Fri, 16 May 2025 21:14:19 +0000 Subject: [PATCH 2/5] :hammer_and_wrench: apply pre-commit fixes --- src/sentry/codecov/client.py | 1 + tests/sentry/codecov/test_client.py | 40 ++++++++++++++++++----------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/sentry/codecov/client.py b/src/sentry/codecov/client.py index 9858c447821100..e70a00d5c1287b 100644 --- a/src/sentry/codecov/client.py +++ b/src/sentry/codecov/client.py @@ -21,6 +21,7 @@ class GitProvider(StrEnum): Codecov doesn't require this to be GitHub, but that's all that's implemented for now. """ + GitHub = "github" diff --git a/tests/sentry/codecov/test_client.py b/tests/sentry/codecov/test_client.py index 0354cd4cdb399d..556a0401d970f5 100644 --- a/tests/sentry/codecov/test_client.py +++ b/tests/sentry/codecov/test_client.py @@ -1,7 +1,7 @@ import datetime +from unittest.mock import patch import pytest -from unittest.mock import patch from sentry.codecov.client import CodecovApiClient, ConfigurationError, GitProvider from sentry.testutils.cases import TestCase @@ -19,25 +19,31 @@ def setUp(self): self.test_timestamp = datetime.datetime.now(datetime.UTC) self._mock_now = patch("datetime.datetime.now", return_value=self.test_timestamp) - with self.options({ - "codecov.base-url": self.test_base_url, - "codecov.api-bridge-signing-secret": self.test_secret, - }): + with self.options( + { + "codecov.base-url": self.test_base_url, + "codecov.api-bridge-signing-secret": self.test_secret, + } + ): self.codecov_client = CodecovApiClient(self.test_git_provider_user) def test_raises_configuration_error_without_base_url(self): - with self.options({ - "codecov.base-url": None, - "codecov.api-bridge-signing-secret": self.test_secret, - }): + with self.options( + { + "codecov.base-url": None, + "codecov.api-bridge-signing-secret": self.test_secret, + } + ): with pytest.raises(ConfigurationError): CodecovApiClient(self.test_git_provider_user) def test_raises_configuration_error_without_signing_secret(self): - with self.options({ - "codecov.base-url": self.test_base_url, - "codecov.api-bridge-signing-secret": None, - }): + with self.options( + { + "codecov.base-url": self.test_base_url, + "codecov.api-bridge-signing-secret": None, + } + ): with pytest.raises(ConfigurationError): CodecovApiClient(self.test_git_provider_user) @@ -69,7 +75,9 @@ def test_creates_valid_jwt(self): @patch("requests.get") def test_sends_get_request_with_jwt_auth_header(self, mock_get): with patch.object(self.codecov_client, "_create_jwt", return_value="test"): - self.codecov_client.get("/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"}) + self.codecov_client.get( + "/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"} + ) mock_get.assert_called_once_with( "http://example.com/example/endpoint", params={"example-param": "foo"}, @@ -83,7 +91,9 @@ def test_sends_get_request_with_jwt_auth_header(self, mock_get): @patch("requests.post") def test_sends_post_request_with_jwt_auth_header(self, mock_post): with patch.object(self.codecov_client, "_create_jwt", return_value="test"): - self.codecov_client.post("/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"}) + self.codecov_client.post( + "/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"} + ) mock_post.assert_called_once_with( "http://example.com/example/endpoint", data={"example-param": "foo"}, From 5ef00086db6f6289d633366d0787fc5b2c9dbc0b Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Wed, 21 May 2025 18:48:08 -0400 Subject: [PATCH 3/5] [wip] feat: forward webhooks to codecov Forwarding webhooks for Sentry running in Monolith mode is considered out of scope for this change depends on: https://github.com/getsentry/sentry/pull/91830 We want to forward webhooks Sentry receives to Codecov since Codecov depends on receiving webhooks from the code forges it supports. We're starting with only GitHub webhooks for now, but plan to support more in the future. We're using the existing WebhookPayload system to forward webhooks to Codecov by creating an additional WebhookPayload object in the GithubRequestParser. Codecov will do additional filtering on its end to filter out irrelevant webhooks so we don't have to do any filtering on Sentry's end, this is to avoid keeping track of what's relevant to Codecov in Sentry. I'm adding a destination_type column to the WebhookPayload so we can distinguish between payloads meant for Codecov and other Sentry regions because when we make the request in the drain_mailbox we want to run different logic, using the CodecovApiClient. I also made the choice to use a different mailbox_name so that the forwarding doesn't interfere with the webhooks being sent to the region silos. Finally, because we want to forward ALL webhooks to Codecov, that means we're forwarding installation events, a webhook that in Sentry gets processed in the control silo, so that gets its own mailbox identifier "installation". --- migrations_lockfile.txt | 2 +- src/sentry/codecov/client.py | 2 +- src/sentry/conf/server.py | 2 + .../migrations/0022_update_webhook_payload.py | 38 ++++++++ .../hybridcloud/models/webhookpayload.py | 21 ++++- .../hybridcloud/tasks/deliver_webhooks.py | 68 ++++++++++++++- .../middleware/hybrid_cloud/parser.py | 24 ++++- .../middleware/integrations/parsers/github.py | 12 +++ src/sentry/testutils/factories.py | 4 +- src/sentry/testutils/outbox.py | 33 +++++-- tests/sentry/codecov/test_client.py | 24 +++++ .../hybridcloud/models/test_webhookpayload.py | 33 ++++++- .../tasks/test_deliver_webhooks.py | 87 ++++++++++++++++++- .../middleware/hybrid_cloud/test_base.py | 21 ++++- .../integrations/parsers/test_github.py | 29 +++++++ 15 files changed, 378 insertions(+), 22 deletions(-) create mode 100644 src/sentry/hybridcloud/migrations/0022_update_webhook_payload.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index c05013ffdd7c12..8638c23ae654b3 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -9,7 +9,7 @@ explore: 0004_add_explore_last_visited_table feedback: 0004_index_together -hybridcloud: 0021_django_arrayfield_scope_list +hybridcloud: 0022_update_webhook_payload insights: 0001_add_starred_transactions_model diff --git a/src/sentry/codecov/client.py b/src/sentry/codecov/client.py index e70a00d5c1287b..5f97606f0f6dba 100644 --- a/src/sentry/codecov/client.py +++ b/src/sentry/codecov/client.py @@ -58,7 +58,7 @@ def _create_jwt(self): def __init__( self, - git_provider_user: GitProviderId, + git_provider_user: GitProviderId | None, git_provider: GitProvider = GitProvider.GitHub, ): """ diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index f8f6fbea704af9..7886c00fa679f9 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -3383,6 +3383,8 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]: SENTRY_REPLAYS_SERVICE_URL = "http://localhost:8090" +# Codecov is disabled by default +SENTRY_CODECOV_URL = None SENTRY_ISSUE_ALERT_HISTORY = "sentry.rules.history.backends.postgres.PostgresRuleHistoryBackend" SENTRY_ISSUE_ALERT_HISTORY_OPTIONS: dict[str, Any] = {} diff --git a/src/sentry/hybridcloud/migrations/0022_update_webhook_payload.py b/src/sentry/hybridcloud/migrations/0022_update_webhook_payload.py new file mode 100644 index 00000000000000..8fb8ecd3b5c56d --- /dev/null +++ b/src/sentry/hybridcloud/migrations/0022_update_webhook_payload.py @@ -0,0 +1,38 @@ +# Generated by Django 5.2.1 on 2025-05-21 20:33 + +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + # This flag is used to mark that a migration shouldn't be automatically run in production. + # This should only be used for operations where it's safe to run the migration after your + # code has deployed. So this should not be used for most operations that alter the schema + # of a table. + # Here are some things that make sense to mark as post deployment: + # - Large data migrations. Typically we want these to be run manually so that they can be + # monitored and not block the deploy for a long period of time while they run. + # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to + # run this outside deployments so that we don't block them. Note that while adding an index + # is a schema change, it's completely safe to run the operation after the code has deployed. + # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment + + is_post_deployment = False + + dependencies = [ + ("hybridcloud", "0021_django_arrayfield_scope_list"), + ] + + operations = [ + migrations.AddField( + model_name="webhookpayload", + name="destination_type", + field=models.CharField(db_default="sentry_region"), + ), + migrations.AlterField( + model_name="webhookpayload", + name="region_name", + field=models.CharField(null=True), + ), + ] diff --git a/src/sentry/hybridcloud/models/webhookpayload.py b/src/sentry/hybridcloud/models/webhookpayload.py index 15767de3b64b47..ba7e6646a78f5f 100644 --- a/src/sentry/hybridcloud/models/webhookpayload.py +++ b/src/sentry/hybridcloud/models/webhookpayload.py @@ -4,7 +4,7 @@ from typing import Any, Self from django.db import models -from django.db.models import Case, ExpressionWrapper, F, IntegerField, Value, When +from django.db.models import Case, ExpressionWrapper, F, IntegerField, TextChoices, Value, When from django.http import HttpRequest from django.utils import timezone @@ -19,13 +19,25 @@ BACKOFF_RATE = 1.4 +class DestinationType(TextChoices): + SENTRY_REGION = "sentry_region" + CODECOV = "codecov" + + @control_silo_model class WebhookPayload(Model): __relocation_scope__ = RelocationScope.Excluded mailbox_name = models.CharField(null=False, blank=False) provider = models.CharField(null=True, blank=True) - region_name = models.CharField(null=False) + + # Destination attributes + # Table is constantly being deleted from so let's make this non-nullable with a default value, since the table should be small at any given point in time. + destination_type = models.CharField( + choices=DestinationType.choices, null=False, db_default=DestinationType.SENTRY_REGION + ) + region_name = models.CharField(null=True) + # May need to add organization_id in the future for debugging. integration_id = models.BigIntegerField(null=True) @@ -69,6 +81,7 @@ class Meta: __repr__ = sane_repr( "mailbox_name", + "destination_type", "region_name", "schedule_for", "attempts", @@ -93,7 +106,8 @@ def get_attributes_from_request( def create_from_request( cls, *, - region: str, + destination_type: DestinationType, + region: str | None, provider: str, identifier: int | str, request: HttpRequest, @@ -103,6 +117,7 @@ def create_from_request( return cls.objects.create( mailbox_name=f"{provider}:{identifier}", provider=provider, + destination_type=destination_type, region_name=region, integration_id=integration_id, **cls.get_attributes_from_request(request), diff --git a/src/sentry/hybridcloud/tasks/deliver_webhooks.py b/src/sentry/hybridcloud/tasks/deliver_webhooks.py index d2fc4e00dfde60..ac698a03d6b04a 100644 --- a/src/sentry/hybridcloud/tasks/deliver_webhooks.py +++ b/src/sentry/hybridcloud/tasks/deliver_webhooks.py @@ -11,8 +11,14 @@ from rest_framework import status from sentry import options +from sentry.codecov.client import CodecovApiClient, ConfigurationError from sentry.exceptions import RestrictedIPAddress -from sentry.hybridcloud.models.webhookpayload import BACKOFF_INTERVAL, MAX_ATTEMPTS, WebhookPayload +from sentry.hybridcloud.models.webhookpayload import ( + BACKOFF_INTERVAL, + MAX_ATTEMPTS, + DestinationType, + WebhookPayload, +) from sentry.shared_integrations.exceptions import ( ApiConflictError, ApiConnectionResetError, @@ -25,7 +31,7 @@ from sentry.tasks.base import instrumented_task from sentry.taskworker.config import TaskworkerConfig from sentry.taskworker.namespaces import hybridcloud_control_tasks -from sentry.types.region import get_region_by_name +from sentry.types.region import Region, get_region_by_name from sentry.utils import metrics logger = logging.getLogger(__name__) @@ -403,12 +409,24 @@ def deliver_message(payload: WebhookPayload) -> None: def perform_request(payload: WebhookPayload) -> None: + destination_type = payload.destination_type + + match destination_type: + case DestinationType.SENTRY_REGION: + region = get_region_by_name(name=payload.region_name) + perform_region_request(region, payload) + case DestinationType.CODECOV: + perform_codecov_request(payload) + case _: + raise ValueError(f"Unknown destination type: {destination_type!r}") + + +def perform_region_request(region: Region, payload: WebhookPayload) -> None: logging_context: dict[str, str | int] = { "payload_id": payload.id, "mailbox_name": payload.mailbox_name, "attempt": payload.attempts, } - region = get_region_by_name(name=payload.region_name) try: client = RegionSiloClient(region=region) @@ -521,3 +539,47 @@ def perform_request(payload: WebhookPayload) -> None: extra={"error": str(err), "response_code": response_code, **logging_context}, ) raise DeliveryFailed() from err + + +def perform_codecov_request(payload: WebhookPayload) -> None: + logging_context: dict[str, str | int] = { + "payload_id": payload.id, + "mailbox_name": payload.mailbox_name, + "attempt": payload.attempts, + "request_method": payload.request_method, + "request_path": payload.request_path, + } + + with metrics.timer( + "hybridcloud.deliver_webhooks.send_request_to_codecov", + ): + try: + client = CodecovApiClient(None) + response = client.post( + endpoint=payload.request_path, + data=payload.request_body.encode("utf-8"), + headers=orjson.loads(payload.request_headers), + ) + except ConfigurationError as err: + metrics.incr( + "hybridcloud.deliver_webhooks.send_request_to_codecov.codecov_configuration_error", + ) + logger.warning( + "deliver_webhooks.send_request_to_codecov.codecov_configuration_error", + extra={"error": str(err), **logging_context}, + ) + return + + if response is None: + metrics.incr( + "hybridcloud.deliver_webhooks.send_request_to_codecov.failure", + ) + return + + logger.debug( + "deliver_webhooks.send_request_to_codecov.success", + extra={ + "status": response.status_code, + **logging_context, + }, + ) diff --git a/src/sentry/integrations/middleware/hybrid_cloud/parser.py b/src/sentry/integrations/middleware/hybrid_cloud/parser.py index 3b40945eee1972..740aa5722393d6 100644 --- a/src/sentry/integrations/middleware/hybrid_cloud/parser.py +++ b/src/sentry/integrations/middleware/hybrid_cloud/parser.py @@ -14,7 +14,7 @@ from sentry.api.base import ONE_DAY from sentry.constants import ObjectStatus -from sentry.hybridcloud.models.webhookpayload import WebhookPayload +from sentry.hybridcloud.models.webhookpayload import DestinationType, WebhookPayload from sentry.hybridcloud.outbox.category import WebhookProviderIdentifier from sentry.hybridcloud.services.organization_mapping import organization_mapping_service from sentry.integrations.middleware.metrics import ( @@ -178,6 +178,7 @@ def get_response_from_webhookpayload( shard_identifier = identifier or self.webhook_identifier.value for region in regions: WebhookPayload.create_from_request( + destination_type=DestinationType.SENTRY_REGION, region=region.name, provider=self.provider, identifier=shard_identifier, @@ -337,3 +338,24 @@ def get_regions_from_organizations( def get_default_missing_integration_response(self) -> HttpResponse: return HttpResponse(status=status.HTTP_400_BAD_REQUEST) + + # Forwarding Helpers + + def forward_to_codecov( + self, + identifier: int | str | None = None, + integration_id: int | None = None, + ): + shard_identifier = f"codecov:{identifier or self.webhook_identifier.value}" + + # create webhookpayloads for each service + WebhookPayload.create_from_request( + destination_type=DestinationType.CODECOV, + region=None, + provider=self.provider, + identifier=shard_identifier, + integration_id=integration_id, + request=self.request, + ) + + return HttpResponse(status=status.HTTP_202_ACCEPTED) diff --git a/src/sentry/middleware/integrations/parsers/github.py b/src/sentry/middleware/integrations/parsers/github.py index b7db6ecf1d2d19..1f542cb2e8a18e 100644 --- a/src/sentry/middleware/integrations/parsers/github.py +++ b/src/sentry/middleware/integrations/parsers/github.py @@ -5,6 +5,7 @@ from typing import Any import orjson +from django.conf import settings from django.http import HttpResponse from sentry.hybridcloud.outbox.category import WebhookProviderIdentifier @@ -53,6 +54,11 @@ def get_response(self): return HttpResponse(status=400) if event.get("installation") and event.get("action") in {"created", "deleted"}: + if settings.SENTRY_CODECOV_URL: # check if codecov is enabled + self.forward_to_codecov( + identifier="installation", + integration_id=None, + ) return self.get_response_from_control_silo() try: @@ -67,6 +73,12 @@ def get_response(self): if len(regions) == 0: return self.get_default_missing_integration_response() + if settings.SENTRY_CODECOV_URL: # check if codecov is enabled + self.forward_to_codecov( + identifier=integration.id, + integration_id=integration.id, + ) + return self.get_response_from_webhookpayload( regions=regions, identifier=integration.id, integration_id=integration.id ) diff --git a/src/sentry/testutils/factories.py b/src/sentry/testutils/factories.py index 78c9f1633eebaa..46d50321c18977 100644 --- a/src/sentry/testutils/factories.py +++ b/src/sentry/testutils/factories.py @@ -1983,7 +1983,9 @@ def create_request_access( @staticmethod @assume_test_silo_mode(SiloMode.CONTROL) - def create_webhook_payload(mailbox_name: str, region_name: str, **kwargs) -> WebhookPayload: + def create_webhook_payload( + mailbox_name: str, region_name: str | None, **kwargs + ) -> WebhookPayload: payload_kwargs = { "request_method": "POST", "request_path": "/extensions/github/webhook/", diff --git a/src/sentry/testutils/outbox.py b/src/sentry/testutils/outbox.py index c4a289bc5c25d2..7735c42448434e 100644 --- a/src/sentry/testutils/outbox.py +++ b/src/sentry/testutils/outbox.py @@ -8,7 +8,7 @@ from django.core.handlers.wsgi import WSGIRequest from sentry.hybridcloud.models.outbox import OutboxBase -from sentry.hybridcloud.models.webhookpayload import THE_PAST, WebhookPayload +from sentry.hybridcloud.models.webhookpayload import THE_PAST, DestinationType, WebhookPayload from sentry.hybridcloud.tasks.deliver_from_outbox import ( enqueue_outbox_jobs, enqueue_outbox_jobs_control, @@ -65,6 +65,7 @@ def assert_webhook_payloads_for_mailbox( request: WSGIRequest, mailbox_name: str, region_names: list[str], + destination_types: dict[DestinationType, int] | None = None, ): """ A test method for asserting that a webhook payload is properly queued for @@ -72,12 +73,13 @@ def assert_webhook_payloads_for_mailbox( :param request: :param mailbox_name: The mailbox name that messages should be found in. - :param region_names: The regions each messages should be queued for + :param region_names: Optional list of regions each messages should be queued for + :param destination_type: The destination type of the messages """ expected_payload = WebhookPayload.get_attributes_from_request(request=request) region_names_set = set(region_names) messages = WebhookPayload.objects.filter(mailbox_name=mailbox_name) - message_count = messages.count() + message_count = messages.filter(region_name__isnull=False).count() if message_count != len(region_names_set): raise Exception( f"Mismatch: Found {message_count} WebhookPayload but {len(region_names_set)} region_names" @@ -89,11 +91,24 @@ def assert_webhook_payloads_for_mailbox( assert message.request_body == expected_payload["request_body"] assert message.schedule_for == THE_PAST assert message.attempts == 0 - try: - region_names_set.remove(message.region_name) - except KeyError: - raise Exception( - f"Found ControlOutbox for '{message.region_name}', which was not in region_names: {str(region_names_set)}" - ) + + if destination_types: + assert message.destination_type in destination_types + destination_types[message.destination_type] -= 1 + if destination_types[message.destination_type] == 0: + del destination_types[message.destination_type] + + if message.region_name: + try: + region_names_set.remove(message.region_name) + except KeyError: + raise Exception( + f"Found ControlOutbox for '{message.region_name}', which was not in region_names: {str(region_names_set)}" + ) if len(region_names_set) != 0: raise Exception(f"WebhookPayload not found for some region_names: {str(region_names_set)}") + + if destination_types and len(destination_types) != 0: + raise Exception( + f"WebhookPayload not found for some destination_types: {str(destination_types)}" + ) diff --git a/tests/sentry/codecov/test_client.py b/tests/sentry/codecov/test_client.py index 556a0401d970f5..185cb1e236b16b 100644 --- a/tests/sentry/codecov/test_client.py +++ b/tests/sentry/codecov/test_client.py @@ -103,3 +103,27 @@ def test_sends_post_request_with_jwt_auth_header(self, mock_post): }, timeout=10, ) + + @patch("requests.post") + def test_sends_post_request_with_jwt_auth_header_empty_git_provider_user(self, mock_post): + with self.options( + { + "codecov.base-url": self.test_base_url, + "codecov.api-bridge-signing-secret": self.test_secret, + } + ): + self.codecov_client = CodecovApiClient(None) + + with patch.object(self.codecov_client, "_create_jwt", return_value="test"): + self.codecov_client.post( + "/example/endpoint", {"example-param": "foo"}, {"X_TEST_HEADER": "bar"} + ) + mock_post.assert_called_once_with( + "http://example.com/example/endpoint", + data={"example-param": "foo"}, + headers={ + "Authorization": "Bearer test", + "X_TEST_HEADER": "bar", + }, + timeout=10, + ) diff --git a/tests/sentry/hybridcloud/models/test_webhookpayload.py b/tests/sentry/hybridcloud/models/test_webhookpayload.py index 0d746e981dad4b..765932ec525ec4 100644 --- a/tests/sentry/hybridcloud/models/test_webhookpayload.py +++ b/tests/sentry/hybridcloud/models/test_webhookpayload.py @@ -4,7 +4,12 @@ from django.utils import timezone from sentry.hybridcloud.models import WebhookPayload -from sentry.hybridcloud.models.webhookpayload import BACKOFF_INTERVAL, BACKOFF_RATE, MAX_ATTEMPTS +from sentry.hybridcloud.models.webhookpayload import ( + BACKOFF_INTERVAL, + BACKOFF_RATE, + MAX_ATTEMPTS, + DestinationType, +) from sentry.testutils.cases import TestCase from sentry.testutils.silo import control_silo_test @@ -19,6 +24,7 @@ def test_create_from_request(self) -> None: content_type="application/json", ) hook = WebhookPayload.create_from_request( + destination_type=DestinationType.SENTRY_REGION, region="us", provider="github", identifier=123, @@ -35,6 +41,31 @@ def test_create_from_request(self) -> None: ) assert hook.request_body == '{"installation": {"id": "github:1"}}' + def test_create_from_request_for_codeocv(self) -> None: + factory = RequestFactory() + request = factory.post( + "/extensions/github/webhook/", + data={"installation": {"id": "github:1"}}, + content_type="application/json", + ) + hook = WebhookPayload.create_from_request( + destination_type=DestinationType.CODECOV, + region=None, + provider="github", + identifier="installation", + request=request, + integration_id=None, + ) + assert hook.mailbox_name == "github:installation" + assert hook.provider == "github" + assert hook.request_method == request.method + assert hook.request_path == request.get_full_path() + assert ( + hook.request_headers + == '{"Cookie":"","Content-Length":"36","Content-Type":"application/json"}' + ) + assert hook.request_body == '{"installation": {"id": "github:1"}}' + def test_schedule_next_attempt_moves_forward(self) -> None: hook = self.create_webhook_payload("jira:123", "us") start = timezone.now() diff --git a/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py b/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py index 04b84d587edbb0..c16d4b2e41d5fe 100644 --- a/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py +++ b/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py @@ -7,7 +7,7 @@ from requests.exceptions import ConnectionError, ReadTimeout from sentry import options -from sentry.hybridcloud.models.webhookpayload import MAX_ATTEMPTS, WebhookPayload +from sentry.hybridcloud.models.webhookpayload import MAX_ATTEMPTS, DestinationType, WebhookPayload from sentry.hybridcloud.tasks import deliver_webhooks from sentry.hybridcloud.tasks.deliver_webhooks import ( MAX_MAILBOX_DRAIN, @@ -17,6 +17,7 @@ ) from sentry.testutils.cases import TestCase from sentry.testutils.factories import Factories +from sentry.testutils.helpers.options import override_options from sentry.testutils.region import override_regions from sentry.testutils.silo import control_silo_test from sentry.types.region import Region, RegionCategory, RegionResolutionError @@ -266,6 +267,20 @@ def create_payloads(num: int, mailbox: str) -> list[WebhookPayload]: return created +def create_payloads_with_destination_type( + num: int, mailbox: str, destination_type: DestinationType +) -> list[WebhookPayload]: + created = [] + for _ in range(0, num): + hook = Factories.create_webhook_payload( + mailbox_name=mailbox, + region_name=None, + destination_type=destination_type, + ) + created.append(hook) + return created + + @control_silo_test class DrainMailboxTest(TestCase): @responses.activate @@ -330,6 +345,76 @@ def test_drain_success(self) -> None: # Mailbox should be empty assert not WebhookPayload.objects.filter().exists() + @responses.activate + @override_options( + { + "codecov.base-url": "https://codecov.io", + "codecov.api-bridge-signing-secret": "test", + } + ) + @override_regions(region_config) + def test_drain_success_codecov(self) -> None: + responses.add( + responses.POST, + "https://codecov.io/extensions/github/webhook/", + status=200, + body="", + ) + + records = create_payloads_with_destination_type( + 3, "github:codecov:123", DestinationType.CODECOV + ) + drain_mailbox(records[0].id) + + # Mailbox should be empty + assert not WebhookPayload.objects.filter().exists() + + @responses.activate + @override_regions(region_config) + def test_drain_codecov_configuration_error(self) -> None: + responses.add( + responses.POST, + "https://codecov.io/extensions/github/webhook/", + status=200, + body="", + ) + + records = create_payloads_with_destination_type( + 3, "github:codecov:123", DestinationType.CODECOV + ) + drain_mailbox(records[0].id) + + # We don't retry codecov requests no matter what + hook = WebhookPayload.objects.filter().first() + assert hook is None + assert len(responses.calls) == 0 + + @responses.activate + @override_options( + { + "codecov.base-url": "https://codecov.io", + "codecov.api-bridge-signing-secret": "test", + } + ) + @override_regions(region_config) + def test_drain_codecov_request_error(self) -> None: + responses.add( + responses.POST, + "https://codecov.io/extensions/github/webhook/", + status=400, + body="", + ) + + records = create_payloads_with_destination_type( + 3, "github:codecov:123", DestinationType.CODECOV + ) + drain_mailbox(records[0].id) + + # We don't retry codecov requests no matter what + hook = WebhookPayload.objects.filter().first() + assert hook is None + assert len(responses.calls) == 3 + @responses.activate @override_regions(region_config) def test_drain_time_limit(self) -> None: diff --git a/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py b/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py index 818faf5f296a8d..0e587cf9219000 100644 --- a/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py +++ b/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py @@ -7,7 +7,7 @@ from rest_framework import status from sentry.constants import ObjectStatus -from sentry.hybridcloud.models.webhookpayload import WebhookPayload +from sentry.hybridcloud.models.webhookpayload import DestinationType, WebhookPayload from sentry.hybridcloud.outbox.category import WebhookProviderIdentifier from sentry.integrations.middleware.hybrid_cloud.parser import BaseRequestParser from sentry.integrations.middleware.metrics import MiddlewareHaltReason @@ -118,6 +118,25 @@ class MockParser(BaseRequestParser): assert payload.mailbox_name == "slack:0" assert payload.request_path assert payload.request_method + assert payload.destination_type == DestinationType.SENTRY_REGION + + @override_settings(SILO_MODE=SiloMode.CONTROL) + def test_forward_to_codecov(self): + class MockParser(BaseRequestParser): + webhook_identifier = WebhookProviderIdentifier.GITHUB + provider = "github" + + parser = MockParser(self.request, self.response_handler) + + parser.forward_to_codecov() + payloads = WebhookPayload.objects.all() + assert len(payloads) == 1 + for payload in payloads: + assert payload.region_name is None + assert payload.mailbox_name == "github:codecov:1" + assert payload.request_path + assert payload.request_method + assert payload.destination_type == DestinationType.CODECOV @override_settings(SILO_MODE=SiloMode.CONTROL) def test_get_organizations_from_integration_success(self): diff --git a/tests/sentry/middleware/integrations/parsers/test_github.py b/tests/sentry/middleware/integrations/parsers/test_github.py index a24844b0986c28..6b45181df98d28 100644 --- a/tests/sentry/middleware/integrations/parsers/test_github.py +++ b/tests/sentry/middleware/integrations/parsers/test_github.py @@ -6,6 +6,7 @@ from rest_framework import status from sentry.hybridcloud.models.outbox import outbox_context +from sentry.hybridcloud.models.webhookpayload import DestinationType from sentry.integrations.models.integration import Integration from sentry.integrations.models.organization_integration import OrganizationIntegration from sentry.middleware.integrations.parsers.github import GithubRequestParser @@ -135,6 +136,34 @@ def test_webhook_outbox_creation(self): region_names=[region.name], ) + @override_settings(SILO_MODE=SiloMode.CONTROL, SENTRY_CODECOV_URL="https://api.codecov.io") + @override_regions(region_config) + def test_webhook_for_codecov(self): + integration = self.get_integration() + 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 == status.HTTP_202_ACCEPTED + assert response.content == b"" + assert_webhook_payloads_for_mailbox( + request=request, + mailbox_name=f"github:{integration.id}", + region_names=[region.name], + destination_types={DestinationType.SENTRY_REGION: 1}, + ) + assert_webhook_payloads_for_mailbox( + request=request, + mailbox_name=f"github:codecov:{integration.id}", + region_names=[], + destination_types={DestinationType.CODECOV: 1}, + ) + @override_settings(SILO_MODE=SiloMode.CONTROL) @override_regions(region_config) @responses.activate From f5e91992cee43504510f0b44eea4313590501e52 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Thu, 22 May 2025 17:28:27 -0400 Subject: [PATCH 4/5] modify the request we forward to codecov Codecov wants to receive the forwarded webhooks at a specific endpoint in a specific format, this is because we may want to have separate logic for processing webhooks forwarded from Sentry vs. directly received from github. The choice to include the event in the request body was just a preference since even though we're forwarding the webhook i want to discern that this is a request that Sentry is sending Codecov. --- .../hybridcloud/tasks/deliver_webhooks.py | 59 +++++++++++++------ .../tasks/test_deliver_webhooks.py | 2 +- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/sentry/hybridcloud/tasks/deliver_webhooks.py b/src/sentry/hybridcloud/tasks/deliver_webhooks.py index ac698a03d6b04a..2f8c203e2924a2 100644 --- a/src/sentry/hybridcloud/tasks/deliver_webhooks.py +++ b/src/sentry/hybridcloud/tasks/deliver_webhooks.py @@ -542,6 +542,9 @@ def perform_region_request(region: Region, payload: WebhookPayload) -> None: def perform_codecov_request(payload: WebhookPayload) -> None: + """ + We're dont retrying Codecov forwarding requests for now. We want to prove out that it would work. + """ logging_context: dict[str, str | int] = { "payload_id": payload.id, "mailbox_name": payload.mailbox_name, @@ -553,12 +556,39 @@ def perform_codecov_request(payload: WebhookPayload) -> None: with metrics.timer( "hybridcloud.deliver_webhooks.send_request_to_codecov", ): + # transform request to match what codecov is expecting + if payload.request_path.strip("/") != "extensions/github/webhook": + metrics.incr( + "hybridcloud.deliver_webhooks.send_request_to_codecov.unexpected_path", + ) + return + try: + endpoint = "/webhooks/sentry" + headers = orjson.loads(payload.request_headers) + data = { + "event": headers.get("HTTP_X_GITHUB_EVENT", "unknown"), + "payload": orjson.loads(payload.request_body), + } client = CodecovApiClient(None) response = client.post( - endpoint=payload.request_path, - data=payload.request_body.encode("utf-8"), - headers=orjson.loads(payload.request_headers), + endpoint=endpoint, + data=data, + headers=headers, + ) + + if response is None: + metrics.incr( + "hybridcloud.deliver_webhooks.send_request_to_codecov.failure", + ) + return + + logger.debug( + "deliver_webhooks.send_request_to_codecov.success", + extra={ + "status": response.status_code, + **logging_context, + }, ) except ConfigurationError as err: metrics.incr( @@ -569,17 +599,12 @@ def perform_codecov_request(payload: WebhookPayload) -> None: extra={"error": str(err), **logging_context}, ) return - - if response is None: - metrics.incr( - "hybridcloud.deliver_webhooks.send_request_to_codecov.failure", - ) - return - - logger.debug( - "deliver_webhooks.send_request_to_codecov.success", - extra={ - "status": response.status_code, - **logging_context, - }, - ) + except orjson.JSONDecodeError as err: + metrics.incr( + "hybridcloud.deliver_webhooks.send_request_to_codecov.json_decode_error", + ) + logger.warning( + "deliver_webhooks.send_request_to_codecov.json_decode_error", + extra={"error": str(err), **logging_context}, + ) + return diff --git a/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py b/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py index c16d4b2e41d5fe..b329bc0eba89be 100644 --- a/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py +++ b/tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py @@ -400,7 +400,7 @@ def test_drain_codecov_configuration_error(self) -> None: def test_drain_codecov_request_error(self) -> None: responses.add( responses.POST, - "https://codecov.io/extensions/github/webhook/", + "https://codecov.io/webhooks/sentry", status=400, body="", ) From 8a8a5d49a352e553ad1e3a47cb365b4854e425d6 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Thu, 22 May 2025 18:03:39 -0400 Subject: [PATCH 5/5] put the codecov webhook forwarding behind option --- src/sentry/middleware/integrations/parsers/github.py | 9 +++++++-- src/sentry/options/defaults.py | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/sentry/middleware/integrations/parsers/github.py b/src/sentry/middleware/integrations/parsers/github.py index 1f542cb2e8a18e..a6852de1b6e088 100644 --- a/src/sentry/middleware/integrations/parsers/github.py +++ b/src/sentry/middleware/integrations/parsers/github.py @@ -8,6 +8,7 @@ from django.conf import settings from django.http import HttpResponse +from sentry import options from sentry.hybridcloud.outbox.category import WebhookProviderIdentifier from sentry.integrations.github.webhook import ( GitHubIntegrationsWebhookEndpoint, @@ -54,7 +55,9 @@ def get_response(self): return HttpResponse(status=400) if event.get("installation") and event.get("action") in {"created", "deleted"}: - if settings.SENTRY_CODECOV_URL: # check if codecov is enabled + if settings.SENTRY_CODECOV_URL and options.get( + "codecov.forward-webhooks" + ): # check if codecov is enabled self.forward_to_codecov( identifier="installation", integration_id=None, @@ -73,7 +76,9 @@ def get_response(self): if len(regions) == 0: return self.get_default_missing_integration_response() - if settings.SENTRY_CODECOV_URL: # check if codecov is enabled + if settings.SENTRY_CODECOV_URL and options.get( + "codecov.forward-webhooks" + ): # check if codecov is enabled self.forward_to_codecov( identifier=integration.id, integration_id=integration.id, diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 91f43a79939d13..ffad94bf0ce28e 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -604,6 +604,7 @@ register("codecov.client-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) register("codecov.base-url", default="https://api.codecov.io") register("codecov.api-bridge-signing-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) +register("codecov.forward-webhooks", default=False, flags=FLAG_AUTOMATOR_MODIFIABLE) # GitHub Integration register("github-app.id", default=0, flags=FLAG_AUTOMATOR_MODIFIABLE)