add tweakable cloning behaviorlcg/cloning-behavior

6 files changed, 164 insertions, 13 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 301a3ba0b6..3b392bd108 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -720,6 +720,14 @@ module ChefConfig
     # break Chef community cookbooks and is very highly discouraged.
     default :ruby_encoding, Encoding::UTF_8
 
+    # Default resource cloning behavior when an identically named resource is found on the resource collection.
+    #  :clone  - CHEF-3694-style resource cloning behavior
+    #  :rewind - chef-rewind style behavior where the prior resource is updated in-place in the resource collection
+    #  :none   - no cloning.
+    #
+    # Setting the default to :rewind is not recommended.
+    default :default_resource_cloning_behavior, :clone
+
     # If installed via an omnibus installer, this gives the path to the
     # "embedded" directory which contains all of the software packaged with
     # omnibus. This is used to locate the cacert.pem file on windows.
diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index 3c86f52b4a..3aad0c9f80 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -381,13 +381,25 @@ class Chef
       end
     end
 
+    #
     # Resource reporters send event information back to the chef server for
-    # processing.  Can only be called after we have a @rest object
+    # processing
+    #
     # @api private
+    #
     def register_reporters
+      # Audit and Resource Reporters set validate_utf8 to false so that they
+      # never fail if we accidentally send binary data through (validate_utf8
+      # will raise when set to true, and sanitizes to utf8 when false)
+      reporter_rest = Chef::REST.new(
+        config[:chef_server_url],
+        client_name,
+        config[:client_key],
+        validate_utf8: false,
+      )
       [
-        Chef::ResourceReporter.new(rest),
-        Chef::Audit::AuditReporter.new(rest)
+        Chef::ResourceReporter.new(reporter_rest),
+        Chef::Audit::AuditReporter.new(reporter_rest)
       ].each do |r|
         events.register(r)
       end
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index ac6f5e8923..f0a7751743 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -127,6 +127,7 @@ class Chef
       @default_guard_interpreter = :default
       @elapsed_time = 0
       @sensitive = false
+      @cloning_behavior = Chef::Config[:default_resource_cloning_behavior]
     end
 
     #
@@ -174,6 +175,21 @@ class Chef
       end
     end
 
+    #
+    # The kind of cloning behavior on the resource.  Supported values are:
+    #
+    #  :clone  - old CHEF-3694 style resource cloning
+    #  :rewind - overwrite the resource properties of the prior resource directly
+    #  :none   - no cloning at all
+    #
+    def cloning_behavior(arg=nil)
+      set_or_return(
+        :cloning_behavior,
+        arg,
+        kind_of: Symbol,
+      )
+    end
+
     # Alias for normal assigment syntax.
     alias_method :action=, :action
 
diff --git a/lib/chef/resource_builder.rb b/lib/chef/resource_builder.rb
index 9e9f7047a4..b1deedb00d 100644
--- a/lib/chef/resource_builder.rb
+++ b/lib/chef/resource_builder.rb
@@ -49,23 +49,30 @@ class Chef
     def build(&block)
       raise ArgumentError, "You must supply a name when declaring a #{type} resource" if name.nil?
 
-      @resource = resource_class.new(name, run_context)
-      if resource.resource_name.nil?
-        raise Chef::Exceptions::InvalidResourceSpecification, "#{resource}.resource_name is `nil`!  Did you forget to put `provides :blah` or `resource_name :blah` in your resource class?"
+      if prior_resource && prior_resource.cloning_behavior == :rewind
+        @resource = prior_resource
+      else
+        @resource = resource_class.new(name, run_context)
+        if resource.resource_name.nil?
+          raise Chef::Exceptions::InvalidResourceSpecification, "#{resource}.resource_name is `nil`!  Did you forget to put `provides :blah` or `resource_name :blah` in your resource class?"
+        end
+        resource.source_line = created_at
+        resource.declared_type = type
       end
-      resource.source_line = created_at
-      resource.declared_type = type
 
       # If we have a resource like this one, we want to steal its state
       # This behavior is very counter-intuitive and should be removed.
       # See CHEF-3694, https://tickets.opscode.com/browse/CHEF-3694
       # Moved to this location to resolve CHEF-5052, https://tickets.opscode.com/browse/CHEF-5052
-      if prior_resource
+      if prior_resource && prior_resource.cloning_behavior == :clone
         resource.load_from(prior_resource)
       end
 
-      resource.cookbook_name = cookbook_name
-      resource.recipe_name = recipe_name
+      unless prior_resource && prior_resource.cloning_behavior == :rewind
+        resource.cookbook_name = cookbook_name
+        resource.recipe_name = recipe_name
+      end
+
       # Determine whether this resource is being created in the context of an enclosing Provider
       resource.enclosing_provider = enclosing_provider
 
@@ -77,7 +84,7 @@ class Chef
       resource.instance_eval(&block) if block_given?
 
       # emit a cloned resource warning if it is warranted
-      if prior_resource
+      if prior_resource && prior_resource.cloning_behavior == :clone
         if is_trivial_resource?(prior_resource) && identicalish_resources?(prior_resource, resource)
           emit_harmless_cloning_debug
         else
diff --git a/spec/unit/recipe_spec.rb b/spec/unit/recipe_spec.rb
index 511e7e9397..f60cb9c7de 100644
--- a/spec/unit/recipe_spec.rb
+++ b/spec/unit/recipe_spec.rb
@@ -404,6 +404,10 @@ describe Chef::Recipe do
         end
       end
 
+      before do
+        duplicated_resource # evaluate the lazy resources in the correct order
+      end
+
       it "copies attributes from the first resource" do
         expect(duplicated_resource.something).to eq("bvb09")
       end
@@ -431,6 +435,109 @@ describe Chef::Recipe do
 
     end
 
+    describe "resource cloning with :rewind" do
+      let(:second_recipe) do
+        Chef::Recipe.new("second_cb", "second_recipe", run_context)
+      end
+
+      let(:original_resource) do
+        recipe.zen_master("klopp") do
+          something "bvb09"
+          cloning_behavior :rewind
+          action :score
+        end
+      end
+
+      let(:duplicated_resource) do
+        original_resource
+        second_recipe.zen_master("klopp") do
+          something "else"
+        end
+      end
+
+      before do
+        duplicated_resource # evaluate the lazy resources in the correct order
+      end
+
+      it "the second resource points at the first resource" do
+        expect(duplicated_resource).to eq(original_resource)
+      end
+
+      it "the first resources property is updated" do
+        expect(duplicated_resource.something).to eq("else")
+        expect(original_resource.something).to eq("else")
+      end
+
+      it "the cookbook_name should come from the first resource" do
+        expect(original_resource.cookbook_name).to eq("hjk")
+        expect(original_resource.cookbook_name).to eq(duplicated_resource.cookbook_name)
+      end
+
+      it "the recipe_name should come from the first resource" do
+        expect(original_resource.recipe_name).to eq("test")
+        expect(original_resource.recipe_name).to eq(duplicated_resource.recipe_name)
+      end
+
+      it "the source_line should make sense" do
+        # hard to test if this is from the first or second resource
+        expect(original_resource.source_line).to include(__FILE__)
+        expect(original_resource.source_line).to eq(duplicated_resource.source_line)
+      end
+    end
+
+    describe "resource cloning with :none" do
+      let(:second_recipe) do
+        Chef::Recipe.new("second_cb", "second_recipe", run_context)
+      end
+
+      let(:original_resource) do
+        recipe.zen_master("klopp") do
+          something "bvb09"
+          cloning_behavior :none
+          action :score
+        end
+      end
+
+      let(:duplicated_resource) do
+        original_resource
+        second_recipe.zen_master("klopp") do
+        end
+      end
+
+      before do
+        duplicated_resource # evaluate the lazy resources in the correct order
+      end
+
+      it "the second resource is different from the first resource" do
+        expect(duplicated_resource).not_to eq(original_resource)
+      end
+
+      it "does not copy attributes from the first resource" do
+        expect(duplicated_resource.something).to eq(nil)
+      end
+
+      it "does not copy the action from the first resource" do
+        expect(original_resource.action).to eq([:score])
+        expect(duplicated_resource.action).to eq([:nothing])
+      end
+
+      it "does not copy the source location of the first resource" do
+        # sanity check source location:
+        expect(original_resource.source_line).to include(__FILE__)
+        expect(duplicated_resource.source_line).to include(__FILE__)
+        # actual test:
+        expect(original_resource.source_line).not_to eq(duplicated_resource.source_line)
+      end
+
+      it "sets the cookbook name on the cloned resource to that resource's cookbook" do
+        expect(duplicated_resource.cookbook_name).to eq("second_cb")
+      end
+
+      it "sets the recipe name on the cloned resource to that resoure's recipe" do
+        expect(duplicated_resource.recipe_name).to eq("second_recipe")
+      end
+    end
+
     describe "resource definitions" do
       it "should execute defined resources" do
         crow_define = Chef::ResourceDefinition.new
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index b9ba80068b..ec6f45ba60 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -443,7 +443,8 @@ describe Chef::Resource do
         :updated_by_last_action, :before, :supports,
         :noop, :ignore_failure, :name, :source_line,
         :action, :retries, :retry_delay, :elapsed_time,
-        :default_guard_interpreter, :guard_interpreter, :sensitive ]
+        :default_guard_interpreter, :guard_interpreter, :sensitive,
+        :cloning_behavior ]
       expect(hash.keys - expected_keys).to eq([])
       expect(expected_keys - hash.keys).to eq([])
       expect(hash[:name]).to eql("funk")