augeas should not be the default provider for sshkey

[alexjfisher]

I started using this module for the ssh_config type, but meanwhile it became the default provider for sshkey (a type from the sshkeys_core module). This has had a hideous impact on performance.

In my testing, 2000 resources take 330 seconds to enforce (even with no changes), compared to 0.91 seconds if I force the provider back to parsed.

It's not uncommon to manage a large number of sshkey resources, so I think using this module to manage sshkey resources should be opt-in, (or the sshkey provider moved into its own module).

[alexjfisher]

When I look at just the sshkey execution time as displayed in reports sent to Foreman, I have seen a 2000x speed difference on a host with about 2.5k sshkey resources.

[binford2k]

My opinion is that 300-2000x performance decrease should not ever be the default. If people want additional features, they should have to opt in to it.

@joshcooper even if we remove the defaultfor, it will still be selected because it's got a confine and is more suitable, correct? Do you know why parsed isn't the default?

[alexjfisher]

According to #55 this provider is currently broken; which is odd as it didn't explode for me, (but it wasn't making any changes so maybe I missed the broken code path.)

Also quite scary how it monkey patches the original type...

puppet-augeasproviders_ssh/lib/puppet/provider/sshkey/augeas.rb

Line 18 in 3a16b2a

# Patch sshkey's ensure parameter to add hashed value

Maybe this made slightly more sense at the time, when the type was in puppet and not split into its own module.

[joshcooper]

it will still be selected because it's got a confine and is more suitable, correct?

If there are no default providers,, then puppet magically picks the provider with the "most specificity" https://github.com/puppetlabs/puppet/blob/a423af825d1e87a3158cd68d630e77352e9a5454/lib/puppet/provider.rb#L336-L345

The whole specificity thing is fragile and dumb. Refactoring a provider to use a new base provider can cause the specificity to change, thereby causing a different provider to be chosen.

Do you know why parsed isn't the default?

Probably because it used to be in core and I'm guessing predated this provider? TBH I don't think either provider should be the defaultfor because they don't support composite namevars in the same way. Instead require users to specify which provider they want to use.

[joshcooper]

300-2000x performance decrease

This is also strange. I would expect augeas to be much faster than parsed file, unless augeas is accidentally loading the entire /etc/ directory. Maybe the provider is not setting the incl filter correctly? https://augeas.net/docs/builtins.html

[alexjfisher]

300-2000x performance decrease

This is also strange. I would expect augeas to be much faster than parsed file, unless augeas is accidentally loading the entire /etc/ directory. Maybe the provider is not setting the incl filter correctly? https://augeas.net/docs/builtins.html

puppet-augeasproviders_ssh/lib/puppet/provider/sshkey/augeas.rb

Lines 82 to 97 in 5d20cee

	def self.find_resource(aug, hostname)
	aug.match('$target/*[label()!="#comment"]').each do |entry|
	hostnames = aug.get(entry)
	# Clear value
	return entry if hostnames.split(',')[0] == hostname
	next unless hashed?(hostnames)
	require 'base64'
	_dummy, _one, salt64, hostname64 = hostnames.split[0].split('|')
	salt = Base64.decode64(salt64)
	return entry if hostname64 == Base64.encode64(OpenSSL::HMAC.digest('sha1', salt, hostname)).strip
	end
	nil
	end

is crazy expensive (even if you're not using hashed hostnames) with a large number of resources. Looks like it gets called 3 times per sshkey catalog resource.

With 1000 sshkey resources that's ~ 1.5 MILLION calls to aug.get as it iterates over non-comment entries until it finds the resource.