-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Call init from auth managers only once
#47869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5dd6d66 to
ab33cc7
Compare
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py
Outdated
Show resolved
Hide resolved
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py
Outdated
Show resolved
Hide resolved
ab33cc7 to
3489ddc
Compare
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py
Dismissed
Show dismissed
Hide dismissed
bugraoz93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool!
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py
Outdated
Show resolved
Hide resolved
3489ddc to
f43beac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that the auth manager initialization is only executed once per process, addressing the multiple invocations caused by the Flask application and FastAPI’s multi-worker environment. Key changes include:
- Adding file locking in SimpleAuthManager to restrict the init method to a single execution.
- Updating and adding tests to verify the correct handling of passwords and file content.
- Modifying FAB auth manager to use a dedicated method (init_flask_resources) for initialization in Flask applications.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/api_fastapi/auth/managers/simple/test_simple_auth_manager.py | Added tests covering password file reading/updating and filtering of user entries. |
| airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py | Introduced file locking via fcntl in the init method and refactored password retrieval logic. |
| providers/fab/src/airflow/providers/fab/www/extensions/init_appbuilder.py | Modified to call init_flask_resources instead of init for FAB auth manager. |
| providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py | Renamed init to init_flask_resources, aligning with the new initialization strategy. |
Comments suppressed due to low confidence (1)
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py:127
- Typo in the comment: 'User dot not exist in the file' should be corrected to 'User does not exist in the file'.
# User dot not exist in the file, adding it
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
f7ed25e to
1dd998c
Compare
The method
initfrom auth managers is called multiples times for two reasons:initis also called in the Flask applications. This is only needed for fab auth manager. Thus, I created a method specificinit_flask_resourcesin FAB auth manager that I call in the Flask applicationsfastapispins up multiple workers (4 by default), thereforeinitis called 4 times. I updated the init function of simple auth manager to be called only once^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.