Skip to content

Fix crash/mutation/log bugs in the shared napalm utils and proxy#69796

Open
ggiesen wants to merge 2 commits into
saltstack:3006.xfrom
ggiesen:fix-napalm-utils-proxy
Open

Fix crash/mutation/log bugs in the shared napalm utils and proxy#69796
ggiesen wants to merge 2 commits into
saltstack:3006.xfrom
ggiesen:fix-napalm-utils-proxy

Conversation

@ggiesen

@ggiesen ggiesen commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes three bugs in the shared NAPALM machinery (salt/utils/napalm.py and
salt/proxy/napalm.py) that every NAPALM module and proxy sits on top of.

  1. get_device_opts crashes on optional_args: null and mutates the caller's
    config.
    device_dict.get("optional_args", {}) returns None when the key is
    present but null (the default only applies to a missing key), so the
    "config_lock" not in ... membership test raised TypeError. And when
    optional_args was a dict, it was used by reference -- the config_lock /
    keepalive defaults injected below were written straight back into the user's
    live opts/pillar structure. Now copy.deepcopy(... or {}).

  2. force_reconnect raises KeyError: 'proxy' on a straight minion.
    proxy_napalm_wrap did opts["proxy"].update(**kwargs) unconditionally, but a
    straight (non-proxy) NAPALM minion has no proxy key. That merge is only
    needed for the always-alive proxy path; a straight minion already picks the
    override up from clean_kwargs further down, so it's now guarded by
    is_proxy(opts).

  3. proxy.shutdown logs a malformed port. A trailing comma made port a
    1-tuple, so a failed close() logged :(830,) / :(None,) (and the
    if port guard never suppressed the empty case). Comma removed.

What issues does this PR fix or reference?

Found by an audit of the NAPALM modules (same series as #69793 / #69794 /
#69795).

Note

The audit also found a salt.utils.napalm.call() reconnect issue -- in
not-always-alive mode, the inner open() during a ConnectionClosedException
retry is closed by its own finally before the command re-runs. It needs a more
careful fix plus a reconnect test harness, so it is intentionally left for a
follow-up rather than shipped unvalidated here.

This PR adds tests/pytests/unit/utils/test_napalm.py; #69793 also adds that
file (different test functions), so whichever merges second needs a trivial
combine.

Merge requirements satisfied?

  • Docs
  • Changelog
  • Tests written/updated

test_get_device_opts_null_optional_args, test_get_device_opts_does_not_mutate_caller,
test_proxy_napalm_wrap_force_reconnect_straight_minion (new
utils/test_napalm.py) and test_shutdown_logs_scalar_port_on_close_failure
(added to proxy/test_napalm.py). All fail against the current code.

Commits signed with GPG?

No

- get_device_opts: optional_args explicitly set to null yielded None (the get
  default only applies to a missing key), crashing the "config_lock" membership
  test; and a present optional_args dict was mutated in place, leaking the
  config_lock / keepalive defaults into the caller's opts/pillar. Use
  copy.deepcopy(device_dict.get("optional_args") or {}).

- proxy_napalm_wrap: force_reconnect did opts["proxy"].update(**kwargs)
  unconditionally, raising KeyError on a straight (non-proxy) minion which has
  no 'proxy' key. That merge is only for the always-alive proxy path; a straight
  minion picks the override up from clean_kwargs, so guard it with is_proxy().

- proxy.shutdown: a trailing comma made 'port' a 1-tuple, so a failed close()
  logged ':(830,)' / ':(None,)'. Remove it.
@ggiesen ggiesen requested a review from a team as a code owner July 12, 2026 03:58
@dwoz dwoz added the test:full Run the full test suite label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants