Fix silent and crashing bugs in the netsnmp and netntp states

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

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

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

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"] == {}

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