From 85778f97c2ccbfcb8997fc68f58293b156ce4d88 Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini Date: Thu, 29 Oct 2026 13:47:51 +0100 Subject: [PATCH] fix(gitlab): Make webhook creation retry-safe Persist a reconciliation identity before creating project hooks so retries can list and adopt the original hook instead of creating duplicates. Paginate hook discovery and serialize reconciliation per repository. --- src/sentry/integrations/gitlab/client.py | 43 ++- src/sentry/integrations/gitlab/tasks.py | 216 ++++++++++--- src/sentry/shared_integrations/client/base.py | 9 +- .../tasks/test_update_all_project_webhooks.py | 302 +++++++++++++++--- .../sentry/integrations/gitlab/test_client.py | 44 +++ 5 files changed, 519 insertions(+), 95 deletions(-) diff --git a/src/sentry/integrations/gitlab/client.py b/src/sentry/integrations/gitlab/client.py index 28f79991cc52..093c3bffb839 100644 --- a/src/sentry/integrations/gitlab/client.py +++ b/src/sentry/integrations/gitlab/client.py @@ -49,7 +49,7 @@ REFRESH_LOCK_WAIT_S = 10 -# Leave enough room for the proxy's retry envelope to fail before the task's 60-second deadline. +# Bound each request so a full paginated reconciliation fits within the task deadline. GITLAB_WEBHOOK_REQUEST_TIMEOUT = 5 @@ -631,12 +631,19 @@ def update_issue_status(self, project_id: str, issue_iid: str, state: str): path = GitLabApiClientPath.issue.format(project=safe_quote(project_id), issue=issue_iid) return self.put(path, data={"state_event": state}) - def _project_webhook_data(self) -> dict[str, Any]: - hook_uri = reverse("sentry-extensions-gitlab-webhook") + def project_webhook_url(self, reconciliation_id: str | None = None) -> str: + hook_url = absolute_uri(reverse("sentry-extensions-gitlab-webhook")) + if reconciliation_id is not None: + hook_url = ( + f"{hook_url}?sentry_webhook_reconciliation_id={quote(reconciliation_id, safe='')}" + ) + return hook_url + + def _project_webhook_data(self, reconciliation_id: str | None = None) -> dict[str, Any]: model = self.installation.model # GitLab clears the token when the URL changes, so always send both together. return { - "url": absolute_uri(hook_uri), + "url": self.project_webhook_url(reconciliation_id), "token": "{}:{}".format(model.external_id, model.metadata["webhook_secret"]), "merge_requests_events": True, "push_events": True, @@ -645,7 +652,9 @@ def _project_webhook_data(self) -> dict[str, Any]: "enable_ssl_verification": model.metadata["verify_ssl"], } - def create_project_webhook(self, project_id): + def create_project_webhook( + self, project_id: int | str, reconciliation_id: str | None = None + ) -> int | str: """Create a webhook on a project. See https://docs.gitlab.com/ee/api/projects.html#add-project-hook @@ -653,22 +662,36 @@ def create_project_webhook(self, project_id): path = GitLabApiClientPath.project_hooks.format(project=safe_quote(project_id)) resp = self.post( path, - data=self._project_webhook_data(), + data=self._project_webhook_data(reconciliation_id), timeout=GITLAB_WEBHOOK_REQUEST_TIMEOUT, api_request_type=GitLabApiRequestType.CREATE_PROJECT_WEBHOOK, ) return resp["id"] - def get_project_webhooks(self, project_id): + def get_project_webhooks(self, project_id: int | str) -> list[dict[str, Any]]: """List webhooks for a project See https://docs.gitlab.com/ee/api/projects.html#list-project-hooks """ path = GitLabApiClientPath.project_hooks.format(project=safe_quote(project_id)) - return self.get(path) + return self.get_with_pagination( + path, + lambda page_number, page_size: { + "page": page_number + 1, + "per_page": page_size, + }, + lambda response: response, + timeout=GITLAB_WEBHOOK_REQUEST_TIMEOUT, + raise_on_page_limit=True, + ) - def update_project_webhook(self, project_id, hook_id): + def update_project_webhook( + self, + project_id: int | str, + hook_id: int | str, + reconciliation_id: str | None = None, + ) -> dict[str, Any]: """Update a webhook on a project to include all required events See https://docs.gitlab.com/ee/api/projects.html#edit-project-hook @@ -678,7 +701,7 @@ def update_project_webhook(self, project_id, hook_id): ) return self.put( path, - data=self._project_webhook_data(), + data=self._project_webhook_data(reconciliation_id), timeout=GITLAB_WEBHOOK_REQUEST_TIMEOUT, api_request_type=GitLabApiRequestType.UPDATE_PROJECT_WEBHOOK, ) diff --git a/src/sentry/integrations/gitlab/tasks.py b/src/sentry/integrations/gitlab/tasks.py index 91ece08aa6f1..1f013ba94c5d 100644 --- a/src/sentry/integrations/gitlab/tasks.py +++ b/src/sentry/integrations/gitlab/tasks.py @@ -2,6 +2,7 @@ import errno import logging +from typing import Any from uuid import uuid4 from redis.exceptions import RedisError @@ -10,6 +11,7 @@ from sentry import options from sentry.constants import ObjectStatus +from sentry.integrations.gitlab.client import GitLabApiClient from sentry.integrations.gitlab.metrics import ( GitLabTaskEvent, GitLabTaskInteractionType, @@ -18,14 +20,20 @@ from sentry.integrations.models.integration import Integration from sentry.integrations.services.integration import integration_service from sentry.integrations.services.repository import repository_service +from sentry.locks import locks from sentry.shared_integrations.exceptions import ( ApiError, ApiForbiddenError, + ApiPaginationTruncated, ApiUnauthorized, + IntegrationConfigurationError, ) from sentry.silo.base import SiloMode from sentry.tasks.base import instrumented_task from sentry.taskworker.namespaces import integrations_tasks +from sentry.utils import metrics +from sentry.utils.locking import UnableToAcquireLock +from sentry.utils.locking.lock import Lock from sentry.utils.redis import load_redis_script, redis_clusters logger = logging.getLogger(__name__) @@ -41,10 +49,29 @@ ) -def _discard_project_webhook(client, project_id: int | str, hook_id: int | str) -> None: - # Retrying the task would not come back here: it re-reads the repository and either - # halts or reconciles the hook the repository now holds. So this is the only chance - # to delete the hook, and a failure only leaves it orphaned. +def _project_webhook_lock(repository_id: int) -> Lock: + return locks.get( + f"gitlab:project-webhook:{repository_id}", + duration=150, + name="gitlab_project_webhook", + ) + + +def _matching_project_webhooks( + client: GitLabApiClient, project_id: int | str, reconciliation_id: str +) -> list[dict[str, Any]]: + expected_url = client.project_webhook_url(reconciliation_id) + hooks = [ + hook + for hook in client.get_project_webhooks(project_id) + if hook.get("url") == expected_url and isinstance(hook.get("id"), int | str) + ] + return sorted(hooks, key=lambda hook: int(hook["id"])) + + +def _discard_project_webhook( + client: GitLabApiClient, project_id: int | str, hook_id: int | str +) -> None: try: client.delete_project_webhook(project_id, hook_id) except ApiError as e: @@ -59,8 +86,13 @@ def _discard_project_webhook(client, project_id: int | str, hook_id: int | str) name="sentry.tasks.integrations.gitlab.update_project_webhook", namespace=integrations_tasks, silo_mode=SiloMode.CELL, - processing_deadline_duration=60, - retry=Retry(times=3, delay=60, on=(Exception,), ignore=(Integration.DoesNotExist,)), + processing_deadline_duration=120, + retry=Retry( + times=3, + delay=60, + on=(Exception,), + ignore=(Integration.DoesNotExist, IntegrationConfigurationError), + ), ) def update_project_webhook(integration_id: int, organization_id: int, repository_id: int) -> None: """ @@ -87,11 +119,6 @@ def update_project_webhook(integration_id: int, organization_id: int, repository interaction_type=GitLabTaskInteractionType.UPDATE_PROJECT_WEBHOOK, integration=integration, ).capture() as lifecycle: - repo = repository_service.get_repository( - organization_id=organization_id, - id=repository_id, - ) - lifecycle.add_extras( { "integration_id": integration_id, @@ -100,47 +127,139 @@ def update_project_webhook(integration_id: int, organization_id: int, repository } ) - if not repo or repo.status != ObjectStatus.ACTIVE: - lifecycle.record_halt( - GitLabWebhookUpdateHaltReason.REPOSITORY_NOT_FOUND, - ) - return + outcome: str | None = None + try: + with _project_webhook_lock(repository_id).blocking_acquire( + initial_delay=0.1, timeout=5 + ): + repo = repository_service.get_repository( + organization_id=organization_id, + id=repository_id, + ) + if not repo or repo.status != ObjectStatus.ACTIVE: + lifecycle.record_halt( + GitLabWebhookUpdateHaltReason.REPOSITORY_NOT_FOUND, + ) + return + if repo.integration_id != integration_id: + lifecycle.record_halt(GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED) + return - webhook_id = repo.config.get("webhook_id") - project_id = repo.config.get("project_id") + webhook_id = repo.config.get("webhook_id") + project_id = repo.config.get("project_id") + reconciliation_id = repo.config.get("webhook_reconciliation_id") - lifecycle.add_extras( - { - "repository_id": repo.id, - "webhook_id": webhook_id, - "project_id": project_id, - } - ) + lifecycle.add_extras( + { + "repository_id": repo.id, + "webhook_id": webhook_id, + "project_id": project_id, + } + ) - if not project_id: - lifecycle.record_halt( - GitLabWebhookUpdateHaltReason.MISSING_WEBHOOK_CONFIG, - ) - return + if not project_id: + lifecycle.record_halt( + GitLabWebhookUpdateHaltReason.MISSING_WEBHOOK_CONFIG, + ) + return - installation = integration.get_installation(organization_id=organization_id) - client = installation.get_client() + installation = integration.get_installation(organization_id=organization_id) + client = installation.get_client() + outcome = "failure" - try: - hook_id = client.ensure_project_webhook(project_id, webhook_id) - if hook_id != webhook_id and not repository_service.update_repository_config( - organization_id=organization_id, - id=repo.id, - config_updates={"webhook_id": hook_id}, - expected_integration_id=integration_id, - expected_config={"webhook_id": webhook_id}, - ): - # The repository was disabled, moved, or repaired concurrently while we - # talked to GitLab, so nothing will ever reference or delete the new hook. - lifecycle.add_extra("created_webhook_id", hook_id) - _discard_project_webhook(client, project_id, hook_id) - lifecycle.record_halt(GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED) + if webhook_id: + try: + hook = client.update_project_webhook(project_id, webhook_id) + except ApiError as e: + # An update may have applied despite an error. Only a 404 is safe + # to follow with a create without risking duplicate deliveries. + if e.code != 404: + raise + else: + if hook.get("alert_status") != "disabled": + outcome = "updated" + return + + # PUT does not re-enable permanently disabled hooks. Temporarily + # disabled hooks recover after backoff; older GitLab omits this field. + # Do not use the test endpoint: it replays commits into Sentry. + try: + client.delete_project_webhook(project_id, webhook_id) + except ApiError as e: + if e.code != 404: + raise + + if not isinstance(reconciliation_id, str) or not reconciliation_id: + new_reconciliation_id = uuid4().hex + if not repository_service.update_repository_config( + organization_id=organization_id, + id=repository_id, + config_updates={"webhook_reconciliation_id": new_reconciliation_id}, + expected_integration_id=integration_id, + expected_config={ + "project_id": project_id, + "webhook_id": webhook_id, + "webhook_reconciliation_id": reconciliation_id, + }, + ): + outcome = "repository_changed" + lifecycle.record_halt(GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED) + return + reconciliation_id = new_reconciliation_id + else: + new_reconciliation_id = reconciliation_id + + matching_hooks = _matching_project_webhooks( + client, project_id, new_reconciliation_id + ) + adoptable_hooks = [ + hook for hook in matching_hooks if hook.get("alert_status") != "disabled" + ] + created_hook = not adoptable_hooks + if adoptable_hooks: + hook_id = adoptable_hooks[0]["id"] + client.update_project_webhook( + project_id, + hook_id, + reconciliation_id=new_reconciliation_id, + ) + outcome = "adopted" + else: + hook_id = client.create_project_webhook( + project_id, + reconciliation_id=new_reconciliation_id, + ) + outcome = "recreated" if webhook_id else "created" + + if not isinstance(hook_id, int | str): + raise TypeError("GitLab returned a project webhook without a valid ID") + + if not repository_service.update_repository_config( + organization_id=organization_id, + id=repository_id, + config_updates={"webhook_id": hook_id}, + expected_integration_id=integration_id, + expected_config={ + "project_id": project_id, + "webhook_id": webhook_id, + "webhook_reconciliation_id": new_reconciliation_id, + }, + ): + outcome = "repository_changed" + if created_hook: + _discard_project_webhook(client, project_id, hook_id) + lifecycle.add_extra("created_webhook_id", hook_id) + lifecycle.record_halt(GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED) + return + except UnableToAcquireLock as e: + lifecycle.record_halt(e) + raise + except ApiPaginationTruncated as e: + outcome = "pagination_truncated" + lifecycle.add_extra("listed_webhook_count", len(e.partial_data)) + lifecycle.record_halt(e) except (ApiUnauthorized, ApiForbiddenError) as e: + outcome = "forbidden" if e.code == 403 else "unauthorized" lifecycle.record_halt(e) # Don't retry if we've lost access return @@ -152,7 +271,12 @@ def update_project_webhook(integration_id: int, organization_id: int, repository # Anything not in the retry list should be considered a hard-stop # failure. + if e.code == 404: + outcome = "not_found" lifecycle.record_failure(e) + finally: + if outcome is not None: + metrics.incr("gitlab.project_webhook.reconcile", tags={"outcome": outcome}) @instrumented_task( diff --git a/src/sentry/shared_integrations/client/base.py b/src/sentry/shared_integrations/client/base.py index 9711e79485c2..8baa8199fbb3 100644 --- a/src/sentry/shared_integrations/client/base.py +++ b/src/sentry/shared_integrations/client/base.py @@ -22,6 +22,7 @@ ApiConnectionResetError, ApiError, ApiHostError, + ApiPaginationTruncated, ApiRetryError, ApiTimeoutError, ) @@ -506,13 +507,14 @@ def get_with_pagination( gen_params: Callable[[int, int], Mapping[str, str | int | bool]], get_results: Callable[[Any], Sequence[_TPaginatedResult]], *args: Any, + raise_on_page_limit: bool = False, **kwargs: Any, ) -> list[_TPaginatedResult]: page_size = self.page_size output: list[_TPaginatedResult] = [] for i in range(self.page_number_limit): - resp = self.get(path, params=gen_params(i, page_size)) + resp = self.get(path, *args, params=gen_params(i, page_size), **kwargs) results = get_results(resp) num_results = len(results) @@ -520,4 +522,9 @@ def get_with_pagination( # if the number is lower than our page_size, we can quit if num_results < page_size: return output + if raise_on_page_limit: + raise ApiPaginationTruncated( + output, + f"pagination stopped at page_number_limit={self.page_number_limit}", + ) return output diff --git a/tests/sentry/integrations/gitlab/tasks/test_update_all_project_webhooks.py b/tests/sentry/integrations/gitlab/tasks/test_update_all_project_webhooks.py index 21bccff3bb5c..9b4bd2171292 100644 --- a/tests/sentry/integrations/gitlab/tasks/test_update_all_project_webhooks.py +++ b/tests/sentry/integrations/gitlab/tasks/test_update_all_project_webhooks.py @@ -1,4 +1,4 @@ -from unittest.mock import call, patch +from unittest.mock import MagicMock, call, patch import pytest import responses @@ -17,6 +17,7 @@ from sentry.silo.base import SiloMode from sentry.testutils.asserts import assert_slo_metric from sentry.testutils.silo import assume_test_silo_mode, cell_silo_test +from sentry.utils.locking import UnableToAcquireLock from sentry.utils.redis import redis_clusters @@ -410,6 +411,8 @@ def test_task_only_processes_active_repositories(self, mock_delay, mock_record_e class UpdateProjectWebhookTest(GitLabTestCase): """Tests for the individual webhook update task""" + reconciliation_id = "reconciliation-id" + def setUp(self) -> None: super().setUp() with assume_test_silo_mode(SiloMode.CELL): @@ -424,6 +427,30 @@ def setUp(self) -> None: } self.repo.save() + def _add_hook_list(self, hooks: list[dict[str, object]] | None = None) -> None: + responses.add( + responses.GET, + "https://example.gitlab.com/api/v4/projects/101/hooks", + json=hooks or [], + ) + + def _set_missing_webhook(self) -> None: + self.repo.update( + config={ + "project_id": "101", + "path": "test-group/repo", + "webhook_reconciliation_id": self.reconciliation_id, + } + ) + + def _marked_webhook_url(self) -> str: + installation = self.integration.get_installation(organization_id=self.organization.id) + return installation.get_client().project_webhook_url(self.reconciliation_id) + + def _unmarked_webhook_url(self) -> str: + installation = self.integration.get_installation(organization_id=self.organization.id) + return installation.get_client().project_webhook_url() + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @responses.activate def test_task_updates_webhook(self, mock_record_event): @@ -465,6 +492,22 @@ def test_task_handles_integration_not_found(self, mock_record_event): # No metrics should be recorded when integration is not found (early return) assert mock_record_event.call_count == 0 + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @patch("sentry.integrations.gitlab.tasks._project_webhook_lock") + def test_task_retries_lock_contention_as_halt(self, project_webhook_lock, record_event): + lock = MagicMock() + lock.blocking_acquire.side_effect = UnableToAcquireLock + project_webhook_lock.return_value = lock + + with pytest.raises(UnableToAcquireLock): + update_project_webhook( + integration_id=self.integration.id, + organization_id=self.organization.id, + repository_id=self.repo.id, + ) + + assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @responses.activate def test_task_handles_repository_not_found(self, mock_record_event): @@ -511,6 +554,25 @@ def test_task_handles_inactive_repository(self, mock_record_event): assert last_call[0][0] == EventLifecycleOutcome.HALTED assert last_call[0][1] == GitLabWebhookUpdateHaltReason.REPOSITORY_NOT_FOUND + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @responses.activate + def test_task_halts_when_repository_was_repointed(self, mock_record_event): + other = self.create_integration( + organization=self.organization, external_id="other", provider="gitlab" + ) + Repository.objects.filter(id=self.repo.id).update(integration_id=other.id) + + update_project_webhook( + integration_id=self.integration.id, + organization_id=self.organization.id, + repository_id=self.repo.id, + ) + + assert len(responses.calls) == 0 + last_call = mock_record_event.call_args_list[-1] + assert last_call[0][0] == EventLifecycleOutcome.HALTED + assert last_call[0][1] == GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @responses.activate def test_task_handles_missing_project_id(self, mock_record_event): @@ -595,6 +657,7 @@ def test_task_handles_not_found_errors(self, mock_record_event): json={"error": "Not Found"}, status=404, ) + self._add_hook_list() responses.add( responses.POST, "https://example.gitlab.com/api/v4/projects/101/hooks", @@ -610,14 +673,15 @@ def test_task_handles_not_found_errors(self, mock_record_event): ) # Verify API call was attempted - assert [call.request.method for call in responses.calls] == ["PUT", "POST"] + assert [call.request.method for call in responses.calls] == ["PUT", "GET", "POST"] # Verify SLO failure metric was recorded assert_slo_metric(mock_record_event, event_outcome=EventLifecycleOutcome.FAILURE) @responses.activate def test_task_creates_missing_webhook(self): - self.repo.update(config={"project_id": "101", "path": "test-group/repo"}) + self._set_missing_webhook() + self._add_hook_list() responses.add( responses.POST, "https://example.gitlab.com/api/v4/projects/101/hooks", @@ -628,17 +692,96 @@ def test_task_creates_missing_webhook(self): assert self.repo.config == { "project_id": "101", "path": "test-group/repo", + "webhook_reconciliation_id": self.reconciliation_id, "webhook_id": 100, } - assert [call.request.method for call in responses.calls] == ["POST"] + assert [call.request.method for call in responses.calls] == ["GET", "POST"] + + @responses.activate + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_task_does_not_create_when_hook_listing_is_truncated(self, record_event): + self._set_missing_webhook() + self._add_hook_list( + [ + {"id": 1, "url": self._unmarked_webhook_url()}, + {"id": 2, "url": self._unmarked_webhook_url()}, + ] + ) + self._add_hook_list( + [ + {"id": 3, "url": self._unmarked_webhook_url()}, + {"id": 4, "url": self._unmarked_webhook_url()}, + ] + ) + + with ( + patch("sentry.integrations.gitlab.client.GitLabApiClient.page_size", 2), + patch("sentry.integrations.gitlab.client.GitLabApiClient.page_number_limit", 2), + ): + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert "webhook_id" not in self.repo.config + assert [call.request.method for call in responses.calls] == ["GET", "GET"] + assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) + + @responses.activate + def test_task_persists_creation_identity_before_post(self): + self.repo.update(config={"project_id": "101", "path": "test-group/repo"}) + self._add_hook_list() + + def create(request): + self.repo.refresh_from_db() + reconciliation_id = self.repo.config["webhook_reconciliation_id"] + assert reconciliation_id.encode() in request.body + return 201, {}, '{"id": 100}' + + responses.add_callback( + responses.POST, + "https://example.gitlab.com/api/v4/projects/101/hooks", + callback=create, + ) + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert self.repo.config["webhook_id"] == 100 + + @responses.activate + @patch("sentry.integrations.gitlab.tasks.metrics.incr") + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @patch( + "sentry.integrations.gitlab.tasks.repository_service.update_repository_config", + return_value=False, + ) + def test_task_records_repository_changed_when_reconciliation_id_write_loses_race( + self, update_repository_config, record_event, incr + ): + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/101/hooks/webhook-1", + status=404, + ) + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + update_repository_config.assert_called_once() + assert [request.request.method for request in responses.calls] == ["PUT"] + assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) + incr.assert_any_call( + "gitlab.project_webhook.reconcile", tags={"outcome": "repository_changed"} + ) @responses.activate def test_task_recreates_stale_webhook(self): + self.repo.config["webhook_reconciliation_id"] = self.reconciliation_id + self.repo.save() responses.add( responses.PUT, "https://example.gitlab.com/api/v4/projects/101/hooks/webhook-1", status=404, ) + self._add_hook_list() responses.add( responses.POST, "https://example.gitlab.com/api/v4/projects/101/hooks", @@ -647,10 +790,12 @@ def test_task_recreates_stale_webhook(self): update_project_webhook(self.integration.id, self.organization.id, self.repo.id) self.repo.refresh_from_db() assert self.repo.config["webhook_id"] == 100 - assert [call.request.method for call in responses.calls] == ["PUT", "POST"] + assert [call.request.method for call in responses.calls] == ["PUT", "GET", "POST"] @responses.activate def test_task_replaces_disabled_webhook(self): + self.repo.config["webhook_reconciliation_id"] = self.reconciliation_id + self.repo.save() responses.add( responses.PUT, "https://example.gitlab.com/api/v4/projects/101/hooks/webhook-1", @@ -661,6 +806,7 @@ def test_task_replaces_disabled_webhook(self): "https://example.gitlab.com/api/v4/projects/101/hooks/webhook-1", status=204, ) + self._add_hook_list() responses.add( responses.POST, "https://example.gitlab.com/api/v4/projects/101/hooks", @@ -669,32 +815,41 @@ def test_task_replaces_disabled_webhook(self): update_project_webhook(self.integration.id, self.organization.id, self.repo.id) self.repo.refresh_from_db() assert self.repo.config["webhook_id"] == 100 - assert [call.request.method for call in responses.calls] == ["PUT", "DELETE", "POST"] + assert [call.request.method for call in responses.calls] == [ + "PUT", + "DELETE", + "GET", + "POST", + ] - def _create_hook_while(self, change_repository, delete_status=204): + def _create_hook_while(self, change_repository): """Register a hook create that changes the repository mid-request, as a concurrent writer would.""" def create(request): change_repository() return 201, {}, '{"id": 100}' - self.repo.update(config={"project_id": "101", "path": "test-group/repo"}) + self._set_missing_webhook() + self.repo.refresh_from_db() + self._add_hook_list() responses.add_callback( responses.POST, "https://example.gitlab.com/api/v4/projects/101/hooks", callback=create ) responses.add( responses.DELETE, "https://example.gitlab.com/api/v4/projects/101/hooks/100", - status=delete_status, + status=204, ) @responses.activate def test_task_preserves_config_written_during_repair(self): - self._create_hook_while( - lambda: Repository.objects.filter(id=self.repo.id).update( - name="renamed", config={**self.repo.config, "sync_comments": True} + def change_repository(): + current = Repository.objects.get(id=self.repo.id) + Repository.objects.filter(id=self.repo.id).update( + name="renamed", config={**current.config, "sync_comments": True} ) - ) + + self._create_hook_while(change_repository) update_project_webhook(self.integration.id, self.organization.id, self.repo.id) self.repo.refresh_from_db() assert self.repo.name == "renamed" @@ -702,13 +857,15 @@ def test_task_preserves_config_written_during_repair(self): "project_id": "101", "path": "test-group/repo", "sync_comments": True, + "webhook_reconciliation_id": self.reconciliation_id, "webhook_id": 100, } - assert [call.request.method for call in responses.calls] == ["POST"] + assert [call.request.method for call in responses.calls] == ["GET", "POST"] @responses.activate + @patch("sentry.integrations.gitlab.tasks.metrics.incr") @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") - def test_task_discards_hook_when_repository_disabled_during_repair(self, record_event): + def test_task_discards_hook_when_repository_disabled_during_repair(self, record_event, incr): self._create_hook_while( lambda: Repository.objects.filter(id=self.repo.id).update(status=ObjectStatus.DISABLED) ) @@ -716,25 +873,11 @@ def test_task_discards_hook_when_repository_disabled_during_repair(self, record_ self.repo.refresh_from_db() assert self.repo.status == ObjectStatus.DISABLED assert "webhook_id" not in self.repo.config - assert [call.request.method for call in responses.calls] == ["POST", "DELETE"] + assert [call.request.method for call in responses.calls] == ["GET", "POST", "DELETE"] assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) - - @responses.activate - @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") - def test_task_halts_when_discarding_the_hook_fails(self, record_event): - for delete_status in (404, 500): - responses.reset() - record_event.reset_mock() - self._create_hook_while( - lambda: Repository.objects.filter(id=self.repo.id).update( - status=ObjectStatus.DISABLED - ), - delete_status=delete_status, - ) - update_project_webhook(self.integration.id, self.organization.id, self.repo.id) - assert [call.request.method for call in responses.calls] == ["POST", "DELETE"] - assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) - Repository.objects.filter(id=self.repo.id).update(status=ObjectStatus.ACTIVE) + incr.assert_any_call( + "gitlab.project_webhook.reconcile", tags={"outcome": "repository_changed"} + ) @responses.activate def test_task_discards_hook_when_repository_moved_during_repair(self): @@ -748,7 +891,7 @@ def test_task_discards_hook_when_repository_moved_during_repair(self): self.repo.refresh_from_db() assert self.repo.integration_id == other.id assert "webhook_id" not in self.repo.config - assert [call.request.method for call in responses.calls] == ["POST", "DELETE"] + assert [call.request.method for call in responses.calls] == ["GET", "POST", "DELETE"] @responses.activate def test_task_discards_hook_when_repaired_concurrently(self): @@ -760,25 +903,108 @@ def test_task_discards_hook_when_repaired_concurrently(self): update_project_webhook(self.integration.id, self.organization.id, self.repo.id) self.repo.refresh_from_db() assert self.repo.config["webhook_id"] == 99 - assert [call.request.method for call in responses.calls] == ["POST", "DELETE"] + assert [call.request.method for call in responses.calls] == ["GET", "POST", "DELETE"] @responses.activate - @patch("sentry.integrations.gitlab.client.metrics.incr") + @patch("sentry.integrations.gitlab.tasks.metrics.incr") @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") def test_create_forbidden_is_terminal(self, record_event, incr): - self.repo.update(config={"project_id": "101"}) + self._set_missing_webhook() + self._add_hook_list() responses.add( responses.POST, "https://example.gitlab.com/api/v4/projects/101/hooks", status=403, ) update_project_webhook(self.integration.id, self.organization.id, self.repo.id) - assert len(responses.calls) == 1 + assert [call.request.method for call in responses.calls] == ["GET", "POST"] self.repo.refresh_from_db() assert "webhook_id" not in self.repo.config assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) incr.assert_any_call("gitlab.project_webhook.reconcile", tags={"outcome": "forbidden"}) + @responses.activate + def test_task_adopts_marked_hook(self): + self._set_missing_webhook() + self._add_hook_list( + [ + {"id": 100, "url": self._marked_webhook_url()}, + {"id": 101, "url": self._marked_webhook_url()}, + {"id": 102, "url": self._unmarked_webhook_url()}, + ] + ) + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/101/hooks/100", + json={"id": 100}, + ) + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert self.repo.config == { + "project_id": "101", + "path": "test-group/repo", + "webhook_reconciliation_id": self.reconciliation_id, + "webhook_id": 100, + } + assert [call.request.method for call in responses.calls] == ["GET", "PUT"] + + @responses.activate + def test_task_does_not_adopt_disabled_marked_hook(self): + self._set_missing_webhook() + self._add_hook_list( + [ + { + "id": 100, + "url": self._marked_webhook_url(), + "alert_status": "disabled", + } + ] + ) + responses.add( + responses.POST, + "https://example.gitlab.com/api/v4/projects/101/hooks", + json={"id": 101}, + ) + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert self.repo.config["webhook_id"] == 101 + assert [call.request.method for call in responses.calls] == ["GET", "POST"] + + @responses.activate + def test_task_adopts_hook_after_ambiguous_create_failure(self): + self._set_missing_webhook() + self._add_hook_list() + responses.add( + responses.POST, + "https://example.gitlab.com/api/v4/projects/101/hooks", + status=500, + ) + + with pytest.raises(ApiError): + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert self.repo.config["webhook_reconciliation_id"] == self.reconciliation_id + assert "webhook_id" not in self.repo.config + + responses.reset() + self._add_hook_list([{"id": 100, "url": self._marked_webhook_url()}]) + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/101/hooks/100", + json={"id": 100}, + ) + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert self.repo.config["webhook_id"] == 100 + assert [call.request.method for call in responses.calls] == ["GET", "PUT"] + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @responses.activate def test_task_updates_webhook_with_correct_data(self, mock_record_event): diff --git a/tests/sentry/integrations/gitlab/test_client.py b/tests/sentry/integrations/gitlab/test_client.py index 632a424f4a2b..55f71521d6b7 100644 --- a/tests/sentry/integrations/gitlab/test_client.py +++ b/tests/sentry/integrations/gitlab/test_client.py @@ -30,6 +30,7 @@ from sentry.shared_integrations.exceptions import ( ApiError, ApiHostError, + ApiPaginationTruncated, ApiRateLimitedError, ApiRetryError, ) @@ -1055,6 +1056,49 @@ def test_multiple_merged_requests(self) -> None: @control_silo_test class GitLabProjectWebhookRequestTypeTest(GitLabClientTest): + @responses.activate + def test_project_webhook_listing_paginates(self) -> None: + hooks_url = f"https://example.gitlab.com/api/v4/projects/{self.gitlab_id}/hooks" + responses.add( + responses.GET, + hooks_url, + status=200, + json=[{"id": 1}, {"id": 2}], + ) + responses.add( + responses.GET, + hooks_url, + status=200, + json=[{"id": 3}], + ) + self.gitlab_client.page_size = 2 + + with mock.patch.object(self.gitlab_client, "get", wraps=self.gitlab_client.get) as get: + hooks = self.gitlab_client.get_project_webhooks(self.gitlab_id) + + assert hooks == [{"id": 1}, {"id": 2}, {"id": 3}] + assert [request.kwargs["timeout"] for request in get.call_args_list] == [ + GITLAB_WEBHOOK_REQUEST_TIMEOUT, + GITLAB_WEBHOOK_REQUEST_TIMEOUT, + ] + assert [response.request.url for response in responses.calls] == [ + f"{hooks_url}?page=1&per_page=2", + f"{hooks_url}?page=2&per_page=2", + ] + + @responses.activate + def test_project_webhook_listing_fails_closed_at_page_limit(self) -> None: + hooks_url = f"https://example.gitlab.com/api/v4/projects/{self.gitlab_id}/hooks" + responses.add(responses.GET, hooks_url, status=200, json=[{"id": 1}, {"id": 2}]) + responses.add(responses.GET, hooks_url, status=200, json=[{"id": 3}, {"id": 4}]) + self.gitlab_client.page_size = 2 + self.gitlab_client.page_number_limit = 2 + + with pytest.raises(ApiPaginationTruncated) as excinfo: + self.gitlab_client.get_project_webhooks(self.gitlab_id) + + assert excinfo.value.partial_data == [{"id": 1}, {"id": 2}, {"id": 3}, {"id": 4}] + @responses.activate @mock.patch("sentry.shared_integrations.client.base.metrics.incr") def test_hook_calls_are_tagged_by_request_type(self, mock_incr: mock.MagicMock) -> None: