summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSalim Afiune <afiune@chef.io>2017-03-23 11:35:48 -0400
committerSalim Afiune <afiune@chef.io>2017-03-24 09:46:44 -0400
commitf7372d9239f0a0e849c50b60ede208f3d4a4b96a (patch)
tree37f02b9b1692f9e463c45058530fe0d08f8b31fb
parentaccb4846106ca4601104a445da0d3544d4c1bf55 (diff)
downloadchef-f7372d9239f0a0e849c50b60ede208f3d4a4b96a.tar.gz
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>
-rw-r--r--lib/chef/data_collector/resource_report.rb36
-rw-r--r--spec/unit/data_collector/resource_report_spec.rb145
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