You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "VladaZakharova (via GitHub)" <gi...@apache.org> on 2023/12/04 14:42:12 UTC
[PR] Attempt to update Connexion library to version 3 [airflow]
VladaZakharova opened a new pull request, #36052:
URL: https://github.com/apache/airflow/pull/36052
@RobbeSneyders @vincbeck @potiuk
Hi all!
I have opened a PR in main repository so more people can be involved in this discussion :)
based on the conversation in this issue https://github.com/apache/airflow/issues/35234, i am trying to update BaseAuthManager() class to use FlaskApp instead of FlaskApi as suggested here: https://github.com/apache/airflow/issues/35234#issuecomment-1793449425
The problem that i have faced is trying to update BaseAuthManager class will lead to errors on the users side who actually use this class with FlaskApi return type from method get_api_endpoints(). Another idea from the Robbe came to fix this problem by combining functionality from Connexion 2 and Connexion 3, but i am not sure how it will look like. Any ideas will be appreciated!
Thank you :)
P.S. I am not an expert in this specific part of Airflow, but will be happy to become one :D
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1898634172
@RobbeSneyders , hope you can help :)
```
TypeError: get() got an unexpected keyword argument 'environ_overrides' this error is the most frequent now. For fixing it we need to find the equivalent for this variable environ_overrides in connexion's test_client(). I didn't find any equivalent in Connexion and Starlette docs. @RobbeSneyders could you help us with that?
```
I think we also need some help in this one question, since it requires some understanding of Connexion library in general.
For tests we also need ability to override variables, what was done in previous version of Connexion using `environ_overrides` variable. For now, using old variable we get error `TypeError: get() got an unexpected keyword argument 'environ_overrides'`.
We didn't find anything about replacement for this variable in the documentation for new version, so maybe you can help us with this?
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1930129837
Hey @potiuk @vincbeck !
The latest update on this issue and the list of the errors we need to fix to be able to upgrade Connexion:
1. Based on this discussion:
> > > > Update init_api_auth_provider function.
> > > > In get_api_endpoints function we use connexion_app.add_api and expect FlaskApi as a return object, but add_api function always return None. Previously we use FlaskApi object for taking blueprints and then extend our base_paths here: https://github.com/apache/airflow/pull/36052/files#diff-a1cf5a1eea3231a292d789d82df7943bfe8fa89ca955404b2f9cdaa25e08fa80R309
> > > > @vincbeck is it important to save this logic for fab_auth_manager or is it enough to register blueprints under connexion_app.add_api functionality?
> > >
> > >
> > > I am not sure this is relevant anymore. The signature of `get_api_endpoints` changed. See [here](https://github.com/apache/airflow/blob/main/airflow/providers/fab/auth_manager/fab_auth_manager.py#L149). It should simplify the problem.
> >
> >
> > Hi @vincbeck ! I think the question here was about the line where we are appending `base_paths` variable with new registered blueprint. With the new version of Connexion the way of registering blueprints will be changed to register in connexion_app. So the method `get_api_endpoints()` with new version of Connexion will return None instead of blueprint object. So, if we can't return blueprint from the `get_api_endpoints()` method, the `base_paths` variable couldn't be extended. Should we consider still updating `base_paths` with blueprint object? Is this variable used later somewhere? The if statement here shows that we still can continue without adding blueprint to this list of base_paths. If we need this variable later, is there other way to add blueprint not using value returned by `get_api_endpoints()` since now it will return None?
> > ```
> > def init_api_auth_provider(connexion_app: connexion.FlaskApp):
> > """Initialize the API offered by the auth manager."""
> > auth_mgr = get_auth_manager()
> > blueprint = auth_mgr.get_api_endpoints(connexion_app)
> > if blueprint:
> > base_paths.append(blueprint.url_prefix if blueprint.url_prefix else "")
> > flask_app = connexion_app.app
> > flask_app.extensions["csrf"].exempt(blueprint)
> > ```
>
> I think the way we should do it is really close to what you have now with few differences:
>
> * Rename `get_api_endpoints` to `set_api_endpoints`. The return type should be updated to `None`. Documentation should be updated as well to something like "Set API endpoint(s) definition for the auth manager.". This is a breaking change but nobody uses this interface yet, so it is a good time to do it.
> * This piece of code `flask_app.extensions["csrf"].exempt(blueprint)` should be moved in the `set_api_endpoints` method using `appbuilder.app.extensions["csrf"].exempt(api.blueprint)`
Blueprint is now can't be retrieved from connexion app and there will be always None.
Find a way to add csrf extension to a newly created blueprint using connexion: to retrieve blueprint object from connexion_app variable to save the current logic (`flask_app.extensions["csrf"].exempt(blueprint)`) or find a way to add this extension on connexion level(check the documentation for available options).
2. Find a way to replace legacy `environ_overrides` method from old version of Connexion to fix the problem in tests
`TypeError: get() got an unexpected keyword argument 'environ_overrides'`. Contact @RobbeSneyders for some help. Currently we didn't find anything in their documentation regarding new way how to override env variables.
3. Fix unit tests which use app.
After connexion update create_app function returns connexion_app instead of flask_app, but unit tests in most cases expect flask_app. In this case we need to update all our unit tests (we have ~1000 tests).
As a reference of existing errors please check the logs from the last build here:
- In `Providers-AWS,Google` part fix problem with legacy `environ_overrides` as mentioned in step#2.
https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:3267
- In `Core` part there is a failing test related to incorrect encoding in the current implementation (check method [here](https://github.com/apache/airflow/blob/main/airflow/utils/helpers.py#L243) that can be related to the error here, maybe we can change the return value from `flask` to `connexion_app`. Needs some investigation. https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:5821
- In `WWW` part fix failing tests with error `AttributeError: 'FlaskApp' object has no attribute 'test_request_context'`. Please, check the documentation [here](https://connexion.readthedocs.io/en/latest/testing.html)
https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:6496
- In `CLI` part fix asserts that are related to new way of calling Gunicorn command:
https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:10464
- In `Other` part fix errors connected to the incorrect header that is generated. This step requires some additional investigation, since the problem could be not connected to incorrect test, but incorrect setting of headers.
https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:11744
- In `Providers-AWS` part fix the problem with `AttributeError: 'FlaskApp' object has no attribute 'test_request_context'` error. Please, check the documentation [here](https://connexion.readthedocs.io/en/latest/testing.html)
https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:20406
- In `Providers-Google` part fix the problem with `AttributeError: 'Response' object has no attribute 'data'` that occurres because Response object from connexion is not the same then from flask.
https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:22081
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1898786641
> > > Update init_api_auth_provider function.
> > > In get_api_endpoints function we use connexion_app.add_api and expect FlaskApi as a return object, but add_api function always return None. Previously we use FlaskApi object for taking blueprints and then extend our base_paths here: https://github.com/apache/airflow/pull/36052/files#diff-a1cf5a1eea3231a292d789d82df7943bfe8fa89ca955404b2f9cdaa25e08fa80R309
> > > @vincbeck is it important to save this logic for fab_auth_manager or is it enough to register blueprints under connexion_app.add_api functionality?
> >
> >
> > I am not sure this is relevant anymore. The signature of `get_api_endpoints` changed. See [here](https://github.com/apache/airflow/blob/main/airflow/providers/fab/auth_manager/fab_auth_manager.py#L149). It should simplify the problem.
>
> Hi @vincbeck ! I think the question here was about the line where we are appending `base_paths` variable with new registered blueprint. With the new version of Connexion the way of registering blueprints will be changed to register in connexion_app. So the method `get_api_endpoints()` with new version of Connexion will return None instead of blueprint object. So, if we can't return blueprint from the `get_api_endpoints()` method, the `base_paths` variable couldn't be extended. Should we consider still updating `base_paths` with blueprint object? Is this variable used later somewhere? The if statement here shows that we still can continue without adding blueprint to this list of base_paths. If we need this variable later, is there other way to add blueprint not using value returned by `get_api_endpoints()` since now it will return None?
>
> ```
> def init_api_auth_provider(connexion_app: connexion.FlaskApp):
> """Initialize the API offered by the auth manager."""
> auth_mgr = get_auth_manager()
> blueprint = auth_mgr.get_api_endpoints(connexion_app)
> if blueprint:
> base_paths.append(blueprint.url_prefix if blueprint.url_prefix else "")
> flask_app = connexion_app.app
> flask_app.extensions["csrf"].exempt(blueprint)
> ```
I think the way we should do it is really close to what you have now with few differences:
- Rename `get_api_endpoints` to `set_api_endpoints`. The return type should be updated to `None`. Documentation should be updated as well to something like "Set API endpoint(s) definition for the auth manager.". This is a breaking change but nobody uses this interface yet, so it is a good time to do it.
- This piece of code `flask_app.extensions["csrf"].exempt(blueprint)` should be moved in the `set_api_endpoints` method using `appbuilder.app.extensions["csrf"].exempt(api.blueprint)`
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "RobbeSneyders (via GitHub)" <gi...@apache.org>.
RobbeSneyders commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1958422648
Sorry for the late response, let me try to give some pointers for the open issues.
> 1. Based on this discussion:
> Blueprint is now can't be retrieved from connexion app and there will be always None. Find a way to add csrf extension to a newly created blueprint using connexion: to retrieve blueprint object from connexion_app variable to save the current logic (`flask_app.extensions["csrf"].exempt(blueprint)`) or find a way to add this extension on connexion level(check the documentation for available options).
The most future-proof solution would be to move the `csfr` protection to a middleware such as [this one](https://github.com/simonw/asgi-csrf/tree/main). The `skif_if_scope` parameter seems to be a replacement for the `exempt` functionality you're using.
> 2. Find a way to replace legacy `environ_overrides` method from old version of Connexion to fix the problem in tests
> `TypeError: get() got an unexpected keyword argument 'environ_overrides'`. Contact @RobbeSneyders for some help. Currently we didn't find anything in their documentation regarding new way how to override env variables.
There's no one-on-one replacement for this as far as I can tell. This is also not something Connexion can easily offer, since we just leverage the `starlette` test client. The easiest approach I can think of, is to add a middleware instead.
```python
class RemoteUserMiddleware:
def __init__(self, asgi_app: Callable, remote_user: str) -> None:
self.asgi_app = asgi_app
self.remote_user = remote_user
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
scope["REMOTE_USER"] = self.remote_user
await self.asgi_app(scope, receive, send)
```
You could use this across tests like so:
```python
app.add_middleware(RemoteUserMiddleware)
with app.test_client():
...
```
Note that:
- This sets the `REMOTE_USER` on a test client level instead of a request level though.
- You might have to update how this variable is accessed as well. `request.scope["REMOTE_USER"]` should work.
> 3. Fix unit tests which use app.
>
> After connexion update create_app function returns connexion_app instead of flask_app, but unit tests in most cases expect flask_app. In this case we need to update all our unit tests (we have ~1000 tests). As a reference of existing errors please check the logs from the last build here:
>
> * In `Providers-AWS,Google` part fix problem with legacy `environ_overrides` as mentioned in step#2.
> https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:3267
See above
> * In `Core` part there is a failing test related to incorrect encoding in the current implementation (check method [here](https://github.com/apache/airflow/blob/main/airflow/utils/helpers.py#L243) that can be related to the error here, maybe we can change the return value from `flask` to `connexion_app`. Needs some investigation. https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:5821
I think this is due to a change in Flask and a non-issue: https://github.com/pallets/flask/issues/5286
> * In `WWW` part fix failing tests with error `AttributeError: 'FlaskApp' object has no attribute 'test_request_context'`. Please, check the documentation [here](https://connexion.readthedocs.io/en/latest/testing.html)
> https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:6496
Two possibilities here:
- Use the underlying Flask app: `app.app.test_request_context()`
- Switch to the Connexion test context in the reference docs, but you will also need to change the context being used in the tested code.
> * In `CLI` part fix asserts that are related to new way of calling Gunicorn command:
> https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:10464
Looks like this just needs an update to the test code to match the renamed arguments.
> * In `Other` part fix errors connected to the incorrect header that is generated. This step requires some additional investigation, since the problem could be not connected to incorrect test, but incorrect setting of headers.
> https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:11744
I don't see how the linked issue is related to headers, but maybe I'm missing some context.
The error I see is
```python
AttributeError: 'FlaskApp' object has no attribute 'config'
```
Which is because you're treating the Connexion App like a Flask App. Changing `app.config` to `app.app.config` should give you what you want.
> * In `Providers-AWS` part fix the problem with `AttributeError: 'FlaskApp' object has no attribute 'test_request_context'` error. Please, check the documentation [here](https://connexion.readthedocs.io/en/latest/testing.html)
> https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:20406
See above.
> * In `Providers-Google` part fix the problem with `AttributeError: 'Response' object has no attribute 'data'` that occurres because Response object from connexion is not the same then from flask.
> https://github.com/apache/airflow/actions/runs/7462817178/job/20307226379#step:6:22081
The returned response [provides a lot of methods](https://www.python-httpx.org/api/#response) to access the data. The one on one translation is probably `.content`, but if you're decoding anyway, you could use `.text` instead.
Hope this helps!
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "lewijw (via GitHub)" <gi...@apache.org>.
lewijw commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1921462946
FYI. I was able to get past the prior issue by adding a "with":
def _handle_api_not_found(
request: ConnexionRequest, ex: starlette.exceptions.HTTPException
) -> ConnexionResponse:
with connexion_app.app.app_context():
if any([request.url.path.startswith(p) for p in base_paths]):
# 404 errors are never handled on the blueprint level
# unless raised from a view func so actual 404 errors,
# i.e. "no route for it" defined, need to be handled
# here on the application level
return _handle_http_exception(ex)
else:
return views.not_found(ex)
I expect something like it needs to be done with the other handlers.
After getting past this problem, I am running into another error message:
RuntimeError: Unable to build URLs outside an active request without 'SERVER_NAME' configured. Also configure 'APPLICATION_ROOT' and 'PREFERRED_URL_SCHEME' as needed.
So, it looks like something is not getting initialized.
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-2041658611
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "RobbeSneyders (via GitHub)" <gi...@apache.org>.
RobbeSneyders commented on code in PR #36052:
URL: https://github.com/apache/airflow/pull/36052#discussion_r1414534535
##########
airflow/www/extensions/init_views.py:
##########
@@ -220,7 +224,7 @@ def resolve(self, operation):
return _LazyResolution(self.resolve_function_from_operation_id, operation_id)
-class _CustomErrorRequestBodyValidator(RequestBodyValidator):
+class _CustomErrorRequestBodyValidator(AbstractRequestBodyValidator):
Review Comment:
This behavior was contributed to Connexion upstream and is included in Connexion 3. A custom validator is no longer needed.
##########
airflow/auth/managers/fab/fab_auth_manager.py:
##########
@@ -147,20 +149,22 @@ def get_cli_commands() -> list[CLICommand]:
SYNC_PERM_COMMAND, # not in a command group
]
- def get_api_endpoints(self) -> None | FlaskApi:
+ def get_api_endpoints(self) -> None | FlaskApp:
Review Comment:
This function should not return a `FlaskApp`.
There are two ways to change this so it will work with Connexion 3:
- Pass in a `FlaskApp` and call `add_api` on it in this function
- Return an the arguments for the `add_api` call from this function. They could be packaged in a class [like this one](https://github.com/spec-first/connexion/blob/0082d7ad331bde3ad3e598a1fb3d222932edd549/connexion/middleware/main.py#L172).
##########
airflow/www/extensions/init_views.py:
##########
@@ -267,24 +271,30 @@ def init_api_connexion(app: Flask) -> None:
base_path = "/api/v1"
base_paths.append(base_path)
- with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
- specification = safe_load(f)
Review Comment:
The specification was preloaded for some speedup in application startup (see #29631)
##########
setup.cfg:
##########
@@ -85,7 +85,7 @@ install_requires =
# The usage was added in #30596, seemingly only to override and improve the default error message.
# Either revert that change or find another way, preferably without using connexion internals.
# This limit can be removed after https://github.com/apache/airflow/issues/35234 is fixed
- connexion[flask]>=2.10.0,<3.0
+ connexion[flask]>=2.10.0
Review Comment:
I would recommend bumping to 3.0 directly since it would probably be hard to write code that is completely compatible with both 2.X and 3.0.
##########
airflow/www/extensions/init_views.py:
##########
@@ -267,24 +271,30 @@ def init_api_connexion(app: Flask) -> None:
base_path = "/api/v1"
base_paths.append(base_path)
- with ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml").open() as f:
- specification = safe_load(f)
- api_bp = FlaskApi(
+
+ specification=ROOT_APP_DIR.joinpath("api_connexion", "openapi", "v1.yaml")
+ connexion_app = FlaskApp(
+ import_name=__name__,
+ specification_dir=specification,
+ strict_validation=True,
+ validate_responses=True,
+ resolver=Resolver(),
+ swagger_ui_options=SwaggerUIOptions(
+ swagger_ui=conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
+ swagger_ui_path=os.fspath(ROOT_APP_DIR.joinpath("www", "static", "dist", "swagger-ui")),
+ ),
+ )
+ connexion_app.app = app
+ connexion_app.add_api(
specification=specification,
- resolver=_LazyResolver(),
base_path=base_path,
- options={
- "swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- "swagger_path": os.fspath(ROOT_APP_DIR.joinpath("www", "static", "dist", "swagger-ui")),
- },
- strict_validation=True,
+ resolver=Resolver(),
validate_responses=True,
- validator_map={"body": _CustomErrorRequestBodyValidator},
- ).blueprint
- api_bp.after_request(set_cors_headers_on_response)
-
- app.register_blueprint(api_bp)
- app.extensions["csrf"].exempt(api_bp)
+ strict_validation=True,
+ )
+ app.after_request_funcs.setdefault(base_path, []).append(set_cors_headers_on_response)
+ app.register_error_handler(ProblemException, common_error_handler)
+ app.extensions["csrf"].exempt(base_path)
def init_api_internal(app: Flask, standalone_api: bool = False) -> None:
Review Comment:
Same here, either the Connexion `FlaskApp` needs to be passed in and `add_api` called, or the `add_api` arguments need to be returned.
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1849644073
> Thanks @VladaZakharova.
>
> I believe we'll need to take a slightly different approach though. We only want to create a single `FlaskApp` and register `Apis` on them. I left some comments.
>
> As mentioned, I started an effort to port to Connexion 3 a couple of weeks ago, but didn't have the time to complete it. I pushed my changes [here](https://github.com/RobbeSneyders/airflow/pull/1/files) so you can have a look.
Thank you for your suggestions! I also checked your PR you have mentioned and i can say we are almost there! :D
I think the best approach here will be to let Robbe create PR in Community with his changes, since all the migration on Apache Airflow side was covered in his PR.
I was also able to test changes with new Gunicorn command to use ASGI as was suggested earlier also by Robbe and i can see that Airflow is actually working in breeze, but when clicking something in the UI i can see this error:
```
webserver | [2023-12-07 10:27:48 +0000] [1312734] [ERROR] Exception in ASGI application
webserver | Traceback (most recent call last):
webserver | File "/usr/local/lib/python3.8/site-packages/starlette/middleware/errors.py", line 164, in __call__
webserver | await self.app(scope, receive, _send)
webserver | File "/usr/local/lib/python3.8/site-packages/connexion/middleware/exceptions.py", line 101, in __call__
webserver | await super().__call__(scope, receive, send)
webserver | File "/usr/local/lib/python3.8/site-packages/starlette/middleware/exceptions.py", line 62, in __call__
webserver | await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send)
webserver | File "/usr/local/lib/python3.8/site-packages/starlette/_exception_handler.py", line 63, in wrapped_app
webserver | response = await handler(conn, exc)
webserver | File "/usr/local/lib/python3.8/site-packages/connexion/middleware/exceptions.py", line 39, in wrapper
webserver | response = await run_in_threadpool(handler, request, exc)
webserver | File "/usr/local/lib/python3.8/site-packages/starlette/concurrency.py", line 35, in run_in_threadpool
webserver | return await anyio.to_thread.run_sync(func, *args)
webserver | File "/usr/local/lib/python3.8/site-packages/anyio/to_thread.py", line 49, in run_sync
webserver | return await get_async_backend().run_sync_in_worker_thread(
webserver | File "/usr/local/lib/python3.8/site-packages/anyio/_backends/_asyncio.py", line 2103, in run_sync_in_worker_thread
webserver | return await future
webserver | File "/usr/local/lib/python3.8/site-packages/anyio/_backends/_asyncio.py", line 823, in run
webserver | result = context.run(func, *args)
webserver | File "/opt/airflow/airflow/www/extensions/init_views.py", line 229, in _handle_api_not_found
webserver | return views.not_found(ex)
webserver | File "/opt/airflow/airflow/www/views.py", line 632, in not_found
webserver | render_template(
webserver | File "/usr/local/lib/python3.8/site-packages/flask/templating.py", line 145, in render_template
webserver | app = current_app._get_current_object() # type: ignore[attr-defined]
webserver | File "/usr/local/lib/python3.8/site-packages/werkzeug/local.py", line 508, in _get_current_object
webserver | raise RuntimeError(unbound_message) from None
webserver | RuntimeError: Working outside of application context.
webserver | This typically means that you attempted to use functionality that needed
webserver | the current application. To solve this, set up an application context
webserver | with app.app_context(). See the documentation for more information.
webserver | During handling of the above exception, another exception occurred:
webserver | Traceback (most recent call last):
webserver | File "/usr/local/lib/python3.8/site-packages/uvicorn/protocols/http/h11_impl.py", line 408, in run_asgi
webserver | result = await app( # type: ignore[func-returns-value]
webserver | File "/usr/local/lib/python3.8/site-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__
webserver | return await self.app(scope, receive, send)
webserver | File "/usr/local/lib/python3.8/site-packages/connexion/apps/abstract.py", line 284, in __call__
webserver | return await self.middleware(scope, receive, send)
webserver | File "/usr/local/lib/python3.8/site-packages/connexion/middleware/main.py", line 501, in __call__
webserver | await self.app(scope, receive, send)
webserver | File "/usr/local/lib/python3.8/site-packages/starlette/middleware/errors.py", line 181, in __call__
webserver | await response(scope, receive, send)
webserver | TypeError: 'coroutine' object is not callable
webserver | 192.168.11.1:48504 - "GET /static/clusterActivity.js HTTP/1.1" 500
```
I think it is connected to the fact that we are trying now to run synchronous methods asynchronously, so we are failing :D
Maybe guys you can suggest something regarding this? Should we rewrite all the calls that are synchronous or there are other approaches to solve this problem?
The final Gunicorn command i was using is:
```
run_args = [
sys.executable,
"-m",
"gunicorn",
"-k",
"uvicorn.workers.UvicornWorker",
"--workers",
str(num_workers),
"--timeout",
str(worker_timeout),
"--bind",
args.hostname + ":" + str(args.port),
"--name",
"airflow-webserver",
"--pid",
pid_file,
"--config",
"python:airflow.www.gunicorn_config",
]
```
Thank you!
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1864958699
Thank you everyone who is involved! :)
I have updated the code and Gunicorn command as it was suggested by @RobbeSneyders in his PR (thank you, great job!)
I have also added some custom Encoder to fix some issue that occurred after some changes made last 2 weeks. It seems now that webserver works as expected, however i am not sure about the correctness of the changes. Will be great if someone familiar will take a look :)
The only problem that remains is warning about unclosed js and css files, like this:
`webserver | /usr/local/lib/python3.8/site-packages/a2wsgi/wsgi.py:246 ResourceWarning: unclosed file <_io.BufferedReader name='/opt/airflow/airflow/www/static/dist/grid.6154b81a58dba7c670ca.js'>
`
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1898788598
You will also have to update the method in `AwsAuthManager`, but that should be straightforward
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1885139430
> Update init_api_auth_provider function.
In get_api_endpoints function we use connexion_app.add_api and expect FlaskApi as a return object, but add_api function always return None. Previously we use FlaskApi object for taking blueprints and then extend our base_paths here: https://github.com/apache/airflow/pull/36052/files#diff-a1cf5a1eea3231a292d789d82df7943bfe8fa89ca955404b2f9cdaa25e08fa80R309
@vincbeck is it important to save this logic for fab_auth_manager or is it enough to register blueprints under connexion_app.add_api functionality?
I am not sure this is relevant anymore. The signature of `get_api_endpoints` changed. See [here](https://github.com/apache/airflow/blob/main/airflow/providers/fab/auth_manager/fab_auth_manager.py#L149). It should simplify the problem.
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "Satoshi-Sh (via GitHub)" <gi...@apache.org>.
Satoshi-Sh commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1958459086
@RobbeSneyders Thanks for the pointers. I will have a look at it and ask you if I have a question.
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #36052:
URL: https://github.com/apache/airflow/pull/36052#discussion_r1414449599
##########
airflow/auth/managers/fab/fab_auth_manager.py:
##########
@@ -147,20 +149,22 @@ def get_cli_commands() -> list[CLICommand]:
SYNC_PERM_COMMAND, # not in a command group
]
- def get_api_endpoints(self) -> None | FlaskApi:
+ def get_api_endpoints(self) -> None | FlaskApp:
folder = Path(__file__).parents[0].resolve() # this is airflow/auth/managers/fab/
- with folder.joinpath("openapi", "v1.yaml").open() as f:
- specification = safe_load(f)
- return FlaskApi(
- specification=specification,
- resolver=_LazyResolver(),
- base_path="/auth/fab/v1",
- options={
- "swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- },
+ # with folder.joinpath("openapi", "v1.yaml").open() as f:
+ # specification = safe_load(f)
+ specification=folder.joinpath("openapi", "v1.yaml")
+ return FlaskApp(
+ import_name=__name__,
+ specification_dir=specification,
+ resolver=Resolver(),
+ # base_path="/auth/fab/v1",
Review Comment:
Why is it commented out? We need it I guess?
##########
airflow/auth/managers/fab/fab_auth_manager.py:
##########
@@ -147,20 +149,22 @@ def get_cli_commands() -> list[CLICommand]:
SYNC_PERM_COMMAND, # not in a command group
]
- def get_api_endpoints(self) -> None | FlaskApi:
+ def get_api_endpoints(self) -> None | FlaskApp:
folder = Path(__file__).parents[0].resolve() # this is airflow/auth/managers/fab/
- with folder.joinpath("openapi", "v1.yaml").open() as f:
- specification = safe_load(f)
- return FlaskApi(
- specification=specification,
- resolver=_LazyResolver(),
- base_path="/auth/fab/v1",
- options={
- "swagger_ui": conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
- },
+ # with folder.joinpath("openapi", "v1.yaml").open() as f:
+ # specification = safe_load(f)
Review Comment:
Remove these comments?
##########
airflow/www/extensions/init_views.py:
##########
Review Comment:
You will have to modify the function `init_api_auth_provider` as well
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "MaksYermak (via GitHub)" <gi...@apache.org>.
MaksYermak commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1884982252
Hello Team,
I see several issues which we need to fix for finishing this PR.
1. Fix unit tests which use `app`.
After connexion update `create_app` function returns `connexion_app` instead of `flask_app`, but unit tests in most cases expect `flask_app`. In this case we need to update all our unit tests (we have ~1000 tests).
List of errors which I found:
- for `test_client()` we should use `connexion_app` not `flask_app`. I fixed this error for most tests in the last commit.
- `TypeError: get() got an unexpected keyword argument 'environ_overrides'` this error is the most frequent now. For fixing it we need to find the equivalent for this variable `environ_overrides` in connexion's `test_client()`. I didn't find any equivalent in Connexion and Starlette docs. @RobbeSneyders could you help us with that?
- `AttributeError: 'Response' object has no attribute 'data'` I think this happens because `Response` object from connexion is not the same then from flask.
Also we have a big amount of different errors which do not have a similar pattern.
2. Update `init_api_auth_provider` function.
In ` get_api_endpoints` function we use `connexion_app.add_api` and expect `FlaskApi` as a return object, but `add_api` function always return `None`. Previously we use `FlaskApi` object for taking blueprints and then extend our `base_paths` here: https://github.com/apache/airflow/pull/36052/files#diff-a1cf5a1eea3231a292d789d82df7943bfe8fa89ca955404b2f9cdaa25e08fa80R309
@vincbeck is it important to save this logic for fab_auth_manager or is it enough to register blueprints under `connexion_app.add_api` functionality?
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "VladaZakharova (via GitHub)" <gi...@apache.org>.
VladaZakharova commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1898612794
> > Update init_api_auth_provider function.
> > In get_api_endpoints function we use connexion_app.add_api and expect FlaskApi as a return object, but add_api function always return None. Previously we use FlaskApi object for taking blueprints and then extend our base_paths here: https://github.com/apache/airflow/pull/36052/files#diff-a1cf5a1eea3231a292d789d82df7943bfe8fa89ca955404b2f9cdaa25e08fa80R309
> > @vincbeck is it important to save this logic for fab_auth_manager or is it enough to register blueprints under connexion_app.add_api functionality?
>
> I am not sure this is relevant anymore. The signature of `get_api_endpoints` changed. See [here](https://github.com/apache/airflow/blob/main/airflow/providers/fab/auth_manager/fab_auth_manager.py#L149). It should simplify the problem.
Hi @vincbeck ! I think the question here was about the line where we are appending `base_paths` variable with new registered blueprint. With the new version of Connexion the way of registering blueprints will be changed to register in connexion_app. So the method `get_api_endpoints()` with new version of Connexion will return None instead of blueprint object. So, if we can't return blueprint from the `get_api_endpoints()` method, the `base_paths` variable couldn't be extended. Should we consider still updating `base_paths` with blueprint object? Is this variable used later somewhere? The if statement here shows that we still can continue without adding blueprint to this list of base_paths. If we need this variable later, is there other way to add blueprint not using value returned by `get_api_endpoints()` since now it will return None?
```
def init_api_auth_provider(connexion_app: connexion.FlaskApp):
"""Initialize the API offered by the auth manager."""
auth_mgr = get_auth_manager()
blueprint = auth_mgr.get_api_endpoints(connexion_app)
if blueprint:
base_paths.append(blueprint.url_prefix if blueprint.url_prefix else "")
flask_app = connexion_app.app
flask_app.extensions["csrf"].exempt(blueprint)
```
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "Satoshi-Sh (via GitHub)" <gi...@apache.org>.
Satoshi-Sh commented on PR #36052:
URL: https://github.com/apache/airflow/pull/36052#issuecomment-1953320107
Hi @vincbeck @VladaZakharova , I started working on the bugs Vlada listed above.
For the first bug, I just updated codes as Vinc's commented. Can you guide me on how to test it. I'm new to this repository so any guidance for this issue would be helpful to me.
Here is my PR draft
https://github.com/apache/airflow/pull/37555
--
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
Re: [PR] Attempt to update Connexion library to version 3 [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #36052: Attempt to update Connexion library to version 3
URL: https://github.com/apache/airflow/pull/36052
--
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