Browse Source

fix(hybridcloud) Add memberRegions to initialData (#66651)

When populating the organization list, we need to only send requests to
regions that the user *has* a membership in. The reason for this is that
sending requests to non-member regions will populate the user cache in
that region. However, that cache value will not be purged when the user
is modified because we only send RPC calls to member regions.

Sending RPC calls to all regions is an alternative solution, but a less
optimal one as it requires that all regions are available when users
make changes to their profiles, as RPC calls are made synchronously.

Having a separate region list for all the regions that exist is
necessary as that list is used during organization creation, relocations
and staff views.

Refs HC-1134
Mark Story 1 year ago
parent
commit
bdf51dcb1d

+ 1 - 0
fixtures/js-stubs/config.tsx

@@ -64,6 +64,7 @@ export function ConfigFixture(params: Partial<Config> = {}): Config {
       organizationUrl: 'https://foobar.sentry.io',
       regionUrl: 'https://us.sentry.io',
     },
+    memberRegions: [{name: 'us', url: 'https://sentry.io'}],
     regions: [{name: 'us', url: 'https://sentry.io'}],
     ...params,
   };

+ 20 - 1
src/sentry/web/client_config.py

@@ -329,7 +329,7 @@ class _ClientConfig:
         """
         The regions available to the current user.
 
-        This will include *all* multi-tenant regions, and if the customer
+        This will include *all* multi-tenant regions, and if the user
         has membership on any single-tenant regions those will also be included.
         """
         user = self.user
@@ -348,6 +348,7 @@ class _ClientConfig:
         if not user or not user.id:
             return [get_region_by_name(name).api_serialize() for name in region_names]
 
+        # TODO(hybridcloud) Have a an RPC for regionmembership for efficiency
         # Ensure all regions the current user is in are included as there
         # could be single tenants or hidden regions
         memberships = user_service.get_organizations(user_id=user.id)
@@ -364,6 +365,23 @@ class _ClientConfig:
         regions.sort(key=region_display_order)
         return [region.api_serialize() for region in regions]
 
+    @property
+    def member_regions(self) -> list[Mapping[str, Any]]:
+        """
+        The regions the user has membership in. Includes single-tenant regions.
+        """
+        user = self.user
+        # If the user is not authenticated they have no region membership
+        if not user or not user.id:
+            return []
+
+        # TODO(hybridcloud) Have a an RPC for regionmembership for efficiency
+        memberships = user_service.get_organizations(user_id=user.id)
+        region_names = {membership.region_name for membership in memberships}
+        regions = [get_region_by_name(name) for name in region_names]
+        regions.sort(key=lambda r: r.name)
+        return [r.api_serialize() for r in regions]
+
     def get_context(self) -> Mapping[str, Any]:
         return {
             "initialTrace": self.tracing_data,
@@ -411,6 +429,7 @@ class _ClientConfig:
                 "allowUrls": self.allow_list,
                 "tracePropagationTargets": settings.SENTRY_FRONTEND_TRACE_PROPAGATION_TARGETS or [],
             },
+            "memberRegions": self.member_regions,
             "regions": self.regions,
             "relocationConfig": {"selectableRegions": options.get("relocation.selectable-regions")},
             "demoMode": settings.DEMO_MODE,

+ 31 - 2
static/app/actionCreators/organizations.spec.tsx

@@ -13,11 +13,40 @@ describe('fetchOrganizations', function () {
     MockApiClient.clearMockResponses();
   });
 
-  it('fetches from multiple regions', async function () {
+  it('has a fallback to regions', async function () {
+    // @ts-ignore-next-line Need to simulate undefined
+    // key that can happen during deploy
+    ConfigStore.set('memberRegions', undefined);
     ConfigStore.set('regions', [
       {name: 'us', url: 'https://us.example.org'},
       {name: 'de', url: 'https://de.example.org'},
     ]);
+    const usMock = MockApiClient.addMockResponse({
+      url: '/organizations/',
+      body: [usorg],
+    });
+    const deMock = MockApiClient.addMockResponse({
+      url: '/organizations/',
+      body: [deorg],
+      match: [
+        function (_url: string, options: Record<string, any>) {
+          return options.host === 'https://de.example.org';
+        },
+      ],
+    });
+    const organizations = await fetchOrganizations(api);
+
+    expect(organizations).toHaveLength(2);
+    expect(organizations[0].slug).toEqual(usorg.slug);
+    expect(usMock).toHaveBeenCalledTimes(1);
+    expect(deMock).toHaveBeenCalledTimes(1);
+  });
+
+  it('fetches from multiple regions', async function () {
+    ConfigStore.set('memberRegions', [
+      {name: 'us', url: 'https://us.example.org'},
+      {name: 'de', url: 'https://de.example.org'},
+    ]);
     const usMock = MockApiClient.addMockResponse({
       url: '/organizations/',
       body: [usorg],
@@ -45,7 +74,7 @@ describe('fetchOrganizations', function () {
   });
 
   it('ignores 401 errors from a region', async function () {
-    ConfigStore.set('regions', [
+    ConfigStore.set('memberRegions', [
       {name: 'us', url: 'https://us.example.org'},
       {name: 'de', url: 'https://de.example.org'},
     ]);

+ 2 - 1
static/app/actionCreators/organizations.tsx

@@ -215,7 +215,8 @@ export async function fetchOrganizationDetails(
  * from /organizations can vary based on query parameters
  */
 export async function fetchOrganizations(api: Client, query?: Record<string, any>) {
-  const regions = ConfigStore.get('regions');
+  // TODO(mark) Remove coalesce after memberRegions
+  const regions = ConfigStore.get('memberRegions') ?? ConfigStore.get('regions');
   const results = await Promise.all(
     regions.map(region =>
       api.requestPromise(`/organizations/`, {

+ 1 - 1
static/app/components/search/sources/apiSource.spec.tsx

@@ -115,7 +115,7 @@ describe('ApiSource', function () {
     const mock = jest.fn().mockReturnValue(null);
     ConfigStore.loadInitialData({
       ...configState,
-      regions: [
+      memberRegions: [
         {name: 'us', url: 'https://us.sentry.io'},
         {name: 'de', url: 'https://de.sentry.io'},
       ],

+ 3 - 1
static/app/types/system.tsx

@@ -158,13 +158,15 @@ export interface Config {
     sentryUrl: string;
     superuserUrl?: string;
   };
+  // A list of regions that the user has membership in.
+  memberRegions: Region[];
   /**
    * This comes from django (django.contrib.messages)
    */
   messages: {level: keyof Theme['alert']; message: string}[];
   needsUpgrade: boolean;
   privacyUrl: string | null;
-  // The list of regions the current user has memberships in.
+  // The list of regions the user has has access to.
   regions: Region[];
   sentryConfig: {
     allowUrls: string[];

+ 46 - 10
tests/sentry/web/test_client_config.py

@@ -1,5 +1,7 @@
 from __future__ import annotations
 
+from typing import Any
+
 import pytest
 from django.conf import settings
 from django.core.cache import cache
@@ -92,17 +94,20 @@ def test_client_config_in_silo_modes(request_factory: RequestFactory):
 
     base_line = dict(get_client_config(request))
 
-    # Removing the region list as it varies based on silo mode.
-    # See Region.to_url()
-    base_line.pop("regions")
-    base_line["links"].pop("regionUrl")
+    def normalize(value: dict[str, Any]):
+        # Removing the region lists as it varies based on silo mode.
+        # See Region.to_url()
+        value.pop("regions")
+        value.pop("memberRegions")
+        value["links"].pop("regionUrl")
+
+    normalize(base_line)
     cache.clear()
 
     for silo_mode in SiloMode:
         with override_settings(SILO_MODE=silo_mode):
             result = dict(get_client_config(request))
-            result.pop("regions")
-            result["links"].pop("regionUrl")
+            normalize(result)
             assert result == base_line
             cache.clear()
 
@@ -132,6 +137,11 @@ def test_client_config_default_region_data():
     assert regions[0]["name"] == settings.SENTRY_MONOLITH_REGION
     assert regions[0]["url"] == options.get("system.url-prefix")
 
+    assert len(result["memberRegions"]) == 1
+    regions = result["memberRegions"]
+    assert regions[0]["name"] == settings.SENTRY_MONOLITH_REGION
+    assert regions[0]["url"] == options.get("system.url-prefix")
+
 
 @no_silo_test
 @django_db_all
@@ -163,11 +173,8 @@ def test_client_config_with_region_data():
     regions = result["regions"]
     assert {r["name"] for r in regions} == {"eu", "us"}
 
+    assert len(result["memberRegions"]) == 1
 
-multiregion_client_config_test = control_silo_test(
-    regions=create_test_regions("us", "eu", "acme", single_tenants=["acme"]),
-    include_monolith_run=True,
-)
 
 hidden_regions = [
     region.Region(
@@ -196,6 +203,31 @@ def test_client_config_with_hidden_region_data():
     assert len(result["regions"]) == 1
     regions = result["regions"]
     assert {r["name"] for r in regions} == {"us"}
+    assert len(result["memberRegions"]) == 1
+
+
+@multiregion_client_config_test
+@django_db_all
+def test_client_config_with_multiple_membership():
+    request, user = make_user_request_from_org()
+    request.user = user
+
+    # multiple us memberships and eu too
+    Factories.create_organization(slug="us-co", owner=user)
+    Factories.create_organization(slug="eu-co", owner=user)
+    mapping = OrganizationMapping.objects.get(slug="eu-co")
+    mapping.update(region_name="eu")
+
+    result = get_client_config(request)
+
+    # Single-tenant doesn't show up unless you have membership
+    assert len(result["regions"]) == 2
+    regions = result["regions"]
+    assert {r["name"] for r in regions} == {"eu", "us"}
+
+    assert len(result["memberRegions"]) == 2
+    regions = result["memberRegions"]
+    assert {r["name"] for r in regions} == {"eu", "us"}
 
 
 @multiregion_client_config_test
@@ -214,6 +246,10 @@ def test_client_config_with_single_tenant_membership():
     regions = result["regions"]
     assert {r["name"] for r in regions} == {"eu", "us", "acme"}
 
+    assert len(result["memberRegions"]) == 2
+    regions = result["memberRegions"]
+    assert {r["name"] for r in regions} == {"us", "acme"}
+
 
 @multiregion_client_config_test
 @django_db_all