[PATCH/puppet 1/1] Fix for ticket #2844 (file recursion generated vs. explicit prefix)

[Markus Roberts]

The routine which was determining if one path was a prefix of another
in arbitrating between explicit and generated resources was using the
raw string for the test without regard to path segments and thus could
be fooled by pairs such as "/tmp/foo" vs. "/tmp/foo2"

Fix was to be path delimiter aware and add a test.

Signed-off-by: Markus Roberts <Mar...@reality.com>
---
lib/puppet/type/file.rb | 26 +++++++++-----------------
spec/integration/type/file.rb | 21 ++++++++++++++++++++-
2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index f920525..a4666a9 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -496,26 +496,18 @@ module Puppet
# not likely to have many actual conflicts, which is good, because
# this is a pretty inefficient implementation.
def remove_less_specific_files(files)
- # We sort the paths so we can short-circuit some tests.
- mypath = self[:path]
- other_paths = catalog.vertices.find_all do |r|
- r.is_a?(self.class) and r[:path][0..(mypath.length - 1)] == mypath
- end.collect { |r| r[:path] }.sort { |a, b| a.length <=> b.length } - [self[:path]]
+ mypath = self[:path].split(File::Separator)
+ other_paths = catalog.vertices.
+ select { |r| r.is_a?(self.class) and r[:path] != self[:path] }.
+ collect { |r| r[:path].split(File::Separator) }.
+ select { |p| p[0,mypath.length] == mypath }

return files if other_paths.empty?

- remove = []
- files.each do |file|
- path = file[:path]
- other_paths.each do |p|
- if path[0..(p.length - 1)] == p
- remove << file
- break
- end
- end
- end
-
- files - remove
+ files.reject { |file|
+ path = file[:path].split(File::Separator)
+ other_paths.any? { |p| path[0,p.length] == p }
+ }
end

# A simple method for determining whether we should be recursing.
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index 40f9244..fe6e2dd 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -223,7 +223,7 @@ describe Puppet::Type.type(:file) do
end

it "should not recursively manage files managed by a more specific explicit file" do
- dir = tmpfile("file_source_integration_source")
+ dir = tmpfile("recursion_vs_explicit_1")

subdir = File.join(dir, "subdir")
file = File.join(subdir, "file")
@@ -242,6 +242,25 @@ describe Puppet::Type.type(:file) do

(File.stat(file).mode & 007777).should == 0644
end
+
+ it "should recursively manage files even if there is an explicit file whose name is a prefix of the managed file" do
+ dir = tmpfile("recursion_vs_explicit_2")
+
+ managed = File.join(dir, "file")
+ generated = File.join(dir, "file_with_a_name_starting_with_the_word_file")
+
+ FileUtils.mkdir_p(dir)
+ File.open(managed, "w") { |f| f.puts "" }
+ File.open(generated, "w") { |f| f.puts "" }
+
+ @catalog = Puppet::Resource::Catalog.new
+ @catalog.add_resource Puppet::Type::File.new(:name => dir, :recurse => true, :backup => false, :mode => "755")
+ @catalog.add_resource Puppet::Type::File.new(:name => managed, :recurse => true, :backup => false, :mode => "644")
+
+ @catalog.apply
+
+ (File.stat(generated).mode & 007777).should == 0755
+ end
end

describe "when generating resources" do
--
1.6.4

[Thomas Bellman]

Markus Roberts wrote:

> The routine which was determining if one path was a prefix of another
> in arbitrating between explicit and generated resources was using the
> raw string for the test without regard to path segments and thus could
> be fooled by pairs such as "/tmp/foo" vs. "/tmp/foo2"
>
> Fix was to be path delimiter aware and add a test.

Applying this patch on top of 0.25.1 does seem to solve my problem.

By the way, an alternative way of solving it could be, instead of
splitting on / and comparing arrays, to add a / to the end of the
paths to compare. I.e., do something like this:

def remove_less_specific_files(files)
mypath = self[:path] + File::Separator
other_paths = catalog.vertices.

select { |r| r.is_a?(self.class) and r[:path] != self[:path] }.

collect { |r| r[:path] + File::Separator }.

select { |p| p[0,mypath.length] == mypath }

return files if other_paths.empty?

files.reject { |file|
path = file[:path] + File::Separator

other_paths.any? { |p| path[0,p.length] == p }
}

end

The above seems to work for at least my problem, but I haven't
tested it on any other test cases. And I don't know which is
more beautiful, or faster, or otherwise better. Just thought
I'd throw out the idea.

(And I suppose we don't care about portability to OS:es like
TOPS-20 and VMS, where paths aren't quite as simple as in Unix,
MacOS and MS-DOS. :-)


/Bellman

[Luke Kanies]

Man I hate that 'period at the end of a line' idiom, but +1.

> --
>
> You received this message because you are subscribed to the Google
> Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com
> .
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=
> .
>
>


--
I am a kind of paranoiac in reverse. I suspect people of plotting
to make me happy. --J. D. Salinger
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com