Data collector multiple fixes

- Fix invalid output_locations raise start_time error.
- Improve validate_output_locations!

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

6 files changed, 222 insertions, 125 deletions

diff --git a/lib/chef/data_collector.rb b/lib/chef/data_collector.rb
index 6c7b2edb56..c9f3f53d1f 100644
--- a/lib/chef/data_collector.rb
+++ b/lib/chef/data_collector.rb
@@ -75,6 +75,8 @@ class Chef
       # (see EventDispatch::Base#run_start)
       #
       def run_start(chef_version, run_status)
+        Chef::DataCollector::ConfigValidation.validate_server_url!
+        Chef::DataCollector::ConfigValidation.validate_output_locations!
         events.unregister(self) unless Chef::DataCollector::ConfigValidation.should_be_enabled?
         @run_status = run_status
       end
@@ -115,9 +117,6 @@ class Chef
       # (see EventDispatch::Base#run_started)
       #
       def run_started(run_status)
-        Chef::DataCollector::ConfigValidation.validate_server_url!
-        Chef::DataCollector::ConfigValidation.validate_output_locations!
-
         send_run_start
       end
 
@@ -210,10 +209,10 @@ class Chef
       # @param message [Hash] message to send
       #
       def send_to_output_locations(message)
-        return unless Chef::Config[:data_collector][:output_locations]
+        Chef::DataCollector::ConfigValidation.validate_output_locations!
 
         Chef::Config[:data_collector][:output_locations].each do |type, locations|
-          locations.each do |location|
+          Array(locations).each do |location|
             send_to_file_location(location, message) if type == :files
             send_to_http_location(location, message) if type == :urls
           end
@@ -226,7 +225,7 @@ class Chef
       # @param message [Hash] the message to render as JSON
       #
       def send_to_file_location(file_name, message)
-        File.open(file_name, "a") do |fh|
+        File.open(File.expand_path(file_name), "a") do |fh|
           fh.puts Chef::JSONCompat.to_json(message, validate_utf8: false)
         end
       end
diff --git a/lib/chef/data_collector/config_validation.rb b/lib/chef/data_collector/config_validation.rb
index 1a7940b0ae..5f8e7d3fe5 100644
--- a/lib/chef/data_collector/config_validation.rb
+++ b/lib/chef/data_collector/config_validation.rb
@@ -46,14 +46,14 @@ class Chef
           return unless output_locations
 
           # but deliberately setting an empty output_location we consider to be an error (XXX: but should we?)
-          if output_locations.empty?
+          unless valid_hash_with_keys?(output_locations, :urls, :files)
             raise Chef::Exceptions::ConfigurationError,
               "Chef::Config[:data_collector][:output_locations] is empty. Please supply an hash of valid URLs and / or local file paths."
           end
 
           # loop through all the types and locations and validate each one-by-one
           output_locations.each do |type, locations|
-            locations.each do |location|
+            Array(locations).each do |location|
               validate_url!(location) if type == :urls
               validate_file!(location) if type == :files
             end
@@ -105,7 +105,7 @@ class Chef
 
         # validate an output_location file
         def validate_file!(file)
-          open(file, "a") {}
+          File.open(File.expand_path(file), "a") {}
         rescue Errno::ENOENT
           raise Chef::Exceptions::ConfigurationError,
             "Chef::Config[:data_collector][:output_locations][:files] contains the location #{file}, which is a non existent file path."
@@ -125,6 +125,17 @@ class Chef
             "Chef::Config[:data_collector][:output_locations][:urls] contains the url #{url} which is not valid."
         end
 
+        # Validate a non-empty hash that includes either of keys of both.
+        #
+        # @param hash [Hash] the hash contains data collector output_locations.
+        # @param keys [Array] the multiple keys as arguments array.
+        # @return [Boolean] true if the hash contains either of keys or both.
+        #
+        def valid_hash_with_keys?(hash, *keys)
+          return false if hash.empty? || !hash.is_a?(Hash)
+
+          keys.any? { |k| hash.key?(k) }
+        end
       end
     end
   end
diff --git a/lib/chef/data_collector/run_end_message.rb b/lib/chef/data_collector/run_end_message.rb
index 6f9f90b323..1900effa26 100644
--- a/lib/chef/data_collector/run_end_message.rb
+++ b/lib/chef/data_collector/run_end_message.rb
@@ -60,8 +60,8 @@ class Chef
             "cookbooks" => ( node && node["cookbooks"] ) || {},
             "policy_name" => node&.policy_name,
             "policy_group" => node&.policy_group,
-            "start_time" => run_status.start_time.utc.iso8601,
-            "end_time" => run_status.end_time.utc.iso8601,
+            "start_time" => run_status&.start_time&.utc&.iso8601,
+            "end_time" => run_status&.end_time&.utc&.iso8601,
             "source" => solo_run? ? "chef_solo" : "chef_client",
             "status" => status,
             "total_resource_count" => all_action_records(action_collection).count,
diff --git a/lib/chef/data_collector/run_start_message.rb b/lib/chef/data_collector/run_start_message.rb
index e5023b83ed..20ac867ef1 100644
--- a/lib/chef/data_collector/run_start_message.rb
+++ b/lib/chef/data_collector/run_start_message.rb
@@ -51,7 +51,7 @@ class Chef
             "organization_name" => organization,
             "run_id" => run_status&.run_id,
             "source" => solo_run? ? "chef_solo" : "chef_client",
-            "start_time" => run_status.start_time.utc.iso8601,
+            "start_time" => run_status&.start_time&.utc&.iso8601,
           }
         end
       end
diff --git a/spec/unit/data_collector/config_validation_spec.rb b/spec/unit/data_collector/config_validation_spec.rb
new file mode 100644
index 0000000000..2165b65768
--- /dev/null
+++ b/spec/unit/data_collector/config_validation_spec.rb
@@ -0,0 +1,200 @@
+#
+# Copyright:: Copyright (c) Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "spec_helper"
+require "chef/data_collector/config_validation"
+
+describe Chef::DataCollector::ConfigValidation do
+  describe "#should_be_enabled?" do
+    shared_examples_for "a solo-like run" do
+      it "is disabled in solo-legacy without a data_collector url and token" do
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+      end
+
+      it "is disabled in solo-legacy with only a url" do
+        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+      end
+
+      it "is disabled in solo-legacy with only a token" do
+        Chef::Config[:data_collector][:token] = "admit_one"
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+      end
+
+      it "is enabled in solo-legacy with both a token and url" do
+        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+        Chef::Config[:data_collector][:token] = "no_cash_value"
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+      end
+
+      it "is enabled in solo-legacy with only an output location to a file" do
+        Chef::Config[:data_collector][:output_locations] = { files: [ "/always/be/counting/down" ] }
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+      end
+
+      it "is disabled in solo-legacy with only an output location to a uri" do
+        Chef::Config[:data_collector][:output_locations] = { urls: [ "https://esa.local/ariane5" ] }
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+      end
+
+      it "is enabled in solo-legacy with only an output location to a uri with a token" do
+        Chef::Config[:data_collector][:output_locations] = { urls: [ "https://esa.local/ariane5" ] }
+        Chef::Config[:data_collector][:token] = "good_for_one_fare"
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+      end
+
+      it "is enabled in solo-legacy when the mode is :solo" do
+        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+        Chef::Config[:data_collector][:token] = "non_redeemable"
+        Chef::Config[:data_collector][:mode] = :solo
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+      end
+
+      it "is enabled in solo-legacy when the mode is :both" do
+        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+        Chef::Config[:data_collector][:token] = "non_negotiable"
+        Chef::Config[:data_collector][:mode] = :both
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+      end
+
+      it "is disabled in solo-legacy when the mode is :client" do
+        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+        Chef::Config[:data_collector][:token] = "NYCTA"
+        Chef::Config[:data_collector][:mode] = :client
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+      end
+
+      it "is disabled in solo-legacy mode when the mode is :nonsense" do
+        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+        Chef::Config[:data_collector][:token] = "MTA"
+        Chef::Config[:data_collector][:mode] = :nonsense
+        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+      end
+    end
+
+    it "by default it is enabled" do
+      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+    end
+
+    it "is disabled in why-run" do
+      Chef::Config[:why_run] = true
+      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+    end
+
+    describe "a solo legacy run" do
+      before(:each) do
+        Chef::Config[:solo_legacy_mode] = true
+      end
+
+      it_behaves_like "a solo-like run"
+    end
+
+    describe "a local mode run" do
+      before(:each) do
+        Chef::Config[:local_mode] = true
+      end
+
+      it_behaves_like "a solo-like run"
+    end
+
+    it "is enabled in client mode when the mode is :both" do
+      Chef::Config[:data_collector][:mode] = :both
+      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+    end
+
+    it "is disabled in client mode when the mode is :solo" do
+      Chef::Config[:data_collector][:mode] = :solo
+      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+    end
+
+    it "is disabled in client mode when the mode is :nonsense" do
+      Chef::Config[:data_collector][:mode] = :nonsense
+      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
+    end
+
+    it "is still enabled if you set a token in client mode" do
+      Chef::Config[:data_collector][:token] =  "good_for_one_ride"
+      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
+    end
+  end
+
+  describe "validate_server_url!" do
+    it "with valid server url" do
+      Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+      expect(Chef::DataCollector::ConfigValidation.validate_server_url!).to be_nil
+    end
+
+    it "with invalid server URL" do
+      Chef::Config[:data_collector][:server_url] = "not valid URL"
+      expect { Chef::DataCollector::ConfigValidation.validate_server_url! }.to raise_error(Chef::Exceptions::ConfigurationError,
+         "Chef::Config[:data_collector][:server_url] (not valid URL) is not a valid URI.")
+    end
+
+    it "with invalid server URL without host" do
+      Chef::Config[:data_collector][:server_url] = "no-host"
+      expect { Chef::DataCollector::ConfigValidation.validate_server_url! }.to raise_error(Chef::Exceptions::ConfigurationError,
+         "Chef::Config[:data_collector][:server_url] (no-host) is a URI with no host. Please supply a valid URL.")
+    end
+
+    it "skip validation if output_locations is set" do
+      Chef::Config[:data_collector][:output_locations] = { files: ["https://www.esa.local/ariane5"] }
+      expect(Chef::DataCollector::ConfigValidation.validate_server_url!).to be_nil
+    end
+
+    it "skip validation if output_locations & server_url both are set" do
+      Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
+      Chef::Config[:data_collector][:output_locations] = { files: ["https://www.esa.local/ariane5"] }
+      expect(Chef::DataCollector::ConfigValidation.validate_server_url!).to be_nil
+    end
+  end
+
+  describe "validate_output_locations!" do
+    it "with nil or not set skip validation" do
+      Chef::Config[:data_collector][:output_locations] = nil
+      expect(Chef::DataCollector::ConfigValidation.validate_output_locations!).to be_nil
+    end
+
+    it "with empty value raise validation error" do
+      Chef::Config[:data_collector][:output_locations] = {}
+      expect { Chef::DataCollector::ConfigValidation.validate_output_locations! }.to raise_error(Chef::Exceptions::ConfigurationError,
+        "Chef::Config[:data_collector][:output_locations] is empty. Please supply an hash of valid URLs and / or local file paths.")
+    end
+
+    it "with valid URLs options" do
+      Chef::Config[:data_collector][:output_locations] = { urls: ["https://www.esa.local/ariane5/data-collector"] }
+      expect { Chef::DataCollector::ConfigValidation.validate_output_locations! }.not_to raise_error
+    end
+
+    it "with valid files options" do
+      allow(File).to receive(:open).and_return(true)
+      Chef::Config[:data_collector][:output_locations] = { files: ["/tmp/client-runs.txt"] }
+      expect { Chef::DataCollector::ConfigValidation.validate_output_locations! }.not_to raise_error
+    end
+
+    it "with valid files & URLs options" do
+      allow(File).to receive(:open).and_return(true)
+      Chef::Config[:data_collector][:output_locations] = { urls: ["https://www.esa.local/ariane5/data-collector"], files: ["/tmp/client-runs.txt"] }
+      expect { Chef::DataCollector::ConfigValidation.validate_output_locations! }.not_to raise_error
+    end
+
+    it "with valid files options & String location value" do
+      allow(File).to receive(:open).and_return(true)
+      Chef::Config[:data_collector][:output_locations] = { files: "/tmp/client-runs.txt" }
+      expect { Chef::DataCollector::ConfigValidation.validate_output_locations! }.not_to raise_error
+    end
+  end
+end
\ No newline at end of file
diff --git a/spec/unit/data_collector_spec.rb b/spec/unit/data_collector_spec.rb
index bcce058d81..6d9d5e2379 100644
--- a/spec/unit/data_collector_spec.rb
+++ b/spec/unit/data_collector_spec.rb
@@ -296,119 +296,6 @@ describe Chef::DataCollector do
     end
   end
 
-  describe "#should_be_enabled?" do
-    shared_examples_for "a solo-like run" do
-      it "is disabled in solo-legacy without a data_collector url and token" do
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-      end
-
-      it "is disabled in solo-legacy with only a url" do
-        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-      end
-
-      it "is disabled in solo-legacy with only a token" do
-        Chef::Config[:data_collector][:token] = "admit_one"
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-      end
-
-      it "is enabled in solo-legacy with both a token and url" do
-        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
-        Chef::Config[:data_collector][:token] = "no_cash_value"
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-      end
-
-      it "is enabled in solo-legacy with only an output location to a file" do
-        Chef::Config[:data_collector][:output_locations] = { files: [ "/always/be/counting/down" ] }
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-      end
-
-      it "is disabled in solo-legacy with only an output location to a uri" do
-        Chef::Config[:data_collector][:output_locations] = { urls: [ "https://esa.local/ariane5" ] }
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-      end
-
-      it "is enabled in solo-legacy with only an output location to a uri with a token" do
-        Chef::Config[:data_collector][:output_locations] = { urls: [ "https://esa.local/ariane5" ] }
-        Chef::Config[:data_collector][:token] = "good_for_one_fare"
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-      end
-
-      it "is enabled in solo-legacy when the mode is :solo" do
-        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
-        Chef::Config[:data_collector][:token] = "non_redeemable"
-        Chef::Config[:data_collector][:mode] = :solo
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-      end
-
-      it "is enabled in solo-legacy when the mode is :both" do
-        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
-        Chef::Config[:data_collector][:token] = "non_negotiable"
-        Chef::Config[:data_collector][:mode] = :both
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-      end
-
-      it "is disabled in solo-legacy when the mode is :client" do
-        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
-        Chef::Config[:data_collector][:token] = "NYCTA"
-        Chef::Config[:data_collector][:mode] = :client
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-      end
-
-      it "is disabled in solo-legacy mode when the mode is :nonsense" do
-        Chef::Config[:data_collector][:server_url] = "https://www.esa.local/ariane5"
-        Chef::Config[:data_collector][:token] = "MTA"
-        Chef::Config[:data_collector][:mode] = :nonsense
-        expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-      end
-    end
-
-    it "by default it is enabled" do
-      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-    end
-
-    it "is disabled in why-run" do
-      Chef::Config[:why_run] = true
-      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-    end
-
-    describe "a solo legacy run" do
-      before(:each) do
-        Chef::Config[:solo_legacy_mode] = true
-      end
-
-      it_behaves_like "a solo-like run"
-    end
-
-    describe "a local mode run" do
-      before(:each) do
-        Chef::Config[:local_mode] = true
-      end
-
-      it_behaves_like "a solo-like run"
-    end
-
-    it "is enabled in client mode when the mode is :both" do
-      Chef::Config[:data_collector][:mode] = :both
-      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-    end
-
-    it "is disabled in client mode when the mode is :solo" do
-      Chef::Config[:data_collector][:mode] = :solo
-      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-    end
-
-    it "is disabled in client mode when the mode is :nonsense" do
-      Chef::Config[:data_collector][:mode] = :nonsense
-      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be false
-    end
-
-    it "is still enabled if you set a token in client mode" do
-      Chef::Config[:data_collector][:token] =  "good_for_one_ride"
-      expect(Chef::DataCollector::ConfigValidation.should_be_enabled?).to be true
-    end
-  end
-
   describe "when the run fails during node load" do
     let(:exception) { Exception.new("imperial to metric conversion error") }
     let(:error_description) { Chef::Formatters::ErrorMapper.registration_failed(node_name, exception, Chef::Config).for_json }