From 53f5065089adc3bc00e37e3b65e003242f1e8206 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 27 Feb 2025 16:33:23 -0800 Subject: [PATCH] fix(uptime): Enforce domain limits everywhere Currently we're only enforcing these at the api level. Move them to autodetection as well, and add an index to make sure we can still query them cheaply. --- migrations_lockfile.txt | 2 +- src/sentry/uptime/detectors/ranking.py | 20 +++++++++-- src/sentry/uptime/detectors/tasks.py | 2 +- src/sentry/uptime/endpoints/validators.py | 17 ++++------ ...7_uptime_subscription_index_domain_cols.py | 34 +++++++++++++++++++ src/sentry/uptime/models.py | 1 + .../uptime/subscriptions/subscriptions.py | 21 ++++++++++++ tests/sentry/uptime/detectors/test_ranking.py | 14 ++++++++ .../test_project_uptime_alert_details.py | 2 +- 9 files changed, 97 insertions(+), 16 deletions(-) create mode 100644 src/sentry/uptime/migrations/0027_uptime_subscription_index_domain_cols.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index f2833d0d54b316..2f283e5c9f02f4 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -21,6 +21,6 @@ social_auth: 0002_default_auto_field tempest: 0002_make_message_type_nullable -uptime: 0026_region_mode_col +uptime: 0027_uptime_subscription_index_domain_cols workflow_engine: 0032_remove_data_source_query_id diff --git a/src/sentry/uptime/detectors/ranking.py b/src/sentry/uptime/detectors/ranking.py index 7fa8cf1b443c96..7a1fc3bb100d51 100644 --- a/src/sentry/uptime/detectors/ranking.py +++ b/src/sentry/uptime/detectors/ranking.py @@ -10,7 +10,11 @@ from sentry.constants import UPTIME_AUTODETECTION from sentry.uptime.models import get_active_auto_monitor_count_for_org -from sentry.uptime.subscriptions.subscriptions import MAX_AUTO_SUBSCRIPTIONS_PER_ORG +from sentry.uptime.subscriptions.subscriptions import ( + MAX_AUTO_SUBSCRIPTIONS_PER_ORG, + MaxUrlsForDomainReachedException, + check_url_limits, +) from sentry.utils import metrics, redis if TYPE_CHECKING: @@ -105,14 +109,24 @@ def delete_candidate_projects_for_org(org: Organization) -> None: cluster.delete(key) -def get_candidate_urls_for_project(project: Project) -> list[tuple[str, int]]: +def get_candidate_urls_for_project(project: Project, limit=5) -> list[tuple[str, int]]: """ Gets all the candidate urls for a project. Returns a tuple of (url, times_url_seen). Urls are sorted by `times_url_seen` desc. """ key = get_project_base_url_rank_key(project) cluster = _get_cluster() - return cluster.zrange(key, 0, -1, desc=True, withscores=True, score_cast_func=int) + candidate_urls = cluster.zrange(key, 0, -1, desc=True, withscores=True, score_cast_func=int) + urls = [] + for candidate_url, url_count in candidate_urls: + try: + check_url_limits(candidate_url) + urls.append((candidate_url, url_count)) + except MaxUrlsForDomainReachedException: + pass + if len(urls) == limit: + break + return urls def delete_candidate_urls_for_project(project: Project) -> None: diff --git a/src/sentry/uptime/detectors/tasks.py b/src/sentry/uptime/detectors/tasks.py index 69bc19e79cb047..1fb7b4b64d4835 100644 --- a/src/sentry/uptime/detectors/tasks.py +++ b/src/sentry/uptime/detectors/tasks.py @@ -146,7 +146,7 @@ def process_project_url_ranking(project: Project, project_url_count: int) -> boo found_url = False - for url, url_count in get_candidate_urls_for_project(project)[:5]: + for url, url_count in get_candidate_urls_for_project(project, limit=5): if process_candidate_url(project, project_url_count, url, url_count): found_url = True break diff --git a/src/sentry/uptime/endpoints/validators.py b/src/sentry/uptime/endpoints/validators.py index 808a4f11406284..4f7e5571c5d4ed 100644 --- a/src/sentry/uptime/endpoints/validators.py +++ b/src/sentry/uptime/endpoints/validators.py @@ -11,7 +11,6 @@ from sentry.auth.superuser import is_active_superuser from sentry.constants import ObjectStatus from sentry.models.environment import Environment -from sentry.uptime.detectors.url_extraction import extract_domain_parts from sentry.uptime.models import ( ProjectUptimeSubscription, ProjectUptimeSubscriptionMode, @@ -20,13 +19,14 @@ from sentry.uptime.subscriptions.subscriptions import ( MAX_MANUAL_SUBSCRIPTIONS_PER_ORG, MaxManualUptimeSubscriptionsReached, + MaxUrlsForDomainReachedException, UptimeMonitorNoSeatAvailable, + check_url_limits, create_project_uptime_subscription, update_project_uptime_subscription, ) from sentry.utils.audit import create_audit_entry -MAX_MONITORS_PER_DOMAIN = 100 """ The bounding upper limit on how many ProjectUptimeSubscription's can exist for a single domain + suffix. @@ -155,15 +155,12 @@ def validate(self, attrs): return attrs def validate_url(self, url): - url_parts = extract_domain_parts(url) - existing_count = ProjectUptimeSubscription.objects.filter( - uptime_subscription__url_domain=url_parts.domain, - uptime_subscription__url_domain_suffix=url_parts.suffix, - ).count() - - if existing_count >= MAX_MONITORS_PER_DOMAIN: + try: + check_url_limits(url) + except MaxUrlsForDomainReachedException as e: raise serializers.ValidationError( - f"The domain *.{url_parts.domain}.{url_parts.suffix} has already been used in {MAX_MONITORS_PER_DOMAIN} uptime monitoring alerts, which is the limit. You cannot create any additional alerts for this domain." + f"The domain *.{e.domain}.{e.suffix} has already been used in {e.max_urls} uptime monitoring alerts, " + "which is the limit. You cannot create any additional alerts for this domain." ) return url diff --git a/src/sentry/uptime/migrations/0027_uptime_subscription_index_domain_cols.py b/src/sentry/uptime/migrations/0027_uptime_subscription_index_domain_cols.py new file mode 100644 index 00000000000000..c756092c953c41 --- /dev/null +++ b/src/sentry/uptime/migrations/0027_uptime_subscription_index_domain_cols.py @@ -0,0 +1,34 @@ +# Generated by Django 5.1.5 on 2025-02-28 00:32 + +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + # This flag is used to mark that a migration shouldn't be automatically run in production. + # This should only be used for operations where it's safe to run the migration after your + # code has deployed. So this should not be used for most operations that alter the schema + # of a table. + # Here are some things that make sense to mark as post deployment: + # - Large data migrations. Typically we want these to be run manually so that they can be + # monitored and not block the deploy for a long period of time while they run. + # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to + # run this outside deployments so that we don't block them. Note that while adding an index + # is a schema change, it's completely safe to run the operation after the code has deployed. + # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment + + is_post_deployment = False + + dependencies = [ + ("uptime", "0026_region_mode_col"), + ] + + operations = [ + migrations.AddIndex( + model_name="uptimesubscription", + index=models.Index( + fields=["url_domain_suffix", "url_domain"], name="uptime_upti_url_dom_ead522_idx" + ), + ), + ] diff --git a/src/sentry/uptime/models.py b/src/sentry/uptime/models.py index 57c33e6bb9d5c7..7cc0868f27da52 100644 --- a/src/sentry/uptime/models.py +++ b/src/sentry/uptime/models.py @@ -115,6 +115,7 @@ class Meta: name="uptime_uptimesubscription_unique_subscription_check_4", ), ] + indexes = (models.Index(fields=("url_domain_suffix", "url_domain")),) @region_silo_model diff --git a/src/sentry/uptime/subscriptions/subscriptions.py b/src/sentry/uptime/subscriptions/subscriptions.py index 98e3e7476c85df..98936e45305c76 100644 --- a/src/sentry/uptime/subscriptions/subscriptions.py +++ b/src/sentry/uptime/subscriptions/subscriptions.py @@ -31,6 +31,7 @@ UPTIME_SUBSCRIPTION_TYPE = "uptime_monitor" MAX_AUTO_SUBSCRIPTIONS_PER_ORG = 1 MAX_MANUAL_SUBSCRIPTIONS_PER_ORG = 100 +MAX_MONITORS_PER_DOMAIN = 100 class MaxManualUptimeSubscriptionsReached(ValueError): @@ -447,3 +448,23 @@ def check_and_update_regions( ) deleted_region.delete() return True + + +class MaxUrlsForDomainReachedException(Exception): + def __init__(self, domain, suffix, max_urls): + self.domain = domain + self.suffix = suffix + self.max_urls = max_urls + + +def check_url_limits(url): + url_parts = extract_domain_parts(url) + existing_count = ProjectUptimeSubscription.objects.filter( + uptime_subscription__url_domain=url_parts.domain, + uptime_subscription__url_domain_suffix=url_parts.suffix, + ).count() + + if existing_count >= MAX_MONITORS_PER_DOMAIN: + raise MaxUrlsForDomainReachedException( + url_parts.domain, url_parts.suffix, MAX_MONITORS_PER_DOMAIN + ) diff --git a/tests/sentry/uptime/detectors/test_ranking.py b/tests/sentry/uptime/detectors/test_ranking.py index db19837c29e18d..43e37cd3b85f63 100644 --- a/tests/sentry/uptime/detectors/test_ranking.py +++ b/tests/sentry/uptime/detectors/test_ranking.py @@ -134,6 +134,20 @@ def test(self): add_base_url_to_rank(self.project, url_2) assert get_candidate_urls_for_project(self.project) == [(url_2, 2), (url_1, 1)] + def test_limits(self): + with mock.patch("sentry.uptime.subscriptions.subscriptions.MAX_MONITORS_PER_DOMAIN", 1): + other_proj = self.create_project() + url_1 = "https://sentry.io" + self.create_project_uptime_subscription( + project=other_proj, uptime_subscription=self.create_uptime_subscription(url=url_1) + ) + url_2 = "https://sentry.sentry.io" + url_3 = "https://sentry.santry.io" + add_base_url_to_rank(self.project, url_1) + add_base_url_to_rank(self.project, url_2) + add_base_url_to_rank(self.project, url_3) + assert get_candidate_urls_for_project(self.project) == [(url_3, 1)] + class DeleteCandidateUrlsForProjectTest(UptimeTestCase): def test(self): diff --git a/tests/sentry/uptime/endpoints/test_project_uptime_alert_details.py b/tests/sentry/uptime/endpoints/test_project_uptime_alert_details.py index 8ee0165df0fee4..9530cdeec6ab6c 100644 --- a/tests/sentry/uptime/endpoints/test_project_uptime_alert_details.py +++ b/tests/sentry/uptime/endpoints/test_project_uptime_alert_details.py @@ -172,7 +172,7 @@ def test_not_found(self): resp = self.get_error_response(self.organization.slug, self.project.slug, 3) assert resp.status_code == 404 - @mock.patch("sentry.uptime.endpoints.validators.MAX_MONITORS_PER_DOMAIN", 1) + @mock.patch("sentry.uptime.subscriptions.subscriptions.MAX_MONITORS_PER_DOMAIN", 1) def test_domain_limit(self): # First monitor is for test-one.example.com self.create_project_uptime_subscription(