fix execute resource to use shell_out_with_systems_locale

This may be a breaking change for some users coming from 13.0/13.1
but this was the intended way that this API would function in Chef 13.0.

With this change we stop inserting environment variables into
execute resources.  That means that whatever users type into the
execute resource should much more exactly agree with the response
on the command line, which will close a shedload of bugs and is how
Chef 13.0 was intended to operate.  We no longer globally mangle the
PATH or the LANG/LC_ locale environment variables.

The way that it worked in Chef 13.0/13.1 resulted in the internal
chef embedded/bin directory being prepended to the PATH environment
variable in the `execute` resource, which was never intended.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

4 files changed, 42 insertions, 24 deletions

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..ff74f3d887 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 enviroment.
+    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