From d5c12f04bbf50c79cbfc438d62ae263f39c383e8 Mon Sep 17 00:00:00 2001 From: Jeff McCune Date: Tue, 15 May 2012 13:16:36 -0700 Subject: [PATCH] (#14501) Fix autorequire case sensitivity Without this patch the behavior of a registry_value auto requiring parent registry_key resources does not work when the parent resources do not share the same letter case as the value. For example, the value fails to require the key in this scenario: registry_key { 'HKLM\Software\Vendor\PUPPETLABS': ensure => present, } registry_value { 'HKLM\Software\Vendor\puppetlabs\ValueString': data => "Hello World!", } The root cause of this problem is that Puppet treats all resource identifier keys as case sensitive and the registry types ascend their `path` attribute looking up resources using the string provided by the user in the path parameter. This patch fixes the problem by aliasing every registry_key and registry_value resource using the downcase version of the path attribute. Other resources are expected to look up related resources in the catalog using the downcase version of the path parameter. This solves the problem because we now have an internally consistent, case insensitive identifier. We preserve case for the user by not munging the case of the path parameter itself. Instead we simply alias the downcase version in the catalog. --- .../registry/should_tolerate_mixed_case.rb | 161 ++++++++++++++++++ lib/puppet/type/registry_key.rb | 14 +- lib/puppet/type/registry_value.rb | 14 +- spec/unit/puppet/type/registry_key_spec.rb | 42 ++++- 4 files changed, 223 insertions(+), 8 deletions(-) create mode 100644 acceptance/tests/resource/registry/should_tolerate_mixed_case.rb diff --git a/acceptance/tests/resource/registry/should_tolerate_mixed_case.rb b/acceptance/tests/resource/registry/should_tolerate_mixed_case.rb new file mode 100644 index 00000000..4bdd472b --- /dev/null +++ b/acceptance/tests/resource/registry/should_tolerate_mixed_case.rb @@ -0,0 +1,161 @@ +require 'pathname' +require Pathname.new(__FILE__).dirname.dirname.dirname.dirname + 'lib/systest/util/registry' +# Include our utility methods in the singleton class of the test case instance. +class << self + include Systest::Util::Registry +end + +test_name "Registry Value Management (Mixed Case)" + +# JJM - The whole purpose of this test is to exercise situations where the user +# specifies registry valies in a case-insensitive but case-preserving way. +# This has particular "gotchas" with regard to autorequire. +# +# Note how SUBKEY1 is capitalized in some resources but downcased in other +# resources. On windows these refer to the same thing, and case is preserved. +# In puppet, however, resource namevars are case sensisitve unless otherwise +# noted. + +# Generate a unique key name +keyname = "PuppetLabsTest_MixedCase_#{randomstring(8)}" +# This is the keypath we'll use for this entire test. We will actually create this key and delete it. +vendor_path = "HKLM\\Software\\Vendor" +keypath = "#{vendor_path}\\#{keyname}" + +master_manifest_content = < "fact_phase: $fact_phase" } + +registry_key { '#{vendor_path}': ensure => present } +registry_key { '32:#{vendor_path}': ensure => present } + +class phase1 { + Registry_key { ensure => present } + registry_key { '#{keypath}': } + registry_key { '#{keypath}\\SUBKEY1': } + # NOTE THE DIFFERENCE IN CASE IN SubKey1 and SUBKEY1 above + registry_key { '#{keypath}\\SubKey1\\SUBKEY2': } + registry_key { '32:#{keypath}': } + registry_key { '32:#{keypath}\\SUBKEY1': } + registry_key { '32:#{keypath}\\SubKey1\\SUBKEY2': } + + # The Default Value + # NOTE THE DIFFERENCE IN CASE IN SubKey1 and SUBKEY1 above + registry_value { '#{keypath}\\SubKey1\\\\': + data => "Default Data", + } + + # String Values + registry_value { '#{keypath}\\SubKey1\\ValueString1': + data => "Should be a string", + } + registry_value { '#{keypath}\\SubKey1\\ValueString2': + type => string, + data => "Should be a string", + } + registry_value { '#{keypath}\\SubKey1\\ValueString3': + ensure => present, + type => string, + data => "Should be a string", + } + registry_value { '#{keypath}\\SubKey1\\ValueString4': + data => "Should be a string", + type => string, + ensure => present, + } + registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString1': + data => "Should be a string", + } + registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString2': + type => string, + data => "Should be a string", + } + registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString3': + ensure => present, + type => string, + data => "Should be a string", + } + registry_value { '#{keypath}\\SubKey1\\SubKey2\\ValueString4': + data => "Should be a string", + type => string, + ensure => present, + } + + # The Default Value + # NOTE THE DIFFERENCE IN CASE IN SubKey1 and SUBKEY1 above + registry_value { '32:#{keypath}\\SubKey1\\\\': + data => "Default Data", + } + + # String Values + registry_value { '32:#{keypath}\\SubKey1\\ValueString1': + data => "Should be a string", + } + registry_value { '32:#{keypath}\\SubKey1\\ValueString2': + type => string, + data => "Should be a string", + } + registry_value { '32:#{keypath}\\SubKey1\\ValueString3': + ensure => present, + type => string, + data => "Should be a string", + } + registry_value { '32:#{keypath}\\SubKey1\\ValueString4': + data => "Should be a string", + type => string, + ensure => present, + } + registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString1': + data => "Should be a string", + } + registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString2': + type => string, + data => "Should be a string", + } + registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString3': + ensure => present, + type => string, + data => "Should be a string", + } + registry_value { '32:#{keypath}\\SubKey1\\SubKey2\\ValueString4': + data => "Should be a string", + type => string, + ensure => present, + } +} + +case $fact_phase { + default: { include phase1 } +} +HERE + +# Setup the master to use the modules specified in the --modules option +setup_master master_manifest_content + +step "Start the master" do + with_master_running_on(master, master_options) do + # A set of keys we expect Puppet to create + phase1_resources_created = [ + /Registry_key\[HKLM.Software.Vendor.PuppetLabsTest\w+\].ensure: created/, + /Registry_key\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SUBKEY1\].ensure: created/, + /Registry_key\[32:HKLM.Software.Vendor.PuppetLabsTest\w+\].ensure: created/, + /Registry_key\[32:HKLM.Software.Vendor.PuppetLabsTest\w+\\SUBKEY1\].ensure: created/, + /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\\\\].ensure: created/, + /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString1\].ensure: created/, + /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString2\].ensure: created/, + /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString3\].ensure: created/, + /Registry_value\[HKLM.Software.Vendor.PuppetLabsTest\w+\\SubKey1\\ValueString4\].ensure: created/, + ] + + windows_agents.each do |agent| + this_agent_args = agent_args % get_test_file_path(agent, agent_lib_dir) + + step "Registry Tolerate Mixed Case Values - Phase 1.a - Create some values" + on agent, puppet_agent(this_agent_args, :environment => { 'FACTER_FACT_PHASE' => '1' }), :acceptable_exit_codes => agent_exit_codes do + phase1_resources_created.each do |val_re| + assert_match(val_re, result.stdout, "Expected output to contain #{val_re.inspect}.") + end + assert_no_match(/err:/, result.stdout, "Expected no error messages.") + end + end + end +end diff --git a/lib/puppet/type/registry_key.rb b/lib/puppet/type/registry_key.rb index 2f1455b1..a5711ec0 100644 --- a/lib/puppet/type/registry_key.rb +++ b/lib/puppet/type/registry_key.rb @@ -20,7 +20,12 @@ def self.title_patterns Puppet::Modules::Registry::RegistryKeyPath.new(path).valid? end munge do |path| - Puppet::Modules::Registry::RegistryKeyPath.new(path).canonical + canonical = Puppet::Modules::Registry::RegistryKeyPath.new(path).canonical + # Windows is case insensitive and case preserving. We deal with this by + # aliasing resources to their downcase values. This is inspired by the + # munge block in the alias metaparameter. + @resource.catalog.alias(@resource, canonical.downcase) if @resource.catalog.respond_to? :alias + canonical end end @@ -59,8 +64,11 @@ def self.title_patterns autorequire(:registry_key) do req = [] path = Puppet::Modules::Registry::RegistryKeyPath.new(value(:path)) - if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s) } - req << found.to_s + # It is important to match against the downcase value of the path because + # other resources are expected to alias themselves to the downcase value so + # that we respect the case insensitive and preserving nature of Windows. + if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s.downcase) } + req << found.to_s.downcase end req end diff --git a/lib/puppet/type/registry_value.rb b/lib/puppet/type/registry_value.rb index 0533e839..ed67570d 100644 --- a/lib/puppet/type/registry_value.rb +++ b/lib/puppet/type/registry_value.rb @@ -20,7 +20,12 @@ def self.title_patterns Puppet::Modules::Registry::RegistryValuePath.new(path).valid? end munge do |path| - Puppet::Modules::Registry::RegistryValuePath.new(path).canonical + canonical = Puppet::Modules::Registry::RegistryValuePath.new(path).canonical + # Windows is case insensitive and case preserving. We deal with this by + # aliasing resources to their downcase values. This is inspired by the + # munge block in the alias metaparameter. + @resource.catalog.alias(@resource, canonical.downcase) if @resource.catalog.respond_to? :alias + canonical end end @@ -97,8 +102,11 @@ def change_to_s(currentvalue, newvalue) autorequire(:registry_key) do req = [] path = Puppet::Modules::Registry::RegistryKeyPath.new(value(:path)) - if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s) } - req << found.to_s + # It is important to match against the downcase value of the path because + # other resources are expected to alias themselves to the downcase value so + # that we respect the case insensitive and preserving nature of Windows. + if found = path.enum_for(:ascend).find { |p| catalog.resource(:registry_key, p.to_s.downcase) } + req << found.to_s.downcase end req end diff --git a/spec/unit/puppet/type/registry_key_spec.rb b/spec/unit/puppet/type/registry_key_spec.rb index 296c4581..ea9b11d3 100644 --- a/spec/unit/puppet/type/registry_key_spec.rb +++ b/spec/unit/puppet/type/registry_key_spec.rb @@ -66,11 +66,17 @@ context "purging" do let (:catalog) do Puppet::Resource::Catalog.new end + # This is overridden here so we get a consistent association with the key + # and a catalog using memoized let methods. + let (:key) do + Puppet::Type.type(:registry_key).new(:name => 'HKLM\Software', :catalog => catalog) + end + before :each do key[:purge_values] = true catalog.add_resource(key) - catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val1")) - catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val2")) + catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val1", :catalog => catalog)) + catalog.add_resource(Puppet::Type.type(:registry_value).new(:path => "#{key[:path]}\\val2", :catalog => catalog)) end it "should return an empty array if the key doesn't have any values" do @@ -92,4 +98,36 @@ end end end + + describe "#autorequire" do + let :the_catalog do + Puppet::Resource::Catalog.new + end + + let(:the_resource_name) { 'HKLM\Software\Vendor\PuppetLabs' } + + let :the_resource do + # JJM Holy cow this is an intertangled mess. ;) + resource = Puppet::Type.type(:registry_key).new(:name => the_resource_name, :catalog => the_catalog) + the_catalog.add_resource resource + resource + end + + it 'Should initialize the catalog instance variable' do + the_resource.catalog.must be the_catalog + end + + it 'Should allow case insensitive lookup using the downcase path' do + the_resource.must be the_catalog.resource(:registry_key, the_resource_name.downcase) + end + + it 'Should preserve the case of the user specified path' do + the_resource.must be the_catalog.resource(:registry_key, the_resource_name) + end + + it 'Should return the same resource regardless of the alias used' do + the_resource.must be the_catalog.resource(:registry_key, the_resource_name) + the_resource.must be the_catalog.resource(:registry_key, the_resource_name.downcase) + end + end end