You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/06/19 17:14:35 UTC
[GitHub] [airflow] potiuk opened a new pull request, #24399: Upgrade FAB to 4.1.1
potiuk opened a new pull request, #24399:
URL: https://github.com/apache/airflow/pull/24399
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: https://github.com/apache/airflow/issues/22397
<!--
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 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 newsfragement file, named `{pr_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 a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903450723
##########
setup.cfg:
##########
@@ -153,8 +134,7 @@ install_requires =
pluggy>=1.0
psutil>=4.2.0
pygments>=2.0.1
- # python daemon crashes with 'socket operation on non-socket' for python 3.8+ in version < 2.2.4
- # https://pagure.io/python-daemon/issue/34
+ pyjwt>=2.0.0
Review Comment:
I just wanted to be sure that we force PyJWT to 2+ because of the "require": ["exp", "iat", "nbf"],` change. I consider it very dangerous change and I want to be sure that we are binding to the right version of PyJWT.
I did not realize that in 2.0 they dropped the "require_iat" aapproach otherwise in #24519 I would have added `PyJWT<2.0.0` because of that. Luckily that was in unreleased Airflow so had no impact, but it coul have been very dangerous if somone by accident had wrong PyJWT.
PyJWT now is much more coupled with us - we are using the PYJWT for signing and verifying log tokens so it makes perfect sense to bind to it explicitly and state the minimum version.
BTW, I tend to not describe "why' we added lower-bound as a comment. It's rather uselss in the "future", because once we lower-bounded something it will never go down (in most circumstances), so I think it's enough to write >N.N.N and keep the reason as history in Git. But if you thin it's worth, we can add also comment here why lower-binding (but I'd prefer to keep the comments when we have upper binding only and remove the comments when we remove it. This is much claner and it gives clear information to the "future us" when we can remove the upper-binidng.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901474850
##########
airflow/www/api/experimental/endpoints.py:
##########
@@ -70,7 +70,8 @@ def add_deprecation_headers(response: Response):
return response
-api_experimental.after_request(add_deprecation_headers)
+# This is really experimental. We do not care too much about typing here
+api_experimental.after_request(add_deprecation_headers) # type: ignore[arg-type]
Review Comment:
I tihnk we already do :). https://airflow.apache.org/docs/apache-airflow/stable/deprecated-rest-api-ref.html
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903419806
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
(btw I am still not super happy with the name so if there are better candidates, I am all ears :)
--
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 closed pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #24399: Upgrade FAB to 4.1.1
URL: https://github.com/apache/airflow/pull/24399
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903388349
##########
airflow/api_connexion/schemas/dag_schema.py:
##########
@@ -83,7 +83,8 @@ def get_owners(obj: DagModel):
@staticmethod
def get_token(obj: DagModel):
"""Return file token"""
- serializer = URLSafeSerializer(conf.get('webserver', 'secret_key'))
+ # the secret key is always available, so we can ignore Optional here
+ serializer = URLSafeSerializer(conf.get('webserver', 'secret_key')) # type: ignore[arg-type]
Review Comment:
We can use `get_mandatory_value` here.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901461431
##########
setup.cfg:
##########
@@ -101,57 +101,32 @@ install_requires =
cryptography>=0.9.3
deprecated>=1.2.13
dill>=0.2.2
- # Flask and all related libraries are limited to below 2.0.0 because we expect it to introduce
- # Serious breaking changes. Flask 2.0 has been introduced in May 2021 and 2.0.2 version is available
- # now (Feb 2022): TODO: we should attempt to migrate to Flask 2 and all below flask libraries soon.
- flask>=1.1.0, <2.0
- # We are tightly coupled with FAB version because we vendored in part of FAB code related to security manager
- # This is done as part of preparation to removing FAB as dependency, but we are not ready for it yet
- # Every time we update FAB version here, please make sure that you review the classes and models in
- # `airflow/www/fab_security` with their upstream counterparts. In particular, make sure any breaking changes,
- # for example any new methods, are accounted for.
Review Comment:
I think we should keep this block to explain the `==` pin.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1163615284
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..
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159810695
I updated packages/constraints after rebasing on tpp of #24516 -to fix `authlib` version.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901473144
##########
tests/www/views/test_views_log.py:
##########
@@ -461,7 +461,7 @@ def test_redirect_to_external_log_with_local_log_handler(log_admin_client, task_
)
response = log_admin_client.get(url)
assert 302 == response.status_code
- assert 'http://localhost/home' == response.headers['Location']
+ assert '/home' == response.headers['Location']
Review Comment:
I tihnk not - it will work in general - it will only redirect using path rather than full URL, I guess in any browser it will be treated properly and redirection will work as the browser will automatically pre-pend the host if it's the same as previously.
I can't imagine scenario where it will create a problem (unless I am mistaken about the circumstances of this different Flask behaviour)
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903385891
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
`get_mapping_from_request()` is called many times in this function, they can be merged.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901458182
##########
airflow/www/api/experimental/endpoints.py:
##########
@@ -70,7 +70,8 @@ def add_deprecation_headers(response: Response):
return response
-api_experimental.after_request(add_deprecation_headers)
+# This is really experimental. We do not care too much about typing here
+api_experimental.after_request(add_deprecation_headers) # type: ignore[arg-type]
Review Comment:
Maybe we should start calling this _deprecated_ API. (It’d still show _experiemental_ in code for compatibility for now, but we can call it deprecated in docs and comments.)
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901474033
##########
airflow/www/auth.py:
##########
@@ -37,7 +37,11 @@ def decorated(*args, **kwargs):
appbuilder = current_app.appbuilder
dag_id = (
- request.args.get("dag_id") or request.form.get("dag_id") or (request.json or {}).get("dag_id")
+ request.args.get("dag_id")
+ or request.form.get("dag_id")
+ or request.is_json
+ and request.json.get("dag_id")
+ or None
)
Review Comment:
Yeah. Readability is king
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903391191
##########
airflow/www/fab_security/manager.py:
##########
@@ -1042,6 +1048,12 @@ def auth_user_ldap(self, username, password):
ldap.set_option(ldap.OPT_X_TLS_CERTFILE, self.auth_ldap_tls_certfile)
if self.auth_ldap_tls_keyfile:
ldap.set_option(ldap.OPT_X_TLS_KEYFILE, self.auth_ldap_tls_keyfile)
+ if self.auth_ldap_allow_self_signed:
+ ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_ALLOW)
+ ldap.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
+ elif self.auth_ldap_tls_demand:
+ ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_DEMAND)
+ ldap.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
Review Comment:
Why is this change needed?
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159766812
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
* PyGithub: 1.54.1 -> 1.55
* 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
* github3.py
* jwcrypto
* python3-openid
--
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] gmsantos commented on pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
gmsantos commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159782603
🤞 for python 3.10 constraints
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1160205587
Cool. I will add a newsfragment note about it.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901460190
##########
airflow/www/auth.py:
##########
@@ -37,7 +37,11 @@ def decorated(*args, **kwargs):
appbuilder = current_app.appbuilder
dag_id = (
- request.args.get("dag_id") or request.form.get("dag_id") or (request.json or {}).get("dag_id")
+ request.args.get("dag_id")
+ or request.form.get("dag_id")
+ or request.is_json
+ and request.json.get("dag_id")
+ or None
)
Review Comment:
Let’s add parentheses around `request.is_json and request.json.get("dag_id")` to make this easier to understand
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903424177
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
This entire operation is pretty hack-ish anyway so I’m not that troubled with the name, to be honest.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903396793
##########
airflow/www/views.py:
##########
@@ -1463,7 +1463,10 @@ def rendered_k8s(self, session: Session = NEW_SESSION):
map_index = request.args.get('map_index', -1, type=int)
logging.info("Retrieving rendered templates.")
- dag: DAG = current_app.dag_bag.get_dag(dag_id)
+ dag: DAG = get_airflow_app().dag_bag.get_dag(dag_id)
+ if task_id is None:
+ logging.warning("Task id not passed in the request")
+ abort(400)
Review Comment:
This can be moved much earlier in the function to avoid unnecessary work (getting a DAG is not trivial).
--
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 closed pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #24399: Upgrade FAB to 4.1.1
URL: https://github.com/apache/airflow/pull/24399
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903470398
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
BTW. The *reall* reason for this hack-ishnes is Flask choice of getting current request by `from flask import request` . This is the root of all evil here :)
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903431656
##########
airflow/utils/jwt_signer.py:
##########
@@ -73,9 +73,7 @@ def verify_token(self, token: str) -> Dict[str, Any]:
algorithms=[self._algorithm],
options={
"verify_signature": True,
- "require_exp": True,
- "require_iat": True,
- "require_nbf": True,
+ "require": ["exp", "iat", "nbf"],
Review Comment:
It's a change in PyJWT 1-> 2. I mentioned it in the commit message:
```
switch to PyJWT 2.* by using non-deprecated "required" claim as
list rather than separate fields
```
But the detailed link is here: https://pyjwt.readthedocs.io/en/stable/changelog.html#dropped-deprecated-require-options-in-jwt-decode
The problem was that PyJWT used in FAB 3 did not have it deprecated (and dictionary of claims did not work - you hadd to use "require_iat" etc. - I tried dict before but it was not implemented yet in the older PyJWT we had).
On the other hand, the PyJWT upgrade used in FAB 4 "jumps" the deprecation - so PyJWT used by FAB not only deprecated the "require_*" parameters but also managed to remove them altogether so it stopped working :). So I had to implement it in old way in #24519 and switch to new way here.
Side comment: I am actually super glad I added tests for it when I implemented it in #24519 , because the change was TERRIBLY breaking. Suddenly your "requirements" stopped being respected and were suddenly dropped silently (!) - no warning (opening the way to requests that had no validity requirements). I was thinking about raising an issue to PyJWT. They should - IMHO fail signers that still use require_nnn, rather than the dictionary - that would have been much safer for their users. But it was a change impemented in December, so that ship has long sailed.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903451884
##########
airflow/www/views.py:
##########
@@ -1463,7 +1463,10 @@ def rendered_k8s(self, session: Session = NEW_SESSION):
map_index = request.args.get('map_index', -1, type=int)
logging.info("Retrieving rendered templates.")
- dag: DAG = current_app.dag_bag.get_dag(dag_id)
+ dag: DAG = get_airflow_app().dag_bag.get_dag(dag_id)
+ if task_id is None:
+ logging.warning("Task id not passed in the request")
+ abort(400)
Review Comment:
Good idea. moved.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901473713
##########
setup.cfg:
##########
@@ -101,57 +101,32 @@ install_requires =
cryptography>=0.9.3
deprecated>=1.2.13
dill>=0.2.2
- # Flask and all related libraries are limited to below 2.0.0 because we expect it to introduce
- # Serious breaking changes. Flask 2.0 has been introduced in May 2021 and 2.0.2 version is available
- # now (Feb 2022): TODO: we should attempt to migrate to Flask 2 and all below flask libraries soon.
- flask>=1.1.0, <2.0
- # We are tightly coupled with FAB version because we vendored in part of FAB code related to security manager
- # This is done as part of preparation to removing FAB as dependency, but we are not ready for it yet
- # Every time we update FAB version here, please make sure that you review the classes and models in
- # `airflow/www/fab_security` with their upstream counterparts. In particular, make sure any breaking changes,
- # for example any new methods, are accounted for.
Review Comment:
Yep. To much of a cleanup :)
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159778271
All is green. I fixed all the tests and it seems that basic airflow with upgraded FAB and resulting dependencies (see above).
As discused in #22397 - since you are interested in getting the dependencies upgrade I'd love if you could help with t
@lacohen @johannesjung @sk0928 @OfirSabanBitSight @blag @josh-fell @Bowrna @thesuperzapper @gmsantos
I prepared some draft package, constraint and image to make it easy for testing. The constraints adn image are for Python 3.7.
* package: https://github.com/potiuk/airflow/releases/download/fab-app-builder-4-1-1-v0-1/apache_airflow-2.4.0.dev0-py3-none-any.whl
* constraints: https://github.com/potiuk/airflow/releases/download/fab-app-builder-4-1-1-v0-1/constraints-fab-4-1.txt
* Image can be pulled via `docker pull potiuk/airflow:fab-4-1-1`
You can install Airflow in your venv via:
1) downloading the .whl `curl -L https://github.com/potiuk/airflow/releases/download/fab-app-builder-4-1-1-v0-1/apache_airflow-2.4.0.dev0-py3-none-any.whl -o apache_airflow-2.4.0.dev0-py3-none-any.whl`
2) downloading the constraints.txt file `curl -L https://github.com/potiuk/airflow/releases/download/fab-app-builder-4-1-1-v0-1/constraints-fab-4-1.txt -o constraints-fab-4-1.txt`
3) installing airflow via `pip install "apache_airflow-2.4.0.dev0-py3-none-any.whl[ADD_YOUR_EXTRAS_HERE]" --constraint constraints-fab-4-1.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 " :).
I have tested the basic, 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 fron our users.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901530890
##########
airflow/www/api/experimental/endpoints.py:
##########
@@ -70,7 +70,8 @@ def add_deprecation_headers(response: Response):
return response
-api_experimental.after_request(add_deprecation_headers)
+# This is really experimental. We do not care too much about typing here
+api_experimental.after_request(add_deprecation_headers) # type: ignore[arg-type]
Review Comment:
This comment still says experiemental though
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1162949498
I updated rich-click to >= 1.5 as it seems that the "yesterday-released" 1.5 version introduced some typing issues
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903390558
##########
airflow/www/fab_security/manager.py:
##########
@@ -654,6 +654,18 @@ def get_oauth_user_info(self, provider, resp):
"email": data.get("email", ""),
"role_keys": data.get("groups", []),
}
+ # for Keycloak
+ if provider in ["keycloak", "keycloak_before_17"]:
+ me = self.appbuilder.sm.oauth_remotes[provider].get("openid-connect/userinfo")
+ me.raise_for_status()
+ data = me.json()
+ log.debug("User info from Keycloak: %s", data)
+ return {
+ "username": data.get("preferred_username", ""),
+ "first_name": data.get("given_name", ""),
+ "last_name": data.get("family_name", ""),
+ "email": data.get("email", ""),
+ }
Review Comment:
Why is this needed now (but not before)?
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903385891
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
`get_mapping_from_request()` is called twice in this function, they can be merged.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1161967440
Any more feedback @lacohen @johannesjung @sk0928 @OfirSabanBitSight @blag @josh-fell @Bowrna @thesuperzapper ? - I think some of you were eager to get it due to dependencies - the sooner we hear from you, the sooner we will merge it and the higher chance it will have to be merged soon.
Maybe even we can decide to release it in 2.3.3 if we hear from you that it all looks good. So now you have a real chance of speeding up the change you advocated for. Don't loose the chance.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24399:
URL: https://github.com/apache/airflow/pull/24399
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903466458
##########
airflow/www/fab_security/manager.py:
##########
@@ -654,6 +654,18 @@ def get_oauth_user_info(self, provider, resp):
"email": data.get("email", ""),
"role_keys": data.get("groups", []),
}
+ # for Keycloak
+ if provider in ["keycloak", "keycloak_before_17"]:
+ me = self.appbuilder.sm.oauth_remotes[provider].get("openid-connect/userinfo")
+ me.raise_for_status()
+ data = me.json()
+ log.debug("User info from Keycloak: %s", data)
+ return {
+ "username": data.get("preferred_username", ""),
+ "first_name": data.get("given_name", ""),
+ "last_name": data.get("family_name", ""),
+ "email": data.get("email", ""),
+ }
Review Comment:
Explained below (FAB 4.1.1 Security Manager synchronization).
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901528130
##########
tests/api_connexion/schemas/test_dag_schema.py:
##########
@@ -30,7 +30,8 @@
from airflow.configuration import conf
from airflow.models import DagModel, DagTag
-SERIALIZER = URLSafeSerializer(conf.get('webserver', 'SECRET_KEY'))
+# the secret key is always available, so we can ignore Optional here
+SERIALIZER = URLSafeSerializer(conf.get('webserver', 'secret_key')) # type: ignore[arg-type]
Review Comment:
I actually went ahead and changed it. It's sooo much better than dealing with the type ignores :)
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901622395
##########
airflow/www/api/experimental/endpoints.py:
##########
@@ -70,7 +70,8 @@ def add_deprecation_headers(response: Response):
return response
-api_experimental.after_request(add_deprecation_headers)
+# This is really experimental. We do not care too much about typing here
+api_experimental.after_request(add_deprecation_headers) # type: ignore[arg-type]
Review Comment:
crossed my mind indeed. Fixed.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1160418980
Added newsfragment describing the scope of the change and warning against the configuration change.
Looking forward to more tests: @lacohen @johannesjung @sk0928 @OfirSabanBitSight @blag @josh-fell @Bowrna @thesuperzapper :)
--
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] uranusjr commented on pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1162763562
Mostly lgtm, just some minor comments.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903388948
##########
airflow/utils/jwt_signer.py:
##########
@@ -73,9 +73,7 @@ def verify_token(self, token: str) -> Dict[str, Any]:
algorithms=[self._algorithm],
options={
"verify_signature": True,
- "require_exp": True,
- "require_iat": True,
- "require_nbf": True,
+ "require": ["exp", "iat", "nbf"],
Review Comment:
Is this a change in FAB 4?
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903442803
##########
airflow/www/fab_security/manager.py:
##########
@@ -1042,6 +1048,12 @@ def auth_user_ldap(self, username, password):
ldap.set_option(ldap.OPT_X_TLS_CERTFILE, self.auth_ldap_tls_certfile)
if self.auth_ldap_tls_keyfile:
ldap.set_option(ldap.OPT_X_TLS_KEYFILE, self.auth_ldap_tls_keyfile)
+ if self.auth_ldap_allow_self_signed:
+ ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_ALLOW)
+ ldap.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
+ elif self.auth_ldap_tls_demand:
+ ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_DEMAND)
+ ldap.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
Review Comment:
This i part of synchronization with FAB 4.1.1 changes.
I basically strictly follow the comment about our Security Manager being essentially coupled with the FAB one. https://github.com/apache/airflow/blob/main/setup.cfg#L108
Not my way of doing it - but seems we chose the way to synchronize manually all the changes in security manager at teh very moment (and in the same commit) when we update FAB version (hence the == and the comment in setup,cfg).
Following that, this commit contains all the relevat changes in the Security Manager that were implemented in FAB since last "upgrade" that are currently released in FAB 4.1.1.
Those were three changes i found relevant:
* LDAP sequence (apparently a fix) - https://github.com/apache/airflow/blob/main/setup.cfg#L108 (this is the change you commented on)
* KeyCloak provider added https://github.com/dpgaspar/Flask-AppBuilder/commit/b21010249a33f657897d26b0a7bec93908290266)
* Upgrading pyJWT https://github.com/dpgaspar/Flask-AppBuilder/commit/2e2931f2fc470a4f1756a0c39e0cc04b86818d84
All of them bring our Security Manager (as far as I can tell) to the parity with the FAB one (there are other modifications and removals of unused parts in our Security Manager before - so they are not identical, but assuming previous changes properly syncrhronized to the previous version of FAB - those are the only relevant changes since.
Again - not the way I'd do it, but I guess we need to live with it
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1162822926
@uranusjr - I pushed the fixup commit with changes requested + added explanation (and rebased it on top of main
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159767250
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.
--
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] gmsantos commented on pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
gmsantos commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1160204350
@potiuk for the FAB authentication integration, it worked here using Azure AD as the service provider. As noted in https://github.com/dpgaspar/Flask-AppBuilder/issues/1861, the only change required was to add `server_metadata_url` to the auth configuration:
```
OAUTH_PROVIDERS = [
{
"name": "azure",
"icon": "fa-windows",
"token_key": "access_token",
"remote_app": {
"client_id": os.getenv('AZURE_APP_ID'),
"client_secret": os.getenv('AZURE_APP_SECRET'),
"api_base_url": f"https://login.microsoftonline.com/{AZURE_TENANT_ID}/oauth2/v2.0",
"client_kwargs": {
"scope": "profile openid",
"resource": os.getenv('AZURE_APP_ID'),
},
"request_token_url": None,
"access_token_url": f"https://login.microsoftonline.com/{AZURE_TENANT_ID}/oauth2/v2.0/token",
"authorize_url": f"https://login.microsoftonline.com/{AZURE_TENANT_ID}/oauth2/v2.0/authorize",
"server_metadata_url": f"https://login.microsoftonline.com/{AZURE_TENANT_ID}/v2.0/.well-known/openid-configuration",
}
}
```
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159778663
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.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901463501
##########
tests/api_connexion/schemas/test_dag_schema.py:
##########
@@ -30,7 +30,8 @@
from airflow.configuration import conf
from airflow.models import DagModel, DagTag
-SERIALIZER = URLSafeSerializer(conf.get('webserver', 'SECRET_KEY'))
+# the secret key is always available, so we can ignore Optional here
+SERIALIZER = URLSafeSerializer(conf.get('webserver', 'secret_key')) # type: ignore[arg-type]
Review Comment:
It’s probably a good idea to make the secret key a pytest param. I’ll do that after this PR is in.
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r901465537
##########
tests/www/views/test_views_log.py:
##########
@@ -461,7 +461,7 @@ def test_redirect_to_external_log_with_local_log_handler(log_admin_client, task_
)
response = log_admin_client.get(url)
assert 302 == response.status_code
- assert 'http://localhost/home' == response.headers['Location']
+ assert '/home' == response.headers['Location']
Review Comment:
Should this be considered breaking compatibility?
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24399:
URL: https://github.com/apache/airflow/pull/24399#issuecomment-1159785466
> 🤞 for python 3.10 constraints
Updated with 3.10 variants as well ^^ see the updated description @gmsantos
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903397755
##########
airflow/www/views.py:
##########
@@ -3541,6 +3544,10 @@ def extra_links(self, session: "Session" = NEW_SESSION):
response = jsonify({'url': None, 'error': 'Task Instances not found'})
response.status_code = 404
return response
+ if link_name is None:
+ response = jsonify({'url': None, 'error': 'Link name not passed'})
+ response.status_code = 400
+ return response
Review Comment:
Same here (actually I _think_ we can use `requests.args["link_name"]` to avoid this check?
##########
airflow/www/views.py:
##########
@@ -3541,6 +3544,10 @@ def extra_links(self, session: "Session" = NEW_SESSION):
response = jsonify({'url': None, 'error': 'Task Instances not found'})
response.status_code = 404
return response
+ if link_name is None:
+ response = jsonify({'url': None, 'error': 'Link name not passed'})
+ response.status_code = 400
+ return response
Review Comment:
Same here; actually I _think_ we can use `requests.args["link_name"]` to avoid this check?
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903467211
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
It is
--
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] uranusjr commented on a diff in pull request #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903393663
##########
setup.cfg:
##########
@@ -153,8 +134,7 @@ install_requires =
pluggy>=1.0
psutil>=4.2.0
pygments>=2.0.1
- # python daemon crashes with 'socket operation on non-socket' for python 3.8+ in version < 2.2.4
- # https://pagure.io/python-daemon/issue/34
+ pyjwt>=2.0.0
Review Comment:
Why is this addition needed?
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903419247
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -87,7 +88,10 @@ def patch_pool(
"""Update a pool"""
# Only slots can be modified in 'default_pool'
try:
- if pool_name == Pool.DEFAULT_POOL_NAME and request.json["name"] != Pool.DEFAULT_POOL_NAME:
+ if (
+ pool_name == Pool.DEFAULT_POOL_NAME
+ and get_mapping_from_request()["name"] != Pool.DEFAULT_POOL_NAME
Review Comment:
Yeah. I also do not like the "get_mapping_from_request" name - I cahnge both package and method_name to <request_dict.get_json_request_dit()`. Might be less accurate but is more readable and explicit.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903420229
##########
airflow/api_connexion/schemas/dag_schema.py:
##########
@@ -83,7 +83,8 @@ def get_owners(obj: DagModel):
@staticmethod
def get_token(obj: DagModel):
"""Return file token"""
- serializer = URLSafeSerializer(conf.get('webserver', 'secret_key'))
+ # the secret key is always available, so we can ignore Optional here
+ serializer = URLSafeSerializer(conf.get('webserver', 'secret_key')) # type: ignore[arg-type]
Review Comment:
Ah right. Not a provider.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903442803
##########
airflow/www/fab_security/manager.py:
##########
@@ -1042,6 +1048,12 @@ def auth_user_ldap(self, username, password):
ldap.set_option(ldap.OPT_X_TLS_CERTFILE, self.auth_ldap_tls_certfile)
if self.auth_ldap_tls_keyfile:
ldap.set_option(ldap.OPT_X_TLS_KEYFILE, self.auth_ldap_tls_keyfile)
+ if self.auth_ldap_allow_self_signed:
+ ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_ALLOW)
+ ldap.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
+ elif self.auth_ldap_tls_demand:
+ ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_DEMAND)
+ ldap.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
Review Comment:
This i part of synchronization with FAB 4.1.1 changes.
I basically strictly follow the comment about Security Manager being strictly coupled with https://github.com/apache/airflow/blob/main/setup.cfg#L108
Not my way of doing it - but seems we chose the way to synchronize manually all the changes in security manager at teh very moment (and in the same commit) when we update FAB version (hence the == and the comment in setup,cfg).
Following that, this commit contains all the relevat changes in the Security Manager that were implemented in FAB since last "upgrade" that are currently released in FAB 4.1.1.
Those were three changes i found relevant:
* LDAP sequence (apparently a fix) - https://github.com/apache/airflow/blob/main/setup.cfg#L108 (this is the change you commented on)
* KeyCloak provider added https://github.com/dpgaspar/Flask-AppBuilder/commit/b21010249a33f657897d26b0a7bec93908290266)
* Upgrading pyJWT https://github.com/dpgaspar/Flask-AppBuilder/commit/2e2931f2fc470a4f1756a0c39e0cc04b86818d84
All of them bring our Security Manager (as far as I can tell) to the parity with the FAB one (there are other modifications and removals of unused parts in our Security Manager before - so they are not identical, but assuming previous changes properly syncrhronized to the previous version of FAB - those are the only relevant changes since.
Again - not the way I'd do it, but I guess we need to live with it
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903456893
##########
airflow/www/views.py:
##########
@@ -3541,6 +3544,10 @@ def extra_links(self, session: "Session" = NEW_SESSION):
response = jsonify({'url': None, 'error': 'Task Instances not found'})
response.status_code = 404
return response
+ if link_name is None:
+ response = jsonify({'url': None, 'error': 'Link name not passed'})
+ response.status_code = 400
+ return response
Review Comment:
I prefer to explicitly handle the error. The `requests.args["link_name"] will fail with ugly 500 error with - usually - cryptic stackrace, but this is really 400 -> bad request, and we should tell the user immediately what is wrong.
But yeah. It can be moved a little higher (I just moved it and also moved link_name retrieval just before that check). We cannot move it up to the top (I think) , because lack of the valid "dag_id" /"task_id" combination is a bit more important (IMHO) than lack of link_name, and should be surfaced earlier.
##########
airflow/www/views.py:
##########
@@ -3541,6 +3544,10 @@ def extra_links(self, session: "Session" = NEW_SESSION):
response = jsonify({'url': None, 'error': 'Task Instances not found'})
response.status_code = 404
return response
+ if link_name is None:
+ response = jsonify({'url': None, 'error': 'Link name not passed'})
+ response.status_code = 400
+ return response
Review Comment:
I prefer to explicitly handle the error. The `requests.args["link_name"] will fail with ugly HTTP 500 with - usually - cryptic stackrace, but this is really 400 -> bad request, and we should tell the user immediately what is wrong.
But yeah. It can be moved a little higher (I just moved it and also moved link_name retrieval just before that check). We cannot move it up to the top (I think) , because lack of the valid "dag_id" /"task_id" combination is a bit more important (IMHO) than lack of link_name, and should be surfaced earlier.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903456893
##########
airflow/www/views.py:
##########
@@ -3541,6 +3544,10 @@ def extra_links(self, session: "Session" = NEW_SESSION):
response = jsonify({'url': None, 'error': 'Task Instances not found'})
response.status_code = 404
return response
+ if link_name is None:
+ response = jsonify({'url': None, 'error': 'Link name not passed'})
+ response.status_code = 400
+ return response
Review Comment:
I prefer to explicitly handle the error `requests.args["link_name"] will fail with ugly 500 error with - usually - cryptic stackrace, but this is really 400 -> bad request, and we should tell the user immediately what is wrong.
But yeah. It can be moved a little higher (I just moved it and also moved link_name retrieval just before that check). We cannot move it up to the top (I think) , because lack of the valid "dag_id" /"task_id" combination is a bit more important (IMHO) than lack of link_name, and should be surfaced earlier.
--
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 #24399: Upgrade FAB to 4.1.1
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903431656
##########
airflow/utils/jwt_signer.py:
##########
@@ -73,9 +73,7 @@ def verify_token(self, token: str) -> Dict[str, Any]:
algorithms=[self._algorithm],
options={
"verify_signature": True,
- "require_exp": True,
- "require_iat": True,
- "require_nbf": True,
+ "require": ["exp", "iat", "nbf"],
Review Comment:
It's a change in PyJWT 1-> 2. I mentioned it in the commit message:
```
switch to PyJWT 2.* by using non-deprecated "required" claim as
list rather than separate fields
```
But the detailed changelog link is here: https://pyjwt.readthedocs.io/en/stable/changelog.html#dropped-deprecated-require-options-in-jwt-decode
The problem was that PyJWT used in FAB 3 did not have it deprecated (and dictionary of claims did not work - you hadd to use "require_iat" etc. - I tried dict before but it was not implemented yet in the older PyJWT we had).
On the other hand, the PyJWT upgrade used in FAB 4 "jumps" the deprecation - so PyJWT used by FAB not only deprecated the "require_*" parameters but also managed to remove them altogether so it stopped working :). So I had to implement it in old way in #24519 and switch to new way here.
Side comment: I am actually super glad I added tests for it when I implemented it in #24519 , because the change was TERRIBLY breaking. Suddenly your "requirements" stopped being respected and were suddenly dropped silently (!) - no warning (opening the way to requests that had no validity requirements). I was thinking about raising an issue to PyJWT. They should - IMHO fail signers that still use require_nnn, rather than the dictionary - that would have been much safer for their users. But it was a change impemented in December, so that ship has long sailed.
--
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