Move most compliance validation to pre-run

Because it is important that when possible, the compliance run
get associated with the converge (via run-id) this PR updates
compliance mode to pre-validate for most common issues before the
converge occurs.

This is a change of behavior, in that previously we would
wait until it was time to send the report after the converge before
validating, which would cause the report to not get captured for current
converge (run-id) if config errors were present.

This also adds error numbers to each of the failure conditions we
detect, in order to simplify providing the operator help with resolving
the errors.

Resolves #11106 and #11105

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

14 files changed, 165 insertions, 66 deletions

diff --git a/cspell.json b/cspell.json
index a54071a0db..0ee63efc67 100644
--- a/cspell.json
+++ b/cspell.json
@@ -235,6 +235,7 @@
     "cmds",
     "CMDS",
     "Cmnd",
+    "CMPL",
     "cname",
     "codepage",
     "coderanger",
diff --git a/lib/chef/compliance/default_attributes.rb b/lib/chef/compliance/default_attributes.rb
index a1ffff440f..f214135d59 100644
--- a/lib/chef/compliance/default_attributes.rb
+++ b/lib/chef/compliance/default_attributes.rb
@@ -38,11 +38,12 @@ class Chef
       # Allow for connections to HTTPS endpoints using self-signed ssl certificates.
       "insecure" => nil,
 
-      # Controls verbosity of Chef InSpec runner.
+      # Controls verbosity of Chef InSpec runner. See less output when true.
       "quiet" => true,
 
       # Chef Inspec Compliance profiles to be used for scan of node.
       # See README.md for details
+      # MPTD - ^there is no README.md anymore^
       "profiles" => {},
 
       # Extra inputs passed to Chef InSpec to allow finer-grained control over behavior.
diff --git a/lib/chef/compliance/fetcher/automate.rb b/lib/chef/compliance/fetcher/automate.rb
index 64aff6833a..47dc7f2e6a 100644
--- a/lib/chef/compliance/fetcher/automate.rb
+++ b/lib/chef/compliance/fetcher/automate.rb
@@ -46,13 +46,6 @@ class Chef
 
             config["token"] = Chef::Config[:data_collector][:token]
 
-            if config["token"].nil?
-              raise Inspec::FetcherFailure,
-                "No data-collector token set, which is required by the chef-automate fetcher. " \
-                "Set the `data_collector.token` configuration parameter in your client.rb " \
-                'or use the "chef-server-automate" reporter which does not require any ' \
-                "data-collector settings and uses #{ChefUtils::Dist::Server::PRODUCT} to fetch profiles."
-            end
           end
 
           new(profile_fetch_url, config)
diff --git a/lib/chef/compliance/reporter/automate.rb b/lib/chef/compliance/reporter/automate.rb
index cae0256085..23514c857d 100644
--- a/lib/chef/compliance/reporter/automate.rb
+++ b/lib/chef/compliance/reporter/automate.rb
@@ -28,18 +28,28 @@ class Chef
           @token = Chef::Config[:data_collector][:token]
         end
 
-        # Method used in order to send the inspec report to the data_collector server
-        def send_report(report)
-          unless @entity_uuid && @run_id
-            Chef::Log.error "entity_uuid(#{@entity_uuid}) or run_id(#{@run_id}) can't be nil, not sending report to #{ChefUtils::Dist::Automate::PRODUCT}"
-            return false
+        def validate_config!
+          unless @entity_uuid
+            # TODO - this is a weird leakage of naming from the parent class
+            # but entity_uuid is never an attribute that the user can see;
+            # it is sourced from chef_guid, which we don't technically know about in this class -
+            # but telling the operator about a missing chef_guid is more helpful than telling
+            # them about a missing field they've never heard of. Better would be a dock link
+            # that described how to fix this situation.
+            raise "CMPL004: automate_reporter: chef_guid is not available and must be provided. Aborting because we cannot report the scan."
+          end
+
+          unless @run_id
+            raise "CMPL005: automate_reporter: run_id is not available, aborting because we cannot report the scan."
           end
 
           unless @url && @token
-            Chef::Log.warn "data_collector.token and data_collector.server_url must be defined in client.rb! Further information: https://docs.chef.io/chef_compliance_phase/#direct-reporting-to-chef-automate"
-            return false
+            raise "CMPL006: data_collector.token and data_collector.server_url must be configured in client.rb! Further information: https://docs.chef.io/chef_compliance_phase/#direct-reporting-to-chef-automate"
           end
+        end
 
+        # Method used in order to send the inspec report to the data_collector server
+        def send_report(report)
           headers = {
             "Content-Type" => "application/json",
             "x-data-collector-auth" => "version=1.0",
diff --git a/lib/chef/compliance/reporter/chef_server_automate.rb b/lib/chef/compliance/reporter/chef_server_automate.rb
index 46ca7dc7ba..569fb22426 100644
--- a/lib/chef/compliance/reporter/chef_server_automate.rb
+++ b/lib/chef/compliance/reporter/chef_server_automate.rb
@@ -30,11 +30,6 @@ class Chef
         end
 
         def send_report(report)
-          unless @entity_uuid && @run_id
-            Chef::Log.error "entity_uuid(#{@entity_uuid}) or run_id(#{@run_id}) can't be nil, not sending report to #{ChefUtils::Dist::Automate::PRODUCT}"
-            return false
-          end
-
           automate_report = truncate_controls_results(enriched_report(report), @control_results_limit)
 
           report_size = Chef::JSONCompat.to_json(automate_report, validate_utf8: false).bytesize
@@ -51,6 +46,16 @@ class Chef
           false
         end
 
+        def validate_config!
+          unless @entity_uuid
+            raise "CMPL007: chef_server_automate reporter: chef_guid is not available and must be provided. Aborting because we cannot report the scan"
+          end
+
+          unless @run_id
+            raise "CMPL008: chef_server_automate reporter: run_id is not available, aborting because we cannot report the scan."
+          end
+        end
+
         def http_client
           config = if @insecure
                      Chef::Config.merge(ssl_verify_mode: :verify_none)
@@ -80,7 +85,7 @@ class Chef
           when /404/
             Chef::Log.error "Object does not exist on remote server."
           when /413/
-            Chef::Log.error "You most likely hit the erchef request size in #{ChefUtils::Dist::Server::PRODUCT} that defaults to ~2MB. To increase this limit see the Compliance Phase troubleshooting documentation (http://docs.chef.io/chef_compliance_phase/#troubleshooting) or the Chef Infra Server configuration documentation (https://docs.chef.io/server/config_rb_server/)"
+            Chef::Log.error "You most likely hit the request size limit in #{ChefUtils::Dist::Server::PRODUCT} that defaults to ~2MB. To increase this limit see the Compliance Phase troubleshooting documentation (http://docs.chef.io/chef_compliance_phase/#troubleshooting) or the Chef Infra Server configuration documentation (https://docs.chef.io/server/config_rb_server/)"
           when /429/
             Chef::Log.error "This error typically means the data sent was larger than #{ChefUtils::Dist::Automate::PRODUCT}'s limit (4 MB). Run InSpec locally to identify any controls producing large diffs."
           end
diff --git a/lib/chef/compliance/reporter/cli.rb b/lib/chef/compliance/reporter/cli.rb
index 4905c1f653..7061e5751c 100644
--- a/lib/chef/compliance/reporter/cli.rb
+++ b/lib/chef/compliance/reporter/cli.rb
@@ -22,6 +22,10 @@ class Chef
           puts output.join("\n")
         end
 
+        def validate_config!
+          true
+        end
+
         private
 
         # pastel.decorate is a lightweight replacement for highline.color
diff --git a/lib/chef/compliance/reporter/compliance_enforcer.rb b/lib/chef/compliance/reporter/compliance_enforcer.rb
index 1c63e43b28..47b3a4d2df 100644
--- a/lib/chef/compliance/reporter/compliance_enforcer.rb
+++ b/lib/chef/compliance/reporter/compliance_enforcer.rb
@@ -14,6 +14,10 @@ class Chef
           end
           true
         end
+
+        def validate_config!
+          true
+        end
       end
     end
   end
diff --git a/lib/chef/compliance/reporter/json_file.rb b/lib/chef/compliance/reporter/json_file.rb
index 471d9f64b1..4d074242ca 100644
--- a/lib/chef/compliance/reporter/json_file.rb
+++ b/lib/chef/compliance/reporter/json_file.rb
@@ -13,6 +13,12 @@ class Chef
 
           File.write(@path, Chef::JSONCompat.to_json(report))
         end
+
+        def validate_config!
+          if @path.nil? || @path.class != String || @path.empty?
+            raise "CMPL007: json_file reporter: node['audit']['json_file']['location'] must contain a file path"
+          end
+        end
       end
     end
   end
diff --git a/lib/chef/compliance/runner.rb b/lib/chef/compliance/runner.rb
index c083109875..7aebeeab36 100644
--- a/lib/chef/compliance/runner.rb
+++ b/lib/chef/compliance/runner.rb
@@ -1,17 +1,15 @@
 autoload :Inspec, "inspec"
 
 require_relative "default_attributes"
-require_relative "reporter/automate"
-require_relative "reporter/chef_server_automate"
-require_relative "reporter/compliance_enforcer"
-require_relative "reporter/cli"
-require_relative "reporter/json_file"
 
 class Chef
   module Compliance
     class Runner < EventDispatch::Base
       extend Forwardable
 
+      SUPPORTED_REPORTERS = %w{chef-automate chef-server-automate json-file audit-enforcer cli}.freeze
+      SUPPORTED_FETCHERS = %w{chef-automate chef-server}.freeze
+
       attr_accessor :run_id
       attr_reader :node
       def_delegators :node, :logger
@@ -47,6 +45,15 @@ class Chef
         self.run_id = run_status.run_id
       end
 
+      def converge_start(run_context)
+        # With all attributes - including cookook - loaded, we now have enough data to validate
+        # configuration.  Because the converge is best coupled with the associated compliance run, these validations
+        # will raise (and abort the converge) if the compliance phase configuration is incorrect/will
+        # prevent compliance phase from completing and submitting its report to all configured reporters.
+        # can abort the converge if the compliance phase configuration (node attributes and client config)
+        load_and_validate!
+      end
+
       def run_completed(_node, _run_status)
         return unless enabled?
 
@@ -85,6 +92,8 @@ class Chef
       end
 
       def report(report = generate_report)
+        # This is invoked at report-time instead of with the normal validations at node loaded,
+        # because we want to ensure that it is visible in the output - and not lost in back-scroll.
         warn_for_deprecated_config_values!
 
         if report.empty?
@@ -92,8 +101,9 @@ class Chef
           return
         end
 
-        Array(node["audit"]["reporter"]).each do |reporter|
-          send_report(reporter, report)
+        Array(node["audit"]["reporter"]).each do |reporter_type|
+          logger.info "Reporting to #{reporter_type}"
+          @reporters[reporter_type].send_report(report)
         end
       end
 
@@ -119,10 +129,8 @@ class Chef
 
       def inspec_profiles
         profiles = node["audit"]["profiles"]
-
-        # TODO: Custom exception class here?
         unless profiles.respond_to?(:map) && profiles.all? { |_, p| p.respond_to?(:transform_keys) && p.respond_to?(:update) }
-          raise "#{Inspec::Dist::PRODUCT_NAME} profiles specified in an unrecognized format, expected a hash of hashes."
+          raise "CMPL010: #{Inspec::Dist::PRODUCT_NAME} profiles specified in an unrecognized format, expected a hash of hashes."
         end
 
         profiles.map do |name, profile|
@@ -138,8 +146,6 @@ class Chef
           require_relative "fetcher/chef_server"
         when nil
           # intentionally blank
-        else
-          raise "Invalid value specified for Compliance Phase's fetcher: '#{node["audit"]["fetcher"]}'. Valid values are 'chef-automate', 'chef-server', or nil."
         end
       end
 
@@ -212,17 +218,10 @@ class Chef
         }
       end
 
-      def send_report(reporter_type, report)
-        logger.info "Reporting to #{reporter_type}"
-
-        reporter = reporter(reporter_type)
-
-        reporter.send_report(report) if reporter
-      end
-
       def reporter(reporter_type)
-        case reporter_type
+        case reporter_type.downcase
         when "chef-automate"
+          require_relative "reporter/automate"
           opts = {
             control_results_limit: node["audit"]["control_results_limit"],
             entity_uuid: node["chef_guid"],
@@ -233,6 +232,7 @@ class Chef
           }
           Chef::Compliance::Reporter::Automate.new(opts)
         when "chef-server-automate"
+          require_relative "reporter/chef_server_automate"
           opts = {
             control_results_limit: node["audit"]["control_results_limit"],
             entity_uuid: node["chef_guid"],
@@ -244,15 +244,16 @@ class Chef
           }
           Chef::Compliance::Reporter::ChefServerAutomate.new(opts)
         when "json-file"
+          require_relative "reporter/json_file"
           path = node["audit"]["json_file"]["location"]
           logger.info "Writing compliance report to #{path}"
           Chef::Compliance::Reporter::JsonFile.new(file: path)
         when "audit-enforcer"
+          require_relative "reporter/compliance_enforcer"
           Chef::Compliance::Reporter::ComplianceEnforcer.new
         when "cli"
+          require_relative "reporter/cli"
           Chef::Compliance::Reporter::Cli.new
-        else
-          raise "'#{reporter_type}' is not a supported reporter for Compliance Phase."
         end
       end
 
@@ -269,6 +270,31 @@ class Chef
         url.path = File.join(url.path, "organizations/#{org}/data-collector")
         url
       end
+
+      # Load the resources required for this runner, and validate configuration
+      # is correct to proceed. Requires node state to be loaded.
+      # Will raise exception if fetcher is not valid, if a reporter is not valid,
+      # or the configuration required by a reporter is not provided.
+      def load_and_validate!
+        return unless enabled?
+
+        @reporters = {}
+        Array(node["audit"]["reporter"]).each do |reporter_type|
+          type = reporter_type.downcase
+          unless SUPPORTED_REPORTERS.include? type
+            raise "CMPL003: '#{reporter_type}' found in node['audit']['reporter'] is not a supported reporter for Compliance Phase. Supported reporters are: #{SUPPORTED_REPORTERS.join(",")}. For more information, see the documentation at https://docs.chef.io/chef_compliance_phase/chef_compliance_runners/#reporters"
+          end
+
+          @reporters[type] = reporter(type)
+          @reporters[type].validate_config!
+        end
+
+        unless (fetcher = node["audit"]["fetcher"]).nil?
+          unless SUPPORTED_FETCHERS.include? fetcher.downcase
+            raise "CMPL002: Unrecognized Compliance Phase fetcher (node['audit']['fetcher'] is #{fetcher}. Supported fetchers are: or #{SUPPORTED_FETCHERS.join(",")}, or nil. For more information, see the documentation at https://docs.chef.io/chef_compliance_phase/chef_compliance_runners/#fetchers"
+          end
+        end
+      end
     end
   end
 end
diff --git a/spec/unit/compliance/fetcher/automate_spec.rb b/spec/unit/compliance/fetcher/automate_spec.rb
index f3554b8b0f..a4cd0c76c3 100644
--- a/spec/unit/compliance/fetcher/automate_spec.rb
+++ b/spec/unit/compliance/fetcher/automate_spec.rb
@@ -29,14 +29,6 @@ describe Chef::Compliance::Fetcher::Automate do
         expect(res.target).to eq(expected)
       end
 
-      it "raises an exception with no data collector token" do
-        Chef::Config[:data_collector].delete(:token)
-
-        expect {
-          Chef::Compliance::Fetcher::Automate.resolve("compliance://namespace/profile_name")
-        }.to raise_error(/No data-collector token set/)
-      end
-
       it "includes the data collector token" do
         expect(Chef::Compliance::Fetcher::Automate).to receive(:new).with(
           "https://automate.test/compliance/profiles/namespace/profile_name/tar",
@@ -108,14 +100,6 @@ describe Chef::Compliance::Fetcher::Automate do
         expect(res.target).to eq(expected)
       end
 
-      it "raises an exception with no data collector token" do
-        Chef::Config[:data_collector].delete(:token)
-
-        expect {
-          Chef::Compliance::Fetcher::Automate.resolve(compliance: "namespace/profile_name")
-        }.to raise_error(Inspec::FetcherFailure, /No data-collector token set/)
-      end
-
       it "includes the data collector token" do
         expect(Chef::Compliance::Fetcher::Automate).to receive(:new).with(
           "https://automate.test/compliance/profiles/namespace/profile_name/tar",
diff --git a/spec/unit/compliance/reporter/automate_spec.rb b/spec/unit/compliance/reporter/automate_spec.rb
index e0a33892b0..60d630d32b 100644
--- a/spec/unit/compliance/reporter/automate_spec.rb
+++ b/spec/unit/compliance/reporter/automate_spec.rb
@@ -1,6 +1,7 @@
 require "spec_helper"
 require "json" # For .to_json
 
+require "chef/compliance/reporter/automate"
 describe Chef::Compliance::Reporter::Automate do
   let(:reporter) { Chef::Compliance::Reporter::Automate.new(opts) }
 
@@ -264,11 +265,34 @@ describe Chef::Compliance::Reporter::Automate do
       expect(metasearch_stub).to have_been_requested
       expect(report_stub).to have_been_requested
     end
+  end
 
-    it "does not send report when entity_uuid is missing" do
+  describe "#validate_config!" do
+    it "raises CMPL004 when entity_uuid is not present" do
       opts.delete(:entity_uuid)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL004/)
+    end
+
+    it "raises CMPL005 when run_id is not present" do
+      opts.delete(:run_id)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL005/)
+    end
+
+    it "raises CMPL006 when data collector URL is missing" do
+      Chef::Config[:data_collector] = { token: "not_nil", server_url: nil }
       reporter = Chef::Compliance::Reporter::Automate.new(opts)
-      expect(reporter.send_report(inspec_report)).to eq(false)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL006/)
+    end
+
+    it "raises CMPL006 when data collector token is missing" do
+      Chef::Config[:data_collector] = { token: nil, server_url: "not_nil" }
+      reporter = Chef::Compliance::Reporter::Automate.new(opts)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL006/)
+    end
+
+    it "otherwise passes" do
+      Chef::Config[:data_collector] = { token: "not_nil", server_url: "not_nil" }
+      reporter.validate_config!
     end
   end
 
diff --git a/spec/unit/compliance/reporter/chef_server_automate_spec.rb b/spec/unit/compliance/reporter/chef_server_automate_spec.rb
index e45a7157ee..33642dea31 100644
--- a/spec/unit/compliance/reporter/chef_server_automate_spec.rb
+++ b/spec/unit/compliance/reporter/chef_server_automate_spec.rb
@@ -1,7 +1,9 @@
 require "spec_helper"
+require "chef/compliance/reporter/chef_server_automate"
 
 describe Chef::Compliance::Reporter::ChefServerAutomate do
   before do
+    # Isn't this already done globally in
     WebMock.disable_net_connect!
 
     Chef::Config[:client_key] = File.expand_path("../../../data/ssl/private_key.pem", __dir__)
@@ -174,4 +176,22 @@ describe Chef::Compliance::Reporter::ChefServerAutomate do
 
     expect(report_stub).to have_been_requested
   end
+
+  describe "#validate_config!" do
+    it "raises CMPL007 when entity_uuid is not present" do
+      opts.delete(:entity_uuid)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL007/)
+    end
+
+    it "raises CMPL008 when run_id is not present" do
+      opts.delete(:run_id)
+      expect { reporter.validate_config! }.to raise_error(/^CMPL008/)
+    end
+
+    it "otherwise passes" do
+      reporter.validate_config!
+    end
+
+  end
+
 end
diff --git a/spec/unit/compliance/reporter/compliance_enforcer_spec.rb b/spec/unit/compliance/reporter/compliance_enforcer_spec.rb
index ae63cf0853..3f3ce6286b 100644
--- a/spec/unit/compliance/reporter/compliance_enforcer_spec.rb
+++ b/spec/unit/compliance/reporter/compliance_enforcer_spec.rb
@@ -1,4 +1,5 @@
 require "spec_helper"
+require "chef/compliance/reporter/compliance_enforcer"
 
 describe Chef::Compliance::Reporter::AuditEnforcer do
   let(:reporter) { Chef::Compliance::Reporter::AuditEnforcer.new }
diff --git a/spec/unit/compliance/runner_spec.rb b/spec/unit/compliance/runner_spec.rb
index c100029a2c..3948970137 100644
--- a/spec/unit/compliance/runner_spec.rb
+++ b/spec/unit/compliance/runner_spec.rb
@@ -130,7 +130,7 @@ describe Chef::Compliance::Runner do
       expect(runner.inspec_profiles).to eq(expected)
     end
 
-    it "raises an error when the profiles are in the old audit-cookbook format" do
+    it "raises a CMPL010 message when the profiles are in the old audit-cookbook format" do
       node.normal["audit"]["profiles"] = [
         {
           name: "Windows 2019 Baseline",
@@ -138,7 +138,7 @@ describe Chef::Compliance::Runner do
         },
       ]
 
-      expect { runner.inspec_profiles }.to raise_error(/profiles specified in an unrecognized format, expected a hash of hashes./)
+      expect { runner.inspec_profiles }.to raise_error(/CMPL010:/)
     end
   end
 
@@ -186,9 +186,29 @@ describe Chef::Compliance::Runner do
       end
     end
 
-    it "fails with unexpected reporter value" do
-      expect { runner.reporter("tacos") }.to raise_error(/'tacos' is not a supported reporter for Compliance Phase/)
+  end
+
+  describe "#load_and_validate! when compliance is enabled" do
+    before do
+      allow(runner).to receive(:enabled?).and_return(true)
+    end
+
+    it "raises CMPL003 when the reporter is not a supported reporter type" do
+      node.normal["audit"]["reporter"] = [ "invalid" ]
+      expect { runner.load_and_validate! }.to raise_error(/^CMPL003:/)
     end
+    it "raises CMPL002 if the configured fetcher is not supported" do
+      node.normal["audit"]["fetcher"] = "invalid"
+      expect { runner.load_and_validate! }.to raise_error(/^CMPL002:/)
+    end
+
+    it "validates configured reporters" do
+      node.normal["audit"]["reporter"] = [ "chef-automate" ]
+      reporter_double = double("reporter", validate_config!: nil)
+      expect(runner).to receive(:reporter).with("chef-automate").and_return(reporter_double)
+      runner.load_and_validate!
+    end
+
   end
 
   describe "#inspec_opts" do