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

Upgrade to FlaskAppBuilder 4.0.* #22397

Closed
1 task done
potiuk opened this issue Mar 21, 2022 · 13 comments · Fixed by #24399
Closed
1 task done

Upgrade to FlaskAppBuilder 4.0.* #22397

potiuk opened this issue Mar 21, 2022 · 13 comments · Fixed by #24399
Labels
area:dependencies Issues related to dependencies problems kind:meta High-level information important to the community

Comments

@potiuk
Copy link
Member

potiuk commented Mar 21, 2022

Body

Flask App Builder 4.0 has been released - including allowing us to bump multiple depending packages that have been held back by it. Apart of bumping version of dependent packages, there does not seem to be breaking changes that impact us directly.

We should migrate to FAB 4.0.* in Airlfow 2.3 I think at earliest convenience, but it also requires some extensive testing (which 2.3.0 will probably be all about.

https://flask-appbuilder.readthedocs.io/en/latest/breaking.html#version-4-0-0

Drops python 3.6 support

  • Removed config key AUTH_STRICT_RESPONSE_CODES, it’s always strict now.
  • Removes Flask-OpenID dependency (you can install it has an extra dependency pip install flask-appbuilder[openid])

Major version bumps on following packages

Flask from 1.X to 2.X

Breaking changes: https://flask.palletsprojects.com/en/2.0.x/changes/#version-2-0-0

flask-jwt-extended 3.X to 4.X:

Breaking changes: https://flask-jwt-extended.readthedocs.io/en/stable/v4_upgrade_guide/

Jinja2 2.X to 3.X

Breaking changes: https://jinja.palletsprojects.com/en/3.0.x/changes/#version-3-0-0

Werkzeug 1.X to 2.X

https://werkzeug.palletsprojects.com/en/2.0.x/changes/#version-2-0-0

The following packages are probably not impactful to you:

pyJWT 1.X to 2.X:

Breaking changes: https://pyjwt.readthedocs.io/en/stable/changelog.html#v2-0-0

Click 7.X to 8.X:

Breaking changes: https://click.palletsprojects.com/en/8.0.x/changes/#version-8-0-0

itsdangerous 1.X to 2.X

Breaking changes: https://github.com/pallets/itsdangerous/blob/main/CHANGES.rst#version-200

Committer

  • I acknowledge that I am a maintainer/committer of the Apache Airflow project.
@potiuk potiuk added kind:meta High-level information important to the community area:dependencies Issues related to dependencies problems labels Mar 21, 2022
potiuk added a commit to potiuk/airflow that referenced this issue Mar 21, 2022
Flask Application Builder 4.0 unblocked many of our dependencies.
This PR updates to FAB 4.0 and related libraries.

Closes: apache#22397
potiuk added a commit that referenced this issue Mar 29, 2022
Flask Application Builder 4.0 unblocked many of our dependencies.
This PR updates to FAB 4.0 and related libraries.

Closes: #22397
potiuk added a commit that referenced this issue Mar 31, 2022
Flask Application Builder 4.0 unblocked many of our dependencies.
This PR updates to FAB 4.0 and related libraries.

Closes: #22397
@johannesjung
Copy link

Hi @potiuk, thank you for pushing towards upgrading Flask-AppBuilder. I see that you had started to work on this issue in #22587, but then suddenly closed it again. What was the reason for closing the PR and what does it mean for this issue?

@potiuk
Copy link
Member Author

potiuk commented May 9, 2022

This was just a draft to try it and see if it works, It had a number of problems in just "dependencies".
It means that it's not "straightforward" and someone will need to make a substantial effort to migrate (and test).

But if you want - you can pick it up and try to make a PR yourself. Woudl you want to take a stab on it ?

@johannesjung
Copy link

Thanks for the quick reply. It is not my field of expertise, so I would be happy if someone else gives it a try. It would be great to get rid of the older version of dependencies related to this.

@potiuk
Copy link
Member Author

potiuk commented May 9, 2022

Thanks for the quick reply. It is not my field of expertise, so I would be happy if someone else gives it a try. It would be great to get rid of the older version of dependencies related to this.

Quite agree. Maybe I will get to it at some point in time :). But if someone else would like to take it on - I am happy to help.

@potiuk
Copy link
Member Author

potiuk commented Jun 15, 2022

I am gathering here the list who will help with testing it when we implement it. Those are the people who want to get rid of dependencies. FAB 4+ upgrade introduces a lot of breaking changes. Migrating it will be a bit painful. but the bulk of the effort will be testing it

This is the list of people who are eager to get the update in their hands, they are not able to help with the upgrade but anyone can help with testing.

Request - since you expect the upgrade, to happen - we will ask you to pick the areas that you will want to test (or ask somene in your team to test). We will provide instructions on how to install airflow from the PR and provide packages that you will be able to install.

@gmsantos
Copy link
Contributor

@potiuk I can help by testing it. I have some scenarios that integrate Airflow with Azure AD for authentication and implement custom roles as well.

@potiuk
Copy link
Member Author

potiuk commented Jun 17, 2022

@potiuk I can help by testing it. I have some scenarios that integrate Airflow with Azure AD for authentication and implement custom roles as well.

Cool :) Thanks!

Status for everyone:

  • I already have a separate PR Get rid of TimedJSONWebSignatureSerializer #24519 that is important prerequisite for the migration (basically changing the library that is used for encryption/verification of log retrieval to more standard one as one of the upgraded dependencies deprecated and removed the class we were used). The good news is that I did it in a much more "standard" way (following JWT standard and pyjwt library)

  • there is another PR Update flask-appbuilder authlib/oauth dependency #24516 that should fix incompatibility of authlib library wiht Flask App Builder (I had to refresh docker images as well to fix it for our past Airflow releases Add note about image regeneration in June 2022 #24524

  • the Upgrade FAB to 4.1.1 #24399 is draft PR that I keep on rebasing that I am using to see what's left and extract PRs like above after fixing them there. I already did the following:

    • merged in all the FAB 4.1.1 security manager changes that we need to synchronize every time
    • I got rid of most of the static MyPy erors (Flask 2 added static check typing that revealed some potential typing issues and MyPy flagged them

What is left:

  • currently new version of Werkzeug fails with ImportError: cannot import name 'safe_str_cmp' from 'werkzeug.security' (/usr/local/lib/python3.7/site-packages/werkzeug/security.py) which I am looking at removing next
  • tests are failing (but for now due to Werkzeug, so we do not know yet the real scope)
  • provider verification is failing due to conflicts in dependencies - I will look to it at the very end
  • docker-compose quick-start tests are failing - those are likely due to some flask failures
  • helm chart tests are failing (same - lilkely flask upgrade problem)

It will take a few days/ week to iterate on all those and slowly fix it one-by-one (but PRs to my PR #24399 are most welcome if anyone would like to take a stab on solving any of the issues there :)

potiuk added a commit to potiuk/airflow that referenced this issue Jun 18, 2022
The TimedJSONWebSignatureSerializer has been deprecated from the
itsdangerous library and they recommended to use dedicated
libraries for it.

pallets/itsdangerous#129

Since we are going to move to FAB 4+ with apache#22397 where newer version of
itsdangerous is used, we need to switch to another library.

We are already using PyJWT so the choice is obvious.

Additionally to switching, the following improvements were done:

* the use of JWT claims has been fixed to follow JWT standard.
  We were using "iat" header wrongly. The specification of JWT only
  expects the header to be there and be valid UTC timestamp, but the
  claim does not impact maturity of the signature - the signature
  is valid if iat is in the future.
  Instead "nbf" - "not before" claim should be used to verify if the
  request is not coming from the future. We now require all claims
  to be present in the request.

* rather than using salt/signing_context we switched to standard
  JWT "audience" claim (same end result)

* we have now much better diagnostics on the server side of the
  reason why request is forbidden - explicit error messages
  are printed in server logs and details of the exception. This
  is secure, we do not spill the information about the reason
  to the client, it's only available in server logs, so there is
  no risk attacker could use it.

* the JWTSigner is "use-agnostic". We should be able to use the
  same class for any other signatures (Internal API from AIP-44)
  with just different audience

* Short, 5 seconds default clock skew is allowed, to account for
  systems that have "almost" synchronized time

* more tests addded with proper time freezing testing both
  expiry and immaturity of the request

This change is not a breaking one because the JWT authentication
details are not "public API" - but in case someone reverse engineered
our claims and implemented their own log file retrieval, we
should add a change in our changelog - therefore newsfragment
is added.
@potiuk
Copy link
Member Author

potiuk commented Jun 18, 2022

Good news. Static checka are all fixed and now I got to real tests to fix.

potiuk added a commit that referenced this issue Jun 18, 2022
The TimedJSONWebSignatureSerializer has been deprecated from the
itsdangerous library and they recommended to use dedicated
libraries for it.

pallets/itsdangerous#129

Since we are going to move to FAB 4+ with #22397 where newer version of
itsdangerous is used, we need to switch to another library.

We are already using PyJWT so the choice is obvious.

Additionally to switching, the following improvements were done:

* the use of JWT claims has been fixed to follow JWT standard.
  We were using "iat" header wrongly. The specification of JWT only
  expects the header to be there and be valid UTC timestamp, but the
  claim does not impact maturity of the signature - the signature
  is valid if iat is in the future.
  Instead "nbf" - "not before" claim should be used to verify if the
  request is not coming from the future. We now require all claims
  to be present in the request.

* rather than using salt/signing_context we switched to standard
  JWT "audience" claim (same end result)

* we have now much better diagnostics on the server side of the
  reason why request is forbidden - explicit error messages
  are printed in server logs and details of the exception. This
  is secure, we do not spill the information about the reason
  to the client, it's only available in server logs, so there is
  no risk attacker could use it.

* the JWTSigner is "use-agnostic". We should be able to use the
  same class for any other signatures (Internal API from AIP-44)
  with just different audience

* Short, 5 seconds default clock skew is allowed, to account for
  systems that have "almost" synchronized time

* more tests addded with proper time freezing testing both
  expiry and immaturity of the request

This change is not a breaking one because the JWT authentication
details are not "public API" - but in case someone reverse engineered
our claims and implemented their own log file retrieval, we
should add a change in our changelog - therefore newsfragment
is added.
potiuk added a commit to potiuk/airflow that referenced this issue Jun 19, 2022
The Flask Application Builder have been updated recently to
support a number of newer dependencies. This PR is the
attempt to migrate FAB to newer version.

Fixes: apache#22397
@potiuk
Copy link
Member Author

potiuk commented Jun 19, 2022

Hey @dpgaspar - FYI: we are close to switch to FAB 4.1.1+ (the #24399 is green and we are starting testing it with the help of our users).

potiuk added a commit to potiuk/airflow that referenced this issue Jun 19, 2022
The Flask Application Builder have been updated recently to
support a number of newer dependencies. This PR is the
attempt to migrate FAB to newer version.

This includes:

* update setup.py and setup.cfg upper and lower bounds to
  account for proper version of dependencies that
  FAB < 4.0.0 was blocking from upgrade
* added typed Flask application retrieval with a custom
  application fields available for MyPy typing checks.
* fix typing to account for typing hints added in multiple
  upgraded libraries optional values and content of request
  returned as Mapping
* switch to PyJWT 2.* by using non-deprecated "required" claim as
  list rather than separate fields
* add possibiliyt to install providers without constraints
  so that we could avoid errors on conflicting constraints when
  upgrade-to-newer-dependencies is used
* add pre-commit to check that 2.4+ only get_airflow_app is not
  used in providers
* avoid Bad Request in case the request sent to Flask 2.0 is not
  JSon content type
* switch imports of internal classes to direct packages
  where classes are available rather than from "airflow.models" to
  satisfy MyPY
* synchronize changes of FAB Security Manager 4.1.1 with our copy
  of the Security Manager.
* add error handling for a few "None" cases detected by MyPY
* corrected test cases that were broken by immutability of
  Flask 2 objects and better escaping done by Flask 2
* updated test cases to account for redirection to "path" rather
  than full URL by Flask2

Fixes: apache#22397
potiuk added a commit to potiuk/airflow that referenced this issue Jun 22, 2022
The Flask Application Builder have been updated recently to
support a number of newer dependencies. This PR is the
attempt to migrate FAB to newer version.

This includes:

* update setup.py and setup.cfg upper and lower bounds to
  account for proper version of dependencies that
  FAB < 4.0.0 was blocking from upgrade
* added typed Flask application retrieval with a custom
  application fields available for MyPy typing checks.
* fix typing to account for typing hints added in multiple
  upgraded libraries optional values and content of request
  returned as Mapping
* switch to PyJWT 2.* by using non-deprecated "required" claim as
  list rather than separate fields
* add possibiliyt to install providers without constraints
  so that we could avoid errors on conflicting constraints when
  upgrade-to-newer-dependencies is used
* add pre-commit to check that 2.4+ only get_airflow_app is not
  used in providers
* avoid Bad Request in case the request sent to Flask 2.0 is not
  JSon content type
* switch imports of internal classes to direct packages
  where classes are available rather than from "airflow.models" to
  satisfy MyPY
* synchronize changes of FAB Security Manager 4.1.1 with our copy
  of the Security Manager.
* add error handling for a few "None" cases detected by MyPY
* corrected test cases that were broken by immutability of
  Flask 2 objects and better escaping done by Flask 2
* updated test cases to account for redirection to "path" rather
  than full URL by Flask2

Fixes: apache#22397
potiuk added a commit that referenced this issue Jun 22, 2022
* Upgrade FAB to 4.1.1

The Flask Application Builder have been updated recently to
support a number of newer dependencies. This PR is the
attempt to migrate FAB to newer version.

This includes:

* update setup.py and setup.cfg upper and lower bounds to
  account for proper version of dependencies that
  FAB < 4.0.0 was blocking from upgrade
* added typed Flask application retrieval with a custom
  application fields available for MyPy typing checks.
* fix typing to account for typing hints added in multiple
  upgraded libraries optional values and content of request
  returned as Mapping
* switch to PyJWT 2.* by using non-deprecated "required" claim as
  list rather than separate fields
* add possibiliyt to install providers without constraints
  so that we could avoid errors on conflicting constraints when
  upgrade-to-newer-dependencies is used
* add pre-commit to check that 2.4+ only get_airflow_app is not
  used in providers
* avoid Bad Request in case the request sent to Flask 2.0 is not
  JSon content type
* switch imports of internal classes to direct packages
  where classes are available rather than from "airflow.models" to
  satisfy MyPY
* synchronize changes of FAB Security Manager 4.1.1 with our copy
  of the Security Manager.
* add error handling for a few "None" cases detected by MyPY
* corrected test cases that were broken by immutability of
  Flask 2 objects and better escaping done by Flask 2
* updated test cases to account for redirection to "path" rather
  than full URL by Flask2

Fixes: #22397

* fixup! Upgrade FAB to 4.1.1
potiuk added a commit to potiuk/airflow that referenced this issue Jun 29, 2022
The TimedJSONWebSignatureSerializer has been deprecated from the
itsdangerous library and they recommended to use dedicated
libraries for it.

pallets/itsdangerous#129

Since we are going to move to FAB 4+ with apache#22397 where newer version of
itsdangerous is used, we need to switch to another library.

We are already using PyJWT so the choice is obvious.

Additionally to switching, the following improvements were done:

* the use of JWT claims has been fixed to follow JWT standard.
  We were using "iat" header wrongly. The specification of JWT only
  expects the header to be there and be valid UTC timestamp, but the
  claim does not impact maturity of the signature - the signature
  is valid if iat is in the future.
  Instead "nbf" - "not before" claim should be used to verify if the
  request is not coming from the future. We now require all claims
  to be present in the request.

* rather than using salt/signing_context we switched to standard
  JWT "audience" claim (same end result)

* we have now much better diagnostics on the server side of the
  reason why request is forbidden - explicit error messages
  are printed in server logs and details of the exception. This
  is secure, we do not spill the information about the reason
  to the client, it's only available in server logs, so there is
  no risk attacker could use it.

* the JWTSigner is "use-agnostic". We should be able to use the
  same class for any other signatures (Internal API from AIP-44)
  with just different audience

* Short, 5 seconds default clock skew is allowed, to account for
  systems that have "almost" synchronized time

* more tests addded with proper time freezing testing both
  expiry and immaturity of the request

This change is not a breaking one because the JWT authentication
details are not "public API" - but in case someone reverse engineered
our claims and implemented their own log file retrieval, we
should add a change in our changelog - therefore newsfragment
is added.

(cherry picked from commit 1f8e4c9)
potiuk added a commit to potiuk/airflow that referenced this issue Jun 29, 2022
* Upgrade FAB to 4.1.1

The Flask Application Builder have been updated recently to
support a number of newer dependencies. This PR is the
attempt to migrate FAB to newer version.

This includes:

* update setup.py and setup.cfg upper and lower bounds to
  account for proper version of dependencies that
  FAB < 4.0.0 was blocking from upgrade
* added typed Flask application retrieval with a custom
  application fields available for MyPy typing checks.
* fix typing to account for typing hints added in multiple
  upgraded libraries optional values and content of request
  returned as Mapping
* switch to PyJWT 2.* by using non-deprecated "required" claim as
  list rather than separate fields
* add possibiliyt to install providers without constraints
  so that we could avoid errors on conflicting constraints when
  upgrade-to-newer-dependencies is used
* add pre-commit to check that 2.4+ only get_airflow_app is not
  used in providers
* avoid Bad Request in case the request sent to Flask 2.0 is not
  JSon content type
* switch imports of internal classes to direct packages
  where classes are available rather than from "airflow.models" to
  satisfy MyPY
* synchronize changes of FAB Security Manager 4.1.1 with our copy
  of the Security Manager.
* add error handling for a few "None" cases detected by MyPY
* corrected test cases that were broken by immutability of
  Flask 2 objects and better escaping done by Flask 2
* updated test cases to account for redirection to "path" rather
  than full URL by Flask2

Fixes: apache#22397

* fixup! Upgrade FAB to 4.1.1

(cherry picked from commit e2f1950)
@sainman
Copy link

sainman commented Apr 18, 2023

Hi

We are using Apache-airflow v2.5.1 which is using with Flask-AppBuilder version 4.1.4

A few days back Flask-AppBuilder version 4.1.4 is marked as vulnerable with high severity by dependabot.

Now the problem is Airflow v2.5.1 is not supporting Flask-AppBuilder version 4.3.0 ( any >4.1.4 )

Could any one please suggest what could be the possible solution or timeline Airflow will support Flask-AppBuilder version 4.3.0?

Thanks

@potiuk
Copy link
Member Author

potiuk commented Apr 18, 2023

Hi

We are using Apache-airflow v2.5.1 which is using with Flask-AppBuilder version 4.1.4

A few days back Flask-AppBuilder version 4.1.4 is marked as vulnerable with high severity by dependabot.

Now the problem is Airflow v2.5.1 is not supporting Flask-AppBuilder version 4.3.0 ( any >4.1.4 )

Could any one please suggest what could be the possible solution or timeline Airflow will support Flask-AppBuilder version 4.3.0?

Thanks

yes. 2.6.0 beta 1 with FAB 4.3.0 has been already published last week, RC1 is expected this week. Please watch the issues/devlist and help us testing the RC1 as soon as it is released, the more people will test it, the faster we will release 2.6.0 final version.

@sainman
Copy link

sainman commented Apr 20, 2023

thanks, Jarek,

Could you please share the satisfactory link to download beta?

@potiuk
Copy link
Member Author

potiuk commented Apr 20, 2023

You will find it in devlist. Subscribing to devlist is a good idea https://lists.apache.org/thread/yj1xgz2gfjl0ff329nmdyll9d0ctk7x9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dependencies Issues related to dependencies problems kind:meta High-level information important to the community
Projects
None yet
4 participants