Skip to content

Fix: Keep compatibility with old FAB versions - #41549

Merged
vincbeck merged 11 commits into
apache:mainfrom
joaopamaral:fix/compatibility-old-fab
Aug 20, 2024
Merged

Fix: Keep compatibility with old FAB versions#41549
vincbeck merged 11 commits into
apache:mainfrom
joaopamaral:fix/compatibility-old-fab

Conversation

@joaopamaral

@joaopamaraljoaopamaral commented Aug 17, 2024

Copy link
Copy Markdown
Contributor

closes: #41540

FAB versions before 1.3.0 still use the old access control format that allows only DAGs resource. This PR fixes the format when airflow is running with FAB versions < 1.3.0.

Tests:

  • FAB 1.2.2
    • Old access_control format: access_control={'Viewer': ["can_read","can_delete"]}
# airflow sync-perm --include-dags Updating actions and resources for all existing roles Updating permission on all DAG views [2024-08-19T16:57:58.390+0000] {dagbag.py:567} INFO - Filling up the DagBag from database [2024-08-19T16:57:58.397+0000] {override.py:1105} WARNING - ACCESS CONTROL: {'Viewer': {'can_read', 'can_delete'}} [2024-08-19T16:57:58.410+0000] {override.py:1882} INFO - Added Permission can read on DAG:tutorial2 to role Viewer [2024-08-19T16:57:58.414+0000] {override.py:1882} INFO - Added Permission can delete on DAG:tutorial2 to role Viewer 
  • New access_control format: access_control={'Viewer': {"DAGs": ["can_read","can_delete"]}}
    image

  • FAB 1.3.0:

    • Mixed formats:
access_control={
'Viewer': ["can_read","can_delete"]
}
access_control={
'Viewer': {"DAGs": ["can_read","can_delete"],
"DAG Runs": ["can_create"],}
},
# airflow sync-perm --include-dags Updating actions and resources for all existing roles Updating permission on all DAG views [2024-08-19T17:15:21.591+0000] {dagbag.py:567} INFO - Filling up the DagBag from database [2024-08-19T17:15:21.601+0000] {override.py:1158} WARNING - ACCESS CONTROL: {'Viewer': {'DAGs': {'can_read', 'can_delete'}}} [2024-08-19T17:15:21.613+0000] {override.py:1158} WARNING - ACCESS CONTROL: {'Viewer': {'DAGs': {'can_read', 'can_delete'}, 'DAG Runs': ['can_create']}} 

^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@potiukpotiuk added this to the Airflow 2.10.1 milestone Aug 19, 2024
@joaopamaral

joaopamaral commented Aug 19, 2024

Copy link
Copy Markdown
ContributorAuthor

There is another breaking change Remove deprecated SubDags that is causing a failure during the sync permissions in old FAB versions: https://github.com/apache/airflow/blame/587015ddc84dfd306e4c2487c3aeb65ae280eab5/airflow/providers/fab/auth_manager/security_manager/override.py#L1076

When I try to run the airflow sync-perm --include-dags with airflow main branch and an old FAB I get the error which is not related to the access control changes:

# airflow sync-perm --include-dags │ ____________ _____________
/usr/local/lib/python3.8/site-packages/flask_limiter/extension.py:333 UserWarning: Using the in-memory storage for tracking rate limits as no storage was explicitly specified. This is not recommended for │ ____ |__( )_________ __/__ /________ __
production use. See: https://flask-limiter.readthedocs.io#configuring-a-storage-backend for documentation about configuring the storage backend. │____ /| |_ /__ ___/_ /_ __ /_ __ \_ | /| / /
Updating actions and resources for all existing roles │___ ___ | / _ / _ __/ _ / / /_/ /_ |/ |/ /
Updating permission on all DAG views │ _/_/ |_/_/ /_/ /_/ /_/ \____/____/|__/
[2024-08-19T19:26:23.125+0000] {dagbag.py:567} INFO - Filling up the DagBag from database │[2024-08-19 19:24:20 +0000] [140] [INFO] Starting gunicorn 23.0.0
Traceback (most recent call last): │[2024-08-19 19:24:20 +0000] [140] [INFO] Listening at: http://[::]:8794 (140)
File "/usr/local/bin/airflow", line 8, in <module> │[2024-08-19 19:24:20 +0000] [140] [INFO] Using worker: sync
sys.exit(main()) │[2024-08-19 19:24:20 +0000] [141] [INFO] Booting worker with pid: 141
File "/opt/airflow/airflow/__main__.py", line 62, in main │[2024-08-19 19:24:20 +0000] [142] [INFO] Booting worker with pid: 142
args.func(args) │[2024-08-19T19:24:20.894+0000] {triggerer_job_runner.py:181} INFO - Setting up TriggererHandlerWrapper with handler <FileTaskHandler (NOTSET)>
File "/opt/airflow/airflow/cli/cli_config.py", line 49, in command │[2024-08-19T19:24:20.894+0000] {triggerer_job_runner.py:237} INFO - Setting up logging queue listener with handlers [<RedirectStdHandler <stdout> (NOTSET)>, <TriggererHandlerWrapper (NOTSET)>]
return func(*args, **kwargs) │[2024-08-19T19:24:20.900+0000] {triggerer_job_runner.py:338} INFO - Starting the triggerer
File "/opt/airflow/airflow/utils/cli.py", line 115, in wrapper │[2024-08-19T19:25:21.099+0000] {triggerer_job_runner.py:510} INFO - 0 triggers currently running
return f(*args, **kwargs) │[2024-08-19T19:26:21.264+0000] {triggerer_job_runner.py:510} INFO - 0 triggers currently running
File "/opt/airflow/airflow/utils/providers_configuration_loader.py", line 55, in wrapped_function │
return func(*args, **kwargs) │
File "/opt/airflow/airflow/providers/fab/auth_manager/cli_commands/sync_perm_command.py", line 39, in sync_perm │
appbuilder.sm.create_dag_specific_permissions() │
File "/opt/airflow/airflow/providers/fab/auth_manager/security_manager/override.py", line 1067, in create_dag_specific_permissions │
root_dag_id = dag.parent_dag.dag_id if dag.parent_dag else dag.dag_id │
AttributeError: 'SerializedDAG' object has no attribute 'parent_dag'

To run the final tests with this PR change I need to apply the this line change.

@kaxil, do you know if it's related to this PR, is it expected to keep the compatibility with old FAB versions?

@joaopamaral
joaopamaral marked this pull request as ready for review August 19, 2024 19:30
@kaxil

Copy link
Copy Markdown
Member

There is another breaking change Remove deprecated SubDags that is causing a failure during the sync permissions in old FAB versions: https://github.com/apache/airflow/blame/587015ddc84dfd306e4c2487c3aeb65ae280eab5/airflow/providers/fab/auth_manager/security_manager/override.py#L1076

When I try to run the airflow sync-perm --include-dags with airflow 2.10 and an old FAB I get the error which is not related to the access control changes:

# airflow sync-perm --include-dags │ ____________ _____________
/usr/local/lib/python3.8/site-packages/flask_limiter/extension.py:333 UserWarning: Using the in-memory storage for tracking rate limits as no storage was explicitly specified. This is not recommended for │ ____ |__( )_________ __/__ /________ __
production use. See: https://flask-limiter.readthedocs.io#configuring-a-storage-backend for documentation about configuring the storage backend. │____ /| |_ /__ ___/_ /_ __ /_ __ \_ | /| / /
Updating actions and resources for all existing roles │___ ___ | / _ / _ __/ _ / / /_/ /_ |/ |/ /
Updating permission on all DAG views │ _/_/ |_/_/ /_/ /_/ /_/ \____/____/|__/
[2024-08-19T19:26:23.125+0000] {dagbag.py:567} INFO - Filling up the DagBag from database │[2024-08-19 19:24:20 +0000] [140] [INFO] Starting gunicorn 23.0.0
Traceback (most recent call last): │[2024-08-19 19:24:20 +0000] [140] [INFO] Listening at: http://[::]:8794 (140)
File "/usr/local/bin/airflow", line 8, in <module> │[2024-08-19 19:24:20 +0000] [140] [INFO] Using worker: sync
sys.exit(main()) │[2024-08-19 19:24:20 +0000] [141] [INFO] Booting worker with pid: 141
File "/opt/airflow/airflow/__main__.py", line 62, in main │[2024-08-19 19:24:20 +0000] [142] [INFO] Booting worker with pid: 142
args.func(args) │[2024-08-19T19:24:20.894+0000] {triggerer_job_runner.py:181} INFO - Setting up TriggererHandlerWrapper with handler <FileTaskHandler (NOTSET)>
File "/opt/airflow/airflow/cli/cli_config.py", line 49, in command │[2024-08-19T19:24:20.894+0000] {triggerer_job_runner.py:237} INFO - Setting up logging queue listener with handlers [<RedirectStdHandler <stdout> (NOTSET)>, <TriggererHandlerWrapper (NOTSET)>]
return func(*args, **kwargs) │[2024-08-19T19:24:20.900+0000] {triggerer_job_runner.py:338} INFO - Starting the triggerer
File "/opt/airflow/airflow/utils/cli.py", line 115, in wrapper │[2024-08-19T19:25:21.099+0000] {triggerer_job_runner.py:510} INFO - 0 triggers currently running
return f(*args, **kwargs) │[2024-08-19T19:26:21.264+0000] {triggerer_job_runner.py:510} INFO - 0 triggers currently running
File "/opt/airflow/airflow/utils/providers_configuration_loader.py", line 55, in wrapped_function │
return func(*args, **kwargs) │
File "/opt/airflow/airflow/providers/fab/auth_manager/cli_commands/sync_perm_command.py", line 39, in sync_perm │
appbuilder.sm.create_dag_specific_permissions() │
File "/opt/airflow/airflow/providers/fab/auth_manager/security_manager/override.py", line 1067, in create_dag_specific_permissions │
root_dag_id = dag.parent_dag.dag_id if dag.parent_dag else dag.dag_id │
AttributeError: 'SerializedDAG' object has no attribute 'parent_dag'

To run the final tests with this PR change I need to apply the this line change.

@kaxil, do you know if it's related to this PR, is it expected to keep the compatibility with old FAB versions?

Older FAB version won't work with Airflow 3. You will need to use the changes from the PR or use apache-airflow-providers-fab==1.3.0rc1

@joaopamaral

Copy link
Copy Markdown
ContributorAuthor

Older FAB version won't work with Airflow 3. You will need to use the changes from https://github.com/apache/airflow/pull/41390or use apache-airflow-providers-fab==1.3.0rc1

Thanks for explaining that @kaxil! I've tested with tag 2.10.0 and it's working.

@vincbeck
vincbeck merged commit d7d944e into apache:mainAug 20, 2024
@potiuk

Copy link
Copy Markdown
Member

According to the new rules- we should also back-port it to v2-10-test by PR

@potiuk

Copy link
Copy Markdown
Member

(we means merging committer shoudl decide if they want to do it themselves or ask the author to do so).

@vincbeck

Copy link
Copy Markdown
Contributor

Good call, actually (and I should have thought about it before merging it), do we need it in main at all? Should it be only in 2.10 branch? Because as Kaxil mentioned:

Older FAB version won't work with Airflow 3. You will need to use the changes from https://github.com/apache/airflow/pull/41390or use apache-airflow-providers-fab==1.3.0rc1

Therefore, this compatibility code should only be for Airflow 2.x?

@potiuk

Copy link
Copy Markdown
Member

Possibly - but we have not seen this kind of need before - we assumed we backport everything from main and we do not have "2.10 only" fixes - but yes, this one looks like a legitimate case (@ephraimbuddy@utkarsharma2@kaxil ?)

Comment threadairflow/models/dag.py
@vincbeck

Copy link
Copy Markdown
Contributor

potiuk pushed a commit to potiuk/airflow that referenced this pull request Aug 27, 2024
@potiuk

Copy link
Copy Markdown
Member

Regardless from decision #41792 is the backport as it needs to go there.

@potiuk

Copy link
Copy Markdown
Member

@joaopamaral -> I just cherry-picked the change to v2-10-test, but tests are failing, would you mind back-porting it and fixing the tests - see #41792

joaopamaral added a commit to joaopamaral/airflow that referenced this pull request Aug 27, 2024
joaopamaral added a commit to joaopamaral/airflow that referenced this pull request Aug 27, 2024
joaopamaral added a commit to joaopamaral/airflow that referenced this pull request Aug 27, 2024
potiuk pushed a commit that referenced this pull request Aug 28, 2024
…1809)
* Fix: Keep compatibility with old FAB versions (#41549)
* Fix: Tests after #41549 (Keep compatibility with old FAB versions)
* Fix test_dag and test_dagbag
@utkarsharma2utkarsharma2 added the type:bug-fix Changelog: Bug Fixes label Aug 30, 2024
utkarsharma2 pushed a commit that referenced this pull request Sep 2, 2024
…1809)
* Fix: Keep compatibility with old FAB versions (#41549)
* Fix: Tests after #41549 (Keep compatibility with old FAB versions)
* Fix test_dag and test_dagbag
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug-fixChangelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Access control error after upgrading from version 2.9.3 to 2.10.0

6 participants

@joaopamaral@kaxil@potiuk@vincbeck@zachliu@utkarsharma2