(PUP-5704) allow array commands in exec resource

This change updates the exec resource to alos accept
arrays as command. The new behaviour is enabled for
the following parameters: `:comand`, `:onlyif`,
`:unless`, `:refresh`.

```
Changing the command to accept an:

command: "/bin/echo *"         # executes through shell
command: ["/bin/echo *"]       # executes through shell
command: ["/bin/echo *", "*"]  # non-existing command "bin/echo *"
command: ["/bin/echo", "*"]    # executes directly

onlyif/unless: '/bin/echo *'                                  # executes one command through shell
onlyif/unless: ['/bin/echo *', '/bin/echo $SHELL']            # executes two commands through shell
onlyif/unless: [["/bin/echo", "*"]]                           # executes one command directly
onlyif/unless: [["/bin/echo", "*"], ["/bin/echo", "$SHELL"]]  # executes two commands directly
```

acceptance/tests/resource/exec/accept_array_commands.rb

@@ -0,0 +1,27 @@
+test_name "Be able to execute array commands" do
+  tag 'audit:high',
+      'audit:acceptance'
+
+  agents.each do |agent|
+    if agent.platform =~ /windows/
+      cmd = ['cmd.exe', '/c', 'echo', '*']
+      path = 'C:\Windows\System32'
+    else
+      cmd = ['/bin/echo', '*']
+      path = '/usr/bin'
+    end
+    exec_manifest = <<~MANIFEST
+      exec { "test exec":
+        command => #{cmd},
+        path => "#{path}",
+        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,20 @@
   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 this 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 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).
+
+    Caution: passing `[cmdname]` will behave exactly as passing `cmdname`
   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
 
@@ -455,9 +457,11 @@ def check(value)
         must fully qualify the command's name.
 
         This parameter can also take an array of commands. For example:
-
             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
@@ -511,9 +515,11 @@ def check(value)
         must fully qualify the command's name.
 
         This parameter can also take an array of commands. For example:
-
             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,15 +568,18 @@ 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
       }
 
+
       [:onlyif, :unless].each { |param|
         tmp = self[param]
         next unless tmp

spec/integration/type/exec_spec.rb

@@ -7,70 +7,83 @@
 
   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']
+      )
 
-  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
 
-    catalog.add_resource exec
-    catalog.apply
+      expect(File.read(path)).to eq('foo')
+    end
 
-    expect(File.read(path)).to eq('foo')
+    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
+
+      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
 
-    catalog.add_resource exec
-    catalog.apply
+  context 'when command is an array' do
+    let(:command) { ['ruby', '-e', "File.open(\"#{path}\", \"w\") { |f| f.print \"foo\" }"] }
 
-    expect(Puppet::FileSystem.exist?(path)).to be_falsey
+    it_behaves_like 'a valid exec resource'
   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 => ['/bin/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,
@@ -379,7 +392,7 @@ 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
+    describe "when given an array as input" do
       before :each do
         @test = Puppet::Type.type(:exec).new(:name => @executable)
       end
@@ -408,16 +421,40 @@ def test(command, valid)
         expect(@test[param]).to eq(input)
       end
     end
+
+    describe "when given an array of arrays as input" do
+      let(:test) { Puppet::Type.type(:exec).new(:name => @executable) }
+
+      it "accepts the array when all commands return valid" do
+        input = [['one, -a'], ['two', '-b'], '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 "rejects the array when any commands return invalid" do
+        input = [['one, -a'], ['two', '-b'], '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)
+        end
+        test[param] = input
+        expect(test[param]).to eq(input)
+      end
+
+      it "rejects the array when all commands return invalid" do
+        input = [['one, -a'], ['two', '-b'], 'three']
+        expect(test.provider).to receive(:validatecmd).exactly(input.length).times.and_return(false)
+        test[param] = input
+        expect(test[param]).to eq(input)
+      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 +796,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