diff --git a/changelog/68248.fixed.md b/changelog/68248.fixed.md new file mode 100644 index 000000000000..e98d492f1964 --- /dev/null +++ b/changelog/68248.fixed.md @@ -0,0 +1 @@ +Fixed deltaproxy sub-proxies returning identical grain data for every controlled minion. ``subproxy_post_master_init`` now re-packs each sub-proxy's freshly loaded per-minion grains into its execution-module, returner, executor and proxy LazyLoaders so ``__grains__`` inside loaded modules reflects that sub-proxy's device instead of the placeholder values captured during the first-pass grains load through the control proxy. diff --git a/salt/metaproxy/deltaproxy.py b/salt/metaproxy/deltaproxy.py index f77ccdcbd344..41ce58c576e1 100644 --- a/salt/metaproxy/deltaproxy.py +++ b/salt/metaproxy/deltaproxy.py @@ -538,6 +538,17 @@ def subproxy_post_master_init(minion_id, uid, opts, main_proxy, main_utils): ) proxyopts["grains"] = proxy_grains + # The execution-module loader was packed earlier with the first-pass + # grains (computed through the parent control proxy's LazyLoader, before + # this sub-proxy's own proxymodule was initialised). Re-pack the fresh + # per-sub-proxy grains so ``__grains__`` inside every loaded module + # reflects this sub-proxy's device, not the placeholder values shared + # with its siblings. See issue #68248. + _proxy_minion.functions.pack["__grains__"] = proxy_grains + _proxy_minion.returners.pack["__grains__"] = proxy_grains + _proxy_minion.executors.pack["__grains__"] = proxy_grains + _proxy_minion.proxy.pack["__grains__"] = proxy_grains + if not hasattr(_proxy_minion, "schedule"): _proxy_minion.schedule = salt.utils.schedule.Schedule( proxyopts, diff --git a/tests/pytests/functional/metaproxy/__init__.py b/tests/pytests/functional/metaproxy/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/pytests/functional/metaproxy/conftest.py b/tests/pytests/functional/metaproxy/conftest.py new file mode 100644 index 000000000000..7d31038dade6 --- /dev/null +++ b/tests/pytests/functional/metaproxy/conftest.py @@ -0,0 +1,80 @@ +""" +Functional test fixtures for ``salt.metaproxy.deltaproxy``. + +These fixtures lay down a real ``extension_modules`` tree containing a +purpose-built proxy module that returns *id-distinguishing* grains, so the +test can drive ``subproxy_post_master_init`` end-to-end through the real +``salt.loader.proxy`` / ``salt.loader.grains`` loaders. +""" + +import textwrap + +import pytest + + +@pytest.fixture +def extension_modules(tmp_path): + """ + Create a tmp extension_modules tree with a custom proxy module whose + ``grains()`` callable returns the sub-proxy's ``id`` as a grain. + + The deltaproxy code path that #68248 fixes runs ``salt.loader.grains`` + twice for every sub-proxy: once via the parent control proxy + (placeholder grains shared across siblings) and again via the sub-proxy's + own proxymodule. ``proxy_merge_grains_in_module=True`` (the default) then + merges the proxymodule's ``grains()`` output into the grains dict, which + is the bit that *must* differ per sub-proxy after the fix. + """ + ext = tmp_path / "extension_modules" + proxy_dir = ext / "proxy" + proxy_dir.mkdir(parents=True) + (ext / "__init__.py").write_text("") + + # The custom proxy module: returns ``serial_number = `` so each + # sub-proxy ends up with a grain that uniquely identifies its device. + proxy_module = textwrap.dedent( + ''' + """ + Test-only proxy module used by tests/pytests/functional/metaproxy. + + Returns a ``serial_number`` grain whose value is the sub-proxy id + currently driving this proxymodule instance. This makes the per + sub-proxy ``__grains__`` distinguishable so the deltaproxy fix + for issue #68248 can be observed end-to-end. + """ + + __proxyenabled__ = ["serial_test_proxy"] + + + def __virtual__(): + return True + + + def init(opts): + return True + + + def initialized(): + return True + + + def grains(): + # ``__opts__`` is injected by the loader; ``id`` is the + # sub-proxy id for this proxymodule instance. + return {"serial_number": __opts__["id"]} + + + def grains_refresh(): + return grains() + + + def shutdown(opts): + return True + + + def ping(): + return True + ''' + ).lstrip() + (proxy_dir / "serial_test_proxy.py").write_text(proxy_module) + return ext diff --git a/tests/pytests/functional/metaproxy/test_deltaproxy.py b/tests/pytests/functional/metaproxy/test_deltaproxy.py new file mode 100644 index 000000000000..7e06b0aef6cf --- /dev/null +++ b/tests/pytests/functional/metaproxy/test_deltaproxy.py @@ -0,0 +1,200 @@ +""" +Functional regression test for issue #68248. + +``salt.metaproxy.deltaproxy.subproxy_post_master_init`` computes per +sub-proxy grains twice: first through the parent (control) proxy's loader +- which only produces placeholder values shared across all sub-proxies - +and then again, after the sub-proxy's own ``proxymodule`` ``init()`` has +been called, through that sub-proxy's loader. + +The unit test for this is mock-heavy. This functional test stands up real +``salt.loader.proxy`` / ``salt.loader.grains`` loaders against an on-disk +``extension_modules`` tree containing a purpose-built proxy module that +returns ``serial_number = ``. We then drive +``subproxy_post_master_init`` for two distinct sub-proxy ids and assert +that the ``__grains__`` packed into each sub-proxy's execution-module +loader reflects *that* sub-proxy's device - not the placeholder shared +with its siblings. + +Without the fix (the four ``pack["__grains__"] = proxy_grains`` +assignments in ``subproxy_post_master_init``) both sub-proxies see the +same first-pass grains and ``grains.item serial_number`` would return +identical values for every controlled minion. +""" + +import logging + +import pytest + +import salt.metaproxy.deltaproxy as deltaproxy +import salt.modules.saltutil +from tests.support.mock import MagicMock, patch + +log = logging.getLogger(__name__) + + +@pytest.fixture +def sub_proxy_opts(minion_opts, extension_modules, tmp_path): + """ + Build a sub-proxy ``opts`` dict suitable for ``subproxy_post_master_init``. + + ``conf_file`` points at a file we never read from disk (we patch + ``salt.config.proxy_config`` to a passthrough), ``extension_modules`` + points at the tree with our test-only proxymodule, and ``file_client`` + is ``local`` so the real ``saltutil.sync_all`` invocation inside the + function under test does not try to talk to a master. + """ + opts = dict(minion_opts) + conf_file = tmp_path / "proxy.conf" + conf_file.write_text("") + opts.update( + { + "id": "control_proxy", + "conf_file": str(conf_file), + "cachedir": str(tmp_path / "cache"), + "extension_modules": str(extension_modules), + "saltenv": "base", + "pillarenv": None, + "user": None, + "file_client": "local", + "proxy": { + "proxytype": "deltaproxy", + "ids": ["device-aaa-001", "device-bbb-002"], + }, + "proxy_keep_alive": False, + "proxy_merge_grains_in_module": True, + "subproxy": False, + } + ) + (tmp_path / "cache").mkdir(exist_ok=True) + return opts + + +@pytest.fixture +def patched_subproxy_post_master_init(sub_proxy_opts): + """ + Patch the few side-effecting pieces of ``subproxy_post_master_init`` + that are *not* part of the fix-under-test, leaving the loader stack + real. + + - ``salt.config.proxy_config`` -> passthrough (no disk read) + - ``salt.pillar.get_pillar`` -> returns ``proxy.proxytype`` = + ``serial_test_proxy`` so the + sub-proxy uses our test module + - ``salt.utils.extmods.sync`` -> no-op so ``saltutil.sync_all`` + does not hit the (absent) master + - ``salt.utils.schedule.Schedule`` -> mock; the scheduler has heavy + threading state we don't exercise + - ``salt.minion.get_proc_dir`` -> tmp dir + """ + + def _passthrough_proxy_config(conf_file, defaults, minion_id): + return defaults + + def _fake_pillar(opts, grains, minion_id, **kwargs): + compiler = MagicMock() + compiler.compile_pillar.return_value = { + "proxy": {"proxytype": "serial_test_proxy"} + } + return compiler + + extmods_sync = MagicMock(return_value=({}, False)) + refresh_modules = MagicMock(return_value=True) + schedule_mock = MagicMock() + get_proc_dir_mock = MagicMock(return_value=sub_proxy_opts["cachedir"]) + + with patch.object( + deltaproxy.salt.config, "proxy_config", side_effect=_passthrough_proxy_config + ), patch.object( + deltaproxy.salt.pillar, "get_pillar", side_effect=_fake_pillar + ), patch( + "salt.utils.extmods.sync", extmods_sync + ), patch.object( + salt.modules.saltutil, "refresh_modules", refresh_modules + ), patch.object( + deltaproxy.salt.minion, "get_proc_dir", get_proc_dir_mock + ), patch.object( + deltaproxy.salt.utils.schedule, "Schedule", schedule_mock + ): + yield + + +def _run_subproxy_post_master_init(minion_id, sub_proxy_opts): + """ + Drive ``subproxy_post_master_init`` for a single sub-proxy id and + return the result dict. + + ``main_proxy`` and ``main_utils`` are loaders from the control proxy's + perspective. For this test they only need to *exist* so the first-pass + ``salt.loader.grains(..., proxy=main_proxy, ...)`` call has something + to dereference; they intentionally don't contain a ``serial_test_proxy + .grains`` callable, which is what forces the (post-init) second-pass + grains to be the per-sub-proxy values. + """ + import salt.loader + + main_proxy = salt.loader.proxy(sub_proxy_opts) + main_utils = salt.loader.utils(sub_proxy_opts) + return deltaproxy.subproxy_post_master_init( + minion_id, 0, sub_proxy_opts, main_proxy, main_utils + ) + + +@pytest.mark.slow_test +def test_subproxy_post_master_init_grains_are_per_device( + sub_proxy_opts, patched_subproxy_post_master_init +): + """ + Regression test for #68248. + + After ``subproxy_post_master_init`` returns for two distinct sub-proxy + ids, each sub-proxy's execution-module loader must expose a + ``__grains__`` dict whose ``serial_number`` reflects that sub-proxy's + own device (the value the proxymodule's ``grains()`` produced under + that sub-proxy's id) - not the placeholder shared with its siblings. + + Before the fix, both sub-proxies' loaders pointed at the same + first-pass grains dict and ``grains.item serial_number`` returned the + same value for every controlled minion. + """ + result_a = _run_subproxy_post_master_init("device-aaa-001", sub_proxy_opts) + result_b = _run_subproxy_post_master_init("device-bbb-002", sub_proxy_opts) + + sub_a = result_a["proxy_minion"] + sub_b = result_b["proxy_minion"] + assert sub_a is not None, "device-aaa-001 sub-proxy was not initialised" + assert sub_b is not None, "device-bbb-002 sub-proxy was not initialised" + + # The per-sub-proxy grains dict computed *after* proxy_init must be + # the one packed into the sub-proxy's execution-module loader. This + # is the dict that backs ``__grains__`` inside every loaded module. + grains_a = sub_a.functions.pack["__grains__"] + grains_b = sub_b.functions.pack["__grains__"] + assert grains_a["serial_number"] == "device-aaa-001" + assert grains_b["serial_number"] == "device-bbb-002" + + # The same dict must be packed into every loader the deltaproxy fix + # touches (functions / returners / executors / proxy) so any module + # that reads ``__grains__`` from any of those scopes sees the right + # device. + for loader in (sub_a.functions, sub_a.returners, sub_a.executors, sub_a.proxy): + assert loader.pack["__grains__"]["serial_number"] == "device-aaa-001" + for loader in (sub_b.functions, sub_b.returners, sub_b.executors, sub_b.proxy): + assert loader.pack["__grains__"]["serial_number"] == "device-bbb-002" + + # And the ``proxy_opts["grains"]`` returned to the caller (which the + # control proxy stores in ``self.deltaproxy_opts``) must match so the + # control side also has the right grains. + assert result_a["proxy_opts"]["grains"]["serial_number"] == "device-aaa-001" + assert result_b["proxy_opts"]["grains"]["serial_number"] == "device-bbb-002" + + # End-to-end view through the execution-module loader: calling + # ``grains.items()`` walks the loader's ``__grains__`` pack via the + # ``NamedLoaderContext`` wrapper. With the fix this returns the per + # sub-proxy device grains; without the fix both sub-proxies return + # the same shared first-pass dict. + items_a = sub_a.functions["grains.items"]() + items_b = sub_b.functions["grains.items"]() + assert items_a["serial_number"] == "device-aaa-001" + assert items_b["serial_number"] == "device-bbb-002" + assert items_a["serial_number"] != items_b["serial_number"] diff --git a/tests/pytests/unit/metaproxy/__init__.py b/tests/pytests/unit/metaproxy/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/pytests/unit/metaproxy/test_deltaproxy.py b/tests/pytests/unit/metaproxy/test_deltaproxy.py new file mode 100644 index 000000000000..96efc80fda9b --- /dev/null +++ b/tests/pytests/unit/metaproxy/test_deltaproxy.py @@ -0,0 +1,213 @@ +""" +Unit tests for ``salt.metaproxy.deltaproxy.subproxy_post_master_init``. + +These tests pin down the data flow inside ``subproxy_post_master_init`` so +each sub-proxy ends up with grains/pillar values that reflect *that* +sub-proxy's own proxymodule, not whatever was loaded the first time through +the parent (control) proxy's loader. +""" + +import logging + +import pytest + +import salt.metaproxy.deltaproxy as deltaproxy +from tests.support.mock import MagicMock, patch + +log = logging.getLogger(__name__) + + +class _FakeLoader(dict): + """ + Mimic ``salt.loader.LazyLoader``'s ``.pack`` attribute, dict-style + function access and ``.reload_modules`` for the deltaproxy code path. + """ + + def __init__(self, items=None, pack=None): + super().__init__(items or {}) + self.pack = pack if pack is not None else {} + self.utils = None + + def reload_modules(self): + # Real LazyLoaders re-read self.pack into the underlying module + # namespaces. The test only cares that the pack dict is what the + # code finally stamped, so a no-op is fine. + pass + + +@pytest.fixture +def fake_main_proxy(): + return _FakeLoader() + + +@pytest.fixture +def fake_main_utils(): + return _FakeLoader() + + +@pytest.fixture +def proxy_opts(tmp_path): + return { + "id": "control_proxy", + "conf_file": str(tmp_path / "proxy"), + "cachedir": str(tmp_path / "cache"), + "saltenv": "base", + "pillarenv": None, + "extension_modules": str(tmp_path / "ext"), + "proxy": {"proxytype": "deltaproxy", "ids": ["minion1", "minion2"]}, + "user": None, + "proxy_keep_alive": False, + } + + +def _make_subproxy_patches(per_minion_grains): + """ + Build the patches needed to call ``subproxy_post_master_init`` without + touching disk or spinning up real loaders. ``per_minion_grains`` maps + sub-proxy id -> the grains dict that the *second* ``salt.loader.grains`` + call (the post-init refresh) should return for that id. + """ + + proxy_config_mock = MagicMock( + side_effect=lambda conf_file, defaults, minion_id: defaults + ) + + # The pillar load for each sub-proxy gives that minion a proxy config so + # the function does not bail out at the "no proxy in pillar" guard. + def _fake_pillar(opts, grains, minion_id, **kwargs): + compiler = MagicMock() + compiler.compile_pillar.return_value = { + "proxy": {"proxytype": "dummy_test_proxy"} + } + return compiler + + get_pillar_mock = MagicMock(side_effect=_fake_pillar) + + # The grains loader is called twice per sub-proxy: first with the parent + # control proxy (returns the placeholder), then again post-init with the + # sub-proxy's own proxymodule (returns the per-id distinguishing dict). + placeholder_grains = {"placeholder": True} + call_state = {"count": 0} + + def _fake_grains(opts, proxy=None, context=None, **kwargs): + call_state["count"] += 1 + # Return a fresh dict each call so identity comparisons stay honest. + if call_state["count"] % 2 == 1: + return dict(placeholder_grains) + return dict(per_minion_grains[opts["id"]]) + + grains_mock = MagicMock(side_effect=_fake_grains) + + # The proxy/utils loaders just need to look like LazyLoaders that already + # contain the per-proxy ``init``/``alive`` callables the code touches. + def _fake_proxy_loader(opts, utils=None, context=None, **kwargs): + proxytype = opts["proxy"]["proxytype"] + return _FakeLoader( + items={ + f"{proxytype}.init": MagicMock(return_value=True), + f"{proxytype}.shutdown": MagicMock(return_value=True), + } + ) + + proxy_loader_mock = MagicMock(side_effect=_fake_proxy_loader) + utils_loader_mock = MagicMock(side_effect=lambda *a, **kw: _FakeLoader()) + + # The ProxyMinion is replaced with a lightweight stand-in so we do not + # touch the real network/event-loop machinery. We still need + # ``_load_modules`` to feed real-looking LazyLoaders (with ``.pack``) + # into the function under test. + def _fake_load_modules(self, opts=None, grains=None, context=None, **kwargs): + functions = _FakeLoader( + items={"saltutil.sync_all": MagicMock(return_value=[])}, + pack={"__grains__": grains, "__opts__": opts}, + ) + returners = _FakeLoader(pack={"__grains__": grains, "__opts__": opts}) + executors = _FakeLoader(pack={"__grains__": grains, "__opts__": opts}) + return functions, returners, {}, executors + + class _FakeProxyMinion: + def __init__(self, opts): + self.opts = opts + self.subprocess_list = MagicMock() + self.connected = False + + _load_modules = _fake_load_modules + + fake_proxy_minion_cls = _FakeProxyMinion + get_proc_dir_mock = MagicMock(return_value="/tmp/proc") + schedule_mock = MagicMock() + + return { + "proxy_config": proxy_config_mock, + "get_pillar": get_pillar_mock, + "grains": grains_mock, + "proxy_loader": proxy_loader_mock, + "utils_loader": utils_loader_mock, + "proxy_minion_cls": fake_proxy_minion_cls, + "get_proc_dir": get_proc_dir_mock, + "schedule": schedule_mock, + } + + +def test_subproxy_post_master_init_packs_per_minion_grains( + proxy_opts, fake_main_proxy, fake_main_utils +): + """ + Regression test for #68248. + + Each sub-proxy must end up with grains in its execution-module loader + that reflect the values produced by *its own* proxymodule, not the + placeholder values the control proxy returned on the first pass. + """ + per_minion_grains = { + "minion1": {"serial_number": "SN-AAA-001", "id": "minion1"}, + "minion2": {"serial_number": "SN-BBB-002", "id": "minion2"}, + } + p = _make_subproxy_patches(per_minion_grains) + + with patch.object( + deltaproxy.salt.config, "proxy_config", p["proxy_config"] + ), patch.object( + deltaproxy.salt.pillar, "get_pillar", p["get_pillar"] + ), patch.object( + deltaproxy.salt.loader, "grains", p["grains"] + ), patch.object( + deltaproxy.salt.loader, "proxy", p["proxy_loader"] + ), patch.object( + deltaproxy.salt.loader, "utils", p["utils_loader"] + ), patch.object( + deltaproxy, "ProxyMinion", p["proxy_minion_cls"] + ), patch.object( + deltaproxy.salt.minion, "get_proc_dir", p["get_proc_dir"] + ), patch.object( + deltaproxy.salt.utils.schedule, "Schedule", p["schedule"] + ): + result1 = deltaproxy.subproxy_post_master_init( + "minion1", 0, proxy_opts, fake_main_proxy, fake_main_utils + ) + result2 = deltaproxy.subproxy_post_master_init( + "minion2", 0, proxy_opts, fake_main_proxy, fake_main_utils + ) + + sub1 = result1["proxy_minion"] + sub2 = result2["proxy_minion"] + assert sub1 is not None + assert sub2 is not None + + # The per-sub-proxy grains dict computed *after* init must be what the + # execution-module loader exposes to modules via ``__grains__``. Without + # the fix, both sub-proxies see the placeholder grains from the first + # (parent-proxy) pass. + assert sub1.functions.pack["__grains__"]["serial_number"] == "SN-AAA-001" + assert sub2.functions.pack["__grains__"]["serial_number"] == "SN-BBB-002" + + # And the proxy loader's pack must agree, otherwise grain modules that + # consult ``__grains__`` from within the proxy module will also see + # stale values. + assert sub1.proxy.pack["__grains__"]["serial_number"] == "SN-AAA-001" + assert sub2.proxy.pack["__grains__"]["serial_number"] == "SN-BBB-002" + + # ``proxyopts["grains"]`` returned to the caller must match too so the + # control proxy stores the right grains in ``self.deltaproxy_opts``. + assert result1["proxy_opts"]["grains"]["serial_number"] == "SN-AAA-001" + assert result2["proxy_opts"]["grains"]["serial_number"] == "SN-BBB-002"