From be02fae9a615505213642baf9e2ee2ee323cc4ac Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sat, 11 Jul 2026 23:30:19 -0400 Subject: [PATCH 1/2] Fix silent and crashing bugs in the netsnmp and netntp states netsnmp: - _expand_config did ``defaults.update(config)`` and crashed with ``AttributeError: 'NoneType' object has no attribute 'update'`` whenever the state declared no ``defaults`` (the common case; the netusers twin of #62170). - _clear_community_details had ``community_details.get["mode"] = ...``, a typo subscripting the bound ``.get`` method that raised ``TypeError`` for every community given in the documented dict form; ``mode`` is also defaulted now. - _create_diff's third branch was ``elif not fun(curr)``, unreachable once the first two are past, so a valid->valid change (e.g. location "A" -> "B") matched no branch, was dropped from the diff, and the state reported success without pushing the change. netntp: - _check resolved names into ``ip_only_peers`` then did ``peers = ip_only_peers`` (rebinding a local), so the resolved addresses were discarded and name-based peers never converged; it now rewrites the list in place, keeps an unresolvable entry (no resolver) instead of dropping it, and tolerates any ``dns.exception.DNSException`` rather than only ``NoAnswer``. - managed masked a device-retrieve failure (which produces no changes) as "Device configured properly."; it now reports result=False for a failure with nothing staged, while still letting the commit path handle a partial change. Both state modules had no unit tests; add tests/pytests/unit/states/test_netsnmp.py and test_netntp.py covering each fix and the behaviour-preserving cases. --- salt/states/netntp.py | 31 ++++++++-- salt/states/netsnmp.py | 32 +++++++++-- tests/pytests/unit/states/test_netntp.py | 69 +++++++++++++++++++++++ tests/pytests/unit/states/test_netsnmp.py | 63 +++++++++++++++++++++ 4 files changed, 185 insertions(+), 10 deletions(-) create mode 100644 tests/pytests/unit/states/test_netntp.py create mode 100644 tests/pytests/unit/states/test_netsnmp.py diff --git a/salt/states/netntp.py b/salt/states/netntp.py index f5bc732b82e7..a3c20aee6283 100644 --- a/salt/states/netntp.py +++ b/salt/states/netntp.py @@ -38,6 +38,7 @@ HAS_NETADDR = False try: + import dns.exception # pylint: disable=no-name-in-module import dns.resolver # pylint: disable=no-name-in-module HAS_DNSRESOLVER = True @@ -115,19 +116,28 @@ def _check(peers): # if not a valid IP Address # will try to see if it is a nameserver and resolve it if not HAS_DNSRESOLVER: - continue # without the dns resolver cannot populate the list of NTP entities based on their nameserver - # so we'll move on + # without the dns resolver we cannot resolve the name; keep the + # entry as specified and let the device validate it on load + ip_only_peers.append(peer) + continue dns_reply = [] try: # try to see if it is a valid NS dns_reply = dns.resolver.query(peer) - except dns.resolver.NoAnswer: - # no a valid DNS entry either + except dns.exception.DNSException: + # not a resolvable name either (NoAnswer, NXDOMAIN, Timeout, + # NoNameservers, ...); treat the input as invalid rather than + # letting the DNS error abort the whole state run return False for dns_ip in dns_reply: ip_only_peers.append(str(dns_ip)) - peers = ip_only_peers + # Rewrite the caller's list in place with the resolved addresses. ``_check`` + # is documented to transform domain names into IP addresses, but the old + # ``peers = ip_only_peers`` only rebound the local name, so the resolved + # values were discarded and domain-name peers never converged (the device + # reports IPs, the desired list kept the names, so the diff never emptied). + peers[:] = ip_only_peers return True @@ -187,6 +197,7 @@ def _check_diff_and_configure(fun_name, peers_servers, name="peers"): _ret["comment"] = "Cannot retrieve NTP {what} from the device: {reason}".format( what=name, reason=ntp_list_output.get("comment") ) + _ret["successfully_changed"] = False return _ret configured_ntp_list = set(ntp_list_output.get("out", {})) @@ -367,6 +378,16 @@ def managed(name, peers=None, servers=None): ret.update({"changes": changes}) + if not successfully_changed and not expected_config_change: + # A failure with nothing staged (e.g. the device retrieve failed, which + # is the case that previously fell through to the "no changes -> + # configured properly" branch below and was reported as result=True). + # Report it. When something *was* staged before a later step failed + # (expected_config_change), fall through so the existing commit path + # still deals with the candidate rather than leaving it dangling. + ret.update({"result": False, "comment": comment}) + return ret + if not (changes or expected_config_change): ret.update({"result": True, "comment": "Device configured properly."}) return ret diff --git a/salt/states/netsnmp.py b/salt/states/netsnmp.py index f18dbd8b44ff..09a131333879 100644 --- a/salt/states/netsnmp.py +++ b/salt/states/netsnmp.py @@ -17,6 +17,7 @@ .. versionadded:: 2016.11.0 """ +import copy import logging import salt.utils.json @@ -72,8 +73,14 @@ def _expand_config(config, defaults): Completed the values of the expected config for the edge cases with the default values. """ - defaults.update(config) - return defaults + # ``defaults`` (the state's optional ``defaults`` argument) is ``None`` when + # unset, and ``config`` may be too; treat either as an empty mapping rather + # than crashing on ``None.update()`` (the netusers twin of #62170). deepcopy + # so the nested community detail dicts are not aliased into the caller's + # data -- ``_clear_community_details`` mutates them in place downstream. + expected = copy.deepcopy(defaults) if defaults else {} + expected.update(copy.deepcopy(config) if config else {}) + return expected def _valid_dict(dic): @@ -108,7 +115,12 @@ def _clear_community_details(community_details): for key in ["acl", "mode"]: _str_elem(community_details, key) - _mode = community_details.get["mode"] = community_details.get("mode").lower() + # NB: ``community_details.get["mode"]`` was a typo for + # ``community_details["mode"]`` -- it subscripted the bound ``.get`` method + # and raised ``TypeError`` for every dict-form community. ``mode`` may also + # be absent (``_str_elem`` drops an invalid value), so default it. + _mode = (community_details.get("mode") or "ro").lower() + community_details["mode"] = _mode if _mode in _COMMUNITY_MODE_MAP: community_details["mode"] = _COMMUNITY_MODE_MAP.get(_mode) @@ -203,9 +215,14 @@ def _create_diff(diff, fun, key, prev, curr): if not fun(prev): _create_diff_action(diff, "added", key, curr) - elif fun(prev) and not fun(curr): - _create_diff_action(diff, "removed", key, prev) elif not fun(curr): + _create_diff_action(diff, "removed", key, prev) + else: + # Both previous and current values are valid and -- since _compute_diff + # only calls this when they differ -- not equal: the value changed. The + # old ``elif not fun(curr)`` here was unreachable, so a valid->valid + # change (e.g. location "A" -> "B") was silently dropped from the diff + # and the state reported success without pushing it. _create_diff_action(diff, "updated", key, curr) @@ -222,6 +239,11 @@ def _compute_diff(existing, expected): for key in ["community"]: # for the moment only onen if existing.get(key) != expected.get(key): + # NOTE: the whole community mapping is diffed as one opaque value, so + # a change lands in "updated" and is applied via snmp.update_config + # (add/modify). Removing an individual community from a multi-entry + # set is not expressed here and is left for a follow-up that diffs + # communities individually. _create_diff(diff, _valid_dict, key, existing.get(key), expected.get(key)) return diff diff --git a/tests/pytests/unit/states/test_netntp.py b/tests/pytests/unit/states/test_netntp.py new file mode 100644 index 000000000000..9a634518a19f --- /dev/null +++ b/tests/pytests/unit/states/test_netntp.py @@ -0,0 +1,69 @@ +""" +Unit tests for the netntp state. +""" + +import pytest + +import salt.states.netntp as netntp +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def configure_loader_modules(): + return {netntp: {"__salt__": {}, "__opts__": {"test": False}}} + + +def test_check_rejects_non_list(): + assert netntp._check("192.0.2.1") is False + + +def test_check_resolves_in_place(): + # ``_check`` is documented to transform names into IP addresses; the resolved + # values must replace the caller's list in place (the old ``peers = ...`` + # only rebound the local, discarding them). + peers = ["192.0.2.1", "192.0.2.2"] + # netaddr may be absent in the test env, so IPAddress is not always bound + # in the module namespace; create=True lets us patch it regardless. + with patch("salt.states.netntp.HAS_NETADDR", True), patch( + "salt.states.netntp.IPAddress", create=True, side_effect=lambda p: f"ip:{p}" + ): + result = netntp._check(peers) + assert result is True + assert peers == ["ip:192.0.2.1", "ip:192.0.2.2"] + + +def test_check_keeps_unresolvable_without_resolver(): + # An entry that is neither an IP nor resolvable (no DNS resolver available) + # is kept as specified, not silently dropped from the desired list. + class _AddrErr(Exception): + pass + + def _raise(peer): + raise _AddrErr(peer) + + peers = ["ntp.example.com"] + with patch("salt.states.netntp.HAS_NETADDR", True), patch( + "salt.states.netntp.HAS_DNSRESOLVER", False + ), patch("salt.states.netntp.AddrFormatError", _AddrErr, create=True), patch( + "salt.states.netntp.IPAddress", create=True, side_effect=_raise + ): + result = netntp._check(peers) + assert result is True + assert peers == ["ntp.example.com"] + + +def test_managed_reports_retrieval_failure(): + # A device-retrieval failure must surface as result=False, not be masked as + # "Device configured properly." by the no-changes branch. + ntp_peers = MagicMock(return_value={"result": False, "comment": "boom"}) + with patch.dict(netntp.__salt__, {"ntp.peers": ntp_peers}): + ret = netntp.managed("t", peers=["192.0.2.1"]) + assert ret["result"] is False + assert "Cannot retrieve NTP peers" in ret["comment"] + + +def test_managed_no_args_is_noop(): + # Neither peers nor servers supplied -> exit without touching the device. + ret = netntp.managed("t") + assert ret["result"] is False + assert ret["changes"] == {} diff --git a/tests/pytests/unit/states/test_netsnmp.py b/tests/pytests/unit/states/test_netsnmp.py new file mode 100644 index 000000000000..2a905d88789f --- /dev/null +++ b/tests/pytests/unit/states/test_netsnmp.py @@ -0,0 +1,63 @@ +""" +Unit tests for the netsnmp state. +""" + +import pytest + +import salt.states.netsnmp as netsnmp + + +@pytest.fixture +def configure_loader_modules(): + return {netsnmp: {}} + + +def test_expand_config_without_defaults(): + # The state's optional ``defaults`` is None when unset -- must not crash. + assert netsnmp._expand_config({"location": "DC1"}, None) == {"location": "DC1"} + + +def test_expand_config_merges_defaults(): + # Per-config values win over defaults on a key collision. + assert netsnmp._expand_config( + {"location": "DC1"}, {"contact": "noc", "location": "old"} + ) == {"contact": "noc", "location": "DC1"} + + +def test_clear_community_details_normalizes_mode(): + # ``read-write``/``write`` -> ``rw``; case-folded; the old ``get["mode"]`` + # typo raised TypeError for every one of these. + assert netsnmp._clear_community_details({"mode": "read-write"})["mode"] == "rw" + assert netsnmp._clear_community_details({"mode": "RO"})["mode"] == "ro" + # Missing mode -> default read-only. + assert netsnmp._clear_community_details({})["mode"] == "ro" + # Unrecognised mode -> default read-only. + assert netsnmp._clear_community_details({"mode": "bogus"})["mode"] == "ro" + + +def test_compute_diff_updated_value_not_dropped(): + # location "OldTown" -> "NewTown": both valid strings. Regression: the old + # dead ``elif not fun(curr)`` branch dropped this, so the change was never + # pushed and the state falsely reported success. + diff = netsnmp._compute_diff({"location": "OldTown"}, {"location": "NewTown"}) + assert diff == {"updated": {"location": "NewTown"}} + + +def test_compute_diff_added_and_removed(): + assert netsnmp._compute_diff({}, {"location": "DC1"}) == { + "added": {"location": "DC1"} + } + assert netsnmp._compute_diff({"location": "DC1"}, {}) == { + "removed": {"location": "DC1"} + } + + +def test_compute_diff_community_updated(): + # The community mapping is diffed via _valid_dict; a mode change on an + # existing community is a valid-dict -> valid-dict update and must land in + # "updated" (exercises the else branch for the dict case, not just str). + diff = netsnmp._compute_diff( + {"community": {"public": {"mode": "ro"}}}, + {"community": {"public": {"mode": "rw"}}}, + ) + assert diff == {"updated": {"community": {"public": {"mode": "rw"}}}} From 46c54470a0921949dc1b466b6aed20a0a8ce7cdb Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sat, 11 Jul 2026 23:30:43 -0400 Subject: [PATCH 2/2] Add changelog for #69794 --- changelog/69794.fixed.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/69794.fixed.md diff --git a/changelog/69794.fixed.md b/changelog/69794.fixed.md new file mode 100644 index 000000000000..1aa63c43997a --- /dev/null +++ b/changelog/69794.fixed.md @@ -0,0 +1,8 @@ +Fixed several bugs in the ``netsnmp`` and ``netntp`` NAPALM states. ``netsnmp`` +no longer crashes with ``AttributeError: 'NoneType' object has no attribute +'update'`` when no ``defaults`` are declared, no longer raises ``TypeError`` on a +dict-form SNMP community, and no longer silently drops (and reports success for) +a changed ``location``/``contact``/``chassis_id``. ``netntp`` now actually +converts domain-name peers/servers to IP addresses instead of discarding the +resolved values, and no longer reports a device-retrieval failure as +"Device configured properly.".