Native types

[jcpunk]

This PR moves from a series of exec based management commands to puppet native types for the IPMI network, SNMP, and user.

Minimal stubs provided for freeipmi.

[jhoblitt]

https://www.youtube.com/watch?v=RP8uhXuS2n8

[jcpunk]

Probably want fairly extensive testing on real hardware before adding to a release.

[jcpunk]

Any thoughts?

[jhoblitt]

I haven't had the time to do manual testing with a real bmc. Have you had the chance?

[jcpunk]

It appears to behave well on my Supermicro systems. Alas, I don't have another BMC vendor to check against.

[jhoblitt]

This is @jhoblitt's AI agent.

Thanks for taking this on. Native types are the right direction for this module, and the checkouts and user lists you captured from real BMCs are what made most of the findings below reproducible. I reviewed the branch at 3ac10b0 against master and checked each item by driving the new types through Puppet's own Puppet::Transaction and Puppet::Util::Execution with a fake ipmitool fed the PR's fixtures, not by reading alone. Details are in the inline comments; where the fix is mechanical there is a suggestion you can apply directly. I applied every suggestion to a copy of the branch: rubocop is clean, and rspec spec/unit passes except the seven examples that stub user list 1 2>/dev/null or assert the empty-list fallback, which the suggestions deliberately change.

Ranked, most severe first:

1. Every provider is unsuitable. confine commands: {...} is not a confine test; it is a fact check on a fact named commands. All six providers answer suitable? == false, and every ipmi_user, ipmi_network and ipmi_snmp resource fails on an agent. The specs pass because Type.new(provider: ...) skips suitability. Suggestion on each provider.
2. enable misreads Supermicro empty slots. Unknown (0x00) is not NO ACCESS, so the user is never created and the empty slot is granted ADMINISTRATOR instead. Suggestion.
3. A failed user list looks like an empty BMC, and user_id => 'auto' then takes slot 2, the vendor admin slot. Suggestions in ipmi.rb and the ipmitool provider.
4. Name and password are never re-applied on a slot that already reads as enabled, so a rename or password rotation is silently skipped. Needs user and password to become properties.
5. The freeipmi providers use keys and sections that do not exist (Lan_Channel_Channel_N_*, SOL_Payload_Channel_N, Lan_Channel:N, Community_String); the fixtures show the real ones. Suggestions for user and network; snmp needs pef-config.
6. The BMC password reaches the debug log, error messages and the report because nothing passes sensitive: true. Suggestions.
7. Validation moved from compile time to agent time, where a failure aborts the whole run: priv when disabled, CIDR addresses that Stdlib::IP::Address admits.
8. purge_id_mismatch only runs when enable is out of sync, not whenever a mismatched slot exists as the docs promise.
9. community, ip, netmask and gateway are interpolated unescaped into shell strings, behind line-anchored validators. Suggestions.
10. Two defines on the same LAN channel now compile and fight every run; the replaced execs were keyed on the channel. Suggestions.
11. stderr is discarded from every ipmitool and bmc-config call, so failures report returned 1: with no reason. Suggestion, combined with 6.
12. No coverage of getters, setters or the purge: replacing any of them with raise still passes CI.
13. auto can resolve to slot 1 despite the docs, via an empty user or a slot 1 that holds the name. Suggestions.
14. Every BMC read is repeated per property, and a few writes are duplicated. Suggestions where cheap.
15. About 130 lines are copy-pasted across the six providers and have already diverged. Worth hoisting into the base before fixing the same bug six times.

One thing I could not reconcile: given item 1, I would expect nothing to apply on the Supermicro host either. If the providers were selected there, a --debug run would show what differed.

[jhoblitt]

This is @jhoblitt's AI agent.

confine commands: {...} is not a Puppet confine test. Puppet::Confine.test(:commands) returns nil, so the confine collection falls back to a fact confine on a fact named commands, which never exists. Result: every provider in this PR answers suitable? == false, Puppet::Type.type(:ipmi_user).defaultprovider is nil, and pinning provider => 'ipmitool' does not help because Puppet::Type#retrieve re-checks suitability and fails with Provider ipmitool is not functional on this host. I reproduced this on a host with both /usr/bin/ipmitool and /usr/sbin/bmc-config in PATH (openvox 8.25). The unit specs do not catch it because Type.new(provider: 'ipmitool') skips the suitability check.

The idiom is the commands class method, which registers the binary as a confine, defines an ipmitool(*args) helper that runs argv without a shell, and lets execute redact sensitive arguments:

Suggested change

	confine commands: { ipmitool: 'ipmitool' }
	commands ipmitool: 'ipmitool'

Same line in the other five providers (separate suggestions there). Given the earlier note that this behaved well on a Supermicro host, it may be worth re-running there with --debug; if the providers were selected, I would like to know what differed.

[jhoblitt]

This is @jhoblitt's AI agent.

Two things about this helper, which all six providers copy:

1. Passing an options hash to Puppet::Util::Execution.execute turns :combine off, so the child's stderr is discarded (Puppet already sends it to /dev/null when combine is false; the 2>/dev/null suffixes at the call sites are redundant). ipmitool writes every diagnostic to stderr, so each failure surfaces as Execution of '... user set name 6 NEWUSER' returned 1: with nothing after the colon. combine: true puts the reason in the message.
2. Nothing passes sensitive: true, so the command line holding the BMC password is written to the debug log and, on failure, into the error message and the report event sent to the server (see the user set password call below). The base define deliberately passed the password via environment for this reason.

Suggested change

	def ipmitool_exec(args, failonfail: false)
	Puppet::Util::Execution.execute("#{ipmitool_cmd} #{args}", failonfail: failonfail)
	end
	def ipmitool_exec(args, failonfail: false, sensitive: false)
	Puppet::Util::Execution.execute("#{ipmitool_cmd} #{args}", failonfail: failonfail, combine: true, sensitive: sensitive)
	end

Longer term, building argv arrays (execute([ipmitool_cmd, 'user', 'set', 'name', id, name], ...)) instead of shell strings removes the need for shellescape entirely and closes the unescaped interpolations in the network and snmp providers.

[jhoblitt]

This is @jhoblitt's AI agent.

failonfail: false plus the discarded stderr makes a failed user list indistinguishable from an empty BMC. With user_id => 'auto' that empty list then resolves to slot 2 (see the comment in ipmi.rb), which is the vendor ADMIN/admin slot the type's own docs mark as reserved, and enable_user! renames and re-passwords it. I reproduced that end to end through the real Puppet::Util::Execution.execute with an ipmitool whose user list 1 exits 1. A realistic trigger is a BMC whose LAN channel is not 1 (iLO, iRMC, Intel) on the first run, when the ipmi.default.channel fact is absent and init.pp falls back to channel 1. With an explicit user_id the same empty output makes enable read :false and re-runs the whole create sequence.

A failed listing should be an error, not an empty list:

Suggested change

	output = ipmitool_exec("user list #{channel} 2>/dev/null")
	output = ipmitool_exec("user list #{channel}", failonfail: true)

This changes the argument the specs stub ('user list 1 2>/dev/null'), so those expects need updating; the "falls back to a maximum of 15 when the user list is empty" example codifies the behaviour this fixes and should become a "raises" example instead.

[jhoblitt]

This is @jhoblitt's AI agent.

Minor: enable and priv each call this, so every run lists users twice (three times with user_id => 'auto', and again inside the purge). Puppet retrieves all properties before it syncs any, so caching the parsed list per provider instance is safe:

Suggested change

	def find_user_by_id(uid)
	parse_user_list.find { |u| u[:id] == uid }
	end
	def find_user_by_id(uid)
	@user_list ||= parse_user_list
	@user_list.find { |u| u[:id] == uid }
	end

The same applies to lan print in the network provider, which runs once per property (four times for a static ipmi_network that is already in sync).

[jhoblitt]

This is @jhoblitt's AI agent.

Inferring "enabled" from the privilege word alone breaks on Supermicro. The PR's own ipmitool_user_list_supermicro.txt reports every empty slot as Unknown (0x00), which is not NO ACCESS, so this returns :true for an empty slot. Because enable= is the only path that runs user set name, user set password, user enable and sol payload enable, the user is never created. Instead priv= grants the unnamed, passwordless slot ADMINISTRATOR with callin=on ipmi=on link=on, and the second run reports everything in sync. Reproduced with a real Puppet::Transaction against that fixture and ipmi_user { user => 'root', password => 'pw', user_id => 3 }: the only writes are user priv 3 4 1 and channel setaccess 1 3 ....

Treating a slot with no name as disabled fixes that case:

Suggested change

	def enable
	entry = find_user_by_id(resolved_user_id)
	return :false if entry.nil?
	# A user with NO ACCESS privilege is considered disabled
	entry[:privilege] == 'NO ACCESS' ? :false : :true
	end
	def enable
	entry = find_user_by_id(resolved_user_id)
	return :false if entry.nil? || entry[:name].empty?
	# A user with NO ACCESS privilege is considered disabled
	entry[:privilege] == 'NO ACCESS' ? :false : :true
	end

It still cannot see a slot that was user disabled but kept its privilege, and it does not notice a slot that holds a different name (see the user/password comment on the type). ipmitool user list does not expose the enabled flag, which is why the base define keyed on the name.

[jhoblitt]

This is @jhoblitt's AI agent.

The exec this replaces was titled by channel, so two ipmi::snmp declarations aimed at the same LAN channel were a duplicate declaration at compile time. With a free-form title they now compile and fight over the BMC every run with no diagnostic: I built a catalog with Ipmi_snmp[ipmi_snmp_a] (community alpha) and Ipmi_snmp[ipmi_snmp_b] (community beta) on channel 1, and one transaction applied lan set 1 snmp alpha then lan set 1 snmp beta, reporting both as successful changes, every run. Keying the native resource on the channel restores the guard (the define spec's contain_ipmi_snmp('ipmi_snmp_example') expectation changes accordingly):

Suggested change

	ipmi_snmp { "ipmi_snmp_${title}":
	ipmi_snmp { "ipmi_snmp_${_real_lan_channel}":

Making lan_channel the namevar on the types would enforce the same at apply time for direct users of the types.

[jhoblitt]

This is @jhoblitt's AI agent.

Same duplicate-channel guard as ipmi::snmp: the replaced execs were keyed on ${_real_lan_channel}.

Suggested change

	ipmi_network { "ipmi_network_${title}":
	ipmi_network { "ipmi_network_${_real_lan_channel}":

[jhoblitt]

This is @jhoblitt's AI agent.

Same as the dhcp branch.

Suggested change

	ipmi_network { "ipmi_network_${title}":
	ipmi_network { "ipmi_network_${_real_lan_channel}":

[jhoblitt]

This is @jhoblitt's AI agent.

The define spec assertions this PR removes (notify graph, refreshonly, purge ordering) were the only coverage of sync behaviour, and the new provider specs exercise only resolved_user_id. Nothing references enable, enable=, priv, priv=, purge_mismatched_ids!, enable_user!, disable_user!, find_user_by_id, bmc_config_get/bmc_config_set, or any method of the network and snmp providers (there is no spec/unit/puppet/provider/ipmi_network or ipmi_snmp directory). As a check, replacing each of those method bodies with raise still passes rspec spec/unit, which is why the Supermicro enable bug, the never-reapplied name and password, and the purge gating all pass CI. Getter and setter examples driven by the two fixtures you already captured would cover most of it. spec/classes/ipmi_spec.rb also asserts contain_ipmi_network and contain_ipmi_snmp but not contain_ipmi_user, even though it passes users:.

[jhoblitt]

This is @jhoblitt's AI agent.

CHANGELOG.md is generated: the prepare_release workflow runs rake changelog (github_changelog_generator) from merged PR titles and labels, which is where the 8.0.0 section came from. A hand-written section will be overwritten at the next release, so it can be dropped; the PR title and labels are what end up in the changelog.

Suggested change

	## Unreleased
	**Implemented enhancements:**
	- Move `ipmi::user` to puppet native types supporting either `ipmitool` or `freeipmi`
	- support `user_id => 'auto'` for `ipmi_user` and `ipmi::user`