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>

5 files changed, 71 insertions, 22 deletions

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