Upgrade FAB to 4.3.0 #29766

potiuk · 2023-02-25T15:53:15Z

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:

While Flask App Builder disabled rate limitig by default, Airlfow is "end product" using it and we decided to enable it.

^ 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.rst or {issue_number}.significant.rst, in newsfragments.

potiuk · 2023-02-25T15:58:15Z

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.

potiuk · 2023-02-25T17:36:34Z

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

potiuk · 2023-02-25T20:21:11Z

I think all the tests should pass now.

potiuk · 2023-02-25T21:36:16Z

Yep. Ready for tests/review. Also added specific test for rate_limiting

potiuk · 2023-02-26T08:49:28Z

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)


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?

cc: @dpgaspar ?

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

airflow/www/extensions/init_appbuilder.py

potiuk · 2023-02-26T08:50:53Z

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)


I figured those would be good defaults.

potiuk · 2023-02-26T09:08:10Z

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):


I don't think we expose that view as we are not exposing other views, but I added it anyway.

potiuk · 2023-02-26T09:10:57Z

Also tested it manually:

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.

potiuk · 2023-03-03T10:58:50Z

Anyone ? @bolkedebruin

dpgaspar · 2023-03-03T11:26:57Z

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)


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

dpgaspar · 2023-03-03T11:31:31Z

Also tested it manually:

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?

potiuk · 2023-03-03T15:57:37Z

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

potiuk · 2023-03-04T13:54:05Z

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.

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: * dpgaspar/Flask-AppBuilder#1976 * dpgaspar/Flask-AppBuilder#1997 * dpgaspar/Flask-AppBuilder#1999 While Flask App Builder disabled rate limitig by default, Airlfow is "end product" using it and we decided to enable it.

bolkedebruin · 2023-03-12T17:23:44Z

Sorry about not seeing this @potiuk ping me on slack (next time) if you want me to take a look, you know my email count ;-)

potiuk · 2023-03-12T17:33:09Z

No worries. I got good support from @dpgaspar

boring-cyborg bot added the area:webserver Webserver related Issues label Feb 25, 2023

potiuk requested review from jhtimmins, ryanahamilton, ashb, bolkedebruin and bbovenzi February 25, 2023 15:53

potiuk force-pushed the upgrade-to-fab-4.3.0 branch 2 times, most recently from 5c3c2fc to d763e80 Compare February 25, 2023 17:02

potiuk force-pushed the upgrade-to-fab-4.3.0 branch 2 times, most recently from 42f8728 to fc0c979 Compare February 25, 2023 20:20

potiuk marked this pull request as ready for review February 25, 2023 20:21

potiuk added this to the Airflow 2.5.2 milestone Feb 25, 2023

potiuk commented Feb 26, 2023

View reviewed changes

airflow/www/extensions/init_appbuilder.py Show resolved Hide resolved

potiuk commented Feb 26, 2023

View reviewed changes

pierrejeambrun modified the milestones: Airflow 2.5.2, Airflow 2.5.3 Feb 28, 2023

pierrejeambrun added the type:misc/internal Changelog: Misc changes that should appear in change log label Mar 1, 2023

potiuk requested review from ephraimbuddy and pierrejeambrun March 1, 2023 22:37

dpgaspar approved these changes Mar 3, 2023

View reviewed changes

potiuk modified the milestones: Airflow 2.5.2, Airflow 2.6.0 Mar 3, 2023

potiuk force-pushed the upgrade-to-fab-4.3.0 branch from fc0c979 to d706275 Compare March 4, 2023 13:49

potiuk force-pushed the upgrade-to-fab-4.3.0 branch from d706275 to 0a1e781 Compare March 4, 2023 13:55

eladkal approved these changes Mar 4, 2023

View reviewed changes

potiuk merged commit b7f0500 into apache:main Mar 4, 2023

potiuk deleted the upgrade-to-fab-4.3.0 branch March 4, 2023 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade FAB to 4.3.0 #29766

Upgrade FAB to 4.3.0 #29766

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk Feb 26, 2023

potiuk Feb 26, 2023

dpgaspar Mar 3, 2023

potiuk Feb 26, 2023

potiuk Feb 26, 2023

potiuk commented Feb 26, 2023

potiuk commented Mar 3, 2023

dpgaspar Mar 3, 2023

dpgaspar commented Mar 3, 2023

potiuk commented Mar 3, 2023

potiuk commented Mar 4, 2023

bolkedebruin commented Mar 12, 2023 •

edited

Loading

potiuk commented Mar 12, 2023 •

edited

Loading

Upgrade FAB to 4.3.0 #29766

Upgrade FAB to 4.3.0 #29766

Conversation

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk commented Feb 25, 2023

potiuk Feb 26, 2023

Choose a reason for hiding this comment

potiuk Feb 26, 2023

Choose a reason for hiding this comment

dpgaspar Mar 3, 2023

Choose a reason for hiding this comment

potiuk Feb 26, 2023

Choose a reason for hiding this comment

potiuk Feb 26, 2023

Choose a reason for hiding this comment

potiuk commented Feb 26, 2023

potiuk commented Mar 3, 2023

dpgaspar Mar 3, 2023

Choose a reason for hiding this comment

dpgaspar commented Mar 3, 2023

potiuk commented Mar 3, 2023

potiuk commented Mar 4, 2023

bolkedebruin commented Mar 12, 2023 • edited Loading

potiuk commented Mar 12, 2023 • edited Loading

bolkedebruin commented Mar 12, 2023 •

edited

Loading

potiuk commented Mar 12, 2023 •

edited

Loading