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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Setting a rate limit on the endpoints using flask limiter #1006

Open
epicadk opened this issue Feb 18, 2021 · 30 comments
Open

Feature: Setting a rate limit on the endpoints using flask limiter #1006

epicadk opened this issue Feb 18, 2021 · 30 comments
Assignees
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Hacktoberfest Type: Enhancement New feature or request.

Comments

@epicadk
Copy link
Member

epicadk commented Feb 18, 2021

Is your feature request related to a problem? Please describe.

Heroku Does offer DDoS mitigation however, according to this post they strongly recommend using a rate limiter library.

Describe the solution you'd like

Rate limit the endpoints of the api using flask limiter.

Additional context

The time and amount of requests that should be allowed should be discussed, possibly in the mentorship session.

@epicadk epicadk added Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request. Status: Available Issue was approved and available to claim or abandoned for over 3 days. Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. and removed Status: Available Issue was approved and available to claim or abandoned for over 3 days. labels Feb 18, 2021
@epicadk
Copy link
Member Author

epicadk commented Feb 18, 2021

Marking this as status on hold because it needs to be approved by @isabelcosta .

@isabelcosta
Copy link
Member

@epicadk this sounds amazing. This is the kind of thing I only learned this year, so I really appreciate you coming with these "out of the box" ideas 🤗 I will approve this. Could you link the library in the description of the issue and add a little more information if you have more.

@isabelcosta isabelcosta added Status: Available Issue was approved and available to claim or abandoned for over 3 days. and removed Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. labels Feb 22, 2021
@epicadk
Copy link
Member Author

epicadk commented Feb 23, 2021

@isabelcosta 😅 Thankyou : ) . I have linked the library in the issue however I haven't used it much so I don't really have any more information about it. We also need to discuss the limit or the amount of times that a user should be allowed to query the backend.

@isabelcosta
Copy link
Member

thank you so much @epicadk !

@jalajcodes
Copy link
Member

I'd like to work on it
I'll be in the next open session to discuss about this issue.

@epicadk
Copy link
Member Author

epicadk commented Mar 2, 2021

I'd like to work on it
I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

@epicadk epicadk removed the Status: Available Issue was approved and available to claim or abandoned for over 3 days. label Mar 2, 2021
@jalajcodes
Copy link
Member

I'd like to work on it
I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

Ohk! Do you mind sharing the meeting docs link? I'd like to go through the discussion.

@epicadk
Copy link
Member Author

epicadk commented Mar 2, 2021

I'd like to work on it
I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

Ohk! Do you mind sharing the meeting docs link? I'd like to go through the discussion.

I went through the docs and didn't find any thing related to this issue so maybe I was mistaken. But isabel has apporved it so you can work on it. What we need to discuss is the rates for each endpoint. You can still work on it and submit a pr. Although you might have to change only the rates later on so up to you.

@jalajcodes
Copy link
Member

I'd like to work on it
I'll be in the next open session to discuss about this issue.

I think we already discussed it. Since Isabel has approved I'll assign you. Happy coding : ) .

Ohk! Do you mind sharing the meeting docs link? I'd like to go through the discussion.

I went through the docs and didn't find any thing related to this issue so maybe I was mistaken. But isabel has apporved it so you can work on it. What we need to discuss is the rates for each endpoint. You can still work on it and submit a pr. Although you might have to change only the rates later on so up to you.

Ok cool I'll start working on it then ;)

@vj-codes
Copy link
Member

vj-codes commented Mar 6, 2021

@jalajcodes hey any updates?

@epicadk
Copy link
Member Author

epicadk commented Mar 6, 2021

@mtreacy002 I think this issue can cause problems for BIT. we'll have to exclude the BIT server from the rate limiting.

@jalajcodes
Copy link
Member

@jalajcodes hey any updates?

I'll create PR tomorrow or if time permits, today

@vj-codes
Copy link
Member

vj-codes commented Mar 6, 2021

@jalajcodes take your time, just update the progress after every 3 days if it's taking long

@mtreacy002
Copy link
Member

mtreacy002 commented Mar 7, 2021

@mtreacy002 I think this issue can cause problems for BIT. we'll have to exclude the BIT server from the rate limiting.

@epicadk , I agree, this could cause issue if the rate limiter sets the amount of requests BIT can do, since in BIT-MS integration the number of requests sent would be extensive 🤣🤣.
If possible, @jalajcodes, please share the approach you're thinking of doing (e.g. what type of limiter you will apply here), so we can assess the impact to BIT better. Perhaps we should add CROSS-PORJECT ISSUE on the title so I can monitor the progress made here as it would impact BIT. cc @vj-codes and @isabelcosta

@jalajcodes
Copy link
Member

If possible, @jalajcodes, please share the approach you're thinking of doing (e.g. what type of limiter you will apply here), so we can assess the impact to BIT better.

As we haven't decided rates for specific endpoints yet, I am thinking of adding a generous rate limit globally for all endpoints and exempt it for those endpoints which don't need to be rate limited like login etc.

regarding the BIT-MS integration, I think flask-limiter allows us to customize rate limits to be based on characteristics of the incoming request. I am not sure but I guess we could add some kind of check to determine if the request is coming from BIT and increase the limit or maybe disable it completely?

@epicadk
Copy link
Member Author

epicadk commented Mar 7, 2021

If possible, @jalajcodes, please share the approach you're thinking of doing (e.g. what type of limiter you will apply here), so we can assess the impact to BIT better.

As we haven't decided rates for specific endpoints yet, I am thinking of adding a generous rate limit globally for all endpoints and exempt it for those endpoints which don't need to be rate limited like login etc.

regarding the BIT-MS integration, I think flask-limiter allows us to customize rate limits to be based on characteristics of the incoming request. I am not sure but I guess we could add some kind of check to determine if the request is coming from BIT and increase the limit or maybe disable it completely?

Login is probably the endpoint we want to rate limit first. 😄 .

@jalajcodes
Copy link
Member

@epicadk 😂😅 my bad, just assume any other endpoint that doesn't require limiting

@Anmollenka
Copy link
Contributor

I would like to take up this issue if it's available.

@jalajcodes
Copy link
Member

I'll create PR today

@epicadk epicadk added the Status: Available Issue was approved and available to claim or abandoned for over 3 days. label Jun 8, 2021
@battuAshita
Copy link
Contributor

Hi @epicadk and @isabelcosta. Please assign this issue to me if it is open :)

@isabelcosta
Copy link
Member

@battuAshita are you still interested in the issue?

@battuAshita
Copy link
Contributor

Hi @isabelcosta. I am actually caught up in some work. It is free to be assigned to someone else. Thank you :)

@isabelcosta
Copy link
Member

No worries @battuAshita ! thank you for responding so quickly 🤗 in this way we can assign to someone else who is interested!

@sakshivij
Copy link

Hi @isabelcosta. Is this open to work on? If yes, can you please assign this to me? Thanks!

@vj-codes
Copy link
Member

vj-codes commented Oct 3, 2021

Assigning you @sakshivij
Happy coding!

@vj-codes vj-codes removed the Status: Available Issue was approved and available to claim or abandoned for over 3 days. label Oct 3, 2021
@sakshivij
Copy link

sakshivij commented Oct 6, 2021

@vj-codes to decide on rate-limit was there any doc created as mentioned on the closed PR? Or I would need to create one?

@vj-codes
Copy link
Member

vj-codes commented Oct 6, 2021

@epicadk can you answer that please? ⬆️

@epicadk
Copy link
Member Author

epicadk commented Oct 6, 2021

@epicadk can you answer that please? ⬆️

That's something we need to discuss in the mentorship session. I think you could probably use a constant right now and then change it later? cc @isabelcosta

@sakshivij
Copy link

@epicadk can you answer that please? ⬆️

That's something we need to discuss in the mentorship session. I think you could probably use a constant right now and then change it later? cc @isabelcosta

Makes sense. If you mean the sync up session, I'll try to join as well.

@vj-codes
Copy link
Member

vj-codes commented Oct 6, 2021

Yes the sync up session, it is conducted on Saturdays biweekly at 6:30 pm IST
The next one scheduled is this Saturday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Coding Changes to code base or refactored code that doesn't fix a bug. Hacktoberfest Type: Enhancement New feature or request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants