fix: initialize self.tok on sub-proxies to prevent AttributeError [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT]

[waterWang]

What does this PR do?

Fixes #70071 — deltaproxy sub-proxies raise AttributeError: 'ProxyMinion' object has no attribute 'tok' when dispatching events.

Why?

Sub-proxies are constructed via ProxyMinion(proxyopts) directly in subproxy_post_master_init and never run connect_master, so self.tok is never set by the usual master-connection path. Any code path that references self.tok (e.g. _fire_master_prepare, _register_resources, _mine_send) raises AttributeError.

Fix

Set _proxy_minion.tok = None after construction in subproxy_post_master_init. This is safe because the channel layer's _package_load regenerates the token via self.auth.gen_token(b"salt") on every send, overwriting whatever value was in the load dict.

Tests

• test_subproxy_post_master_init_packs_per_minion_grains already exercises the sub-proxy creation path; added assertion that _proxy_minion.tok is set (not AttributeError).

New behavior

Sub-proxies can now fire events to the master without raising AttributeError.

[twangboy]

Let's make this against the 3008.x branch. Also, this needs a test and a changelog.

[ggiesen]

@twangboy already asked for this to target 3008.x with a test and a changelog back in August; this comment is not meant to relitigate that, it adds the reproduction and root-cause detail behind it, plus a suggested alternative to the current fix.

I reproduced this independently on a 4-sub-proxy deltaproxy (salt 3008.2, dummy proxytype, multiprocessing: False) to corroborate the diagnosis. The underlying bug is real and your analysis of the cause is correct. A few findings and suggestions.

Confirming the cause, and narrowing the trigger

The AttributeError is not raised by general event dispatch. In my testing only two calls trigger it, once per sub-proxy:

call	tok errors (4 sub-proxies)
event.send	0
mine.update	0
status.master	0
saltutil.refresh_pillar	4
saltutil.refresh_grains	4

The captured traceback is:

handle_event -> ProxyMinion.handle_event (minion.py:4329)
  -> pillar_refresh (minion.py:4094)
    -> _register_resources_with_master (minion.py:3974)
       "tok": self.tok  ->  AttributeError

So the real consequence is narrower but more serious than "sub-proxies cannot fire events": sub-proxy resources are never registered with the master. Everything earlier in pillar_refresh (pillar compile, opts["pillar"], the __pillar__ repack, _discover_resources) completes first, so the pillar refresh itself still appears to work and the breakage is easy to miss. Per the docstring on _register_resources_with_master, an empty resource dict is sent deliberately to clear stale entries, so this also means stale registry entries are never cleared.

Worth noting for the issue text: I verified that pillar refresh, event.send, mine.update and pillar targeting (-I) all still work correctly without this patch. matchers_refresh() is skipped because the exception propagates past it, but that proved harmless since matchers read live opts.

The PR body describes a test that is not in the diff, and there is no changelog

The body says "added assertion that _proxy_minion.tok is set (not AttributeError)", but the diff is one file, +6/-0, touching only deltaproxy.py. There is no test. Salt's merge checklist requires tests, and this one is easy to write directly against subproxy_post_master_init in tests/pytests/unit/metaproxy/test_deltaproxy.py, which already has fixtures for that function. A good test here would also fail against unmodified code, which a hasattr assertion would. There is also no changelog/70071.fixed.md, which @twangboy flagged in his review.

tok = None masks the problem rather than fixing it

The reasoning that the channel regenerates the token is correct as far as it goes: AsyncReqChannel._package_load sets load["tok"] = self.auth.gen_token(b"salt") unconditionally when crypt == "aes" (salt/channel/client.py:157), so the placeholder is overwritten before anything reaches the wire. I confirmed that empirically.

But that makes the attribute merely present, not correct, and it only holds for that one path. self.tok is also read by _fire_master_prepare (minion.py:2297) and _mine_send (minion.py:4423), and the overwrite does not happen on a clear channel. Any future consumer, or any path that does not go through an AES req channel, will now silently ship None instead of raising, which is a worse failure mode than the current loud one.

A suggested alternative

Set a real token the same way the normal connection path does. connect_master does self.tok = pub_channel.auth.gen_token(b"salt") (minion.py:1115, 1175). Sub-proxies already get a channel assigned in post_master_init, and AsyncReqChannel.factory constructs auth eagerly, so the token can be derived at the same point:

self.deltaproxy_objs[minion_id].req_channel = (
    salt.channel.client.AsyncReqChannel.factory(
        sub_proxy_data["proxy_opts"], io_loop=self.io_loop
    )
)
# Mirror what connect_master() does for a normal minion.
_ch = self.deltaproxy_objs[minion_id].req_channel
if getattr(_ch, "auth", None) is not None:
    self.deltaproxy_objs[minion_id].tok = _ch.auth.gen_token(b"salt")

That applies at both req_channel assignment sites in deltaproxy.py (the parallel and non-parallel startup branches). I tested this variant on the same rig: the AttributeError disappears, no new tracebacks appear, resource registration completes with no "Unable to register resources" warnings, and pillar refresh continues to work.

Separately, and independent of which fix is chosen, _register_resources_with_master builds its load outside the try/except that already exists to tolerate registration failures. Moving the load construction inside that block would stop a trivial attribute problem from escalating into an exception that aborts pillar_refresh.

Branch

Per the guidance announced on the Salt community call, bug fixes should target 3008.x and new features target master (3009 STS). CONTRIBUTING.rst says the same in general terms: bug fixes go against the oldest supported branch where the bug exists, with master reserved for new features and enhancements. This is a bug fix, so it should be retargeted to 3008.x, as @twangboy already requested.

That also matches where the failure actually lives: the path I reproduced, _register_resources_with_master, exists only on 3008.x. It is absent from 3006.x and 3007.x, where the single "tok": self.tok site is _fire_master_prepare. Sub-proxies are constructed without a tok on all three branches, so the latent gap itself is older, but the reachable failure is 3008-only.

[twangboy]

@ggiesen Would you like to open a more comprehensive fix against 3008.x? Then we'll close this one.

[ggiesen]

Heads up @waterWang -- I have opened #70228 against 3008.x for this, since #70073 has had changes requested since 18 August and there is a 3008.3 release in the near term that it would be good not to miss.

Your diagnosis here was correct and I have credited it in that PR. The differences are that it derives a real token from the sub-proxy's req_channel auth (the way connect_master does) rather than setting the attribute to None, and it adds the test, changelog and branch that were asked for above.

On None specifically: your reasoning about the channel regenerating the token does hold -- AsyncReqChannel._package_load overwrites load["tok"] unconditionally when crypt == "aes", which I confirmed. It just does not cover the other readers of self.tok (_fire_master_prepare, _mine_send) or a clear channel, where a None would go out silently instead of raising.

No objection at all if you would rather take it from here -- happy to close mine in favour of an updated #70073.

[twangboy]

Closing in favor of #70228