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

Rate limiting for scheduling triggers #587

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Rate limiting for scheduling triggers #587

wants to merge 5 commits into from

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Jan 18, 2023

I completed my tech spike.

I couldn't (figure out how to) decorate only the trigger method of our FlaskView, so I worked around that by decorating the entire SensorAPI and using the cost function to only count towards the limit for the trigger method. It's not the most elegant solution, but it does look easily extensible this way.

We also discussed offline on what to apply the limit (using key_func): users or accounts. Here I decided to combine the account and the sensor id, just to let a test pass (test_trigger_and_get_schedule first schedules a battery and then schedules a charging station). But it might also make sense to apply a limit per sensor.

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x requested a review from nhoening January 18, 2023 21:21
Signed-off-by: F.N. Claessen <felix@seita.nl>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

I found two arguments against protecting a whole resource in one way.

In this solution (also using a syntactic sugar on top of Flask), they found a way to add decorators per method. Not sure if Flask-Classful has that.

Have you tried to decorate an endpoint outside of Flask-Classful's reach?

# Apply rate limit: a schedule can be triggered once per 5 minutes per sensor per account
SensorAPI.decorators.append(
app.limiter.limit(
"1 per 5 minutes",
Copy link
Contributor

Choose a reason for hiding this comment

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

We apply this here for all endpoints, but the timing we want to allow should differ per endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But given the cost function, it is only effectively applied to one endpoint. We are free to add more limiter.limit() decorators with other rate limits that affect other endpoints (via the cost function).

@@ -11,6 +12,21 @@ def register_at(app: Flask):

v3_0_api_prefix = "/api/v3_0"

def cost_function() -> int:
if request.endpoint == "SensorAPI:trigger_schedule":
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid that if for some reason the endpoint's identification string changes, we suddenly stop limiting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can probably find a way to obtain this string in a way that directly references the class method.

SensorAPI.__name__ + ":" + SensorAPI.trigger_schedule.__name__ would be my first try.

@Flix6x
Copy link
Contributor Author

Flix6x commented Jan 19, 2023

In this solution (also using a syntactic sugar on top of Flask), they found a way to add decorators per method. Not sure if Flask-Classful has that.

I'd definitely prefer that, but did they find a way though? I don't see any comment claiming they were able to add a decorator for a distinct class method. Only for all of the methods in the class, by adding the limiter.limit() to the list of method_decorators (which is applied to all class methods). In flask-classful this list variable is called decorators instead, and it is precisely that which I'm appending the limiter.limit() to. (I didn't add it to the list in the class itself, because the app context is not known there.)

What is possible is to tell limiter.limit() to only apply itself to class methods that support a certain http method (e.g. POST). I guess we could split off the schedule triggering part from the SensorAPI class, so both new classes will only contain a single POST method. That might also be more RESTful in the end, since I've come to think about the trigger endpoint as POSTing a scheduling job.

@nhoening
Copy link
Contributor

Have you tried yet to decorate an endpoint outside of Flask-Classful?

@Flix6x
Copy link
Contributor Author

Flix6x commented Jan 19, 2023

I have. That had the same problem of not having the app context available. Applying it to a whole blueprint did work, though. I had accomplished the latter in the register function of __init__.py of API v2, where the app context is available.

@nhoening
Copy link
Contributor

I don't recall why the app context is needed for...

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x
Copy link
Contributor Author

Flix6x commented Jan 23, 2023

I don't recall why the app context is needed for...

It's needed to set up the Limiter (https://flask-limiter.readthedocs.io/en/stable/api.html#flask_limiter.Limiter.params.app), so all of these config variables can be read in, including the redis connection.

@nhoening
Copy link
Contributor

I'm stuck here because it seems weird that such core functionality (adding the app context) would not work in the main use case (an endpoint).

It's passed to the Limiter in the main examples. Sometimes, the limiter decorator cannot access things from the app but the docs propose a solution which might work?

@Flix6x
Copy link
Contributor Author

Flix6x commented Jan 27, 2023

On this branch you'll find the rest of my tech spike where I test out limiting a non-FlaskView endpoint by decorating it directly, and also limiting an entire blueprint. The former doesn't work (the decorator seems to be ignored), the latter does work.

To test out, run pytest -k test_post_prognosis_2_0. (To test out the blueprint approach, comment out the endpoint's decorator, and uncomment my edits in v2's __init__.py.

Sometimes, the limiter decorator cannot access things from the app but the docs propose a solution which might work?

In my branch, the issue with the decorated endpoint approach seems not to be in getting the limit string from the app (I just hardcoded a string), but setting up the limiter to work with the app.

I guess you could carry on my tech spike, or we should come to a decision on how I can move forward. Otherwise, I'm stuck, too. I've made two suggestions:

  • Using the cost function as a way of assigning the limit to a specific endpoint, or
  • Splitting off the triggering functionality from SensorAPI.

@nhoening
Copy link
Contributor

I believe I'm willing to pursue the cost function approach. I did not yet understand at which (accumulated) costs the rate limiter stops access. Is that simply 1? Or can this be some setting? And are costs accumulated per endpoint? Maybe you can point me to the place in the rate-limiter docs where I can learn this. I could not find it.

In any case, I'd still want to:

  • move the cost function to a central place
  • make sure the costs (per endpoint I guess) apply to an account
  • maybe adjust the error message

I'm willing to work on that.

@Flix6x
Copy link
Contributor Author

Flix6x commented Feb 28, 2023

I did not yet understand at which (accumulated) costs the rate limiter stops access. Is that simply 1? Or can this be some setting? And are costs accumulated per endpoint? Maybe you can point me to the place in the rate-limiter docs where I can learn this. I could not find it.

For limiter.limit("10/second") the limit cost would be 10.

Docs here.

…e to the actual endpoints)

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening
Copy link
Contributor

nhoening commented Mar 2, 2023

Current list of ideas of TODOs:

  • make other tests pass, as well
  • base rate limiting for non-resource endpoints (e.g. ping, login)
  • separate base rate limiting for resource from account-based limiting (limit only "smart" endpoints, add attribute to accounts)

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening linked an issue Mar 31, 2023 that may be closed by this pull request
@nhoening nhoening added this to the 0.19.0 milestone Dec 18, 2023
@nhoening nhoening modified the milestones: 0.19.0, 0.20 Feb 18, 2024
@nhoening nhoening modified the milestones: 0.20, 0.21 Mar 26, 2024
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.

Rate limiting for scheduling
2 participants