pyinfra-dev / pyinfra

pyinfra turns Python code into shell commands and runs them on your servers. Execute ad-hoc commands and write declarative operations. Target SSH servers, local machine and Docker containers. Fast and scales from one server to thousands.
https://pyinfra.com
MIT License
3.91k stars 382 forks source link

Normalise non-octal integer file modes #829

Open julienlavergne opened 2 years ago

julienlavergne commented 2 years ago

Describe the bug

When using files.directory with mode="u=rwx,g=rwx,o=rwx", the operation will always perform a chmod on the existing directory, even though the permissions are correct. This is due to the fact that we compare the current permissions 777 to the requested mode u=rwx,g=rwx,o=rwx, which are different strings.

To Reproduce

files.directory(
    name="Create directory",
    path="/home/mydirectory",
    mode="u=rwx,g=rwx,o=rwx",
)
--> Preparing operations...
    Loading: tasks/directory.py
    [pyinfra.api.operation] Adding operation, {'Create directory'}, opOrder=(0, 25), opHash=d01788291140ae301495a252443951382c4e61df
    [pyinfra.api.facts] Getting fact: files.Directory (path=/mydirectory) (ensure_hosts: None)
    [pyinfra.connectors.ssh] Running command on host07: (pty=None) sh -c '! (test -e /mydirectory || test -L /mydirectory ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /mydirectory 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /mydirectory )'
[host07] >>> sh -c '! (test -e /mydirectory || test -L /mydirectory ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' /mydirectory 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' /mydirectory )'
[host07] user=myuser group=users mode=drwxrwxrwx atime=1654053482 mtime=1654053482 ctime=1654053501 size=6 '/mydirectory'
    [pyinfra.connectors.ssh] Waiting for exit status...
    [pyinfra.connectors.ssh] Command exit status: 0
    [host07] Loaded fact files.Directory (path=/mydirectory)
u=rwx,g=rwx,o=rwx {'user': 'myuser', 'group': 'users', 'mode': 777, 'atime': datetime.datetime(2022, 6, 1, 3, 18, 2), 'mtime': datetime.datetime(2022, 6, 1, 3, 18, 2), 'ctime': datetime.datetime(2022, 6, 1, 3, 18, 21), 'size': 6}
    [host07] Ready: tasks/directory.py

--> Proposed changes:
    Groups: ** / **
    [host07]   Operations: 1   Commands: 1

--> Beginning operation run...
--> Starting operation: Create directory
    [pyinfra.api.operations] Starting operation Create directory on host07
    [pyinfra.connectors.ssh] Running command on host07: (pty=None) sh -c 'chmod u=rwx,g=rwx,o=rwx /mydirectory'
[host07] >>> sh -c 'chmod u=rwx,g=rwx,o=rwx /mydirectory'
    [pyinfra.connectors.ssh] Waiting for exit status...
    [pyinfra.connectors.ssh] Command exit status: 0
    [host07] Success

--> Results:
    Groups: ** / **
    [host07]   Successful: 1   Errors: 0   Commands: 1/1

Expected behavior

We should be able to see that 777 and u=rwx,g=rwx,o=rwx are the same.

Meta

v2.1

Fizzadar commented 2 years ago

This should definitely work as expected; this part of the codebase hasn't been touched in a very long time; here's the attempted normalisation:

https://github.com/Fizzadar/pyinfra/blob/dfad6ef512781407187ef91434388e6d705f2d3c/pyinfra/operations/util/files.py#L12-L25

The files.* facts all output octal values so would be good to have the above function also convert other variants such as your example.

julienlavergne commented 2 years ago

This should definitely work as expected; this part of the codebase hasn't been touched in a very long time; here's the attempted normalisation:

Do you mean that the existing code should be enough to have my example working ?

Fizzadar commented 2 years ago

Do you mean that the existing code should be enough to have my example working ?

If/once #830 is resolved it should work, but that is required first to normalise the different mode style.

Fizzadar commented 2 years ago

Updating this issue to reflect the need to parse non integer mode values for comparison with fact data to fix this. Currently mode must be an octal value for changes to be calculated correctly; otherwise chmod will always be executed.

Will first show a warning for this and document it. Actually doing the normalise is quite complex because it requires knowing the default values from umask.