From 598146da16425de86b42732f2e6a9078435bf5e7 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Mon, 15 Oct 2018 17:06:54 -0700 Subject: Node Attributes: Build ImmutableMash properly in deep_merge! closes #7738 The root cause here is that there's a difference between doing Chef::Node::VividMash.new( ) And: Chef::Node::ImmutableMash.new( ) 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 --- lib/chef/node/attribute.rb | 8 ++++---- spec/unit/node_spec.rb | 33 +++++++++++++++++++++++++++++++++ 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 -- cgit v1.2.1