Merge pull request #6250 from chef/lcg/fix-execute-path

fix execute resource to use shell_out_with_systems_locale

5 files changed, 43 insertions, 25 deletions

diff --git a/lib/chef/provider/execute.rb b/lib/chef/provider/execute.rb
index e227528918..d19a95b4ae 100644
--- a/lib/chef/provider/execute.rb
+++ b/lib/chef/provider/execute.rb
@@ -55,7 +55,7 @@ class Chef
 
         converge_by("execute #{description}") do
           begin
-            shell_out!(command, opts)
+            shell_out_with_systems_locale!(command, opts)
           rescue Mixlib::ShellOut::ShellCommandFailed
             if sensitive?
               raise Mixlib::ShellOut::ShellCommandFailed,
diff --git a/spec/functional/resource/execute_spec.rb b/spec/functional/resource/execute_spec.rb
index 09b7286854..c0956c5594 100644
--- a/spec/functional/resource/execute_spec.rb
+++ b/spec/functional/resource/execute_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Serdar Sutay (<serdar@chef.io>)
-# Copyright:: Copyright 2014-2016, Chef Software Inc.
+# Copyright:: Copyright 2014-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -51,7 +51,7 @@ describe Chef::Resource::Execute do
 
       # why_run mode doesn't disable the updated_by_last_action logic, so we really have to look at the provider action
       # to see if why_run correctly disabled the resource.  It should shell_out! for the guard but not the resource.
-      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).once
+      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).once
 
       resource.only_if guard
       resource.run_action(:run)
diff --git a/spec/unit/provider/apt_update_spec.rb b/spec/unit/provider/apt_update_spec.rb
index 351a10051c..e1ab0085ce 100644
--- a/spec/unit/provider/apt_update_spec.rb
+++ b/spec/unit/provider/apt_update_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Thom May (<thom@chef.io>)
-# Copyright:: Copyright (c) 2016 Chef Software, Inc.
+# Copyright:: Copyright (c) 2016-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -45,7 +45,7 @@ describe Chef::Provider::AptUpdate do
     before do
       FileUtils.rmdir config_dir
       expect(File.exist?(config_dir)).to be false
-      allow_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
+      allow_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
     end
 
     it "should create the directory" do
@@ -64,7 +64,7 @@ describe Chef::Provider::AptUpdate do
   describe "#action_update" do
     it "should update the apt cache" do
       provider.load_current_resource
-      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
+      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
       provider.run_action(:update)
       expect(new_resource).to be_updated_by_last_action
     end
@@ -79,14 +79,14 @@ describe Chef::Provider::AptUpdate do
 
     it "should run if the time stamp is old" do
       expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now - 86_500)
-      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
+      expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
       provider.run_action(:periodic)
       expect(new_resource).to be_updated_by_last_action
     end
 
     it "should not run if the time stamp is new" do
       expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now)
-      expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out!).with("apt-get -q update", anything())
+      expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
       provider.run_action(:periodic)
       expect(new_resource).to_not be_updated_by_last_action
     end
@@ -98,14 +98,14 @@ describe Chef::Provider::AptUpdate do
 
       it "should run if the time stamp is old" do
         expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now - 500)
-        expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out!).with("apt-get -q update", anything())
+        expect_any_instance_of(Chef::Provider::Execute).to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
         provider.run_action(:periodic)
         expect(new_resource).to be_updated_by_last_action
       end
 
       it "should not run if the time stamp is new" do
         expect(File).to receive(:mtime).with("#{stamp_dir}/update-success-stamp").and_return(Time.now - 300)
-        expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out!).with("apt-get -q update", anything())
+        expect_any_instance_of(Chef::Provider::Execute).not_to receive(:shell_out_with_systems_locale!).with("apt-get -q update", anything())
         provider.run_action(:periodic)
         expect(new_resource).to_not be_updated_by_last_action
       end
diff --git a/spec/unit/provider/execute_spec.rb b/spec/unit/provider/execute_spec.rb
index 904a2841c5..58cb9f33a8 100644
--- a/spec/unit/provider/execute_spec.rb
+++ b/spec/unit/provider/execute_spec.rb
@@ -76,16 +76,34 @@ describe Chef::Provider::Execute do
 
   describe "#action_run" do
     it "runs shell_out with the default options" do
-      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
       expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
       expect(new_resource).to be_updated
     end
 
+    # this next test is tightly coupled to the implementation of the underlying shell_out mixin that we're using
+    # but the point is to ensure that we are not picking up the PATH mangling and locale-variable mangling that the internal
+    # shell_out API uses.  we are asserting that we emulate `ls -la` when the user does `execute "ls -la"`, and to
+    # do that we get dirty and start mocking the implementation of the shell_out mixin itself.  while arguments like
+    # "timeout", "returns", "log_level" and "log_tag" appear here, we MUST NOT have an "environment" or "env" argument
+    # that we are passing to Mixlib::ShellOut by default -- ever.  you might have to add some other argument here from
+    # time to time, but you MUST NOT change the environment.
+    it "does not use shell_out in such a way as to insert extra environment variables" do
+      mock = instance_double(Mixlib::ShellOut)
+      expect(Mixlib::ShellOut).to receive(:new).with("foo_resource", { timeout: 3600, returns: 0, log_level: :info, log_tag: "execute[foo_resource]" }).and_return(mock)
+      expect(mock).to receive(:live_stream=).with(nil)
+      allow(mock).to receive(:live_stream)
+      expect(mock).to receive(:run_command)
+      expect(mock).to receive(:error!)
+      provider.run_action(:run)
+      expect(new_resource).to be_updated
+    end
+
     it "if you pass a command attribute, it runs the command" do
       new_resource.command "/usr/argelbargle/bin/oogachacka 12345"
-      expect(provider).to receive(:shell_out!).with(new_resource.command, opts)
+      expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.command, opts)
       expect(provider).to receive(:converge_by).with("execute #{new_resource.command}").and_call_original
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
@@ -96,7 +114,7 @@ describe Chef::Provider::Execute do
       new_resource.sensitive true
       # Since the resource is sensitive, it should not have :live_stream set
       opts.delete(:live_stream)
-      expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+      expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
       expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
@@ -106,7 +124,7 @@ describe Chef::Provider::Execute do
     it "should do nothing if the sentinel file exists" do
       new_resource.creates "/foo_resource"
       expect(FileTest).to receive(:exist?).with(new_resource.creates).and_return(true)
-      expect(provider).not_to receive(:shell_out!)
+      expect(provider).not_to receive(:shell_out_with_systems_locale!)
       expect(Chef::Log).not_to receive(:warn)
       provider.run_action(:run)
       expect(new_resource).not_to be_updated
@@ -119,7 +137,7 @@ describe Chef::Provider::Execute do
       end
 
       it "should raise if user specified relative path without cwd for Chef-13" do
-        expect(provider).not_to receive(:shell_out!)
+        expect(provider).not_to receive(:shell_out_with_systems_locale!)
         expect { provider.run_action(:run) }.to raise_error(Chef::Exceptions::Execute)
       end
     end
@@ -130,7 +148,7 @@ describe Chef::Provider::Execute do
       expect(FileTest).not_to receive(:exist?).with(new_resource.creates)
       expect(FileTest).to receive(:exist?).with(File.join("/tmp", new_resource.creates)).and_return(true)
       expect(Chef::Log).not_to receive(:warn)
-      expect(provider).not_to receive(:shell_out!)
+      expect(provider).not_to receive(:shell_out_with_systems_locale!)
 
       provider.run_action(:run)
       expect(new_resource).not_to be_updated
@@ -139,7 +157,7 @@ describe Chef::Provider::Execute do
     it "should not include stdout/stderr in failure exception for sensitive resource" do
       opts.delete(:live_stream)
       new_resource.sensitive true
-      expect(provider).to receive(:shell_out!).and_raise(Mixlib::ShellOut::ShellCommandFailed)
+      expect(provider).to receive(:shell_out_with_systems_locale!).and_raise(Mixlib::ShellOut::ShellCommandFailed)
       expect do
         provider.run_action(:run)
       end.to raise_error(Mixlib::ShellOut::ShellCommandFailed, /suppressed for sensitive resource/)
@@ -148,7 +166,7 @@ describe Chef::Provider::Execute do
     describe "streaming output" do
       it "should not set the live_stream if sensitive is on" do
         new_resource.sensitive true
-        expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+        expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
         expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
         expect(Chef::Log).not_to receive(:warn)
         provider.run_action(:run)
@@ -165,7 +183,7 @@ describe Chef::Provider::Execute do
         it "should set the live_stream if the log level is info or above" do
           nopts = opts
           nopts[:live_stream] = @live_stream
-          expect(provider).to receive(:shell_out!).with(new_resource.name, nopts)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, nopts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -177,7 +195,7 @@ describe Chef::Provider::Execute do
           new_resource.live_stream true
           nopts = opts
           nopts[:live_stream] = @live_stream
-          expect(provider).to receive(:shell_out!).with(new_resource.name, nopts)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, nopts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -186,7 +204,7 @@ describe Chef::Provider::Execute do
 
         it "should not set the live_stream if the resource is sensitive" do
           new_resource.sensitive true
-          expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
           expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -199,7 +217,7 @@ describe Chef::Provider::Execute do
           nopts = opts
           nopts[:live_stream] = STDOUT
           allow(STDOUT).to receive(:tty?).and_return(true)
-          expect(provider).to receive(:shell_out!).with(new_resource.name, nopts)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, nopts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -209,7 +227,7 @@ describe Chef::Provider::Execute do
         it "should not set the live_stream to STDOUT if we are a TTY, not daemonized, but sensitive" do
           new_resource.sensitive true
           allow(STDOUT).to receive(:tty?).and_return(true)
-          expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
           expect(provider).to receive(:converge_by).with("execute sensitive resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
@@ -219,7 +237,7 @@ describe Chef::Provider::Execute do
         it "should not set the live_stream to STDOUT if we are a TTY, but daemonized" do
           Chef::Config[:daemon] = true
           allow(STDOUT).to receive(:tty?).and_return(true)
-          expect(provider).to receive(:shell_out!).with(new_resource.name, opts)
+          expect(provider).to receive(:shell_out_with_systems_locale!).with(new_resource.name, opts)
           expect(provider).to receive(:converge_by).with("execute foo_resource").and_call_original
           expect(Chef::Log).not_to receive(:warn)
           provider.run_action(:run)
diff --git a/spec/unit/provider/script_spec.rb b/spec/unit/provider/script_spec.rb
index 2f024c4c29..951d6546c3 100644
--- a/spec/unit/provider/script_spec.rb
+++ b/spec/unit/provider/script_spec.rb
@@ -143,7 +143,7 @@ describe Chef::Provider::Script, "action_run" do
       end
 
       it "should call shell_out! with the command" do
-        expect(provider).to receive(:shell_out!).with(provider.command, default_opts).and_return(true)
+        expect(provider).to receive(:shell_out_with_systems_locale!).with(provider.command, default_opts).and_return(true)
         provider.action_run
       end