Give deltaproxy sub-proxies a token for the master (#70071)

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.

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:

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

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