You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/02/25 15:53:17 UTC
[GitHub] [airflow] potiuk opened a new pull request, #29766: Upgrade FAB to 4.3.0
potiuk opened a new pull request, #29766:
URL: https://github.com/apache/airflow/pull/29766
FAB 4.3.0 added rate limiting and we would like to upgrade to this version.
This requires to bring some of the changes from the PRs merged in Flask App Builder:
* https://github.com/dpgaspar/Flask-AppBuilder/pull/1976
* https://github.com/dpgaspar/Flask-AppBuilder/pull/1997
* https://github.com/dpgaspar/Flask-AppBuilder/pull/1999
While Flask App Builder disabled rate limitig by default, Airlfow is "end product" using it and we decided to enable it.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.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 [newsfragments](https://github.com/apache/airflow/tree/main/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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1454747547
> If I'm not mistaken, RATELIMIT_ENABLED will broadly enable rate limit since it can be added on ModelRestApi classes and AUTH_RATE_LIMITED will need RATELIMIT_ENABLED and add it to the auth views
Yep. I linked those two in Airflow configuration now.
> Was not aware but makes perfect sense, to rate limit globally a web farm with multiple web servers with multiple gunicorn processes you can use RATELIMIT_STORAGE_URI with redis for example.
> More useful settings can be made for rate limiting based on username, host etc can be found here: https://flask-limiter.readthedocs.io/en/stable/configuration.html
> Will Airflow allow admin's to tune these settings also?
I've added more details and documenation on how to configure those. It's possible - the same way as authentication (via webserver_config.py) - but it was not "explicitly" stated in the docs (the docs only mentioned that you can configure authentication options this way, but not any other flask plugin. I made it explicitly documented and I also added a separate section for "rate limiting" explaining the (a little unexpected) default behaviour where each gunicorn process has their own limit and explained how the users could configure storage to achieve shared limits.
I think that one is ready for final review.
--
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
[GitHub] [airflow] dpgaspar commented on a diff in pull request #29766: Upgrade FAB to 4.3.0
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #29766:
URL: https://github.com/apache/airflow/pull/29766#discussion_r1124332217
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -172,10 +180,13 @@ def init_app(self, app, session):
app.config.setdefault("APP_ICON", "")
app.config.setdefault("LANGUAGES", {"en": {"flag": "gb", "name": "English"}})
app.config.setdefault("ADDON_MANAGERS", [])
+ app.config.setdefault("RATELIMIT_ENABLED", True)
Review Comment:
If I'm not mistaken, `RATELIMIT_ENABLED` will broadly enable rate limit since it can be added on `ModelRestApi` classes and `AUTH_RATE_LIMITED` will need `RATELIMIT_ENABLED` and add it to the auth views
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1445196182
I think all the tests should pass now.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29766:
URL: https://github.com/apache/airflow/pull/29766#discussion_r1118046851
##########
airflow/www/fab_security/manager.py:
##########
@@ -1397,6 +1424,24 @@ def get_user_menu_access(self, menu_names: list[str] | None = None) -> set[str]:
else:
return self._get_user_permission_resources(None, "menu_access", resource_names=menu_names)
+ def add_limit_view(self, baseview):
Review Comment:
I don't think we expose that view as we are not exposing other views, but I added it anyway.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29766:
URL: https://github.com/apache/airflow/pull/29766#discussion_r1118044321
##########
airflow/www/fab_security/manager.py:
##########
@@ -252,6 +254,10 @@ def __init__(self, appbuilder):
app.config.setdefault("AUTH_LDAP_LASTNAME_FIELD", "sn")
app.config.setdefault("AUTH_LDAP_EMAIL_FIELD", "mail")
+ # Rate limiting
+ app.config.setdefault("AUTH_RATE_LIMITED", True)
Review Comment:
I figured those would be good defaults.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29766:
URL: https://github.com/apache/airflow/pull/29766#discussion_r1118044266
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -563,6 +575,11 @@ def security_converge(self, dry=False) -> dict:
"""
return self.sm.security_converge(self.baseviews, self.menu, dry)
+ def get_url_for_login_with(self, next_url: str | None = None) -> str:
Review Comment:
This was added recently in the FAB's app builder and is used in some scenarios, so I added it too.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29766:
URL: https://github.com/apache/airflow/pull/29766#discussion_r1118044148
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -172,10 +180,13 @@ def init_app(self, app, session):
app.config.setdefault("APP_ICON", "")
app.config.setdefault("LANGUAGES", {"en": {"flag": "gb", "name": "English"}})
app.config.setdefault("ADDON_MANAGERS", [])
+ app.config.setdefault("RATELIMIT_ENABLED", True)
Review Comment:
Not sure why this one is here, but it is one of the things the AppBuilder in FAB has comparing to us cc: @bolkedebruin - maybe you know how RATELIMIT_ENABLED relates to AUTH_RATE_LIMITED?
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1445211243
Yep. Ready for tests/review. Also added specific test for rate_limiting
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29766:
URL: https://github.com/apache/airflow/pull/29766#discussion_r1118046994
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -172,10 +180,13 @@ def init_app(self, app, session):
app.config.setdefault("APP_ICON", "")
app.config.setdefault("LANGUAGES", {"en": {"flag": "gb", "name": "English"}})
app.config.setdefault("ADDON_MANAGERS", [])
+ app.config.setdefault("RATELIMIT_ENABLED", True)
Review Comment:
cc: @dpgaspar ?
--
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
[GitHub] [airflow] bolkedebruin commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1465253183
Sorry about not seeting this @potiuk ping me on slack if you want me to take a look, you know my email count ;-)
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1465255002
No worries. I got good support for @dpgaspar
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1445147943
This one attempts to sync the changes in FAB 4.3.0 including @bolkedebruin rate limiter.
I think as an "end product" we should have rate limiting enabled by default, but would love your comments.
As usual, I am not fully confident if this is all that we need because there are some parts of FAB security manager that we do not have at all in our updated copy - so I would appreciate any comments review there. Also it would be great to test if rate limiting actually works as expected (I assume you had some manual testing done for that @bolkedebruin, so maybe you could try it as well (as long as we pass all the tests).
Any other comments there - I'd love to hear them.
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1445305073
Also tested it manually:
<img width="823" alt="Screenshot 2023-02-26 at 10 07 49" src="https://user-images.githubusercontent.com/595491/221401690-8dab4538-88b2-4171-8ecf-ff4b43119e29.png">
BTW. Not sure if you are aware @bolkedebruin (@dpgaspar) that rate limiting implemented currently is "per gunicorn process" not per "webserver" - so if you have say 3 gunicorn processes, really the limit you get (assuming random request distribution) is 3 x higher on average.
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1453352097
Anyone ? @bolkedebruin
--
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
[GitHub] [airflow] dpgaspar commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1453390989
> Also tested it manually:
>
> <img alt="Screenshot 2023-02-26 at 10 07 49" width="823" src="https://user-images.githubusercontent.com/595491/221401690-8dab4538-88b2-4171-8ecf-ff4b43119e29.png">
>
> BTW. Not sure if you are aware @bolkedebruin (@dpgaspar) that rate limiting implemented currently is "per gunicorn process" not per "webserver" - so if you have say 3 gunicorn processes, really the limit you get (assuming random request distribution) is 3 x higher on average.
Was not aware but makes perfect sense, to rate limit globally a web farm with multiple web servers with multiple gunicorn processes you can use `RATELIMIT_STORAGE_URI` with redis for example.
More useful settings can be made for rate limiting based on username, host etc can be found here: https://flask-limiter.readthedocs.io/en/stable/configuration.html
Will Airflow allow admin's to tune these settings also?
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1445165868
Nice ... It seems our unit tests are failing now because .... rate limit has been exceeded :)
```
[2023-02-25T17:21:02.852+0000] {extension.py:981} INFO - ratelimit 10 per 20 second (127.0.0.1) exceeded at endpoint: AuthDBView.login
```
--
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
[GitHub] [airflow] potiuk commented on pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29766:
URL: https://github.com/apache/airflow/pull/29766#issuecomment-1453742552
> Will Airflow allow admin's to tune these settings also?
It might make sense to add ANY flask configuration (that would indeed be useful). I will 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
[GitHub] [airflow] potiuk merged pull request #29766: Upgrade FAB to 4.3.0
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29766:
URL: https://github.com/apache/airflow/pull/29766
--
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