dev: Add Rate limits for rest endpoints #735

ajay-sentry · 2024-08-05T18:26:10Z

Purpose/Motivation

This PR aims to update our DRF (Django Rest Framework) configuration to also include a basic ratelimiter. Rudimentarily, it uses the default "anon" and "user" throttle classes that DRF gives us out the box and sets the request rate to 30/min for anon requests and 90 for user requests. Additionally, we have a "sustained" rate limit of 1000/day for anons and 2000/day for users

Testing it locally with 10x reduction in the above values and it seems to work pretty well. We should be able to track all the instances of 429s coming to sentry and create charts / graphs as well. Maybe even alerts too if there's a way to capture the IP.

It should also be noted that the rate limit currently is shared across ALL endpoints. so the user gets a "90 limit" each minute shared between every call of account-details / users / etc.

Related notion doc

Reference

Hooked up with Gazebo

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

codecov-notifications · 2024-08-12T19:16:54Z

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
codecov/rate_limiter.py	0.00%	9 Missing ⚠️
codecov/settings_staging.py	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

ajay-sentry · 2024-08-12T19:21:51Z

codecov/settings_staging.py

@@ -73,3 +73,18 @@
 CSRF_TRUSTED_ORIGINS = [
    get_config("setup", "trusted_origin", default="https://*.codecov.dev")
 ]
+
+REST_FRAMEWORK = {


Dont want to add rate limits to staging

codecov-qa · 2024-08-15T17:55:14Z

❌ 57 Tests Failed:

Tests completed	Failed	Passed	Skipped
2262	57	2205	6

View the top 3 failed tests by shortest run time

profiling.tests.test_views�test_creating_profiling_commit_already_exist

Stack Traces | 0.038s run time

db = None, mocker = <pytest_mock.plugin.MockerFixture object at 0x7f95ac00c680>

    def test_creating_profiling_commit_already_exist(db, mocker):
        repo = RepositoryFactory.create(active=True)
        token = RepositoryTokenFactory.create(repository=repo, token_type="profiling")
        client = APIClient()
        pc = ProfilingCommit.objects.create(
            repository=repo,
            environment="production",
            version_identifier="v1.0.9",
            code="productionv1.0.9",
        )
        assert ProfilingCommit.objects.filter(
            repository=repo, environment="production", version_identifier="v1.0.9"
        ).exists()
        url = reverse("create_profiling_version")
        client.credentials(HTTP_AUTHORIZATION="repotoken " + token.key)
>       response = client.post(
            url,
            {
                "environment": "production",
                "version_identifier": "v1.0.9",
                "code": "productionv1.0.9",
            },
            format="json",
        )

profiling/tests/test_views.py:131: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:295: in post
    response = super().post(
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:209: in post
    return self.generic('POST', path, data, content_type, **extra)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:233: in generic
    return super().generic(
.../local/lib/python3.12.../django/test/client.py:609: in generic
    return self.request(**r)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:285: in request
    return super().request(**kwargs)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:237: in request
    request = super().request(**kwargs)
.../local/lib/python3.12.../django/test/client.py:891: in request
    self.check_exception(response)
.../local/lib/python3.12.../django/test/client.py:738: in check_exception
    raise exc_value
.../local/lib/python3.12.../core/handlers/exception.py:55: in inner
    response = get_response(request)
.../local/lib/python3.12.../core/handlers/base.py:197: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
.../local/lib/python3.12.../integrations/django/views.py:84: in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
.../local/lib/python3.12.../views/decorators/csrf.py:56: in wrapper_view
    return view_func(*args, **kwargs)
.../local/lib/python3.12.../views/generic/base.py:104: in view
    return self.dispatch(request, *args, **kwargs)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:509: in dispatch
    response = self.handle_exception(exc)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:469: in handle_exception
    self.raise_uncaught_exception(exc)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:480: in raise_uncaught_exception
    raise exc
.../local/lib/python3.12................../site-packages/rest_framework/views.py:497: in dispatch
    self.initial(request, *args, **kwargs)
.../local/lib/python3.12.../integrations/django/__init__.py:312: in sentry_patched_drf_initial
    return old_drf_initial(self, request, *args, **kwargs)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:416: in initial
    self.check_throttles(request)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:359: in check_throttles
    if not throttle.allow_request(request, self):
.../local/lib/python3.12....../site-packages/rest_framework/throttling.py:119: in allow_request
    self.key = self.get_cache_key(request, view)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <codecov.rate_limiter.UserBurstRateThrottle object at 0x7f95a8ee9670>
request = <rest_framework.request.Request: POST '/profiling/versions'>
view = <profiling.views.ProfilingCommitCreateView object at 0x7f95abb1fb90>

    def get_cache_key(self, request, view):
        if request.user and request.user.is_authenticated:
>           ident = request.user.pk
E           AttributeError: 'RepositoryAsUser' object has no attribute 'pk'

.../local/lib/python3.12....../site-packages/rest_framework/throttling.py:195: AttributeError

profiling.tests.test_views�test_creating_profiling_commit_does_not_exist

Stack Traces | 0.038s run time

db = None, mocker = <pytest_mock.plugin.MockerFixture object at 0x7f95a82398e0>

    def test_creating_profiling_commit_does_not_exist(db, mocker):
        repo = RepositoryFactory.create(active=True)
        token = RepositoryTokenFactory.create(repository=repo, token_type="profiling")
        client = APIClient()
        assert not ProfilingCommit.objects.filter(
            repository=repo, code="productionv1.0.9"
        ).exists()
        url = reverse("create_profiling_version")
        client.credentials(HTTP_AUTHORIZATION="repotoken " + token.key)
>       response = client.post(
            url,
            {
                "environment": "production",
                "version_identifier": "v1.0.9",
                "code": "productionv1.0.9",
            },
            format="json",
        )

profiling/tests/test_views.py:85: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:295: in post
    response = super().post(
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:209: in post
    return self.generic('POST', path, data, content_type, **extra)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:233: in generic
    return super().generic(
.../local/lib/python3.12.../django/test/client.py:609: in generic
    return self.request(**r)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:285: in request
    return super().request(**kwargs)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:237: in request
    request = super().request(**kwargs)
.../local/lib/python3.12.../django/test/client.py:891: in request
    self.check_exception(response)
.../local/lib/python3.12.../django/test/client.py:738: in check_exception
    raise exc_value
.../local/lib/python3.12.../core/handlers/exception.py:55: in inner
    response = get_response(request)
.../local/lib/python3.12.../core/handlers/base.py:197: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
.../local/lib/python3.12.../integrations/django/views.py:84: in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
.../local/lib/python3.12.../views/decorators/csrf.py:56: in wrapper_view
    return view_func(*args, **kwargs)
.../local/lib/python3.12.../views/generic/base.py:104: in view
    return self.dispatch(request, *args, **kwargs)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:509: in dispatch
    response = self.handle_exception(exc)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:469: in handle_exception
    self.raise_uncaught_exception(exc)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:480: in raise_uncaught_exception
    raise exc
.../local/lib/python3.12................../site-packages/rest_framework/views.py:497: in dispatch
    self.initial(request, *args, **kwargs)
.../local/lib/python3.12.../integrations/django/__init__.py:312: in sentry_patched_drf_initial
    return old_drf_initial(self, request, *args, **kwargs)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:416: in initial
    self.check_throttles(request)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:359: in check_throttles
    if not throttle.allow_request(request, self):
.../local/lib/python3.12....../site-packages/rest_framework/throttling.py:119: in allow_request
    self.key = self.get_cache_key(request, view)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <codecov.rate_limiter.UserBurstRateThrottle object at 0x7f95ab6be150>
request = <rest_framework.request.Request: POST '/profiling/versions'>
view = <profiling.views.ProfilingCommitCreateView object at 0x7f95abb1d3a0>

    def get_cache_key(self, request, view):
        if request.user and request.user.is_authenticated:
>           ident = request.user.pk
E           AttributeError: 'RepositoryAsUser' object has no attribute 'pk'

.../local/lib/python3.12....../site-packages/rest_framework/throttling.py:195: AttributeError

upload.tests.views.test_commits�test_commit_github_oidc_auth

Stack Traces | 0.039s run time

mock_jwks_client = <MagicMock name='PyJWKClient' id='140280619443184'>
mock_jwt_decode = <MagicMock name='decode' id='140280626518048'>, db = None
mocker = <pytest_mock.plugin.MockerFixture object at 0x7f95a07db530>

    @patch("upload.helpers.jwt.decode")
    @patch("upload.helpers.PyJWKClient")
    def test_commit_github_oidc_auth(mock_jwks_client, mock_jwt_decode, db, mocker):
        repository = RepositoryFactory.create(
            private=False, author__username="codecov", name="the_repo"
        )
        mocked_call = mocker.patch.object(TaskService, "update_commit")
        mock_sentry_metrics = mocker.patch("upload.views.commits.sentry_metrics.incr")
        mock_jwt_decode.return_value = {
            "repository": f"url/{repository.name}",
            "repository_owner": repository.author.username,
            "iss": "https://token.actions.githubusercontent.com",
        }
        token = "ThisValueDoesNotMatterBecauseOf_mock_jwt_decode"
    
        client = APIClient()
        repo_slug = f"{repository.author.username}::::{repository.name}"
        url = reverse(
            "new_upload.commits",
            args=[repository.author.service, repo_slug],
        )
>       response = client.post(
            url,
            {
                "commitid": "commit_sha",
                "pullid": "4",
            },
            format="json",
            headers={"Authorization": f"token {token}", "User-Agent": "codecov-cli/0.4.7"},
        )

.../tests/views/test_commits.py:279: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:295: in post
    response = super().post(
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:209: in post
    return self.generic('POST', path, data, content_type, **extra)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:233: in generic
    return super().generic(
.../local/lib/python3.12.../django/test/client.py:609: in generic
    return self.request(**r)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:285: in request
    return super().request(**kwargs)
.../local/lib/python3.12.............../site-packages/rest_framework/test.py:237: in request
    request = super().request(**kwargs)
.../local/lib/python3.12.../django/test/client.py:891: in request
    self.check_exception(response)
.../local/lib/python3.12.../django/test/client.py:738: in check_exception
    raise exc_value
.../local/lib/python3.12.../core/handlers/exception.py:55: in inner
    response = get_response(request)
.../local/lib/python3.12.../core/handlers/base.py:197: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
.../local/lib/python3.12.../integrations/django/views.py:84: in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
.../local/lib/python3.12.../views/decorators/csrf.py:56: in wrapper_view
    return view_func(*args, **kwargs)
.../local/lib/python3.12.../views/generic/base.py:104: in view
    return self.dispatch(request, *args, **kwargs)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:509: in dispatch
    response = self.handle_exception(exc)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:469: in handle_exception
    self.raise_uncaught_exception(exc)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:480: in raise_uncaught_exception
    raise exc
.../local/lib/python3.12................../site-packages/rest_framework/views.py:497: in dispatch
    self.initial(request, *args, **kwargs)
.../local/lib/python3.12.../integrations/django/__init__.py:312: in sentry_patched_drf_initial
    return old_drf_initial(self, request, *args, **kwargs)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:416: in initial
    self.check_throttles(request)
.../local/lib/python3.12................../site-packages/rest_framework/views.py:359: in check_throttles
    if not throttle.allow_request(request, self):
.../local/lib/python3.12....../site-packages/rest_framework/throttling.py:119: in allow_request
    self.key = self.get_cache_key(request, view)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <codecov.rate_limiter.UserBurstRateThrottle object at 0x7f95a0b5e4e0>
request = <rest_framework.request.Request: POST '.../upload/github/codecov::::the_repo/commits'>
view = <upload.views.commits.CommitViews object at 0x7f95a0b5c3e0>

    def get_cache_key(self, request, view):
        if request.user and request.user.is_authenticated:
>           ident = request.user.pk
E           AttributeError: 'RepositoryAsUser' object has no attribute 'pk'

.../local/lib/python3.12....../site-packages/rest_framework/throttling.py:195: AttributeError

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

codecov · 2024-08-15T18:01:55Z

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Project coverage is 96.08%. Comparing base (1c2b6a8) to head (dc597b4).
Report is 8 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
codecov/rate_limiter.py	0.00%	9 Missing ⚠️
codecov/settings_staging.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##               main       #735        +/-   ##
================================================
+ Coverage   96.05000   96.08000   +0.03000     
================================================
  Files           814        815         +1     
  Lines         18489      18754       +265     
================================================
+ Hits          17760      18020       +260     
- Misses          729        734         +5

Flag	Coverage Δ
unit	`91.86% <47.36%> (+0.08%)`	⬆️
unit-latest-uploader	`91.86% <47.36%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This reverts commit 43f1a58.

ajay-sentry added 5 commits August 5, 2024 11:23

add rate limiter for REST endpoint

c9110e3

Merge branch 'main' into Ajay/rest-framework-rate-limiter

463de3f

add rate limits

da137cf

move to prod

aebbdf0

add back to base, remove from prod, and make staging have no limit

7549ff0

ajay-sentry commented Aug 12, 2024

View reviewed changes

update test settings throttle rates

b4fdef4

ajay-sentry added 6 commits August 15, 2024 11:48

get rid of sustained

f172bcf

remove throttle altogether

b30c630

update ini test file to use test settings

93d70e6

update test runner stuff

196279a

enable to 9000 by default

c35db88

revert, too much work to add pk to all user objects

dc597b4

rohitvinnakota-codecov approved these changes Aug 16, 2024

View reviewed changes

ajay-sentry added this pull request to the merge queue Aug 16, 2024

Merged via the queue into main with commit 43f1a58 Aug 16, 2024
16 of 18 checks passed

ajay-sentry deleted the Ajay/rest-framework-rate-limiter branch August 16, 2024 15:57

ajay-sentry added a commit that referenced this pull request Aug 17, 2024

Revert "dev: Add Rate limits for rest endpoints (#735)"

1584931

This reverts commit 43f1a58.

ajay-sentry mentioned this pull request Aug 17, 2024

Revert "dev: Add Rate limits for rest endpoints" #760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev: Add Rate limits for rest endpoints #735

dev: Add Rate limits for rest endpoints #735

ajay-sentry commented Aug 5, 2024 •

edited

Loading

codecov-notifications bot commented Aug 12, 2024 •

edited

Loading

ajay-sentry Aug 12, 2024

codecov-qa bot commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading

dev: Add Rate limits for rest endpoints #735

dev: Add Rate limits for rest endpoints #735

Conversation

ajay-sentry commented Aug 5, 2024 • edited Loading

Purpose/Motivation

Legal Boilerplate

codecov-notifications bot commented Aug 12, 2024 • edited Loading

Codecov Report

ajay-sentry Aug 12, 2024

Choose a reason for hiding this comment

codecov-qa bot commented Aug 15, 2024 • edited Loading

❌ 57 Tests Failed:

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

ajay-sentry commented Aug 5, 2024 •

edited

Loading

codecov-notifications bot commented Aug 12, 2024 •

edited

Loading

codecov-qa bot commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading