Node Attributes: Build ImmutableMash properly in deep_merge!lcg/deep-merge-cache-fix

closes #7738

The root cause here is that there's a difference between doing

Chef::Node::VividMash.new( <another VividMash> )

And:

Chef::Node::ImmutableMash.new( <a VividMash> )

The former short circuits and does no work in convert_value.  The
latter will not short circuit and does the proper work to dup
and convert the value argument.

Since we build an ImmutableMash now, we do not need to wrap it
with the extra immutablize() call.

This should be perf neutral or very, very slightly better perf.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

2 files changed, 37 insertions, 4 deletions

diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
index 46683d9405..05b625dd02 100644
--- a/lib/chef/node/attribute.rb
+++ b/lib/chef/node/attribute.rb
@@ -403,11 +403,11 @@ class Chef
       end
 
       def combined_override(*path)
-        immutablize(merge_overrides(path))
+        merge_overrides(path)
       end
 
       def combined_default(*path)
-        immutablize(merge_defaults(path))
+        merge_defaults(path)
       end
 
       def normal_unless(*args)
@@ -599,9 +599,9 @@ class Chef
         # In all other cases, replace merge_onto with merge_with
         else
           if merge_with.kind_of?(Hash)
-            Chef::Node::VividMash.new(merge_with)
+            Chef::Node::ImmutableMash.new(merge_with)
           elsif merge_with.kind_of?(Array)
-            Chef::Node::AttrArray.new(merge_with)
+            Chef::Node::ImmutableArray.new(merge_with)
           else
             merge_with
           end
diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb
index 0ca5f83adc..ff912ee7a6 100644
--- a/spec/unit/node_spec.rb
+++ b/spec/unit/node_spec.rb
@@ -1818,4 +1818,37 @@ describe Chef::Node do
       expect(node["a"]["key"]).to eql(1)
     end
   end
+
+  describe "when abusing the deep merge cache" do
+    # https://github.com/chef/chef/issues/7738
+    it "do not corrupt VividMashes that are part of the merge set and not the merge_onto set" do
+      # need to have a merge two-deep (not at the top-level) between at least two default (or two override)
+      # levels where the lowest priority one is the one that is going to be corrupted
+      node.default["foo"]["bar"]["baz"] = "fizz"
+      node.env_default["foo"]["bar"]["quux"] = "buzz"
+      node.default["foo"]["bar"].tap do |bar|
+        bar["test"] = "wrong"
+        # this triggers a deep merge
+        node["foo"]["bar"]["test"]
+        # this should correctly write and dirty the cache so the next read does another deep merge on the correct __root__
+        bar["test"] = "right"
+      end
+      expect(node["foo"]["bar"]["test"]).to eql("right")
+    end
+
+    it "do not corrupt VividMashes that are part of the merge set and not the merge_onto set (when priorities are reversed)" do
+      # need to have a merge two-deep (not at the top-level) between at least two default (or two override)
+      # levels where the *HIGHEST* priority one is the one that is going to be corrupted
+      node.env_default["foo"]["bar"]["baz"] = "fizz"
+      node.default["foo"]["bar"]["quux"] = "buzz"
+      node.env_default["foo"]["bar"].tap do |bar|
+        bar["test"] = "wrong"
+        # this triggers a deep merge
+        node["foo"]["bar"]["test"]
+        # this should correctly write and dirty the cache so the next read does another deep merge on the correct __root__
+        bar["test"] = "right"
+      end
+      expect(node["foo"]["bar"]["test"]).to eql("right")
+    end
+  end
 end