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 2021/04/09 18:01:18 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #15311: WIP: Sync DAG specific permissions when parsing
jedcunningham opened a new pull request #15311:
URL: https://github.com/apache/airflow/pull/15311
This POC allows the DAG specific permissions to be created/updated during DAG parsing, instead of during webserver start or cli sync-perm.
With a large number of DAGs, walking through them all to do DAG specific permissions isn't exactly fast and they can only change during the scheduler parsing anyways. Overall more efficient as we don't need to check every DAG as well, we only need to check a given DAG when it changes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-872160718
If something is not working in Composer that is fixed in open source Airflow then you should raise that issue with Composer support.
--
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] jedcunningham commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-871532697
2.1.0: https://github.com/apache/airflow/blob/e3f5eb9215a016de5d02523fac65a9967c578396/CHANGELOG.txt#L100
--
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] ali-hafidz commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
ali-hafidz commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-881183547
@chodankarcc I also facing the same issue. then already updating store_serialized_dags = False . but I have new issue, the dag that I set running sequentially running not in order, the dag running from middle dag I think its bug ui, when I updating store_serialized_dags = True its working normally . Have you facing the same problem ? I also using composer.
--
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] chodankarcc commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
chodankarcc commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-871536537
> 2.1.0:
>
> https://github.com/apache/airflow/blob/e3f5eb9215a016de5d02523fac65a9967c578396/CHANGELOG.txt#L100
Thanks for quick reply. I am using composer-1.16.7-airflow-1.10.15 (Google Composer), and unfortunately composer don't have this airflow version available yet to upgrade to. So is there any alternative other than admin clicking on refresh to update permissions as I want to automate solution,
--
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] jhtimmins commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r612039109
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,10 +37,25 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import AirflowSecurityManager
log = logging.getLogger(__name__)
+class SimpleSecurityManager(AirflowSecurityManager):
Review comment:
@jedcunningham Oof I need to think about this, because generally speaking we really don't want to extend the webserver-level controls into Airflow core.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] chodankarcc commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
chodankarcc commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-872141868
> store_serialized_dags
I was able to solve issue by updating store_serialized_dags = False in airflow config. Thanks for your pointer
--
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] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613463405
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
Review comment:
Yea missing DAGs are removed from `SerializedDagModel` while we mark it as `is_active=False` in `DagModel`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r612046000
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
Review comment:
I believe that `is_active` and `is_paused` are queried for explicitly bc a DAG could be in neither state, in which case we don't want to fetch it to create new permissions.
IIRC it has to do with some historical reason. Something about DAGs getting deleted in the DB; the record sticks around but they've been soft deleted. We'll need to account for that when fetching results.
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,10 +37,25 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import AirflowSecurityManager
log = logging.getLogger(__name__)
+class SimpleSecurityManager(AirflowSecurityManager):
+ """Security Manager that doesn't need the whole flask app"""
+
+ def __init__(self): # pylint: disable=super-init-not-called
+ self.session = None
+
+ @property
+ def get_session(self):
+ return self.session
+
+
+security_manager = SimpleSecurityManager()
Review comment:
Ok, after thinking more about this, I don't think we should be extending the security manager into the `/airflow/models` directory. I'd much rather create a `sync-permissions` API endpoint if one doesn't exist, and hitting that from the CLI via a separate HTTP request.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] chodankarcc commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
chodankarcc commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-871526913
which airflow version has this fixed change?
--
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] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613589430
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
- .all()
- )
+ dagbag = DagBag(read_dags_from_db=True)
+ dagbag.collect_dags_from_db()
Review comment:
Take a look now. That work?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r612664757
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
Review comment:
The new approach is the one already taken by the cli and is, from what I can tell, the more complete approach. I believe using `DagBag.collect_dags_from_db` is equivalent to `is_active` (as missing DAGs are removed from `SerializedDagModel`). `is_paused`, not sure why that was explicitly included here but I don't think it needs to be.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r612039109
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,10 +37,25 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import AirflowSecurityManager
log = logging.getLogger(__name__)
+class SimpleSecurityManager(AirflowSecurityManager):
Review comment:
@jedcunningham Oof I need to think about this, because generally speaking we really don't want to extend the webserver-level controls into Airflow core.
The ability to define access controls inside the DAG is a huge antipattern imo.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613462536
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,9 +37,12 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import SimpleAirflowSecurityManager
log = logging.getLogger(__name__)
+security_manager = SimpleAirflowSecurityManager()
Review comment:
We should probably move this to `write_dag` too otherwise import anything from `airflow/models/serialized_dag.py` will just need an extra second needlessly
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613462536
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,9 +37,12 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import SimpleAirflowSecurityManager
log = logging.getLogger(__name__)
+security_manager = SimpleAirflowSecurityManager()
Review comment:
We should probably move this to `write_dag` too otherwise importing anything from `airflow/models/serialized_dag.py` will just need an extra second needlessly
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613478067
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
- .all()
- )
+ dagbag = DagBag(read_dags_from_db=True)
+ dagbag.collect_dags_from_db()
Review comment:
oh cool -- as long as it is just used in the CLI it is fine ..
Can you probably just add a comment or note in the docstring to caution a DEV on using it elsewhere
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613465234
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
- .all()
- )
+ dagbag = DagBag(read_dags_from_db=True)
+ dagbag.collect_dags_from_db()
Review comment:
When is this function `create_dag_specific_permissions` called?
I am just concerned if we are just moving loading of all serialized_dags to some other place
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-871542216
I believe running [airflow sync-perm](https://airflow.apache.org/docs/apache-airflow/1.10.15/cli-ref.html#sync-perm) should do it 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
[GitHub] [airflow] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613471571
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
- .all()
- )
+ dagbag = DagBag(read_dags_from_db=True)
+ dagbag.collect_dags_from_db()
Review comment:
The sync-perms cli with the `include_dags` flag set, that's it.
Previously we had this method, used during webserver startup, and the sync-perms cli with used a different approach and also supported access_control. I'm simply moving the more complete version (sync-perms cli with short circuit added) into the security manager.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r614466636
##########
File path: UPDATING.md
##########
@@ -95,6 +95,12 @@ The `default_queue` configuration option has been moved from `[celery]` section
This allows Airflow to work more reliably with some environments (like Azure) by default.
+### `sync-perm` CLI no longer syncs DAG specific permissions by default
+
+The `sync-perm` CLI command will no longer sync DAG specific permissions by default as they are now being handled during
+DAG parsing. If you need or want the old behavoir, you can pass `--include-dags` to have `sync-perm` also sync DAG
Review comment:
🤦♂️ thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r610814927
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,10 +37,25 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import AirflowSecurityManager
log = logging.getLogger(__name__)
+class SimpleSecurityManager(AirflowSecurityManager):
Review comment:
@jhtimmins, curious is you know a better way or trick to using the security manager somewhere where we don't want/need the whole flask app?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613487427
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,9 +37,12 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import SimpleAirflowSecurityManager
log = logging.getLogger(__name__)
+security_manager = SimpleAirflowSecurityManager()
Review comment:
yup both the import and the object
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #15311:
URL: https://github.com/apache/airflow/pull/15311
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r614462435
##########
File path: UPDATING.md
##########
@@ -95,6 +95,12 @@ The `default_queue` configuration option has been moved from `[celery]` section
This allows Airflow to work more reliably with some environments (like Azure) by default.
+### `sync-perm` CLI no longer syncs DAG specific permissions by default
+
+The `sync-perm` CLI command will no longer sync DAG specific permissions by default as they are now being handled during
+DAG parsing. If you need or want the old behavoir, you can pass `--include-dags` to have `sync-perm` also sync DAG
Review comment:
```suggestion
DAG parsing. If you need or want the old behaviour, you can pass `--include-dags` to have `sync-perm` also sync DAG
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r614462810
##########
File path: airflow/www/security.py
##########
@@ -617,7 +619,7 @@ def sync_resource_permissions(self, perms=None):
def sync_perm_for_dag(self, dag_id, access_control=None):
"""
Sync permissions for given dag id. The dag id surely exists in our dag bag
- as only / refresh button or cli.sync_perm will call this function
+ as only / refresh button or SerializedDagModel will call this function
Review comment:
This needs updating
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613479873
##########
File path: airflow/models/serialized_dag.py
##########
@@ -37,9 +37,12 @@
from airflow.utils import timezone
from airflow.utils.session import provide_session
from airflow.utils.sqlalchemy import UtcDateTime
+from airflow.www.security import SimpleAirflowSecurityManager
log = logging.getLogger(__name__)
+security_manager = SimpleAirflowSecurityManager()
Review comment:
Do you mean the import too? Creating the object itself is fast.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613473201
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
Review comment:
I'm going to resolve this, but feel free to chime in again if there are more concerns.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] chodankarcc commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
chodankarcc commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-871568924
No sync_perm is not working as expected. Its not updating roles permission as per DAG access control.
--
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] jedcunningham commented on a change in pull request #15311: WIP: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#discussion_r613472315
##########
File path: airflow/www/security.py
##########
@@ -516,24 +515,25 @@ def _get_all_roles_with_permissions(self) -> Dict[str, Role]:
def create_dag_specific_permissions(self) -> None:
"""
- Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
+ Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs,
+ along with any `access_control` permissions provided in the DAG.
:return: None.
"""
perms = self.get_all_permissions()
- rows = (
- self.get_session.query(models.DagModel.dag_id)
- .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
- .all()
- )
+ dagbag = DagBag(read_dags_from_db=True)
+ dagbag.collect_dags_from_db()
Review comment:
This is functionally different than what we are adding into the parsing process too, as this needs to look at all DAGs whereas parsing is done with a single DAG.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #15311: Sync DAG specific permissions when parsing
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #15311:
URL: https://github.com/apache/airflow/pull/15311#issuecomment-871619947
Interesting, the code looks like it should do it 🤷♂️. Sorry, I'm not sure.
https://github.com/apache/airflow/blob/5786dcdc392f7a2649f398353a0beebef01c428e/airflow/bin/cli.py#L2075-L2080
--
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