Merge pull request #5782 from chef/lcg/dnf-rhel7

rhel7 / dnf 2.0 fixes / improved errors

7 files changed, 278 insertions, 48 deletions

diff --git a/lib/chef/mixin/which.rb b/lib/chef/mixin/which.rb
index 4fa79eeccb..fd386241a0 100644
--- a/lib/chef/mixin/which.rb
+++ b/lib/chef/mixin/which.rb
@@ -1,6 +1,6 @@
 #--
 # Author:: Lamont Granquist <lamont@chef.io>
-# Copyright:: Copyright 2010-2016, Chef Software Inc.
+# Copyright:: Copyright 2010-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -18,15 +18,32 @@
 class Chef
   module Mixin
     module Which
-      def which(cmd, extra_path: nil)
+      def which(*cmds, extra_path: nil, &block)
+        where(*cmds, extra_path: extra_path, &block).first || false
+      end
+
+      def where(*cmds, extra_path: nil, &block)
         # NOTE: unnecessarily duplicates function of path_sanity
         extra_path ||= [ "/bin", "/usr/bin", "/sbin", "/usr/sbin" ]
-        paths = ENV["PATH"].split(File::PATH_SEPARATOR) + extra_path
-        paths.each do |path|
-          filename = Chef.path_to(File.join(path, cmd))
-          return filename if File.executable?(filename)
-        end
-        false
+        paths = env_path.split(File::PATH_SEPARATOR) + extra_path
+        cmds.map do |cmd|
+          paths.map do |path|
+            filename = Chef.path_to(File.join(path, cmd))
+            filename if valid_executable?(filename, &block)
+          end.compact
+        end.flatten
+      end
+
+      private
+
+      # for test stubbing
+      def env_path
+        ENV["PATH"]
+      end
+
+      def valid_executable?(filename, &block)
+        return false unless File.executable?(filename) && !File.directory?(filename)
+        block ? yield(filename) : true
       end
     end
   end
diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index f52614672a..edd27a0439 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software, Inc.
+# Copyright:: Copyright 2008-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
diff --git a/lib/chef/provider/package/dnf/dnf_helper.py b/lib/chef/provider/package/dnf/dnf_helper.py
index 236b967710..ef08bb54c2 100644
--- a/lib/chef/provider/package/dnf/dnf_helper.py
+++ b/lib/chef/provider/package/dnf/dnf_helper.py
@@ -53,7 +53,7 @@ def query(command):
     if len(archq.run()) > 0:
         q = archq
 
-    pkgs = dnf.query.latest_limit_pkgs(q, 1)
+    pkgs = q.latest(1).run()
 
     if not pkgs:
         sys.stdout.write('{} nil nil\n'.format(command['provides'].split().pop(0)))
diff --git a/lib/chef/provider/package/dnf/python_helper.rb b/lib/chef/provider/package/dnf/python_helper.rb
index 466114b339..d6e278a9fb 100644
--- a/lib/chef/provider/package/dnf/python_helper.rb
+++ b/lib/chef/provider/package/dnf/python_helper.rb
@@ -1,5 +1,5 @@
 #
-# Copyright:: Copyright 2016, Chef Software, Inc.
+# Copyright:: Copyright 2016-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -15,6 +15,8 @@
 # limitations under the License.
 #
 
+require "chef/mixin/which"
+require "chef/mixin/shell_out"
 require "chef/provider/package/dnf/version"
 require "timeout"
 
@@ -24,7 +26,8 @@ class Chef
       class Dnf < Chef::Provider::Package
         class PythonHelper
           include Singleton
-          extend Chef::Mixin::Which
+          include Chef::Mixin::Which
+          include Chef::Mixin::ShellOut
 
           attr_accessor :stdin
           attr_accessor :stdout
@@ -32,11 +35,16 @@ class Chef
           attr_accessor :wait_thr
 
           DNF_HELPER = ::File.expand_path(::File.join(::File.dirname(__FILE__), "dnf_helper.py")).freeze
-          DNF_COMMAND = "#{which("python3")} #{DNF_HELPER}"
+
+          def dnf_command
+            @dnf_command ||= which("python", "python3", "python2", "python2.7") do |f|
+              shell_out("#{f} -c 'import dnf'").exitstatus == 0
+            end + " #{DNF_HELPER}"
+          end
 
           def start
             ENV["PYTHONUNBUFFERED"] = "1"
-            @stdin, @stdout, @stderr, @wait_thr = Open3.popen3(DNF_COMMAND)
+            @stdin, @stdout, @stderr, @wait_thr = Open3.popen3(dnf_command)
           end
 
           def reap
@@ -53,6 +61,27 @@ class Chef
             start if stdin.nil?
           end
 
+          # @returns Array<Version>
+          def query(action, provides, version = nil, arch = nil)
+            with_helper do
+              json = build_query(action, provides, version, arch)
+              Chef::Log.debug "sending '#{json}' to python helper"
+              stdin.syswrite json + "\n"
+              output = stdout.sysread(4096).chomp
+              Chef::Log.debug "got '#{output}' from python helper"
+              version = parse_response(output)
+              Chef::Log.debug "parsed #{version} from python helper"
+              version
+            end
+          end
+
+          def restart
+            reap
+            start
+          end
+
+          private
+
           # i couldn't figure out how to decompose an evr on the python side, it seems reasonably
           # painless to do it in ruby (generally massaging nevras in the ruby side is HIGHLY
           # discouraged -- this is an "every rule has an exception" exception -- any additional
@@ -83,35 +112,41 @@ class Chef
             array.each_slice(3).map { |x| Version.new(*x) }.first
           end
 
-          # @returns Array<Version>
-          def query(action, provides, version = nil, arch = nil)
-            with_helper do
-              json = build_query(action, provides, version, arch)
-              Chef::Log.debug "sending '#{json}' to python helper"
-              stdin.syswrite json + "\n"
-              output = stdout.sysread(4096).chomp
-              Chef::Log.debug "got '#{output}' from python helper"
-              version = parse_response(output)
-              Chef::Log.debug "parsed #{version} from python helper"
-              version
+          def drain_stderr
+            output = ""
+            until IO.select([stderr], nil, nil, 0).nil?
+              output += stderr.sysread(4096).chomp
             end
-          end
-
-          def restart
-            reap
-            start
+            output
+          rescue
+            # we must rescue EOFError, and we don't much care about errors on stderr anyway
+            output
           end
 
           def with_helper
             max_retries ||= 5
+            ret = nil
             Timeout.timeout(600) do
               check
-              yield
+              ret = yield
             end
+            output = drain_stderr
+            unless output.empty?
+              Chef::Log.debug "discarding output on stderr from python helper: #{output}"
+            end
+            ret
           rescue EOFError, Errno::EPIPE, Timeout::Error, Errno::ESRCH => e
-            raise e unless ( max_retries -= 1 ) > 0
-            restart
-            retry
+            output = drain_stderr
+            if ( max_retries -= 1 ) > 0
+              unless output.empty?
+                Chef::Log.debug "discarding output on stderr from python helper: #{output}"
+              end
+              restart
+              retry
+            else
+              raise e if output.empty?
+              raise "dnf-helper.py had stderr output:\n\n#{output}"
+            end
           end
         end
       end
diff --git a/spec/unit/mixin/which.rb b/spec/unit/mixin/which.rb
new file mode 100644
index 0000000000..1764b3b89f
--- /dev/null
+++ b/spec/unit/mixin/which.rb
@@ -0,0 +1,160 @@
+#
+# Copyright:: Copyright 2011-2017, Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "spec_helper"
+
+class TestClass
+  include Chef::Mixin::Which
+end
+
+describe Chef::Mixin::Which do
+
+  let(:test) { TestClass.new }
+
+  describe "#which" do
+    def self.test_which(description, *args, finds: nil, others: [], directory: false, &block)
+      it description do
+        # stub the ENV['PATH']
+        expect(test).to receive(:env_path).and_return(["/dir1", "/dir2" ].join(File::PATH_SEPARATOR))
+
+        # most files should not be found
+        allow(File).to receive(:executable?).and_return(false)
+        allow(File).to receive(:directory?).and_return(false)
+
+        # stub the expectation
+        expect(File).to receive(:executable?).with(finds).and_return(true) if finds
+
+        # if the file we find is a directory
+        expect(File).to receive(:directory?).with(finds).and_return(true) if finds && directory
+
+        # allow for stubbing other paths to exist that we should not find
+        others.each do |other|
+          allow(File).to receive(:executable?).with(other).and_return(true)
+        end
+
+        # setup the actual expectation on the return value
+        if finds && !directory
+          expect(test.which(*args, &block)).to eql(finds)
+        else
+          expect(test.which(*args, &block)).to eql(false)
+        end
+      end
+    end
+
+    context "simple usage" do
+      test_which("returns false when it does not find anything", "foo1")
+
+      ["/dir1", "/dir2", "/bin", "/usr/bin", "/sbin", "/usr/sbin" ].each do |dir|
+        test_which("finds `foo1` in #{dir} when it is stubbed", "foo1", finds: "#{dir}/foo1")
+      end
+
+      test_which("does not find an executable directory", "foo1", finds: "/dir1/foo1", directory: true)
+    end
+
+    context "with an array of args" do
+      test_which("finds the first arg", "foo1", "foo2", finds: "/dir2/foo1")
+
+      test_which("finds the second arg", "foo1", "foo2", finds: "/dir2/foo2")
+
+      test_which("finds the first arg when there's both", "foo1", "foo2", finds: "/dir2/foo1", others: [ "/dir1/foo2" ])
+
+      test_which("and the directory order can be reversed", "foo1", "foo2", finds: "/dir1/foo1", others: [ "/dir2/foo2" ])
+
+      test_which("or be the same", "foo1", "foo2", finds: "/dir1/foo1", others: [ "/dir1/foo2" ])
+    end
+
+    context "with a block" do
+      test_which("doesnt find it if its false", "foo1", others: [ "/dir1/foo1" ]) do |f|
+        false
+      end
+
+      test_which("finds it if its true", "foo1", finds: "/dir1/foo1") do |f|
+        true
+      end
+
+      test_which("passes in the filename as the arg", "foo1", finds: "/dir1/foo1") do |f|
+        raise "bad arg to block" unless f == "/dir1/foo1"
+        true
+      end
+
+      test_which("arrays with blocks", "foo1", "foo2", finds: "/dir2/foo1", others: [ "/dir1/foo2" ]) do |f|
+        raise "bad arg to block" unless f == "/dir2/foo1" || f == "/dir1/foo2"
+        true
+      end
+    end
+  end
+
+  describe "#where" do
+    def self.test_where(description, *args, finds: [], others: [], &block)
+      it description do
+        # stub the ENV['PATH']
+        expect(test).to receive(:env_path).and_return(["/dir1", "/dir2" ].join(File::PATH_SEPARATOR))
+
+        # most files should not be found
+        allow(File).to receive(:executable?).and_return(false)
+        allow(File).to receive(:directory?).and_return(false)
+
+        # allow for stubbing other paths to exist that we should not return
+        others.each do |other|
+          allow(File).to receive(:executable?).with(other).and_return(true)
+        end
+
+        # stub the expectation
+        finds.each do |path|
+          expect(File).to receive(:executable?).with(path).and_return(true)
+        end
+
+        # setup the actual expectation on the return value
+        expect(test.where(*args, &block)).to eql(finds)
+      end
+    end
+
+    context "simple usage" do
+      test_where("returns empty array when it doesn't find anything", "foo1")
+
+      ["/dir1", "/dir2", "/bin", "/usr/bin", "/sbin", "/usr/sbin" ].each do |dir|
+        test_where("finds `foo1` in #{dir} when it is stubbed", "foo1", finds: [ "#{dir}/foo1" ])
+      end
+
+      test_where("finds `foo1` in all directories", "foo1", finds: [ "/dir1/foo1", "/dir2/foo1" ])
+    end
+
+    context "with an array of args" do
+      test_where("finds the first arg", "foo1", "foo2", finds: [ "/dir2/foo1" ])
+
+      test_where("finds the second arg", "foo1", "foo2", finds: [ "/dir2/foo2" ])
+
+      test_where("finds foo1 before foo2", "foo1", "foo2", finds: [ "/dir2/foo1", "/dir1/foo2" ])
+
+      test_where("finds foo1 before foo2 if the dirs are reversed", "foo1", "foo2", finds: [ "/dir1/foo1", "/dir2/foo2" ])
+
+      test_where("finds them both in the same directory", "foo1", "foo2", finds: [ "/dir1/foo1", "/dir1/foo2" ])
+
+      test_where("finds foo2 first if they're reversed", "foo2", "foo1", finds: [ "/dir1/foo2", "/dir1/foo1" ])
+    end
+
+    context "with a block do" do
+      test_where("finds foo1 and foo2 if they exist and the block is true", "foo1", "foo2", finds: [ "/dir1/foo2", "/dir2/foo2" ]) do
+        true
+      end
+
+      test_where("does not finds foo1 and foo2 if they exist and the block is false", "foo1", "foo2", others: [ "/dir1/foo2", "/dir2/foo2" ]) do
+        false
+      end
+    end
+  end
+end
diff --git a/spec/unit/provider/package/dnf/python_helper_spec.rb b/spec/unit/provider/package/dnf/python_helper_spec.rb
new file mode 100644
index 0000000000..505217bf90
--- /dev/null
+++ b/spec/unit/provider/package/dnf/python_helper_spec.rb
@@ -0,0 +1,29 @@
+#
+# Copyright:: Copyright 2017-2017, Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "spec_helper"
+
+# NOTE: most of the tests of this functionality are baked into the func tests for the dnf package provider
+
+describe Chef::Provider::Package::Dnf::PythonHelper do
+  let(:helper) { Chef::Provider::Package::Dnf::PythonHelper.instance }
+
+  it "propagates stacktraces on stderr from the forked subprocess" do
+    allow(helper).to receive(:dnf_command).and_return("ruby -e 'raise \"your hands in the air\"'")
+    expect { helper.query(:whatprovides, "tcpdump") }.to raise_error(/your hands in the air/)
+  end
+end
diff --git a/spec/unit/util/selinux_spec.rb b/spec/unit/util/selinux_spec.rb
index 609ff02215..751092bc9a 100644
--- a/spec/unit/util/selinux_spec.rb
+++ b/spec/unit/util/selinux_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Serdar Sutay (<serdar@chef.io>)
-# Copyright:: Copyright 2013-2016, Chef Software Inc.
+# Copyright:: Copyright 2013-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -148,21 +148,10 @@ describe Chef::Util::Selinux do
     end
 
     describe "when restorecon doesn't exist on the system" do
-      before do
-        allow(File).to receive(:executable?) do |file_path|
-          expect(file_path.end_with?("restorecon")).to be_truthy
-          false
-        end
-      end
-
       it "should log a warning message" do
-        log = [ ]
-        allow(Chef::Log).to receive(:warn) do |message|
-          log << message
-        end
-
+        allow(File).to receive(:executable?).with(/restorecon$/).and_return(false)
+        expect(Chef::Log).to receive(:warn).with(/Can not find 'restorecon' on the system. Skipping selinux security context restore./).at_least(:once)
         @test_instance.restore_security_context(path)
-        expect(log).not_to be_empty
         expect(File).not_to receive(:executable?)
         @test_instance.restore_security_context(path)
       end