From ee59a72c8cb3a965841d15715dc76f7bd3d276a6 Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Sat, 17 Aug 2024 00:34:32 -0300 Subject: [PATCH 1/7] Keep compatibility with old FAB versions --- airflow/models/dag.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index 518b367067ef7..2342f094bff19 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -938,8 +938,16 @@ def update_old_perm(permission: str): for role, perms in access_control.items(): updated_access_control[role] = updated_access_control.get(role, {}) if isinstance(perms, (set, list)): - # Support for old-style access_control where only the actions are specified - updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + if hasattr(permissions, "resource_name"): + # Support old versions of FAB provider + updated_access_control[role] = {update_old_perm(perm) for perm in perms} + else: + # Support for old-style access_control where only the actions are specified + updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + elif isinstance(perms, dict) and hasattr(permissions, "resource_name"): + raise AirflowException( + f"Please update FAB provider to use new-style access_control: {access_control}" + ) else: updated_access_control[role] = perms if permissions.RESOURCE_DAG in updated_access_control[role]: From 0023963088477680cc29464ffc5ab181d947fb49 Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Sat, 17 Aug 2024 00:52:38 -0300 Subject: [PATCH 2/7] Revert --- airflow/models/dag.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index 2342f094bff19..518b367067ef7 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -938,16 +938,8 @@ def update_old_perm(permission: str): for role, perms in access_control.items(): updated_access_control[role] = updated_access_control.get(role, {}) if isinstance(perms, (set, list)): - if hasattr(permissions, "resource_name"): - # Support old versions of FAB provider - updated_access_control[role] = {update_old_perm(perm) for perm in perms} - else: - # Support for old-style access_control where only the actions are specified - updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) - elif isinstance(perms, dict) and hasattr(permissions, "resource_name"): - raise AirflowException( - f"Please update FAB provider to use new-style access_control: {access_control}" - ) + # Support for old-style access_control where only the actions are specified + updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) else: updated_access_control[role] = perms if permissions.RESOURCE_DAG in updated_access_control[role]: From 105c7bd5219f8b4136f501191bb007b3ad45ca9b Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Mon, 19 Aug 2024 13:40:52 -0300 Subject: [PATCH 3/7] Use old access control format in FAB versions < 1.3.0 --- airflow/models/dag.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index 518b367067ef7..dfcf9f2b6f6c7 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -57,6 +57,7 @@ import re2 import sqlalchemy_jsonfield from dateutil.relativedelta import relativedelta +from packaging import version as packaging_version from sqlalchemy import ( Boolean, Column, @@ -116,6 +117,7 @@ clear_task_instances, ) from airflow.models.tasklog import LogTemplate +from airflow.providers.fab import __version__ as FAB_VERSION from airflow.secrets.local_filesystem import LocalFilesystemBackend from airflow.security import permissions from airflow.settings import json @@ -936,16 +938,26 @@ def update_old_perm(permission: str): updated_access_control = {} for role, perms in access_control.items(): - updated_access_control[role] = updated_access_control.get(role, {}) - if isinstance(perms, (set, list)): - # Support for old-style access_control where only the actions are specified - updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + if packaging_version.parse(FAB_VERSION) >= packaging_version.parse("1.3.0"): + updated_access_control[role] = updated_access_control.get(role, {}) + if isinstance(perms, (set, list)): + # Support for old-style access_control where only the actions are specified + updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + else: + updated_access_control[role] = perms + if permissions.RESOURCE_DAG in updated_access_control[role]: + updated_access_control[role][permissions.RESOURCE_DAG] = { + update_old_perm(perm) + for perm in updated_access_control[role][permissions.RESOURCE_DAG] + } + elif isinstance(perms, dict): + # Not allow new access control format with old FAB versions + raise AirflowException( + "Please upgrade the FAB provider to a version >= 1.3.0 to allow " + 'use other resources than "DAGs" in the Dag Level Access Control.' + ) else: - updated_access_control[role] = perms - if permissions.RESOURCE_DAG in updated_access_control[role]: - updated_access_control[role][permissions.RESOURCE_DAG] = { - update_old_perm(perm) for perm in updated_access_control[role][permissions.RESOURCE_DAG] - } + updated_access_control[role] = {update_old_perm(perm) for perm in perms} return updated_access_control From 1b7b052d9cbf146c1e5aa5bb5eef6cd283f95611 Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Sat, 17 Aug 2024 00:34:32 -0300 Subject: [PATCH 4/7] Keep compatibility with old FAB versions --- airflow/models/dag.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index 518b367067ef7..2342f094bff19 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -938,8 +938,16 @@ def update_old_perm(permission: str): for role, perms in access_control.items(): updated_access_control[role] = updated_access_control.get(role, {}) if isinstance(perms, (set, list)): - # Support for old-style access_control where only the actions are specified - updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + if hasattr(permissions, "resource_name"): + # Support old versions of FAB provider + updated_access_control[role] = {update_old_perm(perm) for perm in perms} + else: + # Support for old-style access_control where only the actions are specified + updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + elif isinstance(perms, dict) and hasattr(permissions, "resource_name"): + raise AirflowException( + f"Please update FAB provider to use new-style access_control: {access_control}" + ) else: updated_access_control[role] = perms if permissions.RESOURCE_DAG in updated_access_control[role]: From b18fb589060fb3cfc791931b9c239ea7f60f41b0 Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Sat, 17 Aug 2024 00:52:38 -0300 Subject: [PATCH 5/7] Revert --- airflow/models/dag.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index 2342f094bff19..518b367067ef7 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -938,16 +938,8 @@ def update_old_perm(permission: str): for role, perms in access_control.items(): updated_access_control[role] = updated_access_control.get(role, {}) if isinstance(perms, (set, list)): - if hasattr(permissions, "resource_name"): - # Support old versions of FAB provider - updated_access_control[role] = {update_old_perm(perm) for perm in perms} - else: - # Support for old-style access_control where only the actions are specified - updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) - elif isinstance(perms, dict) and hasattr(permissions, "resource_name"): - raise AirflowException( - f"Please update FAB provider to use new-style access_control: {access_control}" - ) + # Support for old-style access_control where only the actions are specified + updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) else: updated_access_control[role] = perms if permissions.RESOURCE_DAG in updated_access_control[role]: From 5764da613c973a9c484201a80c789f87df4c9a6b Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Mon, 19 Aug 2024 13:40:52 -0300 Subject: [PATCH 6/7] Use old access control format in FAB versions < 1.3.0 --- airflow/models/dag.py | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index 518b367067ef7..dfcf9f2b6f6c7 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -57,6 +57,7 @@ import re2 import sqlalchemy_jsonfield from dateutil.relativedelta import relativedelta +from packaging import version as packaging_version from sqlalchemy import ( Boolean, Column, @@ -116,6 +117,7 @@ clear_task_instances, ) from airflow.models.tasklog import LogTemplate +from airflow.providers.fab import __version__ as FAB_VERSION from airflow.secrets.local_filesystem import LocalFilesystemBackend from airflow.security import permissions from airflow.settings import json @@ -936,16 +938,26 @@ def update_old_perm(permission: str): updated_access_control = {} for role, perms in access_control.items(): - updated_access_control[role] = updated_access_control.get(role, {}) - if isinstance(perms, (set, list)): - # Support for old-style access_control where only the actions are specified - updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + if packaging_version.parse(FAB_VERSION) >= packaging_version.parse("1.3.0"): + updated_access_control[role] = updated_access_control.get(role, {}) + if isinstance(perms, (set, list)): + # Support for old-style access_control where only the actions are specified + updated_access_control[role][permissions.RESOURCE_DAG] = set(perms) + else: + updated_access_control[role] = perms + if permissions.RESOURCE_DAG in updated_access_control[role]: + updated_access_control[role][permissions.RESOURCE_DAG] = { + update_old_perm(perm) + for perm in updated_access_control[role][permissions.RESOURCE_DAG] + } + elif isinstance(perms, dict): + # Not allow new access control format with old FAB versions + raise AirflowException( + "Please upgrade the FAB provider to a version >= 1.3.0 to allow " + 'use other resources than "DAGs" in the Dag Level Access Control.' + ) else: - updated_access_control[role] = perms - if permissions.RESOURCE_DAG in updated_access_control[role]: - updated_access_control[role][permissions.RESOURCE_DAG] = { - update_old_perm(perm) for perm in updated_access_control[role][permissions.RESOURCE_DAG] - } + updated_access_control[role] = {update_old_perm(perm) for perm in perms} return updated_access_control From f628934a10c088ce441a07464ea43d8fcc91d217 Mon Sep 17 00:00:00 2001 From: Joao Amaral <7281460+joaopamaral@users.noreply.github.com> Date: Mon, 19 Aug 2024 15:09:30 -0300 Subject: [PATCH 7/7] add tests --- airflow/models/dag.py | 2 +- tests/models/test_dag.py | 57 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/airflow/models/dag.py b/airflow/models/dag.py index dfcf9f2b6f6c7..b685b28343eb9 100644 --- a/airflow/models/dag.py +++ b/airflow/models/dag.py @@ -954,7 +954,7 @@ def update_old_perm(permission: str): # Not allow new access control format with old FAB versions raise AirflowException( "Please upgrade the FAB provider to a version >= 1.3.0 to allow " - 'use other resources than "DAGs" in the Dag Level Access Control.' + "use the Dag Level Access Control new format." ) else: updated_access_control[role] = {update_old_perm(perm) for perm in perms} diff --git a/tests/models/test_dag.py b/tests/models/test_dag.py index 0bd58ddc528d3..4994e4545ddc6 100644 --- a/tests/models/test_dag.py +++ b/tests/models/test_dag.py @@ -2539,6 +2539,63 @@ def test_replace_outdated_access_control_actions(self): assert "permission is deprecated" in str(deprecation_warnings[0].message) assert "permission is deprecated" in str(deprecation_warnings[1].message) + @pytest.mark.parametrize( + "fab_version, perms, expected_exception, expected_perms", + [ + pytest.param( + "1.2.0", + { + "role1": {permissions.ACTION_CAN_READ, permissions.ACTION_CAN_EDIT}, + "role3": {permissions.RESOURCE_DAG_RUN: {permissions.ACTION_CAN_CREATE}}, + # will raise error in old FAB with new access control format + }, + AirflowException, + None, + id="old_fab_new_access_control_format", + ), + pytest.param( + "1.2.0", + { + "role1": [ + permissions.ACTION_CAN_READ, + permissions.ACTION_CAN_EDIT, + permissions.ACTION_CAN_READ, + ], + }, + None, + {"role1": {permissions.ACTION_CAN_READ, permissions.ACTION_CAN_EDIT}}, + id="old_fab_old_access_control_format", + ), + pytest.param( + "1.3.0", + { + "role1": {permissions.ACTION_CAN_READ, permissions.ACTION_CAN_EDIT}, # old format + "role3": {permissions.RESOURCE_DAG_RUN: {permissions.ACTION_CAN_CREATE}}, # new format + }, + None, + { + "role1": { + permissions.RESOURCE_DAG: {permissions.ACTION_CAN_READ, permissions.ACTION_CAN_EDIT} + }, + "role3": {permissions.RESOURCE_DAG_RUN: {permissions.ACTION_CAN_CREATE}}, + }, + id="new_fab_mixed_access_control_format", + ), + ], + ) + def test_access_control_format(self, fab_version, perms, expected_exception, expected_perms): + if expected_exception: + with patch("airflow.models.dag.FAB_VERSION", fab_version): + with pytest.raises( + expected_exception, + match="Please upgrade the FAB provider to a version >= 1.3.0 to allow use the Dag Level Access Control new format.", + ): + DAG(dag_id="dag_test", schedule=None, access_control=perms) + else: + with patch("airflow.models.dag.FAB_VERSION", fab_version): + dag = DAG(dag_id="dag_test", schedule=None, access_control=perms) + assert dag.access_control == expected_perms + def test_validate_executor_field_executor_not_configured(self): dag = DAG("test-dag", schedule=None) EmptyOperator(task_id="t1", dag=dag, executor="test.custom.executor")