Skip to content

Sync DAG specific permissions when parsing#15311

Merged
kaxil merged 13 commits into
apache:masterfrom
astronomer:perm_sync_on_parse
Apr 19, 2021
Merged

Sync DAG specific permissions when parsing#15311
kaxil merged 13 commits into
apache:masterfrom
astronomer:perm_sync_on_parse

Conversation

@jedcunningham

@jedcunningham jedcunningham commented Apr 9, 2021

Copy link
Copy Markdown
Member

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 also fixed a bug where the default webserver DAG specific syncing didn't handle access_control.

Closes #8609

Comment thread airflow/models/serialized_dag.py Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@jhtimmins jhtimmins Apr 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread airflow/www/security.py Outdated
Comment thread airflow/models/serialized_dag.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread airflow/models/serialized_dag.py Outdated
Comment thread airflow/www/security.py Outdated
@jedcunningham jedcunningham marked this pull request as ready for review April 15, 2021 19:26
Comment thread UPDATING.md Outdated
Comment thread airflow/www/security.py Outdated
@kaxil kaxil changed the title WIP: Sync DAG specific permissions when parsing Sync DAG specific permissions when parsing Apr 15, 2021
@kaxil kaxil added this to the Airflow 2.1 milestone Apr 16, 2021
@kaxil kaxil dismissed jhtimmins’s stale review April 19, 2021 11:50

Stale review

@kaxil kaxil merged commit d52ad87 into apache:master Apr 19, 2021
@kaxil kaxil deleted the perm_sync_on_parse branch April 19, 2021 11:50
kaxil pushed a commit to astronomer/airflow that referenced this pull request Apr 26, 2021
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 also fixed a bug where the default webserver DAG specific syncing didn't handle `access_control`.

Closes apache#8609

(cherry picked from commit d52ad87)
@chodankarcc

Copy link
Copy Markdown

which airflow version has this fixed change?

@jedcunningham

Copy link
Copy Markdown
Member Author

2.1.0:

- Sync DAG specific permissions when parsing (#15311)

@chodankarcc

Copy link
Copy Markdown

2.1.0:

- Sync DAG specific permissions when parsing (#15311)

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,

@jedcunningham

Copy link
Copy Markdown
Member Author

I believe running airflow sync-perm should do it as well.

@chodankarcc

Copy link
Copy Markdown

No sync_perm is not working as expected. Its not updating roles permission as per DAG access control.

@jedcunningham

Copy link
Copy Markdown
Member Author

Interesting, the code looks like it should do it 🤷‍♂️. Sorry, I'm not sure.

airflow/airflow/bin/cli.py

Lines 2075 to 2080 in 5786dcd

print('Updating permission on all DAG views')
dags = DagBag(store_serialized_dags=settings.STORE_SERIALIZED_DAGS).dags.values()
for dag in dags:
appbuilder.sm.sync_perm_for_dag(
dag.dag_id,
dag.access_control)

@chodankarcc

Copy link
Copy Markdown

store_serialized_dags

I was able to solve issue by updating store_serialized_dags = False in airflow config. Thanks for your pointer

@ashb

ashb commented Jul 1, 2021

Copy link
Copy Markdown
Member

If something is not working in Composer that is fixed in open source Airflow then you should raise that issue with Composer support.

@ali-hafidz

Copy link
Copy Markdown

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DAG's parameter access_control is not refreshing in the UI

6 participants