ref(hc): Improve default monolith region handling (#52768)

Change default GlobalRegionDirectory behavior to auto-generate a default
monolith region only if no region data is configured. This should be a
no-op change for healthy environments, but will cause incomplete configs
(such as in a dev environment) to fail earlier and more conspicuously.

Add `is_historic_monolith_region` to encapsulate special handling of the
region when it isn't auto-generated, with an explanatory docstring.

Clean up associated tests and exception handling. See also
https://github.com/getsentry/sentry/pull/53028

src/sentry/api/endpoints/accept_organization_invite.py

@@ -2,7 +2,6 @@ from __future__ import annotations
 
 from typing import Mapping, Optional
 
-from django.conf import settings
 from django.urls import reverse
 from rest_framework import status
 from rest_framework.request import Request
@@ -42,7 +41,7 @@ def get_invite_state(
         )
         for mapping in org_mappings:
             try:
-                if get_region_by_name(mapping.region_name).name == settings.SENTRY_MONOLITH_REGION:
+                if get_region_by_name(mapping.region_name).is_historic_monolith_region():
                     member_mapping = member_mappings.get(mapping.organization_id)
                     break
             except RegionResolutionError:

src/sentry/conf/server.py

@@ -3496,6 +3496,7 @@ if USE_SILOS:
             "api_token": "dev-region-silo-token",
         }
     ]
+    SENTRY_MONOLITH_REGION = SENTRY_REGION_CONFIG[0]["name"]
     # RPC authentication and address information
     RPC_SHARED_SECRET = [
         "a-long-value-that-is-shared-but-also-secret",

src/sentry/testutils/factories.py

@@ -4,6 +4,7 @@ import io
 import os
 import random
 from binascii import hexlify
+from contextlib import contextmanager
 from datetime import datetime
 from hashlib import sha1
 from importlib import import_module
@@ -16,6 +17,7 @@ from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
 from django.core.files.base import ContentFile
 from django.db import router, transaction
+from django.test.utils import override_settings
 from django.utils import timezone
 from django.utils.encoding import force_str
 from django.utils.text import slugify
@@ -106,12 +108,13 @@ from sentry.sentry_apps.apps import SentryAppCreator
 from sentry.services.hybrid_cloud.app.serial import serialize_sentry_app_installation
 from sentry.services.hybrid_cloud.hook import hook_service
 from sentry.signals import project_created
-from sentry.silo import SiloMode
+from sentry.silo import SiloMode, unguarded_write
 from sentry.snuba.dataset import Dataset
 from sentry.testutils.outbox import outbox_runner
 from sentry.testutils.silo import assume_test_silo_mode
 from sentry.types.activity import ActivityType
 from sentry.types.integrations import ExternalProviders
+from sentry.types.region import Region, get_region_by_name
 from sentry.utils import json, loremipsum
 from sentry.utils.performance_issues.performance_problem import PerformanceProblem
 
@@ -262,11 +265,30 @@ def _patch_artifact_manifest(path, org=None, release=None, project=None, extra_f
 class Factories:
     @staticmethod
     @assume_test_silo_mode(SiloMode.REGION)
-    def create_organization(name=None, owner=None, **kwargs):
+    def create_organization(name=None, owner=None, region: Region | str | None = None, **kwargs):
         if not name:
             name = petname.generate(2, " ", letters=10).title()
 
-        org = Organization.objects.create(name=name, **kwargs)
+        if isinstance(region, str):
+            region = get_region_by_name(region)
+
+        @contextmanager
+        def org_creation_context():
+            if region is None:
+                yield
+            else:
+                with override_settings(SILO_MODE=SiloMode.REGION, SENTRY_REGION=region.name):
+                    yield
+
+        with org_creation_context():
+            org = Organization.objects.create(name=name, **kwargs)
+
+        if region is not None:
+            with assume_test_silo_mode(SiloMode.CONTROL), unguarded_write(
+                using=router.db_for_write(OrganizationMapping)
+            ):
+                mapping = OrganizationMapping.objects.get(organization_id=org.id)
+                mapping.update(region_name=region.name)
 
         if owner:
             Factories.create_member(organization=org, user_id=owner.id, role="owner")

src/sentry/testutils/helpers/api_gateway.py

@@ -1,8 +1,10 @@
+from __future__ import annotations
+
+import unittest.result
 from urllib.parse import parse_qs
 
 import responses
 from django.conf import settings
-from django.db import router
 from django.test import override_settings
 from django.urls import re_path
 from rest_framework.permissions import AllowAny
@@ -10,21 +12,11 @@ from rest_framework.response import Response
 
 from sentry.api.base import control_silo_endpoint, region_silo_endpoint
 from sentry.api.bases.organization import OrganizationEndpoint
-from sentry.models.organizationmapping import OrganizationMapping
-from sentry.silo import unguarded_write
 from sentry.testutils import APITestCase
+from sentry.testutils.region import override_regions
 from sentry.types.region import Region, RegionCategory, clear_global_regions
 from sentry.utils import json
 
-SENTRY_REGION_CONFIG = [
-    Region(
-        name="region1",
-        snowflake_id=1,
-        address="http://region1.testserver",
-        category=RegionCategory.MULTI_TENANT,
-    ),
-]
-
 
 @control_silo_endpoint
 class ControlEndpoint(OrganizationEndpoint):
@@ -115,31 +107,35 @@ def provision_middleware():
     return middleware
 
 
-@override_settings(SENTRY_REGION_CONFIG=SENTRY_REGION_CONFIG, ROOT_URLCONF=__name__)
+@override_settings(ROOT_URLCONF=__name__)
 class ApiGatewayTestCase(APITestCase):
+    _REGION = Region(
+        name="region1",
+        snowflake_id=1,
+        address="http://region1.testserver",
+        category=RegionCategory.MULTI_TENANT,
+    )
+
     def setUp(self):
         super().setUp()
         clear_global_regions()
         responses.add(
             responses.GET,
-            "http://region1.testserver/get",
+            f"{self._REGION.address}/get",
             body=json.dumps({"proxy": True}),
             content_type="application/json",
             adding_headers={"test": "header"},
         )
         responses.add(
             responses.GET,
-            "http://region1.testserver/error",
+            f"{self._REGION.address}/error",
             body=json.dumps({"proxy": True}),
             status=400,
             content_type="application/json",
             adding_headers={"test": "header"},
         )
 
-        with unguarded_write(using=router.db_for_write(OrganizationMapping)):
-            OrganizationMapping.objects.get(organization_id=self.organization.id).update(
-                region_name="region1"
-            )
+        self.organization = self.create_organization(region=self._REGION)
 
         # Echos the request body and header back for verification
         def return_request_body(request):
@@ -150,12 +146,13 @@ class ApiGatewayTestCase(APITestCase):
             params = parse_qs(request.url.split("?")[1])
             return (200, request.headers, json.dumps(params).encode())
 
-        responses.add_callback(
-            responses.GET, "http://region1.testserver/echo", return_request_params
-        )
-
-        responses.add_callback(
-            responses.POST, "http://region1.testserver/echo", return_request_body
-        )
+        responses.add_callback(responses.GET, f"{self._REGION.address}/echo", return_request_params)
+        responses.add_callback(responses.POST, f"{self._REGION.address}/echo", return_request_body)
 
         self.middleware = provision_middleware()
+
+    def run(
+        self, result: unittest.result.TestResult | None = ...
+    ) -> unittest.result.TestResult | None:
+        with override_regions([self._REGION]):
+            return super().run(result)

src/sentry/testutils/region.py

@@ -1,11 +1,13 @@
 from contextlib import contextmanager
-from typing import Iterable
+from typing import Any, Mapping, Sequence
+
+from django.test.utils import override_settings
 
 from sentry.types import region
 
 
 @contextmanager
-def override_regions(regions: Iterable[region.Region]):
+def override_regions(regions: Sequence[region.Region]):
     """Override the global set of existing regions.
 
     The overriding value takes the place of the `SENTRY_REGION_CONFIG` setting and
@@ -14,7 +16,19 @@ def override_regions(regions: Iterable[region.Region]):
     because the region mapping may already be cached.
     """
 
-    mapping = region.GlobalRegionDirectory(list(regions))
+    @contextmanager
+    def fix_monolith_region_pointer():
+        # Set SENTRY_MONOLITH_REGION to make GlobalRegionDirectory validation happy.
+        # This won't affect the behavior of the Region.is_historic_monolith_region method;
+        # tests that rely on it must override SENTRY_MONOLITH_REGION in their own cases.
+        if regions:
+            with override_settings(SENTRY_MONOLITH_REGION=regions[0].name):
+                yield
+        else:
+            yield
+
+    with fix_monolith_region_pointer():
+        mapping = region.GlobalRegionDirectory(regions)
 
     def override() -> region.GlobalRegionDirectory:
         return mapping
@@ -26,3 +40,10 @@ def override_regions(regions: Iterable[region.Region]):
         yield
     finally:
         region.load_global_regions = existing
+
+
+@contextmanager
+def override_region_config(region_configs: Sequence[Mapping[str, Any]]):
+    region_objs = tuple(region.parse_raw_config(region_configs))
+    with override_regions(region_objs):
+        yield

src/sentry/testutils/silo.py

@@ -88,7 +88,8 @@ class _SiloModeTestCase(TestCase):
                     with override_settings(SENTRY_REGION=self.regions[0].name):
                         return super().run(result)
                 else:
-                    return super().run(result)
+                    with override_settings(SENTRY_MONOLITH_REGION=self.regions[0].name):
+                        return super().run(result)
 
 
 class SiloModeTestDecorator:

src/sentry/types/region.py

@@ -76,6 +76,21 @@ class Region:
 
         return urljoin(generate_region_url(self.name), path)
 
+    def is_historic_monolith_region(self) -> bool:
+        """Check whether this is a historic monolith region.
+
+        In a monolith environment, there exists only the one monolith "region",
+        which is a dummy object.
+
+        In a siloed environment whose data was migrated from a monolith environment,
+        all region-scoped entities that existed before the migration belong to the
+        historic monolith region by default. Unlike in the monolith environment,
+        this region is not a dummy object, but nonetheless is subject to special
+        cases to ensure that legacy data is handled correctly.
+        """
+
+        return self.name == settings.SENTRY_MONOLITH_REGION
+
 
 class RegionResolutionError(Exception):
     """Indicate that a region's identity could not be resolved."""
@@ -93,14 +108,20 @@ class GlobalRegionDirectory:
     """The set of all regions in this Sentry platform instance."""
 
     def __init__(self, regions: Collection[Region]) -> None:
-        if not any(r.name == settings.SENTRY_MONOLITH_REGION for r in regions):
+        if not regions:
             default_monolith_region = Region(
                 name=settings.SENTRY_MONOLITH_REGION,
                 snowflake_id=0,
                 address=options.get("system.url-prefix"),
                 category=RegionCategory.MULTI_TENANT,
             )
-            regions = [default_monolith_region, *regions]
+            regions = [default_monolith_region]
+        elif not any(r.name == settings.SENTRY_MONOLITH_REGION for r in regions):
+            raise RegionConfigurationError(
+                "The SENTRY_MONOLITH_REGION setting must point to a region name "
+                f"({settings.SENTRY_MONOLITH_REGION=!r}; "
+                f"region names = {[r.name for r in regions]!r})"
+            )
 
         self.regions = frozenset(regions)
         self.by_name = {r.name: r for r in self.regions}
@@ -110,7 +131,7 @@ class GlobalRegionDirectory:
             region.validate()
 
 
-def _parse_config(region_config: Any) -> Iterable[Region]:
+def parse_raw_config(region_config: Any) -> Iterable[Region]:
     if isinstance(region_config, (str, bytes)):
         json_config_values = json.loads(region_config)
         config_values = parse_obj_as(List[Region], json_config_values)
@@ -133,11 +154,14 @@ def _parse_config(region_config: Any) -> Iterable[Region]:
 
 def load_from_config(region_config: Any) -> GlobalRegionDirectory:
     try:
-        region_objs = list(_parse_config(region_config))
+        region_objs = list(parse_raw_config(region_config))
         return GlobalRegionDirectory(region_objs)
+    except RegionConfigurationError as e:
+        sentry_sdk.capture_exception(e)
+        raise
     except Exception as e:
         sentry_sdk.capture_exception(e)
-        raise RegionConfigurationError("Unable to parse region_config.")
+        raise RegionConfigurationError("Unable to parse region_config.") from e
 
 
 _global_regions: GlobalRegionDirectory | None = None
@@ -164,18 +188,15 @@ def clear_global_regions() -> None:
 
 def get_region_by_name(name: str) -> Region:
     """Look up a region by name."""
+    global_regions = load_global_regions()
     try:
-        return load_global_regions().by_name[name]
-    except KeyError:
-        raise RegionResolutionError(f"No region with name: {name!r}")
+        return global_regions.by_name[name]
+    except KeyError as e:
+        raise RegionResolutionError(f"No region with name: {name!r}") from e
 
 
 def is_region_name(name: str) -> bool:
-    try:
-        get_region_by_name(name)
-        return True
-    except Exception:
-        return False
+    return name in load_global_regions().by_name
 
 
 @control_silo_function

tests/sentry/api/test_base.py

@@ -16,8 +16,8 @@ from sentry.services.hybrid_cloud.util import FunctionSiloLimit
 from sentry.silo import SiloMode
 from sentry.testutils import APITestCase
 from sentry.testutils.helpers.options import override_options
+from sentry.testutils.region import override_region_config
 from sentry.types.region import RegionCategory, clear_global_regions
-from sentry.utils import json
 from sentry.utils.cursors import Cursor
 
 
@@ -456,7 +456,7 @@ class CustomerDomainTest(APITestCase):
                 "category": RegionCategory.MULTI_TENANT.name,
             },
         ]
-        with override_settings(SENTRY_REGION_CONFIG=json.dumps(region_config)):
+        with override_region_config(region_config):
             assert request_with_subdomain("na") == "na"
             assert request_with_subdomain("eu") == "eu"
             assert request_with_subdomain("sentry") is None

tests/sentry/hybrid_cloud/test_region.py

@@ -1,8 +1,6 @@
 import pytest
-from django.db import router
 from django.test import override_settings
 
-from sentry.models.organizationmapping import OrganizationMapping
 from sentry.models.organizationmember import OrganizationMember
 from sentry.services.hybrid_cloud.region import (
     ByOrganizationId,
@@ -13,7 +11,7 @@ from sentry.services.hybrid_cloud.region import (
     UnimplementedRegionResolution,
 )
 from sentry.services.hybrid_cloud.rpc import RpcServiceUnimplementedException
-from sentry.silo import SiloMode, unguarded_write
+from sentry.silo import SiloMode
 from sentry.testutils import TestCase
 from sentry.testutils.region import override_regions
 from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
@@ -29,16 +27,7 @@ _TEST_REGIONS = (
 class RegionResolutionTest(TestCase):
     def setUp(self):
         self.target_region = _TEST_REGIONS[0]
-        self.organization = self._create_org_in_region(self.target_region)
-
-    def _create_org_in_region(self, target_region):
-        with override_settings(SENTRY_REGION=target_region.name):
-            organization = self.create_organization()
-        org_mapping = OrganizationMapping.objects.get(organization_id=organization.id)
-        with unguarded_write(using=router.db_for_write(OrganizationMapping)):
-            org_mapping.region_name = target_region.name
-            org_mapping.save()
-        return organization
+        self.organization = self.create_organization(region=self.target_region)
 
     def test_by_organization_object(self):
         region_resolution = ByOrganizationObject()
@@ -85,7 +74,7 @@ class RegionResolutionTest(TestCase):
                 region_resolution.resolve({})
 
         with override_regions(_TEST_REGIONS), override_settings(SENTRY_SINGLE_ORGANIZATION=True):
-            self._create_org_in_region(_TEST_REGIONS[1])
+            self.create_organization(region=_TEST_REGIONS[1])
             with pytest.raises(RegionResolutionError):
                 region_resolution.resolve({})
 

tests/sentry/middleware/test_customer_domain.py

@@ -8,9 +8,9 @@ from rest_framework.response import Response
 from sentry.api.base import Endpoint
 from sentry.middleware.customer_domain import CustomerDomainMiddleware
 from sentry.testutils import APITestCase, TestCase
+from sentry.testutils.region import override_region_config
 from sentry.testutils.silo import control_silo_test
 from sentry.types.region import RegionCategory, clear_global_regions
-from sentry.utils import json
 from sentry.web.frontend.auth_logout import AuthLogoutView
 
 
@@ -130,7 +130,7 @@ class CustomerDomainMiddlewareTest(TestCase):
                 "category": RegionCategory.MULTI_TENANT.name,
             },
         ]
-        with override_settings(SENTRY_REGION_CONFIG=json.dumps(region_configs)):
+        with override_region_config(region_configs):
             for region in region_configs:
                 request = RequestFactory().get("/")
                 request.subdomain = region["name"]