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 FAB to 4.1.1 #24399

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Upgrade FAB to 4.1.1 #24399

merged 2 commits into from
Jun 22, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 12, 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: #22397


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@potiuk potiuk force-pushed the fab-app-builder-4-1-1 branch 2 times, most recently from 1905c2b to a7a99b7 Compare June 17, 2022 11:09
@potiuk potiuk mentioned this pull request Jun 17, 2022
1 task
@potiuk potiuk force-pushed the fab-app-builder-4-1-1 branch 8 times, most recently from 05a23d0 to f17ec0e Compare June 19, 2022 15:33
@potiuk potiuk marked this pull request as ready for review June 19, 2022 16:06
@potiuk potiuk changed the title Update FAB to 4.1.1 Upgrade FAB to 4.1.1 Jun 19, 2022
@potiuk potiuk requested a review from josh-fell June 19, 2022 16:08
@potiuk
Copy link
Member Author

potiuk commented Jun 19, 2022

Ok. Seems that all tests are passing and we are ready to start testing FAB 4.1.1. I run airflow with the upgraded FAB and it seems all the basic functionality works. It was a bit simpler than I thought.

I iwll ask interested parties to do more testing.

Upgraded packages

  • Flask-AppBuilder: 3.4.5 -> 4.1.1
  • Flask-JWT-Extended: 3.25.1 -> 4.4.1
  • Flask-Login: 0.4.1 -> 0.6.1
  • Flask-WTF: 0.14.3 -> 0.15.1
  • Flask: 1.1.2 -> 2.1.2
  • Jinja2: 3.0.3 -> 3.1.2
  • MarkupSafe: 2.0.1 -> 2.1.1
  • PyJWT: 1.7.1 -> 2.4.0
  • Werkzeug: 1.0.1 -> 2.1.2
  • itsdangerous: 1.1.0 -> 2.1.2

Removed packages

  • Flask-OpenID
  • jwcrypto
  • python3-openid

@potiuk
Copy link
Member Author

potiuk commented Jun 19, 2022

Would love some reviews - I made some changes to make them "work" but possibly some typing changes especially might be done better - looking forward to the reviews.

@potiuk potiuk requested a review from uranusjr June 19, 2022 16:11
@potiuk
Copy link
Member Author

potiuk commented Jun 19, 2022

All is green. I fixed all the tests and it seems that airflow with upgraded FAB and resulting dependencies (see above) works in basic scenarios..

As discused in #22397 - since you are interested in getting the dependencies upgrade I'd love if you could help with testing:
@lacohen @johannesjung @sk0928 @OfirSabanBitSight @blag @josh-fell @Bowrna @thesuperzapper @gmsantos

I prepared some draft package, constraint file and image to make it easy to test.

Downloads

Python 3.7 image and constraints:

  • Constraints 3.7
  • Image can be pulled via docker pull potiuk/airflow:fab-4-1-1-python3.7

Python 3.10 image and constraints:

  • Constraints 3.10
  • Image can be pulled via docker pull potiuk/airflow:fab-4-1-1-python3.10

Installation

You can install Airflow in your venv via:

  1. downloading the .whl
curl -L https://github.com/potiuk/airflow/releases/download/fab-4-1-1-v0-2/apache_airflow-2.4.0.dev0-py3-none-any.whl \
  -o apache_airflow-2.4.0.dev0-py3-none-any.whl
  1. downloading the constraints.txt file:
curl -L https://github.com/potiuk/airflow/releases/download/fab-4-1-1-v0-2/constraints-fab-4-1-1-python3.7.txt\
   -o constraints-fab-4-1-1-python3.7.txt
  1. installing airflow via
pip install "apache_airflow-2.4.0.dev0-py3-none-any.whl[ADD_YOUR_EXTRAS_HERE]" \
   --constraint constraints-fab-4-1-1-python3.7.txt

They are all "dev" version of upcoming 2.4.0 - we are quite far from the "alpha/beta/rc*" release so this is really "for your eyes and hands only " :).

What to test

I have tested the basics, but I would really appreciate if you could test it with "real configuratios" where FAB features are used. I looked at the breaking changes introduced by FAB and I think it would be great to test:

  • celery log retrieval with secret (PyJWT): we have new mechanism of signing/verifying the log requests
  • FAB authentication integration: OAuth/OpenID and related
  • API and API authentication integration (we have some changes in API implementation) - mostly related to typing
  • LDAP integration (there are some changes/fixes in FAB Security Manager releated to LDAP integration)
  • Some more complex Jinja usages (Jinja is upgraded together with markupsafe library)

I'd really appreciate comments here if you manage to test it in your environment/staging with real configuration! It will be really helpful to get some validation from our users.

@potiuk
Copy link
Member Author

potiuk commented Jun 19, 2022

I am also re-running this PR with "full tests needed" and if needs be I will be able to get images, constraints for all other python versions.

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Jun 19, 2022
@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2022

I updated rich-click to >= 1.5 as it seems that the "yesterday-released" 1.5 version introduced some typing issues

@potiuk potiuk closed this Jun 22, 2022
@potiuk potiuk reopened this 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 potiuk force-pushed the fab-app-builder-4-1-1 branch from 90be60f to 5cfa466 Compare June 22, 2022 14:05
@potiuk potiuk force-pushed the fab-app-builder-4-1-1 branch from 5cfa466 to 7c30228 Compare June 22, 2022 16:42
@potiuk potiuk added this to the Airflow 2.4.0 milestone Jun 22, 2022
@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2022

Merging it now.

@lacohen @johannesjung @sk0928 @OfirSabanBitSight @blag @josh-fell @Bowrna @thesuperzapper - if you have some time to test it and get some feedback here, this might make us speed up some decisisons on when to release it. For now I am marking it as 2.4.0, but, who knows..

@potiuk potiuk merged commit e2f1950 into apache:main Jun 22, 2022
@potiuk potiuk deleted the fab-app-builder-4-1-1 branch June 22, 2022 21:26
ashb added a commit to astronomer/airflow that referenced this pull request Jun 23, 2022
We upgraded flask and werkzeug in apache#24399, and updated the constraints,
but not everyone uses them (such as me in my local virtual environment
when developing) so the min version in setup.cfg has to match as well
@potiuk potiuk modified the milestones: Airflow 2.4.0, Airflow 2.3.3 Jun 23, 2022
potiuk pushed a commit that referenced this pull request Jun 23, 2022
We upgraded flask and werkzeug in #24399, and updated the constraints,
but not everyone uses them (such as me in my local virtual environment
when developing) so the min version in setup.cfg has to match as well
potiuk added a commit to potiuk/airflow that referenced this pull request 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)
potiuk pushed a commit to potiuk/airflow that referenced this pull request Jun 29, 2022
We upgraded flask and werkzeug in apache#24399, and updated the constraints,
but not everyone uses them (such as me in my local virtual environment
when developing) so the min version in setup.cfg has to match as well

(cherry picked from commit 030169d)
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 30, 2022
@ephraimbuddy ephraimbuddy added type:misc/internal Changelog: Misc changes that should appear in change log and removed changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) labels Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to FlaskAppBuilder 4.0.*
4 participants