You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kyounghunJang (via GitHub)" <gi...@apache.org> on 2025/11/14 05:22:58 UTC
[PR] feat(fab): add configurable rate limiter storage options [airflow]
kyounghunJang opened a new pull request, #58293:
URL: https://github.com/apache/airflow/pull/58293
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
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "aeroyorch (via GitHub)" <gi...@apache.org>.
aeroyorch commented on code in PR #58293:
URL: https://github.com/apache/airflow/pull/58293#discussion_r2526041992
##########
providers/fab/provider.yaml:
##########
@@ -245,6 +245,24 @@ config:
type: integer
Review Comment:
We should update the documentation of the provider (```providers/fab/docs/auth-manager/security.rst```, ```Rate limiting``` section) to reflect this change.
##########
providers/fab/provider.yaml:
##########
@@ -245,6 +245,24 @@ config:
type: integer
example: ~
default: "1"
+ auth_rate_limit_storage_uri:
Review Comment:
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```:
```yaml
flask_limiter[redis]>3,<4,!=3.13
```
It might also be helpful to include examples of both supported backends, e.g.: ```redis://localhost:6379 or redis+cluster://redis0:6379,redis1:6379```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #58293: feat(fab): add configurable rate limiter storage options
URL: https://github.com/apache/airflow/pull/58293
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "kyounghunJang (via GitHub)" <gi...@apache.org>.
kyounghunJang commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3531295224
@aeroyorch
Thank you for the review! I will incorporate the feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3536839251
> 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" in our doc 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3536792256
> For example, if all environments share the same configuration but in production you need to enable Keycloak authentication, the production webserver_config.py would have to be a copy-paste of the base file plus the Keycloak setup.
Quite the opposite. You totally misunderstood flexibilty you get with webserver_config.py:
webserver_config.py
```python
# Do dev configuraiton here
if os.env.get("ENVIRONMENT") == 'PROD':
# add your Keycloak configuration here
```
This 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "aeroyorch (via GitHub)" <gi...@apache.org>.
aeroyorch commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3536746575
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 [```app.config.from_prefixed_env()```](https://flask.palletsprojects.com/en/stable/config/#configuring-from-environment-variables), since Flask-Limiter [honors Flask’s configuration values](https://flask-limiter.readthedocs.io/en/stable/configuration.html#using-flask-config). This would enable users to set rate limiter and related settings directly via environment variables, using a prefix such as ```AIRFLOW__FAB__```, or even a more specific namespace (e.g. ```AIRFLOW__FAB__FLASK_SETTINGS```, ```AIRFLOW__FAB__CONFIG```).
This might provide a middle ground that maintains flexibility while keeping things consistent with Flask’s configuration patterns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "aeroyorch (via GitHub)" <gi...@apache.org>.
aeroyorch commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3536807703
> > For example, if all environments share the same configuration but in production you need to enable Keycloak authentication, the production webserver_config.py would have to be a copy-paste of the base file plus the Keycloak setup.
>
> Quite the opposite. You totally misunderstood flexibilty you get with webserver_config.py:
>
> webserver_config.py
>
> ```python
> # Do rgular configuration here
>
> if os.environment.get("ENVIRONMENT") == 'PROD':
> # add your Keycloak configuration here
> ```
>
> This 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "aeroyorch (via GitHub)" <gi...@apache.org>.
aeroyorch commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3532412316
> @aeroyorch Thank you for the review! I will incorporate the feedback.
Thanks @kyounghunJang!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "aeroyorch (via GitHub)" <gi...@apache.org>.
aeroyorch commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3536778299
> > 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 ```webserver_config.py``` starts to grow, for example when you need to extend ```FabAirflowSecurityManagerOverride```, add extra Flask parameters, or include rate limiter settings, having everything in a single file can become less practical to manage.
For example, if all environments share the same configuration but in production you need to enable Keycloak authentication, the production ```webserver_config.py``` would have to be a copy-paste of the base file plus the Keycloak setup. Since integrating Keycloak requires custom (and non-trivial) code, this makes it harder to maintain across environments, especially in Kubernetes or GitOps-based deployments, where you typically want to manage differences declaratively rather than by duplicating files. So while the current approach is very flexible from a dynamic point of view, it can be less convenient for deployment and environment management.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3536757397
> 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 same thing ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(fab): add configurable rate limiter storage options [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #58293:
URL: https://github.com/apache/airflow/pull/58293#issuecomment-3703094052
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org