Skip to content

Commit 1752f52

Browse files
volokluevvolokluev
authored
fix(rate_limits): Remove special rate limits for group_index (#66730)
These specific rate limits were added 3 years ago before there was a default rate limit pool shared amongst endpoints. At this time we are dealing with inc-674 because it is being spammed by a customer every 3 seconds.
1 parent 3ed1872 commit 1752f52

File tree

4 files changed

+36
-61
lines changed

4 files changed

+36
-61
lines changed

‎src/sentry/issues/endpoints/organization_group_index.py‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
from sentry.search.snuba.backend import assigned_or_suggested_filter
4141
from sentry.search.snuba.executors import get_search_filter
4242
from sentry.snuba import discover
43-
from sentry.types.ratelimit import RateLimit, RateLimitCategory
4443
from sentry.utils.cursors import Cursor, CursorResult
4544
from sentry.utils.validators import normalize_event_id
4645

@@ -148,24 +147,6 @@ class OrganizationGroupIndexEndpoint(OrganizationEndpoint):
148147
permission_classes = (OrganizationEventPermission,)
149148
enforce_rate_limit = True
150149

151-
rate_limits = {
152-
"GET": {
153-
RateLimitCategory.IP: RateLimit(10, 1),
154-
RateLimitCategory.USER: RateLimit(10, 1),
155-
RateLimitCategory.ORGANIZATION: RateLimit(10, 1),
156-
},
157-
"PUT": {
158-
RateLimitCategory.IP: RateLimit(5, 5),
159-
RateLimitCategory.USER: RateLimit(5, 5),
160-
RateLimitCategory.ORGANIZATION: RateLimit(5, 5),
161-
},
162-
"DELETE": {
163-
RateLimitCategory.IP: RateLimit(5, 5),
164-
RateLimitCategory.USER: RateLimit(5, 5),
165-
RateLimitCategory.ORGANIZATION: RateLimit(5, 5),
166-
},
167-
}
168-
169150
def _search(
170151
self, request: Request, organization, projects, environments, extra_query_kwargs=None
171152
):

‎tests/sentry/issues/endpoints/test_organization_group_index.py‎

Lines changed: 1 addition & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,8 @@
44
from uuid import uuid4
55

66
from dateutil.parser import parse as parse_datetime
7-
from django.test import override_settings
87
from django.urls import reverse
98
from django.utils import timezone as django_timezone
10-
from rest_framework import status
119

1210
from sentry import options
1311
from sentry.issues.grouptype import PerformanceNPlusOneGroupType, PerformanceSlowDBQueryGroupType
@@ -48,7 +46,7 @@
4846
from sentry.silo import SiloMode
4947
from sentry.testutils.cases import APITestCase, SnubaTestCase
5048
from sentry.testutils.helpers import parse_link_header
51-
from sentry.testutils.helpers.datetime import before_now, freeze_time, iso_format
49+
from sentry.testutils.helpers.datetime import before_now, iso_format
5250
from sentry.testutils.helpers.features import with_feature
5351
from sentry.testutils.silo import assume_test_silo_mode, region_silo_test
5452
from sentry.types.activity import ActivityType
@@ -1903,14 +1901,6 @@ def test_expand_owners(self):
19031901
)
19041902
assert response.data[0]["owners"][2]["type"] == GROUP_OWNER_TYPE[GroupOwnerType.CODEOWNERS]
19051903

1906-
@override_settings(SENTRY_SELF_HOSTED=False)
1907-
def test_ratelimit(self):
1908-
self.login_as(user=self.user)
1909-
with freeze_time("2000-01-01"):
1910-
for i in range(10):
1911-
self.get_success_response()
1912-
self.get_error_response(status_code=status.HTTP_429_TOO_MANY_REQUESTS)
1913-
19141904
def test_filter_not_unresolved(self):
19151905
event = self.store_event(
19161906
data={"timestamp": iso_format(before_now(seconds=500)), "fingerprint": ["group-1"]},
@@ -3599,14 +3589,6 @@ def test_discard(self):
35993589
assert tombstone.project == group1.project
36003590
assert tombstone.data == group1.data
36013591

3602-
@override_settings(SENTRY_SELF_HOSTED=False)
3603-
def test_ratelimit(self):
3604-
self.login_as(user=self.user)
3605-
with freeze_time("2000-01-01"):
3606-
for i in range(5):
3607-
self.get_success_response()
3608-
self.get_error_response(status_code=status.HTTP_429_TOO_MANY_REQUESTS)
3609-
36103592
def test_set_inbox(self):
36113593
group1 = self.create_group()
36123594
group2 = self.create_group()
@@ -3869,14 +3851,6 @@ def test_bulk_delete(self):
38693851
assert not Group.objects.filter(id=group.id).exists()
38703852
assert not GroupHash.objects.filter(group_id=group.id).exists()
38713853

3872-
@override_settings(SENTRY_SELF_HOSTED=False)
3873-
def test_ratelimit(self):
3874-
self.login_as(user=self.user)
3875-
with freeze_time("2000-01-01"):
3876-
for i in range(5):
3877-
self.get_success_response()
3878-
self.get_error_response(status_code=status.HTTP_429_TOO_MANY_REQUESTS)
3879-
38803854
def test_bulk_delete_performance_issues(self):
38813855
groups = []
38823856
for i in range(10, 41):

‎tests/sentry/middleware/test_ratelimit_middleware.py‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ def test_get_rate_limit_key(self):
197197
request = self.factory.get("/")
198198
assert (
199199
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
200-
== "ip:default:OrganizationGroupIndexEndpoint:GET:127.0.0.1"
200+
== "ip:default:GET:127.0.0.1"
201201
)
202202
# Test when IP address is missing
203203
request.META["REMOTE_ADDR"] = None
@@ -207,15 +207,15 @@ def test_get_rate_limit_key(self):
207207
request.META["REMOTE_ADDR"] = "684D:1111:222:3333:4444:5555:6:77"
208208
assert (
209209
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
210-
== "ip:default:OrganizationGroupIndexEndpoint:GET:684D:1111:222:3333:4444:5555:6:77"
210+
== "ip:default:GET:684D:1111:222:3333:4444:5555:6:77"
211211
)
212212

213213
# Test for users
214214
request.session = {}
215215
request.user = self.user
216216
assert (
217217
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
218-
== f"user:default:OrganizationGroupIndexEndpoint:GET:{self.user.id}"
218+
== f"user:default:GET:{self.user.id}"
219219
)
220220

221221
# Test for user auth tokens
@@ -224,21 +224,21 @@ def test_get_rate_limit_key(self):
224224
request.user = self.user
225225
assert (
226226
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
227-
== f"user:default:OrganizationGroupIndexEndpoint:GET:{self.user.id}"
227+
== f"user:default:GET:{self.user.id}"
228228
)
229229

230230
# Test for sentryapp auth tokens:
231231
self.populate_sentry_app_request(request)
232232
assert (
233233
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
234-
== f"org:default:OrganizationGroupIndexEndpoint:GET:{self.organization.id}"
234+
== f"org:default:GET:{self.organization.id}"
235235
)
236236

237237
self.populate_internal_integration_request(request)
238238
# Fallback to IP address limit if we can't find the organization
239239
assert (
240240
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
241-
== "ip:default:OrganizationGroupIndexEndpoint:GET:684D:1111:222:3333:4444:5555:6:77"
241+
== "ip:default:GET:684D:1111:222:3333:4444:5555:6:77"
242242
)
243243

244244
# Test for
@@ -251,7 +251,7 @@ def test_get_rate_limit_key(self):
251251
request.auth = api_key
252252
assert (
253253
get_rate_limit_key(view, request, rate_limit_group, rate_limit_config)
254-
== "ip:default:OrganizationGroupIndexEndpoint:GET:684D:1111:222:3333:4444:5555:6:77"
254+
== "ip:default:GET:684D:1111:222:3333:4444:5555:6:77"
255255
)
256256

257257
def test_enforce_rate_limit_is_false(self):

‎tests/sentry/ratelimits/utils/test_get_ratelimit_key.py‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,43 @@
11
from django.contrib.sessions.backends.base import SessionBase
22
from django.test import RequestFactory
33
from rest_framework.permissions import AllowAny
4+
from rest_framework.response import Response
45

56
from sentry.api.base import Endpoint
67
from sentry.auth.system import SystemToken
7-
from sentry.issues.endpoints.organization_group_index import OrganizationGroupIndexEndpoint
88
from sentry.models.apitoken import ApiToken
99
from sentry.models.user import User
1010
from sentry.ratelimits import get_rate_limit_config, get_rate_limit_key
1111
from sentry.ratelimits.config import RateLimitConfig
1212
from sentry.services.hybrid_cloud.auth import AuthenticatedToken
1313
from sentry.testutils.cases import TestCase
1414
from sentry.testutils.silo import assume_test_silo_mode_of, region_silo_test
15+
from sentry.types.ratelimit import RateLimit, RateLimitCategory
16+
17+
CONCURRENT_RATE_LIMIT = 20
18+
19+
20+
class APITestEndpoint(Endpoint):
21+
permission_classes = (AllowAny,)
22+
enforce_rate_limit = True
23+
rate_limits = RateLimitConfig(
24+
limit_overrides={
25+
"GET": {
26+
RateLimitCategory.IP: RateLimit(20, 1, CONCURRENT_RATE_LIMIT),
27+
RateLimitCategory.USER: RateLimit(20, 1, CONCURRENT_RATE_LIMIT),
28+
RateLimitCategory.ORGANIZATION: RateLimit(20, 1, CONCURRENT_RATE_LIMIT),
29+
},
30+
},
31+
)
32+
33+
def get(self, request):
34+
return Response({"ok": True})
1535

1636

1737
@region_silo_test
1838
class GetRateLimitKeyTest(TestCase):
1939
def setUp(self) -> None:
20-
self.view = OrganizationGroupIndexEndpoint.as_view()
40+
self.view = APITestEndpoint.as_view()
2141
self.request = RequestFactory().get("/")
2242
self.rate_limit_config = get_rate_limit_config(self.view.view_class)
2343
self.rate_limit_group = (
@@ -29,7 +49,7 @@ def test_default_ip(self):
2949
get_rate_limit_key(
3050
self.view, self.request, self.rate_limit_group, self.rate_limit_config
3151
)
32-
== "ip:default:OrganizationGroupIndexEndpoint:GET:127.0.0.1"
52+
== "ip:default:APITestEndpoint:GET:127.0.0.1"
3353
)
3454

3555
def test_ip_address_missing(self):
@@ -47,7 +67,7 @@ def test_ipv6(self):
4767
get_rate_limit_key(
4868
self.view, self.request, self.rate_limit_group, self.rate_limit_config
4969
)
50-
== "ip:default:OrganizationGroupIndexEndpoint:GET:684D:1111:222:3333:4444:5555:6:77"
70+
== "ip:default:APITestEndpoint:GET:684D:1111:222:3333:4444:5555:6:77"
5171
)
5272

5373
def test_system_token(self):
@@ -67,7 +87,7 @@ def test_users(self):
6787
get_rate_limit_key(
6888
self.view, self.request, self.rate_limit_group, self.rate_limit_config
6989
)
70-
== f"user:default:OrganizationGroupIndexEndpoint:GET:{user.id}"
90+
== f"user:default:APITestEndpoint:GET:{user.id}"
7191
)
7292

7393
def test_organization(self):
@@ -87,7 +107,7 @@ def test_organization(self):
87107
get_rate_limit_key(
88108
self.view, self.request, self.rate_limit_group, self.rate_limit_config
89109
)
90-
== f"org:default:OrganizationGroupIndexEndpoint:GET:{install.organization_id}"
110+
== f"org:default:APITestEndpoint:GET:{install.organization_id}"
91111
)
92112

93113
def test_api_token(self):
@@ -99,7 +119,7 @@ def test_api_token(self):
99119
get_rate_limit_key(
100120
self.view, self.request, self.rate_limit_group, self.rate_limit_config
101121
)
102-
== f"user:default:OrganizationGroupIndexEndpoint:GET:{self.user.id}"
122+
== f"user:default:APITestEndpoint:GET:{self.user.id}"
103123
)
104124

105125
def test_authenticated_token(self):
@@ -114,7 +134,7 @@ def test_authenticated_token(self):
114134
get_rate_limit_key(
115135
self.view, self.request, self.rate_limit_group, self.rate_limit_config
116136
)
117-
== f"user:default:OrganizationGroupIndexEndpoint:GET:{self.user.id}"
137+
== f"user:default:APITestEndpoint:GET:{self.user.id}"
118138
)
119139

120140

0 commit comments

Comments
 (0)