From 840be1aa164b76f82c1ef23dd486211e025824e8 Mon Sep 17 00:00:00 2001 From: Gguidini Date: Fri, 28 Jun 2024 14:41:08 +0200 Subject: [PATCH 1/3] feat: track suspended apps This column is part of the model since https://github.com/codecov/shared/pull/237 but was not reflected in the worker yet. related: https://github.com/codecov/internal-issues/issues/519 --- database/models/core.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/database/models/core.py b/database/models/core.py index de7af1023..fdd75eea7 100644 --- a/database/models/core.py +++ b/database/models/core.py @@ -194,6 +194,8 @@ class GithubAppInstallation(CodecovBaseModel, MixinBaseClass): Owner, foreign_keys=[ownerid], back_populates="github_app_installations" ) + is_suspended = Column(types.Boolean, server_default=FetchedValue()) + def repository_queryset(self, dbsession: Session): """Returns a query set of repositories covered by this installation""" if self.repository_service_ids is None: From fcc0563968f5df7dc874ba0ce9e4fb018cba183e Mon Sep 17 00:00:00 2001 From: Gguidini Date: Tue, 2 Jul 2024 12:36:03 +0200 Subject: [PATCH 2/3] feat: don't select suspended apps Currently we ignore if an app is suspended or not. An app is suspended by the user. If we select a suspended app we will fail to get an access_token for it and eventually fail with `InstallationError` These changes filter out suspended apps from being selected. In case all apps for the user are suspended we skip notifications. Other tasks would fail with `NoConfiguredAppsAvailable`. closes https://github.com/codecov/internal-issues/issues/519 --- helpers/exceptions.py | 7 +++-- services/bots/github_apps.py | 43 +++++++++++++++++++++------- tasks/notify.py | 42 ++++++++++++++++++++------- tasks/tests/unit/test_notify_task.py | 37 ++++++++++++++++++++++-- 4 files changed, 103 insertions(+), 26 deletions(-) diff --git a/helpers/exceptions.py b/helpers/exceptions.py index 5e4c8b6dd..e35522afc 100644 --- a/helpers/exceptions.py +++ b/helpers/exceptions.py @@ -21,9 +21,12 @@ class OwnerWithoutValidBotError(Exception): class NoConfiguredAppsAvailable(Exception): - def __init__(self, apps_count: int, all_rate_limited: bool) -> None: + def __init__( + self, apps_count: int, rate_limited_count: int, suspended_count: int + ) -> None: self.apps_count = apps_count - self.all_rate_limited = all_rate_limited + self.rate_limited_count = rate_limited_count + self.suspended_count = suspended_count class CorruptRawReportError(Exception): diff --git a/services/bots/github_apps.py b/services/bots/github_apps.py index 3e8760ead..a934e745c 100644 --- a/services/bots/github_apps.py +++ b/services/bots/github_apps.py @@ -169,6 +169,26 @@ def get_specific_github_app_details( ) +def _filter_rate_limited_apps( + apps_to_consider: List[GithubAppInstallation], +) -> List[GithubAppInstallation]: + redis_connection = get_redis_connection() + return list( + filter( + lambda obj: not is_installation_rate_limited( + redis_connection, obj.installation_id, app_id=obj.app_id + ), + apps_to_consider, + ) + ) + + +def _filter_suspended_apps( + apps_to_consider: List[GithubAppInstallation], +) -> List[GithubAppInstallation]: + return list(filter(lambda obj: not obj.is_suspended, apps_to_consider)) + + def get_github_app_info_for_owner( owner: Owner, *, @@ -225,18 +245,15 @@ def get_github_app_info_for_owner( owner, installation_name, repository ) apps_matching_criteria_count = len(apps_to_consider) - - redis_connection = get_redis_connection() - apps_to_consider = list( - filter( - lambda obj: not is_installation_rate_limited( - redis_connection, obj.installation_id, app_id=obj.app_id - ), - apps_to_consider, - ) - ) + # We can't use apps that are rate limited + apps_to_consider = _filter_rate_limited_apps(apps_to_consider) + rate_limited_apps_count = apps_matching_criteria_count - len(apps_to_consider) + # We can't use apps that are suspended (by the user) + apps_to_consider = _filter_suspended_apps(apps_to_consider) + suspended_apps_count = rate_limited_apps_count - len(apps_to_consider) if apps_to_consider: + # There's at least 1 app that matches all the criteria and can be used to communicate with GitHub main_name = apps_to_consider[0].name info_to_get_tokens = list( map( @@ -261,8 +278,12 @@ def get_github_app_info_for_owner( ) return info_to_get_tokens elif apps_matching_criteria_count > 0: + # There are apps that match the criteria, but we can't use them. + # Either they are currently rate limited or they have been suspended. raise NoConfiguredAppsAvailable( - apps_count=apps_matching_criteria_count, all_rate_limited=True + apps_count=apps_matching_criteria_count, + rate_limited_count=rate_limited_apps_count, + suspended_count=suspended_apps_count, ) # DEPRECATED FLOW - begin if owner.integration_id and ( diff --git a/tasks/notify.py b/tasks/notify.py index f1cfe748c..6a946c5f6 100644 --- a/tasks/notify.py +++ b/tasks/notify.py @@ -210,24 +210,44 @@ def run_impl_within_lock( self.log_checkpoint(kwargs, UploadFlow.NOTIF_NO_VALID_INTEGRATION) return {"notified": False, "notifications": None, "reason": "no_valid_bot"} except NoConfiguredAppsAvailable as exp: - # Min wait time of 1 minute - retry_delay_seconds = max(60, get_seconds_to_next_hour()) + if exp.rate_limited_count > 0: + # There's at least 1 app that we can use to communicate with GitHub, + # but this app happens to be rate limited now. We try again later. + # Min wait time of 1 minute + retry_delay_seconds = max(60, get_seconds_to_next_hour()) + log.warning( + "Unable to start notifications. Retrying again later.", + extra=dict( + repoid=repoid, + commit=commitid, + apps_available=exp.apps_count, + apps_rate_limited=exp.rate_limited_count, + apps_suspended=exp.suspended_count, + countdown_seconds=retry_delay_seconds, + ), + ) + return self._attempt_retry( + max_retries=10, + countdown=retry_delay_seconds, + current_yaml=current_yaml, + commit=commit, + **kwargs, + ) + # Maybe we have apps that are suspended. We can't communicate with github. log.warning( - "Unable to start notifications because all ghapps available are rate limited", + "We can't find an app to communicate with GitHub. Not notifying.", extra=dict( repoid=repoid, commit=commitid, apps_available=exp.apps_count, - countdown_seconds=retry_delay_seconds, + apps_suspended=exp.suspended_count, ), ) - return self._attempt_retry( - max_retries=10, - countdown=retry_delay_seconds, - current_yaml=current_yaml, - commit=commit, - **kwargs, - ) + return { + "notified": False, + "notifications": None, + "reason": "no_valid_github_app_found", + } if current_yaml is None: current_yaml = async_to_sync(get_current_yaml)(commit, repository_service) diff --git a/tasks/tests/unit/test_notify_task.py b/tasks/tests/unit/test_notify_task.py index 98569631f..f1d5289cf 100644 --- a/tasks/tests/unit/test_notify_task.py +++ b/tasks/tests/unit/test_notify_task.py @@ -809,13 +809,13 @@ def test_notify_task_no_bot(self, dbsession, mocker): assert expected_result == res @freeze_time("2024-04-22T11:15:00") - def test_notify_task_no_ghapp_available(self, dbsession, mocker): + def test_notify_task_no_ghapp_available_one_rate_limited(self, dbsession, mocker): get_repo_provider_service = mocker.patch( "tasks.notify.get_repo_provider_service" ) mock_retry = mocker.patch.object(NotifyTask, "retry", return_value=None) get_repo_provider_service.side_effect = NoConfiguredAppsAvailable( - apps_count=2, all_rate_limited=True + apps_count=2, rate_limited_count=1, suspended_count=1 ) commit = CommitFactory.create( message="", @@ -837,6 +837,39 @@ def test_notify_task_no_ghapp_available(self, dbsession, mocker): assert res is None mock_retry.assert_called_with(max_retries=10, countdown=45 * 60) + @freeze_time("2024-04-22T11:15:00") + def test_notify_task_no_ghapp_available_all_suspended(self, dbsession, mocker): + get_repo_provider_service = mocker.patch( + "tasks.notify.get_repo_provider_service" + ) + mock_retry = mocker.patch.object(NotifyTask, "retry", return_value=None) + get_repo_provider_service.side_effect = NoConfiguredAppsAvailable( + apps_count=1, rate_limited_count=0, suspended_count=1 + ) + commit = CommitFactory.create( + message="", + pullid=None, + branch="test-branch-1", + commitid="649eaaf2924e92dc7fd8d370ddb857033231e67a", + repository__using_integration=True, + ) + dbsession.add(commit) + dbsession.flush() + current_yaml = {"codecov": {"require_ci_to_pass": True}} + task = NotifyTask() + res = task.run_impl_within_lock( + dbsession, + repoid=commit.repoid, + commitid=commit.commitid, + current_yaml=current_yaml, + ) + assert res == { + "notified": False, + "notifications": None, + "reason": "no_valid_github_app_found", + } + mock_retry.assert_not_called() + def test_submit_third_party_notifications_exception(self, mocker, dbsession): current_yaml = {} repository = RepositoryFactory.create() From dc665067ed8b2ef0850e10154acdf773d571ffb7 Mon Sep 17 00:00:00 2001 From: Gguidini Date: Thu, 4 Jul 2024 11:00:55 +0200 Subject: [PATCH 3/3] address feedback --- services/bots/tests/test_github_apps.py | 67 +++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/services/bots/tests/test_github_apps.py b/services/bots/tests/test_github_apps.py index 14239f969..b5a2ef19c 100644 --- a/services/bots/tests/test_github_apps.py +++ b/services/bots/tests/test_github_apps.py @@ -1,10 +1,18 @@ +import datetime + import pytest from shared.typings.torngit import GithubInstallationInfo -from database.models.core import GithubAppInstallation +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from database.tests.factories.core import OwnerFactory -from helpers.exceptions import RequestedGithubAppNotFound -from services.bots.github_apps import get_specific_github_app_details +from helpers.exceptions import NoConfiguredAppsAvailable, RequestedGithubAppNotFound +from services.bots.github_apps import ( + get_github_app_info_for_owner, + get_specific_github_app_details, +) class TestGetSpecificGithubAppDetails(object): @@ -49,3 +57,56 @@ def test_get_specific_github_app_not_found(self, dbsession): owner = self._get_owner_with_apps(dbsession) with pytest.raises(RequestedGithubAppNotFound): get_specific_github_app_details(owner, 123456, "commit_id_for_logs") + + @pytest.mark.parametrize( + "app, is_rate_limited", + [ + pytest.param( + GithubAppInstallation( + repository_service_ids=None, + installation_id=1400, + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + app_id=400, + pem_path="pem_path", + created_at=datetime.datetime.now(datetime.UTC), + is_suspended=True, + ), + False, + id="suspended_app", + ), + pytest.param( + GithubAppInstallation( + repository_service_ids=None, + installation_id=1400, + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + app_id=400, + pem_path="pem_path", + created_at=datetime.datetime.now(datetime.UTC), + is_suspended=False, + ), + True, + id="rate_limited_app", + ), + ], + ) + def test_raise_NoAppsConfiguredAvailable_if_suspended_or_rate_limited( + self, app, is_rate_limited, mocker, dbsession + ): + owner = OwnerFactory( + service="github", + bot=None, + unencrypted_oauth_token="owner_token: :refresh_token", + ) + dbsession.add(owner) + + app.owner = owner + dbsession.add(app) + dbsession.flush() + + mock_is_rate_limited = mocker.patch( + "services.bots.github_apps.is_installation_rate_limited", + return_value=is_rate_limited, + ) + with pytest.raises(NoConfiguredAppsAvailable): + get_github_app_info_for_owner(owner) + mock_is_rate_limited.assert_called()