From 1e70e7912bcc23a53673a69bf411b607d4085eff Mon Sep 17 00:00:00 2001 From: Adam Leff Date: Fri, 1 Jul 2016 15:48:51 -0400 Subject: 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. --- lib/chef/data_collector.rb | 19 +++++++++++++- lib/chef/data_collector/resource_report.rb | 1 + spec/unit/data_collector_spec.rb | 42 +++++++++++++++++++++++++++++- 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 -- cgit v1.2.1 From 9cafaa5c68ac987cb7498eef0a413cb8b823b650 Mon Sep 17 00:00:00 2001 From: Adam Leff Date: Tue, 5 Jul 2016 14:20:20 -0400 Subject: Updates to end-user error messages based on PR feedback --- lib/chef/data_collector.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb index ffdba99351..dbb0b3771a 100644 --- a/lib/chef/data_collector.rb +++ b/lib/chef/data_collector.rb @@ -382,16 +382,16 @@ class Chef 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? + "Chef::Config[:data_collector][:server_url] is 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." + raise Chef::Exceptions::ConfigurationError, "Chef::Config[: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? + "Chef::Config[:data_collector][:server_url] (#{data_collector_server_url}) is a URI with no host. Please supply a valid URL." if uri.host.nil? end end end -- cgit v1.2.1