Setting TEMP_DIR config from deploy is not taken into account by `get_temp_filename`

Renerick commented 11 months ago

Describe the bug

In the past I have already reported similar bug (#761) and it was fixed, but today I encountered it again

When trying to upload or template a file to remote host without /tmp access and when SUDO = True I receive an error that /tmp directory is not found even when other value of the TEMP_DIR setting is specified. Currently I'm experiencing this problem when trying to upload a configuration files to Synology NAS where direct SFTP has no access to file system other then user home directory

To Reproduce

import io

from pyinfra.operations import files
from pyinfra import config

config.TEMP_DIR = "tmp"

content = "Hello world"

files.put(io.StringIO(content), "test.txt", force=True, _sudo=True)

--> Loading config...

--> Loading inventory...
    [pyinfra_cli\inventory] Creating fake inventory...
    [pyinfra_cli\inventory] Checking possible group_data directory:[REDACTED]\Dev\homelab
    [pyinfra_cli\inventory] Checking possible group_data directory:[REDACTED]\Dev\homelab\src\synology

--> Connecting to hosts...
    [pyinfra\connectors\ssh] Connecting to: REDACTED ({'allow_agent': True, 'look_for_keys': True, 'hostname': 'REDACTED', '_pyinfra_ssh_forward_agent': None, '_pyinfra_ssh_config_file': None, '_pyinfra_ssh_known_hosts_file': None, '_pyinfra_ssh_strict_host_key_checking': None, '_pyinfra_ssh_paramiko_connect_kwargs': None, 'port': [REDACTED], 'timeout': 10 })
    [pyinfra\connectors\sshuserclient\client] Loading SSH config: None
    [REDACTED] Connected
    [pyinfra\api\state] Activating host: REDACTED

--> Preparing Operations...
    Loading: src\synology\mini.py
    [pyinfra\api\operation] Adding operation, {'Files/Put'}, opOrder=(0, 10), opHash=3b2f90227d43a40a2288506c2a99bdc5e71f32b2
    [pyinfra\api\facts] Getting fact: files.File (path=test.txt) (ensure_hosts: None)
    [pyinfra\connectors\ssh] Running command on REDACTED: (pty=False) sh -c '
temp=$(mktemp "${TMPDIR:=/tmp}/pyinfra-sudo-askpass-XXXXXXXXXXXX")
cat >"$temp"<<'"'"'__EOF__'"'"'
#!/bin/sh
printf '"'"'%s\n'"'"' "$PYINFRA_SUDO_PASSWORD"
__EOF__
chmod 755 "$temp"
echo "$temp"
'
    [pyinfra\connectors\ssh] Waiting for exit status...
    [pyinfra\connectors\ssh] Command exit status: 0
    [pyinfra\connectors\ssh] Running command on REDACTED: (pty=None) env SUDO_ASKPASS=tmp/pyinfra-sudo-askpass-W72v4KbQPe4V *** sudo -H -A -k sh -c 'export "PATH=/bin:/usr/local/bin" "TMPDIR=./tmp" && ! (test -e test.txt || test -L test.txt ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' test.txt 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' test.txt )'
[REDACTED] >>> env SUDO_ASKPASS=tmp/pyinfra-sudo-askpass-W72v4KbQPe4V *** sudo -H -A -k sh -c 'export "PATH=/bin:/usr/local/bin" "TMPDIR=./tmp" && ! (test -e test.txt || test -L test.txt ) || ( stat -c '"'"'user=%U group=%G mode=%A atime=%X mtime=%Y ctime=%Z size=%s %N'"'"' test.txt 2> /dev/null || stat -f '"'"'user=%Su group=%Sg mode=%Sp atime=%a mtime=%m ctime=%c size=%z %N%SY'"'"' test.txt )'
[REDACTED] user=[REDACTED] group=users mode=-rwx--x--x atime=1693171171 mtime=1693171192 ctime=1693171192 size=11 'test.txt'
    [pyinfra\connectors\ssh] Waiting for exit status...
    [pyinfra\connectors\ssh] Command exit status: 0
    [REDACTED] Loaded fact files.File (path=test.txt)
    [REDACTED] Ready: src\synology\mini.py

--> Proposed changes:
    Groups: inventory / synology
    [REDACTED]   Operations: 1   Change: 1   No change: 0

--> Beginning operation run...
--> Starting operation: Files/Put (<_io.StringIO object at 0x0000019716F6EB00>, test.txt, force=True)
    [pyinfra\api\operations] Starting operation {'Files/Put'} on REDACTED
    [pyinfra\connectors\ssh] Attempting upload of <_io.StringIO object at 0x0000019716F6EB00> to /tmp/pyinfra-4b6fcb2d521ef0fd442a5301e7932d16cc9f375a
    [pyinfra\connectors\ssh] Running command on REDACTED: (pty=None) env SUDO_ASKPASS=tmp/pyinfra-sudo-askpass-W72v4KbQPe4V *** sudo -H -A -k sh -c 'export "PATH=/bin:/usr/local/bin" "TMPDIR=./tmp" && cp /tmp/pyinfra-4b6fcb2d521ef0fd442a5301e7932d16cc9f375a test.txt'
[REDACTED] >>> env SUDO_ASKPASS=tmp/pyinfra-sudo-askpass-W72v4KbQPe4V *** sudo -H -A -k sh -c 'export "PATH=/bin:/usr/local/bin" "TMPDIR=./tmp" && cp /tmp/pyinfra-4b6fcb2d521ef0fd442a5301e7932d16cc9f375a test.txt'
[REDACTED] cp: cannot stat '/tmp/pyinfra-4b6fcb2d521ef0fd442a5301e7932d16cc9f375a': No such file or directory
    [pyinfra\connectors\ssh] Waiting for exit status...
    [pyinfra\connectors\ssh] Command exit status: 1
    File upload error: cp: cannot stat '/tmp/pyinfra-4b6fcb2d521ef0fd442a5301e7932d16cc9f375a': No such file or directory
    [REDACTED] Error: executed 0/1 commands
    [pyinfra\api\state] Failing hosts: REDACTED
    [pyinfra\connectors\ssh] Running command on REDACTED: (pty=False) sh -c 'rm -f tmp/pyinfra-sudo-askpass-W72v4KbQPe4V'
    [pyinfra\connectors\ssh] Waiting for exit status...
    [pyinfra\connectors\ssh] Command exit status: 0
No hosts remaining!
--> pyinfra error:

It appears the issue is caused by TEMP_DIR config value are not being propagated to get_temp_filename method. I noticed that context object in _parallel_load_hosts method contains correct values

However, the state object that generates the temporary file name does not

It appears that the root cause of this issue is state.config.copy() call. The context object with copied config is being passed to the deploy files, but the state method uses the original instance of the config. In fact, this issue most likely affects every single configuration value, not only TEMP_DIR. Changing state.config.copy() to state.config seems to fix the error with TEMP_DIR I'm facing at the moment, although I'm not sure if it breaks something somewhere else (probably state isolation between hosts). Regardless, it's quite lucky I didn't encounter this any sooner (probably because I was working with non-synology hosts and using explicit configuration values, passed as global arguments).

I could look into this myself, but, again, not sure what this may break as I'm just occasionally debug pyinfra, not that familiar with all the internals

Expected behavior

Upload successful

Meta

Include output of pyinfra --support.

--> Support information:

If you are having issues with pyinfra or wish to make feature requests, please
check out the GitHub issues at https://github.com/Fizzadar/pyinfra/issues .
When adding an issue, be sure to include the following:

System: Windows
  Platform: Windows-10-10.0.19043-SP0
  Release: 10
  Machine: AMD64
pyinfra: v2.7
Executable: [REDACTED]\Dev\homelab\.venv\Scripts\pyinfra.exe
Python: 3.11.4 (CPython, MSC v.1934 64 bit (AMD64))

How was pyinfra installed (source/pip)?
pip in a venv
Include pyinfra-debug.log (if one was created)
Consider including output with -vv and --debug.

Renerick commented 11 months ago

Probably the most apparent demonstration of the issue. I put a breakpoint in the deploy file and printed config from the context and from the state.

Not sure if it's intended, but given that file operations don't works as expected, I assume it's not

Renerick commented 11 months ago

I spend an evening and came up with this patch

diff --git a/pyinfra/api/state.py b/pyinfra/api/state.py
index e639d645..44413046 100644
--- a/pyinfra/api/state.py
+++ b/pyinfra/api/state.py
@@ -7,7 +7,7 @@ from uuid import uuid4
 from gevent.pool import Pool
 from paramiko import PKey

-from pyinfra import logger
+from pyinfra import logger, context

 from .config import Config
 from .exceptions import PyinfraError
@@ -365,4 +365,8 @@ class State:
         if hash_filename:
             hash_key = sha1_hash(hash_key)

-        return "{0}/pyinfra-{1}".format(self.config.TEMP_DIR, hash_key)
+        config = self.config
+        if context.ctx_config.isset():
+            config = context.config
+
+        return "{0}/pyinfra-{1}".format(config.TEMP_DIR, hash_key)

It seems to works, and judging by the call sites of get_temp_filename, I think there should be no concurrency issues. Is this a reasonable fix? If yes, I'm happy to submit a PR

Fizzadar commented 8 months ago

Hrm, sorry this one has come back again @Renerick, this is very annoying! The idea behind the config copies is to keep a separate version by host to scope any changes that way. But I think this is meaning changes get lost.

But I’m still confused by the original example which should work. Will have a think about this, since it’s come up a few times before I want to put something in that provides guarantees on these things rather than patching it (again).

Fizzadar commented 5 months ago

Going to fix this properly in v3 by making config a global instance again. It shouldn't be per-host and it leads to all sorts of confusion like this.

Renerick commented 4 months ago

Just an updated, I've recently updated to 3.0b0, and it seems to be working as expected, but I'll keep an eye on it

pyinfra-dev / pyinfra