Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(PUP-3399) autorequire local file sources #8909

Open
wants to merge 1 commit into
base: 7.x
Choose a base branch
from
Open

(PUP-3399) autorequire local file sources #8909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/puppet/type/file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,13 @@ def self.title_patterns
end
# if the resource is a link, make sure the target is created first
req << self[:target] if self[:target]
# if the resource has a source set, make sure it is created first
self[:source]&.each do |src|
req << src.delete_prefix('file://') if src.start_with?('file://')
evgeni marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5fea1dc

Copy link
Contributor

@joshcooper joshcooper Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@evgeni evgeni Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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)

Copy link
Contributor Author

@evgeni evgeni Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# the source gets translated to file:///D:/source instead of file://D:/source
# see https://github.com/puppetlabs/puppet/commit/5fea1dc64829e9c8178937764faccd51131b2a77
req << src.delete_prefix('file:///') if src.start_with?('file:///') && Puppet::Util::Platform.windows?
end
req
end

Expand Down
47 changes: 47 additions & 0 deletions spec/unit/type/file_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,6 +1291,53 @@
end
end

describe "source" do
it "should require file resource when specified with the source property" do
file = described_class.new(:path => File.expand_path("/foo"), :ensure => :file, :source => File.expand_path("/bar"))
source = described_class.new(:path => File.expand_path("/bar"), :ensure => :file)
catalog.add_resource file
catalog.add_resource source
reqs = file.autorequire
expect(reqs.size).to eq(1)
expect(reqs[0].source).to eq(source)
expect(reqs[0].target).to eq(file)
end

it "should require file resource when specified with the source property as file: URI" do
file = described_class.new(:path => File.expand_path("/foo"), :ensure => :file, :source => "file://#{File.expand_path("/bar")}")
source = described_class.new(:path => File.expand_path("/bar"), :ensure => :file)
catalog.add_resource file
catalog.add_resource source
reqs = file.autorequire
expect(reqs.size).to eq(1)
expect(reqs[0].source).to eq(source)
expect(reqs[0].target).to eq(file)
end

it "should require file resource when specified with the source property as an array" do
file = described_class.new(:path => File.expand_path("/foo"), :ensure => :file, :source => [File.expand_path("/bar")])
source = described_class.new(:path => File.expand_path("/bar"), :ensure => :file)
catalog.add_resource file
catalog.add_resource source
reqs = file.autorequire
expect(reqs.size).to eq(1)
expect(reqs[0].source).to eq(source)
expect(reqs[0].target).to eq(file)
end

it "should not require source if source is not local" do
file = described_class.new(:path => File.expand_path('/foo'), :ensure => :file, :source => 'puppet:///modules/nfs/conf')
catalog.add_resource file
expect(file.autorequire.size).to eq(0)
end

it "should not require source if source is not managed" do
evgeni marked this conversation as resolved.
Show resolved Hide resolved
file = described_class.new(:path => File.expand_path('/foo'), :ensure => :file, :source => File.expand_path('/bar'))
catalog.add_resource file
expect(file.autorequire.size).to eq(0)
end
end

describe "directories" do
it "should autorequire its parent directory" do
dir = described_class.new(:path => File.dirname(path))
Expand Down