diff --git a/src/sentry/integrations/gitlab/client.py b/src/sentry/integrations/gitlab/client.py index 093c3bffb839..3b794f33654a 100644 --- a/src/sentry/integrations/gitlab/client.py +++ b/src/sentry/integrations/gitlab/client.py @@ -686,6 +686,13 @@ def get_project_webhooks(self, project_id: int | str) -> list[dict[str, Any]]: raise_on_page_limit=True, ) + def get_project_webhook(self, project_id: int | str, hook_id: int | str) -> dict[str, Any]: + """Get a single project webhook.""" + path = GitLabApiClientPath.project_hook.format( + project=safe_quote(project_id), hook_id=hook_id + ) + return self.get(path, timeout=GITLAB_WEBHOOK_REQUEST_TIMEOUT) + def update_project_webhook( self, project_id: int | str, diff --git a/src/sentry/integrations/gitlab/tasks.py b/src/sentry/integrations/gitlab/tasks.py index 1f013ba94c5d..cbbb4bd7c7a4 100644 --- a/src/sentry/integrations/gitlab/tasks.py +++ b/src/sentry/integrations/gitlab/tasks.py @@ -69,15 +69,103 @@ def _matching_project_webhooks( return sorted(hooks, key=lambda hook: int(hook["id"])) -def _discard_project_webhook( - client: GitLabApiClient, project_id: int | str, hook_id: int | str +def _enqueue_project_webhook_cleanup( + *, + integration_id: int, + organization_id: int, + repository_id: int, + project_id: int | str, + reconciliation_id: str, + hook_ids: set[int | str], ) -> None: + for hook_id in hook_ids: + delete_redundant_project_webhook.delay( + integration_id, + organization_id, + repository_id, + project_id, + hook_id, + reconciliation_id, + ) + + +@instrumented_task( + name="sentry.tasks.integrations.gitlab.delete_redundant_project_webhook", + namespace=integrations_tasks, + silo_mode=SiloMode.CELL, + processing_deadline_duration=60, + retry=Retry(times=3, delay=60, on=(Exception,), ignore=(Integration.DoesNotExist,)), +) +def delete_redundant_project_webhook( + integration_id: int, + organization_id: int, + repository_id: int, + project_id: int | str, + hook_id: int | str, + reconciliation_id: str, +) -> None: + """Delete an unreferenced hook whose reconciliation identity proves we created it.""" + integration = integration_service.get_integration( + integration_id=integration_id, + status=ObjectStatus.ACTIVE, + using_replica=options.get("integration_service.get_integration.using_replica"), + ) + if not integration: + return + try: - client.delete_project_webhook(project_id, hook_id) + 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 ( + repo + and repo.config.get("project_id") == project_id + and repo.config.get("webhook_id") == hook_id + ): + return + + installation = integration.get_installation(organization_id=organization_id) + client = installation.get_client() + try: + hook = client.get_project_webhook(project_id, hook_id) + except ApiError as e: + if e.code == 404: + return + raise + + if hook.get("url") != client.project_webhook_url(reconciliation_id): + return + + repo = repository_service.get_repository( + organization_id=organization_id, + id=repository_id, + ) + if ( + repo + and repo.config.get("project_id") == project_id + and repo.config.get("webhook_id") == hook_id + ): + return + + client.delete_project_webhook(project_id, hook_id) + except IntegrationConfigurationError: + logger.info( + "delete-redundant-project-webhook.identity-unavailable", + extra={"project_id": project_id, "webhook_id": hook_id}, + ) + except (ApiUnauthorized, ApiForbiddenError) as e: + logger.warning( + "delete-redundant-project-webhook.authorization-failed", + extra={"project_id": project_id, "webhook_id": hook_id, "status_code": e.code}, + ) except ApiError as e: + if e.code in GITLAB_RETRY_CODES: + raise if e.code != 404: logger.warning( - "update-project-webhook.discard-failed", + "delete-redundant-project-webhook.failed", extra={"project_id": project_id, "webhook_id": hook_id, "status_code": e.code}, ) @@ -148,6 +236,7 @@ def update_project_webhook(integration_id: int, organization_id: int, repository webhook_id = repo.config.get("webhook_id") project_id = repo.config.get("project_id") reconciliation_id = repo.config.get("webhook_reconciliation_id") + reconciliation_pending = repo.config.get("webhook_reconciliation_pending") lifecycle.add_extras( { @@ -177,6 +266,42 @@ def update_project_webhook(integration_id: int, organization_id: int, repository raise else: if hook.get("alert_status") != "disabled": + if reconciliation_pending is True: + cleanup_ids: set[int | str] = set() + if isinstance(reconciliation_id, str): + cleanup_ids = { + candidate["id"] + for candidate in _matching_project_webhooks( + client, project_id, reconciliation_id + ) + if candidate["id"] != webhook_id + } + _enqueue_project_webhook_cleanup( + integration_id=integration_id, + organization_id=organization_id, + repository_id=repository_id, + project_id=project_id, + reconciliation_id=reconciliation_id, + hook_ids=cleanup_ids, + ) + if not repository_service.update_repository_config( + organization_id=organization_id, + id=repository_id, + config_updates={}, + config_deletions=["webhook_reconciliation_pending"], + expected_integration_id=integration_id, + expected_config={ + "project_id": project_id, + "webhook_id": webhook_id, + "webhook_reconciliation_id": reconciliation_id, + "webhook_reconciliation_pending": True, + }, + ): + outcome = "repository_changed" + lifecycle.record_halt( + GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED + ) + return outcome = "updated" return @@ -191,23 +316,31 @@ def update_project_webhook(integration_id: int, organization_id: int, repository if not isinstance(reconciliation_id, str) or not reconciliation_id: new_reconciliation_id = uuid4().hex + else: + new_reconciliation_id = reconciliation_id + + if reconciliation_pending is not True or ( + reconciliation_id != new_reconciliation_id + ): if not repository_service.update_repository_config( organization_id=organization_id, id=repository_id, - config_updates={"webhook_reconciliation_id": new_reconciliation_id}, + config_updates={ + "webhook_reconciliation_id": new_reconciliation_id, + "webhook_reconciliation_pending": True, + }, expected_integration_id=integration_id, expected_config={ "project_id": project_id, "webhook_id": webhook_id, "webhook_reconciliation_id": reconciliation_id, + "webhook_reconciliation_pending": reconciliation_pending, }, ): 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 @@ -215,7 +348,6 @@ def update_project_webhook(integration_id: int, organization_id: int, repository 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( @@ -243,14 +375,56 @@ def update_project_webhook(integration_id: int, organization_id: int, repository "project_id": project_id, "webhook_id": webhook_id, "webhook_reconciliation_id": new_reconciliation_id, + "webhook_reconciliation_pending": True, }, ): outcome = "repository_changed" - if created_hook: - _discard_project_webhook(client, project_id, hook_id) + cleanup_ids = {hook["id"] for hook in matching_hooks} + cleanup_ids.add(hook_id) + _enqueue_project_webhook_cleanup( + integration_id=integration_id, + organization_id=organization_id, + repository_id=repository_id, + project_id=project_id, + reconciliation_id=new_reconciliation_id, + hook_ids=cleanup_ids, + ) lifecycle.add_extra("created_webhook_id", hook_id) lifecycle.record_halt(GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED) return + cleanup_ids = {hook["id"] for hook in matching_hooks if hook["id"] != hook_id} + _enqueue_project_webhook_cleanup( + integration_id=integration_id, + organization_id=organization_id, + repository_id=repository_id, + project_id=project_id, + reconciliation_id=new_reconciliation_id, + hook_ids=cleanup_ids, + ) + + if not repository_service.update_repository_config( + organization_id=organization_id, + id=repository_id, + config_updates={}, + config_deletions=["webhook_reconciliation_pending"], + expected_integration_id=integration_id, + expected_config={ + "project_id": project_id, + "webhook_id": hook_id, + "webhook_reconciliation_id": new_reconciliation_id, + "webhook_reconciliation_pending": True, + }, + ): + _enqueue_project_webhook_cleanup( + integration_id=integration_id, + organization_id=organization_id, + repository_id=repository_id, + project_id=project_id, + reconciliation_id=new_reconciliation_id, + hook_ids={hook_id}, + ) + outcome = "repository_changed" + lifecycle.record_halt(GitLabWebhookUpdateHaltReason.REPOSITORY_CHANGED) except UnableToAcquireLock as e: lifecycle.record_halt(e) raise 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 9b4bd2171292..7f67fe26ec1e 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 @@ -9,11 +9,15 @@ from fixtures.gitlab import GitLabTestCase from sentry.constants import ObjectStatus from sentry.integrations.gitlab.metrics import GitLabWebhookUpdateHaltReason -from sentry.integrations.gitlab.tasks import update_all_project_webhooks, update_project_webhook +from sentry.integrations.gitlab.tasks import ( + delete_redundant_project_webhook, + update_all_project_webhooks, + update_project_webhook, +) from sentry.integrations.services.integration import integration_service from sentry.integrations.types import EventLifecycleOutcome from sentry.models.repository import Repository -from sentry.shared_integrations.exceptions import ApiError +from sentry.shared_integrations.exceptions import ApiError, IntegrationConfigurationError 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 @@ -733,6 +737,7 @@ def test_task_persists_creation_identity_before_post(self): def create(request): self.repo.refresh_from_db() reconciliation_id = self.repo.config["webhook_reconciliation_id"] + assert self.repo.config["webhook_reconciliation_pending"] is True assert reconciliation_id.encode() in request.body return 201, {}, '{"id": 100}' @@ -746,6 +751,7 @@ def create(request): self.repo.refresh_from_db() assert self.repo.config["webhook_id"] == 100 + assert "webhook_reconciliation_pending" not in self.repo.config @responses.activate @patch("sentry.integrations.gitlab.tasks.metrics.incr") @@ -835,11 +841,6 @@ def create(request): 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=204, - ) @responses.activate def test_task_preserves_config_written_during_repair(self): @@ -865,7 +866,10 @@ def change_repository(): @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, incr): + @patch("sentry.integrations.gitlab.tasks.delete_redundant_project_webhook.delay") + def test_task_queues_cleanup_when_repository_disabled_during_repair( + self, cleanup_delay, record_event, incr + ): self._create_hook_while( lambda: Repository.objects.filter(id=self.repo.id).update(status=ObjectStatus.DISABLED) ) @@ -873,14 +877,23 @@ 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] == ["GET", "POST", "DELETE"] + cleanup_delay.assert_called_once_with( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + assert [call.request.method for call in responses.calls] == ["GET", "POST"] 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_discards_hook_when_repository_moved_during_repair(self): + @patch("sentry.integrations.gitlab.tasks.delete_redundant_project_webhook.delay") + def test_task_queues_cleanup_when_repository_moved_during_repair(self, cleanup_delay): other = self.create_integration( organization=self.organization, external_id="other", provider="gitlab" ) @@ -891,10 +904,12 @@ 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] == ["GET", "POST", "DELETE"] + cleanup_delay.assert_called_once() + assert [call.request.method for call in responses.calls] == ["GET", "POST"] @responses.activate - def test_task_discards_hook_when_repaired_concurrently(self): + @patch("sentry.integrations.gitlab.tasks.delete_redundant_project_webhook.delay") + def test_task_queues_cleanup_when_repaired_concurrently(self, cleanup_delay): self._create_hook_while( lambda: Repository.objects.filter(id=self.repo.id).update( config={**self.repo.config, "webhook_id": 99} @@ -903,7 +918,8 @@ 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] == ["GET", "POST", "DELETE"] + cleanup_delay.assert_called_once() + assert [call.request.method for call in responses.calls] == ["GET", "POST"] @responses.activate @patch("sentry.integrations.gitlab.tasks.metrics.incr") @@ -924,7 +940,8 @@ def test_create_forbidden_is_terminal(self, record_event, incr): incr.assert_any_call("gitlab.project_webhook.reconcile", tags={"outcome": "forbidden"}) @responses.activate - def test_task_adopts_marked_hook(self): + @patch("sentry.integrations.gitlab.tasks.delete_redundant_project_webhook.delay") + def test_task_adopts_marked_hook_and_queues_duplicate_cleanup(self, cleanup_delay): self._set_missing_webhook() self._add_hook_list( [ @@ -948,6 +965,14 @@ def test_task_adopts_marked_hook(self): "webhook_reconciliation_id": self.reconciliation_id, "webhook_id": 100, } + cleanup_delay.assert_called_once_with( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 101, + self.reconciliation_id, + ) assert [call.request.method for call in responses.calls] == ["GET", "PUT"] @responses.activate @@ -988,7 +1013,7 @@ def test_task_adopts_hook_after_ambiguous_create_failure(self): 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 self.repo.config["webhook_reconciliation_pending"] is True assert "webhook_id" not in self.repo.config responses.reset() @@ -1003,8 +1028,204 @@ def test_task_adopts_hook_after_ambiguous_create_failure(self): self.repo.refresh_from_db() assert self.repo.config["webhook_id"] == 100 + assert "webhook_reconciliation_pending" not in self.repo.config assert [call.request.method for call in responses.calls] == ["GET", "PUT"] + @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_pending_clear_loses_race( + self, update_repository_config, record_event, incr + ): + self.repo.update( + config={ + **self.repo.config, + "webhook_reconciliation_id": self.reconciliation_id, + "webhook_reconciliation_pending": True, + } + ) + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/101/hooks/webhook-1", + json={"id": "webhook-1"}, + ) + self._add_hook_list() + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + update_repository_config.assert_called_once() + assert_slo_metric(record_event, event_outcome=EventLifecycleOutcome.HALTED) + incr.assert_any_call( + "gitlab.project_webhook.reconcile", tags={"outcome": "repository_changed"} + ) + + @responses.activate + @patch("sentry.integrations.gitlab.tasks.delete_redundant_project_webhook.delay") + def test_task_resumes_duplicate_cleanup_while_pending(self, cleanup_delay): + self.repo.update( + config={ + **self.repo.config, + "webhook_reconciliation_id": self.reconciliation_id, + "webhook_reconciliation_pending": True, + } + ) + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/101/hooks/webhook-1", + json={"id": "webhook-1"}, + ) + self._add_hook_list( + [ + {"id": 100, "url": self._marked_webhook_url()}, + {"id": 101, "url": self._unmarked_webhook_url()}, + ] + ) + + update_project_webhook(self.integration.id, self.organization.id, self.repo.id) + + self.repo.refresh_from_db() + assert "webhook_reconciliation_pending" not in self.repo.config + cleanup_delay.assert_called_once_with( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + + @responses.activate + def test_cleanup_deletes_only_an_exact_unreferenced_hook(self): + self._set_missing_webhook() + responses.add( + responses.GET, + "https://example.gitlab.com/api/v4/projects/101/hooks/100", + json={"id": 100, "url": self._marked_webhook_url()}, + ) + responses.add( + responses.DELETE, + "https://example.gitlab.com/api/v4/projects/101/hooks/100", + status=204, + ) + + delete_redundant_project_webhook( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + + assert [call.request.method for call in responses.calls] == ["GET", "DELETE"] + + @responses.activate + def test_cleanup_preserves_a_hook_without_the_exact_marker(self): + self._set_missing_webhook() + responses.add( + responses.GET, + "https://example.gitlab.com/api/v4/projects/101/hooks/100", + json={"id": 100, "url": self._unmarked_webhook_url()}, + ) + + delete_redundant_project_webhook( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + + assert [call.request.method for call in responses.calls] == ["GET"] + + @responses.activate + def test_cleanup_preserves_stored_hook_after_integration_repoint(self): + other = self.create_integration( + organization=self.organization, external_id="other", provider="gitlab" + ) + Repository.objects.filter(id=self.repo.id).update( + integration_id=other.id, + config={ + "project_id": "101", + "webhook_id": 100, + "webhook_reconciliation_id": self.reconciliation_id, + }, + ) + + delete_redundant_project_webhook( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + + assert len(responses.calls) == 0 + + @responses.activate + def test_cleanup_preserves_hook_repointed_during_remote_fetch(self): + self._set_missing_webhook() + other = self.create_integration( + organization=self.organization, external_id="other", provider="gitlab" + ) + + def get_hook(request): + Repository.objects.filter(id=self.repo.id).update( + integration_id=other.id, + config={ + "project_id": "101", + "webhook_id": 100, + "webhook_reconciliation_id": self.reconciliation_id, + }, + ) + return 200, {}, f'{{"id": 100, "url": "{self._marked_webhook_url()}"}}' + + responses.add_callback( + responses.GET, + "https://example.gitlab.com/api/v4/projects/101/hooks/100", + callback=get_hook, + ) + + delete_redundant_project_webhook( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + + assert [call.request.method for call in responses.calls] == ["GET"] + + @patch("sentry.integrations.gitlab.tasks.logger.info") + @patch( + "sentry.integrations.gitlab.integration.GitlabIntegration.get_client", + side_effect=IntegrationConfigurationError("Identity not found."), + ) + def test_cleanup_halts_when_identity_is_unavailable(self, get_client, logger_info): + self._set_missing_webhook() + + delete_redundant_project_webhook( + self.integration.id, + self.organization.id, + self.repo.id, + "101", + 100, + self.reconciliation_id, + ) + + get_client.assert_called_once() + logger_info.assert_called_once_with( + "delete-redundant-project-webhook.identity-unavailable", + extra={"project_id": "101", "webhook_id": 100}, + ) + @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 803aeaf8badb..a204385c24e4 100644 --- a/tests/sentry/integrations/gitlab/test_client.py +++ b/tests/sentry/integrations/gitlab/test_client.py @@ -1099,6 +1099,16 @@ def test_project_webhook_listing_fails_closed_at_page_limit(self) -> None: assert excinfo.value.partial_data == [{"id": 1}, {"id": 2}, {"id": 3}, {"id": 4}] + @responses.activate + def test_get_project_webhook_has_request_timeout(self) -> None: + hooks_url = f"https://example.gitlab.com/api/v4/projects/{self.gitlab_id}/hooks" + responses.add(responses.GET, f"{hooks_url}/7", status=200, json={"id": 7}) + + with mock.patch.object(self.gitlab_client, "get", wraps=self.gitlab_client.get) as get: + assert self.gitlab_client.get_project_webhook(self.gitlab_id, 7) == {"id": 7} + + assert get.call_args.kwargs["timeout"] == GITLAB_WEBHOOK_REQUEST_TIMEOUT + @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: