Skip to content

fix(gitlab): Make project webhook creation retry-safe #129175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
giovanni-guidini wants to merge 1 commit into from
Open

fix(gitlab): Make project webhook creation retry-safe #129175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions src/sentry/integrations/gitlab/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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,
Expand All @@ -645,30 +652,46 @@ 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
"""
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
Expand All @@ -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,
)
Expand Down
216 changes: 170 additions & 46 deletions src/sentry/integrations/gitlab/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import errno
import logging
from typing import Any
from uuid import uuid4

from redis.exceptions import RedisError
Expand All @@ -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,
Expand All @@ -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__)
Expand All @@ -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:
Expand All @@ -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:
"""
Expand All @@ -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,
Expand All @@ -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
):
Comment thread
giovanni-guidini marked this conversation as resolved.
Resolved
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
Comment thread
giovanni-guidini marked this conversation as resolved.
Resolved
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"
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved
Comment thread
giovanni-guidini marked this conversation as resolved.
Resolved
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
Expand All @@ -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(
Expand Down
Loading
Loading