Fix silent and crashing bugs in the netsnmp and netntp states#69794
Open
ggiesen wants to merge 2 commits into
Open
Fix silent and crashing bugs in the netsnmp and netntp states#69794ggiesen wants to merge 2 commits into
ggiesen wants to merge 2 commits into
Conversation
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 saltstack#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes five bugs in the
netsnmpandnetntpNAPALM network-config states -- twoof them silent (report success while doing the wrong thing), which is why they
went unnoticed.
netsnmp:_expand_configdiddefaults.update(config), crashing withAttributeError: 'NoneType' object has no attribute 'update'whenever thestate declares no
defaults(the common case) -- the same pattern as [BUG] Unable to add users on junos via napalm #62170'snetusers bug.
_clear_community_detailshadcommunity_details.get["mode"] = ..., a typothat subscripts the bound
.getmethod and raisesTypeErrorfor everycommunity given in the (documented, recommended) dict form.
_create_diff's third branch waselif not fun(curr), which is unreachableonce the first two branches are past -- so a valid->valid change (e.g.
location"OldTown" -> "NewTown") matched no branch, was dropped from thediff, and the state reported success without pushing the change.
netntp:4.
_checkresolved domain names intoip_only_peersand then didpeers = ip_only_peers, rebinding a local -- the resolved addresses werediscarded, so domain-name peers never converged (the device reports IPs, the
desired list kept the names, so the diff never emptied). It now rewrites the
caller's list in place, and keeps an unresolvable entry (no DNS resolver
available) instead of silently dropping it.
5.
managedmasked a device-retrieval failure as success: a failedntp.peers/ntp.serversproduced no changes, which fell through to the"no changes -> Device configured properly" branch and returned
result=True.The retrieval-failure path now flags
successfully_changed=False, andmanagedreturnsresult=Falsebefore that branch.Validation on real hardware
Validated against a live Juniper EX3400 (Junos 23.4R2) over NETCONF,
test=True(which exercises the full diff path without touching the device templates):
netsnmp.managedwithcommunity: {splab: {mode: read-only}}-> before: theAttributeErrorcrash; after: normalizes the mode toroand returns acorrect diff.
netntp.managed servers=[...]-> a correct add/remove diff against thedevice's real NTP servers.
What issues does this PR fix or reference?
Found by an audit of the NAPALM state modules; part of the same family as
#62170. (
netsnmp's end-to-end apply additionally needs napalm's py2 SNMPtemplates fixed upstream -- napalm-automation/napalm#2326 -- but the state logic
fixed here is independent and was the crashing/silent layer.)
Merge requirements satisfied?
New
tests/pytests/unit/states/test_netsnmp.pyandtest_netntp.py(neithermodule had any tests): the None-
defaultscrash, community-mode normalization,the valid->valid "updated" diff,
_check's in-place resolution, and themasked-retrieval-failure -- each fails against the current code, plus the
behaviour-preserving inverse cases.
Commits signed with GPG?
No