Skip to content

Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat#64933

Open
jedcunningham wants to merge 3 commits intoapache:mainfrom
astronomer:fix_common_compat_af2
Open

Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat#64933
jedcunningham wants to merge 3 commits intoapache:mainfrom
astronomer:fix_common_compat_af2

Conversation

@jedcunningham
Copy link
Copy Markdown
Member

@jedcunningham jedcunningham commented Apr 8, 2026

Summary

Fixes a regression introduced by #63335, which hardcoded RESOURCE_ASSET = "Assets"
in providers/common/compat/security/permissions.py. This value is correct for
Airflow 3, but breaks Airflow 2.x in two ways:

  1. Wrong resource name in Airflow 2.x. apache-airflow-providers-fab imports
    RESOURCE_ASSET from this module at runtime. FAB's role sync updates stock roles
    to grant the "Assets" resource, but the correct Airflow 2.x name is "Datasets".
    This creates an inconsistency and pollutes the permission model with a resource that
    doesn't belong in Airflow 2.x.

  2. Custom roles break. Any role with "Datasets" permissions stops working, since
    auth checks now look for "Assets". While you could re-grant permissions against
    "Assets" after upgrading, existing roles are silently broken.

Fix

Gate on AIRFLOW_V_3_0_PLUS: return "Assets" for Airflow 3, and fall back to
RESOURCE_DATASET from airflow.security.permissions (not deprecated in Airflow 2.x)
for Airflow 2.x.

Related


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code

PR apache#63335 hardcoded RESOURCE_ASSET = "Assets" in this compat module,
which broke Airflow 2.x deployments. In Airflow 2.x, the equivalent
resource is named "Datasets" (RESOURCE_DATASET), not "Assets".

apache-airflow-providers-fab imports RESOURCE_ASSET from this module at
runtime (via the `else` branch of an `if TYPE_CHECKING` block).
When RESOURCE_ASSET resolves to "Assets" instead of "Datasets", it creates
duplicate "Assets" and "Datasets" resource types for upgraded instances.
And worse "Assets", which dont even exist in AF2, are used for auth checks.
This also breaks any custom roles.

Fix: use AIRFLOW_V_3_0_PLUS to return the correct value for each
version, falling back to RESOURCE_DATASET from airflow.security.permissions
(which is not deprecated in Airflow 2.x) for the Airflow 2.x case.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 8, 2026

I guess that means rc2 for common.compat and the two providers that depend on it's new version from the current vote. Thanks for finding and fixing!

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 8, 2026

BTW. I do think that the if AIRFLOW* pattern with version is way bettter than "Pythonic" try/except - and this is the reason I was pushing for it @ashb (and @xBis7 as you asked) - the main reason for having this pattern is to know exactly when you can remove it - one of the reasons this one happened was that it was not at all clear if we can remove that try/remove.

Also we will have to find out why this was not detected by compatibility tests - it should have been, but possibly this kind of issue was masked by something. To be diagnosed.

@jedcunningham
Copy link
Copy Markdown
Member Author

I suspect it wasn't found in the compatibility tests because it caused "Assets" resources and permissions to be created alongside the "Dataset" ones that should have been used. And if you didn't have a custom role (or maybe modified the existing roles) without the dataset perms, its hidden because it "just works", even if its fundamentally wrong.

Suppress attr-defined (RESOURCE_DATASET doesn't exist in Airflow 3 stubs,
where mypy runs) and no-redef (RESOURCE_ASSET defined in both if/else
branches) on the Airflow 2.x fallback import.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@jedcunningham
Copy link
Copy Markdown
Member Author

An example after the upgrade (all default roles get this) (this is before this fix of course):

Screenshot 2026-04-09 at 9 23 27 AM

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 9, 2026

There is a mypy issue for providers :(

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a compatibility regression in the common-compat provider where RESOURCE_ASSET was hardcoded to the Airflow 3 resource name, which breaks permission/role behavior on Airflow 2.x.

Changes:

  • Introduces AIRFLOW_V_3_0_PLUS gating to set RESOURCE_ASSET to "Assets" on Airflow 3.
  • For Airflow 2.x, maps RESOURCE_ASSET back to airflow.security.permissions.RESOURCE_DATASET to preserve the "Datasets" permission resource name.

Comment on lines +26 to +31
if AIRFLOW_V_3_0_PLUS:
RESOURCE_ASSET = "Assets"
else:
from airflow.security.permissions import (
RESOURCE_DATASET as RESOURCE_ASSET, # noqa: F401 # type: ignore[attr-defined, no-redef]
)
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

The conditional import of RESOURCE_DATASET happens after module-level assignments (RESOURCE_BACKFILL, etc.), which will trigger Ruff E402 (imports not at top of file) for this module. To avoid CI/lint failures, move the if AIRFLOW_V_3_0_PLUS block (and the import) above the resource constant assignments, or otherwise ensure the import occurs before any non-import statements (alternatively, use a literal string for the AF2 value if you want to avoid conditional importing).

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +31
if AIRFLOW_V_3_0_PLUS:
RESOURCE_ASSET = "Assets"
else:
from airflow.security.permissions import (
RESOURCE_DATASET as RESOURCE_ASSET, # noqa: F401 # type: ignore[attr-defined, no-redef]
)
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

This change fixes a version-dependent permission constant, but there’s no regression test that asserts the value of RESOURCE_ASSET for the running Airflow major version (the existing import-only test won’t catch the Airflow 2.x vs 3.x mismatch). Add an assertion-based unit test that checks the expected value under Airflow 2.x vs 3.x (e.g., branch on AIRFLOW_V_3_0_PLUS).

Copilot generated this review using guidance from repository custom instructions.
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.

6 participants