Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: improve traffic filtering at nginx #1359

Merged
merged 16 commits into from
Apr 20, 2023
Merged

Feat: improve traffic filtering at nginx #1359

merged 16 commits into from
Apr 20, 2023

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Apr 14, 2023 •
edited
Loading

As part of reducing the unnecessary traffic we send to Sentry (see Slack thread for more context) and Amplitude (see e.g. #1285 for some prior work), this PR adds filtering at the nginx layer - before traffic even hits the Django app where Sentry, Amplitude, etc. come into play.

Based on a quick analysis of both Amplitude and Sentry, immediately 404 known scraping targets like *.php and /api/whatever.

I also took the opportunity to close #502 by moving rate limiting into nginx.

Notes for reviewers

  • Reminder: only the appcontainer runs nginx, the devcontainer uses a local dev server
  • New cypress tests for the scraper targets, or trigger 404 manually in the browser
  • Rate limit still only applies to POST on /eligibility/confirm, the cypress tests have been updated

@github-actions github-actions bot added documentation [auto] Improvements or additions to documentation docker Application container, devcontainer, Compose, etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. tests Related to automated testing (unit, UI, integration, etc.) and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Apr 14, 2023
@thekaveman thekaveman added the deployment-dev [auto] Changes that will trigger a deploy if merged to dev label Apr 14, 2023
@thekaveman thekaveman force-pushed the feat/nginx-filters branch 2 times, most recently from 6642af0 to a1586a2 Compare April 14, 2023 17:25
@thekaveman thekaveman marked this pull request as ready for review April 14, 2023 17:50
@thekaveman thekaveman requested a review from a team as a code owner April 14, 2023 17:50
@thekaveman thekaveman added this to the Reliability milestone Apr 14, 2023
@thekaveman thekaveman mentioned this pull request Apr 14, 2023
Merged
@angela-tran
Copy link
Member

👀 Finished code reviewing this. Still need to do manual testing

Notes for reviewers

  • Reminder: only the appcontainer runs nginx, the devcontainer uses a local dev server
  • New cypress tests for the scraper targets, or trigger 404 manually in the browser
  • Rate limit still only applies to POST on /eligibility/confirm, the cypress tests have been updated

@thekaveman
Copy link
Member Author

Sorry I realized that there was a bug in one of the regexes, it wouldn't have matched /.env (the filename is just the extension) - fixed that.

Then I also wanted to clarify in the tests that the 404 actually does come from nginx and not Django.

@angela-tran
Copy link
Member

I was able to manually verify that incorrect paths like /.env are given a 404 response by nginx.

Also manually verified that the rate-limiting from nginx works:
image

@thekaveman
Copy link
Member Author

🤔 for the rate limit (just like the 404) we shouldn't be seeing a Benefits error page. The rate limit happens before the request gets to Django, and returns a 503. It should be an nginx error page?

@thekaveman
Copy link
Member Author

I'm going to rebase on dev to clean up merge conflicts

thekaveman added 11 commits April 18, 2023 12:29
@thekaveman
chore(nginx): add conf file from base image
9ecf172 
cal-itp/docker-python-web defines the default nginx.conf

take a copy so we can override for benefits
@thekaveman
feat(nginx): 404 known scraping targets
4f02d5d 
based on data from amplitude and sentry
@thekaveman
refactor: move rate limit from Django to nginx
c3b60ba
@thekaveman
fix(nginx): selective rate-limit for POST
8737676
@thekaveman
refactor(nginx): include common directives
e70b0c5
@thekaveman
docs(rate-limit): update with nginx config
ca0731c
@thekaveman
fix(tests): mock settings on middleware module
53fd701 
settings was removed from the session module with rate-limit refactor

this test shouldn't use the session module anyway, as it is testing
a middleware
@thekaveman
chore(terraform): remove unused rate limit env vars
78a77c7
@thekaveman
refactor(tests): use helper for CC setup
6fb30ac
@thekaveman
refactor(tests): rate limit wait helper
2ddaaf3 
POST to /eligibility/confirm needs to respect rate limit
@thekaveman
test(nginx): assert 404 on known scraping targets
8c0a3b5
@angela-tran
Copy link
Member

🤔 for the rate limit (just like the 404) we shouldn't be seeing a Benefits error page. The rate limit happens before the request gets to Django, and returns a 503. It should be an nginx error page?

Oh hmm. So I should be seeing an nginx error page for both the 404 and 503? I was seeing Benefits error pages.

@thekaveman
Copy link
Member Author

thekaveman commented Apr 18, 2023 •
edited
Loading

🤔 for the rate limit (just like the 404) we shouldn't be seeing a Benefits error page. The rate limit happens before the request gets to Django, and returns a 503. It should be an nginx error page?

Oh hmm. So I should be seeing an nginx error page for both the 404 and 503? I was seeing Benefits error pages.

Exactly. See the modification to the scrapers.cy.js in this commit: 1f33670 to ensure nginx is throwing the 404, not Django.

I'm going to modify the rate-limit.cy.js as well.

angela-tran
angela-tran reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into some issues while trying out the Cypress tests. I don't think they necessarily point out problems with the actual code refactor, but rather are bugs in the tests / local environment issues.

tests/cypress/specs/rate-limit.cy.js Show resolved Hide resolved
tests/cypress/specs/scrapers.cy.js Show resolved Hide resolved
@thekaveman thekaveman requested a review from angela-tran April 18, 2023 20:40
@thekaveman
Copy link
Member Author

OK this is the runtime I expected to see: https://github.com/cal-itp/benefits/actions/runs/4736775820/jobs/8408740937

image

angela-tran
angela-tran approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran through the tests again on rebased commits. Looks good!

@thekaveman thekaveman merged commit cfeda54 into dev Apr 20, 2023
@thekaveman thekaveman deleted the feat/nginx-filters branch April 20, 2023 21:44
@thekaveman thekaveman mentioned this pull request Apr 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angela-tran angela-tran angela-tran approved these changes

Assignees
No one assigned
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev docker Application container, devcontainer, Compose, etc. documentation [auto] Improvements or additions to documentation tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Milestone
Reliability [eng]
Development

Successfully merging this pull request may close these issues.

Offload rate limiting
2 participants
@thekaveman @angela-tran