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.

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

Already on GitHub? Sign in to your account

Add generic rate limiter middleware #1004

Merged
merged 4 commits into from Oct 29, 2020

Conversation

raymondjacobson
Copy link
Member

@raymondjacobson raymondjacobson commented Oct 29, 2020

Trello Card Link

Description

Adds a generic rate limiter to both node services (creator + identity).

The config for the rate limits are per endpoint, per method, per expiry time and are defined as a stringified JSON object in the config.

Services

  • Creator Node
  • Identity Service

Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?

Delete an option.

  • 馃毃 Yes, this touches all API endpoints (middleware). Usually a no-op.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Include log analysis if applicable.

  1. Ran entire system locally and as I added each rate limiter, I verified that I could exercise it with the following snippet:
let url = 'http://localhost:4000/vector_clock_backfill/abc'; let s = async () => { for (let i = 0; i < 40; ++i) { const res = await fetch(url, { method: 'POST' }); console.log(res.status); } }; s()
  1. Ran dapp against the local setup, created account, uploaded tracks, changed profile info

Default config (very permissive):

creator = {
	"/image_upload":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/users":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/users/login":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/users/login/challenge":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/users/logout":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/users/batch_clock_status":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/track_content":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/tracks/metadata":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/tracks":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/audius_users/metadata":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/audius_users":  { "post": [ { "expiry": 60, "max": 100 } ] },
	"/sync":  { "post": [ { "expiry": 60, "max": 20 } ] },
	"/vector_clock_sync":  { "post": [ { "expiry": 60, "max": 20 } ] },
}

identity = {
	"/artist_pick": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/user": { "post": [ { "expiry": 60, "max": 100 } ] },
	"/users/update": { "post": [ { "expiry": 60, "max": 100 } ] },
	"/user_playlist_favorites": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/recovery": { "post": [ { "expiry": 60, "max": 5 } ] },
	"/email/welcome": { "post": [ { "expiry": 60, "max": 5 } ] },
	"/social_handles": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/push_notifications/settings": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/push_notifications/device_token": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/push_notifications/device_token/deregister": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/push_notifications/browser/settings": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/push_notifications/browser/register": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/push_notifications/browser/deregister": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/userEvents": { "post": [ { "expiry": 60, "max": 100 } ] },
	"/twitter": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/twitter/callback": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/twitter/associate": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/instagram": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/instagram/associate": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/notifications": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/notifications/all": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/notifications/clear_badges": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/notifications/settings": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/announcements": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/notifications/subscription": { "post": [ { "expiry": 60, "max": 40 } ] },
	"/authentication": { "post": [ { "expiry": 60, "max": 40 } ] },
}

console.log(JSON.stringify(JSON.stringify(creator)))
console.log('\n\n\n\n')
console.log(JSON.stringify(JSON.stringify(identity)))

Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

i like this a lot

identity-service/default-config.json Show resolved Hide resolved
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, let's wait for @SidSethi ack for cnode

* Combines and applies middlewares in an array
* @param {Array} middleware
*/
const compose = (middleware) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

(low pri / non blocking) nit - compose what? seems like composeMiddleware?

Copy link
Member Author

Choose a reason for hiding this comment

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

will update name :)

@SidSethi
Copy link
Contributor

@dmanjunath
Copy link
Contributor

@raymondjacobson we should add this if it's not there already https://expressjs.com/en/guide/behind-proxies.html

@raymondjacobson
Copy link
Member Author

@raymondjacobson we should add this if it's not there already https://expressjs.com/en/guide/behind-proxies.html

Looks like we only have it on identity. I thought i saw it on both! Adding to cn!

@raymondjacobson raymondjacobson merged commit ad055d9 into master Oct 29, 2020
@raymondjacobson raymondjacobson deleted the rj-rate-limits-rework-hotfix branch October 29, 2020 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants