Chef: Chef-client removes array items in --local-mode with chef-zero

[simonvik]

Description

We found a possible bug in chef-client when we define a json-attribute as an array with two identical strings Like:

{"onearray":["a","a"]}

At the first run chef-client does the correct thing and writes two "a" but at some point it seems to convert that array into a hash and upload one "a" to chef-zero

The 2nd run results in that it removes that 2nd "a"

Chef Version

Chef-client 12.17.44, 11.14.2

Platform Version

Linux, tested on both debian wheezy and arch-linux

Replication Case

cd /tmp
wget http://devsn.se/testcase.tar.gz
tar xf testcase.tar.gz
# First run
/opt/chef/bin/chef-client --local-mode --config /tmp/kitchen/client.rb --log_level auto --force-formatter --no-color --json-attributes /tmp/kitchen/dna.json --chef-zero-port 8889
# Second run
/opt/chef/bin/chef-client --local-mode --config /tmp/kitchen/client.rb --log_level auto --force-formatter --no-color --json-attributes /tmp/kitchen/dna.json --chef-zero-port 8889

Client Output

First run:

+# This file is generated by Chef for arraytest1-wheezy
+
+a
+a

Second run:

# This file is generated by Chef for arraytest1-wheezy

a
-a

[simonvik]

testcase.tar.gz Added test-case

[lamont-granquist]

it saves it correctly to the nodes/ directory with the array with two values. i suspect that on the second run its doing array deep merge between the json attributes and the normal attributes in the node json and getting confused.

[lamont-granquist]

yeah deep merge algorithm is broken:

[1] pry(main)> require 'chef/mixin/deep_merge'
=> true
[2] pry(main)> include Chef::Mixin::DeepMerge
=> Object
[5] pry(main)> require 'chef/mash'
=> true
[7] pry(main)> merge({ array: ['a', 'a'] } , { array: ['a', 'a'] })
=> {"array"=>["a"]}

[lamont-granquist]

https://coderanger.net/arrays-and-chef/

offhand IDK if this get fixed via https://github.com/chef/chef-rfc/pull/238 or if it needs to be fixed before that RFC can proceed.

[lamont-granquist]

dropped in accepted major so its on my radar for Chef 13

[lamont-granquist]

This is still buggy in deep-merge, but I think I removed the use of deep merge in the actual way that node attributes are being merged as part of all kinds of refactoring that got done since chef 12 so this may not be an applicable bug any more.

[lamont-granquist]

Whatever it is this is still a bug in Chef-15-latest:

% cat dna.json
{
  "recipes": ["test::test"],
  "this_thing": [ "a", "a" ]
}

% chef-client -z  -j dna.json  -c client.rb
Starting Chef Client, version 14.13.11
resolving cookbooks for run list: ["test::test"]
Synchronizing Cookbooks:
  - test (0.0.1)
Installing Cookbook Gems:
Compiling Cookbooks...
[["default", :not_present],
 ["env_default", :not_present],
 ["role_default", :not_present],
 ["force_default", :not_present],
 ["normal", ["a", "a"]],
 ["override", :not_present],
 ["role_override", :not_present],
 ["env_override", :not_present],
 ["force_override", :not_present],
 ["automatic", :not_present]]
Converging 1 resources
Recipe: test::test
  * test_test[test] action run
    * ruby_block[test] action run
      - execute the ruby block test


Running handlers:
Running handlers complete
Chef Client finished, 2/2 resources updated in 03 seconds
git: rvm:ruby-2.6.1
15:21 coredump ~/zero % chef-client -z  -j dna.json  -c client.rb
Starting Chef Client, version 14.13.11
resolving cookbooks for run list: ["test::test"]
Synchronizing Cookbooks:
  - test (0.0.1)
Installing Cookbook Gems:
Compiling Cookbooks...
[["default", :not_present],
 ["env_default", :not_present],
 ["role_default", :not_present],
 ["force_default", :not_present],
 ["normal", ["a"]],
 ["override", :not_present],
 ["role_override", :not_present],
 ["env_override", :not_present],
 ["force_override", :not_present],
 ["automatic", :not_present]]
Converging 1 resources
Recipe: test::test
  * test_test[test] action run
    * ruby_block[test] action run
      - execute the ruby block test


Running handlers:
Running handlers complete
Chef Client finished, 2/2 resources updated in 03 seconds

[lamont-granquist]

This is actually correct behavior for deep_merge otherwise arrays would grow without bound.

On every execution of the client we need to take the value we saved and deep_merge it with the -j arguments. If we use += to merge the two arrays, then on every execution the old array gets added to the new array. So as a result it has to use |=.

The purpose is so that you can add a port to a firewall ruleset specified as an array or something like that.

But [ 80, 443 ] merged with [ 80, 443 ] is supposed to result in [ 80, 443 ] and not [ 80, 443, 80, 443 ] by design.

Without a compelling use case in here -- that is substantially MORE compelling that this firewall port number kind of case I think I'm closing this as working as designed.

We could simply not do array merging from the -j attributes and switch to using hash_only_merge instead of deep_merge, which would fix this issue, but if anyone out there is relying on the merging of port numbers above then they'll be broken hard, so that's a major breaking change, and would really need to be justified by something better than a simple example. I don't see the compelling use case here, even though it feels buggy on the surface, and it would certainly hurt people to change the behavior so I think this needs to simply be closed as operating as designed.