Skip to content

Commit

Permalink
fix(hybridcloud) Improve API gateway routing for error-embed (#69815)
Browse files Browse the repository at this point in the history
The browser SDK is still creating URLs for the error-embed views using
`sentry.io` for customers in all regions. Remove the error-embed view
from the region pin list and create DSN based routing. I don't love this
solution, but it should be robust enough to handle the scenarios that
customers are having problems with.

Refs HC-1182
Refs getsentry/sentry-javascript#11813
  • Loading branch information
markstory committed Apr 29, 2024
1 parent b024a7e commit cdb21c0
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 3 deletions.
1 change: 0 additions & 1 deletion src/sentry/conf/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -3965,7 +3965,6 @@ def build_cdc_postgres_init_db_volume(settings: Any) -> dict[str, dict[str, str]
"sentry-api-0-relays-details",
# Backwards compatibility for US customers.
# New usage of these is region scoped.
"sentry-error-page-embed",
"sentry-js-sdk-loader",
"sentry-release-hook",
"sentry-api-0-organizations",
Expand Down
13 changes: 13 additions & 0 deletions src/sentry/hybridcloud/apigateway/apigateway.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from rest_framework.request import Request

from sentry.hybridcloud.apigateway.proxy import (
proxy_error_embed_request,
proxy_region_request,
proxy_request,
proxy_sentryapp_request,
Expand Down Expand Up @@ -105,6 +106,18 @@ def proxy_request_if_needed(
)
return proxy_sentryapp_request(request, app_id_or_slug, url_name)

if url_name == "sentry-error-page-embed" and "dsn" in request.GET:
# Error embed modal is special as customers can't easily use region URLs.
dsn = request.GET["dsn"]
metrics.incr(
"apigateway.proxy_request",
tags={
"url_name": url_name,
"kind": "error-embed",
},
)
return proxy_error_embed_request(request, dsn, url_name)

if (
request.resolver_match
and request.resolver_match.url_name in settings.REGION_PINNED_URL_NAMES
Expand Down
34 changes: 33 additions & 1 deletion src/sentry/hybridcloud/apigateway/proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import logging
from collections.abc import Iterator
from urllib.parse import urljoin
from urllib.parse import urljoin, urlparse
from wsgiref.util import is_hop_by_hop

from django.conf import settings
Expand All @@ -16,6 +16,7 @@
from requests import request as external_request
from requests.exceptions import Timeout

from sentry import options
from sentry.api.exceptions import RequestTimeout
from sentry.models.integrations.sentry_app import SentryApp
from sentry.models.integrations.sentry_app_installation import SentryAppInstallation
Expand Down Expand Up @@ -149,6 +150,37 @@ def proxy_sentryapp_request(
return proxy_region_request(request, region, url_name)


def proxy_error_embed_request(
request: HttpRequest, dsn: str, url_name: str
) -> HttpResponseBase | None:
try:
parsed = urlparse(dsn)
except Exception as err:
logger.info("apigateway.error_embed.invalid_dsn", extra={"dsn": dsn, "error": err})
return None
host = parsed.netloc
app_host = urlparse(options.get("system.url-prefix")).netloc
if not host.endswith(app_host):
# Don't further parse URLs that aren't for us.
return None

app_segments = app_host.split(".")
host_segments = host.split(".")
if len(host_segments) - len(app_segments) < 3:
# If we don't have a o123.ingest.{region}.{app_host} style domain
# we forward to the monolith region
region = get_region_by_name(settings.SENTRY_MONOLITH_REGION)
return proxy_region_request(request, region, url_name)
try:
region_offset = len(app_segments) + 1
region_segment = host_segments[region_offset * -1]
region = get_region_by_name(region_segment)
except Exception:
return None

return proxy_region_request(request, region, url_name)


def proxy_region_request(
request: HttpRequest, region: Region, url_name: str
) -> StreamingHttpResponse:
Expand Down
5 changes: 5 additions & 0 deletions src/sentry/testutils/helpers/apigateway.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ def get(self, request):
RegionEndpoint.as_view(),
name="region-endpoint-id-or-slug",
),
re_path(
r"^api/embed/error-page/$",
RegionEndpoint.as_view(),
name="sentry-error-page-embed",
),
] + api_urls.urlpatterns


Expand Down
54 changes: 53 additions & 1 deletion tests/sentry/hybridcloud/apigateway/test_apigateway.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
from urllib.parse import urlencode

import pytest
import responses
from django.conf import settings
from django.test import override_settings
from django.urls import get_resolver, reverse
from rest_framework.response import Response

from sentry.silo.base import SiloMode
from sentry.silo.base import SiloLimit, SiloMode
from sentry.testutils.helpers.apigateway import ApiGatewayTestCase, verify_request_params
from sentry.testutils.helpers.options import override_options
from sentry.testutils.helpers.response import close_streaming_response
Expand Down Expand Up @@ -261,6 +262,57 @@ def test_proxy_check_region_pinned_issue_urls(self):
assert resp_json["proxy"] is True
assert resp_json["events"]

@responses.activate
def test_proxy_error_embed_dsn(self):
responses.add(
responses.GET,
f"{self.REGION.address}/api/embed/error-page/",
json={"proxy": True, "name": "error-embed"},
)
with override_settings(SILO_MODE=SiloMode.CONTROL, MIDDLEWARE=tuple(self.middleware)):
# no dsn
with pytest.raises(SiloLimit.AvailabilityError):
self.client.get("/api/embed/error-page/")

# invalid dsn
with pytest.raises(SiloLimit.AvailabilityError):
self.client.get("/api/embed/error-page/", data={"dsn": "lolnope"})

# invalid DSN that doesn't match our domain
with pytest.raises(SiloLimit.AvailabilityError):
self.client.get(
"/api/embed/error-page/", data={"dsn": "https://abc123@nope.com/123"}
)

# Older DSN with no region -> monolith region
resp = self.client.get(
"/api/embed/error-page/", data={"dsn": "https://abc123@testserver/123"}
)
assert resp.status_code == 200
self._check_response(resp, "error-embed")

# DSN with o123.ingest.sentry.io style hosts
resp = self.client.get(
"/api/embed/error-page/", data={"dsn": "https://abc123@o123.ingest.testserver/123"}
)
assert resp.status_code == 200
self._check_response(resp, "error-embed")

# DSN with o123.ingest.us.sentry.io style hosts
resp = self.client.get(
"/api/embed/error-page/",
data={"dsn": "https://abc123@o123.ingest.us.testserver/123"},
)
assert resp.status_code == 200
self._check_response(resp, "error-embed")

# DSN with o123.ingest.us.sentry.io style hosts with a garbage region
with pytest.raises(SiloLimit.AvailabilityError):
self.client.get(
"/api/embed/error-page/",
data={"dsn": "https://abc123@o123.ingest.zz.testserver/123"},
)

@staticmethod
def _check_response(resp: Response, expected_name: str) -> None:
if SiloMode.get_current_mode() == SiloMode.MONOLITH:
Expand Down

0 comments on commit cdb21c0

Please sign in to comment.