Please review pull request #511: (#11408) Plugin sync is broken on Windows opened by (joshcooper)
Description:
This fixes plugin sync on Windows agents from remote POSIX puppet masters. While fixing this the existing pluginsync acceptance test revealed a related issue in how the default owner and group are assigned to newly downloaded files. Fixed that and added an acceptance test to verify default mode behavior, e.g. readable directories get execute bit, files do not.
- Opened: Fri Feb 17 01:05:31 UTC 2012
- Based on: puppetlabs:2.7.x (e3f1769420a9183107e8ca34cc0a5e11e1ef59e2)
- Requested merge: joshcooper:ticket/2.7.x/11408-remote-recursion (86f2876980e59d1b3acc32294f1da3bf861e7e2e)
Diff follows:
diff --git a/acceptance/tests/resource/file/should_default_mode.rb b/acceptance/tests/resource/file/should_default_mode.rb
new file mode 100644
index 0000000..ac58159
--- /dev/null
+++ b/acceptance/tests/resource/file/should_default_mode.rb
@@ -0,0 +1,42 @@
+test_name "file resource: set default modes"
+
+def regexp_mode(mode)
+ Regexp.new("mode\s*=>\s*'0?#{mode}'")
+end
+
+agents.each do |agent|
+ step "setup"
+ parent = agent.tmpdir('default-mode-parent')
+ on(agent, "rm -rf #{parent}")
+
+ step "puppet should set execute bit on readable directories"
+ on(agent, puppet_resource("file", parent, "ensure=directory", "mode=0644")) do
+ assert_match(regexp_mode(755), stdout)
+ end
+
+ step "include execute bit on newly created directories"
+ dir = "#{parent}/dir"
+ on(agent, "mkdir #{dir} && cd #{dir} && cd ..")
+
+ step "exclude execute bit from newly created files"
+ file = "#{parent}/file.txt"
+ on(agent, "echo foobar > #{file}")
+ on(agent, "#{file}", :acceptable_exit_codes => (1..255)) do
+ assert_no_match(/foobar/, stdout)
+ end
+
+ step "set execute git on file if explicitly specified"
+ file_750 = "#{parent}/file_750.txt"
+ on(agent, puppet_resource("file", file_750, "ensure=file", "mode=0750")) do
+ assert_match(regexp_mode(750), stdout)
+ end
+
+ step "don't set execute bit if directory not readable"
+ dir_600 = "#{parent}/dir_600"
+ on(agent, puppet_resource("file", dir_600, "ensure=directory", "mode=0600")) do
+ assert_match(regexp_mode(700), stdout) # readable by owner, but not group
+ end
+
+ on(agent, "rm -rf #{parent}")
+end
+
diff --git a/lib/puppet/configurer/downloader.rb b/lib/puppet/configurer/downloader.rb
index 54075ee..2e0fd30 100644
--- a/lib/puppet/configurer/downloader.rb
+++ b/lib/puppet/configurer/downloader.rb
@@ -71,12 +71,16 @@ def default_arguments
:recurse => true,
:source => source,
:tag => name,
- :owner => Puppet.features.microsoft_windows? ? Sys::Admin.get_login : Process.uid,
- :group => Puppet.features.microsoft_windows? ? 'S-1-0-0' : Process.gid,
:purge => true,
:force => true,
:backup => false,
:noop => false
- }
+ }.merge(
+ Puppet.features.microsoft_windows? ? {} :
+ {
+ :owner => Process.uid,
+ :group => Process.gid
+ }
+ )
end
end
diff --git a/lib/puppet/file_serving/base.rb b/lib/puppet/file_serving/base.rb
index b14c51c..8564cff 100644
--- a/lib/puppet/file_serving/base.rb
+++ b/lib/puppet/file_serving/base.rb
@@ -50,10 +50,7 @@ def links=(value)
# Set our base path.
attr_reader :path
def path=(path)
- unless Puppet::Util.absolute_path?(path)
- raise ArgumentError.new("Paths must be fully qualified")
- end
-
+ raise ArgumentError.new("Paths must be fully qualified") unless Puppet::FileServing::Base.absolute?(path)
@path = path
end
@@ -61,7 +58,7 @@ def path=(path)
# the file's path relative to the initial recursion point.
attr_reader :relative_path
def relative_path=(path)
- raise ArgumentError.new("Relative paths must not be fully qualified") if Puppet::Util.absolute_path?(path)
+ raise ArgumentError.new("Relative paths must not be fully qualified") if Puppet::FileServing::Base.absolute?(path)
@relative_path = path
end
@@ -85,4 +82,7 @@ def to_pson_data_hash
}
end
+ def self.absolute?(path)
+ Puppet::Util.absolute_path?(path, :posix) or (Puppet.features.microsoft_windows? and Puppet::Util.absolute_path?(path, :windows))
+ end
end
diff --git a/lib/puppet/util/windows/security.rb b/lib/puppet/util/windows/security.rb
index 368ec50..9b7e107 100644
--- a/lib/puppet/util/windows/security.rb
+++ b/lib/puppet/util/windows/security.rb
@@ -369,13 +369,15 @@ def set_mode(mode, path, protected = true)
#puts "ace: nobody #{well_known_nobody_sid}, mask 0x#{nobody_allow.to_s(16)}"
add_access_allowed_ace(acl, nobody_allow, well_known_nobody_sid)
- # add inheritable aces for child dirs and files that are created within the dir
+ # add inherit-only aces for child dirs and files that are created within the dir
if isdir
- inherit = INHERIT_ONLY_ACE | OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE
-
+ inherit = INHERIT_ONLY_ACE | CONTAINER_INHERIT_ACE
add_access_allowed_ace(acl, owner_allow, Win32::Security::SID::CreatorOwner, inherit)
add_access_allowed_ace(acl, group_allow, Win32::Security::SID::CreatorGroup, inherit)
- add_access_allowed_ace(acl, other_allow, well_known_world_sid, inherit)
+
+ inherit = INHERIT_ONLY_ACE | OBJECT_INHERIT_ACE
+ add_access_allowed_ace(acl, owner_allow & ~FILE_EXECUTE, Win32::Security::SID::CreatorOwner, inherit)
+ add_access_allowed_ace(acl, group_allow & ~FILE_EXECUTE, Win32::Security::SID::CreatorGroup, inherit)
end
end
diff --git a/spec/unit/configurer/downloader_spec.rb b/spec/unit/configurer/downloader_spec.rb
index 69e5e6b..439a25f 100755
--- a/spec/unit/configurer/downloader_spec.rb
+++ b/spec/unit/configurer/downloader_spec.rb
@@ -79,14 +79,13 @@
end
describe "on Windows", :if => Puppet.features.microsoft_windows? do
- it "should always set the owner to the current user" do
- Sys::Admin.expects(:get_login).returns 'foo'
- Puppet::Type.type(:file).expects(:new).with { |opts| opts[:owner] == 'foo' }
+ it "should omit the owner" do
+ Puppet::Type.type(:file).expects(:new).with { |opts| opts[:owner] == nil }
@dler.file
end
- it "should always set the group to nobody" do
- Puppet::Type.type(:file).expects(:new).with { |opts| opts[:group] == 'S-1-0-0' }
+ it "should omit the group" do
+ Puppet::Type.type(:file).expects(:new).with { |opts| opts[:group] == nil }
@dler.file
end
end
diff --git a/spec/unit/file_serving/base_spec.rb b/spec/unit/file_serving/base_spec.rb
index 91e6962..cea22da 100755
--- a/spec/unit/file_serving/base_spec.rb
+++ b/spec/unit/file_serving/base_spec.rb
@@ -127,4 +127,23 @@
file.stat
end
end
+
+ describe "#absolute?" do
+ it "should be accept POSIX paths" do
+ Puppet::FileServing::Base.should be_absolute('/')
+ end
+
+ it "should accept Windows paths on Windows" do
+ Puppet.features.stubs(:microsoft_windows?).returns(true)
+ Puppet.features.stubs(:posix?).returns(false)
+
+ Puppet::FileServing::Base.should be_absolute('c:/foo')
+ end
+
+ it "should reject Windows paths on POSIX" do
+ Puppet.features.stubs(:microsoft_windows?).returns(false)
+
+ Puppet::FileServing::Base.should_not be_absolute('c:/foo')
+ end
+ end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
