(PUP-5704) allow array commands in exec resource

acceptance/tests/resource/exec/accept_array_commands.rb

@@ -0,0 +1,23 @@
+test_name "Be able to execute array commands" do
+  tag 'audit:high',
+      'audit:acceptance'
+
+  agents.each do |agent|
+    if agent.platform =~ /windows/
+      cmd = ['C:\Windows\System32\cmd.exe', '/c', 'echo', '*']
+    else
+      cmd = ['/bin/echo', '*']
+    end
+
+    exec_manifest = <<~MANIFEST
+      exec { "test exec":
+        command => #{cmd},
+        logoutput => true,
+      }
+    MANIFEST
+
+    apply_manifest_on(agent, exec_manifest) do |output|
+      assert_match('Notice: /Stage[main]/Main/Exec[test exec]/returns: *', output.stdout)
+    end
+  end
+end

lib/puppet/provider/exec/posix.rb

@@ -6,10 +6,22 @@
   defaultfor :feature => :posix
 
   desc <<-EOT
-    Executes external binaries directly, without passing through a shell or
-    performing any interpolation. This is a safer and more predictable way
-    to execute most commands, but prevents the use of globbing and shell
-    built-ins (including control logic like "for" and "if" statements).
+    Executes external binaries by invoking Ruby's `Kernel.exec`.
+    When the command is a string, it will be executed directly,
+    without a shell, if it follows these rules:
+     - no meta characters
+     - no shell reserved word and no special built-in
+
+    When the command is an Array of Strings, passed as `[cmdname, arg1, ...]`
+    it will be executed directly(the first element is taken as a command name
+    and the rest are passed as parameters to command with no shell expansion)
+    This is a safer and more predictable way to execute most commands,
+    but prevents the use of globbing and shell built-ins (including control
+    logic like "for" and "if" statements).
+
+    If the use of globbing and shell built-ins is desired, please check
+    the `shell` provider
+
   EOT
 
   # Verify that we have the executable

lib/puppet/type/exec.rb

@@ -201,7 +201,9 @@ def sync
         only uses the resource title to ensure `exec`s are unique."
 
       validate do |command|
-        raise ArgumentError, _("Command must be a String, got value of class %{klass}") % { klass: command.class } unless command.is_a? String
+        unless command.is_a?(String) || command.is_a?(Array)
+          raise ArgumentError, _("Command must be a String or Array<String>, got value of class %{klass}") % { klass: command.class }
+        end
       end
     end
 
@@ -458,6 +460,10 @@ def check(value)
 
             unless => ['test -f /tmp/file1', 'test -f /tmp/file2'],
 
+        or an array of arrays. For example:
+
+            unless => [['test', '-f', '/tmp/file1'], 'test -f /tmp/file2']
+
         This `exec` would only run if every command in the array has a
         non-zero exit code.
       EOT
@@ -514,6 +520,10 @@ def check(value)
 
             onlyif => ['test -f /tmp/file1', 'test -f /tmp/file2'],
 
+        or an array of arrays. For example:
+
+            onlyif => [['test', '-f', '/tmp/file1'], 'test -f /tmp/file2']
+
         This `exec` would only run if every command in the array has an
         exit code of 0 (success).
       EOT
@@ -562,12 +572,14 @@ def check(value)
       reqs << self[:cwd] if self[:cwd]
 
       file_regex = Puppet::Util::Platform.windows? ? %r{^([a-zA-Z]:[\\/]\S+)} : %r{^(/\S+)}
+      cmd = self[:command]
+      cmd = cmd[0] if cmd.is_a? Array
 
-      self[:command].scan(file_regex) { |str|
+      cmd.scan(file_regex) { |str|
         reqs << str
       }
 
-      self[:command].scan(/^"([^"]+)"/) { |str|
+      cmd.scan(/^"([^"]+)"/) { |str|
         reqs << str
       }
 
@@ -583,6 +595,7 @@ def check(value)
           # fully qualified.  It might not be a bad idea to add
           # unqualified files, but, well, that's a bit more annoying
           # to do.
+          line = line[0] if line.is_a? Array
           reqs += line.scan(file_regex)
         end
       }

spec/integration/type/exec_spec.rb

@@ -7,70 +7,95 @@
 
   let(:catalog) { Puppet::Resource::Catalog.new }
   let(:path) { tmpfile('exec_provider') }
-  let(:command) { "ruby -e 'File.open(\"#{path}\", \"w\") { |f| f.print \"foo\" }'" }
 
   before :each do
     catalog.host_config = false
   end
 
-  it "should execute the command" do
-    exec = described_class.new :command => command, :path => ENV['PATH']
+  shared_examples_for 'a valid exec resource' do
+    it "should execute the command" do
+      exec = described_class.new :command => command, :path => ENV['PATH']
 
-    catalog.add_resource exec
-    catalog.apply
+      catalog.add_resource exec
+      catalog.apply
 
-    expect(File.read(path)).to eq('foo')
-  end
+      expect(File.read(path)).to eq('foo')
+    end
 
-  it "should not execute the command if onlyif returns non-zero" do
-    exec = described_class.new(
-      :command => command,
-      :onlyif => "ruby -e 'exit 44'",
-      :path => ENV['PATH']
-    )
+    it "should not execute the command if onlyif returns non-zero" do
+      exec = described_class.new(
+        :command => command,
+        :onlyif => "ruby -e 'exit 44'",
+        :path => ENV['PATH']
+      )
 
-    catalog.add_resource exec
-    catalog.apply
+      catalog.add_resource exec
+      catalog.apply
 
-    expect(Puppet::FileSystem.exist?(path)).to be_falsey
-  end
+      expect(Puppet::FileSystem.exist?(path)).to be_falsey
+    end
 
-  it "should execute the command if onlyif returns zero" do
-    exec = described_class.new(
-      :command => command,
-      :onlyif => "ruby -e 'exit 0'",
-      :path => ENV['PATH']
-    )
+    it "should execute the command if onlyif returns zero" do
+      exec = described_class.new(
+        :command => command,
+        :onlyif => "ruby -e 'exit 0'",
+        :path => ENV['PATH']
+      )
 
-    catalog.add_resource exec
-    catalog.apply
+      catalog.add_resource exec
+      catalog.apply
 
-    expect(File.read(path)).to eq('foo')
-  end
+      expect(File.read(path)).to eq('foo')
+    end
+
+    it "should execute the command if unless returns non-zero" do
+      exec = described_class.new(
+        :command => command,
+        :unless => "ruby -e 'exit 45'",
+        :path => ENV['PATH']
+      )
+
+      catalog.add_resource exec
+      catalog.apply
+
+      expect(File.read(path)).to eq('foo')
+    end
 
-  it "should execute the command if unless returns non-zero" do
-    exec = described_class.new(
-      :command => command,
-      :unless => "ruby -e 'exit 45'",
-      :path => ENV['PATH']
-    )
+    it "should not execute the command if unless returns zero" do
+      exec = described_class.new(
+        :command => command,
+        :unless => "ruby -e 'exit 0'",
+        :path => ENV['PATH']
+      )
 
-    catalog.add_resource exec
-    catalog.apply
+      catalog.add_resource exec
+      catalog.apply
 
-    expect(File.read(path)).to eq('foo')
+      expect(Puppet::FileSystem.exist?(path)).to be_falsey
+    end
   end
 
-  it "should not execute the command if unless returns zero" do
-    exec = described_class.new(
-      :command => command,
-      :unless => "ruby -e 'exit 0'",
-      :path => ENV['PATH']
-    )
+  context 'when command is a string' do
+    let(:command) { "ruby -e 'File.open(\"#{path}\", \"w\") { |f| f.print \"foo\" }'" }
+
+    it_behaves_like 'a valid exec resource'
+  end
+
+  context 'when command is an array' do
+    let(:command) { ['ruby', '-e', "File.open(\"#{path}\", \"w\") { |f| f.print \"foo\" }"] }
+
+    it_behaves_like 'a valid exec resource'
+
+    context 'when is invalid' do
+      let(:command) { [ "ruby -e 'puts 1'" ] }
 
-    catalog.add_resource exec
-    catalog.apply
+      it 'logs error' do
+        exec = described_class.new :command => command, :path => ENV['PATH']
+        catalog.add_resource exec
+        logs = catalog.apply.report.logs
 
-    expect(Puppet::FileSystem.exist?(path)).to be_falsey
+        expect(logs[0].message).to eql("Could not find command 'ruby -e 'puts 1''")
+      end
+    end
   end
 end

spec/unit/type/exec_spec.rb

@@ -239,6 +239,19 @@ def exec_stub(options = {})
     expect(dependencies.collect(&:to_s)).to eq([Puppet::Relationship.new(tmp, execer).to_s])
   end
 
+  it "should be able to autorequire files mentioned in the array command" do
+    foo = make_absolute('/bin/foo')
+    catalog = Puppet::Resource::Catalog.new
+    tmp = Puppet::Type.type(:file).new(:name => foo)
+    execer = Puppet::Type.type(:exec).new(:name => 'test array', :command => [foo, 'bar'])
+
+    catalog.add_resource tmp
+    catalog.add_resource execer
+    dependencies = execer.autorequire(catalog)
+
+    expect(dependencies.collect(&:to_s)).to eq([Puppet::Relationship.new(tmp, execer).to_s])
+  end
+
   describe "when handling the path parameter" do
     expect = %w{one two three four}
     { "an array"                                      => expect,
@@ -346,7 +359,13 @@ def instance(path)
   end
 
   shared_examples_for "all exec command parameters" do |param|
-    { "relative" => "example", "absolute" => "/bin/example" }.sort.each do |name, command|
+    array_cmd = ["/bin/example", "*"]
+    array_cmd = [["/bin/example", "*"]] if [:onlyif, :unless].include?(param)
+
+    commands = { "relative" => "example", "absolute" => "/bin/example" }
+    commands["array"] = array_cmd
+
+    commands.sort.each do |name, command|
       describe "if command is #{name}" do
         before :each do
           @param = param
@@ -379,45 +398,44 @@ def test(command, valid)
   end
 
   shared_examples_for "all exec command parameters that take arrays" do |param|
-    describe "when given an array of inputs" do
-      before :each do
-        @test = Puppet::Type.type(:exec).new(:name => @executable)
-      end
+    [
+      %w{one two three},
+      [%w{one -a}, %w{two, -b}, 'three']
+    ].each do |input|
+      context "when given #{input.inspect} as input" do
+        let(:resource) { Puppet::Type.type(:exec).new(:name => @executable) }
 
-      it "should accept the array when all commands return valid" do
-        input = %w{one two three}
-        expect(@test.provider).to receive(:validatecmd).exactly(input.length).times.and_return(true)
-        @test[param] = input
-        expect(@test[param]).to eq(input)
-      end
+        it "accepts the array when all commands return valid" do
+          input = %w{one two three}
+          allow(resource.provider).to receive(:validatecmd).exactly(input.length).times.and_return(true)
+          resource[param] = input
+          expect(resource[param]).to eq(input)
+        end
 
-      it "should reject the array when any commands return invalid" do
-        input = %w{one two three}
-        expect(@test.provider).to receive(:validatecmd).with(input.first).and_return(false)
-        input[1..-1].each do |cmd|
-          expect(@test.provider).to receive(:validatecmd).with(cmd).and_return(true)
+        it "rejects the array when any commands return invalid" do
+          input = %w{one two three}
+          allow(resource.provider).to receive(:validatecmd).with(input[0]).and_return(true)
+          allow(resource.provider).to receive(:validatecmd).with(input[1]).and_raise(Puppet::Error)
+
+          expect { resource[param] = input }.to raise_error(Puppet::ResourceError, /Parameter #{param} failed/)
         end
-        @test[param] = input
-        expect(@test[param]).to eq(input)
-      end
 
-      it "should reject the array when all commands return invalid" do
-        input = %w{one two three}
-        expect(@test.provider).to receive(:validatecmd).exactly(input.length).times.and_return(false)
-        @test[param] = input
-        expect(@test[param]).to eq(input)
+        it "stops at the first invalid command" do
+          input = %w{one two three}
+          allow(resource.provider).to receive(:validatecmd).with(input[0]).and_raise(Puppet::Error)
+
+          expect(resource.provider).not_to receive(:validatecmd).with(input[1])
+          expect(resource.provider).not_to receive(:validatecmd).with(input[2])
+          expect { resource[param] = input }.to raise_error(Puppet::ResourceError, /Parameter #{param} failed/)
+        end
       end
     end
   end
 
   describe "when setting command" do
     subject { described_class.new(:name => @command) }
-    it "fails when passed an Array" do
-      expect { subject[:command] = [] }.to raise_error Puppet::Error, /Command must be a String/
-    end
-
     it "fails when passed a Hash" do
-      expect { subject[:command] = {} }.to raise_error Puppet::Error, /Command must be a String/
+      expect { subject[:command] = {} }.to raise_error Puppet::Error, /Command must be a String or Array<String>/
     end
   end
 
@@ -759,6 +777,35 @@ def instance(path)
           end
         end
 
+        context 'with an array of arrays with multiple items' do
+          before do
+            [true, false].each do |check|
+              allow(@test.provider).to receive(:run).with([@pass, '--flag'], check).
+                and_return(['test output', @pass_status])
+              allow(@test.provider).to receive(:run).with([@fail, '--flag'], check).
+                and_return(['test output', @fail_status])
+              allow(@test.provider).to receive(:run).with([@pass], check).
+                and_return(['test output', @pass_status])
+              allow(@test.provider).to receive(:run).with([@fail], check).
+                and_return(['test output', @fail_status])
+            end
+          end
+          it "runs if all the commands exits non-zero" do
+            @test[param] = [[@fail, '--flag'], [@fail], [@fail, '--flag']]
+            expect(@test.check_all_attributes).to eq(true)
+          end
+
+          it "does not run if one command exits zero" do
+            @test[param] = [[@pass, '--flag'], [@pass], [@fail, '--flag']]
+            expect(@test.check_all_attributes).to eq(false)
+          end
+
+          it "does not run if all command exits zero" do
+            @test[param] = [[@pass, '--flag'], [@pass], [@pass, '--flag']]
+            expect(@test.check_all_attributes).to eq(false)
+          end
+        end
+
         it "should emit output to debug" do
           Puppet::Util::Log.level = :debug
           @test[param] = @fail