This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new aae4a83cdf Fix permissions of parent folders for log file handler
(#37310)
aae4a83cdf is described below
commit aae4a83cdfb3be4afeefd88a7bfa3c4d8d184958
Author: Jarek Potiuk <[email protected]>
AuthorDate: Sat Feb 10 20:46:55 2024 +0100
Fix permissions of parent folders for log file handler (#37310)
When we created a new folder during log file handler directory creation
we did not change the permissions of parent folders.
Fixes: #37200
---
airflow/utils/log/file_task_handler.py | 37 ++++++++++++++++++++++++----------
tests/utils/test_log_handlers.py | 30 +++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/airflow/utils/log/file_task_handler.py
b/airflow/utils/log/file_task_handler.py
index 6151d0c15f..7e1faca4ec 100644
--- a/airflow/utils/log/file_task_handler.py
+++ b/airflow/utils/log/file_task_handler.py
@@ -159,6 +159,31 @@ def _ensure_ti(ti: TaskInstanceKey | TaskInstance,
session) -> TaskInstance:
raise AirflowException(f"Could not find TaskInstance for {ti}")
+def _change_directory_permissions_up(directory: Path, folder_permissions: int):
+ """
+ Change permissions of the given directory and its parents.
+
+ Only attempt to change permissions for directories owned by the current
user.
+
+ :param directory: directory to change permissions of (including parents)
+ :param folder_permissions: permissions to set
+ """
+ if directory.stat().st_uid == os.getuid():
+ if directory.stat().st_mode % 0o1000 != folder_permissions % 0o1000:
+ print(f"Changing {directory} permission to {folder_permissions}")
+ try:
+ directory.chmod(folder_permissions)
+ except PermissionError as e:
+ # In some circumstances (depends on user and filesystem) we
might not be able to
+ # change the permission for the folder (when the folder was
created by another user
+ # before or when the filesystem does not allow to change
permission). We should not
+ # fail in this case but rather ignore it.
+ print(f"Failed to change {directory} permission to
{folder_permissions}: {e}")
+ return
+ if directory.parent != directory:
+ _change_directory_permissions_up(directory.parent,
folder_permissions)
+
+
class FileTaskHandler(logging.Handler):
"""
FileTaskHandler is a python log handler that handles and reads task
instance logs.
@@ -484,17 +509,7 @@ class FileTaskHandler(logging.Handler):
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 % 0o1000 != new_folder_permissions %
0o1000:
- print(f"Changing {directory} permission to
{new_folder_permissions}")
- try:
- directory.chmod(new_folder_permissions)
- except PermissionError as e:
- # In some circumstances (depends on user and filesystem) we
might not be able to
- # change the permission for the folder (when the folder was
created by another user
- # before or when the filesystem does not allow to change
permission). We should not
- # fail in this case but rather ignore it.
- print(f"Failed to change {directory} permission to
{new_folder_permissions}: {e}")
- pass
+ _change_directory_permissions_up(directory, new_folder_permissions)
def _init_file(self, ti, *, identifier: str | None = None):
"""
diff --git a/tests/utils/test_log_handlers.py b/tests/utils/test_log_handlers.py
index 4b47c4ac49..36810506da 100644
--- a/tests/utils/test_log_handlers.py
+++ b/tests/utils/test_log_handlers.py
@@ -21,6 +21,8 @@ import logging
import logging.config
import os
import re
+import shutil
+import tempfile
from pathlib import Path
from unittest import mock
from unittest.mock import patch
@@ -40,6 +42,7 @@ from airflow.operators.python import PythonOperator
from airflow.utils.log.file_task_handler import (
FileTaskHandler,
LogType,
+ _change_directory_permissions_up,
_interleave_logs,
_parse_timestamps_in_log_file,
)
@@ -719,3 +722,30 @@ def test_interleave_logs_correct_ordering():
"""
assert sample_with_dupe == "\n".join(_interleave_logs(sample_with_dupe,
"", sample_with_dupe))
+
+
+def test_permissions_for_new_directories():
+ tmp_path = Path(tempfile.mkdtemp())
+ try:
+ # Set umask to 0o027: owner rwx, group rx-w, other -rwx
+ old_umask = os.umask(0o027)
+ try:
+ subdir = tmp_path / "subdir1" / "subdir2"
+ # force permissions for the new folder to be owner rwx, group
-rxw, other -rwx
+ new_folder_permissions = 0o700
+ # default permissions are owner rwx, group rx-w, other -rwx (umask
bit negative)
+ default_permissions = 0o750
+ subdir.mkdir(mode=new_folder_permissions, parents=True,
exist_ok=True)
+ assert subdir.exists()
+ assert subdir.is_dir()
+ assert subdir.stat().st_mode % 0o1000 == new_folder_permissions
+ # initially parent permissions are as per umask
+ assert subdir.parent.stat().st_mode % 0o1000 == default_permissions
+ _change_directory_permissions_up(subdir, new_folder_permissions)
+ assert subdir.stat().st_mode % 0o1000 == new_folder_permissions
+ # now parent permissions are as per new_folder_permissions
+ assert subdir.parent.stat().st_mode % 0o1000 ==
new_folder_permissions
+ finally:
+ os.umask(old_umask)
+ finally:
+ shutil.rmtree(tmp_path)
