From fb2ffd6879f4c885fe29f70e3cf9bcde89cdc8e9 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Mon, 25 Jul 2011 15:34:56 -0700 Subject: (#8596) Detect resource alias conflicts when titles do not match The introduction of composite namevars caused the resource title used in resource aliases to be set as an array, even when the resource only had one namevar. This would fail to conflict with non-alias entries in the resource table, which used a string for the title, even though the single element array contained the same string. Now, we flatten the key used in the resource table, so that single element arrays are represented as strings, and will properly conflict with resource titles. Paired-With: Jacob Helwig --- lib/puppet/resource/catalog.rb | 9 ++++-- spec/unit/resource/catalog_spec.rb | 61 +++++++++++++++++++++++++++++++++----- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb index 0a63ff172..2fdd19b0c 100644 --- a/lib/puppet/resource/catalog.rb +++ b/lib/puppet/resource/catalog.rb @@ -98,7 +98,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph resource.ref =~ /^(.+)\[/ class_name = $1 || resource.class.name - newref = [class_name, key] + newref = [class_name, key].flatten if key.is_a? String ref_string = "#{class_name}[#{key}]" @@ -111,7 +111,10 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph # isn't sufficient. if existing = @resource_table[newref] return if existing == resource - raise(ArgumentError, "Cannot alias #{resource.ref} to #{key.inspect}; resource #{newref.inspect} already exists") + resource_definition = " at #{resource.file}:#{resource.line}" if resource.file and resource.line + existing_definition = " at #{existing.file}:#{existing.line}" if existing.file and existing.line + msg = "Cannot alias #{resource.ref} to #{key.inspect}#{resource_definition}; resource #{newref.inspect} already defined#{existing_definition}" + raise ArgumentError, msg end @resource_table[newref] = resource @aliases[resource.ref] ||= [] @@ -377,7 +380,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph res = Puppet::Resource.new(nil, type) end title_key = [res.type, res.title.to_s] - uniqueness_key = [res.type, res.uniqueness_key] + uniqueness_key = [res.type, res.uniqueness_key].flatten @resource_table[title_key] || @resource_table[uniqueness_key] end diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb index a15a912ef..4600022b3 100755 --- a/spec/unit/resource/catalog_spec.rb +++ b/spec/unit/resource/catalog_spec.rb @@ -507,7 +507,7 @@ describe Puppet::Resource::Catalog, "when compiling" do lambda { @catalog.alias(@one, "other") }.should_not raise_error end - it "should create aliases for resources isomorphic resources whose names do not match their titles" do + it "should create aliases for isomorphic resources whose names do not match their titles" do resource = Puppet::Type::File.new(:title => "testing", :path => @basepath+"/something") @catalog.add_resource(resource) @@ -515,7 +515,7 @@ describe Puppet::Resource::Catalog, "when compiling" do @catalog.resource(:file, @basepath+"/something").should equal(resource) end - it "should not create aliases for resources non-isomorphic resources whose names do not match their titles" do + it "should not create aliases for non-isomorphic resources whose names do not match their titles" do resource = Puppet::Type.type(:exec).new(:title => "testing", :command => "echo", :path => %w{/bin /usr/bin /usr/local/bin}) @catalog.add_resource(resource) @@ -531,11 +531,6 @@ describe Puppet::Resource::Catalog, "when compiling" do @catalog.resource("notify", "other").should equal(@one) end - it "should ignore conflicting aliases that point to the aliased resource" do - @catalog.alias(@one, "other") - lambda { @catalog.alias(@one, "other") }.should_not raise_error - end - it "should fail to add an alias if the aliased name already exists" do @catalog.add_resource @one proc { @catalog.alias @two, "one" }.should raise_error(ArgumentError) @@ -589,6 +584,58 @@ describe Puppet::Resource::Catalog, "when compiling" do @catalog.create_resource :file, args @catalog.resource("File[/yay]").should equal(resource) end + + describe "when adding resources with multiple namevars" do + before :each do + Puppet::Type.newtype(:multiple) do + newparam(:color, :namevar => true) + newparam(:designation, :namevar => true) + + def self.title_patterns + [ [ + /^(\w+) (\w+)$/, + [ + [:color, lambda{|x| x}], + [:designation, lambda{|x| x}] + ] + ] ] + end + end + end + + it "should add an alias using the uniqueness key" do + @resource = Puppet::Type.type(:multiple).new(:title => "some resource", :color => "red", :designation => "5") + + @catalog.add_resource(@resource) + @catalog.resource(:multiple, "some resource").must == @resource + @catalog.resource("Multiple[some resource]").must == @resource + @catalog.resource("Multiple[red 5]").must == @resource + end + + it "should conflict with a resource with the same uniqueness key" do + @resource = Puppet::Type.type(:multiple).new(:title => "some resource", :color => "red", :designation => "5") + @other = Puppet::Type.type(:multiple).new(:title => "another resource", :color => "red", :designation => "5") + + @catalog.add_resource(@resource) + expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias Multiple\[another resource\] to \["red", "5"\].*resource \["Multiple", "red", "5"\] already defined/) + end + + it "should conflict when its uniqueness key matches another resource's title" do + @resource = Puppet::Type.type(:file).new(:title => "/tmp/foo") + @other = Puppet::Type.type(:file).new(:title => "another file", :path => "/tmp/foo") + + @catalog.add_resource(@resource) + expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias File\[another file\] to \["\/tmp\/foo"\].*resource \["File", "\/tmp\/foo"\] already defined/) + end + + it "should conflict when its uniqueness key matches the uniqueness key derived from another resource's title" do + @resource = Puppet::Type.type(:multiple).new(:title => "red leader") + @other = Puppet::Type.type(:multiple).new(:title => "another resource", :color => "red", :designation => "leader") + + @catalog.add_resource(@resource) + expect { @catalog.add_resource(@other) }.to raise_error(ArgumentError, /Cannot alias Multiple\[another resource\] to \["red", "leader"\].*resource \["Multiple", "red", "leader"\] already defined/) + end + end end describe "when applying" do -- cgit