Skip to content

Commit fac401f

Browse files
committed
Fix __pub_resource_* key leakage into state functions and multifunction job targeting
- salt/state.py: add __pub_resource_targets, __pub_minion_is_target, __pub_resource_target, and __pub_resource_job to STATE_REQUISITE_KEYWORDS so they are treated as internal pub metadata and stripped before being passed to state module functions (e.g. git.present, ssh_known_hosts.present) - salt/utils/minions.py: guard the _MERGE_RESOURCE_FUNS membership check with isinstance(fun, str) — multifunction jobs pass fun as a list which is not hashable, causing a TypeError swallowed by the broad except that returned an empty minion list and "No minions matched the target" - tests: add test_check_minions_list_fun_still_augments covering the list fun case, and CI-TRACKING.md to record known failures and regression tests
1 parent e1209b8 commit fac401f

4 files changed

Lines changed: 69 additions & 1 deletion

File tree

salt/state.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,10 @@
120120
"__pub_ret",
121121
"__pub_pid",
122122
"__pub_tgt_type",
123+
"__pub_resource_targets",
124+
"__pub_minion_is_target",
125+
"__pub_resource_target",
126+
"__pub_resource_job",
123127
"__prereq__",
124128
"__prerequiring__",
125129
"__umask__",

salt/utils/minions.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1001,7 +1001,7 @@ def check_minions(
10011001
tgt_type == "glob"
10021002
and isinstance(expr, str)
10031003
and any(c in expr for c in ("*", "?", "["))
1004-
and fun not in _MERGE_RESOURCE_FUNS
1004+
and not (isinstance(fun, str) and fun in _MERGE_RESOURCE_FUNS)
10051005
):
10061006
_res["minions"] = self._augment_with_resources(_res["minions"])
10071007
_res["ssh_minions"] = False

tests/pytests/unit/CI-TRACKING.md

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
# CI Test Tracking
2+
3+
Tests that must pass before merging. Run with:
4+
5+
```
6+
source venv311/bin/activate
7+
python -m pytest <test> -v
8+
```
9+
10+
## Tests We Own (new files)
11+
12+
| Test file | What it covers |
13+
|---|---|
14+
| `tests/pytests/unit/utils/test_minions_resources.py` | Resource index, `check_minions` augmentation + merge-mode skip |
15+
| `tests/pytests/unit/test_minion_resources.py` | `_prefix_resource_state_key`, `_MERGE_RESOURCE_FUNS`, merge block branches |
16+
| `tests/pytests/unit/modules/test_sshresource_state.py` | `highstate()` empty-chunks return, `_exec_state_pkg` exception recovery |
17+
| `tests/pytests/unit/matchers/test_resource_matchers.py` | `T@` / `M@` compound matching |
18+
19+
## Existing Tests We've Broken (fixed)
20+
21+
| Test | Root cause | Fix |
22+
|---|---|---|
23+
| `tests/pytests/unit/states/test_saltutil.py::test_saltutil_sync_states_should_match_saltutil_module` | `sync_resources` added to module but not state | Added `sync_resources()` to `salt/states/saltutil.py` |
24+
| `tests/pytests/unit/client/test_netapi.py::test_run_log` | `netapi.py` changed `run()` to `asyncio.run()`, test used plain `Mock` | Changed mock to `AsyncMock` |
25+
| `tests/integration/minion/test_executor.py::ExecutorTest::test_executor_with_multijob` | Multifunction job passes `fun` as a list; `list not in frozenset` raises `TypeError`, caught by broad except → empty minion list | Guard with `isinstance(fun, str)` before set membership check |
26+
27+
## Known CI Noise (not our fault)
28+
29+
| Failure | Reason |
30+
|---|---|
31+
| Package upgrade tests (`test_salt_systemd_*_preservation`) | 502 downloading `salt-install-guide` release from GitHub — transient infra |
32+
| `test_tcp_client_invalid_cert_rejected` | Functional TCP SSL timeout — pre-existing flaky test |
33+
| `test_state.py::test_event` | `RuntimeError: There is no current event loop` — pre-existing flaky |
34+
35+
## Quick Regression Run
36+
37+
```bash
38+
source venv311/bin/activate
39+
python -m pytest \
40+
tests/pytests/unit/utils/test_minions_resources.py \
41+
tests/pytests/unit/test_minion_resources.py \
42+
tests/pytests/unit/modules/test_sshresource_state.py \
43+
tests/pytests/unit/matchers/test_resource_matchers.py \
44+
tests/pytests/unit/states/test_saltutil.py::test_saltutil_sync_states_should_match_saltutil_module \
45+
tests/pytests/unit/client/test_netapi.py::test_run_log \
46+
-v
47+
```

tests/pytests/unit/utils/test_minions_resources.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,23 @@ def test_check_minions_merge_fun_all_merge_funs_skip(ck_with_resources):
378378
assert "dummy-01" not in result["minions"], f"augmented for {fun}"
379379

380380

381+
def test_check_minions_list_fun_still_augments(ck_with_resources):
382+
"""
383+
A multifunction job passes fun as a list. A list is not hashable and must
384+
not cause a TypeError — augmentation must proceed normally.
385+
"""
386+
with patch.object(
387+
ck_with_resources,
388+
"_check_glob_minions",
389+
return_value={"minions": ["minion"], "missing": []},
390+
):
391+
result = ck_with_resources.check_minions(
392+
"*", tgt_type="glob", fun=["test.arg", "test.arg"]
393+
)
394+
assert "dummy-01" in result["minions"]
395+
assert "minion" in result["minions"]
396+
397+
381398
def test_check_minions_non_merge_fun_still_augments(ck_with_resources):
382399
"""
383400
A non-merge function such as test.ping with a wildcard glob must still

0 commit comments

Comments
 (0)