-
Notifications
You must be signed in to change notification settings - Fork 16.3k
feat(fab): add configurable rate limiter storage options #58293
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
feat(fab): add configurable rate limiter storage options #58293
Conversation
3cb9cf1 to
a9e3685
Compare
| type: integer | ||
| example: ~ | ||
| default: "1" | ||
| auth_rate_limit_storage_uri: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d clarify in the description that only the Redis and Redis Cluster backends are supported.
Additionally, we should ensure that the redis extra is included in the fab pyproject.toml:
flask_limiter[redis]>3,<4,!=3.13It might also be helpful to include examples of both supported backends, e.g.: redis://localhost:6379 or redis+cluster://redis0:6379,redis1:6379
| See `Werkzeug: X-Forwarded-For Proxy Fix | ||
| <https://werkzeug.palletsprojects.com/en/2.3.x/middleware/proxy_fix/>`__ for more details. | ||
| version_added: 2.1.0 | ||
| type: integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the documentation of the provider (providers/fab/docs/auth-manager/security.rst, Rate limiting section) to reflect this change.
|
@aeroyorch |
a9e3685 to
4d70776
Compare
Thanks @kyounghunJang! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need it at all. The webserver_config.py is not cumbersome at all. Using Helm Chart you can mount webserver_config.py with config map easily to "${AIRFLOW_HOME}/config", and the idea about configuration it this way is that you can do imperative conditions - for example easily have different configuration depending on dev/prod/staging configuration by if python condition in webserver_config.py
This has been especially designed this way also to make it rather independent from the configuration of FAB - with webserver_config.py you can use any of the environment variables that FAB supports- in any past and future version and you do not have to complicate FAB provider and keep it in sync in Airflow configuration.
All that was absolutely deliberate decision, and I find trying to constraint what you can do and add Fab-specific configuration as Fab provider config, when it is already possible via webserver_config.py as pretty cumbersome idea.
I dont't think it's a good idea to add extra maintenance effort for the provider to have those as options in configuration/
I wonder what others think about it, maybe I am too strict, but I find it rather unnecessary and not a good idea.
|
Hi @potiuk! Perhaps we could explore complementary mechanisms that leverage Flask’s native configuration system without needing to modify Airflow’s configuration directly. For instance, we could use This might provide a middle ground that maintains flexibility while keeping things consistent with Flask’s configuration patterns. |
So you want to add THIRD way of configuring the webserver ? |
I don’t have a strong interest in adding yet another configuration mechanism. My point is more about practicality: when the For example, if all environments share the same configuration but in production you need to enable Keycloak authentication, the production Maybe we’re missing something, though if there’s already a good pattern for splitting webserver configs or handling these kinds of use cases, it would be great to take a look. |
Quite the opposite. You totally misunderstood flexibilty you get with webserver_config.py: webserver_config.py # Do rgular configuration here
if os.environment.get("ENVIRONMENT") == 'PROD':
# add your Keycloak configuration hereThis is precisely the way to avoid copy&pasting different variants of configuration - because the same webserver_config.py might be used in different environments and the fact that it's imperative, actually makes it even easier. Maybe that is not clear in the documentation - but that was precisely the intention behind having webserver_config.py as a python code. Maybe instead, you should make a PR documenting this precise use case and explain it better? Maybe you simply missed that it's done exactly to solve the problem that you mentioned? |
Thanks for the clarification. If no one else sees value in this, I’m fine with rejecting the feature request. Appreciate the explanation. |
I think better way will be if you indeed create a PR describing it better in our docs and showing the use case in our docs - because apparently people like you might get confused. This is the least you can do to contribute back - and you are probably the best person to explain it in the way for people like you - now that you know it, got some time of maintainers to clarify it and you know where you were looking in the docs for it. It's also super easy - just click "suggest a change on this page" (bottom right) in our docs and PR will be opened for you and you will be able to edit the .rst source of relevant docs directly in GitHub UI. Can we count on this @aeroyorch ? You likely uncovered real issue that you can fix this way (clarity of the docs). |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
Add configurable rate limiter storage options to FAB provider
This PR adds new configuration options to the FAB provider, allowing
custom storage for the rate limiter. Existing functionality is preserved.
closes: #58256
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.