Fix managing users on NAPALM proxy minions (#62170)#69792
Open
ggiesen wants to merge 1 commit into
Open
Conversation
Two failures made `netusers.managed` / `users.set_users` unusable on NAPALM
(proxy) minions:
- `netusers.managed` passes its optional `defaults` to `_expand_users` as
`common_users`, then does `copy.deepcopy(common_users).update(...)`. With no
`defaults` (the common case) that is `copy.deepcopy(None).update(...)`, which
raises `AttributeError: 'NoneType' object has no attribute 'update'`.
- `users.set_users` / `users.delete_users` call `net.load_template("set_users",
...)` with a bare template name. `net.load_template` used to route bare names
into NAPALM's own renderer, but that path was removed in the Sodium release
(saltstack#57370) -- whose own deprecation warning explicitly told `netusers`/`netntp`/
`netsnmp` users they could ignore it. The bare name now falls through to the
fileserver as `Local file source set_users does not exist`.
`_expand_users` now treats a missing `defaults` as `{}`. `set_users` /
`delete_users` resolve the NAPALM-shipped per-driver template (`set_users.j2` /
`delete_users.j2`, which NAPALM still ships, searched along the driver class
MRO) to an absolute path and render it through the Salt pipeline, and return a
clear message when a driver ships no such template.
Because the crash previously masked it, `managed` now refuses to proceed when
the expanded user set is empty rather than removing (and committing) every
account on the device -- an admin-lockout foot-gun, e.g. an empty pillar lookup.
Validated end-to-end against a live Juniper EX3400 (Junos 23.4R2) over NETCONF:
users are rendered, committed, confirmed on-device, and removed again.
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?
Restores user management on NAPALM (proxy) minions, which has been broken since
the Sodium release. Two distinct failures are fixed:
1.
netusers.managedcrash when nodefaultsare declared.managed(name, users=None, defaults=None)passesdefaultsto_expand_usersas
common_users, then doescopy.deepcopy(common_users).update(...). With nodefaults(the common case, and the case in the issue's SLS),common_usersisNone, so the state dies with:_expand_usersnow treats a missingdefaultsas an empty mapping.Because that crash previously masked a hazard --
netusers.managedisdeclarative, so an empty expanded user set means "remove every user" --
managednow refuses to proceed when neither
usersnordefaultsyields anyone tomanage, instead of silently wiping (and committing) every account on the device.
This closes an admin-lockout foot-gun, e.g. a pillar lookup that renders to
{}.2.
users.set_users/users.delete_users-> "Local file source set_users does not exist".These functions call
net.load_template("set_users", ...)with a bare templatename.
net.load_templateused to route bare names into NAPALM's own renderer,but that path was removed in Sodium (#57370). The bare name now falls through to
the fileserver, which reports it as a missing local file. Even after working
around bug 1,
netusers.managedtherefore fails withCannot configure new users: Local file source set_users does not exist.Why this is a regression, not a deprecation
The Sodium deprecation warning that preceded the removal said (emphasis added):
The
net<x>state modules and their backingset_*/delete_*functions takeno template argument -- there is nowhere for a user to supply a
salt://template -- so the "use the Salt rendering pipeline" guidance was never aimed at
them, and the warning explicitly told their users to ignore it. But #57370
deleted the entire native-template branch, including the code path those
exempted modules relied on. This PR fixes the
netusershalf of that overshoot.Fix
set_users/delete_usersnow resolve the NAPALM-shipped per-driver template(
set_users.j2/delete_users.j2, which NAPALM still ships next to each drivermodule) to an absolute path via a small
_napalm_template_pathhelper thatmirrors NAPALM's own class-MRO template search, then hand that absolute path to
net.load_template. The template is rendered through the Salt pipeline --which is what the deprecation nudged toward -- instead of NAPALM's removed native
path. If a driver ships no such template (e.g.
ioshas no user templates), thefunctions now return a clear "template is not available for the ''
driver" message instead of the misleading fileserver error.
Validation on real hardware
Reproduced both failures, then validated the fix end-to-end against a live
Juniper EX3400 (Junos 23.4R2) over NETCONF/SSH, driving a real
napalm/junosproxy minion (onedir 3006.x):
netusers.managedwith nodefaults,test=True: crashes with theAttributeErrorbefore the fix; after the fix returns a proper diff and"Testing mode: configuration was not changed!".
users.set_users {'u': {'level': 1}}(class-only) andusers.set_users {'u': {'level': 5, 'sshkeys': [...]}}(ssh key): before thefix, "Local file source ... does not exist"; after the fix the user is
rendered, committed, and confirmed present in the device config, then removed
again with
users.delete_users.Note: NAPALM's shipped
set_users.j2emitsplain-text-password, which Junosrejects over NETCONF (it is an interactive-only statement) -- so cleartext
passwords still fail, independent of this change and identically on NAPALM's own
renderer. SSH keys and class-only users work. That template-content limitation
is a NAPALM concern, not Salt's.
Scope
Fixes the
netusers/napalm_userspath reported in the issue.napalm_ntpandnapalm_snmp(the other two modules the deprecation exempted) carry theidentical bare-name regression and could get the same treatment in a follow-up.
What issues does this PR fix or reference?
Fixes #62170
Merge requirements satisfied?
Direct-altitude unit tests:
_expand_users(None)no longer raises (and stillmerges provided defaults);
managedrefuses an empty user set instead ofwiping the device;
_napalm_template_pathresolves a shipped template(exercising the class-MRO walk and the
object-> exception fall-through) andreturns
Nonewhen absent;set_users/delete_userspass the resolvedabsolute path -- and the correct template name -- to
net.load_template, andreturn a clear error when the driver ships no template. All fail against the
current code, and the routing/MRO tests are mutation-checked (a set/delete name
swap or a collapsed MRO walk makes them fail).
Commits signed with GPG?
No