Skip to content

Conversation

@SamWheating
Copy link
Contributor

@SamWheating SamWheating commented Mar 2, 2023

While investigating #29112 I found another issue in the way that the file task handler checks file access:

new_folder_permissions = int(
conf.get("logging", "file_task_handler_new_folder_permissions", fallback="0o775"), 8
)
directory.mkdir(mode=new_folder_permissions, parents=True, exist_ok=True)
if directory.stat().st_mode != new_folder_permissions:
print(f"Changing dir permission to {new_folder_permissions}")
directory.chmod(new_folder_permissions)

The return value of path.stat().st_mode will include additional bits beyond the tailing 9 access bits (-RWXRWXRWX), which can lead to running chmod when it isn't required, as demonstrated here:

>>> from pathlib import Path

>>> p = Path('/opt/airflow/logs/task/mytask')

>>> new_folder_permissions = 0o755

>>> p.mkdir(mode=new_folder_permissions, parents=True, exist_ok=True)

>>> p.stat().st_mode == new_folder_permissions   # Should return true, because we just created this directory
False

>>> format(p.stat().st_mode, 'o')   # Lets look at the full value of `std_mode`, oct-formatted
'42755'

>>> p.stat().st_mode % 0o1000 == new_folder_permissions   # selecting only the last 9 bits
True

I'm not sure if this is the main cause of #29112, as there could be remaining issues around kubernetes security groups etc, but I think that this bug could definitely lead to unnecessarily chmod'ing directories without being the owner which could then throw the reported errors.

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.

I think & 0o777 is more common but this works as well.

@potiuk potiuk merged commit ec31648 into apache:main Mar 4, 2023
@pierrejeambrun pierrejeambrun added this to the Airflow 2.5.3 milestone Mar 22, 2023
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logging type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants