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")