From b5ad880dd774b2737ed25ef824abc6b157962b54 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Wed, 2 Sep 2026 16:38:57 -0400 Subject: [PATCH] Give deltaproxy sub-proxies a token for the master (#70071) A sub-proxy is constructed directly rather than going through connect_master, which is where an ordinary minion picks up self.tok, so the attribute was simply missing and every read of it raised AttributeError. In practice that is _register_resources_with_master, reached from pillar_refresh, so each saltutil.refresh_pillar and saltutil.refresh_grains failed part way through: the sub-proxy's resources were never registered with the master, and because an empty resource dict is sent deliberately to clear stale entries, stale entries were never cleared either. The pillar refresh itself completed first, so the breakage was easy to miss. post_master_init already hands each sub-proxy a req_channel, so derive the token from that channel's auth the way connect_master does. Both the parallel and serial startup branches were assigning the channel with identical code; fold that into one helper so they cannot drift. Also build the _register_resources_with_master load inside the try that already guards the send. It is best-effort, so a problem assembling the request should be reported like a failure to send it rather than escaping into pillar_refresh and aborting the rest. --- changelog/70071.fixed.md | 1 + salt/metaproxy/deltaproxy.py | 37 ++++++++++--- salt/minion.py | 18 ++++--- .../pytests/unit/metaproxy/test_deltaproxy.py | 52 +++++++++++++++++++ 4 files changed, 93 insertions(+), 15 deletions(-) create mode 100644 changelog/70071.fixed.md diff --git a/changelog/70071.fixed.md b/changelog/70071.fixed.md new file mode 100644 index 000000000000..c8155869f3a4 --- /dev/null +++ b/changelog/70071.fixed.md @@ -0,0 +1 @@ +Gave deltaproxy sub-proxies the token they need to talk to the master. A sub-proxy is constructed directly rather than going through ``connect_master``, where an ordinary minion picks up ``self.tok``, so every read of it raised ``AttributeError`` -- in practice from ``_register_resources_with_master``, which ``pillar_refresh`` calls, so a sub-proxy's resources were never registered with the master and stale registry entries were never cleared. diff --git a/salt/metaproxy/deltaproxy.py b/salt/metaproxy/deltaproxy.py index cb2b8e2c9a09..bbd960e0594b 100644 --- a/salt/metaproxy/deltaproxy.py +++ b/salt/metaproxy/deltaproxy.py @@ -59,6 +59,27 @@ log = logging.getLogger(__name__) +def attach_req_channel(proxy_minion, proxy_opts, io_loop): + """ + Give a sub-proxy its request channel, and the token that goes with it. + + A sub-proxy is constructed directly rather than going through + ``connect_master``, which is where an ordinary minion picks up + ``self.tok``. Without it every read of ``self.tok`` raises + ``AttributeError`` -- in practice from + ``_register_resources_with_master``, which ``pillar_refresh`` calls, so a + sub-proxy's resources never reach the master. Derive the token from the + channel's auth the same way ``connect_master`` does. + """ + proxy_minion.req_channel = salt.channel.client.AsyncReqChannel.factory( + proxy_opts, io_loop=io_loop + ) + auth = getattr(proxy_minion.req_channel, "auth", None) + if auth is not None: + proxy_minion.tok = auth.gen_token(b"salt") + return proxy_minion.req_channel + + async def post_master_init(self, master): """ Function to finish init after a deltaproxy proxy @@ -371,10 +392,10 @@ async def post_master_init(self, master): self.deltaproxy_objs[minion_id] = sub_proxy_data["proxy_minion"] if self.deltaproxy_opts[minion_id] and self.deltaproxy_objs[minion_id]: - self.deltaproxy_objs[minion_id].req_channel = ( - salt.channel.client.AsyncReqChannel.factory( - sub_proxy_data["proxy_opts"], io_loop=self.io_loop - ) + attach_req_channel( + self.deltaproxy_objs[minion_id], + sub_proxy_data["proxy_opts"], + self.io_loop, ) else: log.debug("Initiating non-parallel startup for proxies") @@ -398,10 +419,10 @@ async def post_master_init(self, master): self.deltaproxy_objs[minion_id] = sub_proxy_data["proxy_minion"] if self.deltaproxy_opts[minion_id] and self.deltaproxy_objs[minion_id]: - self.deltaproxy_objs[minion_id].req_channel = ( - salt.channel.client.AsyncReqChannel.factory( - sub_proxy_data["proxy_opts"], io_loop=self.io_loop - ) + attach_req_channel( + self.deltaproxy_objs[minion_id], + sub_proxy_data["proxy_opts"], + self.io_loop, ) if _failed: diff --git a/salt/minion.py b/salt/minion.py index 8292806508a9..4406b7372b84 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -4142,14 +4142,18 @@ async def _register_resources_with_master(self): # Cache locally so :meth:`_resolve_resource_targets` can resolve # ``tgt_type == "grain"`` without re-rendering. self._resource_grains_cache = resource_grains - load = { - "cmd": "_register_resources", - "id": self.opts["id"], - "resources": resources, - "resource_grains": resource_grains, - "tok": self.tok, - } try: + # Build the load inside the guard as well: this method is + # best-effort, so a problem assembling the request should be + # reported the same way a failure to send it is, rather than + # escaping into pillar_refresh and aborting the rest of it. + load = { + "cmd": "_register_resources", + "id": self.opts["id"], + "resources": resources, + "resource_grains": resource_grains, + "tok": self.tok, + } await self._send_req_async_main(load, timeout=self._return_retry_timer()) log.debug("Registered resources with master: %s", list(resources.keys())) except Exception as err: # pylint: disable=broad-except diff --git a/tests/pytests/unit/metaproxy/test_deltaproxy.py b/tests/pytests/unit/metaproxy/test_deltaproxy.py index ded184c23daf..f67f31974488 100644 --- a/tests/pytests/unit/metaproxy/test_deltaproxy.py +++ b/tests/pytests/unit/metaproxy/test_deltaproxy.py @@ -229,3 +229,55 @@ def test_subproxy_post_master_init_packs_per_minion_grains( # 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" + + +def test_attach_req_channel_gives_the_subproxy_a_token(): + """ + Regression test for #70071. + + A sub-proxy is constructed directly rather than going through + ``connect_master``, which is where an ordinary minion picks up + ``self.tok``, so the attribute was simply missing. Every read of it + raised ``AttributeError`` -- in practice from + ``_register_resources_with_master``, which ``pillar_refresh`` calls, so + each ``saltutil.refresh_pillar`` and ``saltutil.refresh_grains`` failed to + register that sub-proxy's resources with the master. + """ + proxy_minion = MagicMock(spec=["req_channel", "tok"]) + channel = MagicMock() + channel.auth.gen_token.return_value = b"a-real-token" + + with patch.object( + deltaproxy.salt.channel.client.AsyncReqChannel, "factory", return_value=channel + ) as factory: + returned = deltaproxy.attach_req_channel( + proxy_minion, {"id": "minion1"}, "an-io-loop" + ) + + # The channel is still built exactly as before ... + assert factory.called + assert factory.call_args[0][0] == {"id": "minion1"} + assert factory.call_args[1]["io_loop"] == "an-io-loop" + assert returned is channel + assert proxy_minion.req_channel is channel + + # ... and the sub-proxy now has a real token derived from its auth. + channel.auth.gen_token.assert_called_once_with(b"salt") + assert proxy_minion.tok == b"a-real-token" + + +def test_attach_req_channel_without_auth_does_not_raise(): + """ + Inverse: a clear channel has no ``auth``, and that must leave the + sub-proxy usable rather than blowing up while wiring it in. + """ + proxy_minion = MagicMock(spec=["req_channel"]) + channel = MagicMock(spec=[]) # no .auth + + with patch.object( + deltaproxy.salt.channel.client.AsyncReqChannel, "factory", return_value=channel + ): + deltaproxy.attach_req_channel(proxy_minion, {"id": "minion1"}, None) + + assert proxy_minion.req_channel is channel + assert not hasattr(proxy_minion, "tok")