Mark run failed on failed audits unless configured to audit as warning.

Remove unnecessary newline from RunFailedWrappingError backtrace

5 files changed, 203 insertions, 132 deletions

diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index 0764d3f3ba..bf99f003fb 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -336,7 +336,7 @@ class Chef
           @events.converge_start(run_context)
           Chef::Log.debug("Converging node #{node_name}")
           @runner = Chef::Runner.new(run_context)
-          runner.converge
+          @runner.converge
           @events.converge_complete
         rescue Exception => e
           @events.converge_failed(e)
@@ -370,9 +370,11 @@ class Chef
         auditor = Chef::Audit::Runner.new(run_context)
         auditor.run
         if auditor.failed?
-          raise Chef::Exceptions::AuditsFailed.new(auditor.num_failed, auditor.num_total)
+          audit_exception = Chef::Exceptions::AuditsFailed.new(auditor.num_failed, auditor.num_total)
+          @events.audit_phase_failed(audit_exception)
+        else
+          @events.audit_phase_complete
         end
-        @events.audit_phase_complete
       rescue Exception => e
         Chef::Log.error("Audit phase failed with error message: #{e.message}")
         @events.audit_phase_failed(e)
@@ -464,10 +466,9 @@ class Chef
           audit_error = run_audits(run_context)
         end
 
-        if converge_error || audit_error
-          e = Chef::Exceptions::RunFailedWrappingError.new(converge_error, audit_error)
-          e.fill_backtrace
-          raise e
+        if err = wrap_exceptions(converge_error, audit_error)
+          err.fill_backtrace
+          raise err
         end
 
         run_status.stop_clock
@@ -537,6 +538,19 @@ class Chef
       Chef::ReservedNames::Win32::Security.has_admin_privileges?
     end
 
+    def wrap_exceptions(converge_error, audit_error)
+      if audit_error && !(audit_error.is_a?(Chef::Exceptions::AuditsFailed) && Chef::Config[:audit_as_warning])
+        if converge_error
+          return Chef::Exceptions::RunFailedWrappingError.new(converge_error, audit_error)
+        else
+          return Chef::Exceptions::RunFailedWrappingError.new(audit_error)
+        end
+      elsif converge_error
+        return Chef::Exceptions::RunFailedWrappingError.new(converge_error)
+      end
+      nil
+    end
+
   end
 end
 
diff --git a/lib/chef/config.rb b/lib/chef/config.rb
index 629553c8ab..9beb18b53e 100644
--- a/lib/chef/config.rb
+++ b/lib/chef/config.rb
@@ -51,4 +51,3 @@ class Chef
 
   end
 end
-
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index da562e70f4..c0f4158db4 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -435,7 +435,7 @@ class Chef
         wrapped_errors.each_with_index do |e,i|
           backtrace << "#{i+1}) #{e.class} -  #{e.message}"
           backtrace += e.backtrace if e.backtrace
-          backtrace << ""
+          backtrace << "" unless i == wrapped_errors.length - 1
         end
         set_backtrace(backtrace)
       end
diff --git a/spec/unit/client_spec.rb b/spec/unit/client_spec.rb
index d8c4ede796..2f03b23e32 100644
--- a/spec/unit/client_spec.rb
+++ b/spec/unit/client_spec.rb
@@ -407,13 +407,18 @@ describe Chef::Client do
       end
     end
 
-    describe "when converge fails" do
+    context "when converge errors" do
       include_context "a client run" do
-        let(:e) { Exception.new }
+
+        let(:converge_error) do
+          err = Chef::Exceptions::UnsupportedAction.new("Action unsupported")
+          err.set_backtrace([ "/path/recipe.rb:15", "/path/recipe.rb:12" ])
+          err
+        end
+
         def stub_for_converge
           expect(Chef::Runner).to receive(:new).and_return(runner)
-          expect(runner).to receive(:converge).and_raise(e)
-          expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(Chef::Exceptions::RunFailedWrappingError)
+          expect(runner).to receive(:converge).and_raise(converge_error)
         end
 
         def stub_for_node_save
@@ -432,179 +437,232 @@ describe Chef::Client do
           expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
           expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_failed)
         end
-      end
 
-      it "runs the audits and raises the error" do
-        expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
-          expect(error.wrapped_errors.size).to eq(1)
-          expect(error.wrapped_errors[0]).to eq(e)
+        before do
+          expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(Chef::Exceptions::RunFailedWrappingError)
         end
-      end
-    end
-
-    describe "when the audit phase fails" do
-      context "with an exception" do
-        context "when audit mode is enabled" do
-          include_context "a client run" do
-            let(:e) { Exception.new }
-            def stub_for_audit
-              expect(Chef::Audit::Runner).to receive(:new).and_return(audit_runner)
-              expect(audit_runner).to receive(:run).and_raise(e)
-              expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(Chef::Exceptions::RunFailedWrappingError)
-            end
-
-            def stub_for_run
-              expect_any_instance_of(Chef::RunLock).to receive(:acquire)
-              expect_any_instance_of(Chef::RunLock).to receive(:save_pid)
-              expect_any_instance_of(Chef::RunLock).to receive(:release)
-
-              # Post conditions: check that node has been filled in correctly
-              expect(client).to receive(:run_started)
-              expect(client).to receive(:run_failed)
 
-              expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
-              expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_failed)
-            end
-          end
-
-          it "should save the node after converge and raise exception" do
+        context "without audit phase errors" do
+          it "skips node save and raises the converge error in a wrapping error" do
             expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
               expect(error.wrapped_errors.size).to eq(1)
-              expect(error.wrapped_errors[0]).to eq(e)
+              expect(error.wrapped_errors).to include(converge_error)
+              expect(error.backtrace).to include(*converge_error.backtrace)
             end
           end
         end
 
-        context "when audit mode is disabled" do
-          include_context "a client run" do
-            before do
-              Chef::Config[:audit_mode] = :disabled
+        context "when audit phase errors" do
+          context "with failed audits" do
+
+            let(:audit_error) do
+              err = Chef::Exceptions::AuditsFailed.new(num_failed, num_total)
+              err.set_backtrace([ "/path/recipe.rb:57", "/path/recipe.rb:55" ])
+              err
             end
 
-            let(:e) { FooError.new }
+            let(:num_failed) { 1 }
+
+            let(:num_total) { 5 }
 
             def stub_for_audit
-              expect(Chef::Audit::Runner).to_not receive(:new)
+              expect(Chef::Audit::Runner).to receive(:new).and_return(audit_runner)
+              expect(audit_runner).to receive(:run)
+              expect(audit_runner).to receive(:failed?).and_return(true)
+              expect(Chef::Exceptions::AuditsFailed).to receive(:new).with(num_failed, num_total).and_return(audit_error)
+              allow(audit_runner).to receive(:num_failed).and_return(num_failed)
+              allow(audit_runner).to receive(:num_total).and_return(num_total)
             end
 
-            def stub_for_converge
-              expect(Chef::Runner).to receive(:new).and_return(runner)
-              expect(runner).to receive(:converge).and_raise(e)
-              expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(FooError)
+            context "with audit_as_warning true" do
+              before do
+                Chef::Config[:audit_as_warning] = true
+              end
+
+              it "skips node save and raises the converge error in a wrapping error" do
+                expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                  expect(error.wrapped_errors.size).to eq(1)
+                  expect(error.wrapped_errors[0]).to eq(converge_error)
+                end
+              end
             end
 
-            def stub_for_node_save
-              expect(client).to_not receive(:save_updated_node)
+            context "with audit_as_warning false" do
+
+              before do
+                Chef::Config[:audit_as_warning] = false
+              end
+
+              it "skips node save and raises converge and audit errors in a wrapping error" do
+                expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                  expect(error.wrapped_errors.size).to eq(2)
+                  expect(error.wrapped_errors).to include(converge_error)
+                  expect(error.wrapped_errors).to include(audit_error)
+                  expect(error.backtrace).to include(*converge_error.backtrace)
+                  expect(error.backtrace).to include(*audit_error.backtrace)
+                end
+              end
             end
+          end
 
-            def stub_for_run
-              expect_any_instance_of(Chef::RunLock).to receive(:acquire)
-              expect_any_instance_of(Chef::RunLock).to receive(:save_pid)
-              expect_any_instance_of(Chef::RunLock).to receive(:release)
-
+          context "with unexpected error" do
 
-              # Post conditions: check that node has been filled in correctly
-              expect(client).to receive(:run_started)
-              expect(client).to receive(:run_failed)
+            let(:audit_error) do
+              err = RuntimeError.new("Unexpected audit error")
+              err.set_backtrace([ "/path/recipe.rb:57", "/path/recipe.rb:55" ])
+              err
+            end
 
-              expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
+            def stub_for_audit
+              expect(Chef::Audit::Runner).to receive(:new).and_return(audit_runner)
+              expect(audit_runner).to receive(:run).and_raise(audit_error)
+            end
 
+            context "with audit_as_warning true" do
+              it "skips node save and raises converge and audit errors in a wrapping error" do
+                expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                  expect(error.wrapped_errors.size).to eq(2)
+                  expect(error.wrapped_errors).to include(converge_error)
+                  expect(error.wrapped_errors).to include(audit_error)
+                  expect(error.backtrace).to include(*converge_error.backtrace)
+                  expect(error.backtrace).to include(*audit_error.backtrace)
+                end
+              end
             end
 
-            it "re-raises an unwrapped exception" do
-              expect { client.run }.to raise_error(FooError)
+            context "with audit_as_warning false" do
+              it "skips node save and raises converge and audit errors in a wrapping error" do
+                expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                  expect(error.wrapped_errors.size).to eq(2)
+                  expect(error.wrapped_errors).to include(converge_error)
+                  expect(error.wrapped_errors).to include(audit_error)
+                  expect(error.backtrace).to include(*converge_error.backtrace)
+                  expect(error.backtrace).to include(*audit_error.backtrace)
+                end
+              end
             end
           end
         end
+      end
+    end
 
+    context "when audit phase errors" do
+      include_context "a client run" do
 
-      end
+        def stub_for_run
+          expect_any_instance_of(Chef::RunLock).to receive(:acquire)
+          expect_any_instance_of(Chef::RunLock).to receive(:save_pid)
+          expect_any_instance_of(Chef::RunLock).to receive(:release)
+
+          # Post conditions: check that node has been filled in correctly
+          expect(client).to receive(:run_started)
+        end
+
+        context "with failed audits" do
 
-      context "with failed audits" do
-        include_context "a client run" do
-          let(:audit_runner) do
-            instance_double("Chef::Audit::Runner", :run => true, :failed? => true, :num_failed => 1, :num_total => 1)
+          let(:audit_error) do
+            err = Chef::Exceptions::AuditsFailed.new(num_failed, num_total)
+            err.set_backtrace([ "/path/recipe.rb:57", "/path/recipe.rb:55" ])
+            err
           end
 
+          let(:num_failed) { 1 }
+
+          let(:num_total) { 5 }
+
           def stub_for_audit
             expect(Chef::Audit::Runner).to receive(:new).and_return(audit_runner)
-            expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(Chef::Exceptions::RunFailedWrappingError)
+            expect(audit_runner).to receive(:run)
+            expect(audit_runner).to receive(:failed?).and_return(true)
+            expect(Chef::Exceptions::AuditsFailed).to receive(:new).with(num_failed, num_total).and_return(audit_error)
+            allow(audit_runner).to receive(:num_failed).and_return(num_failed)
+            allow(audit_runner).to receive(:num_total).and_return(num_total)
           end
 
-          def stub_for_run
-            expect_any_instance_of(Chef::RunLock).to receive(:acquire)
-            expect_any_instance_of(Chef::RunLock).to receive(:save_pid)
-            expect_any_instance_of(Chef::RunLock).to receive(:release)
-
-            # Post conditions: check that node has been filled in correctly
-            expect(client).to receive(:run_started)
-            expect(client).to receive(:run_failed)
+          context "with audit_as_warning true" do
+            before do
+              Chef::Config[:audit_as_warning] = true
+            end
 
-            expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
-            expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_failed)
+            it "saves the node and completes the run successfully" do
+              expect(client).to receive(:run_completed_successfully)
+              expect_any_instance_of(Chef::ResourceReporter).to receive(:run_completed)
+              expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_completed)
+              client.run
+            end
           end
-        end
 
-        it "should save the node after converge and raise exception" do
-          expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
-            expect(error.wrapped_errors.size).to eq(1)
-            expect(error.wrapped_errors[0]).to be_instance_of(Chef::Exceptions::AuditsFailed)
-          end
-        end
-      end
-    end
+          context "with audit_as_warning false" do
 
-    describe "when why_run mode is enabled" do
-      include_context "a client run" do
+            before do
+              Chef::Config[:audit_as_warning] = false
+              expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(Chef::Exceptions::RunFailedWrappingError)
+            end
 
-        before do
-          Chef::Config[:why_run] = true
-        end
+            it "saves the node and raises audit error in a wrapping error" do
+              expect(client).to receive(:run_failed)
+              expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
+              expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_failed)
 
-        def stub_for_audit
-          expect(Chef::Audit::Runner).to_not receive(:new)
+              expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                expect(error.wrapped_errors.size).to eq(1)
+                expect(error.wrapped_errors).to include(audit_error)
+                expect(error.backtrace).to include(*audit_error.backtrace)
+              end
+            end
+          end
         end
 
-        def stub_for_node_save
-          # This is how we should be mocking external calls - not letting it fall all the way through to the
-          # REST call
-          expect(node).to receive(:save)
-        end
+        context "with unexpected error" do
 
-        it "runs successfully without enabling the audit runner" do
-          client.run
+          let(:audit_error) do
+            err = RuntimeError.new("Unexpected audit error")
+            err.set_backtrace([ "/path/recipe.rb:57", "/path/recipe.rb:55" ])
+            err
+          end
 
-          # fork is stubbed, so we can see the outcome of the run
-          expect(node.automatic_attrs[:platform]).to eq("example-platform")
-          expect(node.automatic_attrs[:platform_version]).to eq("example-platform-1.0")
-        end
-      end
-    end
+          def stub_for_audit
+            expect(Chef::Audit::Runner).to receive(:new).and_return(audit_runner)
+            expect(audit_runner).to receive(:run).and_raise(audit_error)
+          end
 
-    describe "when audits are disabled" do
-      include_context "a client run" do
+          before do
+            expect(Chef::Application).to receive(:debug_stacktrace).with an_instance_of(Chef::Exceptions::RunFailedWrappingError)
+          end
 
-        before do
-          Chef::Config[:audit_mode] = :disabled
-        end
+          context "with audit_as_warning true" do
+            it "saves the node and raises audit error in a wrapping error" do
+              expect(client).to receive(:run_failed)
+              expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
+              expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_failed)
 
-        def stub_for_audit
-          expect(Chef::Audit::Runner).to_not receive(:new)
-        end
+              expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                expect(error.wrapped_errors.size).to eq(1)
+                expect(error.wrapped_errors).to include(audit_error)
+                expect(error.backtrace).to include(*audit_error.backtrace)
+              end
+            end
+          end
 
-        it "runs successfully without enabling the audit runner" do
-          client.run
+          context "with audit_as_warning false" do
+            it "skips node save and raises converge and audit errors in a wrapping error" do
+              expect(client).to receive(:run_failed)
+              expect_any_instance_of(Chef::ResourceReporter).to receive(:run_failed)
+              expect_any_instance_of(Chef::Audit::AuditReporter).to receive(:run_failed)
 
-          # fork is stubbed, so we can see the outcome of the run
-          expect(node.automatic_attrs[:platform]).to eq("example-platform")
-          expect(node.automatic_attrs[:platform_version]).to eq("example-platform-1.0")
+              expect{ client.run }.to raise_error(Chef::Exceptions::RunFailedWrappingError) do |error|
+                expect(error.wrapped_errors.size).to eq(1)
+                expect(error.wrapped_errors).to include(audit_error)
+                expect(error.backtrace).to include(*audit_error.backtrace)
+              end
+            end
+          end
         end
       end
-    end
 
+    end
   end
 
-
   describe "when handling run failures" do
 
     it "should remove the run_lock on failure of #load_node" do
diff --git a/spec/unit/exceptions_spec.rb b/spec/unit/exceptions_spec.rb
index d35ecc8ec8..fd90aeab71 100644
--- a/spec/unit/exceptions_spec.rb
+++ b/spec/unit/exceptions_spec.rb
@@ -113,7 +113,7 @@ describe Chef::Exceptions do
     context "initialized with 1 error and nil" do
       let(:e) { Chef::Exceptions::RunFailedWrappingError.new(RuntimeError.new("foo"), nil)  }
       let(:num_errors) { 1 }
-      let(:backtrace) { ["1) RuntimeError -  foo", ""] }
+      let(:backtrace) { ["1) RuntimeError -  foo"] }
 
       include_examples "RunFailedWrappingError expectations"
     end
@@ -121,7 +121,7 @@ describe Chef::Exceptions do
     context "initialized with 2 errors" do
       let(:e) { Chef::Exceptions::RunFailedWrappingError.new(RuntimeError.new("foo"), RuntimeError.new("bar"))  }
       let(:num_errors) { 2 }
-      let(:backtrace) { ["1) RuntimeError -  foo", "", "2) RuntimeError -  bar", ""] }
+      let(:backtrace) { ["1) RuntimeError -  foo", "", "2) RuntimeError -  bar"] }
 
       include_examples "RunFailedWrappingError expectations"
     end