Make ResourceReporter smarter to get resource identity and state

Lets make the ResourceReporter smarter, we will try to get the identity
and state (after and before) of the resource but if we can't because of
an Exception, lets sends a message to the data collector containing a
static resource identity since we were unable to generate a proper one.

The problem is coming from the ResourceReporter, when it is trying to
report back the resource identity and state, it is using basic methods
from the Base class that should be secure to call from eny subscriber.

This is actually not a bug since Chef is doing the right thing, that is
to throw an error when the identity property is nil (or is not the right
kind that we configured for) so, rather than making identity properties
smarter for lazy evaluation we are going to make the ResourceReporter a
bit smarter so that it doesn't explode when the use configured a
resource with a wrong identity property.

Closes https://github.com/chef/chef/issues/5490

Signed-off-by: Salim Afiune <afiune@chef.io>

2 files changed, 177 insertions, 4 deletions

diff --git a/lib/chef/data_collector/resource_report.rb b/lib/chef/data_collector/resource_report.rb
index dcaf9c8e44..01651d5460 100644
--- a/lib/chef/data_collector/resource_report.rb
+++ b/lib/chef/data_collector/resource_report.rb
@@ -18,6 +18,8 @@
 # limitations under the License.
 #
 
+require "chef/exceptions"
+
 class Chef
   class DataCollector
     class ResourceReport
@@ -67,9 +69,9 @@ class Chef
         hash = {
           "type"           => new_resource.resource_name.to_sym,
           "name"           => new_resource.name.to_s,
-          "id"             => new_resource.identity.to_s,
-          "after"          => new_resource.state_for_resource_reporter,
-          "before"         => current_resource ? current_resource.state_for_resource_reporter : {},
+          "id"             => resource_identity,
+          "after"          => new_resource_state_reporter,
+          "before"         => current_resource_state_reporter,
           "duration"       => elapsed_time_in_milliseconds.to_s,
           "delta"          => new_resource.respond_to?(:diff) && potentially_changed? ? new_resource.diff : "",
           "ignore_failure" => new_resource.ignore_failure,
@@ -83,13 +85,39 @@ class Chef
           hash["recipe_name"]      = new_resource.recipe_name
         end
 
-        hash["conditional"]   = conditional.to_text if status == "skipped"
+        hash["conditional"] = conditional.to_text if status == "skipped"
         hash["error_message"] = exception.message unless exception.nil?
 
         hash
       end
       alias :to_h :to_hash
       alias :for_json :to_hash
+
+      # We should be able to call the identity of a resource safely, but there
+      # is an edge case where resources that have a lazy property that is both
+      # the name_property and the identity property, it will thow a validation
+      # exception causing the chef-client run to fail. We are not fixing this
+      # case since Chef is actually doing the right thing but we are making the
+      # ResourceReporter smarter so that it detects the failure and sends a
+      # message to the data collector containing a static resource identity
+      # since we were unable to generate a proper one.
+      def resource_identity
+        new_resource.identity.to_s
+      rescue => e
+        "unknown identity (due to #{e.class})"
+      end
+
+      def new_resource_state_reporter
+        new_resource.state_for_resource_reporter
+      rescue
+        {}
+      end
+
+      def current_resource_state_reporter
+        current_resource ? current_resource.state_for_resource_reporter : {}
+      rescue
+        {}
+      end
     end
   end
 end
diff --git a/spec/unit/data_collector/resource_report_spec.rb b/spec/unit/data_collector/resource_report_spec.rb
new file mode 100644
index 0000000000..b3523622c4
--- /dev/null
+++ b/spec/unit/data_collector/resource_report_spec.rb
@@ -0,0 +1,145 @@
+#
+# Author:: Salim Afiune (<afiune@chef.io)
+#
+# Copyright:: Copyright 2012-2017, 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"
+
+describe Chef::DataCollector::ResourceReport do
+  let(:cookbook_repo_path) { File.join(CHEF_SPEC_DATA, "cookbooks") }
+  let(:cookbook_collection) { Chef::CookbookCollection.new(Chef::CookbookLoader.new(cookbook_repo_path)) }
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, cookbook_collection, events) }
+  let(:resource) { Chef::Resource.new("zelda", run_context) }
+  let(:report) { described_class.new(resource, :create) }
+
+  describe "#skipped" do
+    let(:conditional) { double("Chef::Resource::Conditional") }
+
+    it "should set status and conditional" do
+      report.skipped(conditional)
+      expect(report.conditional).to eq conditional
+      expect(report.status).to eq "skipped"
+    end
+  end
+
+  describe "#up_to_date" do
+    it "should set status" do
+      report.up_to_date
+      expect(report.status).to eq "up-to-date"
+    end
+  end
+
+  describe "#updated" do
+    it "should set status" do
+      report.updated
+      expect(report.status).to eq "updated"
+    end
+  end
+
+  describe "#elapsed_time_in_milliseconds" do
+
+    context "when elapsed_time is not set" do
+      it "should return nil" do
+        allow(report).to receive(:elapsed_time).and_return(nil)
+        expect(report.elapsed_time_in_milliseconds).to eq nil
+      end
+    end
+
+    context "when elapsed_time is set" do
+      it "should return it in milliseconds" do
+        allow(report).to receive(:elapsed_time).and_return(1)
+        expect(report.elapsed_time_in_milliseconds).to eq 1000
+      end
+    end
+  end
+
+  describe "#failed" do
+    let(:exception) { double("Chef::Exception::Test") }
+
+    it "should set exception and status" do
+      report.failed(exception)
+      expect(report.exception).to eq exception
+      expect(report.status).to eq "failed"
+    end
+  end
+
+  describe "#to_hash" do
+    context "for a simple_resource" do
+      let(:resource) do
+        klass = Class.new(Chef::Resource) do
+          resource_name "zelda"
+        end
+        klass.new("hyrule", run_context)
+      end
+      let(:hash) do
+        {
+          "after" => {},
+          "before" => {},
+          "delta" => "",
+          "duration" => "",
+          "id" => "hyrule",
+          "ignore_failure" => false,
+          "name" => "hyrule",
+          "result" => "create",
+          "status" => "unprocessed",
+          "type" => :zelda,
+        }
+      end
+
+      it "returns a hash containing the expected values" do
+        expect(report.to_hash).to eq hash
+      end
+    end
+
+    context "for a lazy_resource that got skipped" do
+      let(:resource) do
+        klass = Class.new(Chef::Resource) do
+          resource_name "link"
+          property :sword, String, name_property: true, identity: true
+        end
+        resource = klass.new("hyrule")
+        resource.sword = Chef::DelayedEvaluator.new { nil }
+        resource
+      end
+      let(:hash) do
+        {
+          "after" => {},
+          "before" => {},
+          "delta" => "",
+          "duration" => "",
+          "conditional" => "because",
+          "id" => "unknown identity (due to Chef::Exceptions::ValidationFailed)",
+          "ignore_failure" => false,
+          "name" => "hyrule",
+          "result" => "create",
+          "status" => "skipped",
+          "type" => :link,
+        }
+      end
+      let(:conditional) do
+        double("Chef::Resource::Conditional", :to_text => "because")
+      end
+
+      it "should handle any Exception and throw a helpful message by mocking the identity" do
+        report.skipped(conditional)
+        expect(report.to_hash).to eq hash
+      end
+    end
+  end
+end