Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues related to access_control={} #34114

Merged

Conversation

SamWheating
Copy link
Contributor

@SamWheating SamWheating commented Sep 5, 2023

Follow-up to #33632, which only tested the relevant code in isolation (my bad)

While verifying this behaviour in order to test the new release, I noticed a few things -

  1. The sync_perm --include-dags CLI command was ignoring DAGs with empty (but not None) access_control
  2. Empty access_control blocks were being overwritten with None during DAG initialization.
  3. Loading a serialized DAG from the DB replaces access_control={} with access_control=None, which appears to be intentional behaviour originating from this function:

def _value_is_hardcoded_default(cls, attrname: str, value: Any, instance: Any) -> bool:
"""
Return true if ``value`` is the hard-coded default for the given attribute.
This takes in to account cases where the ``max_active_tasks`` parameter is
stored in the ``_max_active_tasks`` attribute.
And by using `is` here only and not `==` this copes with the case a
user explicitly specifies an attribute with the same "value" as the
default. (This is because ``"default" is "default"`` will be False as
they are different strings with the same characters.)
Also returns True if the value is an empty list or empty dict. This is done
to account for the case where the default value of the field is None but has the
``field = field or {}`` set.
"""
if attrname in cls._CONSTRUCTOR_PARAMS and (
cls._CONSTRUCTOR_PARAMS[attrname] is value or (value in [{}, []])
):
return True
return False

With this in mind, is there a preferred way to explicitly enable the serialization/deserialization of empty dicts in the _access_control field? For now I've just overridden the method in the SerializedDAG class, but this feels a bit messy.

Also added a test to replicate this issue.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Sep 5, 2023
@SamWheating SamWheating force-pushed the sw-allow-setting-empty-access-control-on-dags branch from bdf12aa to 4ec62d1 Compare September 5, 2023 21:12
@SamWheating SamWheating changed the title (WIP) Allow users to set empty access control on DAGs (WIP) Fix issues related to access_control={} Sep 5, 2023
@SamWheating SamWheating changed the title (WIP) Fix issues related to access_control={} Fix issues related to access_control={} Sep 6, 2023
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Nice!

@SamWheating
Copy link
Contributor Author

cc @ephraimbuddy - this fixes bugs present in 2.7.1rc2.

I think that we should either remove the flawed changes from #33632 or add this fix into the release. Thoughts?

@ephraimbuddy
Copy link
Contributor

cc @ephraimbuddy - this fixes bugs present in 2.7.1rc2.

I think that we should either remove the flawed changes from #33632 or add this fix into the release. Thoughts?

Apart from not working as intended, does #33632 cause a regression?

@potiuk
Copy link
Member

potiuk commented Sep 7, 2023

Apart from not working as intended, does #33632 cause a regression?

I think it partially fixes problems present in 2.6.2+ and the cases mentioned by @SamWheating are just specific cases where permission sync would give wrong output. I do not think it requires cancellation of vote - it could be released in 2.7.2 IMHO (but I think we should release 2.7.2 pretty fix.

@potiuk potiuk added this to the Airflow 2.7.2 milestone Sep 7, 2023
@potiuk potiuk merged commit a61b5e8 into apache:main Sep 7, 2023
42 checks passed
@SamWheating
Copy link
Contributor Author

Apart from not working as intended, does #33632 cause a regression?

No regressions, it just doesn't behave as the documentation describes, which could be misleading. I'm also fine with just leaving the fix until 2.7.2.

@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Oct 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 5, 2023
* allow empty access control on dags

* Add test which demonstrates loss of information during ser/de

* Keep empty value when serializing access_control dict

(cherry picked from commit a61b5e8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants