Skip DAG perm sync during parsing if possible #15464
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedcunningham
requested review from
XD-DENG,
ashb,
jhtimmins,
kaxil and
turbaszek
as code owners
jedcunningham
added
the
area:Scheduler
For DAGs without `access_control` that already have their PermissionView records, we can skip syncing their permissions during parsing. This cuts down on database queries and is faster (~2 seconds, mostly import time).
jedcunningham
force-pushed
the
faster_dag_perm_sync
branch
from
f1a8d99 to
7a67725
Compare
kaxil
reviewed
Apr 21, 2021
airflow/security/permissions.py
Outdated
| DAG_PERMS = {ACTION_CAN_READ, ACTION_CAN_EDIT} | ||
|
|
||
|
|
||
| def prefixed_dag_id(dag_id): |
Member
There was a problem hiding this comment.
Can we rename this to permission_for_dag or something better? WDYT
Member
Author
There was a problem hiding this comment.
I went back and forth on renaming it since it is being moved from www/security, but I'm also happy with a more descriptive name. Related, should we follow up later with deprecating the method in www/security?
Member
There was a problem hiding this comment.
Yup agreed on deprecated method in www/security
kaxil
reviewed
Apr 21, 2021
airflow/models/dagbag.py
Outdated
Comment on lines
548
to
571
| from flask_appbuilder.security.sqla import models as sqla_models | ||
|
|
||
| from airflow.security.permissions import DAG_PERMS, prefixed_dag_id | ||
|
|
||
| def needs_perm_views(dag_id: str) -> bool: | ||
| view_menu_name = prefixed_dag_id(dag_id) | ||
| for permission_name in DAG_PERMS: | ||
| if not ( | ||
| session.query(sqla_models.PermissionView) | ||
| .join(sqla_models.Permission) | ||
| .join(sqla_models.ViewMenu) | ||
| .filter(sqla_models.Permission.name == permission_name) | ||
| .filter(sqla_models.ViewMenu.name == view_menu_name) | ||
| .one_or_none() | ||
| ): | ||
| return True | ||
| return False | ||
|
|
||
| if dag.access_control or needs_perm_views(dag.dag_id): | ||
| self.log.debug("Syncing DAG permissions: %s to the DB", dag.dag_id) | ||
| from airflow.www.security import ApplessAirflowSecurityManager | ||
|
|
||
| security_manager = ApplessAirflowSecurityManager(session=session) | ||
| security_manager.sync_perm_for_dag(dag.dag_id, dag.access_control) |
Member
There was a problem hiding this comment.
Can we separate this code block to a separate internal method too -- will be easier and test just that block in isolation too
Member
There was a problem hiding this comment.
By this I mean the entire block from L548:L571
Member
Author
There was a problem hiding this comment.
I refactored the tests in a separate commit. I can go both ways here.
kaxil
reviewed
Apr 21, 2021
kaxil
reviewed
Apr 21, 2021
Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
jedcunningham
commented
Apr 21, 2021
kaxil
approved these changes
Apr 22, 2021
kaxil
pushed a commit
to astronomer/airflow
that referenced
this pull request
Apr 26, 2021
For DAGs without `access_control` that already have their PermissionView records, we can skip syncing their permissions during parsing. This cuts down on database queries and is faster (~2 seconds, mostly import time). (cherry picked from commit 3bfe0e0)
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For DAGs without
access_controlthat already have their PermissionViewrecords, we can skip syncing their permissions during parsing. This cuts
down on database queries and is faster (~2 seconds, mostly import time).