Skip to content

Commit 5b27d82

Browse files
Copilotnjzjz
andauthored
fix: Replace rsync -az with -rlptDz to avoid permission issues (#547)
The rsync function was using the `-a` (archive) flag which includes `-o` (preserve owner) and `-g` (preserve group) options that require superuser privileges to change file ownership. This caused "Operation not permitted" errors when downloading files from remote systems to local environments where users lack such privileges. **Problem:** ``` rsync: chown "/path/to/file.tar.gz.temp" failed: Operation not permitted (1) rsync error: some files/attrs were not transferred (see previous errors) (code 23) ``` **Root Cause:** The `-a` flag is equivalent to `-rlptgoD`, where `-o` (preserve owner) and `-g` (preserve group) cause permission issues on systems where users cannot change file ownership. **Solution:** Replace `-az` with `-rlptDz` to maintain all useful archive functionality while excluding the problematic owner and group preservation options: - `-r`: recursive - `-l`: copy symlinks as symlinks - `-p`: preserve permissions - `-t`: preserve modification times - `-D`: preserve device files and special files - `-z`: compress during transfer **Changes:** - Modified `dpdispatcher/utils/utils.py` to use `-rlptDz` flags instead of `-az` - Added comprehensive test coverage in `tests/test_rsync_flags.py` - Updated comments to explain the permission issue fix **Testing:** - All existing tests pass with no regression - New unit tests verify correct flag usage - CLI functionality confirmed working - Local rsync operations tested successfully Fixes #434. <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: njzjz <[email protected]>
1 parent 9d0f1a5 commit 5b27d82

File tree

2 files changed

+90
-3
lines changed

2 files changed

+90
-3
lines changed

dpdispatcher/utils/utils.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,9 @@ def rsync(
138138

139139
cmd = [
140140
"rsync",
141-
# -a: archieve
142-
# -z: compress
143-
"-az",
141+
# -r: recursive, -l: links, -p: perms, -t: times, -D: devices/specials
142+
# -z: compress (exclude -o: owner, -g: group to avoid permission issues)
143+
"-rlptDz",
144144
"-e",
145145
ssh_cmd_str,
146146
"-q",

tests/test_rsync_flags.py

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
import os
2+
import sys
3+
import unittest
4+
from unittest.mock import patch
5+
6+
sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "..")))
7+
__package__ = "tests"
8+
9+
from dpdispatcher.utils.utils import rsync
10+
11+
12+
class TestRsyncFlags(unittest.TestCase):
13+
"""Test rsync function flags to ensure correct options are used."""
14+
15+
@patch("dpdispatcher.utils.utils.run_cmd_with_all_output")
16+
def test_rsync_flags_exclude_owner_group(self, mock_run_cmd):
17+
"""Test that rsync uses flags that exclude owner and group preservation."""
18+
# Mock successful command execution
19+
mock_run_cmd.return_value = (0, "", "")
20+
21+
# Call rsync function
22+
rsync("source_file", "dest_file", key_filename="test_key")
23+
24+
# Verify the command was called
25+
mock_run_cmd.assert_called_once()
26+
27+
# Get the command that was executed
28+
called_cmd = mock_run_cmd.call_args[0][0]
29+
30+
# Verify the command contains the correct flags
31+
self.assertIn("-rlptDz", called_cmd)
32+
self.assertNotIn("-az", called_cmd)
33+
34+
# Verify rsync command structure
35+
self.assertIn("rsync", called_cmd)
36+
self.assertIn("source_file", called_cmd)
37+
self.assertIn("dest_file", called_cmd)
38+
self.assertIn("-e", called_cmd)
39+
self.assertIn("-q", called_cmd)
40+
41+
@patch("dpdispatcher.utils.utils.run_cmd_with_all_output")
42+
def test_rsync_with_proxy_command_flags(self, mock_run_cmd):
43+
"""Test that rsync uses correct flags even with proxy command."""
44+
# Mock successful command execution
45+
mock_run_cmd.return_value = (0, "", "")
46+
47+
# Call rsync function with proxy command
48+
rsync(
49+
"source_file",
50+
"dest_file",
51+
key_filename="test_key",
52+
proxy_command="ssh -W target:22 jump_host",
53+
)
54+
55+
# Verify the command was called
56+
mock_run_cmd.assert_called_once()
57+
58+
# Get the command that was executed
59+
called_cmd = mock_run_cmd.call_args[0][0]
60+
61+
# Verify the command contains the correct flags
62+
self.assertIn("-rlptDz", called_cmd)
63+
self.assertNotIn("-az", called_cmd)
64+
65+
@patch("dpdispatcher.utils.utils.run_cmd_with_all_output")
66+
def test_rsync_error_handling(self, mock_run_cmd):
67+
"""Test that rsync properly handles errors."""
68+
# Mock failed command execution
69+
mock_run_cmd.return_value = (
70+
23,
71+
"",
72+
"rsync: chown failed: Operation not permitted",
73+
)
74+
75+
# Call rsync function and expect RuntimeError
76+
with self.assertRaises(RuntimeError) as context:
77+
rsync("source_file", "dest_file")
78+
79+
# Verify error message contains the command and error
80+
self.assertIn("Failed to run", str(context.exception))
81+
self.assertIn(
82+
"rsync: chown failed: Operation not permitted", str(context.exception)
83+
)
84+
85+
86+
if __name__ == "__main__":
87+
unittest.main()

0 commit comments

Comments
 (0)