Tweak data collector exception handling

This isn't Java and Net::HTTP can throw a billion things, so just rescue
(nearly) everything.  I have a hard time imagining that there's a
failure here that we wouldn't want to ignore, other than the ones like
OOM thrown by Exception that indicate internal failures.

Also only throw the "this is normal" info message on 404s, other
responses like 500s should be warns even if it is set to ignore
failures (people with data collectors correctly setup need to know about
500s even if we keep going, since that is abnormal).

This also fixes a bug in handling exceptions like Timeout::Error which
do not have a response.code at all which would throw NoMethodError.

closes #8749

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

2 files changed, 97 insertions, 8 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index aac864f485..47d329b2b6 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -177,14 +177,17 @@ class Chef
 
         @http ||= setup_http_client(Chef::Config[:data_collector][:server_url])
         @http.post(nil, message, headers)
-      rescue Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
-        Errno::ECONNREFUSED, EOFError, Net::HTTPBadResponse,
-        Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError,
-        Errno::EHOSTDOWN => e
+      rescue => e
         # Do not disable data collector reporter if additional output_locations have been specified
         events.unregister(self) unless Chef::Config[:data_collector][:output_locations]
 
-        code = e&.response&.code&.to_s || "Exception Code Empty"
+        begin
+          code = e&.response&.code&.to_s
+        rescue
+          # i really dont care
+        end
+
+        code ||= "No HTTP Code"
 
         msg = "Error while reporting run start to Data Collector. URL: #{Chef::Config[:data_collector][:server_url]} Exception: #{code} -- #{e.message} "
 
@@ -192,9 +195,13 @@ class Chef
           Chef::Log.error(msg)
           raise
         else
-          # Make the message non-scary for folks who don't have automate:
-          msg << " (This is normal if you do not have #{Chef::Dist::AUTOMATE})"
-          Chef::Log.info(msg)
+          if code == "404"
+            # Make the message non-scary for folks who don't have automate:
+            msg << " (This is normal if you do not have #{Chef::Dist::AUTOMATE})"
+            Chef::Log.info(msg)
+          else
+            Chef::Log.warn(msg)
+          end
         end
       end
 
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index d7b18b3c28..dc26ba5c43 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -879,4 +879,86 @@ describe Chef::DataCollector do
       data_collector.send(:send_to_file_location, tempfile, { invalid: shift_jis })
     end
   end
+
+  describe "#send_to_datacollector" do
+    def stub_http_client(exception = nil)
+      if exception.nil?
+        expect(http_client).to receive(:post).with(nil, message, data_collector.send(:headers))
+      else
+        expect(http_client).to receive(:post).with(nil, message, data_collector.send(:headers)).and_raise(exception)
+      end
+    end
+
+    let(:message) { "message" }
+    let(:http_client) { instance_double(Chef::ServerAPI) }
+
+    before do
+      expect(data_collector).to receive(:setup_http_client).and_return(http_client)
+    end
+
+    it "does not disable the data_collector when no exception is raised" do
+      stub_http_client
+      expect(data_collector.events).not_to receive(:unregister)
+      data_collector.send(:send_to_data_collector, message)
+    end
+
+    errors = [ Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
+               Errno::ECONNREFUSED, EOFError, Net::HTTPBadResponse,
+               Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError,
+               Errno::EHOSTDOWN ]
+
+    errors.each do |exception_class|
+      context "when the client raises a #{exception_class} exception" do
+        before do
+          stub_http_client(exception_class)
+        end
+
+        it "disables the reporter" do
+          expect(data_collector.events).to receive(:unregister).with(data_collector)
+          data_collector.send(:send_to_data_collector, message)
+        end
+
+        it "logs an error and raises when raise_on_failure is enabled" do
+          Chef::Config[:data_collector][:raise_on_failure] = true
+          expect(Chef::Log).to receive(:error)
+          expect { data_collector.send(:send_to_data_collector, message) }.to raise_error(exception_class)
+        end
+
+        it "logs a warn message and does not raise an exception when raise_on_failure is disabled" do
+          Chef::Config[:data_collector][:raise_on_failure] = false
+          expect(Chef::Log).to receive(:warn)
+          data_collector.send(:send_to_data_collector, message)
+        end
+      end
+    end
+
+    context "when the client raises a 404 exception" do
+      let(:err) do
+        response = double("Net::HTTP response", code: "404")
+        Net::HTTPClientException.new("Not Found", response)
+      end
+
+      before do
+        stub_http_client(err)
+      end
+
+      it "disables the reporter" do
+        expect(data_collector.events).to receive(:unregister).with(data_collector)
+        data_collector.send(:send_to_data_collector, message)
+      end
+
+      it "logs an error and raises when raise_on_failure is enabled" do
+        Chef::Config[:data_collector][:raise_on_failure] = true
+        expect(Chef::Log).to receive(:error)
+        expect { data_collector.send(:send_to_data_collector, message) }.to raise_error(err)
+      end
+
+      # this is different specifically for 404s
+      it "logs an info message and does not raise an exception when raise_on_failure is disabled" do
+        Chef::Config[:data_collector][:raise_on_failure] = false
+        expect(Chef::Log).to receive(:info).with(/This is normal if you do not have Chef Automate/)
+        data_collector.send(:send_to_data_collector, message)
+      end
+    end
+  end
 end