Data Collector server URL validation, and disable on host down

If a user configured data_collector.server_url in their Chef config
as an empty string, we blissfully passed it along to Chef::HTTP
which would eventually raise a TypeError when trying to dup the
URI's host. This change validates the server_url when setting up
the Data Collector and gives helpful error messages to the user.

We also count Errno::EHOSTDOWN as an error worthy of disabling
the Data Collector reporter for a given run if the user so chooses.

3 files changed, 60 insertions, 2 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index d307fff2ba..ffdba99351 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -57,6 +57,8 @@ class Chef
                   :current_resource_report, :enabled
 
       def initialize
+        validate_data_collector_server_url!
+
         @all_resource_reports    = []
         @current_resource_loaded = nil
         @error_descriptions      = {}
@@ -236,7 +238,8 @@ class Chef
         yield
       rescue Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
              Errno::ECONNREFUSED, EOFError, Net::HTTPBadResponse,
-             Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError => e
+             Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError,
+             Errno::EHOSTDOWN => e
         disable_data_collector_reporter
         code = if e.respond_to?(:response) && e.response.code
                  e.response.code.to_s
@@ -376,6 +379,20 @@ class Chef
       def nested_resource?(new_resource)
         @current_resource_report && @current_resource_report.new_resource != new_resource
       end
+
+      def validate_data_collector_server_url!
+        raise Chef::Exceptions::ConfigurationError,
+          "data_collector.server_url is configured but empty. Please supply a valid URL." if data_collector_server_url.empty?
+
+        begin
+          uri = URI(data_collector_server_url)
+        rescue URI::InvalidURIError
+          raise Chef::Exceptions::ConfigurationError, "data_collector.server_url (#{data_collector_server_url}) is not a valid URI."
+        end
+
+        raise Chef::Exceptions::ConfigurationError,
+          "data_collector.server_url (#{data_collector_server_url}) is a URI with no host. Please upply a valid URL." if uri.host.nil?
+      end
     end
   end
 end
diff --git a/lib/chef/data_collector/resource_report.rb b/lib/chef/data_collector/resource_report.rb
index e031d4bcb6..dcaf9c8e44 100644
--- a/lib/chef/data_collector/resource_report.rb
+++ b/lib/chef/data_collector/resource_report.rb
@@ -80,6 +80,7 @@ class Chef
         if new_resource.cookbook_name
           hash["cookbook_name"]    = new_resource.cookbook_name
           hash["cookbook_version"] = new_resource.cookbook_version.version
+          hash["recipe_name"]      = new_resource.recipe_name
         end
 
         hash["conditional"]   = conditional.to_text if status == "skipped"
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index 831643347b..131d6b8df9 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -156,6 +156,10 @@ describe Chef::DataCollector::Reporter do
   let(:reporter) { described_class.new }
   let(:run_status) { Chef::RunStatus.new(Chef::Node.new, Chef::EventDispatch::Dispatcher.new) }
 
+  before do
+    Chef::Config[:data_collector][:server_url] = "http://my-data-collector-server.mycompany.com"
+  end
+
   describe '#run_started' do
     before do
       allow(reporter).to receive(:update_run_status)
@@ -490,7 +494,8 @@ describe Chef::DataCollector::Reporter do
 
     [ Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
       Errno::ECONNREFUSED, EOFError, Net::HTTPBadResponse,
-      Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError ].each do |exception_class|
+      Net::HTTPHeaderSyntaxError, Net::ProtocolError, OpenSSL::SSL::SSLError,
+      Errno::EHOSTDOWN ].each do |exception_class|
       context "when the block raises a #{exception_class} exception" do
         it "disables the reporter" do
           expect(reporter).to receive(:disable_data_collector_reporter)
@@ -515,4 +520,39 @@ describe Chef::DataCollector::Reporter do
       end
     end
   end
+
+  describe '#validate_data_collector_server_url!' do
+    context "when server_url is empty" do
+      it "raises an exception" do
+        Chef::Config[:data_collector][:server_url] = ""
+        expect { reporter.send(:validate_data_collector_server_url!) }.to raise_error(Chef::Exceptions::ConfigurationError)
+      end
+    end
+
+    context "when server_url is not empty" do
+      context "when server_url is an invalid URI" do
+        it "raises an exception" do
+          Chef::Config[:data_collector][:server_url] = "this is not a URI"
+          expect { reporter.send(:validate_data_collector_server_url!) }.to raise_error(Chef::Exceptions::ConfigurationError)
+        end
+      end
+
+      context "when server_url is a valid URI" do
+        context "when server_url is a URI with no host" do
+          it "raises an exception" do
+            Chef::Config[:data_collector][:server_url] = "/file/uri.txt"
+            expect { reporter.send(:validate_data_collector_server_url!) }.to raise_error(Chef::Exceptions::ConfigurationError)
+          end
+
+        end
+
+        context "when server_url is a URI with a valid host" do
+          it "does not an exception" do
+            Chef::Config[:data_collector][:server_url] = "http://www.google.com/data-collector"
+            expect { reporter.send(:validate_data_collector_server_url!) }.not_to raise_error
+          end
+        end
+      end
+    end
+  end
 end