pyinfra icon indicating copy to clipboard operation
pyinfra copied to clipboard

Normalise non-octal integer file modes

Open julienlavergne opened this issue 3 years ago • 4 comments

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

julienlavergne avatar Jun 01 '22 03:06 julienlavergne

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.

Fizzadar avatar Jun 02 '22 11:06 Fizzadar

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 ?

julienlavergne avatar Jun 06 '22 05:06 julienlavergne

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 avatar Jun 19 '22 07:06 Fizzadar

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.

Fizzadar avatar Nov 03 '22 21:11 Fizzadar