(PUP-3399) autorequire local file sources

[evgeni]

No description provided.

[puppetlabs-jenkins]

Can one of the admins verify this patch?

[CLAassistant]

All committers have signed the CLA.

[evgeni]

it seems on Windows, the file:// prefix is not automatically added and thus this inclusion fails.

[evgeni]

Or rather, it is, but the path being set as file:///D:/bar (note the tree slashes), which is obviously not a valid local windows path pointing at D:/bar

[evgeni]

5fea1dc

[joshcooper]

If source is an absolute path, then it will have been normalized in

puppet/lib/puppet/type/file/source.rb

Lines 111 to 118 in 7f41466

	if Puppet::Util.absolute_path?(source)
	# CGI.unescape will butcher properly escaped URIs
	uri_string = Puppet::Util.path_to_uri(source).to_s
	# Ruby 1.9.3 and earlier have a URI bug in URI
	# to_s returns an ASCII string despite UTF-8 fragments
	# since its escaped its safe to universally call encode
	# Puppet::Util.uri_unescape always returns strings in the original encoding
	Puppet::Util.uri_unescape(uri_string.encode(Encoding::UTF_8))

Essentially doing:

irb(main):034:0> Puppet::Util.uri_unescape(Puppet::Util.path_to_uri("/a path").to_s)
=> "file:///a path"

Since the source has been normalized, I think you can simply do:

irb(main):038:0> Puppet::Util.uri_to_path(URI(Puppet::Util.uri_encode(source)))
=> "/a path"

And it will work correctly on all platforms (including Windows).

Is there a particular scenario where this comes up frequently? Or just something that would be good to fix?

[evgeni]

"this" being Windows or autorequires for the source attribute?

If the latter: we had it come up in one of our (the foreman) modules and I thought it'd be nice to have. Certainly not everyday material.

[joshcooper]

"this" being Windows or autorequires for the source attribute?

yeah the latter, wondering when you'd need to autorequire local sources.

I thought it'd be nice to have. Certainly not everyday material.

I'm always hesitant to modify the file type/provider given the amount of complexity there. For example, are there cases where a file is ensure => absent but has a source? Puppet doesn't automatically reverse the dependency order so we run into things like #9114 (comment)

[evgeni]

I opened this because of this hack we have over in theforeman/foreman_proxy:
https://github.com/theforeman/puppet-foreman_proxy/blob/69c038e607d9fd9558436b2afad312949ef530d7/manifests/plugin/remote_execution/mosquitto.pp#L99-L103

The TL;DR version is:

We have a different deployment modes:

• use Puppet CA for certs
• use a special module (certs) that deploys the certs to a specific place on the system
• let the user break stuff at their will

In either case, the certs need to land in a place where mosquitto can read them (selinux, perms, etc), so we need to copy them from one place to another.

Local source seems like the correct way here, but it needs an explicit require, IF the thing is managed by puppet (it might not be).

[ekohl]

I'm always hesitant to modify the file type/provider given the amount of complexity there. For example, are there cases where a file is ensure => absent but has a source? Puppet doesn't automatically reverse the dependency order so we run into things like #9114 (comment)

I think a source shouldn't apply if something is ensured absent, but you raise a good point.

At first glance, the whole autorequire looks like it's not well designed for ensuring absent. The target relation is not needed either (but probably doesn't hurt in practice). You'd think Finding all ancestors up to the root however is not needed at all and could even introduce problems. Assuming mkdir -p /path/sub:

file { ['/path', '/path/sub']::
  ensure => absent,
}

Yields:

# puppet apply test.pp 
Notice: Compiled catalog for host.example.com in environment production in 0.01 seconds
Notice: /Stage[main]/Main/File[/tmp/path]: Not removing directory; use 'force' to override
Notice: /Stage[main]/Main/File[/tmp/path]/ensure: removed
Notice: /Stage[main]/Main/File[/tmp/path/sub]: Not removing directory; use 'force' to override
Notice: /Stage[main]/Main/File[/tmp/path/sub]/ensure: removed
Notice: Applied catalog in 0.01 seconds

Ok, misleading. So we modify it to:

# puppet apply test.pp 
Notice: Compiled catalog for host.example.com in environment production in 0.01 seconds
Notice: /Stage[main]/Main/File[/tmp/path]/ensure: removed
Notice: Applied catalog in 0.02 seconds

Upon further inspection, this tries to remove the top most directory first. That's actually good. It doesn't list all sub-resources and is probably more efficient. If it then needs to ensure a file within the directory it is actually idempotent: it'll fail because the parent doesn't exist, instead of creating the file and then removing the parent which would cause it fail on the next run.