diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2018-10-15 17:06:54 -0700 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2018-10-15 17:06:54 -0700 |
commit | 598146da16425de86b42732f2e6a9078435bf5e7 (patch) | |
tree | b6dfb2ffba2994bb8786d5e9838ed5cc3b0bde91 | |
parent | 0c8f73047924ef49dc7b7bccdfb3325b15219d19 (diff) | |
download | chef-598146da16425de86b42732f2e6a9078435bf5e7.tar.gz |
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>
-rw-r--r-- | lib/chef/node/attribute.rb | 8 | ||||
-rw-r--r-- | 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 |