diff --git a/README.md b/README.md index 7ab2720..b366f9e 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,8 @@ Steps to follow: dsn = https://foo@bar.ingest.sentry.io/foobar ``` + - **Note:** The app caches this file, so changes to it can take up to 10 minutes to apply. + 4. Install [this GitHub App](https://github.com/apps/sentry-github-app-alpha) 1. **"All repositories"** - this is the easiest option; all repositories will have telemetry from GitHub Actions sent. 2. **"Only select repositories"** diff --git a/cli.py b/cli.py index 3505509..63c32d0 100644 --- a/cli.py +++ b/cli.py @@ -49,7 +49,7 @@ def main() -> int: with GithubAppToken(**web_app.config.gh_app._asdict()).get_token( installation_id ) as token: - dsn = fetch_dsn_for_github_org(org, token) + dsn = fetch_dsn_for_github_org(org, token, installation_id) client = GithubClient(token=token, dsn=dsn) client.send_trace(job) diff --git a/requirements.txt b/requirements.txt index 008063d..c95af15 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ +cachetools==5.3.2 flask==2.3.3 google-cloud-secret-manager==2.11.0 PyJWT[crypto]==2.4.0 diff --git a/src/github_sdk.py b/src/github_sdk.py index cf28d01..4ece368 100644 --- a/src/github_sdk.py +++ b/src/github_sdk.py @@ -49,7 +49,6 @@ def _fetch_github(self, url): def _get_extra_metadata(self, job): # XXX: This is the slowest call runs = self._fetch_github(job["run_url"]).json() - workflow = self._fetch_github(runs["workflow_url"]).json() repo = runs["repository"]["full_name"] meta = { # "workflow_name": workflow["name"], @@ -68,7 +67,10 @@ def _get_extra_metadata(self, job): "run_attempt": runs["run_attempt"], # Rerunning a job "event": runs["event"], # It allows querying jobs within the same workflow (e.g. foo.yml) - "workflow": workflow["path"].rsplit("/")[-1], + # The run has the path, so there's no need to fetch its workflow, which + # fails for required workflows (`workflow_url` 422s). Their path can end + # in a ref (e.g. `foo.yml@refs/heads/main`), which isn't part of the name + "workflow": runs["path"].split("@", 1)[0].rsplit("/")[-1], }, } if runs.get("pull_requests"): diff --git a/src/sentry_config.py b/src/sentry_config.py index 30678da..820a2b7 100644 --- a/src/sentry_config.py +++ b/src/sentry_config.py @@ -3,10 +3,11 @@ import base64 import logging import os +import threading from configparser import ConfigParser -from functools import lru_cache import requests +from cachetools import TTLCache LOGGING_LEVEL = os.environ.get("LOGGING_LEVEL", logging.INFO) logger = logging.getLogger(__name__) @@ -15,9 +16,27 @@ SENTRY_CONFIG_API_URL = ( "https://api.github.com/repos/{owner}/.sentry/contents/sentry_config.ini" ) +# Fetching the config on every webhook exhausts busy orgs' GitHub API rate limit +DSN_CACHE_TTL_SECONDS = 10 * 60 +# Module globals rather than @cached, whose closure would put every cached DSN in the +# local variables Sentry captures with an error +_dsn_cache = TTLCache(maxsize=1024, ttl=DSN_CACHE_TTL_SECONDS) +_dsn_cache_lock = threading.Lock() -def fetch_dsn_for_github_org(org: str, token: str) -> str: +def fetch_dsn_for_github_org(org: str, token: str, installation_id: int) -> str: + # Only serve a DSN to the installation whose token could read it + key = (installation_id, org) + with _dsn_cache_lock: + dsn = _dsn_cache.get(key) + if dsn is None: + dsn = _fetch_dsn(org, token) + with _dsn_cache_lock: + _dsn_cache[key] = dsn + return dsn + + +def _fetch_dsn(org: str, token: str) -> str: # Using the GH app token allows fetching the file in a private repo headers = { "Accept": "application/vnd.github+json", diff --git a/src/web_app_handler.py b/src/web_app_handler.py index 5b96c07..d1a3d0d 100644 --- a/src/web_app_handler.py +++ b/src/web_app_handler.py @@ -29,6 +29,9 @@ def handle_event(self, data, headers): reason = "Event not supported." elif data["action"] != "completed": reason = "We cannot do anything with this workflow state." + elif data["workflow_job"]["conclusion"] == "skipped": + # These are not traced, so don't spend GitHub API calls on them + reason = "Skipped jobs are not traced." else: # For now, this simplifies testing if self.dry_run: @@ -43,7 +46,7 @@ def handle_event(self, data, headers): installation_id ) as token: # Once the Sentry org has a .sentry repo we can remove the DSN from the deployment - dsn = fetch_dsn_for_github_org(org, token) + dsn = fetch_dsn_for_github_org(org, token, installation_id) client = GithubClient( token=token, dsn=dsn, @@ -52,7 +55,7 @@ def handle_event(self, data, headers): client.send_trace(data["workflow_job"]) else: # Once the Sentry org has a .sentry repo we can remove the DSN from the deployment - dsn = fetch_dsn_for_github_org(org, token) + dsn = fetch_dsn_for_github_org(org, token, installation_id) client = GithubClient( token=self.config.gh.token, dsn=dsn, diff --git a/tests/conftest.py b/tests/conftest.py index b1e96f0..a6f09bf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,6 +4,13 @@ import pytest +from src import sentry_config + + +@pytest.fixture(autouse=True) +def clear_dsn_cache(): + sentry_config._dsn_cache.clear() + @pytest.fixture def jobA_job(): @@ -17,12 +24,6 @@ def jobA_runs(): return json.load(f) -@pytest.fixture -def jobA_workflow(): - with open("tests/fixtures/jobA/workflow.json") as f: - return json.load(f) - - @pytest.fixture def skipped_workflow(): with open("tests/fixtures/skipped_workflow.json") as f: @@ -65,9 +66,3 @@ def failure_job(): def failure_runs(): with open("tests/fixtures/failedJob/runs.json") as f: return json.load(f) - - -@pytest.fixture -def failure_workflow(): - with open("tests/fixtures/failedJob/workflow.json") as f: - return json.load(f) diff --git a/tests/fixtures/failedJob/workflow.json b/tests/fixtures/failedJob/workflow.json deleted file mode 100644 index 259e1fd..0000000 --- a/tests/fixtures/failedJob/workflow.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "id": 48858869248, - "node_id": "CR_kwDOPkC4Ds8AAAALYDcuAA", - "name": "test", - "path": ".github/workflows/test.yml", - "state": "active", - "created_at": "2025-08-25T22:03:57Z", - "updated_at": "2025-08-25T22:03:57Z", - "url": "https://api.github.com/repos/getsentry/ken-test-repo/actions/workflows/48858869248", - "html_url": "https://github.com/getsentry/ken-test-repo/actions/workflows/test.yml", - "badge_url": "https://github.com/getsentry/ken-test-repo/workflows/test/badge.svg" -} diff --git a/tests/fixtures/jobA/runs.json b/tests/fixtures/jobA/runs.json index cc27e3e..80142ed 100644 --- a/tests/fixtures/jobA/runs.json +++ b/tests/fixtures/jobA/runs.json @@ -4,6 +4,7 @@ "node_id": "WFR_kwLOAA1TcM59c-PH", "head_branch": "ahmed/add-naming-layer", "head_sha": "fd976218a94d8a0cd203c711ea7dbe68c573ef4d", + "path": ".github/workflows/acceptance.yml", "run_number": 53142, "event": "pull_request", "status": "completed", diff --git a/tests/fixtures/jobA/workflow.json b/tests/fixtures/jobA/workflow.json deleted file mode 100644 index 3398a0f..0000000 --- a/tests/fixtures/jobA/workflow.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "id": 1174556, - "node_id": "MDg6V29ya2Zsb3cxMTc0NTU2", - "name": "acceptance", - "path": ".github/workflows/acceptance.yml", - "state": "active", - "created_at": "2020-05-01T23:00:30.000Z", - "updated_at": "2022-03-17T18:24:06.000Z", - "url": "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556", - "html_url": "https://github.com/getsentry/sentry/blob/master/.github/workflows/acceptance.yml", - "badge_url": "https://github.com/getsentry/sentry/workflows/acceptance/badge.svg" -} diff --git a/tests/test_github_sdk.py b/tests/test_github_sdk.py index 7f7e401..6df063a 100644 --- a/tests/test_github_sdk.py +++ b/tests/test_github_sdk.py @@ -66,7 +66,6 @@ def test_trace_generation( mock_get_uuid, jobA_job, jobA_runs, - jobA_workflow, jobA_trace, uuid_list, ): @@ -75,10 +74,6 @@ def test_trace_generation( "https://api.github.com/repos/getsentry/sentry/actions/runs/2104746951", json=jobA_runs, ) - responses.get( - "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556", - json=jobA_workflow, - ) client = GithubClient(dsn=DSN, token=TOKEN) trace = client._generate_trace(jobA_job) assert trace == jobA_trace @@ -91,16 +86,11 @@ def test_trace_generation( def test_trace_generation_with_failing_steps( failure_job, failure_runs, - failure_workflow, ): responses.get( failure_job["run_url"], json=failure_runs, ) - responses.get( - failure_runs["workflow_url"], - json=failure_workflow, - ) client = GithubClient(dsn=DSN, token=TOKEN) trace = client._generate_trace(failure_job) @@ -110,6 +100,32 @@ def test_trace_generation_with_failing_steps( assert trace["tags"]["event"] == "push" +@pytest.mark.parametrize( + "path", + [ + ".github/workflows/acceptance.yml", + ".github/workflows/acceptance.yml@main", + "getsentry/.github/.github/workflows/acceptance.yml@refs/heads/main", + "getsentry/.github/.github/workflows/acceptance.yml@1595d4b6de6a9e9751fb270a41019ce507d4099e", + ], +) +@responses.activate +def test_trace_generation_for_required_workflow(jobA_job, jobA_runs, path): + # Fetching a required workflow's `workflow_url` fails with a 422, and `responses` + # raises on any request that isn't registered + jobA_runs[ + "workflow_url" + ] = "https://api.github.com/repos/getsentry/sentry/actions/required_workflows/1" + # A required workflow's path can name the repo it lives in and end in a ref + jobA_runs["path"] = path + responses.get(jobA_job["run_url"], json=jobA_runs) + + trace = GithubClient(dsn=DSN, token=TOKEN)._generate_trace(jobA_job) + + assert trace["tags"]["workflow"] == "acceptance.yml" + assert len(responses.calls) == 1 + + @freeze_time() @responses.activate @patch("src.github_sdk.get_uuid") @@ -117,7 +133,6 @@ def test_send_trace( mock_get_uuid, jobA_job, jobA_runs, - jobA_workflow, uuid_list, ): mock_get_uuid.side_effect = uuid_list @@ -125,10 +140,6 @@ def test_send_trace( "https://api.github.com/repos/getsentry/sentry/actions/runs/2104746951", json=jobA_runs, ) - responses.get( - "https://api.github.com/repos/getsentry/sentry/actions/workflows/1174556", - json=jobA_workflow, - ) responses.post("https://foo@random.ingest.sentry.io/api/bar/envelope/") diff --git a/tests/test_sentry_config_file.py b/tests/test_sentry_config_file.py index db95aa4..7c62d70 100644 --- a/tests/test_sentry_config_file.py +++ b/tests/test_sentry_config_file.py @@ -1,8 +1,11 @@ from __future__ import annotations +import base64 from unittest import TestCase +import pytest import responses +from requests import HTTPError from src.sentry_config import fetch_dsn_for_github_org from src.sentry_config import SENTRY_CONFIG_API_URL as api_url @@ -30,6 +33,15 @@ } org = "armenzg" token = "foo_token" +installation_id = 1 + + +def config_file_meta(dsn: str) -> dict: + contents = f"[sentry-github-actions-app]\ndsn = {dsn}\n" + return { + **sentry_config_file_meta, + "content": base64.b64encode(contents.encode()).decode(), + } class TestSentryConfigCase(TestCase): @@ -45,7 +57,54 @@ def setUp(self) -> None: @responses.activate def test_fetch_parse_sentry_config_file(self) -> None: - assert fetch_dsn_for_github_org(org, token) == expected_dsn + assert fetch_dsn_for_github_org(org, token, installation_id) == expected_dsn + + @responses.activate + def test_dsn_is_cached_per_installation_and_org(self) -> None: + other_org_dsn = "https://e0bb9a6e1c4f4d3c9b3a6c2d8f1e7a5b@o1.ingest.sentry.io/2" + other_org_api_url = api_url.replace("{owner}", "other_org") + responses.get(other_org_api_url, json=config_file_meta(other_org_dsn)) + + # Every webhook comes with a new token + assert fetch_dsn_for_github_org(org, "token_1", 1) == expected_dsn + assert fetch_dsn_for_github_org(org, "token_2", 1) == expected_dsn + assert fetch_dsn_for_github_org("other_org", "token_3", 2) == other_org_dsn + responses.assert_call_count(self.api_url, 1) + responses.assert_call_count(other_org_api_url, 1) + + # Another installation has to be able to read the org's config itself + responses.replace(responses.GET, self.api_url, status=404) + with pytest.raises(HTTPError): + fetch_dsn_for_github_org(org, "token_4", 2) + + @responses.activate + def test_failed_fetch_is_not_cached(self) -> None: + responses.replace(responses.GET, self.api_url, status=403) + with pytest.raises(HTTPError): + fetch_dsn_for_github_org(org, token, installation_id) + + # e.g. the org's GitHub API rate limit has been reset + responses.replace( + responses.GET, + self.api_url, + json=sentry_config_file_meta, + status=200, + ) + assert fetch_dsn_for_github_org(org, token, installation_id) == expected_dsn + + @responses.activate + def test_failed_fetch_does_not_expose_cached_dsns(self) -> None: + fetch_dsn_for_github_org(org, token, installation_id) + responses.get(api_url.replace("{owner}", "other_org"), status=403) + + with pytest.raises(HTTPError) as excinfo: + fetch_dsn_for_github_org("other_org", token, 2) + + # Sentry sends every frame's local variables along with an error + tb = excinfo.tb + while tb: + assert expected_dsn not in repr(tb.tb_frame.f_locals) + tb = tb.tb_next def test_fetch_private_repo(self) -> None: pass diff --git a/tests/test_web_app_handler.py b/tests/test_web_app_handler.py index e5bb0ec..c1034ac 100644 --- a/tests/test_web_app_handler.py +++ b/tests/test_web_app_handler.py @@ -1,9 +1,13 @@ from __future__ import annotations +import base64 import json from unittest import mock import pytest +import responses +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa from src.web_app_handler import WebAppHandler @@ -54,6 +58,38 @@ def test_not_completed_workflow(): assert http_code == 200 +@pytest.fixture +def gh_app_env(monkeypatch): + private_key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + pem = private_key.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ) + monkeypatch.delenv("K_SERVICE", raising=False) + monkeypatch.setenv("GH_APP_ID", "1") + monkeypatch.setenv("GH_APP_PRIVATE_KEY", base64.b64encode(pem).decode()) + + +@responses.activate +def test_skipped_job_makes_no_api_calls(gh_app_env, skipped_workflow): + handler = WebAppHandler() + # Only Github App mode mints an installation token + assert handler.config.gh_app + reason, http_code = handler.handle_event( + data={ + "action": "completed", + "installation": {"id": 1}, + "repository": {"owner": {"login": "getsentry"}}, + "workflow_job": skipped_workflow, + }, + headers={"X-GitHub-Event": "workflow_job"}, + ) + assert reason == "Skipped jobs are not traced." + assert http_code == 200 + assert len(responses.calls) == 0 + + @pytest.mark.skip(reason="Not so important") def test_missing_workflow_job(monkeypatch): monkeypatch.delenv("GH_APP_ID", raising=False)