Skip to content

Commit 7fe551e

Browse files
committed
Fix __resource__ injection in resource_modules loader and add tests
salt.loader.resource_modules was missing "__resource__" from its pack, so LazyLoader never created a NamedLoaderContext for it and modules like sshresource_state raised NameError when accessing __resource__["id"]. Added the sentinel entry so the loader wires __resource__ to resource_ctxvar — the per-thread contextvar set by _thread_return before each resource job executes. Added a regression test (test_resource_modules_packs_resource_dunder) and a full suite of unit and integration tests covering T@ targeting, wildcard augmentation, resource_ctxvar thread isolation, and the _resolve_resource_targets dispatch path. Fixed lint issues across the new test files (blacklisted unittest.mock imports, f-string without interpolation, unused import, broad exception catch).
1 parent 816d8e0 commit 7fe551e

File tree

18 files changed

+1101
-88
lines changed

18 files changed

+1101
-88
lines changed

docker/ssh-resource/Dockerfile

Lines changed: 0 additions & 43 deletions
This file was deleted.

salt/client/ssh/__init__.py

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,18 @@
227227
tar --strip-components=1 -xf "$RELENV_TAR" -C "{THIN_DIR}"
228228
fi
229229
230+
# BUG-WORKAROUND: salt-ssh relenv path never writes the minion config that
231+
# Single.__init__ builds in self.minion_config. The non-relenv (salt-thin)
232+
# path embeds it in SSH_PY_SHIM via OPTIONS.config, which the Python shim
233+
# writes to thin_dir/minion. The relenv shim has no equivalent, so salt-call
234+
# falls back to system defaults (/var/cache/salt, /var/log/salt) and fails for
235+
# any unprivileged user. Writing it here replicates the salt-thin behaviour.
236+
# See: https://github.com/saltstack/salt (file as issue against salt-ssh relenv)
237+
mkdir -p "{THIN_DIR}/running_data/pki"
238+
cat > "{THIN_DIR}/minion" << 'SALT_MINION_CONF_EOF'
239+
__SALT_MINION_CONFIG__
240+
SALT_MINION_CONF_EOF
241+
230242
# Check if Python binary is executable
231243
if [ ! -x "$SALT_CALL_BIN" ]; then
232244
echo "ERROR: salt-call binary not found or not executable at $SALT_CALL_BIN" >&2
@@ -1148,10 +1160,18 @@ def __init__(
11481160
self.arch = arch.strip()
11491161

11501162
if self.opts.get("relenv"):
1151-
kernel, os_arch = self.detect_os_arch()
1152-
self.thin = salt.utils.relenv.gen_relenv(
1153-
opts["cachedir"], kernel=kernel, os_arch=os_arch
1154-
)
1163+
if thin:
1164+
# Caller pre-resolved the relenv tarball path — skip the SSH
1165+
# round-trip that detect_os_arch() would otherwise make during
1166+
# __init__. This is important when Single is created inside a
1167+
# minion job worker where every extra SSH connection adds latency
1168+
# and can cause hangs.
1169+
self.thin = thin
1170+
else:
1171+
kernel, os_arch = self.detect_os_arch()
1172+
self.thin = salt.utils.relenv.gen_relenv(
1173+
opts["cachedir"], kernel=kernel, os_arch=os_arch
1174+
)
11551175
else:
11561176
self.thin = thin if thin else salt.utils.thin.thin_path(opts["cachedir"])
11571177

@@ -1597,7 +1617,11 @@ def _cmd_str(self):
15971617
debug = "1"
15981618

15991619
if self.opts.get("relenv"):
1600-
return SSH_SH_SHIM_RELENV.format(
1620+
# Use .replace() for minion_config — it is YAML flow-style and
1621+
# may contain literal { } which would break .format(). This
1622+
# mirrors how SSH_PY_SHIM injects OPTIONS.config on the non-relenv
1623+
# path (via SSH_PY_SHIM.replace("#%%OPTS", arg_str)).
1624+
shim = SSH_SH_SHIM_RELENV.format(
16011625
DEBUG=debug,
16021626
SUDO=sudo,
16031627
SUDO_USER=sudo_user or "",
@@ -1606,6 +1630,7 @@ def _cmd_str(self):
16061630
RSTR=RSTR,
16071631
ARGS=" ".join(self.argv),
16081632
)
1633+
return shim.replace("__SALT_MINION_CONFIG__", self.minion_config)
16091634

16101635
thin_code_digest, thin_sum = salt.utils.thin.thin_sum(cachedir, "sha1")
16111636
arg_str = '''

salt/loader/__init__.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,11 @@ def resource_modules(
570570
"__utils__": utils,
571571
"__resource_funcs__": resource_funcs,
572572
"__opts__": resource_opts,
573+
# Empty sentinel so LazyLoader creates a NamedLoaderContext for
574+
# __resource__ on every loaded module. The NamedLoaderContext
575+
# reads from resource_ctxvar, which _thread_return sets per-call
576+
# before dispatching — giving each resource job its own identity.
577+
"__resource__": {},
573578
},
574579
extra_module_dirs=utils.module_dirs if utils else None,
575580
loaded_base_name=loaded_base_name,

salt/loader/context.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,14 @@
1919

2020
loader_ctxvar = contextvars.ContextVar(DEFAULT_CTX_VAR)
2121

22+
# Per-call resource context. Set via resource_ctxvar.set() in
23+
# _thread_return before executing the job. contextvars are per-thread: each
24+
# new thread inherits a copy of the parent's context, and set() only mutates
25+
# the current thread's copy. LazyLoader.run() calls copy_context() fresh on
26+
# every invocation, so the snapshot it passes to _last_context.run() already
27+
# contains the value we set here — completely isolated from other threads.
28+
resource_ctxvar = contextvars.ContextVar("__resource__", default={})
29+
2230

2331
@contextlib.contextmanager
2432
def loader_context(loader):
@@ -68,6 +76,13 @@ def value(self):
6876
"""
6977
The value of the current for this context
7078
"""
79+
# __resource__ is served from resource_ctxvar, which is set
80+
# per-thread in _thread_return before the job function executes.
81+
# LazyLoader.run() snapshots the thread context via copy_context()
82+
# on every call, so each _run_as invocation sees the value that was
83+
# current when the function was invoked — no pack mutation needed.
84+
if self.name == "__resource__":
85+
return resource_ctxvar.get()
7186
loader = self.loader()
7287
if loader is None:
7388
return self.default

salt/matchers/managing_minion_match.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,10 @@ def match(tgt, opts=None, minion_id=None):
3232
:param str minion_id: The minion ID to evaluate; defaults to ``opts["id"]``.
3333
:rtype: bool
3434
"""
35+
if opts is None:
36+
opts = __opts__ # pylint: disable=undefined-variable
37+
if minion_id is None:
38+
minion_id = opts.get("id", "")
39+
result = minion_id == tgt
40+
log.debug("managing_minion_match: M@%s => %s (id=%s)", tgt, result, minion_id)
41+
return result

salt/minion.py

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -483,20 +483,32 @@ def gen_modules(self, initial_load=False, context=None):
483483
context=context,
484484
)
485485
self.resource_funcs.pack["__salt__"] = self.functions
486-
self.resource_loaders = {}
486+
# Build resource_loaders into a local dict before assigning to
487+
# self.resource_loaders. Without this, the previous pattern:
488+
#
489+
# self.resource_loaders = {} ← exposes empty dict
490+
# for ...: self.resource_loaders[t] = …
491+
#
492+
# creates a window where a concurrent thread (multiprocessing: False)
493+
# calling gen_modules() can read resource_loaders.get(type) == None
494+
# and fail with "No resource loader available". A single dict
495+
# assignment is atomic in CPython, so the old loaders remain visible
496+
# until the new complete set is ready.
497+
_new_resource_loaders = {}
487498
for resource_type in self.opts.get("resources", {}):
488499
rtype_base = (
489500
f"{self.opts.get('loaded_base_name', 'salt.loaded.int')}"
490501
f".resource.{resource_type}"
491502
)
492-
self.resource_loaders[resource_type] = salt.loader.resource_modules(
503+
_new_resource_loaders[resource_type] = salt.loader.resource_modules(
493504
self.opts,
494505
resource_type,
495506
resource_funcs=self.resource_funcs,
496507
utils=self.utils,
497508
context=context,
498509
loaded_base_name=rtype_base,
499510
)
511+
self.resource_loaders = _new_resource_loaders
500512

501513
# Call init() on each resource type so that __context__ is populated
502514
# before any per-resource operations (grains, ping, etc.) are dispatched.
@@ -544,12 +556,19 @@ def _discover_resources(self):
544556
``discover(opts)``; the return value is a dict of
545557
``{resource_type: [resource_id, ...]}``.
546558
559+
If pillar does not specify any resources, any resources already
560+
present in ``opts["resources"]`` (e.g. set directly in the minion
561+
config file) are preserved as-is. This supports testing and simple
562+
deployments where pillar-driven discovery is not needed.
563+
547564
Called from :meth:`gen_modules` after pillar is compiled and before
548565
the per-type execution-module loaders are created.
549566
"""
550567
pillar_resources = self.opts.get("pillar", {}).get("resources", {})
551568
if not pillar_resources:
552-
return {}
569+
# No pillar resources — preserve whatever is already in opts
570+
# (set directly in the minion config or from a previous discovery).
571+
return {k: list(v) for k, v in self.opts.get("resources", {}).items()}
553572

554573
# A minimal resource loader is sufficient here — discover() only reads
555574
# from the opts dict passed to it and does not need other dunders.
@@ -2656,13 +2675,15 @@ def _thread_return(cls, minion_instance, opts, data):
26562675
)
26572676
ret["retcode"] = salt.defaults.exitcodes.EX_GENERIC
26582677
else:
2659-
# Inject per-call resource context into the type loader
2660-
# AND into resource_funcs so that resource module
2661-
# functions like dummy.ping() can read __resource__.
2662-
functions_to_use.pack["__resource__"] = resource_target
2663-
minion_instance.resource_funcs.pack["__resource__"] = (
2664-
resource_target
2665-
)
2678+
# Set the per-call resource context via resource_ctxvar.
2679+
# contextvars are per-thread, so this value is invisible
2680+
# to other threads. LazyLoader.run() calls copy_context()
2681+
# fresh on every invocation, capturing this value in the
2682+
# snapshot before _run_as executes — fully isolated from
2683+
# concurrent resource jobs sharing the same loader object.
2684+
import salt.loader.context as _loader_ctx
2685+
2686+
_loader_ctx.resource_ctxvar.set(resource_target)
26662687
grains_fn = f"{resource_type}.grains"
26672688
if grains_fn in minion_instance.resource_funcs:
26682689
functions_to_use.pack["__grains__"] = (

salt/modules/saltutil.py

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -402,23 +402,52 @@ def refresh_grains(**kwargs):
402402

403403
def refresh_resources():
404404
"""
405-
Trigger resource discovery on this minion and report the current set of
406-
managed resources back to the Master, which updates the Resource Registry.
405+
Re-register this minion's managed resources with the master.
407406
408-
Each resource type module's ``discover()`` function is called to enumerate
409-
the resources the minion manages. The results are sent to the Master,
410-
which updates the ``resources`` cache bank for each resource. Resources
411-
no longer present in the discovery result are removed from the Registry.
412-
413-
This is the resource analogue of ``saltutil.refresh_grains`` — it should
414-
be called whenever the set of resources a minion manages may have changed.
407+
Sends the current ``opts["resources"]`` to the master so that the master's
408+
``minion_resources`` cache bank is up-to-date. This is the resource
409+
analogue of ``saltutil.refresh_grains`` — call it whenever the set of
410+
resources a minion manages may have changed, or after a minion restart to
411+
ensure the master's targeting cache reflects the current state.
415412
416413
CLI Example:
417414
418415
.. code-block:: bash
419416
420417
salt '*' saltutil.refresh_resources
421418
"""
419+
resources = __opts__.get("resources", {})
420+
if not resources:
421+
return True
422+
423+
masters = []
424+
if "master_uri_list" in __opts__:
425+
masters.extend(__opts__["master_uri_list"])
426+
else:
427+
masters.append(__opts__["master_uri"])
428+
429+
from salt.exceptions import SaltReqTimeoutError
430+
431+
for master in masters:
432+
with salt.channel.client.ReqChannel.factory(
433+
__opts__, master_uri=master
434+
) as channel:
435+
load = {
436+
"cmd": "_register_resources",
437+
"id": __opts__["id"],
438+
"resources": resources,
439+
"tok": channel.auth.gen_token(b"salt"),
440+
}
441+
try:
442+
channel.send(load, timeout=60)
443+
log.debug(
444+
"refresh_resources: registered %s with master %s",
445+
list(resources),
446+
master,
447+
)
448+
except SaltReqTimeoutError:
449+
log.warning("refresh_resources: timed out contacting master %s", master)
450+
return True
422451

423452

424453
def sync_grains(

salt/modules/sshresource_state.py

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import logging
2626
import os
27+
import uuid
2728

2829
import salt.client.ssh
2930
import salt.client.ssh.shell
@@ -32,6 +33,7 @@
3233
import salt.defaults.exitcodes
3334
import salt.fileclient
3435
import salt.utils.hashutils
36+
import salt.utils.network
3537
import salt.utils.state
3638
from salt.client.ssh.wrapper.state import (
3739
_cleanup_slsmod_low_data,
@@ -72,6 +74,12 @@ def _host_cfg():
7274
) # pylint: disable=undefined-variable
7375

7476

77+
def _relenv_path():
78+
"""Return the local path of our pre-built custom relenv tarball."""
79+
cachedir = __opts__.get("cachedir", "") # pylint: disable=undefined-variable
80+
return os.path.join(cachedir, "relenv", "linux", "x86_64", "salt-relenv.tar.xz")
81+
82+
7583
def _target_opts():
7684
"""
7785
Build a copy of ``__opts__`` suitable for ``SSHHighState`` and ``Single``.
@@ -97,6 +105,7 @@ def _target_opts():
97105
)
98106
if "known_hosts_file" in cfg:
99107
opts["known_hosts_file"] = cfg["known_hosts_file"]
108+
opts["relenv"] = True
100109
return opts
101110

102111

@@ -121,23 +130,31 @@ def _connection_kwargs():
121130
}
122131

123132

124-
def _seed_thin_dir(opts):
133+
def _thin_dir():
125134
"""
126-
Create a throw-away ``Single`` to force ``opts["thin_dir"]`` to be set.
135+
Return the remote working directory for the salt-thin bundle.
127136
128-
``Single.__init__`` computes ``thin_dir`` from the remote user and the
129-
local FQDN and writes it back into opts in-place. We need that value
130-
before building the ``state.pkg`` command string.
137+
Mirrors the logic in ``salt.resource.ssh._thin_dir``: uses the per-host
138+
``thin_dir`` config key when set, otherwise builds a path under ``/tmp/``
139+
(always world-writable) to avoid ``/var/tmp/`` which may be root-only.
131140
"""
132-
resource_id = _resource_id()
133-
dummy = salt.client.ssh.Single(
134-
opts,
135-
["test.ping"],
136-
resource_id,
137-
**_connection_kwargs(),
138-
)
139-
# thin_dir is now written to opts["thin_dir"] as a side-effect
140-
return dummy.thin_dir
141+
cfg = _host_cfg()
142+
if "thin_dir" in cfg:
143+
return cfg["thin_dir"]
144+
fqdn_uuid = uuid.uuid3(uuid.NAMESPACE_DNS, salt.utils.network.get_fqhostname()).hex[
145+
:6
146+
]
147+
return "/tmp/.{}_{}_salt".format(cfg.get("user", "root"), fqdn_uuid)
148+
149+
150+
def _seed_thin_dir(opts):
151+
"""
152+
Compute ``thin_dir`` and write it into *opts* so that ``SSHHighState``
153+
and ``prep_trans_tar`` use a consistent, writable path.
154+
"""
155+
thin = _thin_dir()
156+
opts["thin_dir"] = thin
157+
return thin
141158

142159

143160
def _get_initial_pillar(opts):
@@ -407,6 +424,8 @@ def _exec_state_pkg(opts, trans_tar, test):
407424
opts,
408425
cmd,
409426
_resource_id(),
427+
thin=_relenv_path(),
428+
thin_dir=opts["thin_dir"],
410429
**_connection_kwargs(),
411430
)
412431
single.shell.send(trans_tar, "{}/salt_state.tgz".format(opts["thin_dir"]))

0 commit comments

Comments
 (0)